Merge ~jasonzio/cloud-init:requestMountAll into cloud-init:master

[Jason Zions]

cc_mounts: check if mount -a on no-change fstab path

Under some circumstances, cc_disk_setup may reformat volumes which
already appear in /etc/fstab (e.g. Azure ephemeral drive is reformatted
from NTFS to ext4 after service-heal). Normally, cc_mounts only calls
mount -a if it altered /etc/fstab. With this change cc_mounts will read
/proc/mounts and verify if configured mounts are already mounted and if
not raise flag to request a mount -a. This handles the case where no
changes to fstab occur but a mount -a is required due to change in
underlying device which prevented the .mount unit from running until
after disk was reformatted.

LP: #1825596

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

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

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

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

I would like to capture the alternative discussion here as well.

I've requested some more debug info in the bug linked.

IIUC, in the scenario described, the ephemeral disk may be replaced with one that is no longer formatted with ext4, so the existing entry in fstab for the ephemeral drive will no longer mount.

As soon as rootfs is mounted, the systemd generator for fstab runs and will create .mount units for each entry. I want to capture those errors to confirm that's what is really going wrong.

To the solution, I had suggested that in cc_mounts.py that the logic which is ensuring we have an entry in /etc/fstab for the supplied disks, could also check if the device is mounted, and if not, enable the 'mount -a' command in the activate_cmds list.

While we're only seeing the issue on Azure; it's possible that this would affect other platforms with a similar issue and fixing it in a common path would be preferable to adding additional per-datasource config.

[Ryan Harper (raharper)]

Thanks for the logs in the bug. I would prefer to avoid adding additional datasource config here if we can avoid it. I think if we check if we're missing a mount, that we can have cc_mounts do the mount -a for us. This would handle things more generally I think.

Something like this (untested):

% git diff cloudinit/config/cc_mounts.py
diff --git a/cloudinit/config/cc_mounts.py b/cloudinit/config/cc_mounts.py
index 339baba..f920ea2 100644
--- a/cloudinit/config/cc_mounts.py
+++ b/cloudinit/config/cc_mounts.py
@@ -439,6 +439,7 @@ def handle(_name, cfg, cloud, log, _args):

     cc_lines = []
     needswap = False
+ needmountall = False
     dirs = []
     for line in actlist:
         # write 'comment' in the fs_mntops, entry, claiming this
@@ -449,11 +450,18 @@ def handle(_name, cfg, cloud, log, _args):
             dirs.append(line[1])
         cc_lines.append('\t'.join(line))

+ # query current system mountpoints (reads from /proc/mounts)
+ mpoints = [v['mountpoint']
+ for k, v in util.mounts().items() if 'mountpoint' in v]
     for d in dirs:
         try:
             util.ensure_dir(d)
         except Exception:
             util.logexc(log, "Failed to make '%s' config-mount", d)
+ # if we find a mountpoint from the config not currently mounted
+ # then set the flag
+ if not needmountall and d not in mpoints:
+ needmountall = True

     sadds = [WS.sub(" ", n) for n in cc_lines]
     sdrops = [WS.sub(" ", n) for n in fstab_removed]
@@ -473,6 +481,8 @@ def handle(_name, cfg, cloud, log, _args):
         log.debug("No changes to /etc/fstab made.")
     else:
         log.debug("Changes to fstab: %s", sops)
+
+ if needmountall or len(sops) > 0:
         activate_cmds.append(["mount", "-a"])
         if uses_systemd:
             activate_cmds.append(["systemctl", "daemon-reload"])

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

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

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

review: Needs Fixing (continuous-integration)

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

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

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

review: Needs Fixing (continuous-integration)

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

PASSED: Continuous integration, rev:1688892cc8df87d5c841febf3504859e3b510c9f
https://jenkins.ubuntu.com/server/job/cloud-init-ci/697/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    IN_PROGRESS: Declarative: Post Actions

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

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

I've a unittest to verify the path we're interested in here:

http://paste.ubuntu.com/p/PpMFn92CwV/

Please apply that to you branch.

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

PASSED: Continuous integration, rev:82ebcc3f6150ee68f1b1b7544f0b2c2c61d14214
https://jenkins.ubuntu.com/server/job/cloud-init-ci/705/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    IN_PROGRESS: Declarative: Post Actions

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

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

Thanks for updating the branch and testing.

review: Approve

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

