| Message ID | 20231218233124.669221-1-twilson@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Simon Horman |
| Headers | show |
| Series | [ovs-dev,v2] python: idl: Handle monitor_canceled | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
Bleep bloop. Greetings Terry Wilson, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: The subject summary should end with a dot. Subject: python: idl: Handle monitor_canceled Lines checked: 76, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
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 <twilson@redhat.com> Hi Terry, is it possible to add a test to tests/ovsdb-idl.at for this feature?
On Fri, Jan 5, 2024 at 9:56 AM Simon Horman <horms@ovn.org> 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 <twilson@redhat.com> > > 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. 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 Terry
On 1/5/24 18:35, Terry Wilson wrote: > On Fri, Jan 5, 2024 at 9:56 AM Simon Horman <horms@ovn.org> 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 <twilson@redhat.com> >> >> 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/20240108145248.1763075-1-i.maximets@ovn.org/ 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
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 <horms@ovn.org> 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 <twilson@redhat.com> >>> >>> 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/20240108145248.1763075-1-i.maximets@ovn.org/ > 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 >
On 12/19/23 00:31, 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 <twilson@redhat.com> > --- Hi Terry, Thanks for the v2! > python/ovs/db/idl.py | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py > index 9fc2159b0..be6ae2ca4 100644 > --- a/python/ovs/db/idl.py > +++ b/python/ovs/db/idl.py > @@ -299,6 +299,7 @@ class Idl(object): > self._server_schema_request_id = None > self._server_monitor_request_id = None > self._db_change_aware_request_id = None > + self._monitor_cancel_request_id = None This makes me a bit uneasy. I don't think there's ever a case when the first 3 IDs above are simultaneously not-None. For the monitor_cancel request ID it's possible it's not-None while others are also not-None because we immediately restart the FSM after sending the cancel request. Note: it's also what the ovsdb-cs layer does today (except that the cs layer doesn't even validate that the ID we get in the reply matches the ID of the request). I think ideally the state machine should be a bit more robust and just wait for the cancel reply to be received before restarting the FSM. Anyway, end of rant, I guess all of the above can be fixed as a follow up patch if we ever find time for it. Above rant aside: Acked-by: Dumitru Ceara <dceara@redhat.com> Regards, Dumitru > self._server_db_name = '_Server' > self._server_db_table = 'Database' > self.server_tables = None > @@ -481,6 +482,10 @@ class Idl(object): > break > else: > self.__parse_update(msg.params[1], OVSDB_UPDATE) > + elif self.handle_monitor_canceled(msg): > + break > + elif self.handle_monitor_cancel_reply(msg): > + break > elif (msg.type == ovs.jsonrpc.Message.T_REPLY > and self._monitor_request_id is not None > and self._monitor_request_id == msg.id): > @@ -615,6 +620,33 @@ class Idl(object): > > return initial_change_seqno != self.change_seqno > > + def handle_monitor_canceled(self, msg): > + if msg.type != msg.T_NOTIFY: > + return False > + if msg.method != "monitor_canceled": > + return False > + > + if msg.params[0] == str(self.uuid): > + params = [str(self.server_monitor_uuid)] > + elif msg.params[0] == str(self.server_monitor_uuid): > + params = [str(self.uuid)] > + else: > + return False > + > + mc_msg = ovs.jsonrpc.Message.create_request("monitor_cancel", params) > + self._monitor_cancel_request_id = mc_msg.id > + self.send_request(mc_msg) > + self.restart_fsm() > + return True > + > + def handle_monitor_cancel_reply(self, msg): > + if msg.type != msg.T_REPLY: > + return False > + if msg.id != self._monitor_cancel_request_id: > + return False > + self._monitor_cancel_request_id = None > + return True > + > def compose_cond_change(self): > if not self.cond_changed: > return
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 <horms@ovn.org> 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 <twilson@redhat.com> >>>> >>>> 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/20240108145248.1763075-1-i.maximets@ovn.org/ >> 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 >> >
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 <horms@ovn.org> 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 <twilson@redhat.com> > >>>> > >>>> 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/20240108145248.1763075-1-i.maximets@ovn.org/ > >> 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 ...
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 <horms@ovn.org> 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 <twilson@redhat.com> >>>>>> >>>>>> 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/20240108145248.1763075-1-i.maximets@ovn.org/ >>>> 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 > > ...
On Wed, Jan 10, 2024 at 06:38:48PM +0100, Ilya Maximets wrote: > 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 <horms@ovn.org> 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 <twilson@redhat.com> > >>>>>> > >>>>>> 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/20240108145248.1763075-1-i.maximets@ovn.org/ > >>>> 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. I knew I missed something from my previous email. It is there: * Fixes: c39751e ("python: Monitor Database table to manage lifecycle of IDL client.") For now the patches are queued to allow the GitHub actions to run. If that goes well I plan to push them. * https://github.com/horms/ovs/commits/series_387233%2B389371/ * https://github.com/horms/ovs/commits/branch-3.2-series_387233%2B389371/ * https://github.com/horms/ovs/commits/branch-3.1-series_387233%2B389371/ * https://github.com/horms/ovs/commits/branch-3.0-series_387233%2B389371/ * https://github.com/horms/ovs/commits/branch-2.17-series_387233%2B389371/
On Wed, Jan 10, 2024 at 05:49:58PM +0000, Simon Horman wrote: > On Wed, Jan 10, 2024 at 06:38:48PM +0100, Ilya Maximets wrote: > > On 1/10/24 18:18, Simon Horman wrote: > > > On Wed, Jan 10, 2024 at 12:28:22PM +0100, Dumitru Ceara wrote: ... > > >> 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. > > I knew I missed something from my previous email. It is there: > > * Fixes: c39751e ("python: Monitor Database table to manage lifecycle of IDL client.") ... Applied: - ovsdb-idl.at: Test IDL behavior during database conversion. https://github.com/openvswitch/ovs/commit/67ee6308781d - python: idl: Handle monitor_canceled. https://github.com/openvswitch/ovs/commit/ac04dfa7ec36 Backport to branch-3.2: - ovsdb-idl.at: Test IDL behavior during database conversion. https://github.com/openvswitch/ovs/commit/b3d094b4fb4e - python: idl: Handle monitor_canceled. https://github.com/openvswitch/ovs/commit/a1935e962876 Backport to branch-3.1 (with dependency): - ovsdb-idl.at: Test IDL behavior during database conversion. https://github.com/openvswitch/ovs/commit/dd3f0626ed16 - tests-ovsdb: Switch OVSDB_START_IDLTEST to macro. https://github.com/openvswitch/ovs/commit/2c6b81ad6e22 - python: idl: Handle monitor_canceled. https://github.com/openvswitch/ovs/commit/e9c5226cc296 Backport to branch-3.2 (with dependency): - ovsdb-idl.at: Test IDL behavior during database conversion. https://github.com/openvswitch/ovs/commit/28926225c422 - tests-ovsdb: Switch OVSDB_START_IDLTEST to macro. https://github.com/openvswitch/ovs/commit/2404ca561e94 - python: idl: Handle monitor_canceled. https://github.com/openvswitch/ovs/commit/219f1ebb0624 Backport to branch-2.17 (with dependencies): - ovsdb-idl.at: Test IDL behavior during database conversion. https://github.com/openvswitch/ovs/commit/9bbc2cf8a89e - tests: Use _DAEMONIZE macro's to start tcpdump. https://github.com/openvswitch/ovs/commit/049189584f5a - tests-ovsdb: Switch OVSDB_START_IDLTEST to macro. https://github.com/openvswitch/ovs/commit/30099c5d9ef7 - python: idl: Handle monitor_canceled. https://github.com/openvswitch/ovs/commit/f4b4d650a1e0
diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py index 9fc2159b0..be6ae2ca4 100644 --- a/python/ovs/db/idl.py +++ b/python/ovs/db/idl.py @@ -299,6 +299,7 @@ class Idl(object): self._server_schema_request_id = None self._server_monitor_request_id = None self._db_change_aware_request_id = None + self._monitor_cancel_request_id = None self._server_db_name = '_Server' self._server_db_table = 'Database' self.server_tables = None @@ -481,6 +482,10 @@ class Idl(object): break else: self.__parse_update(msg.params[1], OVSDB_UPDATE) + elif self.handle_monitor_canceled(msg): + break + elif self.handle_monitor_cancel_reply(msg): + break elif (msg.type == ovs.jsonrpc.Message.T_REPLY and self._monitor_request_id is not None and self._monitor_request_id == msg.id): @@ -615,6 +620,33 @@ class Idl(object): return initial_change_seqno != self.change_seqno + def handle_monitor_canceled(self, msg): + if msg.type != msg.T_NOTIFY: + return False + if msg.method != "monitor_canceled": + return False + + if msg.params[0] == str(self.uuid): + params = [str(self.server_monitor_uuid)] + elif msg.params[0] == str(self.server_monitor_uuid): + params = [str(self.uuid)] + else: + return False + + mc_msg = ovs.jsonrpc.Message.create_request("monitor_cancel", params) + self._monitor_cancel_request_id = mc_msg.id + self.send_request(mc_msg) + self.restart_fsm() + return True + + def handle_monitor_cancel_reply(self, msg): + if msg.type != msg.T_REPLY: + return False + if msg.id != self._monitor_cancel_request_id: + return False + self._monitor_cancel_request_id = None + return True + def compose_cond_change(self): if not self.cond_changed: return
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 <twilson@redhat.com> --- python/ovs/db/idl.py | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)