> Hi, Terry. Thanks for the patch and sorry for delay.
Thanks for taking a look!
> I was thinking about the user interface, since this supposed to be a bug
> fix, but we obviously can't just change the behavior for existing users.
> So, we need to keep it opt-in, at least on stable branches, but somehow
> make the functionality seamless for new users who want to take advantage
> of the bug fix.
I think there is generally a long enough lead time for downstream
python-ovs users picking up new releases that doing it w/o the opt-in
option in main and doing the option only in the backport would be
fine. If we wanted to be super-careful we could have a deprecation
cycle w/ a WARNING message that the default was going to change. But I
can certainly make sure that neutron/ovsdbapp are ready well in
advance of picking up the release.
> My main point of thinking was that there is a missing bit that is the
> 'reconnect' notification. Users can't really find out that the database
> was reconnected underneath. And so they are using random side channels
> to get this information. The 'bulk_notify' fixes that sort of by adding
> the 'initial' flag, but this is still not really a direct communication.
I think this makes sense. I was thinking that a bulk_notify() would be
more generally useful, but it really is only the reconnect case where
having them grouped matters. Outside of that, what ends up in the
"bulk" is just whatever ovsdb-server puts in a message which isn't
necessarily any logically connected grouping.
> So, maybe instead of 'bulk_notify', we add a new hook 'reconnect_notify'
> with the similar functionality. It will be triggered when we reconnected
> and will carry the reconciled events. It will not need the 'initial'
> flag, as we're delivering all the deletions/creations/updates.
> All the normal updates will keep coming via current notify() hook, so
> no changes to the current users. Same as for notify(), idl will not
> define the new reconnect_notify(). The existence of the method then can
> be used to check if we need to generate all the extra deletions on clear.
>
> This also makes the backporting simple, but also will provide forward
> compatibility for users who choses not to implement the new hook while
> using future versions of OVS.
For main, we could define reconnect_notify() in the Idl which loops
over pending notices sending to normal notify(), implementing the
default behavior "fix" behavior.Then leave that out for backports and
just subclass or define the method to "opt-in". And then for anything
that wants to do something special can override it (e.g. mark a full
sync of some local state or something).
> What do you think about this? Did I miss some use case?
>
> I guess, one disadvantage of this method is that we can no longer bulk
> notify normal updates, but I'm not sure how important that is. We could
> extend the standard notify() or add a new bulk_notify() hook in the
> future for that purpose, if necessary.
I thought it was important, but it turns out I don't think it is at all. :)
> While at it, I ran some LLM review on the patch, and the following
> feedback seems worth looking at:
I'll look at that when doing the new version.
> ===
>
> > def __parse_update(self, update, version, tables=None, initial=False):
> > try:
> > [...]
> > self.__do_parse_update(update, version, self.tables,
> > initial=initial)
> > [...]
> > except error.Error as e:
> > vlog.err("%s: error parsing update: %s"
> > % (self._session.get_name(), e))
>
> > def __clear(self):
> > [...]
> > if self.notify_on_clear:
> > for row_uuid, row in table.rows.items():
> > self._pending_notices[row_uuid].append(
> > Notice(ROW_DELETE, row))
>
> > self.bulk_notify(ReconciledNotices(self._pending_notices),
> > initial=initial)
> > self._pending_notices.clear()
>
> When __clear() runs with notify_on_clear=True, it appends
> ROW_DELETE notices into _pending_notices. If __do_parse_update()
> then raises an error.Error (e.g. from a malformed table-update or
> unknown table name), __parse_update() catches the exception and
> returns, but bulk_notify() and _pending_notices.clear() at the end
> of __do_parse_update() are never reached.
>
> The caller (e.g. the monitor_cond_since handler) then sets
> self.state = IDL_S_MONITORING, since __parse_update() swallowed
> the error. On the next incremental update, __do_parse_update()
> appends new notices to the still-dirty _pending_notices. If a UUID
> already has a stale DELETE notice and gets a new CREATE plus
> UPDATE, _reconcile() hits the assert at line 102 with three events
> for the same UUID. With python -O, the assert is stripped and
> _reconcile() silently returns None, dropping legitimate notices.
>
> Could _pending_notices be cleared in the except clause of
> __parse_update(), or in __clear() itself after the error path, to
> prevent stale notices from leaking across update cycles?
>
> ===
>
> > @staticmethod
> > def _reconcile(row_uuid, events):
> > [...]
> > if old_data:
> > return Notice(ROW_UPDATE, new_row,
> > Row(None, old_row._table, row_uuid, old_data))
>
> > def __process_update2(self, table, uuid, row_update):
> > [...]
> > return Notice(ROW_UPDATE, row, Row(self, table, uuid, old_row))
>
> The reconciled ROW_UPDATE notice creates its updates Row with
> idl=None, while __process_update2() passes self (the Idl instance).
> Since _reconcile() is a @staticmethod, it has no access to the Idl.
>
> If a notify() callback accesses a reference-type column on that
> updates Row, Row._uuid_to_row() tries self._idl.tables, which
> raises AttributeError because _idl is None. Scalar columns work
> fine since _uuid_to_row() returns the atom directly when
> base.ref_table is None, so this only surfaces for reference
> columns.
>
> Would it make sense to pass the Idl instance into _reconcile()
> (or into ReconciledNotices) so the updates Row is constructed
> consistently with the other code paths?
> ===
>
> Best regards, Ilya Maximets.
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
