changed 5 years ago
Linked with GitHub

Refactoring cloudinit/net

Background

About a year ago i got involved with cloud-init to add better FreeBSD support.
This culuminated in https://code.launchpad.net/~i.galic/cloud-init/+git/cloud-init/+merge/358228

I'd like to come back to this, and more imporantly, I'd like to get some help before I get started with it.

@Goneri's Pull Request to introduce a FreeBSD renderer is also a big inspiration.
I'm currently testing this patch, but it seems to be "write-only" so far.

Our primary goal in the refactoring then will be to open up cloudinit/net to be extensible in different Distros:

Currently cloudinit/net is almost entirely Linux specific. This has lead Goneri to introduce some rather unfortunate constructions:

# https://github.com/canonical/cloud-init/blob/3f6192b39894761e8ddecefa3736ea0abbc35b90/cloudinit/net/__init__.py#L799-L803
def get_interfaces_by_mac():
    if util.is_FreeBSD():
        return get_interfaces_by_mac_on_freebsd()
    else:
        return get_interfaces_by_mac_on_linux()

and

# https://github.com/canonical/cloud-init/blob/3f6192b39894761e8ddecefa3736ea0abbc35b90/cloudinit/net/__init__.py#L333-L338
def find_fallback_nic(blacklist_drivers=None):
    """Return the name of the 'fallback' network device."""
    if util.is_FreeBSD():
        return find_fallback_nic_on_freebsd(blacklist_drivers)
    else:
        return find_fallback_nic_on_linux(blacklist_drivers)

This only gets worse with the additions of NetBSD and OpenBSD

OOP?

In lp#358228 we tried to preserve the current API by moving the actual implementations into cloudinit/net/linux.py and cloudinit/net/freebsd.py respectively and importing the correct implementation

This got tricky with functions that were already system independent and needed to be used in the system specific modules.
Perhaps it would be easier to pull this kind of design off with a class based aproach as we already use in Distro.

comments

from @Odd_Bloke:

I think a good way to work out how a class-based approach could work would be to select a single function in cloudinit.net which is Linux-specific (perhaps our old friend get_devicelist), and move that to a method on a Linux-specific network class. Then see what refactoring you have to perform to get Linux networking functioning again.
That would give us an idea of what a class-based approach would look like, without requiring (a) refactoring everything, or (b) working out the right behaviour on FreeBSD.
(I'm not saying we would necessarily land such a change, either, it may just serve as an investigative spike.)

Revisiting modules

In my previous attempt at refactoring cloudinit.net, i tried to move function by function to a new module, instead of moving it functionality.
Perhaps a better aproach would be by checking which functions from cloudinit/net are actually used throughout the code, and declare those as the public interface.
Then we can move most of cloudinit/net into cloudinit/net/linux.py, and only implement the public functions for FreeBSD.

code duplication

[]There appears to be some code duplication between the Ephemeral classes in cloudinit/net and the Distros:

class Distro(): # defines
    def _bring_up_device(self, device_name):
# it is specialised for Arch and Gentoo

vs

class EphemeralIPv4Network(object):
    def _bringup_device(self):
        """Perform the ip comands to fully setup the device."""

This class has a lot of that kind of duplication, and it would be wise to rethink it.

netinfo

The (public?) functions of this file do not seem to be used anywhere other than in:

# cloudinit/cmd/main.py
26:from cloudinit import netinfo
284:        sys.stderr.write("%s\n" % (netinfo.debug_info()))

… and getgateway() is never used.

[blackboxsw Thanks this was just fixed via Odd Bloke's PR #111]

Ironically, netdev_info() is exactly what i'd like to create for FreeBSD's ifconfig — because it provides so much info. All the info which on Linux we get from /sys/class/net/*/*

Select a repo