Merge ~ma-brothier/cloud-init:cs-multi-dhcp into cloud-init:master

[Marc-Aurele Brothier]

CloudStack changes on DHCP leases crawling mechanism for VR address

Currently the DHCP leases are search in reverse chronological order to look for the virtual router IP inside the lease information. In case more than one interface is added to a VM using a DHCP service to configure it, the latest DHCP lease will be linked to this new interface instead of the default one, thus waiting for the fault back method to fetch the default gateway address.

This change looks for all DHCP leases, grab all potential IP addresses (removing potential ducplicate due to multiple lease information in the same file) for the VR and try all of them in alphabetical order of the interface names until a good match is found (correct response), with a last default to the default gateway IP as originally.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/697/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/697/rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/698/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/698/rebuild

review: Needs Fixing (continuous-integration)

[Scott Moser (smoser)]

Marc,

Thanks for this MP.

I have two thoughts:
a.) we should probably look into using the output of 'cloud-init dhclient-hook' which gets called by tools/hook-dhclient. That writes files in /run/cloud-init/dhclient.hooks/eth0.json which look like this below. I think its just a matter of then reading those files and looking for a dhcp_server_identifier key.

{
 "broadcast_address": "10.75.205.255",
 "dhcp_lease_time": "3600",
 "dhcp_message_type": "5",
 "dhcp_rebinding_time": "3150",
 "dhcp_renewal_time": "1800",
 "dhcp_server_identifier": "10.75.205.1",
 "domain_name": "lxd",
 "domain_name_servers": "10.75.205.1",
 "expiry": "1516216015",
 "host_name": "x1",
 "ip_address": "10.75.205.55",
 "network_number": "10.75.205.0",
 "next_server": "10.75.205.1",
 "routers": "10.75.205.1",
 "subnet_mask": "255.255.255.0"
}

b. I think that cloudstack is probably broken on Ubuntu 17.10 and bionic (18.04) and anywhere else that uses systemd-networkd. systemd-networkd doesnt' write the same leases files as dhclient, nor do the hooks get called. It does though provide info like:
$ cat /run/systemd/netif/leases/41
# This is private data. Do not parse.
ADDRESS=10.75.205.120
NETMASK=255.255.255.0
ROUTER=10.75.205.1
SERVER_ADDRESS=10.75.205.1
NEXT_SERVER=10.75.205.1
BROADCAST=10.75.205.255
T1=1609
T2=2959
LIFETIME=3600
DNS=10.75.205.1
DOMAINNAME=lxd
HOSTNAME=b1
CLIENTID=ffc011a99100020000ab118d8462b1b543ef0c

(Yes, i'm aware of the 'Do not parse' comment in the file, but there is currently no other way to get this information out of the networkd-systemd built in dhcp client).

[Scott Moser (smoser)]

Oh, also, if we *were* to need to parse dhcp leases files, there is now some code in cloudinit/net/dhcp.py that could be re-used or improved on.

[Scott Moser (smoser)]

So I was wrong about my 'b' above. that was fixed already. for systemd systems it currently just walks the leases dir in sorted order. these are numbers of the iface... not sorted like 'eth0', 'eth1' or any other mreaningful manner.

then the first entry that has that key it will return the value.

[Marc-Aurele Brothier (ma-brothier)]

Hi Scott,

If cloud-init creates a file for the lease of the interface it's taking
care, it's much better than using the default system location. What if
cloud-init manage multiple interface as part of the cloud-config? I don't
think it's the case now to say that eth1 should be confogured through DHCP?

Does the failed tests are expected? Localy I had some failing on another
part of the code I didn't touch (or at least thought it's not related).

On Thu, Jan 18, 2018 at 7:33 PM, Scott Moser <email address hidden>
wrote:

> So I was wrong about my 'b' above. that was fixed already. for systemd
> systems it currently just walks the leases dir in sorted order. these are
> numbers of the iface... not sorted like 'eth0', 'eth1' or any other
> mreaningful manner.
>
> then the first entry that has that key it will return the value.
>
> --
> https://code.launchpad.net/~ma-brothier/cloud-init/+git/
> cloud-init/+merge/336145
> You are the owner of ~ma-brothier/cloud-init:cs-multi-dhcp.
>

[Scott Moser (smoser)]

