diff options
| -rw-r--r-- | HACKING.rst | 227 | ||||
| -rwxr-xr-x | cloudinit/distros/__init__.py | 3 | ||||
| -rw-r--r-- | cloudinit/distros/bsd.py | 2 | ||||
| -rw-r--r-- | cloudinit/distros/networking.py | 137 |
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) |