distros.networking: initial implementation of layout (#391)

This commit introduces the initial structure for the "cloudinit.net -> cloudinit.distros.networking Hierarchy" refactor, as detailed in [0]. It also updates that section with some changes driven by this initial implementation, as well as adding a lot more specifics to it.

[0] https://cloudinit.readthedocs.io/en/latest/topics/hacking.html#cloudinit-net-cloudinit-distros-networking-hierarchy

4 files changed, 346 insertions, 23 deletions

diff --git a/HACKING.rst b/HACKING.rst
index 47ac6a86..4e5f56d7 100644
--- a/HACKING.rst
+++ b/HACKING.rst
@@ -347,7 +347,7 @@ intended as documentation for developers involved in the refactoring,
 but also for other developers who may interact with the code being
 refactored in the meantime.
 
-``cloudinit.net`` -> ``cloudinit.distros.Networking`` Hierarchy
+``cloudinit.net`` -> ``cloudinit.distros.networking`` Hierarchy
 ---------------------------------------------------------------
 
 ``cloudinit.net`` was imported from the curtin codebase as a chunk, and
@@ -367,12 +367,13 @@ there may already be differences in tooling that we currently
 work around in less obvious ways.
 
 The high-level plan is to introduce a hierarchy of networking classes
-in ``cloudinit.distros``, which each ``Distro`` subclass will
-reference.  These will capture the differences between networking on
-our various distros, while still allowing easy reuse of code between
+in ``cloudinit.distros.networking``, which each ``Distro`` subclass
+will reference.  These will capture the differences between networking
+on our various distros, while still allowing easy reuse of code between
 distros that share functionality (e.g. most of the Linux networking
-behaviour).  Callers will call ``distro.net.func`` instead of
-``cloudinit.net.func``, which will necessitate access to an
+behaviour).  ``Distro`` objects will instantiate the networking classes
+at ``self.net``, so callers will call ``distro.net.<func>`` instead of
+``cloudinit.net.<func>``; this will necessitate access to an
 instantiated ``Distro`` object.
 
 An implementation note: there may be external consumers of the
@@ -386,9 +387,9 @@ existing API exactly.)
 In more detail:
 
 * The root of this hierarchy will be the
-  ``cloudinit.distros.Networking`` class.  This class will have
-  a corresponding method for every ``cloudinit.net`` function that we
-  identify to be involved in refactoring.  Initially, these methods'
+  ``cloudinit.distros.networking.Networking`` class.  This class will
+  have a corresponding method for every ``cloudinit.net`` function that
+  we identify to be involved in refactoring.  Initially, these methods'
   implementations will simply call the corresponding ``cloudinit.net``
   function.  (This gives us the complete API from day one, for existing
   consumers.)
@@ -413,31 +414,208 @@ In more detail:
   its ``net`` attribute.  (This is the entry point for existing
   consumers to migrate to.)
 * Callers of refactored functions will change from calling
-  ``cloudinit.net.some_func`` to ``distro.net.some_func``, where
-  ``distro`` is an instance of the appropriate ``Distro`` class for
-  this system.  (This will require making such an instance available to
-  callers, which will constitute a large part of the work in this
-  project.)
+  ``cloudinit.net.<func>`` to ``distro.net.<func>``, where ``distro``
+  is an instance of the appropriate ``Distro`` class for this system.
+  (This will require making such an instance available to callers,
+  which will constitute a large part of the work in this project.)
 
 After the initial structure is in place, the work in this refactor will
 consist of replacing the ``cloudinit.net.some_func`` call in each
-``cloudinit.distros.Networking`` method with the actual implementation.
-This can be done incrementally, one function at a time:
-
-* pick an unmigrated ``cloudinit.distros.Networking`` method
-* refactor all of its callers to call the ``distro.net`` method on
-  ``Distro`` instead of the ``cloudinit.net`` function. (This is likely
-  to be the most time-consuming step, as it may require plumbing
-  ``Distro`` objects through to places that previously have not
-  consumed them.)
+``cloudinit.distros.networking.Networking`` method with the actual
+implementation.  This can be done incrementally, one function at a
+time:
+
+* pick an unmigrated ``cloudinit.distros.networking.Networking`` method
+* refactor all of its callers to call the ``distro.net.<func>`` method
+  on ``Distro`` instead of the ``cloudinit.net.<func>`` function. (This
+  is likely to be the most time-consuming step, as it may require
+  plumbing ``Distro`` objects through to places that previously have
+  not consumed them.)
 * refactor its implementation from ``cloudinit.net`` into the
   ``Networking`` hierarchy (e.g. if it has an if/else on BSD, this is
   the time to put the implementations in their respective subclasses)