Marc,
cloud-init only currently creates the files in /run/ on azure. We can change that to do it also on CloudStack. It is optimized to only run on azure just to stop cloud-init (and python) in the hook path for dhcp and nic bringup everywhere.

I'm not sure I follow "What if cloud-init mange multiple interface..."

Tests should pass on trunk. nightly runs of tip can be seen:
 https://jenkins.ubuntu.com/server/view/cloud-init/job/cloud-init-ci-nightly/

its possible that there are some bad tests that interact poorly with the environment they're running in. unittests fail for you, please paste those failures somewhere and I'll look at helping out (feel free to ping in #cloud-init also).

[Marc-Aurele Brothier (ma-brothier)]

I've pushed a fixed for the failed test, it's running now locally.

Sorry for the confusion on my previous mail, it was unclear.Since
cloud-init does change the configuration of the first interface to be
configured through a DHCP, it would be great to read the JSON file to grab
the cloudstack password/metadata server address. Then if another interface
is configured through DHCP, cloud-init won't create the related json file,
am I right? Or would it be possible to pass a cloud-init config to state a
list of interface to be configured through a DHCP ?

On Fri, Jan 19, 2018 at 8:03 PM, Scott Moser <email address hidden>
wrote:

> Marc,
> cloud-init only currently creates the files in /run/ on azure. We can
> change that to do it also on CloudStack. It is optimized to only run on
> azure just to stop cloud-init (and python) in the hook path for dhcp and
> nic bringup everywhere.
>
> I'm not sure I follow "What if cloud-init mange multiple interface..."
>
> Tests should pass on trunk. nightly runs of tip can be seen:
> https://jenkins.ubuntu.com/server/view/cloud-init/job/
> cloud-init-ci-nightly/
>
> its possible that there are some bad tests that interact poorly with the
> environment they're running in. unittests fail for you, please paste those
> failures somewhere and I'll look at helping out (feel free to ping in
> #cloud-init also).
>
>
> --
> https://code.launchpad.net/~ma-brothier/cloud-init/+git/
> cloud-init/+merge/336145
> You are the owner of ~ma-brothier/cloud-init:cs-multi-dhcp.
>

[Marc-Aurele Brothier (ma-brothier)]

I'm force pushing the change to force a rebuild of the CI bot, but it does seem to work. How should I do?

1832cc7... by Marc-Aurele Brothier on 2018-03-13

CloudStack: go through all potential DHCP lease to find the meta/user-data server

Signed-off-by: Marc-Aurèle Brothier <email address hidden>

[Scott Moser (smoser)]

Started at
https://jenkins.ubuntu.com/server/job/cloud-init-ci/879/

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/879/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/879/rebuild

review: Needs Fixing (continuous-integration)

[Scott Moser (smoser)]

@Marc,

Can you rebase and push --force again ? it looks like you have one failing test. possibly it is just notn completely mocked. i didnt' look much further, but below is how it fails on my system.

$ tox-venv py27 - tests/unittests/test_datasource/test_cloudstack.py
envpython=python2.7
inside tox:py27 running: python -m nose tests/unittests/test_datasource/test_cloudstack.py
.........F...
======================================================================
FAIL: If multiple files match, all should be used.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/smoser-public/src/cloud-init/cloud-init/tests/unittests/test_datasource/test_cloudstack.py", line 173, in test_selects_all_matching
    get_leases(lease_d))
AssertionError: Lists differ: ['/tm[29 chars]lient.leases', '/tmp/ci-TestGetLeases.LS7lQh/d[62 chars]ase'] != ['/tm[29 chars]lient-eth3.lease', '/tmp/ci-TestGetLeases.LS7l[62 chars]ase']

First differing element 0:
/tmp/ci-TestGetLeases.LS7lQh/dhclient.leases
/tmp/ci-TestGetLeases.LS7lQh/dhclient-eth3.lease

