On 7/10/26 8:25 PM, Terry Wilson via dev wrote:
> When the IDL clears its local replica (on initial connection or when
> monitor_cond_since cannot provide incremental updates), clients
> currently receive ROW_CREATE for all rows in the new snapshot with
> no indication of which rows were deleted while disconnected.
>
> Add a notify_on_clear option that generates ROW_DELETE notices for
> existing rows before they are cleared. These are reconciled with
> the subsequent ROW_CREATE notices via a new ReconciledNotices view:
> - Surviving rows with identical data are suppressed
> - Surviving rows with changed data become ROW_UPDATE
> - Rows only in the old snapshot become ROW_DELETE
> - Rows only in the new snapshot become ROW_CREATE
>
> Assisted-by: Claude Opus 4.6
> Signed-off-by: Terry Wilson <[email protected]>
> ---
Hi, Terry. Thanks for the patch and sorry for delay.
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.
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.
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.
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.
While at it, I ran some LLM review on the patch, and the following
feedback seems worth looking at:
===
> 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
