On Tue, Oct 20, 2026 at 5:16 PM Ilya Maximets <[email protected]> wrote:
>
> On 9/14/26 10:57 PM, Terry Wilson via dev wrote:
> > On reconnect the Python IDL discards its local replica with __clear()
> > and repopulates it from a fresh monitor dump. __clear() dropped the old
> > rows silently, so clients saw only a ROW_CREATE for every row in the new
> > snapshot -- a create-for-every-row "storm" that told the client nothing
> > about what actually changed while it was disconnected (rows that were
> > unchanged or modified looked identical: a fresh create, deletes missing).
>
> Hi, Terry. Thanks for the follow up and sorry for delay.
Thanks for the review! I think everything below look like valid concerns.
> > + def __iter__(self):
> > + for row_uuid in self._raw:
> > + notice = self._reconcile(row_uuid, self._raw[row_uuid])
> > + if notice is not None:
> > + yield notice
> > +
> > + @staticmethod
> > + def _reconcile(row_uuid, events):
> > + if len(events) == 1:
> > + return events[0]
>
> This may report a row as deleted when the row is re-inserted after the
> clear, but the update doesn't generate the creation event. I think, you
> have a patch for this in the other set. I guess, it needs to be applied
> first or sent together with this patch.
It looks like there are several places where this either could
incorporate fixes I have out for review or would conflict with them.
Given that the other fixes are a longer patch set, and are generally
less invasive and easy to compare against their referenced C patches,
it would probably be preferable to merge those and base this off of
them. I've also got the ovsdb-cs patch waiting to propose after those
get in. But I can rework that locally without spamming changes to the
mailing list.
> > + def _flush_notices(self, reconnect):
> > + """Deliver the notices buffered while parsing a monitor reply.
> > +
> > + On reconnect the buffered per-row DELETE (from __clear()) and
> > CREATE
> > + events are reconciled and handed to notify_reconnect() in a single
> > + call. Otherwise (initial download and incremental updates) each
> > + buffered notice is delivered individually through notify().
> > + """
> > + if reconnect:
> > + # Hand the buffered notices to the view and start a fresh
> > buffer,
> > + # so the (lazy) ReconciledNotices remains valid even if the
> > + # callback holds on to it past this call.
>
> The view references the real new row that may change if the processing is
> deferred. Right?
Yeah, it isn't valid after the next run() call.
> > + pending = self._pending_notices
> > + self._pending_notices = collections.defaultdict(list)
> > + self.notify_reconnect(ReconciledNotices(pending))
> > + else:
> > + for events in self._pending_notices.values():
> > + for notice in events:
> > + self.notify(*notice)
> > + self._pending_notices.clear()
>
> An LLM complains that if notify() raises, then events stay in the list
> and will be delivered again on the next run. i.e. if we have 5 events,
> 5th notify() raises, then we'll re-deliver same first 4 events next time.
>
> Should we absorb the exceptions here? Or maybe detach the pending notices
> before the loop and re-attach only the undelivered ones on exception
> and re-raise?
Yeah, eatch batch should probably just have its own pending_notices
instead of storing on Idl. I can probably pass the notices/reconnect
info to the parse_update methods, return them from __clear() etc. Of
course, if I do it that way I'm going to hit conflicts with my
ovsdb-cs patch. I'll think about it a bit.
> There is also a similar concern for when cooperative_yield raises in the
> middle of processing updates from the server, on reconnect we may deliver
> duplicate events.
>
> And there is a re-entrance concern. If the notify() happens to run idl,
> then the list can be modified while it's being iterated here raising the
> runtime error.
Yes, calling run() from notify() would be very broken. Ovsdbapp locks
around run() calls to prevent that (since it has a thread for
processing notices with "mostly static" row copies--references are a
problem for it though).
> > diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
> > index cd534e09d..41b399cc8 100644
> > --- a/tests/ovsdb-idl.at
> > +++ b/tests/ovsdb-idl.at
> > @@ -2099,13 +2099,32 @@ OVSDB_CHECK_IDL_NOTIFY([simple idl verify notify],
> > 012: table simple: i=-1 r=125 b=false s=newstring u=<2> ia=[1] ra=[1.5]
> > ba=[false] sa=[] ua=[] uuid=<6>
> > 012: table simple: i=1 r=123.5 b=false s=mystring u=<3> ia=[1 2 3]
> > ra=[-0.5] ba=[true] sa=[abc def] ua=[<4> <5>] uuid=<0>
> > 013: reconnect
> > -014: event:create, row={i=-1 r=125 b=false s=newstring u=<2> ia=[1]
> > ra=[1.5] ba=[false] sa=[] ua=[]}, uuid=<6>, updates=None
> > -014: event:create, row={i=1 r=123.5 b=false s=mystring u=<3> ia=[1 2 3]
> > ra=[-0.5] ba=[true] sa=[abc def] ua=[<4> <5>]}, uuid=<0>, updates=None
>
> This highlights that the change is a change in behavior and cannot be easily
> backported without potentially breaking something. I'd still suggest
> splitting
> this patch in two:
> 1. No default notify_reconnect() implementation and use the fact if it is
> registered as an indication if the deletion events supposed to be generated
> on clear().
> 2. Add the default implementation for the notify_reconnect.
> The first we could backport to 3.7 LTS. The second will stay on main, i.e.
> 4.1.
>
> WDYT?
Sounds good, thanks!
Terry Wilson
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
