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