- ['/tmp/ci-TestGetLeases.LS7lQh/dhclient.leases',
? -

+ ['/tmp/ci-TestGetLeases.LS7lQh/dhclient-eth3.lease',
? +++++

- '/tmp/ci-TestGetLeases.LS7lQh/dhclient.lease',
+ '/tmp/ci-TestGetLeases.LS7lQh/dhclient.leases',
? +

- '/tmp/ci-TestGetLeases.LS7lQh/dhclient-eth3.lease']
? -----

+ '/tmp/ci-TestGetLeases.LS7lQh/dhclient.lease']
-------------------- >> begin captured logging << --------------------
cloudinit.util: DEBUG: Writing to /tmp/ci-TestGetLeases.LS7lQh/dhclient.leases - wb: [644] 15 bytes
cloudinit.util: DEBUG: Writing to /tmp/ci-TestGetLeases.LS7lQh/dhclient.lease - wb: [644] 14 bytes
cloudinit.util: DEBUG: Writing to /tmp/ci-TestGetLeases.LS7lQh/dhclient.leases - wb: [644] 15 bytes
cloudinit.util: DEBUG: Writing to /tmp/ci-TestGetLeases.LS7lQh/dhclient.lease - wb: [644] 14 bytes
cloudinit.util: DEBUG: Writing to /tmp/ci-TestGetLeases.LS7lQh/dhclient-eth3.lease - wb: [644] 19 bytes
--------------------- >> end captured logging << ---------------------

----------------------------------------------------------------------
Ran 13 tests in 0.259s

FAILED (failures=1)

[Scott Moser (smoser)]

Bump.

@Marc,
I had a request above.

[Ryan Harper (raharper)]

Hi Marc,

I've marked this branch Work in Progress. Scott had left you some review comments that need applied before we can land this branch. Once you've done so (and likely rebase on master) please push and then update this merge proposal back to Needs Review and we'll pick things back up.

Unmerged commits

1832cc7... by Marc-Aurele Brothier on 2018-03-13

CloudStack: go through all potential DHCP lease to find the meta/user-data server

Signed-off-by: Marc-Aurèle Brothier <email address hidden>

diff --git a/cloudinit/sources/DataSourceCloudStack.py b/cloudinit/sources/DataSourceCloudStack.py
index 0df545f..e3e922b 100644
--- a/cloudinit/sources/DataSourceCloudStack.py
+++ b/cloudinit/sources/DataSourceCloudStack.py
@@ -13,23 +13,22 @@
 # This file is part of cloud-init. See LICENSE file for license information.
 
 import os
+import time
 from socket import inet_ntoa
 from struct import pack
-import time
 
 from cloudinit import ec2_utils as ec2
 from cloudinit import log as logging
-from cloudinit.net import dhcp
 from cloudinit import sources
 from cloudinit import url_helper as uhelp
 from cloudinit import util
+from cloudinit.net import dhcp
 
 LOG = logging.getLogger(__name__)
 
 
 class CloudStackPasswordServerClient(object):
-    """
-    Implements password fetching from the CloudStack password server.
+    """Implements password fetching from the CloudStack password server.
 
     http://cloudstack-administration.readthedocs.org/
        en/latest/templates.html#adding-password-management-to-your-templates
@@ -74,10 +73,9 @@ class DataSourceCloudStack(sources.DataSource):
         # Cloudstack has its metadata/userdata URLs located at
         # http://<virtual-router-ip>/latest/
         self.api_ver = 'latest'
-        self.vr_addr = get_vr_address()
-        if not self.vr_addr:
+        self.vr_addresses = get_vr_addresses()
+        if not self.vr_addresses:
             raise RuntimeError("No virtual router found!")
-        self.metadata_address = "http://%s/" % (self.vr_addr,)
         self.cfg = {}
 
     def _get_url_settings(self):
@@ -102,14 +100,17 @@ class DataSourceCloudStack(sources.DataSource):
     def wait_for_metadata_service(self):
         (max_wait, timeout) = self._get_url_settings()
 
-        urls = [uhelp.combine_url(self.metadata_address,
-                                  'latest/meta-data/instance-id')]
+        urls = [uhelp.combine_url("http://%s/" % vr_potential_ip,
+                                  'latest/meta-data/instance-id')
+                for vr_potential_ip in self.vr_addresses]
         start_time = time.time()
         url = uhelp.wait_for_url(urls=urls, max_wait=max_wait,
                                  timeout=timeout, status_cb=LOG.warn)
 
         if url:
             LOG.debug("Using metadata source: '%s'", url)
