Merge ~alfonsosanchezbeato/snappy-hwe-snaps/+git/network-manager:fix-route-end-dhcp-lease into ~snappy-hwe-team/snappy-hwe-snaps/+git/network-manager:network-manager/xenial/1.2.2

[Alfonso Sanchez-Beato]

Make sure default routes survive across expiring of DHCP leases. Commits taken from upstream. Related to LP: #1800712.

Make sure default routes survive across expiring of DHCP leases. Commits taken from upstream. Related to LP: #1800712.

[Tony Espy (awe)]

The bug you reference says:

FTR, this MP would also help here, although the initially proposed fix for the configuration snap should be enough.

It doesn't sound to me like this is *really* needed? Also the RH bug associated with the first commit isn't viewable w/out a login. ;(-

What version of NM did you cherry pick these fixes from?

Would also mind explaining the actual scenario in some more detail?

It sounds like NM is assuming an existing connection from networkd? And that when this happens, and DHCP leases associated with the connection expire, when a DHCP lease is re-acquired the default route wasn't being restored?

Finally, why is networkd involved here at all? Don't we ship a netplan file which makes NM the default renderer?

review: Needs Information

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

Actually, both changes (the MP in the bug and this one) solve the issue, the first one by making sure the IP does not survive after networkd does not manage the interface anymore, this one by making sure NM handles well that situation. This MP is for more general cases, so it is good that we land it, it is a bug fix anyway.

The fixes come from branch 1.2 of NM upstream.

The scenario is basically as you describe. Bottom line is that NM finds an IP already existing in the interface, but when it uses DHCP (which it does always when starting) the lease gives a different IP. In that situation, some times the default route is not restored. This is a race which happens maybe 50% of the times. This is what this MP fixes.

We do ship a netplan file that sets NM as the rendered, but on *first boot* networkd takes eth0 until a systemd service that sets the new netplan file does the change of netplan files.

[Tony Espy (awe)]

Thanks for the explanation!

review: Approve

[System Enablement Bot (system-enablement-ci-bot)]

FAILED: Continuous integration, rev:06511d90695866218578feeff8740298b1034480
https://jenkins.canonical.com/system-enablement/job/snappy-hwe-snaps-snap-build-prepare/317/
Executed test runs:
    ABORTED: https://jenkins.canonical.com/system-enablement/job/snappy-hwe-snaps-snap-build/ARCHITECTURE=amd64/358/console
    SUCCESS: https://jenkins.canonical.com/system-enablement/job/snappy-hwe-snaps-snap-build/ARCHITECTURE=arm64/358
    SUCCESS: https://jenkins.canonical.com/system-enablement/job/snappy-hwe-snaps-snap-build/ARCHITECTURE=armhf/358
    SUCCESS: https://jenkins.canonical.com/system-enablement/job/snappy-hwe-snaps-snap-build/ARCHITECTURE=i386/358
    None: https://jenkins.canonical.com/system-enablement/job/snappy-hwe-snaps-snap-update-mp/931/console

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-enablement/job/snappy-hwe-snaps-snap-build-prepare/317/rebuild

review: Needs Fixing (continuous-integration)

diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index 8472ff8..e8da548 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -4287,6 +4287,7 @@ ip4_config_merge_and_apply (NMDevice *self,
 	gboolean routes_full_sync;
 	gboolean ignore_auto_routes = FALSE;
 	gboolean ignore_auto_dns = FALSE;
+	gboolean auto_method = FALSE;
 
 	/* Merge all the configs into the composite config */
 	if (config) {
@@ -4302,6 +4303,10 @@ ip4_config_merge_and_apply (NMDevice *self,
 		if (s_ip4) {
 			ignore_auto_routes = nm_setting_ip_config_get_ignore_auto_routes (s_ip4);
 			ignore_auto_dns = nm_setting_ip_config_get_ignore_auto_dns (s_ip4);
+
+			if (nm_streq0 (nm_setting_ip_config_get_method (s_ip4),
+			               NM_SETTING_IP4_CONFIG_METHOD_AUTO))
+				auto_method = TRUE;
 		}
 	}
 
@@ -4357,10 +4362,12 @@ ip4_config_merge_and_apply (NMDevice *self,
 		goto END_ADD_DEFAULT_ROUTE;
 	}
 
-	if (nm_device_uses_generated_assumed_connection (self)) {
-		/* a generate-assumed-connection always detects the default route from platform */
+	/* a generated-assumed connection detects the default route from the platform,
+	 * but if the IP method is automatic we need to update the default route to
+	 * maintain connectivity.
+	 */
+	if (nm_device_uses_generated_assumed_connection (self) && !auto_method)
 		goto END_ADD_DEFAULT_ROUTE;
-	}
 
 	/* At this point, we treat assumed and non-assumed connections alike.
 	 * For assumed connections we do that because we still manage RA and DHCP
@@ -5009,6 +5016,7 @@ ip6_config_merge_and_apply (NMDevice *self,
 	gboolean routes_full_sync;
 	gboolean ignore_auto_routes = FALSE;
 	gboolean ignore_auto_dns = FALSE;
+	gboolean auto_method = FALSE;
 
 	/* Apply ignore-auto-routes and ignore-auto-dns settings */
 	connection = nm_device_get_applied_connection (self);
@@ -5018,6 +5026,11 @@ ip6_config_merge_and_apply (NMDevice *self,
 		if (s_ip6) {
 			ignore_auto_routes = nm_setting_ip_config_get_ignore_auto_routes (s_ip6);
 			ignore_auto_dns = nm_setting_ip_config_get_ignore_auto_dns (s_ip6);
+
+			if (NM_IN_STRSET (nm_setting_ip_config_get_method (s_ip6),
+			                  NM_SETTING_IP6_CONFIG_METHOD_AUTO,
+			                  NM_SETTING_IP6_CONFIG_METHOD_DHCP))
+				auto_method = TRUE;
 		}
 	}
 
@@ -5081,10 +5094,12 @@ ip6_config_merge_and_apply (NMDevice *self,
 		goto END_ADD_DEFAULT_ROUTE;
 	}
 
-	if (nm_device_uses_generated_assumed_connection (self)) {
-		/* a generate-assumed-connection always detects the default route from platform */
+	/* a generated-assumed connection detects the default route from the platform,
+	 * but if the IP method is automatic we need to update the default route to
+	 * maintain connectivity.
+	 */
+	if (nm_device_uses_generated_assumed_connection (self) && !auto_method)
 		goto END_ADD_DEFAULT_ROUTE;
-	}
 
 	/* At this point, we treat assumed and non-assumed connections alike.
 	 * For assumed connections we do that because we still manage RA and DHCP
@@ -7963,23 +7978,16 @@ nm_device_set_ip4_config (NMDevice *self,
 			nm_exported_object_clear_and_unexport (&old_config);
 
 		if (nm_device_uses_generated_assumed_connection (self)) {
-			NMConnection *connection = nm_device_get_applied_connection (self);
 			NMConnection *settings_connection = NM_CONNECTION (nm_device_get_settings_connection (self));
 			NMSetting *s_ip4;
 
-			g_object_freeze_notify (G_OBJECT (connection));
 			g_object_freeze_notify (G_OBJECT (settings_connection));
 
 			nm_connection_remove_setting (settings_connection, NM_TYPE_SETTING_IP4_CONFIG);
 			s_ip4 = nm_ip4_config_create_setting (priv->ip4_config);
 			nm_connection_add_setting (settings_connection, s_ip4);
 
-			nm_connection_remove_setting (connection, NM_TYPE_SETTING_IP4_CONFIG);
-			s_ip4 = nm_ip4_config_create_setting (priv->ip4_config);
-			nm_connection_add_setting (connection, s_ip4);
-
 			g_object_thaw_notify (G_OBJECT (settings_connection));
-			g_object_thaw_notify (G_OBJECT (connection));
 		}
 
 		nm_device_queue_recheck_assume (self);
@@ -8128,23 +8136,16 @@ nm_device_set_ip6_config (NMDevice *self,
 			nm_exported_object_clear_and_unexport (&old_config);
 
 		if (nm_device_uses_generated_assumed_connection (self)) {
-			NMConnection *connection = nm_device_get_applied_connection (self);
 			NMConnection *settings_connection = NM_CONNECTION (nm_device_get_settings_connection (self));
 			NMSetting *s_ip6;
 
-			g_object_freeze_notify (G_OBJECT (connection));
 			g_object_freeze_notify (G_OBJECT (settings_connection));
 
 			nm_connection_remove_setting (settings_connection, NM_TYPE_SETTING_IP6_CONFIG);
 			s_ip6 = nm_ip6_config_create_setting (priv->ip6_config);
 			nm_connection_add_setting (settings_connection, s_ip6);
 
-			nm_connection_remove_setting (connection, NM_TYPE_SETTING_IP6_CONFIG);
-			s_ip6 = nm_ip6_config_create_setting (priv->ip6_config);
-			nm_connection_add_setting (connection, s_ip6);
-
 			g_object_thaw_notify (G_OBJECT (settings_connection));
-			g_object_thaw_notify (G_OBJECT (connection));
 		}
 
 		nm_device_queue_recheck_assume (self);