cloud-init-dev team mailing list archive

[Scott Moser <smoser@xxxxxxxxxx>]

Brent,
this looks good. thank you for your patience.

a few comments in line.


Diff comments:

> 
> === modified file 'cloudinit/sources/helpers/azure.py'
> --- cloudinit/sources/helpers/azure.py	2016-05-12 20:52:56 +0000
> +++ cloudinit/sources/helpers/azure.py	2016-08-04 12:58:51 +0000
> @@ -187,17 +189,30 @@
>          '  </Container>',
>          '</Health>'])
>  
> -    def __init__(self):
> +    def __init__(self, lease_file):

here, let lease_file=None by default since that appears to be supported.

>          LOG.debug('WALinuxAgentShim instantiated...')
> -        self.endpoint = self.find_endpoint()
> +        self.dhcpoptions = None
> +        self._endpoint = None
>          self.openssl_manager = None
>          self.values = {}
> +        self.lease_file = lease_file
>  
>      def clean_up(self):
>          if self.openssl_manager is not None:
>              self.openssl_manager.clean_up()
>  
>      @staticmethod
> +    def _get_hooks_dir():
> +        _paths = stages.Init()
> +        return os.path.join(_paths.paths.get_runpath(), "dhclient.hooks")
> +
> +    @property
> +    def endpoint(self):
> +        if self._endpoint is None:
> +            self._endpoint = self.find_endpoint(self.lease_file)
> +        return self._endpoint
> +
> +    @staticmethod
>      def get_ip_from_lease_value(lease_value):
>          unescaped_value = lease_value.replace('\\', '')
>          if len(unescaped_value) > 4:
> @@ -213,15 +228,75 @@
>          return socket.inet_ntoa(packed_bytes)
>  
>      @staticmethod
> -    def find_endpoint():
> +    def _get_value_from_leases_file(lease_file):
> +        leases = []
> +        content = util.load_file(lease_file)
> +        LOG.debug("content is {}".format(content))
> +        for line in content.splitlines():
> +            if 'unknown-245' in line:
> +                # Example line from Ubuntu
> +                # option unknown-245 a8:3f:81:10;
> +                leases.append(line.strip(' ').split(' ', 2)[-1].strip(';\n"'))
> +        # Return the "most recent" one in the list
> +        if len(leases) < 1:
> +            return None
> +        else:
> +            return leases[-1]
> +
> +    @staticmethod
> +    def _load_dhclient_json():
> +        dhcp_options = {}
> +        hooks_dir = WALinuxAgentShim._get_hooks_dir()
> +        if not os.path.exists(hooks_dir):
> +            LOG.debug("%s not found.", hooks_dir)
> +            return None
> +        hook_files = [os.path.join(hooks_dir, x)
> +                      for x in os.listdir(hooks_dir)]
> +        for hook_file in hook_files:
> +            try:
> +                _file_dict = json.load(open(hook_file))
> +                dhcp_options[os.path.basename(
> +                    hook_file).replace('.json', '')] = _file_dict
> +            except ValueError:
> +                raise ValueError("%s is not valid JSON data", hook_file)
> +        return dhcp_options
> +
> +    @staticmethod
> +    def _get_value_from_dhcpoptions(dhcp_options):
> +        if dhcp_options is None:
> +            return None
> +        # the MS endpoint server is given to us as DHPC option 245
> +        _value = None
> +        for interface in dhcp_options:
> +            _value = dhcp_options[interface].get('unknown_245', None)
> +            if _value is not None:
> +                break
> +        return _value
> +
> +    @staticmethod
> +    def find_endpoint(lease_file):
>          LOG.debug('Finding Azure endpoint...')
> -        content = util.load_file('/var/lib/dhcp/dhclient.eth0.leases')
>          value = None
> -        for line in content.splitlines():
> -            if 'unknown-245' in line:
> -                value = line.strip(' ').split(' ', 2)[-1].strip(';\n"')
> -        if value is None:
> -            raise ValueError('No endpoint found in DHCP config.')
> +        # Option-245 stored in /run/cloud-init/dhclient.hooks/<ifc>.json
> +        # a dhclient exit hook that calls cloud-init-dhclient-hook
> +        LOG.debug("Lease file %s found", lease_file)
> +        dhcp_options = WALinuxAgentShim._load_dhclient_json()
> +        value = WALinuxAgentShim._get_value_from_dhcpoptions(dhcp_options)
> +        if value is None:
> +            # Fallback and check the leases file if unsuccessful
> +            LOG.debug("Unable to find endpoint in dhclient logs. "
> +                      " Falling back to check lease files")
> +            if lease_file is None:
> +                LOG.warn("No lease file was specified.")

