| Message ID | 20240123130328.891918-1-mheib@redhat.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [ovs-dev] Controller: Handle unconditional IDL messages while paused. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
Hi Mohammad, I'm having a hard time with this one, and mainly it's because of what the intended semantics of being "paused" are. I could see it argued that pausing ovn-controller should have the existing behavior. I could see someone writing a test where they pause ovn-controller specifically to trigger disconnection SB DB. My comments below have the mindset that this change is the correct behavior. But in addition to my comments, I think we need to verify if this proposed change is what we actually want ovn-controller to do. On 1/23/24 08:03, Mohammad Heib wrote: > If the user triggers a pause command to the ovn-controller the current > implementation will wait for commands from unixctl server only and > ignore the other component. > > This implementation works fine if we don't have inactivity_probe set in > the SB DataBase, but once the user sets the inactivity_probe in SB > DataBase the connection will be dropped by the SBDB. Once the controller > resumes the execution it will try to commit some changes to the SBDB but > the transaction will fail since we lost the connection to the SBDB and > the controller must reconnect before committing the transaction again. > > To avoid the above scenario the controller can keep handling > unconditional IDL messages to avoid reconnecting to SB. > > Signed-off-by: Mohammad Heib <mheib@redhat.com> > --- > controller/ovn-controller.c | 16 +++++++++--- > ovn-sb.xml | 2 +- > tests/ovn-controller.at | 51 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 65 insertions(+), 4 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 856e5e270..d2c8f66d9 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -5534,12 +5534,22 @@ main(int argc, char *argv[]) > simap_destroy(&usage); > } > > - /* If we're paused just run the unixctl server and skip most of the > - * processing loop. > + /* If we're paused just run the unixctl-server/unconditional IDL and > + * skip most of the processing loop. > */ > if (paused) { > unixctl_server_run(unixctl); > + int ovnsb_seq = ovsdb_idl_get_seqno(ovnsb_idl_loop.idl); > + ovsdb_idl_run(ovnsb_idl_loop.idl); > + int new_ovnsb_seq = ovsdb_idl_get_seqno(ovnsb_idl_loop.idl); > + /* If the IDL content has changed while the controller is > + * in pause state, trigger a recompute. > + */ > + if (new_ovnsb_seq != ovnsb_seq) { > + engine_set_force_recompute(true); > + } If we're going to safeguard ourselves from being disconnected from the SB DB, then we should do the same for the OVS DB. > unixctl_server_wait(unixctl); > + ovsdb_idl_wait(ovnsb_idl_loop.idl); > goto loop_done; > } > > @@ -6009,7 +6019,6 @@ main(int argc, char *argv[]) > OVS_NOT_REACHED(); > } > > - ovsdb_idl_track_clear(ovnsb_idl_loop.idl); > ovsdb_idl_track_clear(ovs_idl_loop.idl); > > lflow_cache_run(ctrl_engine_ctx.lflow_cache); > @@ -6017,6 +6026,7 @@ main(int argc, char *argv[]) > > loop_done: > memory_wait(); > + ovsdb_idl_track_clear(ovnsb_idl_loop.idl); > poll_block(); > if (should_service_stop()) { > exit_args.exiting = true; > diff --git a/ovn-sb.xml b/ovn-sb.xml > index e393f92b3..43c13f23c 100644 > --- a/ovn-sb.xml > +++ b/ovn-sb.xml > @@ -4308,7 +4308,7 @@ tcp.flags = RST; > <column name="inactivity_probe"> > Maximum number of milliseconds of idle time on connection to the client > before sending an inactivity probe message. If Open vSwitch does not > - communicate with the client for the specified number of seconds, it > + communicate with the client for the specified number of milliseconds,it Please put a space between the comma and "it". > will send a probe. If a response is not received for the same > additional amount of time, Open vSwitch assumes the connection has been > broken and attempts to reconnect. Default is implementation-specific. > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > index 9d2a37c72..04e4c52e7 100644 > --- a/tests/ovn-controller.at > +++ b/tests/ovn-controller.at > @@ -352,6 +352,57 @@ OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > AT_CLEANUP > ]) > > +# Check that the connection to the Southbound database > +# is not dropped when probe-interval is set and the controller > +# is in pause state. > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ovn-controller - check sbdb connection while pause]) > +AT_KEYWORDS([ovn]) > +ovn_start > + > +net_add n1 > +sim_add hv > +as hv > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > + > +ovs-vsctl -- add-port br-int hv1-vif1 -- \ > + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ > + options:tx_pcap=hv1/vif1-tx.pcap \ > + options:rxq_pcap=hv1/vif1-rx.pcap \ > + ofport-request=1 > + > +ovs-vsctl set open . external_ids:ovn-remote-probe-interval=100000 > +ovn-sbctl set connection . inactivity_probe=1 > + > +ovn-nbctl ls-add sw0 > +check ovn-nbctl lsp-add sw0 sw0-p1 > +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3" > +check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3" > +ovn-nbctl --wait=hv sync I think the above can be simplified. Since we're only interested in connectivity during a pause, we don't need to be creating any actual network topology. > + > +sleep_controller hv > +# Trigger DB change to make SBDB connect to controller. > +check ovn-nbctl lsp-del sw0-p1 Is this necessary? Wouldn't the SB DB connect to ovn-controller anyway without this line? > + > +# wait for 2 sec to give enough time to the SBDB to drop the connection > +# if there is no answer from the controller. The connection should not > +# be dropped since we keep handle the idl messages from SBDB even if we > +# in pause state. > +sleep 2 > +wake_up_controller hv > + > +check ovn-nbctl lsp-add sw0 sw0-p1 > +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3" > +check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3" > +ovn-nbctl --wait=hv sync I think the above four lines can be removed. > + > +OVS_WAIT_UNTIL([test 0 = `cat hv/ovn-controller.log| grep -c "OVNSB commit failed, force recompute next time"`]) > +OVS_WAIT_UNTIL([test 0 = `cat hv/ovn-controller.log| grep -c "connection closed by peer"`]) > +OVN_CLEANUP([hv]) > +AT_CLEANUP > +]) > + > # Checks that ovn-controller recreates its chassis record when deleted externally. > OVN_FOR_EACH_NORTHD([ > AT_SETUP([ovn-controller - Chassis self record])
Hi Mark, Thank you for your quick review, On Thu, Jan 25, 2024 at 12:40 AM Mark Michelson <mmichels@redhat.com> wrote: > Hi Mohammad, > > I'm having a hard time with this one, and mainly it's because of what > the intended semantics of being "paused" are. I could see it argued that > pausing ovn-controller should have the existing behavior. I could see > someone writing a test where they pause ovn-controller specifically to > trigger disconnection SB DB. > > > My comments below have the mindset that this change is the correct > behavior. But in addition to my comments, I think we need to verify if > this proposed change is what we actually want ovn-controller to do. > > i agree with you indeed this patch changed the controller behavior a little bit, but we need it to handle cases such as BZ [1]. This BZ is affected by similar scenarios that this patch is fixing: when we have the ovn-controller in a pause state some pinctrl pkt-in can be received on the ovn-controller and pinctrl thread will handle them and save the needed data for SBDB to temporary storage that will be removed each controller loop iteration (see run_put_vport_bindings in the pinctrl for example). so once the user triggers a resume to the ovn-controller the controller will try to commit this temporary data and the TXN will fail and we lose the needed data. The issue is present even if the controller wasn't on pause but the ovsdb server is a bit busy and TXN has failed. i will submit a separate patch to handle vport specific issue when TXN fails maybe keep the temporary data for a bit longer using timestamp but i think we must also keep answering the ovsdb server prob messages to avoid mult-reconnecting from the ovsdb server. What do you think? [1] https://bugzilla.redhat.com/show_bug.cgi?id=1954659 On 1/23/24 08:03, Mohammad Heib wrote: > > If the user triggers a pause command to the ovn-controller the current > > implementation will wait for commands from unixctl server only and > > ignore the other component. > > > > This implementation works fine if we don't have inactivity_probe set in > > the SB DataBase, but once the user sets the inactivity_probe in SB > > DataBase the connection will be dropped by the SBDB. Once the controller > > resumes the execution it will try to commit some changes to the SBDB but > > the transaction will fail since we lost the connection to the SBDB and > > the controller must reconnect before committing the transaction again. > > > > To avoid the above scenario the controller can keep handling > > unconditional IDL messages to avoid reconnecting to SB. > > > > Signed-off-by: Mohammad Heib <mheib@redhat.com> > > --- > > controller/ovn-controller.c | 16 +++++++++--- > > ovn-sb.xml | 2 +- > > tests/ovn-controller.at | 51 +++++++++++++++++++++++++++++++++++++ > > 3 files changed, 65 insertions(+), 4 deletions(-) > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 856e5e270..d2c8f66d9 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -5534,12 +5534,22 @@ main(int argc, char *argv[]) > > simap_destroy(&usage); > > } > > > > - /* If we're paused just run the unixctl server and skip most of > the > > - * processing loop. > > + /* If we're paused just run the unixctl-server/unconditional > IDL and > > + * skip most of the processing loop. > > */ > > if (paused) { > > unixctl_server_run(unixctl); > > + int ovnsb_seq = ovsdb_idl_get_seqno(ovnsb_idl_loop.idl); > > + ovsdb_idl_run(ovnsb_idl_loop.idl); > > + int new_ovnsb_seq = ovsdb_idl_get_seqno(ovnsb_idl_loop.idl); > > + /* If the IDL content has changed while the controller is > > + * in pause state, trigger a recompute. > > + */ > > + if (new_ovnsb_seq != ovnsb_seq) { > > + engine_set_force_recompute(true); > > + } > > If we're going to safeguard ourselves from being disconnected from the > SB DB, then we should do the same for the OVS DB. > > > unixctl_server_wait(unixctl); > > + ovsdb_idl_wait(ovnsb_idl_loop.idl); > > goto loop_done; > > } > > > > @@ -6009,7 +6019,6 @@ main(int argc, char *argv[]) > > OVS_NOT_REACHED(); > > } > > > > - ovsdb_idl_track_clear(ovnsb_idl_loop.idl); > > ovsdb_idl_track_clear(ovs_idl_loop.idl); > > > > lflow_cache_run(ctrl_engine_ctx.lflow_cache); > > @@ -6017,6 +6026,7 @@ main(int argc, char *argv[]) > > > > loop_done: > > memory_wait(); > > + ovsdb_idl_track_clear(ovnsb_idl_loop.idl); > > poll_block(); > > if (should_service_stop()) { > > exit_args.exiting = true; > > diff --git a/ovn-sb.xml b/ovn-sb.xml > > index e393f92b3..43c13f23c 100644 > > --- a/ovn-sb.xml > > +++ b/ovn-sb.xml > > @@ -4308,7 +4308,7 @@ tcp.flags = RST; > > <column name="inactivity_probe"> > > Maximum number of milliseconds of idle time on connection to > the client > > before sending an inactivity probe message. If Open vSwitch > does not > > - communicate with the client for the specified number of > seconds, it > > + communicate with the client for the specified number of > milliseconds,it > > Please put a space between the comma and "it". > > > will send a probe. If a response is not received for the same > > additional amount of time, Open vSwitch assumes the connection > has been > > broken and attempts to reconnect. Default is > implementation-specific. > > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > > index 9d2a37c72..04e4c52e7 100644 > > --- a/tests/ovn-controller.at > > +++ b/tests/ovn-controller.at > > @@ -352,6 +352,57 @@ OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > > AT_CLEANUP > > ]) > > > > +# Check that the connection to the Southbound database > > +# is not dropped when probe-interval is set and the controller > > +# is in pause state. > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([ovn-controller - check sbdb connection while pause]) > > +AT_KEYWORDS([ovn]) > > +ovn_start > > + > > +net_add n1 > > +sim_add hv > > +as hv > > +ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.1 > > + > > +ovs-vsctl -- add-port br-int hv1-vif1 -- \ > > + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ > > + options:tx_pcap=hv1/vif1-tx.pcap \ > > + options:rxq_pcap=hv1/vif1-rx.pcap \ > > + ofport-request=1 > > + > > +ovs-vsctl set open . external_ids:ovn-remote-probe-interval=100000 > > +ovn-sbctl set connection . inactivity_probe=1 > > + > > +ovn-nbctl ls-add sw0 > > +check ovn-nbctl lsp-add sw0 sw0-p1 > > +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3" > > +check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 > 10.0.0.3" > > +ovn-nbctl --wait=hv sync > > I think the above can be simplified. Since we're only interested in > connectivity during a pause, we don't need to be creating any actual > network topology. > > > + > > +sleep_controller hv > > +# Trigger DB change to make SBDB connect to controller. > > +check ovn-nbctl lsp-del sw0-p1 > > Is this necessary? Wouldn't the SB DB connect to ovn-controller anyway > without this line? > > > + > > +# wait for 2 sec to give enough time to the SBDB to drop the connection > > +# if there is no answer from the controller. The connection should not > > +# be dropped since we keep handle the idl messages from SBDB even if we > > +# in pause state. > > +sleep 2 > > +wake_up_controller hv > > + > > +check ovn-nbctl lsp-add sw0 sw0-p1 > > +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3" > > +check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 > 10.0.0.3" > > +ovn-nbctl --wait=hv sync > > I think the above four lines can be removed. > > > + > > +OVS_WAIT_UNTIL([test 0 = `cat hv/ovn-controller.log| grep -c "OVNSB > commit failed, force recompute next time"`]) > > +OVS_WAIT_UNTIL([test 0 = `cat hv/ovn-controller.log| grep -c > "connection closed by peer"`]) > > +OVN_CLEANUP([hv]) > > +AT_CLEANUP > > +]) > > + > > # Checks that ovn-controller recreates its chassis record when deleted > externally. > > OVN_FOR_EACH_NORTHD([ > > AT_SETUP([ovn-controller - Chassis self record]) > >
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 856e5e270..d2c8f66d9 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -5534,12 +5534,22 @@ main(int argc, char *argv[]) simap_destroy(&usage); } - /* If we're paused just run the unixctl server and skip most of the - * processing loop. + /* If we're paused just run the unixctl-server/unconditional IDL and + * skip most of the processing loop. */ if (paused) { unixctl_server_run(unixctl); + int ovnsb_seq = ovsdb_idl_get_seqno(ovnsb_idl_loop.idl); + ovsdb_idl_run(ovnsb_idl_loop.idl); + int new_ovnsb_seq = ovsdb_idl_get_seqno(ovnsb_idl_loop.idl); + /* If the IDL content has changed while the controller is + * in pause state, trigger a recompute. + */ + if (new_ovnsb_seq != ovnsb_seq) { + engine_set_force_recompute(true); + } unixctl_server_wait(unixctl); + ovsdb_idl_wait(ovnsb_idl_loop.idl); goto loop_done; } @@ -6009,7 +6019,6 @@ main(int argc, char *argv[]) OVS_NOT_REACHED(); } - ovsdb_idl_track_clear(ovnsb_idl_loop.idl); ovsdb_idl_track_clear(ovs_idl_loop.idl); lflow_cache_run(ctrl_engine_ctx.lflow_cache); @@ -6017,6 +6026,7 @@ main(int argc, char *argv[]) loop_done: memory_wait(); + ovsdb_idl_track_clear(ovnsb_idl_loop.idl); poll_block(); if (should_service_stop()) { exit_args.exiting = true; diff --git a/ovn-sb.xml b/ovn-sb.xml index e393f92b3..43c13f23c 100644 --- a/ovn-sb.xml +++ b/ovn-sb.xml @@ -4308,7 +4308,7 @@ tcp.flags = RST; <column name="inactivity_probe"> Maximum number of milliseconds of idle time on connection to the client before sending an inactivity probe message. If Open vSwitch does not - communicate with the client for the specified number of seconds, it + communicate with the client for the specified number of milliseconds,it will send a probe. If a response is not received for the same additional amount of time, Open vSwitch assumes the connection has been broken and attempts to reconnect. Default is implementation-specific. diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at index 9d2a37c72..04e4c52e7 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -352,6 +352,57 @@ OVS_APP_EXIT_AND_WAIT([ovsdb-server]) AT_CLEANUP ]) +# Check that the connection to the Southbound database +# is not dropped when probe-interval is set and the controller +# is in pause state. +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ovn-controller - check sbdb connection while pause]) +AT_KEYWORDS([ovn]) +ovn_start + +net_add n1 +sim_add hv +as hv +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.1 + +ovs-vsctl -- add-port br-int hv1-vif1 -- \ + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ + options:tx_pcap=hv1/vif1-tx.pcap \ + options:rxq_pcap=hv1/vif1-rx.pcap \ + ofport-request=1 + +ovs-vsctl set open . external_ids:ovn-remote-probe-interval=100000 +ovn-sbctl set connection . inactivity_probe=1 + +ovn-nbctl ls-add sw0 +check ovn-nbctl lsp-add sw0 sw0-p1 +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3" +check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3" +ovn-nbctl --wait=hv sync + +sleep_controller hv +# Trigger DB change to make SBDB connect to controller. +check ovn-nbctl lsp-del sw0-p1 + +# wait for 2 sec to give enough time to the SBDB to drop the connection +# if there is no answer from the controller. The connection should not +# be dropped since we keep handle the idl messages from SBDB even if we +# in pause state. +sleep 2 +wake_up_controller hv + +check ovn-nbctl lsp-add sw0 sw0-p1 +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3" +check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3" +ovn-nbctl --wait=hv sync + +OVS_WAIT_UNTIL([test 0 = `cat hv/ovn-controller.log| grep -c "OVNSB commit failed, force recompute next time"`]) +OVS_WAIT_UNTIL([test 0 = `cat hv/ovn-controller.log| grep -c "connection closed by peer"`]) +OVN_CLEANUP([hv]) +AT_CLEANUP +]) + # Checks that ovn-controller recreates its chassis record when deleted externally. OVN_FOR_EACH_NORTHD([ AT_SETUP([ovn-controller - Chassis self record])
If the user triggers a pause command to the ovn-controller the current implementation will wait for commands from unixctl server only and ignore the other component. This implementation works fine if we don't have inactivity_probe set in the SB DataBase, but once the user sets the inactivity_probe in SB DataBase the connection will be dropped by the SBDB. Once the controller resumes the execution it will try to commit some changes to the SBDB but the transaction will fail since we lost the connection to the SBDB and the controller must reconnect before committing the transaction again. To avoid the above scenario the controller can keep handling unconditional IDL messages to avoid reconnecting to SB. Signed-off-by: Mohammad Heib <mheib@redhat.com> --- controller/ovn-controller.c | 16 +++++++++--- ovn-sb.xml | 2 +- tests/ovn-controller.at | 51 +++++++++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 4 deletions(-)