[ovs-dev,v2] python: Index references.

For an index on a reference to work, we should not resolve the reference to
to the corresponding row object but instead use the uuid for indexing.

an index on Datapath in the Port_Binding Table reduces the lookup in the
neutron metadata agent for each request from approx. 400ms per request to
980μs with approx. 40k port bindings.

Signed-off-by: Luca Czesla <Luca.Czesla@stackit.cloud>
---
 python/ovs/db/custom_index.py | 14 +++++++++++---
 python/ovs/db/idl.py          |  9 ++++++---
 2 files changed, 17 insertions(+), 6 deletions(-)


base-commit: 1b99649025ae767974d0a7cea88d1e9f46f4378d

On Tue, Oct 1, 2024 at 5:21 AM Luca Czesla via dev
<ovs-dev@openvswitch.org> wrote:
>
> For an index on a reference to work, we should not resolve the reference to
> to the corresponding row object but instead use the uuid for indexing.
>
> an index on Datapath in the Port_Binding Table reduces the lookup in the
> neutron metadata agent for each request from approx. 400ms per request to
> 980μs with approx. 40k port bindings.
>
> Signed-off-by: Luca Czesla <Luca.Czesla@stackit.cloud>

Thanks for the changes.

Acked-by: Mike Pattrick <mkp@redhat.com>

> ---
>  python/ovs/db/custom_index.py | 14 +++++++++++---
>  python/ovs/db/idl.py          |  9 ++++++---
>  2 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/python/ovs/db/custom_index.py b/python/ovs/db/custom_index.py
> index 3fa03d3c9..803baaccd 100644
> --- a/python/ovs/db/custom_index.py
> +++ b/python/ovs/db/custom_index.py
> @@ -11,7 +11,7 @@ try:
>  except ImportError:
>      from ovs.compat import sortedcontainers
>
> -from ovs.db import data
> +from ovs.db import data, idl
>
>  OVSDB_INDEX_ASC = "ASC"
>  OVSDB_INDEX_DESC = "DESC"
> @@ -43,7 +43,7 @@ class MultiColumnIndex(object):
>
>      def _cmp(self, a, b):
>          for col, direction, key in self.columns:
> -            aval, bval = key(a), key(b)
> +            aval, bval = idl._row_to_uuid(key(a)), idl._row_to_uuid(key(b))
>              if aval == bval:
>                  continue
>              result = (aval > bval) - (aval < bval)
> @@ -53,7 +53,8 @@ class MultiColumnIndex(object):
>      def index_entry_from_row(self, row):
>          return row._table.rows.IndexEntry(
>              uuid=row.uuid,
> -            **{c.column: getattr(row, c.column) for c in self.columns})
> +            **{c.column: row.get_column(c.column, self._uuid_to_uuid)
> +                for c in self.columns})
>
>      def add(self, row):
>          if not all(hasattr(row, col.column) for col in self.columns):
> @@ -76,6 +77,13 @@ class MultiColumnIndex(object):
>      def __iter__(self):
>          return iter(r._table.rows[r.uuid] for r in self.values)
>
> +    # For indexing we do not want to resolve the references as we cannot
> +    # ensure that the referenced table is already filled, the uuid is
> +    # sufficient for the index.
> +    @staticmethod
> +    def _uuid_to_uuid(atom, base):
> +        return atom
> +
>
>  class IndexedRows(DictBase, object):
>      def __init__(self, table, *args, **kwargs):
> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
> index c8cc54346..3feb068b8 100644
> --- a/python/ovs/db/idl.py
> +++ b/python/ovs/db/idl.py
> @@ -1351,7 +1351,7 @@ class Row(object):
>          else:
>              return atom
>
> -    def __getattr__(self, column_name):
> +    def get_column(self, column_name, uuid_to_row):
>          assert self._changes is not None
>          assert self._mutations is not None
>
> @@ -1392,7 +1392,7 @@ class Row(object):
>                      datum = data.Datum.from_python(column.type, dlist,
>                                                     _row_to_uuid)
>                  elif column.type.is_map():
> -                    dmap = datum.to_python(self._uuid_to_row)
> +                    dmap = datum.to_python(uuid_to_row)
>                      if inserts is not None:
>                          dmap.update(inserts)
>                      if removes is not None:
> @@ -1409,7 +1409,10 @@ class Row(object):
>                  else:
>                      datum = inserts
>
> -        return datum.to_python(self._uuid_to_row)
> +        return datum.to_python(uuid_to_row)
> +
> +    def __getattr__(self, column_name):
> +        return self.get_column(column_name, self._uuid_to_row)
>
>      def __setattr__(self, column_name, value):
>          assert self._changes is not None
>
> base-commit: 1b99649025ae767974d0a7cea88d1e9f46f4378d
> --
> 2.39.5
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On 1 Oct 2024, at 11:20, Luca Czesla via dev wrote:

