On Tue, Apr 12, 2022 at 3:25 AM Dumitru Ceara <dce...@redhat.com> wrote:
>
> 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 <dce...@redhat.com> 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 <twil...@redhat.com>
> >>> ---
> >>
> >> 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.
Ah, I understand now. Thanks. There was no use case in neutron for
reading the condition that was set (and no real "supported" way to do
it with the old or current code either).
We could probably change all of the existing code in the Idl to
get/set .condition_state, then have a condition property that returned
the list of ConditionState.latest/set via cond_change. I'll test that
change.
> 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
