cloud-init-dev team mailing list archive

[Chad Smith <chad.smith@xxxxxxxxxxxxx>]

Thanks for the bug and fix here paul.

Diff comments:

> 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" ' +

Thanks for these, the more logging the better the user understanding during failures.

Any objections to logging the actual command specified so the user can track which list item in fs_setup is problematic? (You'd have to tweak your unit tests a bit)

        if overwrite:
            LOG.warning(
                "fs_setup:overwrite ignored because cmd was "
                "specified: {}".format(fs_cmd))
        if fs_opts:
            LOG.warning(
                "fs_setup:extra_opts ignored because cmd was "
                "specified: {}".format(fs_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:

Thanks for clarifying in your earlier comment, yes you are right in your assessment, extra_opts and cmd can't be used together. I'll put up a documentation MP on this shortly to describe it once I chat with the team about use-cases for extra_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/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
> @@ -147,4 +151,49 @@ class TestUpdateFsSetupDevices(TestCase):
>              'filesystem': 'xfs'
>          }, fs_setup)
>  
> +
> +@mock.patch('cloudinit.config.cc_disk_setup.find_device_node',
> +            return_value=('/dev/xdb1', False))
> +@mock.patch('cloudinit.config.cc_disk_setup.device_type', return_value=None)
> +@mock.patch('cloudinit.config.cc_disk_setup.util.subp', return_value=('', ''))
> +class TestMkfsCommandHandling(TestCase):
> +
> +    def test_with_cmd(self, subp, *args):

Can we add a oneliner docstring about the intent of these tests:
def test_cmd_warns_about_overwrite_and_extra_opts(...)
"""mkfs honors cmd and logs warnings when extra_opts or overwrite are provided."""

> +        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):

test_overwrite_and_extra_opts_without_cmd(...)
    """mkfs observes extra_opts and overwrite settings when cmd is not present."""

> +        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


-- 
https://code.launchpad.net/~paul-meyer/cloud-init/+git/cloud-init/+merge/323532
Your team cloud init development team is requested to review the proposed merge of ~paul-meyer/cloud-init:bug-1687712 into cloud-init:master.