cloud-init-dev team mailing list archive

[Ryan Harper <ryan.harper@xxxxxxxxxxxxx>]

I really like this branch; some comments inline.

Diff comments:

> diff --git a/cloudinit/analyze/tests/test_dump.py b/cloudinit/analyze/tests/test_dump.py
> index f4c4284..7bd834c 100644
> --- a/cloudinit/analyze/tests/test_dump.py
> +++ b/cloudinit/analyze/tests/test_dump.py
> @@ -59,37 +40,22 @@ class TestParseTimestamp(CiTestCase):
>          # convert stamp ourselves by adding the missing year value
>          year = datetime.now().year
>          dt = datetime.strptime(journal_stamp + " " + str(year), journal_fmt)
> -        expected = float(dt.strftime('%s.%f'))
> -        parsed = parse_timestamp(journal_stamp)
> -
> -        # use date(1)
> -        out, _ = subp(['date', '+%s.%6N', '-d', journal_stamp])
> -        timestamp = out.strip()
> -        date_ts = float(timestamp)
> -
> -        self.assertEqual(expected, parsed)
> -        self.assertEqual(expected, date_ts)
> -        self.assertEqual(date_ts, parsed)
> +        self.assertEqual(
> +            float(dt.strftime('%s.%f')), parse_timestamp(journal_stamp))
>  
> +    @skipIf(not which("date"), "'date' command not available.")

Hrm, doesn't this mean that a platform could pass make check/tox without the date command but at runtime it will fail?

>      def test_parse_unexpected_timestamp_format_with_date_command(self):
> -        """Dump sends unexpected timestamp formats to data for processing."""
> +        """Dump sends unexpected timestamp formats to date for processing."""
>          new_fmt = '%H:%M %m/%d %Y'
>          new_stamp = '17:15 08/08'
> -
>          # convert stamp ourselves by adding the missing year value
>          year = datetime.now().year
>          dt = datetime.strptime(new_stamp + " " + str(year), new_fmt)
> -        expected = float(dt.strftime('%s.%f'))
> -        parsed = parse_timestamp(new_stamp)
>  
>          # use date(1)
> -        out, _ = subp(['date', '+%s.%6N', '-d', new_stamp])
> -        timestamp = out.strip()
> -        date_ts = float(timestamp)
> -
> -        self.assertEqual(expected, parsed)
> -        self.assertEqual(expected, date_ts)
> -        self.assertEqual(date_ts, parsed)
> +        with self.allow_subp(["date"]):
> +            self.assertEqual(
> +                float(dt.strftime('%s.%f')), parse_timestamp(new_stamp))
>  
>  
>  class TestParseCILogLine(CiTestCase):
> diff --git a/cloudinit/cmd/tests/test_status.py b/cloudinit/cmd/tests/test_status.py
> index 37a8993..aded858 100644
> --- a/cloudinit/cmd/tests/test_status.py
> +++ b/cloudinit/cmd/tests/test_status.py
> @@ -39,7 +39,8 @@ class TestStatus(CiTestCase):
>          ensure_file(self.disable_file)  # Create the ignored disable file
>          (is_disabled, reason) = wrap_and_call(
>              'cloudinit.cmd.status',
> -            {'uses_systemd': False},
> +            {'uses_systemd': False,
> +             'get_cmdline': "root=/dev/my-root not-important"},

This isn't strictly related to removing use of subp, but a different host leak (ie, read from /proc) ?

>              status._is_cloudinit_disabled, self.disable_file, self.paths)
>          self.assertFalse(
>              is_disabled, 'expected enabled cloud-init on sysvinit')
> diff --git a/cloudinit/sources/DataSourceAltCloud.py b/cloudinit/sources/DataSourceAltCloud.py
> index 24fd65f..8dc63dc 100644
> --- a/cloudinit/sources/DataSourceAltCloud.py
> +++ b/cloudinit/sources/DataSourceAltCloud.py
> @@ -181,27 +181,21 @@ class DataSourceAltCloud(sources.DataSource):
>  
>          # modprobe floppy
>          try:
> -            cmd = CMD_PROBE_FLOPPY
> -            (cmd_out, _err) = util.subp(cmd)
> -            LOG.debug('Command: %s\nOutput%s', ' '.join(cmd), cmd_out)
> +            modprobe_floppy()
>          except ProcessExecutionError as e:
> -            util.logexc(LOG, 'Failed command: %s\n%s', ' '.join(cmd), e)
> +            util.logexc(LOG, 'Failed modprobe: %s', e)
>              return False
>          except OSError as e:
> -            util.logexc(LOG, 'Failed command: %s\n%s', ' '.join(cmd), e)
> +            util.logexc(LOG, 'No modprobe: %s', e)
>              return False
>  
>          floppy_dev = '/dev/fd0'
>  
>          # udevadm settle for floppy device
>          try:
> -            (cmd_out, _err) = util.udevadm_settle(exists=floppy_dev, timeout=5)
> -            LOG.debug('Command: %s\nOutput%s', ' '.join(cmd), cmd_out)
> -        except ProcessExecutionError as e:
> -            util.logexc(LOG, 'Failed command: %s\n%s', ' '.join(cmd), e)
> -            return False
> -        except OSError as e:
> -            util.logexc(LOG, 'Failed command: %s\n%s', ' '.join(cmd), e)

Also unrelated to the unittest changes, no?

> +            util.udevadm_settle(exists=floppy_dev, timeout=5)
> +        except (ProcessExecutionError, OSError) as e:
> +            util.logexc(LOG, 'Failed udevadm_settle: %s\n', e)
>              return False
>  
>          try:
> @@ -258,6 +252,11 @@ class DataSourceAltCloud(sources.DataSource):
>              return False
>  
>  
> +def modprobe_floppy():
> +    out, _err = util.subp(CMD_PROBE_FLOPPY)
> +    LOG.debug('Command: %s\nOutput%s', ' '.join(CMD_PROBE_FLOPPY), out)
> +
> +

This is for ease of mocking, which makes sense.  Is the above exception handling changes removing unused code paths?

>  # Used to match classes to dependencies
>  # Source DataSourceAltCloud does not really depend on networking.
>  # In the future 'dsmode' like behavior can be added to offer user
> diff --git a/tests/unittests/test_datasource/test_smartos.py b/tests/unittests/test_datasource/test_smartos.py
> index dca0b3d..860ee7d 100644
> --- a/tests/unittests/test_datasource/test_smartos.py
> +++ b/tests/unittests/test_datasource/test_smartos.py
> @@ -1066,7 +1094,11 @@ class TestSerialConcurrency(TestCase):
>         there is only one session over a connection.  In contrast, in the
>         absence of proper locking multiple processes opening the same serial
>         port can corrupt each others' exchanges with the metadata server.
> +
> +       This takes on the order of 2 to 3 minutes to run.

We have this disabled by default?

>      """
> +    allowed_subp = ['mdata-get']
> +
>      def setUp(self):
>          self.mdata_proc = multiprocessing.Process(target=self.start_mdata_loop)
>          self.mdata_proc.start()


-- 
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/354001
Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:fix/fix-unittest-use-of-subp into cloud-init:master.