+            self.vr_addr = url.split('/')[2]
+            self.metadata_address = "http://%s/" % (self.vr_addr,)
         else:
             LOG.critical(("Giving up on waiting for the metadata from %s"
                           " after %s seconds"),
@@ -168,7 +169,7 @@ class DataSourceCloudStack(sources.DataSource):
 
 
 def get_default_gateway():
-    # Returns the default gateway ip address in the dotted format.
+    """Returns the default gateway ip address in the dotted format."""
     lines = util.load_file("/proc/net/route").splitlines()
     for line in lines:
         items = line.split("\t")
@@ -181,7 +182,7 @@ def get_default_gateway():
 
 
 def get_dhclient_d():
-    # find lease files directory
+    """Find lease files directory."""
     supported_dirs = ["/var/lib/dhclient", "/var/lib/dhcp",
                       "/var/lib/NetworkManager"]
     for d in supported_dirs:
@@ -191,15 +192,14 @@ def get_dhclient_d():
     return None
 
 
-def get_latest_lease(lease_d=None):
-    # find latest lease file
+def get_leases(lease_d=None):
+    """Find all lease files."""
     if lease_d is None:
         lease_d = get_dhclient_d()
     if not lease_d:
         return None
     lease_files = os.listdir(lease_d)
-    latest_mtime = -1
-    latest_file = None
+    valid_lease_files = []
 
     # lease files are named inconsistently across distros.
     # We assume that 'dhclient6' indicates ipv6 and ignore it.
@@ -215,18 +215,17 @@ def get_latest_lease(lease_d=None):
             continue
         if not (fname.endswith(".lease") or fname.endswith(".leases")):
             continue
-
-        abs_path = os.path.join(lease_d, fname)
-        mtime = os.path.getmtime(abs_path)
-        if mtime > latest_mtime:
-            latest_mtime = mtime
-            latest_file = abs_path
-    return latest_file
+        valid_lease_files.append(os.path.join(lease_d, fname))
+    return valid_lease_files
 
 
-def get_vr_address():
-    # Get the address of the virtual router via dhcp leases
-    # If no virtual router is detected, fallback on default gateway.
+def get_vr_addresses():
+    """Get a list of potential addresses for the virtual router via DHCP
+    leases. This overcomes the situation when multiple interfaces are
+    configured through DHCP, one of them will come from CloudStack VR and
+    all addresses should be tested until the meta-data server responds. The
+    latest entry will always be the default gateway.
+    """
     # See http://docs.cloudstack.apache.org/projects/cloudstack-administration/en/4.8/virtual_machines/user-data.html # noqa
 
     # Try networkd first...
@@ -234,27 +233,30 @@ def get_vr_address():
     if latest_address:
         LOG.debug("Found SERVER_ADDRESS '%s' via networkd_leases",
                   latest_address)
-        return latest_address
+        return [latest_address]
 
     # Try dhcp lease files next...
-    lease_file = get_latest_lease()
-    if not lease_file:
+    lease_files = get_leases()
+    if not lease_files:
         LOG.debug("No lease file found, using default gateway")
-        return get_default_gateway()
-
-    with open(lease_file, "r") as fd:
-        for line in fd:
-            if "dhcp-server-identifier" in line:
-                words = line.strip(" ;\r\n").split(" ")
-                if len(words) > 2:
-                    dhcptok = words[2]
-                    LOG.debug("Found DHCP identifier %s", dhcptok)
-                    latest_address = dhcptok
-    if not latest_address:
+        return [get_default_gateway()]
+
+    potential_addresses = []
+    for lease_file in lease_files:
+        with open(lease_file, "r") as fd:
+            for line in fd:
+                if "dhcp-server-identifier" in line:
+                    words = line.strip(" ;\r\n").split(" ")
+                    if len(words) > 2:
+                        dhcptok = words[2]
+                        if dhcptok not in potential_addresses:
+                            LOG.debug("Found DHCP identifier %s", dhcptok)
+                            potential_addresses.append(dhcptok)
+    if not potential_addresses:
         # No virtual router found, fallback on default gateway
         LOG.debug("No DHCP found, using default gateway")
-        return get_default_gateway()
-    return latest_address
+        return [get_default_gateway()]
+    return potential_addresses
 
 
 # Used to match classes to dependencies
@@ -263,8 +265,8 @@ datasources = [
 ]
 
 
-# Return a list of data sources that match this set of dependencies
 def get_datasource_list(depends):
+    """Return a list of data sources that match this set of dependencies."""
     return sources.list_from_depends(depends, datasources)
 
 # vi: ts=4 expandtab
diff --git a/tests/unittests/test_datasource/test_cloudstack.py b/tests/unittests/test_datasource/test_cloudstack.py
index d6d2d6b..29de7e4 100644
--- a/tests/unittests/test_datasource/test_cloudstack.py
+++ b/tests/unittests/test_datasource/test_cloudstack.py
@@ -1,15 +1,14 @@
 # This file is part of cloud-init. See LICENSE file for license information.
 
+import os
+import time
+
 from cloudinit import helpers
 from cloudinit import util
 from cloudinit.sources.DataSourceCloudStack import (
-    DataSourceCloudStack, get_latest_lease)
-
+    DataSourceCloudStack, get_leases)
 from cloudinit.tests.helpers import CiTestCase, ExitStack, mock
 
