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