| Message ID | 20260826043433.1832409-2-twilson@redhat.com |
|---|---|
| State | New |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | python: Backport C fixes that never made it. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_FreeBSD_Build_and_Test | success | github build: passed |
Terry Wilson via dev <ovs-dev@openvswitch.org> writes: > The Python IDL's row_update2 "modify" handling always generated a > ROW_UPDATE notification and bumped change_seqno, even when the diff only > touched columns registered without an alert. This diverged from the C > IDL, whose ovsdb_idl_modify_row_by_diff() reports whether a monitored > (alerted) column actually changed and only then treats the row as > updated. > > Make _apply_diff() report whether an alerted column changed, and have > _process_update2()'s "modify" branch notify only in that case, matching > the C behavior. This is also a prerequisite for faithfully porting the > inconsistency detection that keys off modify_row_by_diff()'s return > value. > > There is no single C commit to port here: the alert gating originates in > C commit c547535a7 ("ovsdb-idl: Make it possible to omit or pay less > attention to columns.") applied to the row-update2 diff path added in > db2b57573 ("lib: add monitor2 support in ovsdb-idl."). This commit > brings the Python IDL in line with that long-standing C behavior. > > De-mangle __apply_diff/__process_update2 to single-underscore names as > they are modified here. > > Assisted-by: Claude Opus 4.8 <noreply@anthropic.com> > Signed-off-by: Terry Wilson <twilson@redhat.com> > --- This is an AI generated review of your patch. A human has looked at the results and deemed any concerns as plausible. Reviewed commit 6564fada ("python: idl: Notify on update2 diff only if alerted column changes."), touching python/ovs/db/idl.py only. The change makes `_apply_diff()` return `(old_row, changed)` where `changed` is set only when a datum actually differs from its pre-diff copy *and* the column has `alert` set, and gates the ROW_UPDATE notice (and hence the change_seqno bump in `__do_parse_update`) on that flag. It also de-mangles `__apply_diff`/`__process_update2` to single underscore. Verification performed: - Read `_process_update2`, `_apply_diff`, `__row_update`, and `__do_parse_update` at the commit revision (git show of the file). The name rename has no other callers: repo-wide grep for `apply_diff|process_update2` shows only C-side `ovsdb_datum_apply_diff*` hits plus idl.py itself; no Python subclasses or tests reference the old mangled names (`_Idl__apply_diff`). - Verified the comment's claim about in-place mutation: `Datum.diff()` (python/ovs/db/data.py:418-429 at this revision) mutates `self.values` for sets/maps and returns `self`, so the pre-diff `copy()` is indeed required for a meaningful comparison; `Datum.copy()` (data.py:302) shallow-copies the values dict, which is safe because `Atom` is immutable. - Confirmed `column.alert` always exists (`IdlColumn.__init__` sets it True at idl.py:174; server-table columns are forced True near line 924), so `column.alert` cannot raise AttributeError, and the gating mirrors `__row_update` (idl.py:1271 at this revision). - Compared with C: `ovsdb_idl_row_change()` in lib/ovsdb-idl.c gates the alert check identically for the non-xor path, but for the xor/diff path it sets `datum_changed = true` whenever the diff applies without a parse error, without comparing old vs new values. Concern: > old = row._data[column_name].copy() > old_row[column_name] = old > datum = row._data[column_name].diff(datum_diff) > row._data[column_name] = datum > if datum != old and column.alert: > changed = True The `datum != old` requirement makes Python slightly stricter than the C IDL for the diff path. In C (`ovsdb_idl_row_change()`, lib/ovsdb-idl.c around line 1902-1910), the xor branch sets `datum_changed = true` on any successful `ovsdb_datum_apply_diff_in_place()` regardless of whether the resulting datum actually differs, so a diff that touches an alert column but is a value no-op still counts as a change there. Here, such a diff would not set `changed`, so no ROW_UPDATE notice and no change_seqno bump. In practice servers compute diffs from real changes, so a no-op diff should not occur, and the Python behavior arguably matches the intent better (and matches `__row_update`'s equality gating); but it means the commit does not match C exactly on this edge, which matters for the stated follow-up work ("faithfully porting the inconsistency detection that keys off modify_row_by_diff()'s return value") if that port assumes C's "diff applied = changed" semantics. Minor documentation/semantics nit rather than a practical bug. Two smaller observations, no action needed: (1) `_apply_diff` is now a single-underscore method with a changed return signature (dict -> tuple); any out-of-tree subclass overriding the old name-mangled `__apply_diff` would silently stop being called, but nothing in this repo does. (2) For scalar columns whose diff parses to an equal value, the new unconditional `row._data[column_name] = datum` materializes an explicit entry in the lazy `ColumnDefaultDict` where the old code left the default lazy; values are equal and `ColumnDefaultDict.__contains__`/`keys()` cover all columns, so behavior is unchanged. Verdict: PASS
diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py index fe504a63a..61107c189 100644 --- a/python/ovs/db/idl.py +++ b/python/ovs/db/idl.py @@ -962,7 +962,7 @@ class Idl(object): self.cooperative_yield() if version in (OVSDB_UPDATE2, OVSDB_UPDATE3): - changes = self.__process_update2(table, uuid, row_update) + changes = self._process_update2(table, uuid, row_update) if changes and tables is not self.server_tables: notices.append(changes) self.change_seqno += 1 @@ -984,7 +984,7 @@ class Idl(object): for notice in notices: self.notify(*notice) - def __process_update2(self, table, uuid, row_update): + def _process_update2(self, table, uuid, row_update): """Returns Notice if a column changed, False otherwise.""" row = table.rows.get(uuid) if "delete" in row_update: @@ -1015,9 +1015,11 @@ class Idl(object): raise error.Error('Modify non-existing row') del table.rows[uuid] - old_row = self.__apply_diff(table, row, row_update['modify']) + old_row, changed = self._apply_diff(table, row, + row_update['modify']) table.rows[uuid] = row - return Notice(ROW_UPDATE, row, Row(self, table, uuid, old_row)) + if changed: + return Notice(ROW_UPDATE, row, Row(self, table, uuid, old_row)) else: raise error.Error('<row-update> unknown operation', row_update) @@ -1152,8 +1154,9 @@ class Idl(object): if column.type.n_min != 0 and not column.type.is_map(): row_update[column.name] = self.__column_name(column) - def __apply_diff(self, table, row, row_diff): + def _apply_diff(self, table, row, row_diff): old_row = {} + changed = False for column_name, datum_diff_json in row_diff.items(): column = table.columns.get(column_name) if not column: @@ -1170,12 +1173,18 @@ class Idl(object): % (column_name, table.name, e)) continue - old_row[column_name] = row._data[column_name].copy() + # Datum.diff() mutates the datum in place for sets and maps and + # returns 'self', so compare the new value against the pre-diff + # copy rather than against row._data[column_name] (which diff() + # may already have updated). + old = row._data[column_name].copy() + old_row[column_name] = old datum = row._data[column_name].diff(datum_diff) - if datum != row._data[column_name]: - row._data[column_name] = datum + row._data[column_name] = datum + if datum != old and column.alert: + changed = True - return old_row + return old_row, changed def __row_update(self, table, row, row_json): changed = False
The Python IDL's row_update2 "modify" handling always generated a ROW_UPDATE notification and bumped change_seqno, even when the diff only touched columns registered without an alert. This diverged from the C IDL, whose ovsdb_idl_modify_row_by_diff() reports whether a monitored (alerted) column actually changed and only then treats the row as updated. Make _apply_diff() report whether an alerted column changed, and have _process_update2()'s "modify" branch notify only in that case, matching the C behavior. This is also a prerequisite for faithfully porting the inconsistency detection that keys off modify_row_by_diff()'s return value. There is no single C commit to port here: the alert gating originates in C commit c547535a7 ("ovsdb-idl: Make it possible to omit or pay less attention to columns.") applied to the row-update2 diff path added in db2b57573 ("lib: add monitor2 support in ovsdb-idl."). This commit brings the Python IDL in line with that long-standing C behavior. De-mangle __apply_diff/__process_update2 to single-underscore names as they are modified here. Assisted-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Terry Wilson <twilson@redhat.com> --- python/ovs/db/idl.py | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-)