On Wed, Jan 10, 2024 at 06:38:48PM +0100, Ilya Maximets wrote:
> On 1/10/24 18:18, Simon Horman wrote:
> > On Wed, Jan 10, 2024 at 12:28:22PM +0100, Dumitru Ceara wrote:
> >> On 1/8/24 16:31, Ilya Maximets wrote:
> >>> On 1/8/24 16:05, Ilya Maximets wrote:
> >>>> On 1/5/24 18:35, Terry Wilson wrote:
> >>>>> On Fri, Jan 5, 2024 at 9:56 AM Simon Horman <[email protected]> wrote:
> >>>>>>
> >>>>>> On Mon, Dec 18, 2023 at 05:31:24PM -0600, Terry Wilson wrote:
> >>>>>>> Currently python-ovs claims to be "db change aware" but does not
> >>>>>>> parse the "monitor_canceled" notification. Transactions can continue
> >>>>>>> being made, but the monitor updates will not be sent. This handles
> >>>>>>> monitor_cancel similarly to how ovsdb-cs currently does.
> >>>>>>>
> >>>>>>> Signed-off-by: Terry Wilson <[email protected]>
> >>>>>>
> >>>>>> Hi Terry,
> >>>>>>
> >>>>>> is it possible to add a test to tests/ovsdb-idl.at for this feature?
> >>>>>
> >>>>> It might be, but it seems like it'd be a bit of work and I'm not sure
> >>>>> if I'll have the time to look at it for a while. I'm just trying to
> >>>>> bring the functionality up to what the C IDL has and from what I can
> >>>>> tell this feature isn't tested in the C IDL either.
> >>>>
> >>>> Hi, Terry and Simon.
> >>>>
> >>>> I spent some time and came up with the following test for the issue:
> >>>>
> >>>> https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/
> >>>> The test in the patch will fail without the fix provided here.
> >>>>
> >>>> Also, this change is not really a new feature. Python IDL today claims
> >>>> that it is database change aware and implements the monitoring of the
> >>>> _Server database, but it is not handling the monitor cancellation that
> >>>> is a vital part of being change aware. So, IMO, it is a bug that should
> >>>> be fixed in stable branches as well, unless there are reasons not to.
> >>>>
> >>>> The following tag should be added:
> >>>>
> >>>> Fixes: c39751e44539 ("python: Monitor Database table to manage lifecycle
> >>>> of IDL client.")
> >>>>
> >>>> The test is quite large and requires changing the test utilities, so
> >>>> I'm not sure if it should be squashed with the fix or just treated as
> >>>> a separate patch. OTOH, tests should normally go together with the
> >>>> fix. I'm OK with either option, but the commit message of the test
> >>>> patch should be preserved as it contains important information about a
> >>>> different bug. What do you think?
> >>>
> >>> Actually, I guess, the following might be an option:
> >>>
> >>> 1. I can carve out python-related things from the test I posted, keeping
> >>> it for C IDL only, but easily extendable for python.
> >>> 2. Get the test reviewed/accepted and backported.
> >>> 3. Add the python test bits to this fix, so the test is included.
> >>>
> >>> The change for the test will look something like this:
> >>>
> >>> diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
> >>> index b522208c8..003ba6571 100644
> >>> --- a/tests/ovsdb-idl.at
> >>> +++ b/tests/ovsdb-idl.at
> >>> @@ -2809,14 +2809,21 @@ m4_define([OVSDB_CHECK_IDL_CHANGE_AWARE],
> >>> AT_CAPTURE_FILE([idl-c.out])
> >>> AT_CAPTURE_FILE([idl-c.err])
> >>>
> >>> + OVS_DAEMONIZE([$PYTHON3 $srcdir/test-ovsdb.py -t30 \
> >>> + idl $srcdir/idltest.ovsschema unix:socket COND
> >>> monitor \
> >>> + >idl-python.out 2>idl-python.err], [idl-python.pid])
> >>> + AT_CAPTURE_FILE([idl-python.out])
> >>> + AT_CAPTURE_FILE([idl-python.err])
> >>> +
> >>> dnl Wait for monitors to receive the data.
> >>> OVS_WAIT_UNTIL([grep -q 'third row' idl-c.err])
> >>> + OVS_WAIT_UNTIL([grep -q 'third row' idl-python.err])
> >>>
> >>> dnl Convert the database.
> >>> AT_CHECK([ovsdb-client convert unix:socket new-idltest.ovsschema])
> >>>
> >>> dnl Check for the monitor cancellation and the data being requested
> >>> again.
> >>> - m4_foreach([FILE], [[idl-c]],
> >>> + m4_foreach([FILE], [[idl-c], [idl-python]],
> >>> [OVS_WAIT_UNTIL([grep -q 'monitor_canceled' FILE.err])
> >>> OVS_WAIT_UNTIL([test 2 -eq $(grep -c 'send request,
> >>> method="monitor_cond_since", params=."idltest"' FILE.err)])
> >>>
> >>> ---
> >>> The changes for my test patch would be a revert of that, of course
> >>> (keeping the test-ovsdb.py modification in the test patch to have
> >>> parity between C and Python test utilities).
> >>>
> >>> What do you think?
> >>>
> >>
> >> TBH I'd just apply both this patch and the test you posted as is
> >> (separate patches) and backport them both. The changes in test
> >> utilities are not features either IMO.
>
> OK. Sounds good to me. It's definitely easier this way. :)
>
> >
> > Thanks, I like the simplicity of this approach.
> > I will work on making it so and report back.
>
> Thanks!
>
> >
> > For the record (mainly for my benefit) I have this patch applied locally
> > with:
> > * A dot appended to the subject
> > * Your ack
>
> Please, add a Fixes tag as well, if it's not there already.
I knew I missed something from my previous email. It is there:
* Fixes: c39751e ("python: Monitor Database table to manage lifecycle of IDL
client.")
For now the patches are queued to allow the GitHub actions to run.
If that goes well I plan to push them.
* https://github.com/horms/ovs/commits/series_387233%2B389371/
* https://github.com/horms/ovs/commits/branch-3.2-series_387233%2B389371/
* https://github.com/horms/ovs/commits/branch-3.1-series_387233%2B389371/
* https://github.com/horms/ovs/commits/branch-3.0-series_387233%2B389371/
* https://github.com/horms/ovs/commits/branch-2.17-series_387233%2B389371/
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
