networking: refactor wait_for_physdevs from cloudinit.net

[OddBloke]

• 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_physdevss one callsite to use the new location

[lucasmoura]

I don't know if we want this behavior, but should we parametrize the number of times to retry calling settle ? That would allow us to have a named variable here in the for loop and maybe modify that behavior for slower detection of physical devices.

[OddBloke]

IMO, it's easier to read if the value is hard-coded; you don't have to go and find the callers to check if they are passing in something other than the default. Until we have a compelling case to parameterise it (i.e. we want different behaviour for different callers), I'd prefer to leave it as-is. Is that reasonable?

[lucasmoura]

Yes, makes sense

[lucasmoura]

I don't have all the context regarding the creation of that abstraction, but my concern about it is that since this is an abstract method, we cannot ensure that the classes that implement it will follow the exists logic detailed in the description here.

Maybe we can enforce that by turning settle into this concrete implementation:

def settle(self, *, exists=None) -> None
    if exists is not None:
        exists = self.find_device_path(exists)
    self.settle_device(exists)

Then we would need to make both find_device_path and settle_device abstract. However, I am basing that only on what I saw on the LinuxNetworking implementation, so I don't know if that is a pattern we really want to enforce.

[OddBloke]

So in the Linux case, we're calling udevadm settle via util.udevadm_settle:

cloud-init/cloudinit/util.py

Lines 2628 to 2639 in 2b72791

	def udevadm_settle(exists=None, timeout=None):
	"""Invoke udevadm settle with optional exists and timeout parameters"""
	settle_cmd = ["udevadm", "settle"]
	if exists:
	# skip the settle if the requested path already exists
	if os.path.exists(exists):
	return
	settle_cmd.extend(['--exit-if-exists=%s' % exists])
	if timeout:
	settle_cmd.extend(['--timeout=%s' % timeout])
	return subp.subp(settle_cmd)

As you can see, exists is essentially an optimisation: it allows us to return sooner than we might otherwise, but it doesn't materially change the action being performed. I've updated the docstring to try and reflect this more accurately. Is that a reasonable resolution?

(As an aside: BSDNetworking implements this as a pass because there is no equivalent on BSD, so this is really Linux-specific behaviour we're talking about: this implementation will only vary if we have a distro that doesn't use udev, which I infer we don't[0] because this code is run unconditionally today.)

[0] Currently; I wonder if Alpine Linux uses udev.

[lucasmoura]

@OddBloke Yes, agreed

[igalic]

alpine uses eudev which is udev compatible

[blackboxsw]

minor nit on docstring and typing for exists param and a question about a followup PR. Land as you see fit.

[blackboxsw]

My reading of this parameter name made me think initially this was a boolean. This is probably just a poorly named parameter (could have been device_name).

For this PR, can we please add typing hints to this exists parameter definition and maybe make to docstring for :param exists mention first that it's a Optional string of a specific DeviceName. If given, only perform....

Would you be opposed to me providing a separate PR to rename that param in udevadm_settle and settle from exists to device_name? There is only one callsite that uses the "exists" param in DataSourceAltCloud.py.

[OddBloke]

On Tue, Jul 14, 2020 at 09:17:46AM -0700, Chad Smith wrote: minor nit on docstring and typing for exists param and a question about a followup PR. Land as you see fit. > @@ -102,10 +106,71 @@ def is_vlan(self, devname: DeviceName) -> bool: 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: My reading of this parameter name made me think initially this was a boolean. This is probably just a poorly named parameter (could have been device_name).

Yep, that's a good point, I should have given this more thought when refactoring.

For this PR, can we please add typing hints to this exists parameter definition

We cannot, unfortunately, because that would introduce a dependency on the `typing` module which isn't present in Python 3.4.

and maybe make to docstring for :param exists mention first that it's a Optional string of a specific DeviceName. If given, only perform....

I've instead added ":type exists: Optional[DeviceName]" to the docstring, per https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#info-field-lists (FWIW, it's not "a string of a specific DeviceName", it is a "DeviceName"; we should understand "DeviceName" to mean "a string containing a specific device name".)

Would you be opposed to me providing a separate PR to rename that param in udevadm_settle and settle from exists to device_name? There is only one callsite that uses the "exists" param in DataSourceAltCloud.py.

Not at all.

[blackboxsw]

+1 this is good for me. Forgot about the typing deps concerns.