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.
Thanks, I like the simplicity of this approach. I will work on making it so and report back. For the record (mainly for my benefit) I have this patch applied locally with: * A dot appended to the subject * Your ack > > >> > >> Dumitru, could you, also, please, take a look at this version of the fix? > >> I didn't spent much time on a fix itself, I only checked that it works > >> fine with the test I made. > >> > > I acked it, thanks! > > Regards, > Dumitru ... _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
