networking: refactor wait_for_physdevs from cloudinit.net (#466)

* Refactor `cloudinit.net.wait_for_physdevs` to `cloudinit.distros.networking.Networking.wait_for_physdevs`
* Split the Linux-specific `udevadm_settle` call out to a separate abstract `Networking.settle` method; implement it on `LinuxNetworking` and add a `NotImplementedError` implementation to `BSDNetworking`
* Modify `wait_for_physdevs`s one callsite to use the new location

LP: #1884626

5 files changed, 229 insertions, 116 deletions

diff --git a/cloudinit/distros/networking.py b/cloudinit/distros/networking.py
index e421a2ce..75e69df5 100644
--- a/cloudinit/distros/networking.py
+++ b/cloudinit/distros/networking.py
@@ -1,7 +1,11 @@
 import abc
+import logging
 import os
 
-from cloudinit import net
+from cloudinit import net, util
+
+
+LOG = logging.getLogger(__name__)
 
 
 # Type aliases (https://docs.python.org/3/library/typing.html#type-aliases),
@@ -102,10 +106,72 @@ class Networking(metaclass=abc.ABCMeta):
     def master_is_bridge_or_bond(self, devname: DeviceName) -> bool:
         return net.master_is_bridge_or_bond(devname)
 
+    @abc.abstractmethod
+    def settle(self, *, exists=None) -> None:
+        """Wait for device population in the system to complete.
+
+        :param exists:
+            An optional optimisation.  If given, only perform as much of the
+            settle process as is required for the given DeviceName to be
+            present in the system.  (This may include skipping the settle
+            process entirely, if the device already exists.)
+        :type exists: Optional[DeviceName]
+        """
+        pass
+
     def wait_for_physdevs(
         self, netcfg: NetworkConfig, *, strict: bool = True
     ) -> None:
-        return net.wait_for_physdevs(netcfg, strict=strict)
+        """Wait for all the physical devices in `netcfg` to exist on the system
+
+        Specifically, this will call `self.settle` 5 times, and check after
+        each one if the physical devices are now present in the system.
+
+        :param netcfg:
+            The NetworkConfig from which to extract physical devices to wait
+            for.
+        :param strict:
+            Raise a `RuntimeError` if any physical devices are not present
+            after waiting.
+        """
+        physdevs = self.extract_physdevs(netcfg)
+
+        # set of expected iface names and mac addrs
+        expected_ifaces = dict([(iface[0], iface[1]) for iface in physdevs])
+        expected_macs = set(expected_ifaces.keys())
+
+        # set of current macs
+        present_macs = self.get_interfaces_by_mac().keys()
+
+        # compare the set of expected mac address values to
+        # the current macs present; we only check MAC as cloud-init
+        # has not yet renamed interfaces and the netcfg may include
+        # such renames.
+        for _ in range(0, 5):
+            if expected_macs.issubset(present_macs):
+                LOG.debug("net: all expected physical devices present")
+                return
+
+            missing = expected_macs.difference(present_macs)
+            LOG.debug("net: waiting for expected net devices: %s", missing)
+            for mac in missing:
+                # trigger a settle, unless this interface exists
+                devname = expected_ifaces[mac]
+                msg = "Waiting for settle or {} exists".format(devname)
+                util.log_time(
+                    LOG.debug,
+                    msg,
+                    func=self.settle,
+                    kwargs={"exists": devname},
+                )
+
+            # update present_macs after settles
+            present_macs = self.get_interfaces_by_mac().keys()
+
+        msg = "Not all expected physical devices present: %s" % missing
+        LOG.warning(msg)
+        if strict:
+            raise RuntimeError(msg)
 
 
 class BSDNetworking(Networking):
@@ -114,6 +180,10 @@ class BSDNetworking(Networking):
     def is_physical(self, devname: DeviceName) -> bool:
         raise NotImplementedError()
 
+    def settle(self, *, exists=None) -> None:
+        """BSD has no equivalent to `udevadm settle`; noop."""
+        pass
+
 
 class LinuxNetworking(Networking):
     """Implementation of networking functionality common to Linux distros."""
@@ -138,3 +208,8 @@ class LinuxNetworking(Networking):
 
     def is_physical(self, devname: DeviceName) -> bool:
         return os.path.exists(net.sys_dev_path(devname, "device"))
+
+    def settle(self, *, exists=None) -> None:
+        if exists is not None:
+            exists = net.sys_dev_path(exists)
+        util.udevadm_settle(exists=exists)
diff --git a/cloudinit/distros/tests/test_networking.py b/cloudinit/distros/tests/test_networking.py
index 2acb12f4..b9a63842 100644
--- a/cloudinit/distros/tests/test_networking.py
+++ b/cloudinit/distros/tests/test_networking.py
@@ -2,7 +2,39 @@ from unittest import mock
 
 import pytest
 
-from cloudinit.distros.networking import BSDNetworking, LinuxNetworking
+from cloudinit import net
+from cloudinit.distros.networking import (
+    BSDNetworking,
+    LinuxNetworking,
+    Networking,
+)
+
+# See https://docs.pytest.org/en/stable/example
+# /parametrize.html#parametrizing-conditional-raising
+from contextlib import ExitStack as does_not_raise
+
+
+@pytest.yield_fixture
+def generic_networking_cls():
+    """Returns a direct Networking subclass which errors on /sys usage.
+
+    This enables the direct testing of functionality only present on the
+    ``Networking`` super-class, and provides a check on accidentally using /sys
+    in that context.
+    """
+
+    class TestNetworking(Networking):
+        def is_physical(self, *args, **kwargs):
+            raise NotImplementedError
+
+        def settle(self, *args, **kwargs):
+            raise NotImplementedError
+
+    error = AssertionError("Unexpectedly used /sys in generic networking code")
+    with mock.patch(
+        "cloudinit.net.get_sys_class_path", side_effect=error,
+    ):
+        yield TestNetworking
 
 
 @pytest.yield_fixture
@@ -40,3 +72,121 @@ class TestLinuxNetworkingIsPhysical:
         device_dir.join("device").write("")
 
         assert LinuxNetworking().is_physical(devname)
+
+
+class TestBSDNetworkingSettle:
+    def test_settle_doesnt_error(self):
+        # This also implicitly tests that it doesn't use subp.subp
+        BSDNetworking().settle()
+
+
+@pytest.mark.usefixtures("sys_class_net")
+@mock.patch("cloudinit.distros.networking.util.udevadm_settle", autospec=True)
+class TestLinuxNetworkingSettle:
+    def test_no_arguments(self, m_udevadm_settle):
+        LinuxNetworking().settle()
+
+        assert [mock.call(exists=None)] == m_udevadm_settle.call_args_list
+
+    def test_exists_argument(self, m_udevadm_settle):
+        LinuxNetworking().settle(exists="ens3")
+
+        expected_path = net.sys_dev_path("ens3")
+        assert [
+            mock.call(exists=expected_path)
+        ] == m_udevadm_settle.call_args_list
+
+
+class TestNetworkingWaitForPhysDevs:
+    @pytest.fixture
+    def wait_for_physdevs_netcfg(self):
+        """This config is shared across all the tests in this class."""
+
+        def ethernet(mac, name, driver=None, device_id=None):
+            v2_cfg = {"set-name": name, "match": {"macaddress": mac}}
+            if driver:
+                v2_cfg["match"].update({"driver": driver})
+            if device_id:
+                v2_cfg["match"].update({"device_id": device_id})
+
+            return v2_cfg
+
+        physdevs = [
+            ["aa:bb:cc:dd:ee:ff", "eth0", "virtio", "0x1000"],
+            ["00:11:22:33:44:55", "ens3", "e1000", "0x1643"],
+        ]
+        netcfg = {
+            "version": 2,
+            "ethernets": {args[1]: ethernet(*args) for args in physdevs},
+        }
+        return netcfg
+
+    def test_skips_settle_if_all_present(
+        self, generic_networking_cls, wait_for_physdevs_netcfg,
+    ):
+        networking = generic_networking_cls()
+        with mock.patch.object(
+            networking, "get_interfaces_by_mac"
+        ) as m_get_interfaces_by_mac:
+            m_get_interfaces_by_mac.side_effect = iter(
+                [{"aa:bb:cc:dd:ee:ff": "eth0", "00:11:22:33:44:55": "ens3"}]
+            )
+            with mock.patch.object(
+                networking, "settle", autospec=True
+            ) as m_settle:
+                networking.wait_for_physdevs(wait_for_physdevs_netcfg)
+            assert 0 == m_settle.call_count
+
+    def test_calls_udev_settle_on_missing(
+        self, generic_networking_cls, wait_for_physdevs_netcfg,
+    ):
+        networking = generic_networking_cls()
+        with mock.patch.object(
+            networking, "get_interfaces_by_mac"
+        ) as m_get_interfaces_by_mac:
+            m_get_interfaces_by_mac.side_effect = iter(
+                [
+                    {
+                        "aa:bb:cc:dd:ee:ff": "eth0"
+                    },  # first call ens3 is missing
+                    {
+                        "aa:bb:cc:dd:ee:ff": "eth0",
+                        "00:11:22:33:44:55": "ens3",
+                    },  # second call has both
+                ]
+            )
+            with mock.patch.object(
+                networking, "settle", autospec=True
+            ) as m_settle:
+                networking.wait_for_physdevs(wait_for_physdevs_netcfg)
+            m_settle.assert_called_with(exists="ens3")
+
+    @pytest.mark.parametrize(
+        "strict,expectation",
+        [(True, pytest.raises(RuntimeError)), (False, does_not_raise())],
+    )
+    def test_retrying_and_strict_behaviour(
+        self,
+        strict,
+        expectation,
+        generic_networking_cls,
+        wait_for_physdevs_netcfg,
+    ):
+        networking = generic_networking_cls()
+        with mock.patch.object(
+            networking, "get_interfaces_by_mac"
+        ) as m_get_interfaces_by_mac:
+            m_get_interfaces_by_mac.return_value = {}
+
+            with mock.patch.object(
+                networking, "settle", autospec=True
+            ) as m_settle:
+                with expectation:
+                    networking.wait_for_physdevs(
+                        wait_for_physdevs_netcfg, strict=strict
+                    )
+
+        assert (
+            5 * len(wait_for_physdevs_netcfg["ethernets"])
+            == m_settle.call_count
+        )
diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py
index 9d8c7ba9..322af77b 100644
--- a/cloudinit/net/__init__.py
+++ b/cloudinit/net/__init__.py
@@ -10,7 +10,6 @@ import ipaddress
 import logging
 import os
 import re
-from functools import partial
 
 from cloudinit import subp
 from cloudinit import util
@@ -494,43 +493,6 @@ def extract_physdevs(netcfg):
     raise RuntimeError('Unknown network config version: %s' % version)
 
 
-def wait_for_physdevs(netcfg, strict=True):
-    physdevs = extract_physdevs(netcfg)
-
-    # set of expected iface names and mac addrs
-    expected_ifaces = dict([(iface[0], iface[1]) for iface in physdevs])
-    expected_macs = set(expected_ifaces.keys())
-
-    # set of current macs
-    present_macs = get_interfaces_by_mac().keys()
-
-    # compare the set of expected mac address values to
-    # the current macs present; we only check MAC as cloud-init
-    # has not yet renamed interfaces and the netcfg may include
-    # such renames.
-    for _ in range(0, 5):
-        if expected_macs.issubset(present_macs):
-            LOG.debug('net: all expected physical devices present')
-            return
-
-        missing = expected_macs.difference(present_macs)
-        LOG.debug('net: waiting for expected net devices: %s', missing)
-        for mac in missing:
-            # trigger a settle, unless this interface exists
-            syspath = sys_dev_path(expected_ifaces[mac])
-            settle = partial(util.udevadm_settle, exists=syspath)
-            msg = 'Waiting for udev events to settle or %s exists' % syspath
-            util.log_time(LOG.debug, msg, func=settle)
-
-        # update present_macs after settles
-        present_macs = get_interfaces_by_mac().keys()
-
-    msg = 'Not all expected physical devices present: %s' % missing
-    LOG.warning(msg)
-    if strict:
-        raise RuntimeError(msg)
-
-
 def apply_network_config_names(netcfg, strict_present=True, strict_busy=True):
     """read the network config and rename devices accordingly.
     if strict_present is false, then do not raise exception if no devices
diff --git a/cloudinit/net/tests/test_init.py b/cloudinit/net/tests/test_init.py
index eb458c39..311ab6f8 100644
--- a/cloudinit/net/tests/test_init.py
+++ b/cloudinit/net/tests/test_init.py
@@ -963,80 +963,6 @@ class TestExtractPhysdevs(CiTestCase):
             net.extract_physdevs({'version': 3, 'awesome_config': []})
 
 
-class TestWaitForPhysdevs(CiTestCase):
-
-    def setUp(self):
-        super(TestWaitForPhysdevs, self).setUp()
-        self.add_patch('cloudinit.net.get_interfaces_by_mac',
-                       'm_get_iface_mac')
-        self.add_patch('cloudinit.util.udevadm_settle', 'm_udev_settle')
-
-    def test_wait_for_physdevs_skips_settle_if_all_present(self):
-        physdevs = [
-            ['aa:bb:cc:dd:ee:ff', 'eth0', 'virtio', '0x1000'],
-            ['00:11:22:33:44:55', 'ens3', 'e1000', '0x1643'],
-        ]
-        netcfg = {
-            'version': 2,
-            'ethernets': {args[1]: _mk_v2_phys(*args)
-                          for args in physdevs},
-        }
-        self.m_get_iface_mac.side_effect = iter([
-            {'aa:bb:cc:dd:ee:ff': 'eth0',
-             '00:11:22:33:44:55': 'ens3'},
-        ])
-        net.wait_for_physdevs(netcfg)
-        self.assertEqual(0, self.m_udev_settle.call_count)
-
-    def test_wait_for_physdevs_calls_udev_settle_on_missing(self):
-        physdevs = [
-            ['aa:bb:cc:dd:ee:ff', 'eth0', 'virtio', '0x1000'],
-            ['00:11:22:33:44:55', 'ens3', 'e1000', '0x1643'],
-        ]
-        netcfg = {
-            'version': 2,
-            'ethernets': {args[1]: _mk_v2_phys(*args)
-                          for args in physdevs},
-        }
-        self.m_get_iface_mac.side_effect = iter([
-            {'aa:bb:cc:dd:ee:ff': 'eth0'},   # first call ens3 is missing
-            {'aa:bb:cc:dd:ee:ff': 'eth0',
-             '00:11:22:33:44:55': 'ens3'},   # second call has both
-        ])
-        net.wait_for_physdevs(netcfg)
-        self.m_udev_settle.assert_called_with(exists=net.sys_dev_path('ens3'))
-
-    def test_wait_for_physdevs_raise_runtime_error_if_missing_and_strict(self):
-        physdevs = [
-            ['aa:bb:cc:dd:ee:ff', 'eth0', 'virtio', '0x1000'],
-            ['00:11:22:33:44:55', 'ens3', 'e1000', '0x1643'],
-        ]
-        netcfg = {
-            'version': 2,
-            'ethernets': {args[1]: _mk_v2_phys(*args)
-                          for args in physdevs},
-        }
-        self.m_get_iface_mac.return_value = {}
-        with self.assertRaises(RuntimeError):
-            net.wait_for_physdevs(netcfg)
-
-        self.assertEqual(5 * len(physdevs), self.m_udev_settle.call_count)
-
-    def test_wait_for_physdevs_no_raise_if_not_strict(self):
-        physdevs = [
-            ['aa:bb:cc:dd:ee:ff', 'eth0', 'virtio', '0x1000'],
-            ['00:11:22:33:44:55', 'ens3', 'e1000', '0x1643'],
-        ]
-        netcfg = {
-            'version': 2,
-            'ethernets': {args[1]: _mk_v2_phys(*args)
-                          for args in physdevs},
-        }
-        self.m_get_iface_mac.return_value = {}
-        net.wait_for_physdevs(netcfg, strict=False)
-        self.assertEqual(5 * len(physdevs), self.m_udev_settle.call_count)
-
-
 class TestNetFailOver(CiTestCase):
 
     def setUp(self):
diff --git a/cloudinit/stages.py b/cloudinit/stages.py
index db8ba64c..69e6b7e1 100644
--- a/cloudinit/stages.py
+++ b/cloudinit/stages.py
@@ -696,7 +696,7 @@ class Init(object):
                     netcfg, src = self._find_networking_config()
 
         # ensure all physical devices in config are present
-        net.wait_for_physdevs(netcfg)
+        self.distro.networking.wait_for_physdevs(netcfg)
 
         # apply renames from config
         self._apply_netcfg_names(netcfg)