this doesn't seem like a warning, right?
lease_file can be passed in. i thinkt his generally can use some logging improvements.

It looks like the lease_file provided is only used if the hooks failed to find something. but if the caller provided you with a non-None lease file, you should probably read *that* first.

> +                value = None
> +            else:
> +                value = WALinuxAgentShim._get_value_from_leases_file(
> +                    lease_file)
> +                LOG.debug("second value is {}".format(value))
> +
> +        if value is None:
> +            raise ValueError('No endpoint found.')
> +
>          endpoint_ip_address = WALinuxAgentShim.get_ip_from_lease_value(value)
>          LOG.debug('Azure endpoint found at %s', endpoint_ip_address)
>          return endpoint_ip_address
> 
> === modified file 'tests/unittests/test_datasource/test_azure_helper.py'
> --- tests/unittests/test_datasource/test_azure_helper.py	2016-06-10 21:22:17 +0000
> +++ tests/unittests/test_datasource/test_azure_helper.py	2016-08-04 12:58:51 +0000
> @@ -309,15 +319,15 @@
>              mock.patch.object(azure_helper.time, 'sleep', mock.MagicMock()))
>  
>      def test_http_client_uses_certificate(self):
> -        shim = azure_helper.WALinuxAgentShim()
> +        shim = azure_helper.WALinuxAgentShim(None)

Since you're passing None to WALinuxAgentShim, as the only param, can we change that to just have a default value for lease_file of None? that would kill all of these next several changes.

>          shim.register_with_azure_and_fetch_data()
>          self.assertEqual(
>              [mock.call(self.OpenSSLManager.return_value.certificate)],
>              self.AzureEndpointHttpClient.call_args_list)
>  
>      def test_correct_url_used_for_goalstate(self):
> +        shim = azure_helper.WALinuxAgentShim(None)
>          self.find_endpoint.return_value = 'test_endpoint'
> -        shim = azure_helper.WALinuxAgentShim()
>          shim.register_with_azure_and_fetch_data()
>          get = self.AzureEndpointHttpClient.return_value.get
>          self.assertEqual(
> 
> === added file 'tools/hook-dhclient'
> --- tools/hook-dhclient	1970-01-01 00:00:00 +0000
> +++ tools/hook-dhclient	2016-08-04 12:58:51 +0000
> @@ -0,0 +1,7 @@
> +#!/bin/bash
> +# Writes DHCP lease information into the cloud-init data directory.
> +# This file is sourced by dhclient so do not prepend 'exec' to the

you can nix the "do not prepend exec' bit.  saying its sourced is good enough.
Also, can mention dhclient-script(8) is what does it. maybe:

 # This script writes DHCP lease information into the clodu-init run directory
 # It is sourced, not executed.  For more information see dhclient-script(8).

> +# following.
> +
> +cloud-init dhclient_hook

i think we need some sort of case statement around the $OPERATION here.

> +
> 
> === added file 'tools/hook-network-manager'
> --- tools/hook-network-manager	1970-01-01 00:00:00 +0000
> +++ tools/hook-network-manager	2016-08-04 12:58:51 +0000
> @@ -0,0 +1,8 @@
> +#!/bin/bash
> +# Writes DHCP lease information into the cloud-init data directory.
> +#
> +
> +case "$1:$2" in
> +   *:up) exec cloud-init dhclient_hook
> +   *:down) ;;

shouldn't 'down' delete the files that 'up' wrote for the interface?
also, mention NetworkManager(8) here for doc.

> +esac


-- 
https://code.launchpad.net/~bbaude/cloud-init/azure_dhcp/+merge/298677
Your team cloud init development team is requested to review the proposed merge of lp:~bbaude/cloud-init/azure_dhcp into lp:cloud-init.