Hi Igor, Thanks for the thoughts and suggestions on this Igor in IRC and on list so we can have boarder discussion about the intent of Ephemeral* content managers.
The original implementation of the EphemeralDHCPv4 and EphemeralIPv4Network context managers were shallow and intended initially to be used in cloud-init's "local" stage before networking was even set up on the vm. As such, it was simple to determine whether an interface was configured with the desired IP by trying to add that IP and handle a known 'already exists' error when that IP was already configured. But, that solution is a bit short-sighted per bug https://bugs.launchpad.net/cloud-init/+bug/1802598, As it stands now, the current docstring on EphemeralIPv4Network is incorrect : "No operations are performed if the provided interface is already connected." In reality, the docstring should say: "No operations are performed if the provided interface already has the specified IP." Minimally, I'd want cloud-init to fix the logic in EphemeralIPv4Network to check each aspect of the requested network configuration separately: IP, prefix/mask, broadcast, router IP and link status and ensure that all desired aspects of configuration are correct. EphemeralIPv4Network should not exit in success upon just seeing an IP address configured. That said, you sound interested in something more than that; a basic connectivity check prior to running any 'ip' commands. >> Scott brought up the issue of performance, of wasted cycles thru repeated checking if the link is_up(). But i'm convinced that the benefit from a consistent API is far greater than a few wasted calls that are probably easily cached by a modern Kernel. Yeah, I think I mentioned that concern on Friday about cost of our call sites which happen during the cloud-init 'local' (pre-network) stage where network is not yet up. I still feel like some call sites will know whether it is in a pre-network stage and not have to pay to additional cost of a connectivity checks. What do folks think about making the connectivity check an optional parameter to EphemeralIPv4Network? def __init__(self, interface, ip, prefix_or_mask, broadcast, router=None, connectivity_url=None): If connectivity_url is set, EphemeralIPv4Network would check connectivity to the connectivity_url, then EphemeralIPv4Network would perform some basic connectivity check and noop if unnecessary. Otherwise, it would setup the desired network configuration provided. The problem we would have to solve with EphemeralIPv4Network being 'smart' about connection checking is how to determine what is the 'right' connectivity to check? * Is connectivity just as simple as the link on the desired interface being in 'up' state operstate per net.is_up? * Is connectivity just net.is_connected() which checks iflink state and carrier (for wireless) on the desired interface? * Is connectivity the ability to talk to the router IP or a specific external destination address? * Does EphemeralIPv4 have to make sure we have 'connectivity' using the specific interface name requested to a destination address or will acess from a different interface meet that requirement? I would vote for an optional connectivity_url or check_connection param to EphemeralDHCP if we can nail down what we think "connectivity" means per the above questions. At least with an optional parameter, we can avoid connectivity checks for paths that don't need to worry about it. Best regards, Chad Smith - * To*: cloud-init@xxxxxxxxxxxxxxxxxxx <[email protected]> - * From*: Igor Galić <igalic@xxxxxxxxxxxxx <[email protected]>> - * Date*: Sat, 10 Nov 2018 23:22:22 +0100 Hi folks, i'm writing this so long as it's fresh to my memory, as we discussed it on Friday evening in IRC … despite the subject, I think it should be persisted somewhere ;) I've been working on "porting" coudinit/net to freebsd: - https://code.launchpad.net/~i.galic/cloud-init/+git/cloud-init/+merge/358228 - https://lists.launchpad.net/cloud-init/msg00185.html so my problem is with class EphemeralIPv4Networkhttps://git.launchpad.net/cloud-init/tree/cloudinit/net/__init__.py#n650 """Context manager which sets up temporary static network configuration. No operations are performed if the provided interface is already connected. If unconnected, bring up the interface with valid ip, prefix and broadcast. If router is provided setup a default route for that interface. Upon context exit, clean up the interface leaving no configuration behind. """ Let me emphasize, "No operations are performed if the provided interface is already connected." However, this is currently not done, or at least not directly: https://git.launchpad.net/cloud-init/tree/cloudinit/net/__init__.py#n684 def __enter__(self): """Perform ephemeral network setup if interface is not connected.""" self._bringup_device() if self.router: self._bringup_router() it's only that somewhere in _bringup_device()'s exception path, that "nothing" is done. Given this convoluted code, it's perhaps no wonder that EphemeralIPv4Network is currently suffering from this bug: - https://bugs.launchpad.net/cloud-init/+bug/1802598 I think from an API point of view, it's important that "No operation is performed". Especially since this class implements __enter__() and __exit__()[1] — and is used by six different sources in this exact way. Scott brought up the issue of performance, of wasted cycles thru repeated checking if the link is_up(). But i'm convinced that the benefit from a consistent API is far greater than a few wasted calls that are probably easily cached by a modern Kernel. To summarize: - EphemeralIPv4Network is currently not implemented to its own spec - EphemeralIPv4Network has basic functionality bugs[2] Fixing these issues would make porting cloudinit.net to non-linux platforms easier as well. --- [1]: see https://docs.python.org/3/reference/compound_stmts.html#with for that [2]: you could say it's not idempotent, but i would claim it's not functional
-- Mailing list: https://launchpad.net/~cloud-init Post to : [email protected] Unsubscribe : https://launchpad.net/~cloud-init More help : https://help.launchpad.net/ListHelp
