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`)
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
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
