On 4/13/22 15:15, Terry Wilson wrote: > On Wed, Apr 13, 2022 at 3:06 AM Dumitru Ceara <[email protected]> wrote: >> >> On 4/12/22 21:52, Terry Wilson wrote: >>> Before 46d44cf3b, it was technically possible to assign a monitor >>> condition directly to Idl.tables[table_name].condition. If done >>> before the connection was established, it would successfully apply >>> the condition (where cond_change() actually would fail). >>> >>> Although this wasn't meant to be supported, several OpenStack >>> projects made use of this. After 46d44cf3b, .condition is no >>> longer a list, but a ConditionState. Assigning a list to it breaks >>> the Idl. >>> >>> The Neutron and ovsdbapp projects have patches in-flight to >>> use Idl.cond_change() if ConditionState exists, as it now works >>> before connection as well, but here could be other users that also >>> start failing when upgrading to OVS 2.17. >>> >>> Instead of directly adding attributes to TableSchema/ColumnSchema, >>> this adds the IdlTable/IdlColumn objects which hold Idl-specific >>> data and adds a 'condition' property to TableSchema that maintains >>> the old interface. Row._uuid_to_row is added to allow looking up >>> ref_table rows. >>> >>> Fixes: 46d44cf3b ("python: idl: Add monitor_cond_since support") >>> Signed-off-by: Terry Wilson <[email protected]> >>> --- >> >> Hi Terry, >> >> Thanks for this revision! >> >> There's one small issue reported in CI: >> >> ../../python/ovs/db/idl.py:175:1: H238: old style class declaration, >> use new style (inherit from `object`) > > Hmm, we probably need to get rid of that check. It is no longer an > old-style class--it's how it's done in py3. (you can use object as > well, though). >
You're right, thanks for pointing this out! I didn't know that this is the new (and old) way of declaring classes. We should probably remove H238 from FLAKE_SELECT now that we don't support python 2 anymore. But that should probably be a separate commit. Until then, and because the rest of the idl classes do it to, I'd say we should inherit from object explicitly. >> ovsrobot is complaining about this too (unfortunately automated emails >> with test results are not being sent out at the moment): >> >> https://github.com/ovsrobot/ovs/runs/5996694299?check_suite_focus=true#step:12:4197 >> >> CI passes after fixing the flake8 reported issue: >> https://github.com/dceara/ovs/actions/runs/2159668320 >> >> I have one question and a minor comment below. >> >>> python/ovs/db/idl.py | 95 +++++++++++++++++++++++++++++++------------- >>> 1 file changed, 67 insertions(+), 28 deletions(-) >>> >>> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py >>> index 83a634dd0..6ae31d106 100644 >>> --- a/python/ovs/db/idl.py >>> +++ b/python/ovs/db/idl.py >>> @@ -140,6 +140,47 @@ class ConditionState(object): >>> return False >>> >>> >>> +class IdlTable: >>> + def __init__(self, idl, table): >>> + assert(isinstance(table, ovs.db.schema.TableSchema)) >>> + self._table = table >>> + self.need_table = False >>> + self.rows = custom_index.IndexedRows(self) >>> + self.idl = idl >>> + self._condition_state = ConditionState() >>> + self.columns = {k: IdlColumn(v) for k, v in table.columns.items()} >>> + >>> + def __getattr__(self, attr): >>> + return getattr(self._table, attr) >>> + >>> + @property >>> + def condition_state(self): >>> + # read-only, no setter >>> + return self._condition_state >>> + >>> + @property >>> + def condition(self): >>> + return self.condition_state.latest >>> + >>> + @condition.setter >>> + def condition(self, condition): >>> + assert(isinstance(condition, list)) >>> + self.idl.cond_change(self.name, condition) >>> + >>> + @classmethod >>> + def schema_tables(cls, idl, schema): >>> + return {k: cls(idl, v) for k, v in schema.tables.items()} >>> + >>> + >>> +class IdlColumn: >>> + def __init__(self, column): >>> + self._column = column >>> + self.alert = True >>> + >> >> Just to make sure, a user of the IDL can still instantiate the IDL and >> then selectively disable alerting for some columns, right? The >> interface won't change and something like the following will still work: >> >> idl.tables["table-name"].columns["column-name"].alert = False > > I did not specifically test setting 'alert' but, since it is looked > up, but as long as an IdlTable is passed to insert() (which is the > only way a user would do it), it *should* work. The old code defaulted > to True if the attribute hadn't been added. In this case, we just > default to True when creating the new IdlColumn. Setting it *should* > be the same. > Ack, I did hack some of the unit tests and it seems to be the same behavior. >> Ideally we should add a test for this. OTOH we don't have a test for >> the C implementation either so this is maybe something we should do as a >> follow up after your patch is accepted. >> >>> + def __getattr__(self, attr): >>> + return getattr(self._column, attr) >>> + >>> + >>> class Idl(object): >>> """Open vSwitch Database Interface Definition Language (OVSDB IDL). >>> >>> @@ -241,7 +282,7 @@ class Idl(object): >>> assert isinstance(schema_helper, SchemaHelper) >>> schema = schema_helper.get_idl_schema() >>> >>> - self.tables = schema.tables >>> + self.tables = IdlTable.schema_tables(self, schema) >>> self.readonly = schema.readonly >>> self._db = schema >>> remotes = self._parse_remotes(remote) >>> @@ -282,15 +323,6 @@ class Idl(object): >>> self.cond_changed = False >>> self.cond_seqno = 0 >>> >>> - for table in schema.tables.values(): >>> - for column in table.columns.values(): >>> - if not hasattr(column, 'alert'): >>> - column.alert = True >>> - table.need_table = False >>> - table.rows = custom_index.IndexedRows(table) >>> - table.idl = self >>> - table.condition = ConditionState() >>> - >>> def _parse_remotes(self, remote): >>> # If remote is - >>> # "tcp:10.0.0.1:6641,unix:/tmp/db.sock,t,s,tcp:10.0.0.2:6642" >>> @@ -330,7 +362,7 @@ class Idl(object): >>> def ack_conditions(self): >>> """Mark all requested table conditions as acked""" >>> for table in self.tables.values(): >>> - table.condition.ack() >>> + table.condition_state.ack() >>> >>> def sync_conditions(self): >>> """Synchronize condition state when the FSM is restarted >>> @@ -361,10 +393,10 @@ class Idl(object): >>> >>> for table in self.tables.values(): >>> if ack_all: >>> - table.condition.request() >>> - table.condition.ack() >>> + table.condition_state.request() >>> + table.condition_state.ack() >>> else: >>> - if table.condition.reset(): >>> + if table.condition_state.reset(): >>> self.last_id = str(uuid.UUID(int=0)) >>> self.cond_changed = True >>> >>> @@ -485,7 +517,7 @@ class Idl(object): >>> sh.register_table(self._server_db_table) >>> schema = sh.get_idl_schema() >>> self._server_db = schema >>> - self.server_tables = schema.tables >>> + self.server_tables = IdlTable.schema_tables(self, >>> schema) >>> self.__send_server_monitor_request() >>> except error.Error as e: >>> vlog.err("%s: error receiving server schema: %s" >>> @@ -591,10 +623,10 @@ class Idl(object): >>> for table in self.tables.values(): >>> # Always use the most recent conditions set by the IDL client >>> when >>> # requesting monitor_cond_change >>> - if table.condition.new is not None: >>> + if table.condition_state.new is not None: >>> change_requests[table.name] = [ >>> - {"where": table.condition.new}] >>> - table.condition.request() >>> + {"where": table.condition_state.new}] >>> + table.condition_state.request() >>> >>> if not change_requests: >>> return >>> @@ -630,19 +662,20 @@ class Idl(object): >>> cond = [False] >>> >>> # Compare the new condition to the last known condition >>> - if table.condition.latest != cond: >>> - table.condition.init(cond) >>> + if table.condition_state.latest != cond: >>> + table.condition_state.init(cond) >>> self.cond_changed = True >>> >>> # New condition will be sent out after all already requested ones >>> # are acked. >>> - if table.condition.new: >>> - any_reqs = any(t.condition.request for t in >>> self.tables.values()) >>> + if table.condition_state.new: >>> + any_reqs = any(t.condition_state.request >>> + for t in self.tables.values()) >>> return self.cond_seqno + int(any_reqs) + 1 >>> >>> # Already requested conditions should be up to date at >>> # self.cond_seqno + 1 while acked conditions are already up to date >>> - return self.cond_seqno + int(bool(table.condition.requested)) >>> + return self.cond_seqno + int(bool(table.condition_state.requested)) >>> >>> def wait(self, poller): >>> """Arranges for poller.block() to wake up when self.run() has >>> something >>> @@ -814,8 +847,8 @@ class Idl(object): >>> columns.append(column) >>> monitor_request = {"columns": columns} >>> if method in ("monitor_cond", "monitor_cond_since") and ( >>> - not ConditionState.is_true(table.condition.acked)): >>> - monitor_request["where"] = table.condition.acked >>> + not >>> ConditionState.is_true(table.condition_state.acked)): >>> + monitor_request["where"] = table.condition_state.acked >>> monitor_requests[table.name] = [monitor_request] >>> >>> args = [self._db.name, str(self.uuid), monitor_requests] >>> @@ -1271,6 +1304,12 @@ class Row(object): >>> data=", ".join("{col}={val}".format(col=c, val=getattr(self, >>> c)) >>> for c in sorted(self._table.columns))) >>> >>> + def _uuid_to_row(self, atom, base): >>> + if base.ref_table: >>> + return self._idl.tables[base.ref_table.name].rows.get(atom) >>> + else: >>> + return atom >>> + >> >> I think we can safely remove the top-level _uuid_to_row() now. >> >>> def __getattr__(self, column_name): >>> assert self._changes is not None >>> assert self._mutations is not None >>> @@ -1312,7 +1351,7 @@ class Row(object): >>> datum = data.Datum.from_python(column.type, dlist, >>> _row_to_uuid) >>> elif column.type.is_map(): >>> - dmap = datum.to_python(_uuid_to_row) >>> + dmap = datum.to_python(self._uuid_to_row) >>> if inserts is not None: >>> dmap.update(inserts) >>> if removes is not None: >>> @@ -1329,7 +1368,7 @@ class Row(object): >>> else: >>> datum = inserts >>> >>> - return datum.to_python(_uuid_to_row) >>> + return datum.to_python(self._uuid_to_row) >>> >>> def __setattr__(self, column_name, value): >>> assert self._changes is not None >>> @@ -1413,7 +1452,7 @@ class Row(object): >>> if value: >>> try: >>> old_value = data.Datum.to_python(self._data[column_name], >>> - _uuid_to_row) >>> + self._uuid_to_row) >>> except error.Error: >>> return >>> if key not in old_value: >> >> With the above minor comments addressed, feel free to add my ack to the >> next revision: >> >> Acked-by: Dumitru Ceara <[email protected]> >> >> Thanks! >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
