Merge ~larsks/cloud-init:bz/1542578 into cloud-init:master

[Lars Kellogg-Stedman]

cc_mounts: Do not add devices to fstab that are already present.

Do not add new entries to /etc/fstab for devices that already have an
existing fstab entry.

Resolves: rhbz#1542578

We had a customer who was booting images in OpenStack that had a pre-configured swap partition. Cloud-init was adding a second entry for the same partition. This commit resolves the problem by parsing device names from /etc/fstab and declining to generate new entries for devices that already exist.

As with the existing code, entries with the "comment=cloudconfig" option will be removed and potentially replaced.

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

PASSED: Continuous integration, rev:288fbfe09ef5846547e96212252c7e8cb8d53fd7
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1102/
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/1102/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

Thats my first quick pass.
This code is really in need of re-factoring.

And could really use unit tests.

[Lars Kellogg-Stedman (larsks)]

I agree re: refactoring, but I'm not sure I'm up to that right now. I'll make the changes you suggested and I'll add a test or two.

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

PASSED: Continuous integration, rev:acd63782c1c4c90bb541355615aaee91adf8f6ed
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1103/
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/1103/rebuild

review: Approve (continuous-integration)

[Lars Kellogg-Stedman (larsks)]

I've added a few unit tests for the fstab handling code.

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

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

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

review: Needs Fixing (continuous-integration)

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

PASSED: Continuous integration, rev:74671dc8366b4ed30224f27b397bbf0a42c7605a
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1106/
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/1106/rebuild

review: Approve (continuous-integration)

[Lars Kellogg-Stedman (larsks)]

Ping @smoser; let me know if this looks okay. Thanks!

[Scott Moser (smoser)]

Lars, thank you, and prefectly fine to poke me.
I'll try to take a look at this on monday.

one comment in line about some infrastructure that can make this easier.

Thank you for adding some tests!

Scott

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

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

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

review: Needs Fixing (continuous-integration)

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

PASSED: Continuous integration, rev:68c2ca587a00e86c8bb01a23a503e5a79d93e0a0
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1118/
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/1118/rebuild

review: Approve (continuous-integration)

[Lars Kellogg-Stedman (larsks)]

Updated to address Chad's comments.

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

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

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

review: Needs Fixing (continuous-integration)

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

PASSED: Continuous integration, rev:5bd9e873de60135b96729d1be2d044d7276e85f8
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1125/
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/1125/rebuild

review: Approve (continuous-integration)

[Chad Smith (chad.smith)]

This looks good to me. Thanks for the changes Lars!

review: Approve

[Scott Moser (smoser)]

Lars, can you rebase this ... it has conflicts on head and wont land.

review: Needs Fixing

[Lars Kellogg-Stedman (larsks)]

I've rebased this on b4ae0e1fb8a48a83ea325cf032eb1acb196ee31c.

[Scott Moser (smoser)]

https://jenkins.ubuntu.com/server/job/cloud-init-ci/24/console shall report and then i'll grab.

review: Approve

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

PASSED: Continuous integration, rev:e65b8058aa663c096de90854b56e98f390717636
https://jenkins.ubuntu.com/server/job/cloud-init-ci/24/
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/24/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

An upstream commit landed for this bug.

To view that commit see the following URL:
https://git.launchpad.net/cloud-init/commit/?id=529d48f6

diff --git a/cloudinit/config/cc_mounts.py b/cloudinit/config/cc_mounts.py
index b3b25c3..eca6ea3 100644
--- a/cloudinit/config/cc_mounts.py
+++ b/cloudinit/config/cc_mounts.py
@@ -76,6 +76,7 @@ DEVICE_NAME_FILTER = r"^([x]{0,1}[shv]d[a-z][0-9]*|sr[0-9]+)$"
 DEVICE_NAME_RE = re.compile(DEVICE_NAME_FILTER)
 WS = re.compile("[%s]+" % (whitespace))
 FSTAB_PATH = "/etc/fstab"
+MNT_COMMENT = "comment=cloudconfig"
 
 LOG = logging.getLogger(__name__)
 
@@ -327,6 +328,22 @@ def handle(_name, cfg, cloud, log, _args):
 
     LOG.debug("mounts configuration is %s", cfgmnt)
 
+    fstab_lines = []
+    fstab_devs = {}
+    fstab_removed = []
+
+    for line in util.load_file(FSTAB_PATH).splitlines():
+        if MNT_COMMENT in line:
+            fstab_removed.append(line)
+            continue
+
+        try:
+            toks = WS.split(line)
+        except Exception:
+            pass
+        fstab_devs[toks[0]] = line
+        fstab_lines.append(line)
+
     for i in range(len(cfgmnt)):
         # skip something that wasn't a list
         if not isinstance(cfgmnt[i], list):
