cloud-init-dev team mailing list archive

[Scott Moser <smoser@xxxxxxxxxx>]

Scott Moser has proposed merging ~smoser/cloud-init:bug/1717147-centos-cloudstack-dhclient into cloud-init:master.

Commit message:
CloudStack: consider dhclient lease files named with a hyphen.

A regression in 'get_latest_lease' made it ignore files starting
with 'dhclient-' rather than just 'dhclient.'.  The fix here is
to allow those files to be considered.

There is a lot more we could do here to better ensure that we pick
the most recent lease, but this change fixes the regression.

LP: #1717147

Requested reviews:
  cloud-init commiters (cloud-init-dev)
Related bugs:
  Bug #1717147 in cloud-init: "cloud-init 0.7.9 fails for CentOS 7.4 in Cloudstack"
  https://bugs.launchpad.net/cloud-init/+bug/1717147

For more details, see:
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/330846
-- 
Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:bug/1717147-centos-cloudstack-dhclient into cloud-init:master.

diff --git a/cloudinit/sources/DataSourceCloudStack.py b/cloudinit/sources/DataSourceCloudStack.py
index 0188d89..7e0f9bb 100644
--- a/cloudinit/sources/DataSourceCloudStack.py
+++ b/cloudinit/sources/DataSourceCloudStack.py
@@ -187,22 +187,36 @@ def get_dhclient_d():
     return None
 
 
-def get_latest_lease():
+def get_latest_lease(lease_d=None):
     # find latest lease file
-    lease_d = get_dhclient_d()
+    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
-    for file_name in lease_files:
-        if file_name.startswith("dhclient.") and \
-           (file_name.endswith(".lease") or file_name.endswith(".leases")):
-            abs_path = os.path.join(lease_d, file_name)
-            mtime = os.path.getmtime(abs_path)
-            if mtime > latest_mtime:
-                latest_mtime = mtime
-                latest_file = abs_path
+
+    # lease files are named inconsistently across distros.
+    # We assume that 'dhclient6' indicates ipv6 and ignore it.
+    # ubuntu:
+    #   dhclient.<iface>.leases, dhclient.leases, dhclient6.leases
+    # centos6:
+    #   dhclient-<iface>.leases, dhclient6.leases
+    # centos7: ('--' is not a typo)
+    #   dhclient--<iface>.lease, dhclient6.leases
+    for fname in lease_files:
+        if fname.startswith("dhclient6"):
+            # avoid files that start with dhclient6 assuming dhcpv6.
+            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
 
 
diff --git a/tests/unittests/test_datasource/test_cloudstack.py b/tests/unittests/test_datasource/test_cloudstack.py
index 2dc9030..80d8e5e 100644
--- a/tests/unittests/test_datasource/test_cloudstack.py
+++ b/tests/unittests/test_datasource/test_cloudstack.py
@@ -1,12 +1,16 @@
 # This file is part of cloud-init. See LICENSE file for license information.
 
 from cloudinit import helpers
-from cloudinit.sources.DataSourceCloudStack import DataSourceCloudStack
+from cloudinit.sources.DataSourceCloudStack import (
+    DataSourceCloudStack, get_latest_lease)
 
-from cloudinit.tests.helpers import TestCase, mock, ExitStack
+from cloudinit.tests.helpers import CiTestCase, ExitStack, mock
 
+import os
+import time
 
-class TestCloudStackPasswordFetching(TestCase):
+
+class TestCloudStackPasswordFetching(CiTestCase):
 
     def setUp(self):
         super(TestCloudStackPasswordFetching, self).setUp()
@@ -89,4 +93,66 @@ class TestCloudStackPasswordFetching(TestCase):
     def test_password_not_saved_if_bad_request(self):
         self._check_password_not_saved_for('bad_request')
 
+
+class TestGetLatestLease(CiTestCase):
+
+    def _populate_dir_list(self, bdir, files):
+        """populate_dir_list([(name, data), (name, data)])
+
+        writes files to prefix in order provided, ensuring that
+        the mtime of files later in the list is after those before."""
+
+        for fname in files:
+            with open(os.path.sep.join((bdir, fname)), "w") as fp:
+                fp.write(fname)
+
+    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))
+
+    def test_skips_dhcpv6_files(self):
+        """files started with dhclient6 should be skipped."""
+        expected = "dhclient.lease"
+        self._pop_and_test([expected, "dhclient6.lease"], expected)
+
+    def test_selects_dhclient_dot_files(self):
+        """files named dhclient.lease or dhclient.leases should be used.
+
+        Ubuntu names files dhclient.eth0.leases dhclient6.leases and
+        sometimes dhclient.leases."""
+        self._pop_and_test(["dhclient.lease"], "dhclient.lease")
+        self._pop_and_test(["dhclient.leases"], "dhclient.leases")
+
+    def test_selects_dhclient_dash_files(self):
+        """files named dhclient-lease or dhclient-leases should be used.
+
+        Redhat/Centos names files with dhclient--eth0.lease (centos 7) or
+        dhclient-eth0.leases (centos 6).
+        """
+        self._pop_and_test(["dhclient-eth0.lease"], "dhclient-eth0.lease")
+        self._pop_and_test(["dhclient--eth0.lease"], "dhclient--eth0.lease")
+
+    def test_ignores_by_extension(self):
+        """only .lease or .leases file should be considered."""
+
+        self._pop_and_test(["dhclient.lease", "dhclient.lease.bk",
+                            "dhclient.lease-old", "dhclient.leaselease"],
+                           "dhclient.lease")
+
+    def test_selects_newest_matching(self):
+        """If multiple files match, the newest written should be used."""
+        test_start = int(time.time())
+        lease_d = self.tmp_dir()
+        self._populate_dir_list(lease_d, ["dhclient.lease", "dhclient.leases"])
+        dhclient_leases = self.tmp_path("dhclient.leases", lease_d)
+        dhclient_lease = self.tmp_path("dhclient.lease", lease_d)
+        self.assertEqual(dhclient_leases, get_latest_lease(lease_d))
+
+        # now update mtime on dhclient.leases, and re-check.
+        os.utime(dhclient_leases, times=((test_start - 1, test_start - 1)))
+        self.assertEqual(dhclient_lease, get_latest_lease(lease_d))
+
+
 # vi: ts=4 expandtab