On 9/19/24 14:11, Ales Musil wrote: > On Thu, Jul 25, 2024 at 3:16 PM Xavier Simonart <[email protected]> wrote: > >> There was a potential ovs_assert when exiting ovn-controller: >> controller/if-status.c:261: assertion >> shash_is_empty(&mgr->ovn_uninstall_hash) failed in if_status_mgr_clear() >> >> Fixes: 395eac485b87 ("ovn-controller: fixed ovn-installed not always >> properly added or removed.") >> >> Signed-off-by: Xavier Simonart <[email protected]> >> --- >> controller/if-status.c | 24 +++++++++--------------- >> tests/system-ovn.at | 23 +++++++++++++++++++---- >> 2 files changed, 28 insertions(+), 19 deletions(-) >> >> diff --git a/controller/if-status.c b/controller/if-status.c >> index 9a7488057..ada78a18b 100644 >> --- a/controller/if-status.c >> +++ b/controller/if-status.c >> @@ -219,7 +219,8 @@ ovs_iface_create(struct if_status_mgr *, const char >> *iface_id, >> static void add_to_ovn_uninstall_hash(struct if_status_mgr *, const char >> *, >> const struct uuid *); >> static void ovs_iface_destroy(struct if_status_mgr *, struct ovs_iface *); >> -static void ovn_uninstall_hash_destroy(struct if_status_mgr *mgr, char >> *name); >> +static void ovn_uninstall_hash_destroy(struct if_status_mgr *mgr, >> + struct shash_node *node); >> static void ovs_iface_set_state(struct if_status_mgr *, struct ovs_iface >> *, >> enum if_state); >> >> @@ -256,7 +257,7 @@ if_status_mgr_clear(struct if_status_mgr *mgr) >> ovs_assert(shash_is_empty(&mgr->ifaces)); >> >> SHASH_FOR_EACH_SAFE (node, &mgr->ovn_uninstall_hash) { >> - ovn_uninstall_hash_destroy(mgr, node->data); >> + ovn_uninstall_hash_destroy(mgr, node); >> } >> ovs_assert(shash_is_empty(&mgr->ovn_uninstall_hash)); >> >> @@ -789,20 +790,13 @@ ovs_iface_destroy(struct if_status_mgr *mgr, struct >> ovs_iface *iface) >> } >> >> static void >> -ovn_uninstall_hash_destroy(struct if_status_mgr *mgr, char *name) >> +ovn_uninstall_hash_destroy(struct if_status_mgr *mgr, struct shash_node >> *node) >> { >> - struct shash_node *node = shash_find(&mgr->ovn_uninstall_hash, name); >> - char *node_name = NULL; >> - if (node) { >> - free(node->data); >> - VLOG_DBG("Interface name %s destroy", name); >> - node_name = shash_steal(&mgr->ovn_uninstall_hash, node); >> - ovn_uninstall_hash_account_mem(name, true); >> - free(node_name); >> - } else { >> - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); >> - VLOG_WARN_RL(&rl, "Interface name %s not found", name); >> - } >> + free(node->data); >> + VLOG_DBG("Interface name %s destroy", node->name); >> + char *node_name = shash_steal(&mgr->ovn_uninstall_hash, node); >> + ovn_uninstall_hash_account_mem(node_name, true); >> + free(node_name); >> } >> >> static void >> diff --git a/tests/system-ovn.at b/tests/system-ovn.at >> index ddb3d14e9..e9d889f43 100644 >> --- a/tests/system-ovn.at >> +++ b/tests/system-ovn.at >> @@ -11325,7 +11325,25 @@ check_ovn_installed >> check_ports_up >> check_ports_bound >> >> -OVS_APP_EXIT_AND_WAIT([ovn-controller]) >> +AS_BOX(["Leave some ovn-installed while closing ovn-controller"]) >> +# Block IDL from ovn-controller to OVSDB >> +stop_ovsdb_controller_updates $TCP_PORT >> +remove_iface_id vif2 >> +ensure_controller_run >> + >> +# OVSDB should now be seen as read-only by ovn-controller >> +remove_iface_id vif1 >> +check ovn-nbctl --wait=hv sync >> + >> +# Stop ovsdb before ovn-controller to ensure it's not updated >> +as >> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d >> +/connection dropped.*/d"]) >> + >> +# Don't use OVS_APP_EXIT... to use --restart to avoid cleaning up the >> databases. >> +TMPPID=$(cat $OVS_RUNDIR/ovn-controller.pid) >> +check ovs-appctl -t ovn-controller exit --restart >> +OVS_WAIT_WHILE([kill -0 $TMPPID 2>/dev/null]) >> >> as ovn-sb >> OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >> @@ -11336,9 +11354,6 @@ OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >> as northd >> OVS_APP_EXIT_AND_WAIT([ovn-northd]) >> >> -as >> -OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d >> -/connection dropped.*/d"]) >> AT_CLEANUP >> ]) >> >> -- >> 2.31.1 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > Now that we have the check for nftables, feel free to add my ack after > rebase. > Acked-by: Ales Musil <[email protected]> >
Thanks for the fix, Xavier and for the review, Ales! There was no need to rebase, the patch still applies cleanly. I pushed it to main and backported it down to 23.09. Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