+
+  * if part of the method contains distro-independent logic, then you
+    may need to create new methods to capture this distro-specific
+    logic; we don't want to replicate common logic in different
+    ``Networking`` subclasses
+  * if after the refactor, the method on the root ``Networking`` class
+    no longer has any implementation, it should be converted to an
+    `abstractmethod`_
+
 * ensure that the new implementation has unit tests (either by moving
   existing tests, or by writing new ones)
+* ensure that the new implementation has a docstring
+* add any appropriate type annotations
+
+  * note that we must follow the constraints described in the "Type
+    Annotations" section above, so you may not be able to write
+    complete annotations
+  * we have `type aliases`_ defined in ``cloudinit.distros.networking``
+    which should be used when applicable
+
 * finally, remove it (and any other now-unused functions) from
   cloudinit.net (to avoid having two parallel implementations)
 
+``cloudinit.net`` Functions/Classes
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The functions/classes that need refactoring break down into some broad
+categories:
+
+* helpers for accessing ``/sys`` (that should not be on the top-level
+  ``Networking`` class as they are Linux-specific):
+
+  * ``get_sys_class_path``
+  * ``sys_dev_path``
+  * ``read_sys_net``
+  * ``read_sys_net_safe``
+  * ``read_sys_net_int``
+
+* those that directly access ``/sys`` (via helpers) and should (IMO) be
+  included in the API of the ``Networking`` class:
+
+  * ``generate_fallback_config``
+
+    * the ``config_driver`` parameter is used and passed as a boolean,
+      so we can change the default value to ``False`` (instead of
+      ``None``)
+
+  * ``get_ib_interface_hwaddr``
+  * ``get_interface_mac``
+  * ``interface_has_own_mac``
+  * ``is_bond``
+  * ``is_bridge``
+  * ``is_connected``
+  * ``is_physical``
+  * ``is_present``
+  * ``is_renamed``
+  * ``is_up``
+  * ``is_vlan``
+  * ``is_wireless``
+  * ``wait_for_physdevs``
+
+* those that directly access ``/sys`` (via helpers) but may be
+  Linux-specific concepts or names:
+
+  * ``get_master``
+  * ``device_devid``
+  * ``device_driver``
+
+* those that directly use ``ip``:
+
+  * ``_get_current_rename_info``
+
+    * this has non-distro-specific logic so should potentially be
+      refactored to use helpers on ``self`` instead of ``ip`` directly
+      (rather than being wholesale reimplemented in each of
+      ``BSDNetworking`` or ``LinuxNetworking``)
+    * we can also remove the ``check_downable`` argument, it's never
+      specified so is always ``True``
+
+  * ``_rename_interfaces``
+
+    * this has several internal helper functions which use ``ip``
+      directly, and it calls ``_get_current_rename_info``.  That said,
+      there appears to be a lot of non-distro-specific logic that could
+      live in a function on ``Networking``, so this will require some
+      careful refactoring to avoid duplicating that logic in each of
+      ``BSDNetworking`` and ``LinuxNetworking``.
+    * only the ``renames`` and ``current_info`` parameters are ever
+      passed in (and ``current_info`` only by tests), so we can remove
+      the others from the definition
+
+  * ``EphemeralIPv4Network``
+
+    * this is another case where it mixes distro-specific and
+      non-specific functionality.  Specifically, ``__init__``,
+      ``__enter__`` and ``__exit__`` are non-specific, and the
+      remaining methods are distro-specific.
+    * when refactoring this, the need to track ``cleanup_cmds`` likely
+      means that the distro-specific behaviour cannot be captured only
+      in the ``Networking`` class.  See `this comment in PR #363`_ for
+      more thoughts.
+
+* those that implicitly use ``/sys`` via their call dependencies:
+
+  * ``master_is_bridge_or_bond``
+
+    * appends to ``get_master`` return value, which is a ``/sys`` path
+
+  * ``extract_physdevs``
+
+    * calls ``device_driver`` and ``device_devid`` in both
+      ``_version_*`` impls
+
+  * ``apply_network_config_names``
+
+    * calls ``extract_physdevs``
+    * there is already a ``Distro.apply_network_config_names`` which in
+      the default implementation calls this function; this and its BSD
+      subclass implementations should be refactored at the same time
+    * the ``strict_present`` and ``strict_busy`` parameters are never
+      passed, nor are they used in the function definition, so they can
+      be removed
+
+  * ``get_interfaces``
+
+    * calls ``device_driver``, ``device_devid`` amongst others
+
+  * ``get_ib_hwaddrs_by_interface``
+
+    * calls ``get_interfaces``
+
+* those that may fall into the above categories, but whose use is only
+  related to netfailover (which relies on a Linux-specific network
+  driver, so is unlikely to be relevant elsewhere without a substantial
+  refactor; these probably only need implementing in
+  ``LinuxNetworking``):
+
+  * ``get_dev_features``
+
+  * ``has_netfail_standby_feature``
+
+    * calls ``get_dev_features``
+
+  * ``is_netfailover``
+  * ``is_netfail_master``
+
+    * this is called from ``generate_fallback_config``
+
+  * ``is_netfail_primary``
+  * ``is_netfail_standby``
+
+  * N.B. all of these take an optional ``driver`` argument which is
+    used to pass around a value to avoid having to look it up by
+    calling ``device_driver`` every time.  This is something of a leaky
+    abstraction, and is better served by caching on ``device_driver``
+    or storing the cached value on ``self``, so we can drop the
+    parameter from the new API.
+
+* those that use ``/sys`` (via helpers) and have non-exhaustive BSD
+  logic:
+
+  * ``get_devicelist``
+
+* those that already have separate Linux/BSD implementations:
+
+  * ``find_fallback_nic``
+  * ``get_interfaces_by_mac``
+
+* those that have no OS-specific functionality (so do not need to be
+  refactored):
+
+  * ``ParserError``
+  * ``RendererNotFoundError``
+  * ``has_url_connectivity``
+  * ``is_ip_address``
+  * ``is_ipv4_address``
+  * ``natural_sort_key``
+
+Note that the functions in ``cloudinit.net`` use inconsistent parameter
+names for "string that contains a device name"; we can standardise on
+``devname`` (the most common one) in the refactor.
+
 References
 ~~~~~~~~~~
 
