On 6/6/24 20:55, Terry Wilson wrote: > On Thu, Jun 6, 2024 at 10:41 AM Dumitru Ceara <[email protected]> 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 <[email protected]> >>> Signed-off-by: Ilya Maximets <[email protected]> >>> --- >> >> Looks good to me: >> >> Acked-by: Dumitru Ceara <[email protected]> >> >> 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 <[email protected]> >
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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
