On Thu, Feb 27, 2025 at 3:17 PM Terry Wilson <[email protected]> wrote: > > 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. >
Reading again (sigh), IndexedRows.clear() deletes the indexes as well. But it'd be easy to add a 'clear_data()' etc. > > > 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
