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