> For an index on a reference to work, we should not resolve the reference to
> to the corresponding row object but instead use the uuid for indexing.
>
> an index on Datapath in the Port_Binding Table reduces the lookup in the
> neutron metadata agent for each request from approx. 400ms per request to
> 980μs with approx. 40k port bindings.
>
> Signed-off-by: Luca Czesla <Luca.Czesla@stackit.cloud>


Hi Luca,

The v2 looks good to me.

//Eelco

Acked-by: Eelco Chaudron <echaudro@redhat.com>

> ---
>  python/ovs/db/custom_index.py | 14 +++++++++++---
>  python/ovs/db/idl.py          |  9 ++++++---
>  2 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/python/ovs/db/custom_index.py b/python/ovs/db/custom_index.py
> index 3fa03d3c9..803baaccd 100644
> --- a/python/ovs/db/custom_index.py
> +++ b/python/ovs/db/custom_index.py
> @@ -11,7 +11,7 @@ try:
>  except ImportError:
>      from ovs.compat import sortedcontainers
>
> -from ovs.db import data
> +from ovs.db import data, idl
>
>  OVSDB_INDEX_ASC = "ASC"
>  OVSDB_INDEX_DESC = "DESC"
> @@ -43,7 +43,7 @@ class MultiColumnIndex(object):
>
>      def _cmp(self, a, b):
>          for col, direction, key in self.columns:
> -            aval, bval = key(a), key(b)
> +            aval, bval = idl._row_to_uuid(key(a)), idl._row_to_uuid(key(b))
>              if aval == bval:
>                  continue
>              result = (aval > bval) - (aval < bval)
> @@ -53,7 +53,8 @@ class MultiColumnIndex(object):
>      def index_entry_from_row(self, row):
>          return row._table.rows.IndexEntry(
>              uuid=row.uuid,
> -            **{c.column: getattr(row, c.column) for c in self.columns})
> +            **{c.column: row.get_column(c.column, self._uuid_to_uuid)
> +                for c in self.columns})
>
>      def add(self, row):
>          if not all(hasattr(row, col.column) for col in self.columns):
> @@ -76,6 +77,13 @@ class MultiColumnIndex(object):
>      def __iter__(self):
>          return iter(r._table.rows[r.uuid] for r in self.values)
>
> +    # For indexing we do not want to resolve the references as we cannot
> +    # ensure that the referenced table is already filled, the uuid is
> +    # sufficient for the index.
> +    @staticmethod
> +    def _uuid_to_uuid(atom, base):
> +        return atom
> +
>
>  class IndexedRows(DictBase, object):
>      def __init__(self, table, *args, **kwargs):
> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
> index c8cc54346..3feb068b8 100644
> --- a/python/ovs/db/idl.py
> +++ b/python/ovs/db/idl.py
> @@ -1351,7 +1351,7 @@ class Row(object):
>          else:
>              return atom
>
> -    def __getattr__(self, column_name):
> +    def get_column(self, column_name, uuid_to_row):
>          assert self._changes is not None
>          assert self._mutations is not None
>
> @@ -1392,7 +1392,7 @@ class Row(object):
>                      datum = data.Datum.from_python(column.type, dlist,
>                                                     _row_to_uuid)
>                  elif column.type.is_map():
> -                    dmap = datum.to_python(self._uuid_to_row)
> +                    dmap = datum.to_python(uuid_to_row)
>                      if inserts is not None:
>                          dmap.update(inserts)
>                      if removes is not None:
> @@ -1409,7 +1409,10 @@ class Row(object):
>                  else:
>                      datum = inserts
>
> -        return datum.to_python(self._uuid_to_row)
> +        return datum.to_python(uuid_to_row)
> +
> +    def __getattr__(self, column_name):
> +        return self.get_column(column_name, self._uuid_to_row)
>
>      def __setattr__(self, column_name, value):
>          assert self._changes is not None
>
> base-commit: 1b99649025ae767974d0a7cea88d1e9f46f4378d
> -- 
> 2.39.5
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On 4 Oct 2024, at 12:23, Eelco Chaudron wrote:

