diff mbox series

[ovs-dev] python: Index references.

Message ID ZvHTCvBUDxHpHWKq@SIT-SDELAP2361.int.lidl.net
State Changes Requested
Delegated to: Simon Horman
Headers show
Series [ovs-dev] python: Index references. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test fail github build: failed
ovsrobot/github-robot-_Build_and_Test fail github build: failed

Commit Message

Luca Czesla Sept. 23, 2024, 8:45 p.m. UTC 
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 | 12 +++++++++---
 python/ovs/db/idl.py          |  9 ++++++---
 2 files changed, 15 insertions(+), 6 deletions(-)


base-commit: 1b99649025ae767974d0a7cea88d1e9f46f4378d

Comments

Eelco Chaudron Sept. 24, 2024, 7:59 a.m. UTC | #1 
On 23 Sep 2024, at 22:45, 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>

Recheck-request: github-robot

> ---
>  python/ovs/db/custom_index.py | 12 +++++++++---
>  python/ovs/db/idl.py          |  9 ++++++---
>  2 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/python/ovs/db/custom_index.py b/python/ovs/db/custom_index.py
> index 3fa03d3c9..2e10f9505 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,7 @@ 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, _uuid_to_uuid) for c in self.columns})
>
>      def add(self, row):
>          if not all(hasattr(row, col.column) for col in self.columns):
> @@ -131,6 +131,12 @@ class IndexedRows(DictBase, object):
>          raise NotImplementedError()
>
>
> +# 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.
> +def _uuid_to_uuid(atom, base):
> +    return atom
> +
>  def IndexEntryClass(table):
>      """Create a class used represent Rows in indexes
>
> 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
Mike Pattrick Sept. 24, 2024, 7:38 p.m. UTC | #2 
On Mon, Sep 23, 2024 at 4:46 PM 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>
> ---
>  python/ovs/db/custom_index.py | 12 +++++++++---
>  python/ovs/db/idl.py          |  9 ++++++---
>  2 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/python/ovs/db/custom_index.py b/python/ovs/db/custom_index.py
> index 3fa03d3c9..2e10f9505 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,7 @@ 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, _uuid_to_uuid) for c in self.columns})

Hello Luca,

Thank you for the patch, all flake8 warnings must be addressed prior
to its acceptance. I've included them below.

python/ovs/db/custom_index.py:56:80: E501 line too long (88 > 79 characters)
python/ovs/db/custom_index.py:140:1: E302 expected 2 blank lines, found 1

Would it make sense to make the _uuid_to_uuid function a staticmethod
of the MultiColumnIndex class here? It doesn't seem to be used
externally.

Cheers,
M

>
>      def add(self, row):
>          if not all(hasattr(row, col.column) for col in self.columns):
> @@ -131,6 +131,12 @@ class IndexedRows(DictBase, object):
>          raise NotImplementedError()
>
>
> +# 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.
> +def _uuid_to_uuid(atom, base):
> +    return atom
> +
>  def IndexEntryClass(table):
>      """Create a class used represent Rows in indexes
>
> 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
Luca Czesla Oct. 1, 2024, 9:17 a.m. UTC | #3 
Hey Patrick,

thanks for your feedback.

Cheers,
Luca

> -----Original Message-----
> From: Mike Pattrick <mkp@redhat.com>
> Sent: Tuesday, September 24, 2024 9:38 PM
> To: Luca Czesla <Luca.Czesla@stackit.cloud>
> Cc: dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH] python: Index references.
> 
> On Mon, Sep 23, 2024 at 4:46 PM 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>
> > ---
> >  python/ovs/db/custom_index.py | 12 +++++++++---
> >  python/ovs/db/idl.py          |  9 ++++++---
> >  2 files changed, 15 insertions(+), 6 deletions(-)
> >
> > diff --git a/python/ovs/db/custom_index.py
> > b/python/ovs/db/custom_index.py index 3fa03d3c9..2e10f9505 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,7 @@
> > 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, _uuid_to_uuid) for
> > + c in self.columns})
> 
> Hello Luca,
> 
> Thank you for the patch, all flake8 warnings must be addressed prior to its
> acceptance. I've included them below.
> 
> python/ovs/db/custom_index.py:56:80: E501 line too long (88 > 79
> characters)
> python/ovs/db/custom_index.py:140:1: E302 expected 2 blank lines, found 1

Fixed. Thanks.

> 
> Would it make sense to make the _uuid_to_uuid function a staticmethod of
> the MultiColumnIndex class here? It doesn't seem to be used externally.

Makes sense. I'll change it.

> 
> Cheers,
> M
> 
> >
> >      def add(self, row):
> >          if not all(hasattr(row, col.column) for col in self.columns):
> > @@ -131,6 +131,12 @@ class IndexedRows(DictBase, object):
> >          raise NotImplementedError()
> >
> >
> > +# 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.
> > +def _uuid_to_uuid(atom, base):
> > +    return atom
> > +
> >  def IndexEntryClass(table):
> >      """Create a class used represent Rows in indexes
> >
> > 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://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail
> > .openvswitch.org%2Fmailman%2Flistinfo%2Fovs-
> dev&data=05%7C02%7C%7C817d
> >
> a2a92a7c4b6094d108dcdcd07c0b%7Cd04f47175a6e4b98b3f96918e0385f
> 4c%7C0%7C
> >
> 0%7C638628035202077071%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4
> wLjAwMDAiLCJ
> >
> QIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata
> =J39HtsR
> > I6SiavV%2B6U0E6HOYogz7gJosjF%2FXymSudObE%3D&reserved=0
diff mbox series

Patch

diff --git a/python/ovs/db/custom_index.py b/python/ovs/db/custom_index.py
index 3fa03d3c9..2e10f9505 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,7 @@  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, _uuid_to_uuid) for c in self.columns})
 
     def add(self, row):
         if not all(hasattr(row, col.column) for col in self.columns):
@@ -131,6 +131,12 @@  class IndexedRows(DictBase, object):
         raise NotImplementedError()
 
 
+# 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.
+def _uuid_to_uuid(atom, base):
+    return atom
+
 def IndexEntryClass(table):
     """Create a class used represent Rows in indexes
 
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