| Message ID | 20201122144458.3857059-1-numans@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] controller: Allow pinctrl thread to handle packet-ins when version mismatch with northd. | expand |
Acked-by: Flavio Fernandes <flavio@flaviof.com <mailto:flavio@flaviof.com>> Tested-by: Flavio Fernandes <flavio@flaviof.com <mailto:flavio@flaviof.com>> > On Nov 22, 2020, at 9:44 AM, numans@ovn.org wrote: > > From: Numan Siddique <numans@ovn.org> > > The commit 1dd27ea7aea4 added the support to pin ovn-controller to a > specific version of ovn-northd. This patch did not handle the > scenario the packet-in scenario when ovn-controller is started and it > detects version mismatch with ovn-northd. In this case, pinctrl > thread is not configured with the proper integration bridge name, > because of which it cannot handle any packet-ins. > > This patch addresses this scenario. This is required so that any > existing VMs do not loose DHCP address during renewal. > > Fixes: 1dd27ea7aea4("Provide the option to pin ovn-controller and ovn-northd to a specific version") > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > controller/ovn-controller.c | 27 ++++++++++++----- > controller/pinctrl.c | 34 ++++++++++++++------- > controller/pinctrl.h | 2 +- > tests/ovn.at | 59 ++++++++++++++++++++++++++++++++++++- > 4 files changed, 102 insertions(+), 20 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 41e2678cf4..61de0af8d9 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -2648,25 +2648,29 @@ main(int argc, char *argv[]) > > engine_set_context(&eng_ctx); > > - if (ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl) && > + bool northd_version_match = > check_northd_version(ovs_idl_loop.idl, ovnsb_idl_loop.idl, > - ovn_version)) { > + ovn_version); > + > + const struct ovsrec_bridge_table *bridge_table = > + ovsrec_bridge_table_get(ovs_idl_loop.idl); > + const struct ovsrec_open_vswitch_table *ovs_table = > + ovsrec_open_vswitch_table_get(ovs_idl_loop.idl); > + const struct ovsrec_bridge *br_int = > + process_br_int(ovs_idl_txn, bridge_table, ovs_table); > + > + if (ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl) && > + northd_version_match) { > /* Contains the transport zones that this Chassis belongs to */ > struct sset transport_zones = SSET_INITIALIZER(&transport_zones); > sset_from_delimited_string(&transport_zones, > get_transport_zones(ovsrec_open_vswitch_table_get( > ovs_idl_loop.idl)), ","); > > - const struct ovsrec_bridge_table *bridge_table = > - ovsrec_bridge_table_get(ovs_idl_loop.idl); > - const struct ovsrec_open_vswitch_table *ovs_table = > - ovsrec_open_vswitch_table_get(ovs_idl_loop.idl); > const struct sbrec_chassis_table *chassis_table = > sbrec_chassis_table_get(ovnsb_idl_loop.idl); > const struct sbrec_chassis_private_table *chassis_pvt_table = > sbrec_chassis_private_table_get(ovnsb_idl_loop.idl); > - const struct ovsrec_bridge *br_int = > - process_br_int(ovs_idl_txn, bridge_table, ovs_table); > const char *chassis_id = get_ovs_chassis_id(ovs_table); > const struct sbrec_chassis *chassis = NULL; > const struct sbrec_chassis_private *chassis_private = NULL; > @@ -2836,6 +2840,13 @@ main(int argc, char *argv[]) > } > } > > + if (!northd_version_match && br_int) { > + /* Set the integration bridge name to pinctrl so that the pinctrl > + * thread can handle any packet-ins when we are not processing > + * any DB updates due to version mismatch. */ > + pinctrl_set_br_int_name(br_int->name); > + } > + > unixctl_server_run(unixctl); > > unixctl_server_wait(unixctl); > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index a5236564b4..c9dd5ea301 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -3114,6 +3114,29 @@ pinctrl_handler(void *arg_) > return NULL; > } > > +static void > +pinctrl_set_br_int_name_(char *br_int_name) > +{ > + if (br_int_name && (!pinctrl.br_int_name || strcmp(pinctrl.br_int_name, > + br_int_name))) { > + if (pinctrl.br_int_name) { > + free(pinctrl.br_int_name); > + } > + pinctrl.br_int_name = xstrdup(br_int_name); > + /* Notify pinctrl_handler that integration bridge is > + * set/changed. */ > + notify_pinctrl_handler(); > + } > +} > + > +void > +pinctrl_set_br_int_name(char *br_int_name) > +{ > + ovs_mutex_lock(&pinctrl_mutex); > + pinctrl_set_br_int_name_(br_int_name); > + ovs_mutex_unlock(&pinctrl_mutex); > +} > + > /* Called by ovn-controller. */ > void > pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > @@ -3133,16 +3156,7 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > const struct sset *active_tunnels) > { > ovs_mutex_lock(&pinctrl_mutex); > - if (br_int && (!pinctrl.br_int_name || strcmp(pinctrl.br_int_name, > - br_int->name))) { > - if (pinctrl.br_int_name) { > - free(pinctrl.br_int_name); > - } > - pinctrl.br_int_name = xstrdup(br_int->name); > - /* Notify pinctrl_handler that integration bridge is > - * set/changed. */ > - notify_pinctrl_handler(); > - } > + pinctrl_set_br_int_name_(br_int->name); > run_put_mac_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key, > sbrec_port_binding_by_key, > sbrec_mac_binding_by_lport_ip); > diff --git a/controller/pinctrl.h b/controller/pinctrl.h > index 8fa4baae9e..4b101ec925 100644 > --- a/controller/pinctrl.h > +++ b/controller/pinctrl.h > @@ -49,5 +49,5 @@ void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > const struct sset *active_tunnels); > void pinctrl_wait(struct ovsdb_idl_txn *ovnsb_idl_txn); > void pinctrl_destroy(void); > - > +void pinctrl_set_br_int_name(char *br_int_name); > #endif /* controller/pinctrl.h */ > diff --git a/tests/ovn.at b/tests/ovn.at > index b322b681af..e682f9edf4 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -6008,7 +6008,64 @@ expected_dhcp_opts=${boofile}330400000e100104ffffff0003040a00000136040a000001 > test_dhcp 19 1 f00000000001 01 0 $ciaddr $offer_ip $request_ip 1 0 ff1000000001 $server_ip 02 $expected_dhcp_opts > compare_dhcp_packets 1 > > -OVN_CLEANUP([hv1]) > +# Test that ovn-controller pinctrl thread handles dhcp requests when it > +# connects to a wrong version of ovn-northd at startup. > + > +# Stop ovn-northd so that we can modify the northd_version. > +as northd > +OVS_APP_EXIT_AND_WAIT([ovn-northd]) > + > +as northd-backup > +OVS_APP_EXIT_AND_WAIT([ovn-northd]) > + > +northd_version=$(ovn-sbctl get SB_Global . options:northd_internal_version | sed s/\"//g) > +echo "northd version = $northd_version" > + > +check ovn-sbctl set SB_Global . options:northd_internal_version=foo > + > +echo > +echo "__file__:__line__: Stop ovn-controller." > +as hv1 > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > + > +echo > +echo "__file__:__line__: Pin ovn-controller to ovn-northd version." > + > +as hv1 > +check ovs-vsctl set open . external_ids:ovn-match-northd-version=true > + > +# Start ovn-controller > +as hv1 > +start_daemon ovn-controller > + > +OVS_WAIT_UNTIL( > + [test 1 = $(grep -c "controller version - $northd_version mismatch with northd version - foo" hv1/ovn-controller.log) > +]) > + > +reset_pcap_file hv1-vif1 hv1/vif1 > +reset_pcap_file hv1-vif2 hv1/vif2 > +rm -f 1.expected > +rm -f 2.expected > + > +# ---------------------------------------------------------------------- > + > +offer_ip=`ip_to_hex 10 0 0 4` > +server_ip=`ip_to_hex 10 0 0 1` > +ciaddr=`ip_to_hex 0 0 0 0` > +request_ip=0 > +boofile=4308626f6f7466696c65 > +expected_dhcp_opts=${boofile}330400000e100104ffffff0003040a00000136040a000001 > +test_dhcp 20 1 f00000000001 01 0 $ciaddr $offer_ip $request_ip 1 0 ff1000000001 $server_ip 02 $expected_dhcp_opts > +compare_dhcp_packets 1 > + > +as hv1 > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > + > +as ovn-sb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as ovn-nb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > > AT_CLEANUP > > -- > 2.28.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Acked-by: Flavio Fernandes <flavio@flaviof.com <mailto:flavio@flaviof.com>> > On Nov 22, 2020, at 9:44 AM, numans@ovn.org wrote: > > From: Numan Siddique <numans@ovn.org> > > The commit 1dd27ea7aea4 added the support to pin ovn-controller to a > specific version of ovn-northd. This patch did not handle the > scenario the packet-in scenario when ovn-controller is started and it > detects version mismatch with ovn-northd. In this case, pinctrl > thread is not configured with the proper integration bridge name, > because of which it cannot handle any packet-ins. > > This patch addresses this scenario. This is required so that any > existing VMs do not loose DHCP address during renewal. > > Fixes: 1dd27ea7aea4("Provide the option to pin ovn-controller and ovn-northd to a specific version") > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > controller/ovn-controller.c | 27 ++++++++++++----- > controller/pinctrl.c | 34 ++++++++++++++------- > controller/pinctrl.h | 2 +- > tests/ovn.at | 59 ++++++++++++++++++++++++++++++++++++- > 4 files changed, 102 insertions(+), 20 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 41e2678cf4..61de0af8d9 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -2648,25 +2648,29 @@ main(int argc, char *argv[]) > > engine_set_context(&eng_ctx); > > - if (ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl) && > + bool northd_version_match = > check_northd_version(ovs_idl_loop.idl, ovnsb_idl_loop.idl, > - ovn_version)) { > + ovn_version); > + > + const struct ovsrec_bridge_table *bridge_table = > + ovsrec_bridge_table_get(ovs_idl_loop.idl); > + const struct ovsrec_open_vswitch_table *ovs_table = > + ovsrec_open_vswitch_table_get(ovs_idl_loop.idl); > + const struct ovsrec_bridge *br_int = > + process_br_int(ovs_idl_txn, bridge_table, ovs_table); > + > + if (ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl) && > + northd_version_match) { > /* Contains the transport zones that this Chassis belongs to */ > struct sset transport_zones = SSET_INITIALIZER(&transport_zones); > sset_from_delimited_string(&transport_zones, > get_transport_zones(ovsrec_open_vswitch_table_get( > ovs_idl_loop.idl)), ","); > > - const struct ovsrec_bridge_table *bridge_table = > - ovsrec_bridge_table_get(ovs_idl_loop.idl); > - const struct ovsrec_open_vswitch_table *ovs_table = > - ovsrec_open_vswitch_table_get(ovs_idl_loop.idl); > const struct sbrec_chassis_table *chassis_table = > sbrec_chassis_table_get(ovnsb_idl_loop.idl); > const struct sbrec_chassis_private_table *chassis_pvt_table = > sbrec_chassis_private_table_get(ovnsb_idl_loop.idl); > - const struct ovsrec_bridge *br_int = > - process_br_int(ovs_idl_txn, bridge_table, ovs_table); > const char *chassis_id = get_ovs_chassis_id(ovs_table); > const struct sbrec_chassis *chassis = NULL; > const struct sbrec_chassis_private *chassis_private = NULL; > @@ -2836,6 +2840,13 @@ main(int argc, char *argv[]) > } > } > > + if (!northd_version_match && br_int) { > + /* Set the integration bridge name to pinctrl so that the pinctrl > + * thread can handle any packet-ins when we are not processing > + * any DB updates due to version mismatch. */ > + pinctrl_set_br_int_name(br_int->name); > + } > + > unixctl_server_run(unixctl); > > unixctl_server_wait(unixctl); > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index a5236564b4..c9dd5ea301 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -3114,6 +3114,29 @@ pinctrl_handler(void *arg_) > return NULL; > } > > +static void > +pinctrl_set_br_int_name_(char *br_int_name) > +{ > + if (br_int_name && (!pinctrl.br_int_name || strcmp(pinctrl.br_int_name, > + br_int_name))) { > + if (pinctrl.br_int_name) { > + free(pinctrl.br_int_name); > + } > + pinctrl.br_int_name = xstrdup(br_int_name); > + /* Notify pinctrl_handler that integration bridge is > + * set/changed. */ > + notify_pinctrl_handler(); > + } > +} > + > +void > +pinctrl_set_br_int_name(char *br_int_name) > +{ > + ovs_mutex_lock(&pinctrl_mutex); > + pinctrl_set_br_int_name_(br_int_name); > + ovs_mutex_unlock(&pinctrl_mutex); > +} > + > /* Called by ovn-controller. */ > void > pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > @@ -3133,16 +3156,7 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > const struct sset *active_tunnels) > { > ovs_mutex_lock(&pinctrl_mutex); > - if (br_int && (!pinctrl.br_int_name || strcmp(pinctrl.br_int_name, > - br_int->name))) { > - if (pinctrl.br_int_name) { > - free(pinctrl.br_int_name); > - } > - pinctrl.br_int_name = xstrdup(br_int->name); > - /* Notify pinctrl_handler that integration bridge is > - * set/changed. */ > - notify_pinctrl_handler(); > - } > + pinctrl_set_br_int_name_(br_int->name); > run_put_mac_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key, > sbrec_port_binding_by_key, > sbrec_mac_binding_by_lport_ip); > diff --git a/controller/pinctrl.h b/controller/pinctrl.h > index 8fa4baae9e..4b101ec925 100644 > --- a/controller/pinctrl.h > +++ b/controller/pinctrl.h > @@ -49,5 +49,5 @@ void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > const struct sset *active_tunnels); > void pinctrl_wait(struct ovsdb_idl_txn *ovnsb_idl_txn); > void pinctrl_destroy(void); > - > +void pinctrl_set_br_int_name(char *br_int_name); > #endif /* controller/pinctrl.h */ > diff --git a/tests/ovn.at b/tests/ovn.at > index b322b681af..e682f9edf4 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -6008,7 +6008,64 @@ expected_dhcp_opts=${boofile}330400000e100104ffffff0003040a00000136040a000001 > test_dhcp 19 1 f00000000001 01 0 $ciaddr $offer_ip $request_ip 1 0 ff1000000001 $server_ip 02 $expected_dhcp_opts > compare_dhcp_packets 1 > > -OVN_CLEANUP([hv1]) > +# Test that ovn-controller pinctrl thread handles dhcp requests when it > +# connects to a wrong version of ovn-northd at startup. > + > +# Stop ovn-northd so that we can modify the northd_version. > +as northd > +OVS_APP_EXIT_AND_WAIT([ovn-northd]) > + > +as northd-backup > +OVS_APP_EXIT_AND_WAIT([ovn-northd]) > + > +northd_version=$(ovn-sbctl get SB_Global . options:northd_internal_version | sed s/\"//g) > +echo "northd version = $northd_version" > + > +check ovn-sbctl set SB_Global . options:northd_internal_version=foo > + > +echo > +echo "__file__:__line__: Stop ovn-controller." > +as hv1 > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > + > +echo > +echo "__file__:__line__: Pin ovn-controller to ovn-northd version." > + > +as hv1 > +check ovs-vsctl set open . external_ids:ovn-match-northd-version=true > + > +# Start ovn-controller > +as hv1 > +start_daemon ovn-controller > + > +OVS_WAIT_UNTIL( > + [test 1 = $(grep -c "controller version - $northd_version mismatch with northd version - foo" hv1/ovn-controller.log) > +]) > + > +reset_pcap_file hv1-vif1 hv1/vif1 > +reset_pcap_file hv1-vif2 hv1/vif2 > +rm -f 1.expected > +rm -f 2.expected > + > +# ---------------------------------------------------------------------- > + > +offer_ip=`ip_to_hex 10 0 0 4` > +server_ip=`ip_to_hex 10 0 0 1` > +ciaddr=`ip_to_hex 0 0 0 0` > +request_ip=0 > +boofile=4308626f6f7466696c65 > +expected_dhcp_opts=${boofile}330400000e100104ffffff0003040a00000136040a000001 > +test_dhcp 20 1 f00000000001 01 0 $ciaddr $offer_ip $request_ip 1 0 ff1000000001 $server_ip 02 $expected_dhcp_opts > +compare_dhcp_packets 1 > + > +as hv1 > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > + > +as ovn-sb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as ovn-nb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > > AT_CLEANUP > > -- > 2.28.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Tested-by: Flavio Fernandes <flavio@flaviof.com <mailto:flavio@flaviof.com>> > On Nov 22, 2020, at 9:44 AM, numans@ovn.org wrote: > > From: Numan Siddique <numans@ovn.org> > > The commit 1dd27ea7aea4 added the support to pin ovn-controller to a > specific version of ovn-northd. This patch did not handle the > scenario the packet-in scenario when ovn-controller is started and it > detects version mismatch with ovn-northd. In this case, pinctrl > thread is not configured with the proper integration bridge name, > because of which it cannot handle any packet-ins. > > This patch addresses this scenario. This is required so that any > existing VMs do not loose DHCP address during renewal. > > Fixes: 1dd27ea7aea4("Provide the option to pin ovn-controller and ovn-northd to a specific version") > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > controller/ovn-controller.c | 27 ++++++++++++----- > controller/pinctrl.c | 34 ++++++++++++++------- > controller/pinctrl.h | 2 +- > tests/ovn.at | 59 ++++++++++++++++++++++++++++++++++++- > 4 files changed, 102 insertions(+), 20 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 41e2678cf4..61de0af8d9 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -2648,25 +2648,29 @@ main(int argc, char *argv[]) > > engine_set_context(&eng_ctx); > > - if (ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl) && > + bool northd_version_match = > check_northd_version(ovs_idl_loop.idl, ovnsb_idl_loop.idl, > - ovn_version)) { > + ovn_version); > + > + const struct ovsrec_bridge_table *bridge_table = > + ovsrec_bridge_table_get(ovs_idl_loop.idl); > + const struct ovsrec_open_vswitch_table *ovs_table = > + ovsrec_open_vswitch_table_get(ovs_idl_loop.idl); > + const struct ovsrec_bridge *br_int = > + process_br_int(ovs_idl_txn, bridge_table, ovs_table); > + > + if (ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl) && > + northd_version_match) { > /* Contains the transport zones that this Chassis belongs to */ > struct sset transport_zones = SSET_INITIALIZER(&transport_zones); > sset_from_delimited_string(&transport_zones, > get_transport_zones(ovsrec_open_vswitch_table_get( > ovs_idl_loop.idl)), ","); > > - const struct ovsrec_bridge_table *bridge_table = > - ovsrec_bridge_table_get(ovs_idl_loop.idl); > - const struct ovsrec_open_vswitch_table *ovs_table = > - ovsrec_open_vswitch_table_get(ovs_idl_loop.idl); > const struct sbrec_chassis_table *chassis_table = > sbrec_chassis_table_get(ovnsb_idl_loop.idl); > const struct sbrec_chassis_private_table *chassis_pvt_table = > sbrec_chassis_private_table_get(ovnsb_idl_loop.idl); > - const struct ovsrec_bridge *br_int = > - process_br_int(ovs_idl_txn, bridge_table, ovs_table); > const char *chassis_id = get_ovs_chassis_id(ovs_table); > const struct sbrec_chassis *chassis = NULL; > const struct sbrec_chassis_private *chassis_private = NULL; > @@ -2836,6 +2840,13 @@ main(int argc, char *argv[]) > } > } > > + if (!northd_version_match && br_int) { > + /* Set the integration bridge name to pinctrl so that the pinctrl > + * thread can handle any packet-ins when we are not processing > + * any DB updates due to version mismatch. */ > + pinctrl_set_br_int_name(br_int->name); > + } > + > unixctl_server_run(unixctl); > > unixctl_server_wait(unixctl); > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index a5236564b4..c9dd5ea301 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -3114,6 +3114,29 @@ pinctrl_handler(void *arg_) > return NULL; > } > > +static void > +pinctrl_set_br_int_name_(char *br_int_name) > +{ > + if (br_int_name && (!pinctrl.br_int_name || strcmp(pinctrl.br_int_name, > + br_int_name))) { > + if (pinctrl.br_int_name) { > + free(pinctrl.br_int_name); > + } > + pinctrl.br_int_name = xstrdup(br_int_name); > + /* Notify pinctrl_handler that integration bridge is > + * set/changed. */ > + notify_pinctrl_handler(); > + } > +} > + > +void > +pinctrl_set_br_int_name(char *br_int_name) > +{ > + ovs_mutex_lock(&pinctrl_mutex); > + pinctrl_set_br_int_name_(br_int_name); > + ovs_mutex_unlock(&pinctrl_mutex); > +} > + > /* Called by ovn-controller. */ > void > pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > @@ -3133,16 +3156,7 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > const struct sset *active_tunnels) > { > ovs_mutex_lock(&pinctrl_mutex); > - if (br_int && (!pinctrl.br_int_name || strcmp(pinctrl.br_int_name, > - br_int->name))) { > - if (pinctrl.br_int_name) { > - free(pinctrl.br_int_name); > - } > - pinctrl.br_int_name = xstrdup(br_int->name); > - /* Notify pinctrl_handler that integration bridge is > - * set/changed. */ > - notify_pinctrl_handler(); > - } > + pinctrl_set_br_int_name_(br_int->name); > run_put_mac_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key, > sbrec_port_binding_by_key, > sbrec_mac_binding_by_lport_ip); > diff --git a/controller/pinctrl.h b/controller/pinctrl.h > index 8fa4baae9e..4b101ec925 100644 > --- a/controller/pinctrl.h > +++ b/controller/pinctrl.h > @@ -49,5 +49,5 @@ void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > const struct sset *active_tunnels); > void pinctrl_wait(struct ovsdb_idl_txn *ovnsb_idl_txn); > void pinctrl_destroy(void); > - > +void pinctrl_set_br_int_name(char *br_int_name); > #endif /* controller/pinctrl.h */ > diff --git a/tests/ovn.at b/tests/ovn.at > index b322b681af..e682f9edf4 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -6008,7 +6008,64 @@ expected_dhcp_opts=${boofile}330400000e100104ffffff0003040a00000136040a000001 > test_dhcp 19 1 f00000000001 01 0 $ciaddr $offer_ip $request_ip 1 0 ff1000000001 $server_ip 02 $expected_dhcp_opts > compare_dhcp_packets 1 > > -OVN_CLEANUP([hv1]) > +# Test that ovn-controller pinctrl thread handles dhcp requests when it > +# connects to a wrong version of ovn-northd at startup. > + > +# Stop ovn-northd so that we can modify the northd_version. > +as northd > +OVS_APP_EXIT_AND_WAIT([ovn-northd]) > + > +as northd-backup > +OVS_APP_EXIT_AND_WAIT([ovn-northd]) > + > +northd_version=$(ovn-sbctl get SB_Global . options:northd_internal_version | sed s/\"//g) > +echo "northd version = $northd_version" > + > +check ovn-sbctl set SB_Global . options:northd_internal_version=foo > + > +echo > +echo "__file__:__line__: Stop ovn-controller." > +as hv1 > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > + > +echo > +echo "__file__:__line__: Pin ovn-controller to ovn-northd version." > + > +as hv1 > +check ovs-vsctl set open . external_ids:ovn-match-northd-version=true > + > +# Start ovn-controller > +as hv1 > +start_daemon ovn-controller > + > +OVS_WAIT_UNTIL( > + [test 1 = $(grep -c "controller version - $northd_version mismatch with northd version - foo" hv1/ovn-controller.log) > +]) > + > +reset_pcap_file hv1-vif1 hv1/vif1 > +reset_pcap_file hv1-vif2 hv1/vif2 > +rm -f 1.expected > +rm -f 2.expected > + > +# ---------------------------------------------------------------------- > + > +offer_ip=`ip_to_hex 10 0 0 4` > +server_ip=`ip_to_hex 10 0 0 1` > +ciaddr=`ip_to_hex 0 0 0 0` > +request_ip=0 > +boofile=4308626f6f7466696c65 > +expected_dhcp_opts=${boofile}330400000e100104ffffff0003040a00000136040a000001 > +test_dhcp 20 1 f00000000001 01 0 $ciaddr $offer_ip $request_ip 1 0 ff1000000001 $server_ip 02 $expected_dhcp_opts > +compare_dhcp_packets 1 > + > +as hv1 > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > + > +as ovn-sb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as ovn-nb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > > AT_CLEANUP > > -- > 2.28.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Mon, Nov 23, 2020 at 4:09 AM Flavio Fernandes <flavio@flaviof.com> wrote: > > Acked-by: Flavio Fernandes <flavio@flaviof.com <mailto:flavio@flaviof.com>> > Tested-by: Flavio Fernandes <flavio@flaviof.com <mailto:flavio@flaviof.com>> Thanks a lot for the review and testing it. I applied this patch to master. Numan > > > > On Nov 22, 2020, at 9:44 AM, numans@ovn.org wrote: > > > > From: Numan Siddique <numans@ovn.org> > > > > The commit 1dd27ea7aea4 added the support to pin ovn-controller to a > > specific version of ovn-northd. This patch did not handle the > > scenario the packet-in scenario when ovn-controller is started and it > > detects version mismatch with ovn-northd. In this case, pinctrl > > thread is not configured with the proper integration bridge name, > > because of which it cannot handle any packet-ins. > > > > This patch addresses this scenario. This is required so that any > > existing VMs do not loose DHCP address during renewal. > > > > Fixes: 1dd27ea7aea4("Provide the option to pin ovn-controller and ovn-northd to a specific version") > > Signed-off-by: Numan Siddique <numans@ovn.org> > > --- > > controller/ovn-controller.c | 27 ++++++++++++----- > > controller/pinctrl.c | 34 ++++++++++++++------- > > controller/pinctrl.h | 2 +- > > tests/ovn.at | 59 ++++++++++++++++++++++++++++++++++++- > > 4 files changed, 102 insertions(+), 20 deletions(-) > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 41e2678cf4..61de0af8d9 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -2648,25 +2648,29 @@ main(int argc, char *argv[]) > > > > engine_set_context(&eng_ctx); > > > > - if (ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl) && > > + bool northd_version_match = > > check_northd_version(ovs_idl_loop.idl, ovnsb_idl_loop.idl, > > - ovn_version)) { > > + ovn_version); > > + > > + const struct ovsrec_bridge_table *bridge_table = > > + ovsrec_bridge_table_get(ovs_idl_loop.idl); > > + const struct ovsrec_open_vswitch_table *ovs_table = > > + ovsrec_open_vswitch_table_get(ovs_idl_loop.idl); > > + const struct ovsrec_bridge *br_int = > > + process_br_int(ovs_idl_txn, bridge_table, ovs_table); > > + > > + if (ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl) && > > + northd_version_match) { > > /* Contains the transport zones that this Chassis belongs to */ > > struct sset transport_zones = SSET_INITIALIZER(&transport_zones); > > sset_from_delimited_string(&transport_zones, > > get_transport_zones(ovsrec_open_vswitch_table_get( > > ovs_idl_loop.idl)), ","); > > > > - const struct ovsrec_bridge_table *bridge_table = > > - ovsrec_bridge_table_get(ovs_idl_loop.idl); > > - const struct ovsrec_open_vswitch_table *ovs_table = > > - ovsrec_open_vswitch_table_get(ovs_idl_loop.idl); > > const struct sbrec_chassis_table *chassis_table = > > sbrec_chassis_table_get(ovnsb_idl_loop.idl); > > const struct sbrec_chassis_private_table *chassis_pvt_table = > > sbrec_chassis_private_table_get(ovnsb_idl_loop.idl); > > - const struct ovsrec_bridge *br_int = > > - process_br_int(ovs_idl_txn, bridge_table, ovs_table); > > const char *chassis_id = get_ovs_chassis_id(ovs_table); > > const struct sbrec_chassis *chassis = NULL; > > const struct sbrec_chassis_private *chassis_private = NULL; > > @@ -2836,6 +2840,13 @@ main(int argc, char *argv[]) > > } > > } > > > > + if (!northd_version_match && br_int) { > > + /* Set the integration bridge name to pinctrl so that the pinctrl > > + * thread can handle any packet-ins when we are not processing > > + * any DB updates due to version mismatch. */ > > + pinctrl_set_br_int_name(br_int->name); > > + } > > + > > unixctl_server_run(unixctl); > > > > unixctl_server_wait(unixctl); > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > > index a5236564b4..c9dd5ea301 100644 > > --- a/controller/pinctrl.c > > +++ b/controller/pinctrl.c > > @@ -3114,6 +3114,29 @@ pinctrl_handler(void *arg_) > > return NULL; > > } > > > > +static void > > +pinctrl_set_br_int_name_(char *br_int_name) > > +{ > > + if (br_int_name && (!pinctrl.br_int_name || strcmp(pinctrl.br_int_name, > > + br_int_name))) { > > + if (pinctrl.br_int_name) { > > + free(pinctrl.br_int_name); > > + } > > + pinctrl.br_int_name = xstrdup(br_int_name); > > + /* Notify pinctrl_handler that integration bridge is > > + * set/changed. */ > > + notify_pinctrl_handler(); > > + } > > +} > > + > > +void > > +pinctrl_set_br_int_name(char *br_int_name) > > +{ > > + ovs_mutex_lock(&pinctrl_mutex); > > + pinctrl_set_br_int_name_(br_int_name); > > + ovs_mutex_unlock(&pinctrl_mutex); > > +} > > + > > /* Called by ovn-controller. */ > > void > > pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > > @@ -3133,16 +3156,7 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > > const struct sset *active_tunnels) > > { > > ovs_mutex_lock(&pinctrl_mutex); > > - if (br_int && (!pinctrl.br_int_name || strcmp(pinctrl.br_int_name, > > - br_int->name))) { > > - if (pinctrl.br_int_name) { > > - free(pinctrl.br_int_name); > > - } > > - pinctrl.br_int_name = xstrdup(br_int->name); > > - /* Notify pinctrl_handler that integration bridge is > > - * set/changed. */ > > - notify_pinctrl_handler(); > > - } > > + pinctrl_set_br_int_name_(br_int->name); > > run_put_mac_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key, > > sbrec_port_binding_by_key, > > sbrec_mac_binding_by_lport_ip); > > diff --git a/controller/pinctrl.h b/controller/pinctrl.h > > index 8fa4baae9e..4b101ec925 100644 > > --- a/controller/pinctrl.h > > +++ b/controller/pinctrl.h > > @@ -49,5 +49,5 @@ void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > > const struct sset *active_tunnels); > > void pinctrl_wait(struct ovsdb_idl_txn *ovnsb_idl_txn); > > void pinctrl_destroy(void); > > - > > +void pinctrl_set_br_int_name(char *br_int_name); > > #endif /* controller/pinctrl.h */ > > diff --git a/tests/ovn.at b/tests/ovn.at > > index b322b681af..e682f9edf4 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -6008,7 +6008,64 @@ expected_dhcp_opts=${boofile}330400000e100104ffffff0003040a00000136040a000001 > > test_dhcp 19 1 f00000000001 01 0 $ciaddr $offer_ip $request_ip 1 0 ff1000000001 $server_ip 02 $expected_dhcp_opts > > compare_dhcp_packets 1 > > > > -OVN_CLEANUP([hv1]) > > +# Test that ovn-controller pinctrl thread handles dhcp requests when it > > +# connects to a wrong version of ovn-northd at startup. > > + > > +# Stop ovn-northd so that we can modify the northd_version. > > +as northd > > +OVS_APP_EXIT_AND_WAIT([ovn-northd]) > > + > > +as northd-backup > > +OVS_APP_EXIT_AND_WAIT([ovn-northd]) > > + > > +northd_version=$(ovn-sbctl get SB_Global . options:northd_internal_version | sed s/\"//g) > > +echo "northd version = $northd_version" > > + > > +check ovn-sbctl set SB_Global . options:northd_internal_version=foo > > + > > +echo > > +echo "__file__:__line__: Stop ovn-controller." > > +as hv1 > > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > > + > > +echo > > +echo "__file__:__line__: Pin ovn-controller to ovn-northd version." > > + > > +as hv1 > > +check ovs-vsctl set open . external_ids:ovn-match-northd-version=true > > + > > +# Start ovn-controller > > +as hv1 > > +start_daemon ovn-controller > > + > > +OVS_WAIT_UNTIL( > > + [test 1 = $(grep -c "controller version - $northd_version mismatch with northd version - foo" hv1/ovn-controller.log) > > +]) > > + > > +reset_pcap_file hv1-vif1 hv1/vif1 > > +reset_pcap_file hv1-vif2 hv1/vif2 > > +rm -f 1.expected > > +rm -f 2.expected > > + > > +# ---------------------------------------------------------------------- > > + > > +offer_ip=`ip_to_hex 10 0 0 4` > > +server_ip=`ip_to_hex 10 0 0 1` > > +ciaddr=`ip_to_hex 0 0 0 0` > > +request_ip=0 > > +boofile=4308626f6f7466696c65 > > +expected_dhcp_opts=${boofile}330400000e100104ffffff0003040a00000136040a000001 > > +test_dhcp 20 1 f00000000001 01 0 $ciaddr $offer_ip $request_ip 1 0 ff1000000001 $server_ip 02 $expected_dhcp_opts > > +compare_dhcp_packets 1 > > + > > +as hv1 > > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > > + > > +as ovn-sb > > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > > + > > +as ovn-nb > > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > > > > AT_CLEANUP > > > > -- > > 2.28.0 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 41e2678cf4..61de0af8d9 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -2648,25 +2648,29 @@ main(int argc, char *argv[]) engine_set_context(&eng_ctx); - if (ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl) && + bool northd_version_match = check_northd_version(ovs_idl_loop.idl, ovnsb_idl_loop.idl, - ovn_version)) { + ovn_version); + + const struct ovsrec_bridge_table *bridge_table = + ovsrec_bridge_table_get(ovs_idl_loop.idl); + const struct ovsrec_open_vswitch_table *ovs_table = + ovsrec_open_vswitch_table_get(ovs_idl_loop.idl); + const struct ovsrec_bridge *br_int = + process_br_int(ovs_idl_txn, bridge_table, ovs_table); + + if (ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl) && + northd_version_match) { /* Contains the transport zones that this Chassis belongs to */ struct sset transport_zones = SSET_INITIALIZER(&transport_zones); sset_from_delimited_string(&transport_zones, get_transport_zones(ovsrec_open_vswitch_table_get( ovs_idl_loop.idl)), ","); - const struct ovsrec_bridge_table *bridge_table = - ovsrec_bridge_table_get(ovs_idl_loop.idl); - const struct ovsrec_open_vswitch_table *ovs_table = - ovsrec_open_vswitch_table_get(ovs_idl_loop.idl); const struct sbrec_chassis_table *chassis_table = sbrec_chassis_table_get(ovnsb_idl_loop.idl); const struct sbrec_chassis_private_table *chassis_pvt_table = sbrec_chassis_private_table_get(ovnsb_idl_loop.idl); - const struct ovsrec_bridge *br_int = - process_br_int(ovs_idl_txn, bridge_table, ovs_table); const char *chassis_id = get_ovs_chassis_id(ovs_table); const struct sbrec_chassis *chassis = NULL; const struct sbrec_chassis_private *chassis_private = NULL; @@ -2836,6 +2840,13 @@ main(int argc, char *argv[]) } } + if (!northd_version_match && br_int) { + /* Set the integration bridge name to pinctrl so that the pinctrl + * thread can handle any packet-ins when we are not processing + * any DB updates due to version mismatch. */ + pinctrl_set_br_int_name(br_int->name); + } + unixctl_server_run(unixctl); unixctl_server_wait(unixctl); diff --git a/controller/pinctrl.c b/controller/pinctrl.c index a5236564b4..c9dd5ea301 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -3114,6 +3114,29 @@ pinctrl_handler(void *arg_) return NULL; } +static void +pinctrl_set_br_int_name_(char *br_int_name) +{ + if (br_int_name && (!pinctrl.br_int_name || strcmp(pinctrl.br_int_name, + br_int_name))) { + if (pinctrl.br_int_name) { + free(pinctrl.br_int_name); + } + pinctrl.br_int_name = xstrdup(br_int_name); + /* Notify pinctrl_handler that integration bridge is + * set/changed. */ + notify_pinctrl_handler(); + } +} + +void +pinctrl_set_br_int_name(char *br_int_name) +{ + ovs_mutex_lock(&pinctrl_mutex); + pinctrl_set_br_int_name_(br_int_name); + ovs_mutex_unlock(&pinctrl_mutex); +} + /* Called by ovn-controller. */ void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, @@ -3133,16 +3156,7 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, const struct sset *active_tunnels) { ovs_mutex_lock(&pinctrl_mutex); - if (br_int && (!pinctrl.br_int_name || strcmp(pinctrl.br_int_name, - br_int->name))) { - if (pinctrl.br_int_name) { - free(pinctrl.br_int_name); - } - pinctrl.br_int_name = xstrdup(br_int->name); - /* Notify pinctrl_handler that integration bridge is - * set/changed. */ - notify_pinctrl_handler(); - } + pinctrl_set_br_int_name_(br_int->name); run_put_mac_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key, sbrec_port_binding_by_key, sbrec_mac_binding_by_lport_ip); diff --git a/controller/pinctrl.h b/controller/pinctrl.h index 8fa4baae9e..4b101ec925 100644 --- a/controller/pinctrl.h +++ b/controller/pinctrl.h @@ -49,5 +49,5 @@ void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, const struct sset *active_tunnels); void pinctrl_wait(struct ovsdb_idl_txn *ovnsb_idl_txn); void pinctrl_destroy(void); - +void pinctrl_set_br_int_name(char *br_int_name); #endif /* controller/pinctrl.h */ diff --git a/tests/ovn.at b/tests/ovn.at index b322b681af..e682f9edf4 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -6008,7 +6008,64 @@ expected_dhcp_opts=${boofile}330400000e100104ffffff0003040a00000136040a000001 test_dhcp 19 1 f00000000001 01 0 $ciaddr $offer_ip $request_ip 1 0 ff1000000001 $server_ip 02 $expected_dhcp_opts compare_dhcp_packets 1 -OVN_CLEANUP([hv1]) +# Test that ovn-controller pinctrl thread handles dhcp requests when it +# connects to a wrong version of ovn-northd at startup. + +# Stop ovn-northd so that we can modify the northd_version. +as northd +OVS_APP_EXIT_AND_WAIT([ovn-northd]) + +as northd-backup +OVS_APP_EXIT_AND_WAIT([ovn-northd]) + +northd_version=$(ovn-sbctl get SB_Global . options:northd_internal_version | sed s/\"//g) +echo "northd version = $northd_version" + +check ovn-sbctl set SB_Global . options:northd_internal_version=foo + +echo +echo "__file__:__line__: Stop ovn-controller." +as hv1 +OVS_APP_EXIT_AND_WAIT([ovn-controller]) + +echo +echo "__file__:__line__: Pin ovn-controller to ovn-northd version." + +as hv1 +check ovs-vsctl set open . external_ids:ovn-match-northd-version=true + +# Start ovn-controller +as hv1 +start_daemon ovn-controller + +OVS_WAIT_UNTIL( + [test 1 = $(grep -c "controller version - $northd_version mismatch with northd version - foo" hv1/ovn-controller.log) +]) + +reset_pcap_file hv1-vif1 hv1/vif1 +reset_pcap_file hv1-vif2 hv1/vif2 +rm -f 1.expected +rm -f 2.expected + +# ---------------------------------------------------------------------- + +offer_ip=`ip_to_hex 10 0 0 4` +server_ip=`ip_to_hex 10 0 0 1` +ciaddr=`ip_to_hex 0 0 0 0` +request_ip=0 +boofile=4308626f6f7466696c65 +expected_dhcp_opts=${boofile}330400000e100104ffffff0003040a00000136040a000001 +test_dhcp 20 1 f00000000001 01 0 $ciaddr $offer_ip $request_ip 1 0 ff1000000001 $server_ip 02 $expected_dhcp_opts +compare_dhcp_packets 1 + +as hv1 +OVS_APP_EXIT_AND_WAIT([ovn-controller]) + +as ovn-sb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as ovn-nb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) AT_CLEANUP