ovs: rework asynchronous deactivation of ovs interfaces

Tracking the deletion of link by ifindex is difficult because the
ifindex of the device is updated through delayed (idle) calls in
NMDevice and so there is the possibility that at a certain time the
device ifindex is not in sync with platform state. It seems simpler to
watch instead the interface name. The ugly thing is that the interface
name can be changed externally, but if users do that on an activating
device they are looking for trouble.

Also change the deactivate code to deal with the scenario where we
already created the interface in the ovsdb but the link didn't show up
yet. To ensure a proper cleanup we must wait that the link appears and
then goes away; however the link may never appear if vswitchd sees
only the last state in ovsdb, and so we must use a ugly timeout to
avoid waiting forever.

https://bugzilla.redhat.com/show_bug.cgi?id=1787989

1 files changed, 59 insertions, 27 deletions

diff --git a/src/devices/ovs/nm-device-ovs-interface.c b/src/devices/ovs/nm-device-ovs-interface.c
index a15af28d0d..763a5d0d8b 100644
--- a/src/devices/ovs/nm-device-ovs-interface.c
+++ b/src/devices/ovs/nm-device-ovs-interface.c
@@ -21,7 +21,6 @@ _LOG_DECLARE_SELF(NMDeviceOvsInterface);
 
 typedef struct {
 	bool waiting_for_interface:1;
-	int link_ifindex;
 } NMDeviceOvsInterfacePrivate;
 
 struct _NMDeviceOvsInterface {
@@ -99,13 +98,12 @@ link_changed (NMDevice *device,
 {
 	NMDeviceOvsInterfacePrivate *priv = NM_DEVICE_OVS_INTERFACE_GET_PRIVATE (device);
 
-	if (pllink)
-		priv->link_ifindex = pllink->ifindex;
+	if (!pllink || !priv->waiting_for_interface)
+		return;
+
+	priv->waiting_for_interface = FALSE;
 
-	if (   pllink
-	    && priv->waiting_for_interface
-	    && nm_device_get_state (device) == NM_DEVICE_STATE_IP_CONFIG) {
-		priv->waiting_for_interface = FALSE;
+	if (nm_device_get_state (device) == NM_DEVICE_STATE_IP_CONFIG) {
 		nm_device_bring_up (device, TRUE, NULL);
 		nm_device_activate_schedule_stage3_ip_config_start (device);
 	}
@@ -129,12 +127,14 @@ act_stage3_ip_config_start (NMDevice *device,
                             gpointer *out_config,
                             NMDeviceStateReason *out_failure_reason)
 {
+	NMDeviceOvsInterface *self = NM_DEVICE_OVS_INTERFACE (device);
 	NMDeviceOvsInterfacePrivate *priv = NM_DEVICE_OVS_INTERFACE_GET_PRIVATE (device);
 
 	if (!_is_internal_interface (device))
 		return NM_ACT_STAGE_RETURN_IP_FAIL;
 
 	if (nm_device_get_ip_ifindex (device) <= 0) {
+		_LOGT (LOGD_DEVICE, "waiting for link to appear");
 		priv->waiting_for_interface = TRUE;
 		return NM_ACT_STAGE_RETURN_POSTPONE;
 	}
@@ -164,11 +164,17 @@ typedef struct {
 	gpointer callback_user_data;
 	gulong link_changed_id;
 	gulong cancelled_id;
+	guint link_timeout_id;
 } DeactivateData;
 
 static void
 deactivate_invoke_cb (DeactivateData *data, GError *error)
 {
+	NMDeviceOvsInterface *self = data->self;
+
+	_LOGT (LOGD_CORE,
+	       "deactivate: async callback (%s)",
+	       error ? error->message : "success");
 	data->callback (NM_DEVICE (data->self),
 	                error,
 	                data->callback_user_data);
@@ -177,34 +183,43 @@ deactivate_invoke_cb (DeactivateData *data, GError *error)
 	                           &data->link_changed_id);
 	nm_clear_g_signal_handler (data->cancellable,
 	                           &data->cancelled_id);
+	nm_clear_g_source (&data->link_timeout_id);
 	g_object_unref (data->self);
 	g_object_unref (data->cancellable);
 	nm_g_slice_free (data);
 }
 
 static void
-link_changed_cb (NMPlatform *platform,
-                 int obj_type_i,
-                 int ifindex,
-                 NMPlatformLink *info,
-                 int change_type_i,
-                 DeactivateData *data)
+deactivate_link_changed_cb (NMPlatform *platform,
+                            int obj_type_i,
+                            int ifindex,
+                            NMPlatformLink *info,
+                            int change_type_i,
+                            DeactivateData *data)
 {
 	NMDeviceOvsInterface *self = data->self;
-	NMDeviceOvsInterfacePrivate *priv = NM_DEVICE_OVS_INTERFACE_GET_PRIVATE (self);
 	const NMPlatformSignalChangeType change_type = change_type_i;
 
 	if (   change_type == NM_PLATFORM_SIGNAL_REMOVED
-	    && ifindex == priv->link_ifindex) {
-		_LOGT (LOGD_DEVICE,
-		       "link %d gone, proceeding with deactivation",
-		       priv->link_ifindex);
-		priv->link_ifindex = 0;
+	    && nm_streq0 (info->name, nm_device_get_iface (NM_DEVICE (self)))) {
+		_LOGT (LOGD_DEVICE, "deactivate: link removed, proceeding");
+		nm_device_update_from_platform_link (NM_DEVICE (self), NULL);
 		deactivate_invoke_cb (data, NULL);
 		return;
 	}
 }
 
+static gboolean
+deactivate_link_timeout (gpointer user_data)
+{
+	DeactivateData *data = user_data;
+	NMDeviceOvsInterface *self = data->self;
+
+	_LOGT (LOGD_DEVICE, "deactivate: timeout waiting link removal");
+	deactivate_invoke_cb (data, NULL);
+	return G_SOURCE_REMOVE;
+}
+
 static void
 deactivate_cancelled_cb (GCancellable *cancellable,
                          gpointer user_data)
@@ -236,7 +251,14 @@ deactivate_async (NMDevice *device,
 	NMDeviceOvsInterfacePrivate *priv = NM_DEVICE_OVS_INTERFACE_GET_PRIVATE (self);
 	DeactivateData *data;
 
-	priv->waiting_for_interface = FALSE;
+	_LOGT (LOGD_CORE, "deactivate: start async");
+
+	/* We want to ensure that the kernel link for this device is
+	 * removed upon disconnection so that it will not interfere with
+	 * later activations of the same device. Unfortunately there is
+	 * no synchronization mechanism with vswitchd, we only update
+	 * ovsdb and wait that changes are picked up.
+	 */
 
 	data = g_slice_new (DeactivateData);
 	*data = (DeactivateData) {
@@ -246,16 +268,26 @@ deactivate_async (NMDevice *device,
 		.callback_user_data = callback_user_data,
 	};
 
-	if (   !priv->link_ifindex
-	    || !nm_platform_link_get (nm_device_get_platform (device), priv->link_ifindex)) {
-		priv->link_ifindex = 0;
+	if (   !priv->waiting_for_interface
+	    && !nm_platform_link_get_by_ifname (nm_device_get_platform (device),
+	                                        nm_device_get_iface (device))) {
+		_LOGT (LOGD_CORE, "deactivate: link not present, proceeding");
+		nm_device_update_from_platform_link (NM_DEVICE (self), NULL);
 		nm_utils_invoke_on_idle (deactivate_cb_on_idle, data, cancellable);
 		return;
 	}
 
-	_LOGT (LOGD_DEVICE,
-	       "async deactivation: waiting for link %d to disappear",
-	       priv->link_ifindex);
+	if (priv->waiting_for_interface) {
+		/* At this point we have issued an INSERT and a DELETE
+		 * command for the interface to ovsdb. We don't know if
+		 * vswitchd will see the two updates or only one. We
+		 * must add a timeout to avoid waiting forever in case
+		 * the link doesn't appear.
+		 */
+		data->link_timeout_id = g_timeout_add (6000, deactivate_link_timeout, data);
+		_LOGT (LOGD_DEVICE, "deactivate: waiting for link to disappear in 6 seconds");
+	} else
+		_LOGT (LOGD_DEVICE, "deactivate: waiting for link to disappear");
 
 	data->cancelled_id = g_cancellable_connect (cancellable,
 	                                            G_CALLBACK (deactivate_cancelled_cb),
@@ -263,7 +295,7 @@ deactivate_async (NMDevice *device,
 	                                            NULL);
 	data->link_changed_id = g_signal_connect (nm_device_get_platform (device),
 	                                          NM_PLATFORM_SIGNAL_LINK_CHANGED,
-	                                          G_CALLBACK (link_changed_cb),
+	                                          G_CALLBACK (deactivate_link_changed_cb),
 	                                          data);
 }