cloud-init team mailing list archive

[Chad Smith <chad.smith@xxxxxxxxxxxxx>]

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 <cloud-init@DOMAIN.HIDDEN>
   - * From*: Igor Galić <igalic@xxxxxxxxxxxxx <igalic@DOMAIN.HIDDEN>>
   - * 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