Skip to content

Commit

Permalink
python: idl: Fix index not being updated on row modification.
Browse files Browse the repository at this point in the history 
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: 13973bc ("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]>
Acked-by: Mike Pattrick <[email protected]>
Acked-by: Dumitru Ceara <[email protected]>
Acked-by: Terry Wilson <[email protected]>
Signed-off-by: Ilya Maximets <[email protected]>
  • Loading branch information
@igsilya
igsilya committed Jun 7, 2024
1 parent d401291 commit fad8c8f
Show file tree
Hide file tree
Showing 4 changed files with 160 additions and 6 deletions.
13 changes: 9 additions & 4 deletions python/ovs/db/idl.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1013,7 +1013,9 @@ def __process_update2(self, table, uuid, row_update):
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',
Expand Down Expand Up @@ -1044,9 +1046,10 @@ def __process_update(self, table, uuid, old, new):
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:
Expand All @@ -1058,9 +1061,11 @@ def __process_update(self, table, uuid, old, new):
# 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
Expand Down
95 changes: 93 additions & 2 deletions tests/ovsdb-idl.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -1288,6 +1322,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.
Expand Down Expand Up @@ -2036,6 +2097,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
Expand Down
43 changes: 43 additions & 0 deletions tests/test-ovsdb.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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++;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
15 changes: 15 additions & 0 deletions tests/test-ovsdb.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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():
Expand Down Expand Up @@ -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(',')
Expand Down

0 comments on commit fad8c8f

Please sign in to comment.