> On 1 Oct 2024, at 11:20, Luca Czesla via dev wrote:
>
>> For an index on a reference to work, we should not resolve the reference to
>> to the corresponding row object but instead use the uuid for indexing.
>>
>> an index on Datapath in the Port_Binding Table reduces the lookup in the
>> neutron metadata agent for each request from approx. 400ms per request to
>> 980μs with approx. 40k port bindings.
>>
>> Signed-off-by: Luca Czesla <Luca.Czesla@stackit.cloud>
>
>
> Hi Luca,
>
> The v2 looks good to me.
>
> //Eelco
>
> Acked-by: Eelco Chaudron <echaudro@redhat.com>


In addition, Terry would you be able to have a look before we apply the patch?

Also, we might need to adjust the title a bit, probably something like:

  python: Use index references in the MultiColumnIndex class.

Cheers,

Eelco

>> ---
>>  python/ovs/db/custom_index.py | 14 +++++++++++---
>>  python/ovs/db/idl.py          |  9 ++++++---
>>  2 files changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/python/ovs/db/custom_index.py b/python/ovs/db/custom_index.py
>> index 3fa03d3c9..803baaccd 100644
>> --- a/python/ovs/db/custom_index.py
>> +++ b/python/ovs/db/custom_index.py
>> @@ -11,7 +11,7 @@ try:
>>  except ImportError:
>>      from ovs.compat import sortedcontainers
>>
>> -from ovs.db import data
>> +from ovs.db import data, idl
>>
>>  OVSDB_INDEX_ASC = "ASC"
>>  OVSDB_INDEX_DESC = "DESC"
>> @@ -43,7 +43,7 @@ class MultiColumnIndex(object):
>>
>>      def _cmp(self, a, b):
>>          for col, direction, key in self.columns:
>> -            aval, bval = key(a), key(b)
>> +            aval, bval = idl._row_to_uuid(key(a)), idl._row_to_uuid(key(b))
>>              if aval == bval:
>>                  continue
>>              result = (aval > bval) - (aval < bval)
>> @@ -53,7 +53,8 @@ class MultiColumnIndex(object):
>>      def index_entry_from_row(self, row):
>>          return row._table.rows.IndexEntry(
>>              uuid=row.uuid,
>> -            **{c.column: getattr(row, c.column) for c in self.columns})
>> +            **{c.column: row.get_column(c.column, self._uuid_to_uuid)
>> +                for c in self.columns})
>>
>>      def add(self, row):
>>          if not all(hasattr(row, col.column) for col in self.columns):
>> @@ -76,6 +77,13 @@ class MultiColumnIndex(object):
>>      def __iter__(self):
>>          return iter(r._table.rows[r.uuid] for r in self.values)
>>
>> +    # For indexing we do not want to resolve the references as we cannot
>> +    # ensure that the referenced table is already filled, the uuid is
>> +    # sufficient for the index.
>> +    @staticmethod
>> +    def _uuid_to_uuid(atom, base):
>> +        return atom
>> +
>>
>>  class IndexedRows(DictBase, object):
>>      def __init__(self, table, *args, **kwargs):
>> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
>> index c8cc54346..3feb068b8 100644
>> --- a/python/ovs/db/idl.py
>> +++ b/python/ovs/db/idl.py
>> @@ -1351,7 +1351,7 @@ class Row(object):
>>          else:
>>              return atom
>>
>> -    def __getattr__(self, column_name):
>> +    def get_column(self, column_name, uuid_to_row):
>>          assert self._changes is not None
>>          assert self._mutations is not None
>>
>> @@ -1392,7 +1392,7 @@ class Row(object):
>>                      datum = data.Datum.from_python(column.type, dlist,
>>                                                     _row_to_uuid)
>>                  elif column.type.is_map():
>> -                    dmap = datum.to_python(self._uuid_to_row)
>> +                    dmap = datum.to_python(uuid_to_row)
>>                      if inserts is not None:
>>                          dmap.update(inserts)
>>                      if removes is not None:
>> @@ -1409,7 +1409,10 @@ class Row(object):
>>                  else:
>>                      datum = inserts
>>
>> -        return datum.to_python(self._uuid_to_row)
>> +        return datum.to_python(uuid_to_row)
>> +
>> +    def __getattr__(self, column_name):
>> +        return self.get_column(column_name, self._uuid_to_row)
>>
>>      def __setattr__(self, column_name, value):
>>          assert self._changes is not None
>>
>> base-commit: 1b99649025ae767974d0a7cea88d1e9f46f4378d
>> -- 
>> 2.39.5
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

