Merge ~paul-meyer/cloud-init:bug-1687712 into cloud-init:master

[Paul Meyer]

Fixes 1687712. Added some tests for mkfs that verify bug and fix

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

PASSED: Continuous integration, rev:4522bc620dcab48f9d46a938837cae327e9021b5
https://jenkins.ubuntu.com/server/job/cloud-init-ci/298/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/298
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/298
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/298
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/298

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

review: Approve (continuous-integration)

[Scott Moser (smoser)]

Paul,
Thanks for finding this bug and fixing.

Please don't take my comments as harsh. Your help is really appreciated.

There are a few small comments inline.

[Paul Meyer (paul-meyer)]

Of course, all comments much appreciated. @Chad, would adding warning for options that are ignored (like having "cmd" and "extra_opts") cover your comment as well?

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

PASSED: Continuous integration, rev:32d0737827f5998edb4ac72817820c510156cb41
https://jenkins.ubuntu.com/server/job/cloud-init-ci/300/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/300
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/300
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/300
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/300

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

review: Approve (continuous-integration)

[Chad Smith (chad.smith)]

Thanks for the bug and fix here paul.

[Chad Smith (chad.smith)]

Minor comments inline.

[Chad Smith (chad.smith)]

review: Approve

[Paul Meyer (paul-meyer)]

Updated code with CR suggestions

diff --git a/cloudinit/config/cc_disk_setup.py b/cloudinit/config/cc_disk_setup.py
index f49386e..5aeef4b 100644
--- a/cloudinit/config/cc_disk_setup.py
+++ b/cloudinit/config/cc_disk_setup.py
@@ -910,12 +910,21 @@ def mkfs(fs_cfg):
                         "must be set.", label)
 
     # Create the commands
+    shell = False
     if fs_cmd:
         fs_cmd = fs_cfg['cmd'] % {
             'label': label,
             'filesystem': fs_type,
             'device': device,
         }
+        shell = True
+
+        if overwrite:
+            LOG.warning('fs_setup option "overwrite" ignored because "cmd" ' +
+                        'was specified')
+        if fs_opts:
+            LOG.warning('fs_setup option "extra_opts" ignored because "cmd" ' +
+                        'was specified')
     else:
         # Find the mkfs command
         mkfs_cmd = util.which("mkfs.%s" % fs_type)
@@ -936,14 +945,14 @@ def mkfs(fs_cfg):
         if overwrite or device_type(device) == "disk":
             fs_cmd.append(lookup_force_flag(fs_type))
 
-    # Add the extends FS options
-    if fs_opts:
-        fs_cmd.extend(fs_opts)
+        # Add the extends FS options
+        if fs_opts:
+            fs_cmd.extend(fs_opts)
 
     LOG.debug("Creating file system %s on %s", label, device)
-    LOG.debug("     Using cmd: %s", " ".join(fs_cmd))
+    LOG.debug("     Using cmd: %s", str(fs_cmd))
     try:
-        util.subp(fs_cmd)
+        util.subp(fs_cmd, shell=shell)
     except Exception as e:
         raise Exception("Failed to exec of '%s':\n%s" % (fs_cmd, e))
 
diff --git a/doc/examples/cloud-config-disk-setup.txt b/doc/examples/cloud-config-disk-setup.txt
index 3e46a22..38ad052 100644
--- a/doc/examples/cloud-config-disk-setup.txt
+++ b/doc/examples/cloud-config-disk-setup.txt
@@ -155,11 +155,11 @@ fs_setup:
      filesystem: 'ext3'
      device: 'ephemeral0'
      partition: 'auto'
-   - label:  mylabl2
+   - label: mylabl2
      filesystem: 'ext4'
      device: '/dev/xvda1'
-   - special:
-     cmd: mkfs -t %(FILESYSTEM)s -L %(LABEL)s %(DEVICE)s
+   - cmd: mkfs -t %(filesystem)s -L %(label)s %(device)s
+     label: mylabl3
      filesystem: 'btrfs'
      device: '/dev/xvdh'
 
diff --git a/tests/unittests/test_handler/test_handler_disk_setup.py b/tests/unittests/test_handler/test_handler_disk_setup.py
index 7ff3922..36e53c7 100644
--- a/tests/unittests/test_handler/test_handler_disk_setup.py
+++ b/tests/unittests/test_handler/test_handler_disk_setup.py
@@ -17,6 +17,10 @@ class TestIsDiskUsed(TestCase):
         self.check_fs = self.patches.enter_context(
             mock.patch('{0}.check_fs'.format(mod_name)))
 
+    def tearDown(self):
+        super(TestIsDiskUsed, self).tearDown()
+        self.patches.close()
+
     def test_multiple_child_nodes_returns_true(self):
         self.enumerate_disk.return_value = (mock.MagicMock() for _ in range(2))
         self.check_fs.return_value = (mock.MagicMock(), None, mock.MagicMock())
@@ -147,4 +151,49 @@ class TestUpdateFsSetupDevices(TestCase):
             'filesystem': 'xfs'
         }, fs_setup)
 
+
[e-mail]('cloudinit.config.cc_disk_setup.find_device_node',
+            return_value=('/dev/xdb1', False))
[e-mail]('cloudinit.config.cc_disk_setup.device_type', return_value=None)
[e-mail]('cloudinit.config.cc_disk_setup.util.subp', return_value=('', ''))
+class TestMkfsCommandHandling(TestCase):
+
+    def test_with_cmd(self, subp, *args):
+        with self.assertLogs(
+                'cloudinit.config.cc_disk_setup') as logs:
+            cc_disk_setup.mkfs({
+                'cmd': 'mkfs -t %(filesystem)s -L %(label)s %(device)s',
+                'filesystem': 'ext4',
+                'device': '/dev/xdb1',
+                'label': 'with_cmd',
+                'extra_opts': ['should', 'generate', 'warning'],
+                'overwrite': 'should generate warning too'
+            })
+
+        self.assertIn(
+            'WARNING:cloudinit.config.cc_disk_setup:' +
+            'fs_setup option "extra_opts" ignored because "cmd" was specified',
+            logs.output)
+        self.assertIn(
+            'WARNING:cloudinit.config.cc_disk_setup:' +
+            'fs_setup option "overwrite" ignored because "cmd" was specified',
+            logs.output)
+
+        subp.assert_called_once_with(
+            'mkfs -t ext4 -L with_cmd /dev/xdb1', shell=True)
+
+    def test_without_cmd(self, subp, *args):
+        cc_disk_setup.mkfs({
+            'filesystem': 'ext4',
+            'device': '/dev/xdb1',
+            'label': 'without_cmd',
+            'extra_opts': ['are', 'added'],
+            'overwrite': True
+        })
+
+        subp.assert_called_once_with(
+            ['/sbin/mkfs.ext4', '/dev/xdb1',
+             '-L', 'without_cmd', '-F', 'are', 'added'],
+            shell=False)
+
 # vi: ts=4 expandtab