-import os
-import time
-
 
 class TestCloudStackPasswordFetching(CiTestCase):
 
@@ -21,9 +20,9 @@ class TestCloudStackPasswordFetching(CiTestCase):
         self.patches.enter_context(mock.patch('{0}.ec2'.format(mod_name)))
         self.patches.enter_context(mock.patch('{0}.uhelp'.format(mod_name)))
         default_gw = "192.201.20.0"
-        get_latest_lease = mock.MagicMock(return_value=None)
+        get_leases = mock.MagicMock(return_value=None)
         self.patches.enter_context(mock.patch(
-            mod_name + '.get_latest_lease', get_latest_lease))
+            mod_name + '.get_leases', get_leases))
 
         get_default_gw = mock.MagicMock(return_value=default_gw)
         self.patches.enter_context(mock.patch(
@@ -105,7 +104,7 @@ class TestCloudStackPasswordFetching(CiTestCase):
         self._check_password_not_saved_for('bad_request')
 
 
-class TestGetLatestLease(CiTestCase):
+class TestGetLeases(CiTestCase):
 
     def _populate_dir_list(self, bdir, files):
         """populate_dir_list([(name, data), (name, data)])
@@ -122,8 +121,8 @@ class TestGetLatestLease(CiTestCase):
     def _pop_and_test(self, files, expected):
         lease_d = self.tmp_dir()
         self._populate_dir_list(lease_d, files)
-        self.assertEqual(self.tmp_path(expected, lease_d),
-                         get_latest_lease(lease_d))
+        self.assertEqual([self.tmp_path(expected, lease_d)],
+                         get_leases(lease_d))
 
     def test_skips_dhcpv6_files(self):
         """files started with dhclient6 should be skipped."""
@@ -154,22 +153,24 @@ class TestGetLatestLease(CiTestCase):
                             "dhclient.lease-old", "dhclient.leaselease"],
                            "dhclient.lease")
 
-    def test_selects_newest_matching(self):
-        """If multiple files match, the newest written should be used."""
+    def test_selects_all_matching(self):
+        """If multiple files match, all should be used."""
         lease_d = self.tmp_dir()
         valid_1 = "dhclient.leases"
         valid_2 = "dhclient.lease"
         valid_1_path = self.tmp_path(valid_1, lease_d)
         valid_2_path = self.tmp_path(valid_2, lease_d)
 
-        self._populate_dir_list(lease_d, [valid_1, valid_2])
-        self.assertEqual(valid_2_path, get_latest_lease(lease_d))
+        self._populate_dir_list(lease_d, [valid_2, valid_1])
+        self.assertEqual([valid_1_path, valid_2_path], get_leases(lease_d))
 
-        # now update mtime on valid_2 to be older than valid_1 and re-check.
-        mtime = int(os.path.getmtime(valid_1_path)) - 1
-        os.utime(valid_2_path, (mtime, mtime))
+        # now add another lease file
+        valid_3 = "dhclient-eth3.lease"
+        valid_3_path = self.tmp_path(valid_3, lease_d)
+        self._populate_dir_list(lease_d, [valid_3, valid_2, valid_1])
 
-        self.assertEqual(valid_1_path, get_latest_lease(lease_d))
+        self.assertEqual([valid_1_path, valid_2_path, valid_3_path],
+                         get_leases(lease_d))
 
 
 # vi: ts=4 expandtab