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.mo...@canonical.com', > 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. _______________________________________________ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp