From c68cbcb8fa8d73c4b6dffae1b3773901f8244b40 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 17 Apr 2023 11:35:06 +0200 Subject: [PATCH 1/3] device: minor cleanup of code path in delete_cb() --- src/core/devices/nm-device.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index 3f14105d8..0a1e4fddd 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -13583,10 +13583,13 @@ delete_cb(NMDevice *self, /* Authorized */ nm_audit_log_device_op(NM_AUDIT_OP_DEVICE_DELETE, self, TRUE, NULL, subject, NULL); - if (nm_device_unrealize(self, TRUE, &local)) - g_dbus_method_invocation_return_value(context, NULL); - else + + if (!nm_device_unrealize(self, TRUE, &local)) { g_dbus_method_invocation_take_error(context, local); + return; + } + + g_dbus_method_invocation_return_value(context, NULL); } static void From 14d429dd17dcc242f13403374a519d3ea17df261 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 17 Apr 2023 11:42:19 +0200 Subject: [PATCH 2/3] device: block autoconnect of profile when deleting device Currently, when we delete a device then autoconnect does not kick in right away. But that is only, because we happen not to schedule a "autoactivate" recheck. What should be happen, is that rechecking whether to autoconnect is always allowed, and that we have the necessary state to know that autoconnect currently should not work. Instead, block autoconnect of the involved profile. That makes sense, because clearly we don't want to autoconnect right again after `nmcli device delete $iface`. --- src/core/devices/nm-device.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index 0a1e4fddd..25b53ee34 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -13568,7 +13568,8 @@ delete_cb(NMDevice *self, GError *error, gpointer user_data) { - GError *local = NULL; + NMSettingsConnection *sett_conn; + GError *local = NULL; if (error) { g_dbus_method_invocation_return_gerror(context, error); @@ -13584,6 +13585,19 @@ delete_cb(NMDevice *self, /* Authorized */ nm_audit_log_device_op(NM_AUDIT_OP_DEVICE_DELETE, self, TRUE, NULL, subject, NULL); + sett_conn = nm_device_get_settings_connection(self); + if (sett_conn) { + /* Block profile from autoconnecting. We block the profile, which may + * be ugly/wrong with multi-connect profiles. However, it's not + * obviously wrong, because profiles for software devices tend not to + * work with multi-connect anyway, because they describe a (unique) + * interface by name. */ + nm_settings_connection_autoconnect_blocked_reason_set( + sett_conn, + NM_SETTINGS_AUTOCONNECT_BLOCKED_REASON_USER_REQUEST, + TRUE); + } + if (!nm_device_unrealize(self, TRUE, &local)) { g_dbus_method_invocation_take_error(context, local); return; From e699dff46a7b24be9974bb779fd23b0802ba7a39 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 12 Apr 2023 11:18:29 +0200 Subject: [PATCH 3/3] device: trigger a recheck to autoconnect when unrealizing ovs-interface NM_reboot_openvswitch_vlan_configuration_var2 test exposes a race. What the test does, is to create OVS profiles and repeatedly restart NetworkManager, checking that those profiles autoconnect and the OVS configuration gets created. There is a race, where: - the OVS interface exists, and an NMDeviceOvsInterface is created - first ovsdb cleans up old interfaces, sending a json request. - OVS deletes the interface, and NetworkManager first picks up the platform signal (there is a race here, usually the ovsdb request completes first, which will cleanup the NMDeviceOvsInterface in a different way). - when the device gets unrealized, we don't schedule a check-autoactivate, so the device stays down. See https://bugzilla.redhat.com/show_bug.cgi?id=2152864#c5 for a log file with more details. What should instead happen, is to autoactivate the OVS interface, which then also fully configures the port and bridge interfaces. Explicitly schedule an autoactivate when unrealizing devices. Note that there are now several cases, where NetworkManager autoconnects more eagerly. This even affects some CI tests and user-visible behavior. But I think relying on "just don't call nm_device_emit_recheck_auto_activate() to hope that autoconnect doesn't happen is wrong. It must always be possible to trigger an autoconnect check, and the right thing must happen. We only don't trigger autoconnect checks *all* the time, because it would be a waste of CPU resources, but whenever we slightly suspect that an autoconnect may happen, we must be allowed to trigger a check. If a device is in a condition where it previously did not autoconnect, and it also *should* not autoconnect, then we need to fix the code that evaluates whether an autoconnect may happen (not avoid triggering a check). https://bugzilla.redhat.com/show_bug.cgi?id=2152864 Fixes-test: @NM_reboot_openvswitch_vlan_configuration_var2 --- src/core/devices/nm-device.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index 25b53ee34..c69bb4480 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -7860,6 +7860,10 @@ nm_device_unrealize(NMDevice *self, gboolean remove_resources, GError **error) /* Garbage-collect unneeded unrealized devices. */ nm_device_recheck_available_connections(self); + /* In case the unrealized device is not going away, it may need to + * autoactivate. Schedule also a check for that. */ + nm_device_emit_recheck_auto_activate(self); + return TRUE; }