On Thu, Feb 27, 2025 at 12:08 PM Terry Wilson <[email protected]> wrote: > > On Wed, Feb 19, 2025 at 11:49 AM Ilya Maximets <[email protected]> wrote: > > > > On 2/19/25 16:11, Max Lamprecht wrote: > > > On Wed, Feb 19, 2025 at 02:48:10PM +0100, Ilya Maximets wrote: > > >> On 2/13/25 16:39, Max Lamprecht via dev wrote: > > >>> This hook can be used by libaries to restore previous indexes after a > > >>> clear. > > >>> > > >>> Signed-off-by: Max Lamprecht <[email protected]> > > >>> --- > > >>> python/ovs/db/idl.py | 4 ++++ > > >>> 1 file changed, 4 insertions(+) > > >>> > > >>> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py > > >>> index c8cc54346..03efb1a01 100644 > > >>> --- a/python/ovs/db/idl.py > > >>> +++ b/python/ovs/db/idl.py > > >>> @@ -790,6 +790,9 @@ class Idl(object): > > >>> to doing nothing to avoid overhead where it is not needed. > > >>> """ > > >>> > > >>> + def after_clear(self): > > >>> + """Hook that can be used to restore indexes after a clear.""" > > >>> + > > >>> def __clear(self): > > >>> changed = False > > >>> > > >>> @@ -802,6 +805,7 @@ class Idl(object): > > >>> > > >>> if changed: > > >>> self.change_seqno += 1 > > >>> + self.after_clear() > > >>> > > >>> def __update_has_lock(self, new_has_lock): > > >>> if new_has_lock and not self.has_lock: > > >>> -- > > >>> 2.43.0 > > >> > > >> Hi, Max. Thanks for the patch! Could you please explain in more details > > >> what's the use case for this functionality? It's not obvious to me why > > >> such a hook is needed. > > >> > > >> Best regards, Ilya Maximets. > > > Hi Ilya, > > > > > > We use openstack in combination with ovn/ovs. > > > We observed that our networking api(neutron) got slower over time and > > > discovered that the > > > ovsidl loses indexes. > > > > > > We noticed that right before we see the lookups without indices, we > > > called self.clear() in ovs [1] > > > It seems like the idl called for a new full sync(comment: "clear table > > > rows for new dump") > > > With "table.rows = custom_index.IndexedRows(table)" this clear() function > > > reinitializes > > > the complete table.rows object which also clears all indixes which where > > > created by the ovsdbapp lib [2] > > > > > > After this patch I will submit another patch to ovsdbapp that uses this > > > hook to restore previous indexes. > > > > Thanks for the explanation. I think, I understand the problem now, > > but I'm not sure if we need a separate callback for this. > > > > The clearing of the database happens on re-connections if a new data > > snapshot is received from the database (e.g. because the current one > > is too old and the server couldn't send an incremental update). So, > > IDL clears all the old data and adds all the nex data from the new > > snapshot. > > > > C applications like ovn-controller detect such changes by checking > > condition_seqno and triggering a re-compute. Change tracking in the > > python IDL is a bit different, so I understand why we may want to > > handle this situation via a hook, since other change notifications are > > done this way. But I'm nto sure about it being a separate hook. > > > > A few potential solutions for the situation: > > > > 1. Track cond_seqno in ovsdbapp and recover caches when it changes. > > This would be similar to what C applications like ovn-controller do. > > But may be less convenient for python applications that already > > use notification hooks. And we may need to remove the setting it > > to zero in the _clear() call (it's strange that we clear it, C > > version doesn't do that). > > This sounds, on the surface anyway, most reasonable to me on the > surface, anyway. I should be able to spend a bit of time looking into > it soon if no one beats me to it. ;) >
Briefly looking at this again, in __clear() can't we just call table.rows.clear() instead of recreating table.rows? A quick look makes it seem like deleting the values, but keeping the index definitions is what we'd want. Then when the updates come back, since the indexes exist the indexes will get updated. I haven't tested or looked particularly in-depth yet, but Max, if you have a reproducer up it's something you could try. > > 2. Notify row deletion for all the rows and then parsing of the new > > data will notify addition of all the rows once the new snapshot is > > parsed. > > > > This seems logical, but may be confusing for the application, since > > the rows are not actually deleted from the database, we just report > > that temporarily. Applications will need to act on that though as > > if the row was removed, so it will create a lot of churn and > > potentially unwanted side effects. So, we really shouldn't do that. > > > > 3. Notify about the database reset once. This is this patch. However, > > instead of adding a new hook, maybe we can add a new event type > > for the notify() hook? Something like "DB_CLEAR" or "DB_RESET" ? > > This event will not come with the removed rows, obviously. > > The application can use this notification to clear its caches or > > any other data and prepare for new re-parsed data to come. > > > > What do you think? > > > > Best regards, Ilya Maximets. > > > > > > > > [1] > > > https://github.com/openvswitch/ovs/blob/8b7f1eb8db1aa99ccf7b542662129450caff65e0/python/ovs/db/idl.py#L495 > > > [2] > > > https://github.com/openstack/ovsdbapp/blob/14300779028fab5b1756dc35544d08ec0bb1f1b4/ovsdbapp/backend/ovs_idl/__init__.py#L81 > > > [3] https://bugs.launchpad.net/neutron/+bug/2092133/comments/5 > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
