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. ;) > 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
