On 4/12/22 00:22, 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. There could be other users that also
> start failing when upgrading to OVS 2.17, so this patch makes
> TableSchema.condition a property, warning users who assign to
> .condition to use cond_change() instead, but doing the equivalent
> to what it used to do.
>
> Fixes: 46d44cf3b ("python: idl: Add monitor_cond_since support")
> Signed-off-by: Terry Wilson <[email protected]>
> ---
> python/ovs/db/idl.py | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
> index 83a634dd0..1f75e9d8e 100644
> --- a/python/ovs/db/idl.py
> +++ b/python/ovs/db/idl.py
> @@ -140,6 +140,21 @@ class ConditionState(object):
> return False
>
>
> +def get_condition(self):
> + return self._condition
> +
> +
> +def set_condition(self, condition):
> + if isinstance(condition, ConditionState):
> + self._condition = condition
> + else:
> + vlog.warn("Setting table.condition directly is unsupported! "
> + "Use Idl.cond_change() instead.")
> + if not hasattr(self, '_condition'):
> + self._condition = ConditionState()
> + self.idl.cond_change(self.name, condition)
> +
> +
I'm guessing the reason why these are defined here, top-level, is that
we don't have a "Table" type and kind of forcefully extending the
Idl.tables instances (of Type ovs.db.schema.TableSchema) to store more
information: the rows, the conditions, etc.
Should we consider adding a new class to idl.py, e.g., Table, to wrap a
TableSchema and all these additional fields?
The property getter/setters would fit better in such a class I think.
On the other hand, that's quite a change and might not be easy to backport.
> class Idl(object):
> """Open vSwitch Database Interface Definition Language (OVSDB IDL).
>
> @@ -205,6 +220,9 @@ class Idl(object):
> Monitor.monitor_cond: IDL_S_DATA_MONITOR_COND_REQUESTED,
> Monitor.monitor_cond_since: IDL_S_DATA_MONITOR_COND_SINCE_REQUESTED}
>
> + ovs.db.schema.TableSchema.condition = property(get_condition,
> + set_condition)
> +
> def __init__(self, remote, schema_helper, probe_interval=None,
> leader_only=True):
> """Creates and returns a connection to the database named 'db_name'
> on
What if we change the approach and make ConditionState() "internal"?
We could change the getter/setter to always expect lists. The
getter/setter would be used by clients that didn't correctly call
cond_change(), ensuring backwards compatibility.
Internally, when creating the Idl instance, we would always initialize
the table conditions as:
table._condition = ConditionState()
Then the property should change to something like (not tested):
def get_condition(self):
return self._condition.latest
def set_condition(self, cond_list):
# Maybe vlog if not list?
# Maybe vlog to use cond_change instead?
assert(isinstance(cond_list, list))
self.idl.cond_change(self.name, cond_list)
And in idl.py we would need to use 'table._condition' everywhere instead
of 'table.condition'.
What do you think?
Thanks,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
