cc: ovs-dev (it got dropped by accident)
On 4/12/22 00:07, Terry Wilson wrote:
> On Fri, Apr 1, 2022 at 9:02 AM Dumitru Ceara <[email protected]> wrote:
>>
>> On 3/25/22 18:37, 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 (which is still broken behavior after a
>>> connection is established).
>>>
>>> Fixes: 46d44cf3b ("python: idl: Add monitor_cond_since support")
>>> Signed-off-by: Terry Wilson <[email protected]>
>>> ---
>>
>> Hi Terry,
>>
>> This patch needs a small cleanup, it's currently failing CI due to:
>>
>> python/ovs/db/idl.py:223:80: E501 line too long (80 > 79 characters)
>
> Oops. I'll do a quick respin for that.
>
>>
>> https://mail.openvswitch.org/pipermail/ovs-build/2022-March/020832.html
>>
>>> python/ovs/db/idl.py | 17 +++++++++++++++++
>>> 1 file changed, 17 insertions(+)
>>>
>>> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
>>> index 4ecdcaa19..b67504278 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
>>> +
>>
>> Doesn't this mean that older clients that were reading "condition" as a
>> list will now be broken? Or am I missing something?
>>
>> On the other hand, I'm not sure if we can fix that in a polite way.
>
>
> What the line below creating the property will do is add an attribute
> on TableSchema named "condition", that when referenced will call these
> getter/setter methods. So this should allow previous users to use this
> without interruption, for users of the Idl (but if they just use
> ovs.db.schema directly, it won't be touched at all). So this *should*
> politely handle things so that nobody notices. It worked when I tested
> locally, anyway.
>
This works for the "set" case, I agree. That is, clients that write to
the condition directly without using cond_change().
What I meant is that we can't protect against clients reading
Idl.tables[table_name].condition directly and expecting it to be a list.
For example the following assertion fails when running the IDL tests
both with and without your patch and used to pass before 46d44cf3b. But
that's fine, I guess, clients shouldn't do that.
diff --git a/tests/test-ovsdb.py b/tests/test-ovsdb.py
index 853264f22bdc..81f75d3aff1a 100644
--- a/tests/test-ovsdb.py
+++ b/tests/test-ovsdb.py
@@ -627,6 +627,8 @@ def update_condition(idl, commands):
table = command[0]
cond = ovs.json.from_string(command[1])
+ assert(isinstance(idl.tables[table].condition, list))
+
idl.cond_change(table, cond)
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