@@ -450,3 +628,6 @@ References
 .. _Mina Galić's email the the cloud-init ML in 2018: https://lists.launchpad.net/cloud-init/msg00185.html
 .. _Mina Galić's email to the cloud-init ML in 2019: https://lists.launchpad.net/cloud-init/msg00237.html
 .. _PR #363: https://github.com/canonical/cloud-init/pull/363
+.. _this comment in PR #363: https://github.com/canonical/cloud-init/pull/363#issuecomment-628829489
+.. _abstractmethod: https://docs.python.org/3/library/abc.html#abc.abstractmethod
+.. _type aliases: https://docs.python.org/3/library/typing.html#type-aliases
diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py
index 016ba64d..89940cf0 100755
--- a/cloudinit/distros/__init__.py
+++ b/cloudinit/distros/__init__.py
@@ -29,6 +29,7 @@ from cloudinit import subp
 from cloudinit import util
 
 from cloudinit.distros.parsers import hosts
+from .networking import LinuxNetworking
 
 
 # Used when a cloud-config module can be run on all cloud-init distibutions.
@@ -67,11 +68,13 @@ class Distro(metaclass=abc.ABCMeta):
     init_cmd = ['service']  # systemctl, service etc
     renderer_configs = {}
     _preferred_ntp_clients = None
+    networking_cls = LinuxNetworking
 
     def __init__(self, name, cfg, paths):
         self._paths = paths
         self._cfg = cfg
         self.name = name
+        self.networking = self.networking_cls()
 
     @abc.abstractmethod
     def install_packages(self, pkglist):
diff --git a/cloudinit/distros/bsd.py b/cloudinit/distros/bsd.py
index c2d1f77d..2ed7a7d5 100644
--- a/cloudinit/distros/bsd.py
+++ b/cloudinit/distros/bsd.py
@@ -7,11 +7,13 @@ from cloudinit import log as logging
 from cloudinit import net
 from cloudinit import subp
 from cloudinit import util
+from .networking import BSDNetworking
 
 LOG = logging.getLogger(__name__)
 
 
 class BSD(distros.Distro):
+    networking_cls = BSDNetworking
     hostname_conf_fn = '/etc/rc.conf'
     rc_conf_fn = "/etc/rc.conf"
 