I'll take a look today. Might take me a little bit as python-ovs and
the indexing code isn't always the easiest to review.

On Fri, Oct 4, 2024 at 5:35 AM Eelco Chaudron <echaudro@redhat.com> wrote:
>
>
>
> On 4 Oct 2024, at 12:23, Eelco Chaudron wrote:
>
> > On 1 Oct 2024, at 11:20, Luca Czesla via dev wrote:
> >
> >> For an index on a reference to work, we should not resolve the reference to
> >> to the corresponding row object but instead use the uuid for indexing.
> >>
> >> an index on Datapath in the Port_Binding Table reduces the lookup in the
> >> neutron metadata agent for each request from approx. 400ms per request to
> >> 980μs with approx. 40k port bindings.
> >>
> >> Signed-off-by: Luca Czesla <Luca.Czesla@stackit.cloud>
> >
> >
> > Hi Luca,
> >
> > The v2 looks good to me.
> >
> > //Eelco
> >
> > Acked-by: Eelco Chaudron <echaudro@redhat.com>
>
>
> In addition, Terry would you be able to have a look before we apply the patch?
>
> Also, we might need to adjust the title a bit, probably something like:
>
>   python: Use index references in the MultiColumnIndex class.
>
> Cheers,
>
> Eelco
>
> >> ---
> >>  python/ovs/db/custom_index.py | 14 +++++++++++---
> >>  python/ovs/db/idl.py          |  9 ++++++---
> >>  2 files changed, 17 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/python/ovs/db/custom_index.py b/python/ovs/db/custom_index.py
> >> index 3fa03d3c9..803baaccd 100644
> >> --- a/python/ovs/db/custom_index.py
> >> +++ b/python/ovs/db/custom_index.py
> >> @@ -11,7 +11,7 @@ try:
> >>  except ImportError:
> >>      from ovs.compat import sortedcontainers
> >>
> >> -from ovs.db import data
> >> +from ovs.db import data, idl
> >>
> >>  OVSDB_INDEX_ASC = "ASC"
> >>  OVSDB_INDEX_DESC = "DESC"
> >> @@ -43,7 +43,7 @@ class MultiColumnIndex(object):
> >>
> >>      def _cmp(self, a, b):
> >>          for col, direction, key in self.columns:
> >> -            aval, bval = key(a), key(b)
> >> +            aval, bval = idl._row_to_uuid(key(a)), idl._row_to_uuid(key(b))
> >>              if aval == bval:
> >>                  continue
> >>              result = (aval > bval) - (aval < bval)
> >> @@ -53,7 +53,8 @@ class MultiColumnIndex(object):
> >>      def index_entry_from_row(self, row):
> >>          return row._table.rows.IndexEntry(
> >>              uuid=row.uuid,
> >> -            **{c.column: getattr(row, c.column) for c in self.columns})
> >> +            **{c.column: row.get_column(c.column, self._uuid_to_uuid)
> >> +                for c in self.columns})
> >>
> >>      def add(self, row):
> >>          if not all(hasattr(row, col.column) for col in self.columns):
> >> @@ -76,6 +77,13 @@ class MultiColumnIndex(object):
> >>      def __iter__(self):
> >>          return iter(r._table.rows[r.uuid] for r in self.values)
> >>
> >> +    # For indexing we do not want to resolve the references as we cannot
> >> +    # ensure that the referenced table is already filled, the uuid is
> >> +    # sufficient for the index.
> >> +    @staticmethod
> >> +    def _uuid_to_uuid(atom, base):
> >> +        return atom
> >> +
> >>
> >>  class IndexedRows(DictBase, object):
> >>      def __init__(self, table, *args, **kwargs):
> >> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
> >> index c8cc54346..3feb068b8 100644
> >> --- a/python/ovs/db/idl.py
> >> +++ b/python/ovs/db/idl.py
> >> @@ -1351,7 +1351,7 @@ class Row(object):
> >>          else:
> >>              return atom
> >>
> >> -    def __getattr__(self, column_name):
> >> +    def get_column(self, column_name, uuid_to_row):
> >>          assert self._changes is not None
> >>          assert self._mutations is not None
> >>
> >> @@ -1392,7 +1392,7 @@ class Row(object):
> >>                      datum = data.Datum.from_python(column.type, dlist,
> >>                                                     _row_to_uuid)
> >>                  elif column.type.is_map():
> >> -                    dmap = datum.to_python(self._uuid_to_row)
> >> +                    dmap = datum.to_python(uuid_to_row)
> >>                      if inserts is not None:
> >>                          dmap.update(inserts)
> >>                      if removes is not None:
> >> @@ -1409,7 +1409,10 @@ class Row(object):
> >>                  else:
> >>                      datum = inserts
> >>
> >> -        return datum.to_python(self._uuid_to_row)
> >> +        return datum.to_python(uuid_to_row)
> >> +
> >> +    def __getattr__(self, column_name):
> >> +        return self.get_column(column_name, self._uuid_to_row)
> >>
> >>      def __setattr__(self, column_name, value):
> >>          assert self._changes is not None
> >>
> >> base-commit: 1b99649025ae767974d0a7cea88d1e9f46f4378d
> >> --
> >> 2.39.5
> >>
> >> _______________________________________________
> >> dev mailing list
> >> dev@openvswitch.org
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

Apologies, I sent this last week but failed at reply-to-all.
Definitely a needed fix, thanks!

On Tue, Oct 1, 2024 at 4:21 AM Luca Czesla via dev
<ovs-dev@openvswitch.org> wrote:
> @@ -53,7 +53,8 @@ class MultiColumnIndex(object):
>      def index_entry_from_row(self, row):
>          return row._table.rows.IndexEntry(
>              uuid=row.uuid,
> -            **{c.column: getattr(row, c.column) for c in self.columns})
> +            **{c.column: row.get_column(c.column, self._uuid_to_uuid)
> +                for c in self.columns})

Here, instead of doing the modification to ovs.db.idl code and adding
the _uuid_to_uuid method, can't we just do:

-            **{c.column: getattr(row, c.column) for c in self.columns})
+            **{c.column: idl._row_to_uuid(getattr(row, c.column))
+               for c in self.columns})

It seemed to work in my local tests. Also, I think we should add a
test case just to make sure this case doesn't get broken in the
future.

Terry