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