diff mbox series

[ovs-dev] python: idl: Fix index not being updated on row modification.

Message ID 20240527213907.1362022-1-i.maximets@ovn.org
State Accepted
Commit fad8c8f7f651796fe20001d9d2679b43ed44adf8
Delegated to: Ilya Maximets
Headers show
Series [ovs-dev] python: idl: Fix index not being updated on row modification. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Ilya Maximets May 27, 2024, 9:39 p.m. UTC 
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 <roberto.acosta@luizalabs.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 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(-)

Comments

Vladislav Odintsov May 28, 2024, 3:23 p.m. UTC | #1 
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 <ovs-dev-bounces@openvswitch.org> on behalf of Ilya Maximets <i.maximets@ovn.org>
Date: Tuesday, 28 May 2024 at 00:39
To: "ovs-dev@openvswitch.org" <ovs-dev@openvswitch.org>
Cc: Ilya Maximets <i.maximets@ovn.org>, Dumitru Ceara <dceara@redhat.com>
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 <roberto.acosta@luizalabs.com>
    Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
    ---
     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
    dev@openvswitch.org
    https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Roberto Bartzen Acosta May 29, 2024, 1:29 p.m. UTC | #2 
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 <twilson@redhat.com>, 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 <odivlad@gmail.com>
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 <ovs-dev-bounces@openvswitch.org> on behalf of Ilya Maximets <
> i.maximets@ovn.org>
> Date: Tuesday, 28 May 2024 at 00:39
> To: "ovs-dev@openvswitch.org" <ovs-dev@openvswitch.org>
> Cc: Ilya Maximets <i.maximets@ovn.org>, Dumitru Ceara <dceara@redhat.com>
> 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 <roberto.acosta@luizalabs.com>
>     Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
>     ---
>      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
>     dev@openvswitch.org
>     https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Mike Pattrick June 5, 2024, 5:13 p.m. UTC | #3 
On Mon, May 27, 2024 at 5:39 PM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> 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 <roberto.acosta@luizalabs.com>
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

I've tested this a bit and it seems like a reasonable solution.

Acked-by: Mike Pattrick <mkp@redhat.com>
Dumitru Ceara June 6, 2024, 3:40 p.m. UTC | #4 
On 5/27/24 23:39, Ilya Maximets wrote:
> 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 <roberto.acosta@luizalabs.com>
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---

Looks good to me:

Acked-by: Dumitru Ceara <dceara@redhat.com>

Regards,
Dumitru
Terry Wilson June 6, 2024, 6:55 p.m. UTC | #5 
On Thu, Jun 6, 2024 at 10:41 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 5/27/24 23:39, Ilya Maximets wrote:
> > 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 <roberto.acosta@luizalabs.com>
> > Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> > ---
>
> Looks good to me:
>
> Acked-by: Dumitru Ceara <dceara@redhat.com>
>
> Regards,
> Dumitru
>

Looks good to me. I don't like that my code for IndexedRows strongly
implies that it behaves exactly like a dict, and in this case it
doesn't. Maybe some comments explaining why a delete has to be done
for posterity would be helpful.

Acked-by: Terry Wilson <twilson@redhat.com>
Ilya Maximets June 7, 2024, 1:49 p.m. UTC | #6 
On 6/6/24 20:55, Terry Wilson wrote:
> On Thu, Jun 6, 2024 at 10:41 AM Dumitru Ceara <dceara@redhat.com> wrote:
>>
>> On 5/27/24 23:39, Ilya Maximets wrote:
>>> 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 <roberto.acosta@luizalabs.com>
>>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
>>> ---
>>
>> Looks good to me:
>>
>> Acked-by: Dumitru Ceara <dceara@redhat.com>
>>
>> Regards,
>> Dumitru
>>
> 
> Looks good to me. I don't like that my code for IndexedRows strongly
> implies that it behaves exactly like a dict, and in this case it
> doesn't. Maybe some comments explaining why a delete has to be done
> for posterity would be helpful.
> 
> Acked-by: Terry Wilson <twilson@redhat.com>
> 

Thanks, Terry, Mike and Dumitru for reviews and Roberto and Vladislav
for testing!

I think, we can try to make the interfaces better, this ties a little
with the persistent uuid discussion.  But for now, applied this fix
and backported down to 2.17.

Best regards, Ilya Maximets.
diff mbox series

Patch

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(',')