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?
>
> 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.
>
> Best regards, Ilya Maximets.
>
>> What I'm doing to
>> manually test is to run this:
>>
>> import logging
>> import ovs
>>
>> import ovsdbapp.schema.ovn_northbound.impl_idl as nb_idl
>> from ovsdbapp.backend.ovs_idl import connection, vlog
>>
>> print(f"Using OVS {ovs.__file__}")
>> logging.basicConfig(level=logging.DEBUG)
>> vlog.use_python_logger()
>> LOG = logging.getLogger(__name__)
>>
>> remote = "unix:///home/twilson/src/ovn/tutorial/sandbox/nb1.ovsdb"
>>
>> def get_idl():
>> """Connection getter."""
>>
>> idl = connection.OvsdbIdl.from_server(remote, "OVN_Northbound",
>> leader_only=False)
>> return nb_idl.OvnNbApiIdlImpl(connection.Connection(idl, 100))
>>
>> idl = get_idl()
>> LOG.info(f"monitor_id: {str(idl.idl.uuid)}")
>> LOG.info(f"server_monitor_id: {str(idl.idl.server_monitor_uuid)}")
>> input("******** Press Enter ********")
>> idl.ls_add("test").execute(check_error=True)
>>
>> and then in another window running:
>> ovsdb-client convert
>> unix:/home/twilson/src/ovn/tutorial/sandbox/nb1.ovsdb
>> /home/twilson/src/ovn/ovn-nb.ovsschema
>>
>> and then pressing enter in the original window. Without the patch,
>> after running the ovsdb-client convert, you get:
>> DEBUG:ovsdbapp.backend.ovs_idl.vlog:unix:///home/twilson/src/ovn/tutorial/sandbox/nb1.ovsdb:
>> received unexpected notification message
>> and then ovsdbapp starts throwing exceptions. With the patch, things
>> work as one would expect.
>>
>> The problem with testing is that the client connection needs to be
>> active during the monitor_canceled that happens when ovsdb-client
>> convert is called. The tests in ovsdb-idl.at use ovsdb-client transact
>> for doing everything, so it's not easy to do the convert between
>> connection and transaction execution.
>>
>> Maybe something could be added test test-ovsdb.py stuff.
>>
>> Terry
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
