cloud-init-dev team mailing list archive

[Lars Kellogg-Stedman <lars@xxxxxxxxxx>]

I added a bunch of minor comments.  I'm going to try to find some time to actually test things out some time this week.

Diff comments:

> === added file 'cloudinit/cmd/dhclient_hook.py'
> --- cloudinit/cmd/dhclient_hook.py	1970-01-01 00:00:00 +0000
> +++ cloudinit/cmd/dhclient_hook.py	2016-07-08 14:29:22 +0000
> @@ -0,0 +1,56 @@
> +import os
> +from cloudinit import stages
> +import json
> +from cloudinit import log as logging

If you could sort these imports my brain would feel better.

> +
> +
> +class logDhclient():
> +
> +    LOG = logging.getLogger(__name__)
> +
> +    def __init__(self):
> +        self.hooks_dir = self._get_hooks_dir()
> +        print(self.hooks_dir)
> +
> +    @staticmethod
> +    def _get_hooks_dir():
> +        i = stages.Init()
> +        return os.path.join(i.paths.get_runpath(), 'dhclient.hooks')
> +
> +    def _check_hooks_dir(self):
> +        if not os.path.exists(self.hooks_dir):
> +            os.makedirs(self.hooks_dir)
> +        else:
> +            hook_files = [os.path.join(self.hooks_dir, x)
> +                          for x in os.listdir(self.hooks_dir)]
> +            print(hook_files)

Is this print statement really supposed to be here?

> +            if len(hook_files) > 0:

Since hook_files is already a list, you don't need to check its len().  "for hook_file in hook_files" won't execute anything if the list is empty.

> +                for hook_file in hook_files:
> +                    os.remove(hook_file)
> +
> +    @staticmethod
> +    def pp_json(json_data):

This method doesn't seem to be used anywhere?

> +        return json.dumps(json_data, indent=4)
> +
> +    @staticmethod
> +    def get_vals(info):
> +        new_info = {}
> +        for k, v in info.iteritems():
> +            if k.startswith("DHCP4_") or k.startswith("new_"):
> +                key = (k.replace('DHCP4_', '').replace('new_', '')).lower()
> +                new_info[key] = v
> +        return new_info

You could replace all of the above with: return dict((k.replace('DHCP4_', '').replace('new_', ''), v) for k, v in info.items())

> +
> +    def record(self):
> +        envs = dict(os.environ)

Why this?  os.environ is already a dictionary...

> +        ifc_name = envs.get("interface", envs.get("DEVICE_IFACE", 'foobar'))

Do we really want to default to the name "foobar"?  If we can't find either "interface"  or "DEVICE_IFACE", shouldn't we just give up?

> +        ifc_file_name = os.path.join(self.hooks_dir, ifc_name + '.json')
> +        with open(ifc_file_name, 'w') as outfile:
> +            json.dump(self.get_vals(envs), outfile, indent=4)
> +            self.LOG.debug("Wrote dhclient options in %s", ifc_file_name)
> +
> +
> +def main():
> +    record = logDhclient()
> +    record._check_hooks_dir()
> +    record.record()
> 
> === 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-07-08 14:29:22 +0000
> @@ -213,15 +227,60 @@
>          return socket.inet_ntoa(packed_bytes)
>  
>      @staticmethod
> +    def _get_value_from_leases_file():
> +        leases = []
> +        content = util.load_file('/var/lib/dhcp/dhclient.eth0.leases')

The path to the leases file really needs to be configurable.  E.g., on some distributions this will be /var/lib/dhclient/dhclient.leases.

> +        for line in content.splitlines():
> +            if 'unknown-245' in line:
> +                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 = {}
> +        hook_files = [os.path.join(WALinuxAgentShim._get_hooks_dir(), x)
> +                      for x in os.listdir(WALinuxAgentShim._get_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:
> +                LOG.info("%s is not valid JSON data", hook_file)
> +        return dhcp_options
> +
> +    @staticmethod
> +    def _get_value_from_dhcpoptions(dhcp_options):
> +        # 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:
> +                return _value
> +        return _value

Instead of two return statements, you could just "break" from the loop if _value is not None...

> +
> +    @staticmethod
>      def find_endpoint():
>          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
> +
> +        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")
> +            value = WALinuxAgentShim._get_value_from_leases_file()
> +
> +        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 'setup.py'
> --- setup.py	2016-06-14 21:56:51 +0000
> +++ setup.py	2016-07-08 14:29:22 +0000
> @@ -176,6 +176,8 @@
>          (ETC + '/cloud', glob('config/*.cfg')),
>          (ETC + '/cloud/cloud.cfg.d', glob('config/cloud.cfg.d/*')),
>          (ETC + '/cloud/templates', glob('templates/*')),
> +        (ETC + '/NetworkManager/dispatcher.d/', ['tools/NM-cloudinit']),
> +        (ETC + '/dhcp/dhclient-exit-hooks.d/', ['tools/cloud-init-log-dhcp']),

Note that RHEL7 does not support /etc/dhcp/dhclient-exit-hooks.d.  When networkmanager is not in use, RHEL/CentOS/etc 7 systems will only run /etc/dhcp/dhclient.d scripts (which have a different format).  This may be something that gets resolved in packaging.  Support for dhclient-exit-hooks.d was introduced in the dhcp client 4.3.x packages, while RHEL 7 has 4.2.x.

>          (USR_LIB_EXEC + '/cloud-init', ['tools/uncloud-init',
>                                          'tools/write-ssh-key-fingerprints']),
>          (USR + '/share/doc/cloud-init', [f for f in glob('doc/*') if is_f(f)]),
> @@ -204,14 +206,16 @@
>      author_email='scott.moser@xxxxxxxxxxxxx',
>      url='http://launchpad.net/cloud-init/',
>      packages=setuptools.find_packages(exclude=['tests']),
> -    scripts=['tools/cloud-init-per'],
> +    scripts=['tools/cloud-init-per'
> +             ],

This looks like a no-op change.

>      license='GPLv3',
>      data_files=data_files,
>      install_requires=requirements,
>      cmdclass=cmdclass,
>      entry_points={
>          'console_scripts': [
> -            'cloud-init = cloudinit.cmd.main:main'
> +            'cloud-init = cloudinit.cmd.main:main',
> +            'cloud-init-dhclient-hook = cloudinit.cmd.dhclient_hook:main'
>          ],
>      }
>  )


-- 
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.