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. > >> >>>> >>>> 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
