Hi Ilya, Thanks for the patch! LGTM+
works perfectly for the use case of inclusion/removal of ovn-ic transit-switches with active monitoring of the Datapath_Binding table via python-idl client (ovsdbapp). Hey @Terry Wilson <[email protected]>, since you were interested in ovn-ic, take a look at this to avoid problems with neutron-ovn-metadata-agent. Kind regards, Roberto Em ter., 28 de mai. de 2024 às 09:23, Vladislav Odintsov <[email protected]> escreveu: > Hi Ilya, > > Thanks for the fix! > Some time ago we internally noticed a problem with index updates, which > was not a real issue, but I tried your fix and it fixes our original issue. > > How we observed that issue: > > In terminal one: > ovn-nbctl ls-add test > > In terminal two: > Run python, load IDL and query the name for the created object (ls "test") > (we use ovsdbapp, so example uses it as well): > > >>> from ovsdbapp.backend.ovs_idl import connection, idlutils > >>> import ovsdbapp.schema.ovn_northbound.impl_idl as nb_idl > >>> > >>> idl = connection.OvsdbIdl.from_server("tcp:127.0.0.1:6641", > "OVN_Northbound") > >>> api_idl = nb_idl.OvnNbApiIdlImpl(connection.Connection(idl, 100)) > >>> > >>> sw = api_idl.ls_get("test").execute().name > >>> 'test' > > Than switch back to first terminal and change ls 'name' (which is an > indexed field): > > ovn-nbctl set logical-switch test name=test2 > > Switch back to python terminal and try to get the name again. > In case of affected python-ovs the old instance "test" returns new name > "test2" from "test" instance and "test2" instance is not accessible: > > >>> sw = api_idl.ls_get("test").execute().name > >>> 'test2' > >>> sw = api_idl.ls_get("test2").execute().name > Traceback (most recent call last): > File "<stdin>", line 1, in <module> > AttributeError: 'NoneType' object has no attribute 'name' > > With your patch it works as expected: > > >>> sw = api_idl.ls_get("test").execute().name > Traceback (most recent call last): > File "<stdin>", line 1, in <module> > AttributeError: 'NoneType' object has no attribute 'name' > >>> sw = api_idl.ls_get("test2").execute().name > >>> 'test2' > > I just wanted to share our experience with this problem and patch. > You can add this to OVS python tests, if you consider it's worth it. > > Thanks again :) > > regards, > > Vladislav Odintsov > > -----Original Message----- > From: dev <[email protected]> on behalf of Ilya Maximets < > [email protected]> > Date: Tuesday, 28 May 2024 at 00:39 > To: "[email protected]" <[email protected]> > Cc: Ilya Maximets <[email protected]>, Dumitru Ceara <[email protected]> > Subject: [ovs-dev] [PATCH] python: idl: Fix index not being updated on > row modification. > > When a row is modified, python IDL doesn't perform any operations on > existing client-side indexes. This means that if the column on which > index is created changes, the old value will remain in the index and > the new one will not be added to the index. Beside lookup failures > this is also causing inability to remove modified rows, because the > new column value doesn't exist in the index causing an exception on > attempt to remove it: > > Traceback (most recent call last): > File "ovsdbapp/backend/ovs_idl/connection.py", line 110, in run > self.idl.run() > File "ovs/db/idl.py", line 465, in run > self.__parse_update(msg.params[2], OVSDB_UPDATE3) > File "ovs/db/idl.py", line 924, in __parse_update > self.__do_parse_update(update, version, self.tables) > File "ovs/db/idl.py", line 964, in __do_parse_update > changes = self.__process_update2(table, uuid, row_update) > File "ovs/db/idl.py", line 991, in __process_update2 > del table.rows[uuid] > File "ovs/db/custom_index.py", line 102, in __delitem__ > index.remove(val) > File "ovs/db/custom_index.py", line 66, in remove > self.values.remove(self.index_entry_from_row(row)) > File "sortedcontainers/sortedlist.py", line 2015, in remove > raise ValueError('{0!r} not in list'.format(value)) > ValueError: Datapath_Binding( > uuid=UUID('498e66a2-70bc-4587-a66f-0433baf82f60'), > tunnel_key=16711683, load_balancers=[], external_ids={}) not in list > > Fix that by always removing an existing row from indexes before > modification and adding back afterwards. This ensures that old > values are removed from the index and new ones are added. > > This behavior is consistent with the C implementation. > > The new test that reproduces the removal issue is added. Some extra > testing infrastructure added to be able to handle and print out the > 'indexed' table from the idltest schema. > > Fixes: 13973bc41524 ("Add multi-column index support for the Python > IDL") > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-discuss/2024-May/053159.html > Reported-by: Roberto Bartzen Acosta <[email protected]> > Signed-off-by: Ilya Maximets <[email protected]> > --- > python/ovs/db/idl.py | 13 ++++-- > tests/ovsdb-idl.at | 95 > +++++++++++++++++++++++++++++++++++++++++++- > tests/test-ovsdb.c | 43 ++++++++++++++++++++ > tests/test-ovsdb.py | 15 +++++++ > 4 files changed, 160 insertions(+), 6 deletions(-) > > diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py > index a80da84e7..ae4cfe7e2 100644 > --- a/python/ovs/db/idl.py > +++ b/python/ovs/db/idl.py > @@ -1013,7 +1013,9 @@ class Idl(object): > if not row: > raise error.Error('Modify non-existing row') > > + del table.rows[uuid] > old_row = self.__apply_diff(table, row, > row_update['modify']) > + table.rows[uuid] = row > return Notice(ROW_UPDATE, row, Row(self, table, uuid, > old_row)) > else: > raise error.Error('<row-update> unknown operation', > @@ -1044,9 +1046,10 @@ class Idl(object): > op = ROW_UPDATE > vlog.warn("cannot add existing row %s to table %s" > % (uuid, table.name)) > + del table.rows[uuid] > + > changed |= self.__row_update(table, row, new) > - if op == ROW_CREATE: > - table.rows[uuid] = row > + table.rows[uuid] = row > if changed: > return Notice(ROW_CREATE, row) > else: > @@ -1058,9 +1061,11 @@ class Idl(object): > # XXX rate-limit > vlog.warn("cannot modify missing row %s in table %s" > % (uuid, table.name)) > + else: > + del table.rows[uuid] > + > changed |= self.__row_update(table, row, new) > - if op == ROW_CREATE: > - table.rows[uuid] = row > + table.rows[uuid] = row > if changed: > return Notice(op, row, Row.from_json(self, table, > uuid, old)) > return False > diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at > index c9e36d678..34f3902e0 100644 > --- a/tests/ovsdb-idl.at > +++ b/tests/ovsdb-idl.at > @@ -167,8 +167,17 @@ m4_define([OVSDB_CHECK_IDL_REGISTER_COLUMNS_PY], > OVSDB_START_IDLTEST > m4_if([$2], [], [], > [AT_CHECK([ovsdb-client transact unix:socket $2], [0], [ignore], > [ignore])]) > - AT_CHECK([$PYTHON3 $srcdir/test-ovsdb.py -t10 idl > $srcdir/idltest.ovsschema unix:socket > ?simple:b,ba,i,ia,r,ra,s,sa,u,ua?simple3:name,uset,uref?simple4:name?simple6:name,weak_ref?link1:i,k,ka,l2?link2:i,l1?singleton:name > $3], > - [0], [stdout], [ignore]) > + m4_define([REGISTER], m4_joinall([?], [], > + [simple:b,ba,i,ia,r,ra,s,sa,u,ua], > + [simple3:name,uset,uref], > + [simple4:name], > + [simple6:name,weak_ref], > + [link1:i,k,ka,l2], > + [link2:i,l1], > + [indexed:i], > + [singleton:name])) > + AT_CHECK([$PYTHON3 $srcdir/test-ovsdb.py -t10 idl > $srcdir/idltest.ovsschema \ > + unix:socket REGISTER $3], [0], [stdout], [ignore]) > AT_CHECK([sort stdout | uuidfilt]m4_if([$6],,, [[| $6]]), > [0], [$4]) > OVSDB_SERVER_SHUTDOWN > @@ -747,6 +756,31 @@ OVSDB_CHECK_IDL([simple idl, conditional, > multiple tables], > 009: done > ]]) > > +OVSDB_CHECK_IDL([indexed idl, modification and removal], > + [], > + [['["idltest", > + {"op": "insert", > + "table": "indexed", > + "row": {"i": 123 }}]' \ > + '["idltest", > + {"op": "update", > + "table": "indexed", > + "where": [["i", "==", 123]], > + "row": {"i": 456}}]' \ > + '["idltest", > + {"op": "delete", > + "table": "indexed", > + "where": [["i", "==", 456]]}]']], > + [[000: empty > +001: {"error":null,"result":[{"uuid":["uuid","<0>"]}]} > +002: table indexed: i=123 uuid=<0> > +003: {"error":null,"result":[{"count":1}]} > +004: table indexed: i=456 uuid=<0> > +005: {"error":null,"result":[{"count":1}]} > +006: empty > +007: done > +]]) > + > OVSDB_CHECK_IDL([self-linking idl, consistent ops], > [], > [['["idltest", > @@ -1274,6 +1308,33 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, > initially populated], > 003: done > ]]) > > +OVSDB_CHECK_IDL_TRACK([track, indexed idl, modification and removal], > + [], > + [['["idltest", > + {"op": "insert", > + "table": "indexed", > + "row": {"i": 123 }}]' \ > + '["idltest", > + {"op": "update", > + "table": "indexed", > + "where": [["i", "==", 123]], > + "row": {"i": 456}}]' \ > + '["idltest", > + {"op": "delete", > + "table": "indexed", > + "where": [["i", "==", 456]]}]']], > + [[000: empty > +001: {"error":null,"result":[{"uuid":["uuid","<0>"]}]} > +002: table indexed: inserted row: i=123 uuid=<0> > +002: table indexed: updated columns: i > +003: {"error":null,"result":[{"count":1}]} > +004: table indexed: i=456 uuid=<0> > +004: table indexed: updated columns: i > +005: {"error":null,"result":[{"count":1}]} > +006: empty > +007: done > +]]) > + > dnl This test creates database with weak references and checks that > orphan > dnl rows created for weak references are not available for iteration > via > dnl list of tracked changes. > @@ -2022,6 +2083,36 @@ OVSDB_CHECK_IDL_NOTIFY([simple idl verify > notify], > 015: done > ]]) > > +OVSDB_CHECK_IDL_NOTIFY([indexed idl, modification and removal notify], > + [['track-notify' \ > + '["idltest", > + {"op": "insert", > + "table": "indexed", > + "row": {"i": 123 }}]' \ > + '["idltest", > + {"op": "update", > + "table": "indexed", > + "where": [["i", "==", 123]], > + "row": {"i": 456}}]' \ > + '["idltest", > + {"op": "delete", > + "table": "indexed", > + "where": [["i", "==", 456]]}]']], > + [[000: empty > +000: event:create, row={}, uuid=<0>, updates=None > +000: event:create, row={}, uuid=<1>, updates=None > +001: {"error":null,"result":[{"uuid":["uuid","<2>"]}]} > +002: event:create, row={i=123}, uuid=<2>, updates=None > +002: table indexed: i=123 uuid=<2> > +003: {"error":null,"result":[{"count":1}]} > +004: event:update, row={i=456}, uuid=<2>, updates={i=123} > +004: table indexed: i=456 uuid=<2> > +005: {"error":null,"result":[{"count":1}]} > +006: empty > +006: event:delete, row={i=456}, uuid=<2>, updates=None > +007: done > +]]) > + > # Tests to verify the functionality of the one column compound index. > # It tests index for one column string and integer indexes. > # The run of test-ovsdb generates the output of the display of data > using the different indexes defined in > diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c > index c4ab899d4..41c1525f4 100644 > --- a/tests/test-ovsdb.c > +++ b/tests/test-ovsdb.c > @@ -2023,6 +2023,24 @@ print_idl_row_updated_link2(const struct > idltest_link2 *l2, int step) > } > } > > +static void > +print_idl_row_updated_indexed(const struct idltest_indexed *ind, int > step) > +{ > + struct ds updates = DS_EMPTY_INITIALIZER; > + > + for (size_t i = 0; i < IDLTEST_INDEXED_N_COLUMNS; i++) { > + if (idltest_indexed_is_updated(ind, i)) { > + ds_put_format(&updates, " %s", > idltest_indexed_columns[i].name); > + } > + } > + if (updates.length) { > + print_and_log("%03d: table %s: updated columns:%s", > + step, ind->header_.table->class_->name, > + ds_cstr(&updates)); > + ds_destroy(&updates); > + } > +} > + > static void > print_idl_row_updated_simple3(const struct idltest_simple3 *s3, int > step) > { > @@ -2172,6 +2190,21 @@ print_idl_row_link2(const struct idltest_link2 > *l2, int step, bool terse) > print_idl_row_updated_link2(l2, step); > } > > +static void > +print_idl_row_indexed(const struct idltest_indexed *ind, int step, > bool terse) > +{ > + struct ds msg = DS_EMPTY_INITIALIZER; > + > + ds_put_format(&msg, "i=%"PRId64, ind->i); > + > + char *row_msg = format_idl_row(&ind->header_, step, > ds_cstr(&msg), terse); > + print_and_log("%s", row_msg); > + ds_destroy(&msg); > + free(row_msg); > + > + print_idl_row_updated_indexed(ind, step); > +} > + > static void > print_idl_row_simple3(const struct idltest_simple3 *s3, int step, > bool terse) > { > @@ -2252,6 +2285,7 @@ print_idl_row_singleton(const struct > idltest_singleton *sng, int step, > static void > print_idl(struct ovsdb_idl *idl, int step, bool terse) > { > + const struct idltest_indexed *ind; > const struct idltest_simple3 *s3; > const struct idltest_simple4 *s4; > const struct idltest_simple6 *s6; > @@ -2285,6 +2319,10 @@ print_idl(struct ovsdb_idl *idl, int step, bool > terse) > print_idl_row_simple6(s6, step, terse); > n++; > } > + IDLTEST_INDEXED_FOR_EACH (ind, idl) { > + print_idl_row_indexed(ind, step, terse); > + n++; > + } > IDLTEST_SINGLETON_FOR_EACH (sng, idl) { > print_idl_row_singleton(sng, step, terse); > n++; > @@ -2297,6 +2335,7 @@ print_idl(struct ovsdb_idl *idl, int step, bool > terse) > static void > print_idl_track(struct ovsdb_idl *idl, int step, bool terse) > { > + const struct idltest_indexed *ind; > const struct idltest_simple3 *s3; > const struct idltest_simple4 *s4; > const struct idltest_simple6 *s6; > @@ -2329,6 +2368,10 @@ print_idl_track(struct ovsdb_idl *idl, int > step, bool terse) > print_idl_row_simple6(s6, step, terse); > n++; > } > + IDLTEST_INDEXED_FOR_EACH (ind, idl) { > + print_idl_row_indexed(ind, step, terse); > + n++; > + } > > if (!n) { > print_and_log("%03d: empty", step); > diff --git a/tests/test-ovsdb.py b/tests/test-ovsdb.py > index 48f8ee2d7..393ddf2ac 100644 > --- a/tests/test-ovsdb.py > +++ b/tests/test-ovsdb.py > @@ -228,6 +228,10 @@ def get_link2_table_printable_row(row): > return s > > > +def get_indexed_table_printable_row(row): > + return "i=%s" % row.i > + > + > def get_singleton_table_printable_row(row): > return "name=%s" % row.name > > @@ -307,6 +311,14 @@ def print_idl(idl, step, terse=False): > terse) > n += 1 > > + if "indexed" in idl.tables: > + ind = idl.tables["indexed"].rows > + for row in ind.values(): > + print_row("indexed", row, step, > + get_indexed_table_printable_row(row), > + terse) > + n += 1 > + > if "singleton" in idl.tables: > sng = idl.tables["singleton"].rows > for row in sng.values(): > @@ -690,6 +702,9 @@ def do_idl(schema_file, remote, *commands): > idl = ovs.db.idl.Idl(remote, schema_helper, leader_only=False) > if "simple3" in idl.tables: > idl.index_create("simple3", "simple3_by_name") > + if "indexed" in idl.tables: > + idx = idl.index_create("indexed", "indexed_by_i") > + idx.add_column("i") > > if commands: > remotes = remote.split(',') > -- > 2.45.0 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > -- _‘Esta mensagem é direcionada apenas para os endereços constantes no cabeçalho inicial. Se você não está listado nos endereços constantes no cabeçalho, pedimos-lhe que desconsidere completamente o conteúdo dessa mensagem e cuja cópia, encaminhamento e/ou execução das ações citadas estão imediatamente anuladas e proibidas’._ * **‘Apesar do Magazine Luiza tomar todas as precauções razoáveis para assegurar que nenhum vírus esteja presente nesse e-mail, a empresa não poderá aceitar a responsabilidade por quaisquer perdas ou danos causados por esse e-mail ou por seus anexos’.* _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
