From 628706fab592f6c5611438a21742391ea1a2e8cc Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Thu, 13 Feb 2020 14:52:11 +0100 Subject: [PATCH 1/2] 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 (cherry picked from commit 9c49f8a87962d8009dc18973845a1c46aba38d00) (cherry picked from commit 2e5e409bf2cf825af46c46d110b0502050adcb3c) --- src/devices/ovs/nm-device-ovs-interface.c | 86 ++++++++++++++++------- 1 file 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 6768d3efc2..a8a66bb361 100644 --- a/src/devices/ovs/nm-device-ovs-interface.c +++ b/src/devices/ovs/nm-device-ovs-interface.c @@ -35,7 +35,6 @@ _LOG_DECLARE_SELF(NMDeviceOvsInterface); typedef struct { bool waiting_for_interface:1; - int link_ifindex; } NMDeviceOvsInterfacePrivate; struct _NMDeviceOvsInterface { @@ -113,13 +112,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; - if ( pllink - && priv->waiting_for_interface - && nm_device_get_state (device) == NM_DEVICE_STATE_IP_CONFIG) { - priv->waiting_for_interface = FALSE; + 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); } @@ -143,12 +141,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; } @@ -178,11 +178,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); @@ -191,34 +197,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) @@ -250,7 +265,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) { @@ -260,16 +282,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), @@ -277,7 +309,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); } From 34a9247a640504b15daeab578f569a4a151bfa7d Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Fri, 14 Feb 2020 17:46:31 +0100 Subject: [PATCH 2/2] ovs: discard link updates when deactivating When the ovs interface gets deactivated, it is released from the master port and we call nm_device_update_from_platform_link (dev, NULL) to ignore any later event for the interface. This is important especially because it sets a zero ifindex on the interface and so, later when the link disappears, we don't unmanage the device but directly remove it. However, since ovs commands are queued, the link could appear during the deactivation and we need to ignore such events. Add a new device method can_update_from_platform_link() for such purpose. (cherry picked from commit e9fc1dea437a3f78212143e8d2b14bcea8926f90) (cherry picked from commit c4eb0c6852d96b82081babe2c16a54df75717aba) --- src/devices/nm-device.c | 10 ++++++++++ src/devices/nm-device.h | 2 ++ src/devices/ovs/nm-device-ovs-interface.c | 13 +++++++++++++ 3 files changed, 25 insertions(+) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 05e310d8db..61878da2df 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -4193,6 +4193,12 @@ nm_device_create_and_realize (NMDevice *self, return TRUE; } +static gboolean +can_update_from_platform_link (NMDevice *self, const NMPlatformLink *plink) +{ + return TRUE; +} + void nm_device_update_from_platform_link (NMDevice *self, const NMPlatformLink *plink) { @@ -4201,6 +4207,9 @@ nm_device_update_from_platform_link (NMDevice *self, const NMPlatformLink *plink int ifindex; guint32 mtu; + if (!NM_DEVICE_GET_CLASS (self)->can_update_from_platform_link (self, plink)) + return; + g_return_if_fail (plink == NULL || link_type_compatible (self, plink->type, NULL, NULL)); str = plink ? nm_platform_link_get_udi (nm_device_get_platform (self), plink->ifindex) : NULL; @@ -17238,6 +17247,7 @@ nm_device_class_init (NMDeviceClass *klass) klass->get_type_description = get_type_description; klass->can_auto_connect = can_auto_connect; + klass->can_update_from_platform_link = can_update_from_platform_link; klass->check_connection_compatible = check_connection_compatible; klass->check_connection_available = check_connection_available; klass->can_unmanaged_external_down = can_unmanaged_external_down; diff --git a/src/devices/nm-device.h b/src/devices/nm-device.h index 412a57a0af..b8b1e400a7 100644 --- a/src/devices/nm-device.h +++ b/src/devices/nm-device.h @@ -472,6 +472,8 @@ typedef struct _NMDeviceClass { guint32 (* get_dhcp_timeout) (NMDevice *self, int addr_family); + gboolean (* can_update_from_platform_link) (NMDevice *self, const NMPlatformLink *plink); + /* Controls, whether to call act_stage2_config() callback also for assuming * a device or for external activations. In this case, act_stage2_config() must * take care not to touch the device's configuration. */ diff --git a/src/devices/ovs/nm-device-ovs-interface.c b/src/devices/ovs/nm-device-ovs-interface.c index a8a66bb361..9e6ac94750 100644 --- a/src/devices/ovs/nm-device-ovs-interface.c +++ b/src/devices/ovs/nm-device-ovs-interface.c @@ -313,6 +313,18 @@ deactivate_async (NMDevice *device, data); } +static gboolean +can_update_from_platform_link (NMDevice *device, const NMPlatformLink *plink) +{ + /* If the device is deactivating, we already sent the + * deletion command to ovsdb and we don't want to deal + * with any new link appearing from the previous + * activation. + */ + return !plink + || nm_device_get_state (device) != NM_DEVICE_STATE_DEACTIVATING; +} + /*****************************************************************************/ static void @@ -342,6 +354,7 @@ nm_device_ovs_interface_class_init (NMDeviceOvsInterfaceClass *klass) device_class->connection_type_check_compatible = NM_SETTING_OVS_INTERFACE_SETTING_NAME; device_class->link_types = NM_DEVICE_DEFINE_LINK_TYPES (NM_LINK_TYPE_OPENVSWITCH); + device_class->can_update_from_platform_link = can_update_from_platform_link; device_class->deactivate = deactivate; device_class->deactivate_async = deactivate_async; device_class->get_type_description = get_type_description;