@@ -336,12 +353,17 @@ def handle(_name, cfg, cloud, log, _args):
 
         start = str(cfgmnt[i][0])
         sanitized = sanitize_devname(start, cloud.device_name_to_device, log)
+        if sanitized != start:
+            log.debug("changed %s => %s" % (start, sanitized))
+
         if sanitized is None:
             log.debug("Ignoring nonexistent named mount %s", start)
             continue
+        elif sanitized in fstab_devs:
+            log.info("Device %s already defined in fstab: %s",
+                     sanitized, fstab_devs[sanitized])
+            continue
 
-        if sanitized != start:
-            log.debug("changed %s => %s" % (start, sanitized))
         cfgmnt[i][0] = sanitized
 
         # in case the user did not quote a field (likely fs-freq, fs_passno)
@@ -373,11 +395,17 @@ def handle(_name, cfg, cloud, log, _args):
     for defmnt in defmnts:
         start = defmnt[0]
         sanitized = sanitize_devname(start, cloud.device_name_to_device, log)
+        if sanitized != start:
+            log.debug("changed default device %s => %s" % (start, sanitized))
+
         if sanitized is None:
             log.debug("Ignoring nonexistent default named mount %s", start)
             continue
-        if sanitized != start:
-            log.debug("changed default device %s => %s" % (start, sanitized))
+        elif sanitized in fstab_devs:
+            log.debug("Device %s already defined in fstab: %s",
+                      sanitized, fstab_devs[sanitized])
+            continue
+
         defmnt[0] = sanitized
 
         cfgmnt_has = False
@@ -409,31 +437,18 @@ def handle(_name, cfg, cloud, log, _args):
         log.debug("No modifications to fstab needed")
         return
 
-    comment = "comment=cloudconfig"
     cc_lines = []
     needswap = False
     dirs = []
     for line in actlist:
         # write 'comment' in the fs_mntops, entry,  claiming this
-        line[3] = "%s,%s" % (line[3], comment)
+        line[3] = "%s,%s" % (line[3], MNT_COMMENT)
         if line[2] == "swap":
             needswap = True
         if line[1].startswith("/"):
             dirs.append(line[1])
         cc_lines.append('\t'.join(line))
 
-    fstab_lines = []
-    removed = []
-    for line in util.load_file(FSTAB_PATH).splitlines():
-        try:
-            toks = WS.split(line)
-            if toks[3].find(comment) != -1:
-                removed.append(line)
-                continue
-        except Exception:
-            pass
-        fstab_lines.append(line)
-
     for d in dirs:
         try:
             util.ensure_dir(d)
@@ -441,7 +456,7 @@ def handle(_name, cfg, cloud, log, _args):
             util.logexc(log, "Failed to make '%s' config-mount", d)
 
     sadds = [WS.sub(" ", n) for n in cc_lines]
-    sdrops = [WS.sub(" ", n) for n in removed]
+    sdrops = [WS.sub(" ", n) for n in fstab_removed]
 
     sops = (["- " + drop for drop in sdrops if drop not in sadds] +
             ["+ " + add for add in sadds if add not in sdrops])
diff --git a/cloudinit/tests/helpers.py b/cloudinit/tests/helpers.py
index 117a9cf..07059fd 100644
--- a/cloudinit/tests/helpers.py
+++ b/cloudinit/tests/helpers.py
@@ -111,12 +111,12 @@ class TestCase(unittest2.TestCase):
         super(TestCase, self).setUp()
         self.reset_global_state()
 
-    def add_patch(self, target, attr, **kwargs):
+    def add_patch(self, target, attr, *args, **kwargs):
         """Patches specified target object and sets it as attr on test
         instance also schedules cleanup"""
         if 'autospec' not in kwargs:
             kwargs['autospec'] = True
-        m = mock.patch(target, **kwargs)
+        m = mock.patch(target, *args, **kwargs)
         p = m.start()
         self.addCleanup(m.stop)
         setattr(self, attr, p)
diff --git a/tests/unittests/test_handler/test_handler_mounts.py b/tests/unittests/test_handler/test_handler_mounts.py
index fe492d4..8fea6c2 100644
--- a/tests/unittests/test_handler/test_handler_mounts.py
+++ b/tests/unittests/test_handler/test_handler_mounts.py
@@ -1,8 +1,6 @@
 # This file is part of cloud-init. See LICENSE file for license information.
 
 import os.path
