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

2 files changed, 228 insertions, 3 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
+        )