diff --git a/cloudinit/distros/networking.py b/cloudinit/distros/networking.py
new file mode 100644
index 00000000..f9842889
--- /dev/null
+++ b/cloudinit/distros/networking.py
@@ -0,0 +1,137 @@
+import abc
+
+from cloudinit import net
+
+
+# Type aliases (https://docs.python.org/3/library/typing.html#type-aliases),
+# used to make the signatures of methods a little clearer
+DeviceName = str
+NetworkConfig = dict
+
+
+class Networking(metaclass=abc.ABCMeta):
+    """The root of the Networking hierarchy in cloud-init.
+
+    This is part of an ongoing refactor in the cloud-init codebase, for more
+    details see "``cloudinit.net`` -> ``cloudinit.distros.networking``
+    Hierarchy" in HACKING.rst for full details.
+    """
+
+    def _get_current_rename_info(self) -> dict:
+        return net._get_current_rename_info()
+
+    def _rename_interfaces(self, renames: list, *, current_info=None) -> None:
+        return net._rename_interfaces(renames, current_info=current_info)
+
+    def apply_network_config_names(self, netcfg: NetworkConfig) -> None:
+        return net.apply_network_config_names(netcfg)
+
+    def device_devid(self, devname: DeviceName):
+        return net.device_devid(devname)
+
+    def device_driver(self, devname: DeviceName):
+        return net.device_driver(devname)
+
+    def extract_physdevs(self, netcfg: NetworkConfig) -> list:
+        return net.extract_physdevs(netcfg)
+
+    def find_fallback_nic(self, *, blacklist_drivers=None):
+        return net.find_fallback_nic(blacklist_drivers=blacklist_drivers)
+
+    def generate_fallback_config(
+        self, *, blacklist_drivers=None, config_driver: bool = False
+    ):
+        return net.generate_fallback_config(
+            blacklist_drivers=blacklist_drivers, config_driver=config_driver
+        )
+
+    def get_devicelist(self) -> list:
+        return net.get_devicelist()
+
+    def get_ib_hwaddrs_by_interface(self) -> dict:
+        return net.get_ib_hwaddrs_by_interface()
+
+    def get_ib_interface_hwaddr(
+        self, devname: DeviceName, ethernet_format: bool
+    ):
+        return net.get_ib_interface_hwaddr(devname, ethernet_format)
+
+    def get_interface_mac(self, devname: DeviceName):
+        return net.get_interface_mac(devname)
+
+    def get_interfaces(self) -> list:
+        return net.get_interfaces()
+
+    def get_interfaces_by_mac(self) -> dict:
+        return net.get_interfaces_by_mac()
+
+    def get_master(self, devname: DeviceName):
+        return net.get_master(devname)
+
+    def interface_has_own_mac(
+        self, devname: DeviceName, *, strict: bool = False
+    ) -> bool:
+        return net.interface_has_own_mac(devname, strict=strict)
+
+    def is_bond(self, devname: DeviceName) -> bool:
+        return net.is_bond(devname)
+
+    def is_bridge(self, devname: DeviceName) -> bool:
+        return net.is_bridge(devname)
+
+    def is_connected(self, devname: DeviceName) -> bool:
+        return net.is_connected(devname)
+
+    def is_physical(self, devname: DeviceName) -> bool:
+        return net.is_physical(devname)
+
+    def is_present(self, devname: DeviceName) -> bool:
+        return net.is_present(devname)
+
+    def is_renamed(self, devname: DeviceName) -> bool:
+        return net.is_renamed(devname)
+
+    def is_up(self, devname: DeviceName) -> bool:
+        return net.is_up(devname)
+
+    def is_vlan(self, devname: DeviceName) -> bool:
+        return net.is_vlan(devname)
+
+    def is_wireless(self, devname: DeviceName) -> bool:
+        return net.is_wireless(devname)
+
+    def master_is_bridge_or_bond(self, devname: DeviceName) -> bool:
+        return net.master_is_bridge_or_bond(devname)
+
+    def wait_for_physdevs(
+        self, netcfg: NetworkConfig, *, strict: bool = True
+    ) -> None:
+        return net.wait_for_physdevs(netcfg, strict=strict)
+
+
+class BSDNetworking(Networking):
+    """Implementation of networking functionality shared across BSDs."""
+
+    pass
+
+
+class LinuxNetworking(Networking):
+    """Implementation of networking functionality common to Linux distros."""
+
+    def get_dev_features(self, devname: DeviceName) -> str:
+        return net.get_dev_features(devname)
+
+    def has_netfail_standby_feature(self, devname: DeviceName) -> bool:
+        return net.has_netfail_standby_feature(devname)
+
+    def is_netfailover(self, devname: DeviceName) -> bool:
+        return net.is_netfailover(devname)
+
+    def is_netfail_master(self, devname: DeviceName) -> bool:
+        return net.is_netfail_master(devname)
+
+    def is_netfail_primary(self, devname: DeviceName) -> bool:
+        return net.is_netfail_primary(devname)
+
+    def is_netfail_standby(self, devname: DeviceName) -> bool:
+        return net.is_netfail_standby(devname)