cloud-init-dev team mailing list archive

[Scott Moser <smoser@xxxxxxxxxx>]

yikes. lots of comments.
hope this is not overwhelming.

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-06-29 16:34:09 +0000
> @@ -189,6 +189,7 @@
>  
>      def __init__(self):
>          LOG.debug('WALinuxAgentShim instantiated...')
> +        self.dhcpoptions = os.path.join("/run/cloud-init", "dhcpoptions")

paths.get_runpath should give you /run/cloud-init.

paths.get_runpath("dhcpoptions)

>          self.endpoint = self.find_endpoint()
>          self.openssl_manager = None
>          self.values = {}
> @@ -212,14 +213,30 @@
>              packed_bytes = unescaped_value.encode('utf-8')
>          return socket.inet_ntoa(packed_bytes)
>  
> -    @staticmethod
> -    def find_endpoint():
> +    def find_endpoint(self):
>          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"')
> +        try:
> +            # Option-245 stored in /run/cloud-init/dhcpoptions by dhclient

comment on what *wrote* that file (cloud-init-dhcp-options)

> +            content = util.load_file(self.dhcpoptions)
> +            dhcp_options = dict(line.split('=', 1) for line in
> +                                content.strip().split("\n"))
> +

The output of that program is intended to be sh style key=value (and could be key="value" or key='value').
Best to use shlex to parse that. https://gist.github.com/smoser/6466708 is another option, which actually uses bash, but shlex is probably sufficient.

> +            # Depending on the environment, the option can be
> +            # preceded by DHCP4 or new
> +            value = dhcp_options.get("DHCP4_UNKNOWN_245") if \
> +                dhcp_options.get("new_unknown_245") is None else \
> +                dhcp_options.get("new_unknown_245")

maybe:
 for name in ("DHCP_UNKNOWN_245", "new_unknown_245"):
    if name in dhcp_options:
       value = dhcp_options[name]
or
 value = dhcp_options.get("DHCP_UNKNOWN_245", dhcp_options.get("new_unknown_245"))

I dont like the '\' at the end of lines.

> +            LOG.debug("Option-245 has value of {}".format(value))

use %s for LOG messages to
a.) be more consistent with other cloud-init
b.) (not really an issue here) 'LOG.debug("foo %s", myvar)' is faster than LOG.debug("foo %s".format(myvar)) because 'myvar' is never converted to a string in the first.

> +        except Exception:
> +            # Fallback and check the leases file if unsuccessful
> +            LOG.debug("Unable to find {}.  Falling back to check lease "
> +                      "files".format(self.dhcpoptions))

%s also looks nicer as the ',' allows for normal line break opportunity.
  LOG.debug("Unable to find %s. Falling back to check lease files",
            self.dhcpoption)

> +            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"')

can you put the "get_value_from_leases_file()' into a method ?

> +        LOG.debug(value)
>          if value is None:
>              raise ValueError('No endpoint found in DHCP config.')
>          endpoint_ip_address = WALinuxAgentShim.get_ip_from_lease_value(value)
> 
> === modified file 'doc/sources/azure/README.rst'
> --- doc/sources/azure/README.rst	2013-07-25 18:37:10 +0000
> +++ doc/sources/azure/README.rst	2016-06-29 16:34:09 +0000
> @@ -9,8 +9,17 @@
>  The azure cloud-platform provides initial data to an instance via an attached
>  CD formated in UDF.  That CD contains a 'ovf-env.xml' file that provides some
>  information.  Additional information is obtained via interaction with the
> -"endpoint".  The ip address of the endpoint is advertised to the instance
> -inside of dhcp option 245.  On ubuntu, that can be seen in
> +"endpoint".
> +
> +To find the endpoint, we now leverage the dhcp client's ability to log its
> +known values on exit.  The endpoint servcer is special DHCP option 245.

servcer -> server

> +Depending on your networking stack, this can be done
> +by calling a script in /etc/dhcp/dhclient-exit-hooks or a file in
> +/etc/NetworkManager/dispatcher.d.  Both of these call /usr/bin/cloud-init-log-dhcp
> +which will write the client information to /run/cloud-init/dhcpoptions.
> +
> +If those files are not available, the fallback is to check the leases file
> +for the endpoint server (again option 245).  On ubuntu, that can be seen in
>  /var/lib/dhcp/dhclient.eth0.leases as a colon delimited hex value (example:
>  ``option unknown-245 64:41:60:82;`` is 100.65.96.130)
>  
> 
> === added file 'tools/NM-cloudinit'
> --- tools/NM-cloudinit	1970-01-01 00:00:00 +0000
> +++ tools/NM-cloudinit	2016-06-29 16:34:09 +0000
> @@ -0,0 +1,9 @@
> +#!/bin/bash
> +# 
> +# Writes DHCP lease information into the cloud-init data
> +# directory.
> +
> +MDFILE=/run/cloud-init/dhcpoptions
> +
> +cloud-init-dhcp-options
> +

#!/bin/sh
# single line comment (yours does fit in 80 chars)
MDFILE=/run/cloud-init/dhcpoptions exec cloud-init-dhcp-options

preferred to yours in that
a.) does not use bash (posix /bin/sh is almost certainly faster)
b.) without exec, your process waits around for exit of the other. 

splitting hairs, but if you can be faster, you might as well.

> 
> === added file 'tools/cloud-init-dhcp-options'
> --- tools/cloud-init-dhcp-options	1970-01-01 00:00:00 +0000
> +++ tools/cloud-init-dhcp-options	2016-06-29 16:34:09 +0000
> @@ -0,0 +1,15 @@
> +#!/bin/bash
> +# 
> +# Writes DHCP lease information into the cloud-init data
> +# directory.
> +
> +MDFILE=/run/cloud-init/dhcpoptions
> +
> +if [ ! -d $(dirname $MDFILE) ]; then
> +        mkdir -p $(dirname $MDFILE)
> +fi

[ -d "${MDFILE%/*}" ] || mkdir -p "${MDFILE%/*}"
will be much faster.

> +if [ -f $MDFILE ]; then
> +        rm -f $MDFILE
> +fi
> +
> +env | grep '^new_\|^DHCP4_' > $MDFILE

not sure what i feel about this.  Since at this point we're very explicitly only after a few variables...
maybe better for now to just list them.  i think that the environment that this thing is run in is described in dhclient-script(8).   should we check the $reason ?

This script is apparently sourced ('.') so it probably has to be /bin/sh rather than bash.  We should probably define a function and call taht, and use local variables to not pollute unintentionally.
 cloudinit_log_options() {
  local int=$interface
  local mdfile=/run/cloud-init/dhcpoptions
  [ -d "${mdfile%/*}" ] || ...
 }

What shall we do if there are multiple dhclients?  say eth0 and eth1.  That may not be the case on azure (yet), but it may be true elsewhere

> 
> === added file 'tools/cloud-init-log-dhcp'
> --- tools/cloud-init-log-dhcp	1970-01-01 00:00:00 +0000
> +++ tools/cloud-init-log-dhcp	2016-06-29 16:34:09 +0000
> @@ -0,0 +1,3 @@
> +#!/bin/bash

symlink ?
if not then the following, but that just seems so much better solved with a symlink.
#!/bin/sh
exec cloud-init-dhcp-options

> +
> +cloud-init-dhcp-options


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