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