-import shutil
-import tempfile
 
 from cloudinit.config import cc_mounts
 
@@ -18,8 +16,7 @@ class TestSanitizeDevname(test_helpers.FilesystemMockingTestCase):
 
     def setUp(self):
         super(TestSanitizeDevname, self).setUp()
-        self.new_root = tempfile.mkdtemp()
-        self.addCleanup(shutil.rmtree, self.new_root)
+        self.new_root = self.tmp_dir()
         self.patchOS(self.new_root)
 
     def _touch(self, path):
@@ -134,4 +131,103 @@ class TestSanitizeDevname(test_helpers.FilesystemMockingTestCase):
             cc_mounts.sanitize_devname(
                 'ephemeral0.1', lambda x: disk_path, mock.Mock()))
 
+
+class TestFstabHandling(test_helpers.FilesystemMockingTestCase):
+
+    swap_path = '/dev/sdb1'
+
+    def setUp(self):
+        super(TestFstabHandling, self).setUp()
+        self.new_root = self.tmp_dir()
+        self.patchOS(self.new_root)
+
+        self.fstab_path = os.path.join(self.new_root, 'etc/fstab')
+        self._makedirs('/etc')
+
+        self.add_patch('cloudinit.config.cc_mounts.FSTAB_PATH',
+                       'mock_fstab_path',
+                       self.fstab_path,
+                       autospec=False)
+
+        self.add_patch('cloudinit.config.cc_mounts._is_block_device',
+                       'mock_is_block_device',
+                       return_value=True)
+
+        self.add_patch('cloudinit.config.cc_mounts.util.subp',
+                       'mock_util_subp')
+
+        self.mock_cloud = mock.Mock()
+        self.mock_log = mock.Mock()
+        self.mock_cloud.device_name_to_device = self.device_name_to_device
+
+    def _makedirs(self, directory):
+        directory = os.path.join(self.new_root, directory.lstrip('/'))
+        if not os.path.exists(directory):
+            os.makedirs(directory)
+
+    def device_name_to_device(self, path):
+        if path == 'swap':
+            return self.swap_path
+        else:
+            dev = None
+
+        return dev
+
+    def test_fstab_no_swap_device(self):
+        '''Ensure that cloud-init adds a discovered swap partition
+        to /etc/fstab.'''
+
+        fstab_original_content = ''
+        fstab_expected_content = (
+            '%s\tnone\tswap\tsw,comment=cloudconfig\t'
+            '0\t0\n' % (self.swap_path,)
+        )
+
+        with open(cc_mounts.FSTAB_PATH, 'w') as fd:
+            fd.write(fstab_original_content)
+
+        cc_mounts.handle(None, {}, self.mock_cloud, self.mock_log, [])
+
+        with open(cc_mounts.FSTAB_PATH, 'r') as fd:
+            fstab_new_content = fd.read()
+            self.assertEqual(fstab_expected_content, fstab_new_content)
+
+    def test_fstab_same_swap_device_already_configured(self):
+        '''Ensure that cloud-init will not add a swap device if the same
+        device already exists in /etc/fstab.'''
+
+        fstab_original_content = '%s swap swap defaults 0 0\n' % (
+            self.swap_path,)
+        fstab_expected_content = fstab_original_content
+
+        with open(cc_mounts.FSTAB_PATH, 'w') as fd:
+            fd.write(fstab_original_content)
+
+        cc_mounts.handle(None, {}, self.mock_cloud, self.mock_log, [])
+
+        with open(cc_mounts.FSTAB_PATH, 'r') as fd:
+            fstab_new_content = fd.read()
+            self.assertEqual(fstab_expected_content, fstab_new_content)
+
+    def test_fstab_alternate_swap_device_already_configured(self):
+        '''Ensure that cloud-init will add a discovered swap device to
+        /etc/fstab even when there exists a swap definition on another
+        device.'''
+
+        fstab_original_content = '/dev/sdc1 swap swap defaults 0 0\n'
+        fstab_expected_content = (
+            fstab_original_content +
+            '%s\tnone\tswap\tsw,comment=cloudconfig\t'
+            '0\t0\n' % (self.swap_path,)
+        )
+
+        with open(cc_mounts.FSTAB_PATH, 'w') as fd:
+            fd.write(fstab_original_content)
+
+        cc_mounts.handle(None, {}, self.mock_cloud, self.mock_log, [])
+
+        with open(cc_mounts.FSTAB_PATH, 'r') as fd:
+            fstab_new_content = fd.read()
+            self.assertEqual(fstab_expected_content, fstab_new_content)
+
 # vi: ts=4 expandtab