fix(NetworkManager): Fix network activator

Reload NetworkManager configuration rather than bringing up interfaces. This enables NetworkManager to configure interfaces which are not yet available in userspace, and fixes a race that was previously worked around in a service file.
canonical · Sep 16, 2024 · 122643a · 122643a
1 parent 1c61f3f
commit 122643a
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 12 deletions.
diff --git a/cloudinit/net/activators.py b/cloudinit/net/activators.py
@@ -184,6 +184,30 @@ def bring_down_interface(device_name: str) -> bool:
         cmd = ["nmcli", "device", "disconnect", device_name]
         return _alter_interface(cmd, device_name)
 
+    @classmethod
+    def bring_up_interfaces(cls, device_names: Iterable[str]) -> bool:
+        """Activate network
+
+        Return True on success
+        """
+        state = subp.subp(
+            [
+                "systemctl",
+                "show",
+                "--property=SubState",
+                "NetworkManager.service",
+            ]
+        ).stdout.rstrip()
+        if "SubState=running" != state:
+            LOG.warning(
+                "Expected NetworkManager SubState=running, but detected: %s",
+                state,
+            )
+        return _alter_interface(
+            ["systemctl", "reload-or-try-restart", "NetworkManager.service"],
+            "all",
+        )
+
 
 class NetplanActivator(NetworkActivator):
     NETPLAN_CMD = ["netplan", "apply"]

diff --git a/systemd/cloud-final.service.tmpl b/systemd/cloud-final.service.tmpl
@@ -25,16 +25,7 @@ Type=oneshot
 ExecStart=sh -c 'echo "start" | netcat -Uu -W1 /run/cloud-init/share/final.sock -s /run/cloud-init/share/final-return.sock | sh'
 RemainAfterExit=yes
 TimeoutSec=0
-{% if variant in ["almalinux", "cloudlinux", "rhel"] %}
-# Restart NetworkManager if it is present and running.
-ExecStartPost=/bin/sh -c 'u=NetworkManager.service; \
- out=$(systemctl show --property=SubState $u) || exit; \
- [ "$out" = "SubState=running" ] || exit 0; \
- systemctl reload-or-try-restart $u'
-{% else %}
 TasksMax=infinity
-{% endif %}
-
 
 # Output needs to appear in instance console output
 StandardOutput=journal+console

diff --git a/tests/unittests/test_net_activators.py b/tests/unittests/test_net_activators.py
@@ -6,6 +6,7 @@
 import pytest
 import yaml
 
+from cloudinit import subp
 from cloudinit.net.activators import (
     DEFAULT_PRIORITY,
     NAME_TO_ACTIVATOR,
@@ -234,6 +235,21 @@ def test_available(self, activator, available_calls, available_mocks):
     ),
 ]
 
+NETWORK_MANAGER_BRING_UP_ALL_CALL_LIST: list = [
+    (
+        (
+            [
+                "systemctl",
+                "show",
+                "--property=SubState",
+                "NetworkManager.service",
+            ],
+        ),
+        {},
+    ),
+    ((["systemctl", "reload-or-try-restart", "NetworkManager.service"],), {}),
+]
+
 NETWORKD_BRING_UP_CALL_LIST: list = [
     ((["ip", "link", "set", "dev", "eth0", "up"],), {}),
     ((["ip", "link", "set", "dev", "eth1", "up"],), {}),
@@ -278,7 +294,18 @@ def test_bring_up_interface(
             assert call == expected_call_list[index]
             index += 1
 
-    @patch("cloudinit.subp.subp", return_value=("", ""))
+
+@pytest.mark.parametrize(
+    "activator, expected_call_list",
+    [
+        (IfUpDownActivator, IF_UP_DOWN_BRING_UP_CALL_LIST),
+        (NetplanActivator, NETPLAN_CALL_LIST),
+        (NetworkManagerActivator, NETWORK_MANAGER_BRING_UP_ALL_CALL_LIST),
+        (NetworkdActivator, NETWORKD_BRING_UP_CALL_LIST),
+    ],
+)
+class TestActivatorsBringUpAll:
+    @patch("cloudinit.subp.subp", return_value=subp.SubpResult("", ""))
     def test_bring_up_interfaces(
         self, m_subp, activator, expected_call_list, available_mocks
     ):
@@ -288,7 +315,7 @@ def test_bring_up_interfaces(
             assert call == expected_call_list[index]
             index += 1
 
-    @patch("cloudinit.subp.subp", return_value=("", ""))
+    @patch("cloudinit.subp.subp", return_value=subp.SubpResult("", ""))
     def test_bring_up_all_interfaces_v1(
         self, m_subp, activator, expected_call_list, available_mocks
     ):
@@ -297,7 +324,7 @@ def test_bring_up_all_interfaces_v1(
         for call in m_subp.call_args_list:
             assert call in expected_call_list
 
-    @patch("cloudinit.subp.subp", return_value=("", ""))
+    @patch("cloudinit.subp.subp", return_value=subp.SubpResult("", ""))
     def test_bring_up_all_interfaces_v2(
         self, m_subp, activator, expected_call_list, available_mocks
     ):