Commit message lints:
 - Line #2 has 223 too many characters. Line starts with: "Under some circumstances,"...

review: Needs Fixing

[Joshua Powers (powersj)]

Cleaned up commit message, remarking approve

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

review: Approve (continuous-integration)

diff --git a/cloudinit/config/cc_mounts.py b/cloudinit/config/cc_mounts.py
index 339baba..123ffb8 100644
--- a/cloudinit/config/cc_mounts.py
+++ b/cloudinit/config/cc_mounts.py
@@ -439,6 +439,7 @@ def handle(_name, cfg, cloud, log, _args):
 
     cc_lines = []
     needswap = False
+    need_mount_all = False
     dirs = []
     for line in actlist:
         # write 'comment' in the fs_mntops, entry,  claiming this
@@ -449,11 +450,18 @@ def handle(_name, cfg, cloud, log, _args):
             dirs.append(line[1])
         cc_lines.append('\t'.join(line))
 
+    mount_points = [v['mountpoint'] for k, v in util.mounts().items()
+                    if 'mountpoint' in v]
     for d in dirs:
         try:
             util.ensure_dir(d)
         except Exception:
             util.logexc(log, "Failed to make '%s' config-mount", d)
+        # dirs is list of directories on which a volume should be mounted.
+        # If any of them does not already show up in the list of current
+        # mount points, we will definitely need to do mount -a.
+        if not need_mount_all and d not in mount_points:
+            need_mount_all = True
 
     sadds = [WS.sub(" ", n) for n in cc_lines]
     sdrops = [WS.sub(" ", n) for n in fstab_removed]
@@ -473,6 +481,9 @@ def handle(_name, cfg, cloud, log, _args):
         log.debug("No changes to /etc/fstab made.")
     else:
         log.debug("Changes to fstab: %s", sops)
+        need_mount_all = True
+
+    if need_mount_all:
         activate_cmds.append(["mount", "-a"])
         if uses_systemd:
             activate_cmds.append(["systemctl", "daemon-reload"])
diff --git a/tests/unittests/test_handler/test_handler_mounts.py b/tests/unittests/test_handler/test_handler_mounts.py
index 8fea6c2..0fb160b 100644
--- a/tests/unittests/test_handler/test_handler_mounts.py
+++ b/tests/unittests/test_handler/test_handler_mounts.py
@@ -154,7 +154,15 @@ class TestFstabHandling(test_helpers.FilesystemMockingTestCase):
                        return_value=True)
 
         self.add_patch('cloudinit.config.cc_mounts.util.subp',
-                       'mock_util_subp')
+                       'm_util_subp')
+
+        self.add_patch('cloudinit.config.cc_mounts.util.mounts',
+                       'mock_util_mounts',
+                       return_value={
+                           '/dev/sda1': {'fstype': 'ext4',
+                                         'mountpoint': '/',
+                                         'opts': 'rw,relatime,discard'
+                                         }})
 
         self.mock_cloud = mock.Mock()
         self.mock_log = mock.Mock()
@@ -230,4 +238,24 @@ class TestFstabHandling(test_helpers.FilesystemMockingTestCase):
             fstab_new_content = fd.read()
             self.assertEqual(fstab_expected_content, fstab_new_content)
 
+    def test_no_change_fstab_sets_needs_mount_all(self):
+        '''verify unchanged fstab entries are mounted if not call mount -a'''
+        fstab_original_content = (
+            'LABEL=cloudimg-rootfs / ext4 defaults 0 0\n'
+            'LABEL=UEFI /boot/efi vfat defaults 0 0\n'
+            '/dev/vdb /mnt auto defaults,noexec,comment=cloudconfig 0 2\n'
+        )
+        fstab_expected_content = fstab_original_content
+        cc = {'mounts': [
+                 ['/dev/vdb', '/mnt', 'auto', 'defaults,noexec']]}
+        with open(cc_mounts.FSTAB_PATH, 'w') as fd:
+            fd.write(fstab_original_content)
+        with open(cc_mounts.FSTAB_PATH, 'r') as fd:
+            fstab_new_content = fd.read()
+            self.assertEqual(fstab_expected_content, fstab_new_content)
+        cc_mounts.handle(None, cc, self.mock_cloud, self.mock_log, [])
+        self.m_util_subp.assert_has_calls([
+            mock.call(['mount', '-a']),
+            mock.call(['systemctl', 'daemon-reload'])])
+
 # vi: ts=4 expandtab