Merge ~smoser/cloud-init:bug/1717147-centos-cloudstack-dhclient into cloud-init:master

[Scott Moser]

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

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

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

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

review: Needs Fixing (continuous-integration)

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

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

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

review: Needs Fixing (continuous-integration)

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

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

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

review: Needs Fixing (continuous-integration)

[Chad Smith (chad.smith)]

Looks good!

flake error and one line comment:
tests/unittests/test_datasource/test_cloudstack.py:113:5: E303 too many blank lines (2)

review: Approve

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

PASSED: Continuous integration, rev:2271ffbd0f2cba146ad938aeab876a9d9aca680e
https://jenkins.ubuntu.com/server/job/cloud-init-ci/311/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

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

review: Approve (continuous-integration)

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

PASSED: Continuous integration, rev:0e16612b7c4f64e34d412634471ceee5b9efbbdf
https://jenkins.ubuntu.com/server/job/cloud-init-ci/313/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

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

review: Approve (continuous-integration)

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..8e98e1b 100644
--- a/tests/unittests/test_datasource/test_cloudstack.py
+++ b/tests/unittests/test_datasource/test_cloudstack.py
@@ -1,12 +1,17 @@
 # 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 import util
+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 +94,72 @@ 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 bdir, and updates timestamps to ensure
+        that their mtime increases with each file."""
+
+        start = int(time.time())
+        for num, fname in enumerate(reversed(files)):
+            fpath = os.path.sep.join((bdir, fname))
+            util.write_file(fpath, fname.encode())
+            os.utime(fpath, (start - num, start - num))
+
+    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."""
+        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))
+
+        # 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))
+
+        self.assertEqual(valid_1_path, get_latest_lease(lease_d))
+
+
 # vi: ts=4 expandtab