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

1 files changed, 204 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