Message ID | Zvu-5MwUWrN7sIA8@SIT-SDELAP2361.int.lidl.net |
---|---|
State | New |
Headers | show |
Series | [ovs-dev,v2] python: Index references. | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
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
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
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