cloud-init-dev team mailing list archive

[Scott Moser <smoser@xxxxxxxxxx>]

small questions in line.


Diff comments:

> diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py
> index d1740e5..8745ee7 100644
> --- a/cloudinit/net/__init__.py
> +++ b/cloudinit/net/__init__.py
> @@ -557,6 +558,67 @@ def get_interfaces():
>      return ret
>  
>  
> +class EphemeralIPv4Network(object):
> +    """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, tear down the interface leaving no configuration behind.
> +    """
> +
> +    def __init__(self, interface, ip, prefix_or_mask, broadcast, router=None):
> +        """Setup context manager and validate call signature.
> +
> +        @param interface: Name of the network interface to bring up.
> +        @param ip: IP address to assign to the interface.
> +        @param prefix_or_mask: Either netmask of the format X.X.X.X or an int
> +            prefix.
> +        @param broadcast: Broadcast address for the IPv4 network.
> +        @param route: Optionally the default gatway IP.
> +        """
> +        if not all([interface, ip, prefix_or_mask, broadcast]):
> +            raise ValueError(
> +                'Cannot init network on {0} with {1}/{2} and bcast {3}'.format(
> +                    interface, ip, prefix_or_mask, broadcast))
> +        try:
> +            self.prefix = mask_to_net_prefix(prefix_or_mask)
> +        except ValueError as e:
> +            raise ValueError(
> +                'Cannot setup network: {0}'.format(e))
> +        self.interface = interface
> +        self.ip = ip
> +        self.broadcast = broadcast
> +        self.router = router
> +        self.network_teardown = True
> +
> +    def __enter__(self):
> +        """Perform ephemeral network setup if interface is not connected."""
> +        if is_up(self.interface):

is_up just checks if the interface is up, not if it has this expected address, right?
the result of the __enter__ should be that there is an IP on that nic like you expected, not just that the nic is 'ip link set up'

> +            LOG.debug(
> +                "Skipping ephemeral network setup. %s is connected.",
> +                self.interface)
> +            self.network_teardown = False
> +            return
> +        util.subp([
> +            'ip', '-family', 'inet', 'addr', 'add',
> +            '%s/%s' % (self.ip, self.prefix), 'broadcast', self.broadcast,
> +            'dev', self.interface], capture=True)
> +        util.subp(
> +            ['ip', '-family', 'inet', 'link', 'set', 'dev', self.interface,
> +             'up'], capture=True)
> +        if self.router:
> +            util.subp(
> +                ['ip', '-4', 'route', 'add', 'default', 'via', self.router,
> +                 'dev', self.interface], capture=True)
> +
> +    def __exit__(self, excp_type, excp_value, excp_traceback):
> +        if self.network_teardown:
> +            util.subp(
> +                ['ip', 'link', 'set', 'dev', self.interface, 'down'],
> +                capture=True)

more cleanly we could only reverse the operations we *did*.
if we up'd the link, then set it down.
if we added an address then drop that address... if added that route...

> +
> +
>  class RendererNotFoundError(RuntimeError):
>      pass
>  
> diff --git a/tox.ini b/tox.ini
> index 1140f9b..0bf9ee8 100644
> --- a/tox.ini
> +++ b/tox.ini
> @@ -21,7 +21,11 @@ setenv =
>      LC_ALL = en_US.utf-8
>  
>  [testenv:pylint]
> -deps = pylint==1.7.1

why are these changes necessary? lint didn't previously have any depends in its environment.
if that was a mistake, then shouldn't we put *all* depends in? why just mock?

> +deps = 
> +    # requirements
> +    pylint==1.7.1
> +    # test-requirements
> +    mock==1.3.0
>  commands = {envpython} -m pylint {posargs:cloudinit}
>  
>  [testenv:py3]


-- 
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/327827
Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:unittests-in-cloudinit-package into cloud-init:master.