From 31d0a9524dc3360966925196ebcbde026e6b593f Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Wed, 2 Dec 2020 17:30:24 +0100 Subject: [PATCH] ovs: clean up interfaces from ovsdb at startup During shutdown, NM always tries to remove from ovsdb all bridges, ports, interfaces that it previously added. Currently NM doesn't run the main loop during shutdown and so it's not possible to perform asynchronous operations. In particular, the NMOvsdb singleton is disposed in a destructor where it's not possible to send out all the queued deletions. The result is that NM deletes only one OVS interface, keeping the others. This needs to be fixed, but requires a rework of the shutdown procedure that involves many parts of NM. Even when a better shutdown procedure will be implemented, we should support an unclean shutdown caused by e.g. a kernel panic or a NM crash. In these cases, the interfaces added by NM would still linger in the ovsdb. Delete all those interface at NM startup. If there are connections profiles for them, NM will create them again. Also, NMOvsdb now emits a NM_OVSDB_READY signal and provides a nm_ovsdb_is_ready() to allow other parts of the daemon to order actions after the initial OVS cleanup. https://bugzilla.redhat.com/show_bug.cgi?id=1861296 https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/700 --- src/devices/ovs/nm-ovsdb.c | 102 ++++++++++++++++++++++++++++++++++++- src/devices/ovs/nm-ovsdb.h | 3 ++ 2 files changed, 104 insertions(+), 1 deletion(-) diff --git a/src/devices/ovs/nm-ovsdb.c b/src/devices/ovs/nm-ovsdb.c index fc9e6505da..c89c5452ee 100644 --- a/src/devices/ovs/nm-ovsdb.c +++ b/src/devices/ovs/nm-ovsdb.c @@ -106,7 +106,13 @@ typedef struct { /*****************************************************************************/ -enum { DEVICE_ADDED, DEVICE_REMOVED, INTERFACE_FAILED, LAST_SIGNAL }; +enum { + DEVICE_ADDED, + DEVICE_REMOVED, + INTERFACE_FAILED, + READY, + LAST_SIGNAL, +}; static guint signals[LAST_SIGNAL] = {0}; @@ -127,6 +133,8 @@ typedef struct { GHashTable *bridges; /* bridge uuid => OpenvswitchBridge */ char * db_uuid; guint num_failures; + guint num_pending_deletions; + bool ready : 1; } NMOvsdbPrivate; struct _NMOvsdb { @@ -2219,6 +2227,9 @@ ovsdb_disconnect(NMOvsdb *self, gboolean retry, gboolean is_disposing) _LOGD("disconnecting from ovsdb, retry %d", retry); + /* FIXME(shutdown): NMOvsdb should process the pending calls before + * shutting down, and cancel the remaining calls after the timeout. */ + if (retry) { if (!c_list_is_empty(&priv->calls_lst_head)) { call = c_list_first_entry(&priv->calls_lst_head, OvsdbMethodCall, calls_lst); @@ -2247,6 +2258,75 @@ ovsdb_disconnect(NMOvsdb *self, gboolean retry, gboolean is_disposing) ovsdb_try_connect(self); } +static void +_check_ready(NMOvsdb *self) +{ + NMOvsdbPrivate *priv = NM_OVSDB_GET_PRIVATE(self); + + nm_assert(!priv->ready); + + if (priv->num_pending_deletions == 0) { + priv->ready = TRUE; + g_signal_emit(self, signals[READY], 0); + } +} + +static void +_del_initial_iface_cb(GError *error, gpointer user_data) +{ + NMOvsdb * self; + gs_free char * ifname = NULL; + NMOvsdbPrivate *priv; + + nm_utils_user_data_unpack(user_data, &self, &ifname); + + if (nm_utils_error_is_cancelled_or_disposing(error)) + return; + + priv = NM_OVSDB_GET_PRIVATE(self); + nm_assert(priv->num_pending_deletions > 0); + priv->num_pending_deletions--; + + _LOGD("delete initial interface '%s': %s %s%s%s, pending %u", + ifname, + error ? "error" : "success", + error ? "(" : "", + error ? error->message : "", + error ? ")" : "", + priv->num_pending_deletions); + + _check_ready(self); +} + +static void +ovsdb_cleanup_initial_interfaces(NMOvsdb *self) +{ + NMOvsdbPrivate * priv = NM_OVSDB_GET_PRIVATE(self); + const OpenvswitchInterface *interface; + NMUtilsUserData * data; + GHashTableIter iter; + + if (priv->ready || priv->num_pending_deletions != 0) + return; + + /* Delete OVS interfaces added by NM. Bridges and ports and + * not considered because they are deleted automatically + * when no interface is present. */ + g_hash_table_iter_init(&iter, self->_priv.interfaces); + while (g_hash_table_iter_next(&iter, NULL, (gpointer *) &interface)) { + if (interface->connection_uuid) { + priv->num_pending_deletions++; + _LOGD("deleting initial interface '%s' (pending: %u)", + interface->name, + priv->num_pending_deletions); + data = nm_utils_user_data_pack(self, g_strdup(interface->name)); + nm_ovsdb_del_interface(self, interface->name, _del_initial_iface_cb, data); + } + } + + _check_ready(self); +} + static void _monitor_bridges_cb(NMOvsdb *self, json_t *result, GError *error, gpointer user_data) { @@ -2261,6 +2341,8 @@ _monitor_bridges_cb(NMOvsdb *self, json_t *result, GError *error, gpointer user_ /* Treat the first response the same as the subsequent "update" * messages we eventually get. */ ovsdb_got_update(self, result); + + ovsdb_cleanup_initial_interfaces(self); } static void @@ -2380,6 +2462,14 @@ ovsdb_call_new(NMOvsdbCallback callback, gpointer user_data) return call; } +gboolean +nm_ovsdb_is_ready(NMOvsdb *self) +{ + NMOvsdbPrivate *priv = NM_OVSDB_GET_PRIVATE(self); + + return priv->ready; +} + void nm_ovsdb_add_interface(NMOvsdb * self, NMConnection * bridge, @@ -2553,4 +2643,14 @@ nm_ovsdb_class_init(NMOvsdbClass *klass) G_TYPE_STRING, G_TYPE_STRING, G_TYPE_STRING); + + signals[READY] = g_signal_new(NM_OVSDB_READY, + G_OBJECT_CLASS_TYPE(object_class), + G_SIGNAL_RUN_LAST, + 0, + NULL, + NULL, + NULL, + G_TYPE_NONE, + 0); } diff --git a/src/devices/ovs/nm-ovsdb.h b/src/devices/ovs/nm-ovsdb.h index 533b23450a..d0b4d19d45 100644 --- a/src/devices/ovs/nm-ovsdb.h +++ b/src/devices/ovs/nm-ovsdb.h @@ -16,6 +16,7 @@ #define NM_OVSDB_DEVICE_ADDED "device-added" #define NM_OVSDB_DEVICE_REMOVED "device-removed" #define NM_OVSDB_INTERFACE_FAILED "interface-failed" +#define NM_OVSDB_READY "ready" typedef struct _NMOvsdb NMOvsdb; typedef struct _NMOvsdbClass NMOvsdbClass; @@ -55,4 +56,6 @@ void nm_ovsdb_set_external_ids(NMOvsdb * self, struct _NMSettingOvsExternalIDs *s_exid_old, struct _NMSettingOvsExternalIDs *s_exid_new); +gboolean nm_ovsdb_is_ready(NMOvsdb *self); + #endif /* __NETWORKMANAGER_OVSDB_H__ */