| Message ID | 20241106201031.151466-1-rosemarie@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] controller: Prevent crash when db is empty. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On 11/6/24 21:10, Rosemarie O'Riorden wrote: > ovn-controller crashed when connected to an empty local db, forcing the user to > initialize a local db before starting the controller daemon. With this fix, the > controller will no longer crash in this case, but instead keep running and wait > until the database is initialized to do anything else. > > Reported-at: https://issues.redhat.com/browse/FDP-715 > Signed-off-by: Rosemarie O'Riorden <rosemarie@redhat.com> > --- > v2: > - Add Reported-at tag. > - Syntax and style fixes. > - Change macro used in test to OVS_WAIT_FOR_OUTPUT instead of AT_CHECK. > - Remove unwanted line deletion. > --- > controller/chassis.c | 3 +++ > controller/ovn-controller.c | 8 ++++---- > tests/ovn-controller.at | 25 +++++++++++++++++++++++++ > 3 files changed, 32 insertions(+), 4 deletions(-) > > diff --git a/controller/chassis.c b/controller/chassis.c > index 2991a0af3..b2eefd58a 100644 > --- a/controller/chassis.c > +++ b/controller/chassis.c > @@ -1016,6 +1016,9 @@ store_chassis_index_if_needed( > { > const struct ovsrec_open_vswitch *cfg = > ovsrec_open_vswitch_table_first(ovs_table); > + if (!cfg) { > + return; > + } > const char *chassis_id = get_ovs_chassis_id(ovs_table); > > char *idx_key = xasprintf(CHASSIS_IDX_PREFIX "%s", chassis_id); > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index c48667887..1e987e630 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -5434,6 +5434,8 @@ main(int argc, char *argv[]) > ovsrec_open_vswitch_table_get(ovs_idl_loop.idl); > const struct ovsrec_bridge *br_int = NULL; > const struct ovsrec_datapath *br_int_dp = NULL; > + const struct ovsrec_open_vswitch *cfg = > + ovsrec_open_vswitch_table_first(ovs_table); > process_br_int(ovs_idl_txn, bridge_table, ovs_table, &br_int, > ovsrec_server_has_datapath_table(ovs_idl_loop.idl) > ? &br_int_dp > @@ -5445,9 +5447,7 @@ main(int argc, char *argv[]) > br_int_remote.probe_interval); > > /* Enable ACL matching for double tagged traffic. */ > - if (ovs_idl_txn) { > - const struct ovsrec_open_vswitch *cfg = > - ovsrec_open_vswitch_table_first(ovs_table); > + if (ovs_idl_txn && cfg) { > int vlan_limit = smap_get_int( > &cfg->other_config, "vlan-limit", -1); > if (vlan_limit != 0) { > @@ -5463,7 +5463,7 @@ main(int argc, char *argv[]) > } > > if (ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl) && > - northd_version_match) { > + northd_version_match && cfg) { > > /* Unconditionally remove all deleted lflows from the lflow > * cache. > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > index a2e451880..82ed5d0f1 100644 > --- a/tests/ovn-controller.at > +++ b/tests/ovn-controller.at > @@ -3419,3 +3419,28 @@ AT_CHECK([test $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log) -ge 1]) > > OVN_CLEANUP([hv1]) > AT_CLEANUP > + > +AT_SETUP([ovn-controller - qwerty Start controller with empty db]) Leftover from testing --------^^^^^^ I suppose, can be removed while applying. With that, Acked-by: Ilya Maximets <i.maximets@ovn.org> > +AT_KEYWORDS([ovn]) > + > +check ovsdb-tool create conf.db $ovs_srcdir/vswitchd/vswitch.ovsschema > +start_daemon ovsdb-server --remote=punix:db.sock > +start_daemon ovn-controller unix:db.sock > + > +AT_CHECK([ovsdb-client --bare dump unix:db.sock Open_vSwitch Open_vSwitch], [], [dnl > +Open_vSwitch table > +]) > +check ovs-vsctl --no-wait init > +OVS_WAIT_FOR_OUTPUT([ovs-vsctl show | tail -n +2], [], [dnl > + Bridge br-int > + fail_mode: secure > + datapath_type: system > + Port br-int > + Interface br-int > + type: internal > +]) > + > +# Gracefully terminate daemons. > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > +AT_CLEANUP
On 11/14/24 10:41 PM, Ilya Maximets wrote: > On 11/6/24 21:10, Rosemarie O'Riorden wrote: >> ovn-controller crashed when connected to an empty local db, forcing the user to >> initialize a local db before starting the controller daemon. With this fix, the >> controller will no longer crash in this case, but instead keep running and wait >> until the database is initialized to do anything else. >> >> Reported-at: https://issues.redhat.com/browse/FDP-715 >> Signed-off-by: Rosemarie O'Riorden <rosemarie@redhat.com> >> --- Hi Rosemarie, >> v2: >> - Add Reported-at tag. >> - Syntax and style fixes. >> - Change macro used in test to OVS_WAIT_FOR_OUTPUT instead of AT_CHECK. >> - Remove unwanted line deletion. >> --- >> controller/chassis.c | 3 +++ >> controller/ovn-controller.c | 8 ++++---- >> tests/ovn-controller.at | 25 +++++++++++++++++++++++++ >> 3 files changed, 32 insertions(+), 4 deletions(-) >> >> diff --git a/controller/chassis.c b/controller/chassis.c >> index 2991a0af3..b2eefd58a 100644 >> --- a/controller/chassis.c >> +++ b/controller/chassis.c >> @@ -1016,6 +1016,9 @@ store_chassis_index_if_needed( >> { >> const struct ovsrec_open_vswitch *cfg = >> ovsrec_open_vswitch_table_first(ovs_table); >> + if (!cfg) { >> + return; >> + } >> const char *chassis_id = get_ovs_chassis_id(ovs_table); >> >> char *idx_key = xasprintf(CHASSIS_IDX_PREFIX "%s", chassis_id); >> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >> index c48667887..1e987e630 100644 >> --- a/controller/ovn-controller.c >> +++ b/controller/ovn-controller.c >> @@ -5434,6 +5434,8 @@ main(int argc, char *argv[]) >> ovsrec_open_vswitch_table_get(ovs_idl_loop.idl); >> const struct ovsrec_bridge *br_int = NULL; >> const struct ovsrec_datapath *br_int_dp = NULL; >> + const struct ovsrec_open_vswitch *cfg = >> + ovsrec_open_vswitch_table_first(ovs_table); >> process_br_int(ovs_idl_txn, bridge_table, ovs_table, &br_int, >> ovsrec_server_has_datapath_table(ovs_idl_loop.idl) >> ? &br_int_dp >> @@ -5445,9 +5447,7 @@ main(int argc, char *argv[]) >> br_int_remote.probe_interval); >> >> /* Enable ACL matching for double tagged traffic. */ >> - if (ovs_idl_txn) { >> - const struct ovsrec_open_vswitch *cfg = >> - ovsrec_open_vswitch_table_first(ovs_table); >> + if (ovs_idl_txn && cfg) { >> int vlan_limit = smap_get_int( >> &cfg->other_config, "vlan-limit", -1); >> if (vlan_limit != 0) { >> @@ -5463,7 +5463,7 @@ main(int argc, char *argv[]) >> } >> >> if (ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl) && >> - northd_version_match) { >> + northd_version_match && cfg) { >> >> /* Unconditionally remove all deleted lflows from the lflow >> * cache. >> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at >> index a2e451880..82ed5d0f1 100644 >> --- a/tests/ovn-controller.at >> +++ b/tests/ovn-controller.at >> @@ -3419,3 +3419,28 @@ AT_CHECK([test $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log) -ge 1]) >> >> OVN_CLEANUP([hv1]) >> AT_CLEANUP >> + >> +AT_SETUP([ovn-controller - qwerty Start controller with empty db]) > > Leftover from testing --------^^^^^^ > I suppose, can be removed while applying. > > With that, > > Acked-by: Ilya Maximets <i.maximets@ovn.org> > I fixed up the test name and applied the patch to main, 24.09 and 24.03. Thanks Rosemarie and Ilya! Regards, Dumitru
diff --git a/controller/chassis.c b/controller/chassis.c index 2991a0af3..b2eefd58a 100644 --- a/controller/chassis.c +++ b/controller/chassis.c @@ -1016,6 +1016,9 @@ store_chassis_index_if_needed( { const struct ovsrec_open_vswitch *cfg = ovsrec_open_vswitch_table_first(ovs_table); + if (!cfg) { + return; + } const char *chassis_id = get_ovs_chassis_id(ovs_table); char *idx_key = xasprintf(CHASSIS_IDX_PREFIX "%s", chassis_id); diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index c48667887..1e987e630 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -5434,6 +5434,8 @@ main(int argc, char *argv[]) ovsrec_open_vswitch_table_get(ovs_idl_loop.idl); const struct ovsrec_bridge *br_int = NULL; const struct ovsrec_datapath *br_int_dp = NULL; + const struct ovsrec_open_vswitch *cfg = + ovsrec_open_vswitch_table_first(ovs_table); process_br_int(ovs_idl_txn, bridge_table, ovs_table, &br_int, ovsrec_server_has_datapath_table(ovs_idl_loop.idl) ? &br_int_dp @@ -5445,9 +5447,7 @@ main(int argc, char *argv[]) br_int_remote.probe_interval); /* Enable ACL matching for double tagged traffic. */ - if (ovs_idl_txn) { - const struct ovsrec_open_vswitch *cfg = - ovsrec_open_vswitch_table_first(ovs_table); + if (ovs_idl_txn && cfg) { int vlan_limit = smap_get_int( &cfg->other_config, "vlan-limit", -1); if (vlan_limit != 0) { @@ -5463,7 +5463,7 @@ main(int argc, char *argv[]) } if (ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl) && - northd_version_match) { + northd_version_match && cfg) { /* Unconditionally remove all deleted lflows from the lflow * cache. diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at index a2e451880..82ed5d0f1 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -3419,3 +3419,28 @@ AT_CHECK([test $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log) -ge 1]) OVN_CLEANUP([hv1]) AT_CLEANUP + +AT_SETUP([ovn-controller - qwerty Start controller with empty db]) +AT_KEYWORDS([ovn]) + +check ovsdb-tool create conf.db $ovs_srcdir/vswitchd/vswitch.ovsschema +start_daemon ovsdb-server --remote=punix:db.sock +start_daemon ovn-controller unix:db.sock + +AT_CHECK([ovsdb-client --bare dump unix:db.sock Open_vSwitch Open_vSwitch], [], [dnl +Open_vSwitch table +]) +check ovs-vsctl --no-wait init +OVS_WAIT_FOR_OUTPUT([ovs-vsctl show | tail -n +2], [], [dnl + Bridge br-int + fail_mode: secure + datapath_type: system + Port br-int + Interface br-int + type: internal +]) + +# Gracefully terminate daemons. +OVS_APP_EXIT_AND_WAIT([ovn-controller]) +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) +AT_CLEANUP
ovn-controller crashed when connected to an empty local db, forcing the user to initialize a local db before starting the controller daemon. With this fix, the controller will no longer crash in this case, but instead keep running and wait until the database is initialized to do anything else. Reported-at: https://issues.redhat.com/browse/FDP-715 Signed-off-by: Rosemarie O'Riorden <rosemarie@redhat.com> --- v2: - Add Reported-at tag. - Syntax and style fixes. - Change macro used in test to OVS_WAIT_FOR_OUTPUT instead of AT_CHECK. - Remove unwanted line deletion. --- controller/chassis.c | 3 +++ controller/ovn-controller.c | 8 ++++---- tests/ovn-controller.at | 25 +++++++++++++++++++++++++ 3 files changed, 32 insertions(+), 4 deletions(-)