On Fri, Feb 28, 2025 at 6:30 AM Max Lamprecht <[email protected]> wrote: > > On Thu, Feb 27, 2025 at 04:22:09PM -0600, Terry Wilson wrote: > > On Thu, Feb 27, 2025 at 3:22 PM Terry Wilson <[email protected]> wrote: > > > > > > 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. > > > > Ok, I wrote a little test program, and from what I can tell, just doing > > diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py > > index 384428c3f..f01e21b5e 100644 > > --- a/python/ovs/db/idl.py > > +++ b/python/ovs/db/idl.py > > @@ -796,7 +796,7 @@ class Idl(object): > > for table in self.tables.values(): > > if table.rows: > > changed = True > > - table.rows = custom_index.IndexedRows(table) > > + table.rows.clear() > > > > self.cond_seqno = 0 > > > > works fine. I just tried it with an Open_vSwitch schema with 100 > > bridges, which get indexes created automatically with ovsdbapp. > > Verified the indexes were there, called __clear() and verified the > > data entries went away and the index definitions still existed, > > restarted ovsdb-server to trigger a reconnect/db dump, and the index > > was refilled with entries when the new dump came in. > Hi Terry, > > I initially thought about this solution. But I was not sure if this > would break during upgrades on the ovsdb-server, e.g. schema changes. > What dou you think about this?
The lib/ovsdb-idl.c ovsdb_idl_clear() loops through the rows and removes them from the index, so it seems like this would be a similar solution and doesn't seem to break any tests--though I don't think there are any python schema change w/ indexing tests. To me it just looks like I was mistaken with the original code that replaced the rows object. > Best regards > Max > > > > > > > > > > 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
