Move subp into its own module. (#416)

This was painful, but it finishes a TODO from cloudinit/subp.py.

It moves the following from util to subp:
  ProcessExecutionError
  subp
  which
  target_path

I moved subp_blob_in_tempfile into cc_chef, which is its only caller.
That saved us from having to deal with it using write_file
and temp_utils from subp (which does not import any cloudinit things now).

It is arguable that 'target_path' could be moved to a 'path_utils' or
something, but in order to use it from subp and also from utils,
we had to get it out of utils.

6 files changed, 276 insertions, 49 deletions

diff --git a/cloudinit/tests/helpers.py b/cloudinit/tests/helpers.py
index f3ab7e8c..58f63b69 100644
--- a/cloudinit/tests/helpers.py
+++ b/cloudinit/tests/helpers.py
@@ -23,9 +23,10 @@ from cloudinit import distros
 from cloudinit import helpers as ch
 from cloudinit.sources import DataSourceNone
 from cloudinit.templater import JINJA_AVAILABLE
+from cloudinit import subp
 from cloudinit import util
 
-_real_subp = util.subp
+_real_subp = subp.subp
 
 # Used for skipping tests
 SkipTest = unittest.SkipTest
@@ -134,9 +135,9 @@ class CiTestCase(TestCase):
             self.old_handlers = self.logger.handlers
             self.logger.handlers = [handler]
         if self.allowed_subp is True:
-            util.subp = _real_subp
+            subp.subp = _real_subp
         else:
-            util.subp = self._fake_subp
+            subp.subp = self._fake_subp
 
     def _fake_subp(self, *args, **kwargs):
         if 'args' in kwargs:
@@ -171,7 +172,7 @@ class CiTestCase(TestCase):
             # Remove the handler we setup
             logging.getLogger().handlers = self.old_handlers
             logging.getLogger().level = None
-        util.subp = _real_subp
+        subp.subp = _real_subp
         super(CiTestCase, self).tearDown()
 
     def tmp_dir(self, dir=None, cleanup=True):
@@ -280,13 +281,13 @@ class FilesystemMockingTestCase(ResourceUsingTestCase):
                     mock.patch.object(mod, f, trap_func))
 
         # Handle subprocess calls
-        func = getattr(util, 'subp')
+        func = getattr(subp, 'subp')
 
         def nsubp(*_args, **_kwargs):
             return ('', '')
 
         self.patched_funcs.enter_context(
-            mock.patch.object(util, 'subp', nsubp))
+            mock.patch.object(subp, 'subp', nsubp))
 
         def null_func(*_args, **_kwargs):
             return None
diff --git a/cloudinit/tests/test_conftest.py b/cloudinit/tests/test_conftest.py
index 773ef8fe..a6537248 100644
--- a/cloudinit/tests/test_conftest.py
+++ b/cloudinit/tests/test_conftest.py
@@ -1,6 +1,6 @@
 import pytest
 
-from cloudinit import util
+from cloudinit import subp
 from cloudinit.tests.helpers import CiTestCase
 
 
@@ -9,17 +9,17 @@ class TestDisableSubpUsage:
 
     def test_using_subp_raises_assertion_error(self):
         with pytest.raises(AssertionError):
-            util.subp(["some", "args"])
+            subp.subp(["some", "args"])
 
     def test_typeerrors_on_incorrect_usage(self):
         with pytest.raises(TypeError):
             # We are intentionally passing no value for a parameter, so:
             #  pylint: disable=no-value-for-parameter
-            util.subp()
+            subp.subp()
 
     @pytest.mark.parametrize('disable_subp_usage', [False], indirect=True)
     def test_subp_usage_can_be_reenabled(self):
-        util.subp(['whoami'])
+        subp.subp(['whoami'])
 
     @pytest.mark.parametrize(
         'disable_subp_usage', [['whoami'], 'whoami'], indirect=True)
@@ -27,18 +27,18 @@ class TestDisableSubpUsage:
         # The two parameters test each potential invocation with a single
         # argument
         with pytest.raises(AssertionError) as excinfo:
-            util.subp(["some", "args"])
+            subp.subp(["some", "args"])
         assert "allowed: whoami" in str(excinfo.value)
-        util.subp(['whoami'])
+        subp.subp(['whoami'])
 
     @pytest.mark.parametrize(
         'disable_subp_usage', [['whoami', 'bash']], indirect=True)
     def test_subp_usage_can_be_conditionally_reenabled_for_multiple_cmds(self):
         with pytest.raises(AssertionError) as excinfo:
-            util.subp(["some", "args"])
+            subp.subp(["some", "args"])
         assert "allowed: whoami,bash" in str(excinfo.value)
-        util.subp(['bash', '-c', 'true'])
-        util.subp(['whoami'])
+        subp.subp(['bash', '-c', 'true'])
+        subp.subp(['whoami'])
 
 
 class TestDisableSubpUsageInTestSubclass(CiTestCase):
@@ -46,16 +46,16 @@ class TestDisableSubpUsageInTestSubclass(CiTestCase):
 
     def test_using_subp_raises_exception(self):
         with pytest.raises(Exception):
-            util.subp(["some", "args"])
+            subp.subp(["some", "args"])
 
     def test_typeerrors_on_incorrect_usage(self):
         with pytest.raises(TypeError):
-            util.subp()
+            subp.subp()
 
     def test_subp_usage_can_be_reenabled(self):
         _old_allowed_subp = self.allow_subp
         self.allowed_subp = True
         try:
-            util.subp(['bash', '-c', 'true'])
+            subp.subp(['bash', '-c', 'true'])
         finally:
             self.allowed_subp = _old_allowed_subp
diff --git a/cloudinit/tests/test_gpg.py b/cloudinit/tests/test_gpg.py
index 8dd57137..f96f5372 100644
--- a/cloudinit/tests/test_gpg.py
+++ b/cloudinit/tests/test_gpg.py
@@ -4,19 +4,19 @@
 from unittest import mock
 
 from cloudinit import gpg
-from cloudinit import util
+from cloudinit import subp
 from cloudinit.tests.helpers import CiTestCase
 
 
 @mock.patch("cloudinit.gpg.time.sleep")
-@mock.patch("cloudinit.gpg.util.subp")
+@mock.patch("cloudinit.gpg.subp.subp")
 class TestReceiveKeys(CiTestCase):
     """Test the recv_key method."""
 
     def test_retries_on_subp_exc(self, m_subp, m_sleep):
         """retry should be done on gpg receive keys failure."""
         retries = (1, 2, 4)
-        my_exc = util.ProcessExecutionError(
+        my_exc = subp.ProcessExecutionError(
             stdout='', stderr='', exit_code=2, cmd=['mycmd'])
         m_subp.side_effect = (my_exc, my_exc, ('', ''))
         gpg.recv_key("ABCD", "keyserver.example.com", retries=retries)
@@ -26,7 +26,7 @@ class TestReceiveKeys(CiTestCase):
         """If the final run fails, error should be raised."""
         naplen = 1
         keyid, keyserver = ("ABCD", "keyserver.example.com")
-        m_subp.side_effect = util.ProcessExecutionError(
+        m_subp.side_effect = subp.ProcessExecutionError(
             stdout='', stderr='', exit_code=2, cmd=['mycmd'])
         with self.assertRaises(ValueError) as rcm:
             gpg.recv_key(keyid, keyserver, retries=(naplen,))
@@ -36,7 +36,7 @@ class TestReceiveKeys(CiTestCase):
 
     def test_no_retries_on_none(self, m_subp, m_sleep):
         """retry should not be done if retries is None."""
-        m_subp.side_effect = util.ProcessExecutionError(
+        m_subp.side_effect = subp.ProcessExecutionError(
             stdout='', stderr='', exit_code=2, cmd=['mycmd'])
         with self.assertRaises(ValueError):
             gpg.recv_key("ABCD", "keyserver.example.com", retries=None)
diff --git a/cloudinit/tests/test_netinfo.py b/cloudinit/tests/test_netinfo.py
index 1c8a791e..e44b16d8 100644
--- a/cloudinit/tests/test_netinfo.py
+++ b/cloudinit/tests/test_netinfo.py
@@ -27,8 +27,8 @@ class TestNetInfo(CiTestCase):
     maxDiff = None
     with_logs = True
 
-    @mock.patch('cloudinit.netinfo.util.which')
-    @mock.patch('cloudinit.netinfo.util.subp')
+    @mock.patch('cloudinit.netinfo.subp.which')
+    @mock.patch('cloudinit.netinfo.subp.subp')
     def test_netdev_old_nettools_pformat(self, m_subp, m_which):
         """netdev_pformat properly rendering old nettools info."""
         m_subp.return_value = (SAMPLE_OLD_IFCONFIG_OUT, '')
@@ -36,8 +36,8 @@ class TestNetInfo(CiTestCase):
         content = netdev_pformat()
         self.assertEqual(NETDEV_FORMATTED_OUT, content)
 
-    @mock.patch('cloudinit.netinfo.util.which')
-    @mock.patch('cloudinit.netinfo.util.subp')
+    @mock.patch('cloudinit.netinfo.subp.which')
+    @mock.patch('cloudinit.netinfo.subp.subp')
     def test_netdev_new_nettools_pformat(self, m_subp, m_which):
         """netdev_pformat properly rendering netdev new nettools info."""
         m_subp.return_value = (SAMPLE_NEW_IFCONFIG_OUT, '')
@@ -45,8 +45,8 @@ class TestNetInfo(CiTestCase):
         content = netdev_pformat()
         self.assertEqual(NETDEV_FORMATTED_OUT, content)
 
-    @mock.patch('cloudinit.netinfo.util.which')
-    @mock.patch('cloudinit.netinfo.util.subp')
+    @mock.patch('cloudinit.netinfo.subp.which')
+    @mock.patch('cloudinit.netinfo.subp.subp')
     def test_netdev_freebsd_nettools_pformat(self, m_subp, m_which):
         """netdev_pformat properly rendering netdev new nettools info."""
         m_subp.return_value = (SAMPLE_FREEBSD_IFCONFIG_OUT, '')
@@ -57,8 +57,8 @@ class TestNetInfo(CiTestCase):
         print()
         self.assertEqual(FREEBSD_NETDEV_OUT, content)
 
-    @mock.patch('cloudinit.netinfo.util.which')
-    @mock.patch('cloudinit.netinfo.util.subp')
+    @mock.patch('cloudinit.netinfo.subp.which')
+    @mock.patch('cloudinit.netinfo.subp.subp')
     def test_netdev_iproute_pformat(self, m_subp, m_which):
         """netdev_pformat properly rendering ip route info."""
         m_subp.return_value = (SAMPLE_IPADDRSHOW_OUT, '')
@@ -72,8 +72,8 @@ class TestNetInfo(CiTestCase):
             '255.0.0.0   |   .    |', '255.0.0.0   |  host  |')
         self.assertEqual(new_output, content)
 
-    @mock.patch('cloudinit.netinfo.util.which')
-    @mock.patch('cloudinit.netinfo.util.subp')
+    @mock.patch('cloudinit.netinfo.subp.which')
+    @mock.patch('cloudinit.netinfo.subp.subp')
     def test_netdev_warn_on_missing_commands(self, m_subp, m_which):
         """netdev_pformat warns when missing both ip and 'netstat'."""
         m_which.return_value = None  # Niether ip nor netstat found
@@ -85,8 +85,8 @@ class TestNetInfo(CiTestCase):
             self.logs.getvalue())
         m_subp.assert_not_called()
 
-    @mock.patch('cloudinit.netinfo.util.which')
-    @mock.patch('cloudinit.netinfo.util.subp')
+    @mock.patch('cloudinit.netinfo.subp.which')
+    @mock.patch('cloudinit.netinfo.subp.subp')
     def test_netdev_info_nettools_down(self, m_subp, m_which):
         """test netdev_info using nettools and down interfaces."""
         m_subp.return_value = (
@@ -100,8 +100,8 @@ class TestNetInfo(CiTestCase):
                     'hwaddr': '.', 'up': True}},
             netdev_info("."))
 
-    @mock.patch('cloudinit.netinfo.util.which')
-    @mock.patch('cloudinit.netinfo.util.subp')
+    @mock.patch('cloudinit.netinfo.subp.which')
+    @mock.patch('cloudinit.netinfo.subp.subp')
     def test_netdev_info_iproute_down(self, m_subp, m_which):
         """Test netdev_info with ip and down interfaces."""
         m_subp.return_value = (
@@ -130,8 +130,8 @@ class TestNetInfo(CiTestCase):
             readResource("netinfo/netdev-formatted-output-down"),
             netdev_pformat())
 
-    @mock.patch('cloudinit.netinfo.util.which')
-    @mock.patch('cloudinit.netinfo.util.subp')
+    @mock.patch('cloudinit.netinfo.subp.which')
+    @mock.patch('cloudinit.netinfo.subp.subp')
     def test_route_nettools_pformat(self, m_subp, m_which):
         """route_pformat properly rendering nettools route info."""
 
@@ -147,8 +147,8 @@ class TestNetInfo(CiTestCase):
         content = route_pformat()
         self.assertEqual(ROUTE_FORMATTED_OUT, content)
 
-    @mock.patch('cloudinit.netinfo.util.which')
-    @mock.patch('cloudinit.netinfo.util.subp')
+    @mock.patch('cloudinit.netinfo.subp.which')
+    @mock.patch('cloudinit.netinfo.subp.subp')
     def test_route_iproute_pformat(self, m_subp, m_which):
         """route_pformat properly rendering ip route info."""
 
@@ -165,8 +165,8 @@ class TestNetInfo(CiTestCase):
         content = route_pformat()
         self.assertEqual(ROUTE_FORMATTED_OUT, content)
 
-    @mock.patch('cloudinit.netinfo.util.which')
-    @mock.patch('cloudinit.netinfo.util.subp')
+    @mock.patch('cloudinit.netinfo.subp.which')
+    @mock.patch('cloudinit.netinfo.subp.subp')
     def test_route_warn_on_missing_commands(self, m_subp, m_which):
         """route_pformat warns when missing both ip and 'netstat'."""
         m_which.return_value = None  # Niether ip nor netstat found
diff --git a/cloudinit/tests/test_subp.py b/cloudinit/tests/test_subp.py
index 448097d3..911c1f3d 100644
--- a/cloudinit/tests/test_subp.py
+++ b/cloudinit/tests/test_subp.py
@@ -2,10 +2,21 @@
 
 """Tests for cloudinit.subp utility functions"""
 
-from cloudinit import subp
+import json
+import os
+import sys
+import stat
+
+from unittest import mock
+
+from cloudinit import subp, util
 from cloudinit.tests.helpers import CiTestCase
 
 
+BASH = subp.which('bash')
+BOGUS_COMMAND = 'this-is-not-expected-to-be-a-program-name'
+
+
 class TestPrependBaseCommands(CiTestCase):
 
     with_logs = True
@@ -58,4 +69,218 @@ class TestPrependBaseCommands(CiTestCase):
         self.assertEqual('', self.logs.getvalue())
         self.assertEqual(expected, fixed_commands)
 
+
+class TestSubp(CiTestCase):
+    allowed_subp = [BASH, 'cat', CiTestCase.SUBP_SHELL_TRUE,
+                    BOGUS_COMMAND, sys.executable]
+
+    stdin2err = [BASH, '-c', 'cat >&2']
+    stdin2out = ['cat']
+    utf8_invalid = b'ab\xaadef'
+    utf8_valid = b'start \xc3\xa9 end'
+    utf8_valid_2 = b'd\xc3\xa9j\xc8\xa7'
+    printenv = [BASH, '-c', 'for n in "$@"; do echo "$n=${!n}"; done', '--']
+
+    def printf_cmd(self, *args):
+        # bash's printf supports \xaa.  So does /usr/bin/printf
+        # but by using bash, we remove dependency on another program.
+        return([BASH, '-c', 'printf "$@"', 'printf'] + list(args))
+
+    def test_subp_handles_bytestrings(self):
+        """subp can run a bytestring command if shell is True."""
+        tmp_file = self.tmp_path('test.out')
+        cmd = 'echo HI MOM >> {tmp_file}'.format(tmp_file=tmp_file)
+        (out, _err) = subp.subp(cmd.encode('utf-8'), shell=True)
+        self.assertEqual(u'', out)
+        self.assertEqual(u'', _err)
+        self.assertEqual('HI MOM\n', util.load_file(tmp_file))
+
+    def test_subp_handles_strings(self):
+        """subp can run a string command if shell is True."""
+        tmp_file = self.tmp_path('test.out')
+        cmd = 'echo HI MOM >> {tmp_file}'.format(tmp_file=tmp_file)
+        (out, _err) = subp.subp(cmd, shell=True)
+        self.assertEqual(u'', out)
+        self.assertEqual(u'', _err)
+        self.assertEqual('HI MOM\n', util.load_file(tmp_file))
+
+    def test_subp_handles_utf8(self):
+        # The given bytes contain utf-8 accented characters as seen in e.g.
+        # the "deja dup" package in Ubuntu.
+        cmd = self.printf_cmd(self.utf8_valid_2)
+        (out, _err) = subp.subp(cmd, capture=True)
+        self.assertEqual(out, self.utf8_valid_2.decode('utf-8'))
+
+    def test_subp_respects_decode_false(self):
+        (out, err) = subp.subp(self.stdin2out, capture=True, decode=False,
+                               data=self.utf8_valid)
+        self.assertTrue(isinstance(out, bytes))
+        self.assertTrue(isinstance(err, bytes))
+        self.assertEqual(out, self.utf8_valid)
+
+    def test_subp_decode_ignore(self):
+        # this executes a string that writes invalid utf-8 to stdout
+        (out, _err) = subp.subp(self.printf_cmd('abc\\xaadef'),
+                                capture=True, decode='ignore')
+        self.assertEqual(out, 'abcdef')
+
+    def test_subp_decode_strict_valid_utf8(self):
+        (out, _err) = subp.subp(self.stdin2out, capture=True,
+                                decode='strict', data=self.utf8_valid)
+        self.assertEqual(out, self.utf8_valid.decode('utf-8'))
+
+    def test_subp_decode_invalid_utf8_replaces(self):
+        (out, _err) = subp.subp(self.stdin2out, capture=True,
+                                data=self.utf8_invalid)
+        expected = self.utf8_invalid.decode('utf-8', 'replace')
+        self.assertEqual(out, expected)
+
+    def test_subp_decode_strict_raises(self):
+        args = []
+        kwargs = {'args': self.stdin2out, 'capture': True,
+                  'decode': 'strict', 'data': self.utf8_invalid}
+        self.assertRaises(UnicodeDecodeError, subp.subp, *args, **kwargs)
+
+    def test_subp_capture_stderr(self):
+        data = b'hello world'
+        (out, err) = subp.subp(self.stdin2err, capture=True,
+                               decode=False, data=data,
+                               update_env={'LC_ALL': 'C'})
+        self.assertEqual(err, data)
+        self.assertEqual(out, b'')
+
+    def test_subp_reads_env(self):
+        with mock.patch.dict("os.environ", values={'FOO': 'BAR'}):
+            out, _err = subp.subp(self.printenv + ['FOO'], capture=True)
+        self.assertEqual('FOO=BAR', out.splitlines()[0])
+
+    def test_subp_env_and_update_env(self):
+        out, _err = subp.subp(
+            self.printenv + ['FOO', 'HOME', 'K1', 'K2'], capture=True,
+            env={'FOO': 'BAR'},
+            update_env={'HOME': '/myhome', 'K2': 'V2'})
+        self.assertEqual(
+            ['FOO=BAR', 'HOME=/myhome', 'K1=', 'K2=V2'], out.splitlines())
+
+    def test_subp_update_env(self):
+        extra = {'FOO': 'BAR', 'HOME': '/root', 'K1': 'V1'}
+        with mock.patch.dict("os.environ", values=extra):
+            out, _err = subp.subp(
+                self.printenv + ['FOO', 'HOME', 'K1', 'K2'], capture=True,
+                update_env={'HOME': '/myhome', 'K2': 'V2'})
+
+        self.assertEqual(
+            ['FOO=BAR', 'HOME=/myhome', 'K1=V1', 'K2=V2'], out.splitlines())
+
+    def test_subp_warn_missing_shebang(self):
+        """Warn on no #! in script"""
+        noshebang = self.tmp_path('noshebang')
+        util.write_file(noshebang, 'true\n')
+
+        print("os is %s" % os)
+        os.chmod(noshebang, os.stat(noshebang).st_mode | stat.S_IEXEC)
+        with self.allow_subp([noshebang]):
+            self.assertRaisesRegex(subp.ProcessExecutionError,
+                                   r'Missing #! in script\?',
+                                   subp.subp, (noshebang,))
+
+    def test_subp_combined_stderr_stdout(self):
+        """Providing combine_capture as True redirects stderr to stdout."""
+        data = b'hello world'
+        (out, err) = subp.subp(self.stdin2err, capture=True,
+                               combine_capture=True, decode=False, data=data)
+        self.assertEqual(b'', err)
+        self.assertEqual(data, out)
+
+    def test_returns_none_if_no_capture(self):
+        (out, err) = subp.subp(self.stdin2out, data=b'', capture=False)
+        self.assertIsNone(err)
+        self.assertIsNone(out)
+
+    def test_exception_has_out_err_are_bytes_if_decode_false(self):
+        """Raised exc should have stderr, stdout as bytes if no decode."""
+        with self.assertRaises(subp.ProcessExecutionError) as cm:
+            subp.subp([BOGUS_COMMAND], decode=False)
+        self.assertTrue(isinstance(cm.exception.stdout, bytes))
+        self.assertTrue(isinstance(cm.exception.stderr, bytes))
+
+    def test_exception_has_out_err_are_bytes_if_decode_true(self):
+        """Raised exc should have stderr, stdout as string if no decode."""
+        with self.assertRaises(subp.ProcessExecutionError) as cm:
+            subp.subp([BOGUS_COMMAND], decode=True)
+        self.assertTrue(isinstance(cm.exception.stdout, str))
+        self.assertTrue(isinstance(cm.exception.stderr, str))
+
+    def test_bunch_of_slashes_in_path(self):
+        self.assertEqual("/target/my/path/",
+                         subp.target_path("/target/", "//my/path/"))
+        self.assertEqual("/target/my/path/",
+                         subp.target_path("/target/", "///my/path/"))
+
+    def test_c_lang_can_take_utf8_args(self):
+        """Independent of system LC_CTYPE, args can contain utf-8 strings.
+
+        When python starts up, its default encoding gets set based on
+        the value of LC_CTYPE.  If no system locale is set, the default
+        encoding for both python2 and python3 in some paths will end up
+        being ascii.
+
+        Attempts to use setlocale or patching (or changing) os.environ
+        in the current environment seem to not be effective.
+
+        This test starts up a python with LC_CTYPE set to C so that
+        the default encoding will be set to ascii.  In such an environment
+        Popen(['command', 'non-ascii-arg']) would cause a UnicodeDecodeError.
+        """
+        python_prog = '\n'.join([
+            'import json, sys',
+            'from cloudinit.subp import subp',
+            'data = sys.stdin.read()',
+            'cmd = json.loads(data)',
+            'subp(cmd, capture=False)',
+            ''])
+        cmd = [BASH, '-c', 'echo -n "$@"', '--',
+               self.utf8_valid.decode("utf-8")]
+        python_subp = [sys.executable, '-c', python_prog]
+
+        out, _err = subp.subp(
+            python_subp, update_env={'LC_CTYPE': 'C'},
+            data=json.dumps(cmd).encode("utf-8"),
+            decode=False)
+        self.assertEqual(self.utf8_valid, out)
+
+    def test_bogus_command_logs_status_messages(self):
+        """status_cb gets status messages logs on bogus commands provided."""
+        logs = []
+
+        def status_cb(log):
+            logs.append(log)
+
+        with self.assertRaises(subp.ProcessExecutionError):
+            subp.subp([BOGUS_COMMAND], status_cb=status_cb)
+
+        expected = [
+            'Begin run command: {cmd}\n'.format(cmd=BOGUS_COMMAND),
+            'ERROR: End run command: invalid command provided\n']
+        self.assertEqual(expected, logs)
+
+    def test_command_logs_exit_codes_to_status_cb(self):
+        """status_cb gets status messages containing command exit code."""
+        logs = []
+
+        def status_cb(log):
+            logs.append(log)
+
+        with self.assertRaises(subp.ProcessExecutionError):
+            subp.subp([BASH, '-c', 'exit 2'], status_cb=status_cb)
+        subp.subp([BASH, '-c', 'exit 0'], status_cb=status_cb)
+
+        expected = [
+            'Begin run command: %s -c exit 2\n' % BASH,
+            'ERROR: End run command: exit(2)\n',
+            'Begin run command: %s -c exit 0\n' % BASH,
+            'End run command: exit(0)\n']
+        self.assertEqual(expected, logs)
+
+
 # vi: ts=4 expandtab
diff --git a/cloudinit/tests/test_util.py b/cloudinit/tests/test_util.py
index bfccfe1e..0d9a8fd9 100644
--- a/cloudinit/tests/test_util.py
+++ b/cloudinit/tests/test_util.py
@@ -9,6 +9,7 @@ import platform
 import pytest
 
 import cloudinit.util as util
+from cloudinit import subp
 
 from cloudinit.tests.helpers import CiTestCase, mock
 from textwrap import dedent
@@ -332,7 +333,7 @@ class TestBlkid(CiTestCase):
                           "PARTUUID": self.ids["id09"]},
         })
 
-    @mock.patch("cloudinit.util.subp")
+    @mock.patch("cloudinit.subp.subp")
     def test_functional_blkid(self, m_subp):
         m_subp.return_value = (
             self.blkid_out.format(**self.ids), "")
@@ -340,7 +341,7 @@ class TestBlkid(CiTestCase):
         m_subp.assert_called_with(["blkid", "-o", "full"], capture=True,
                                   decode="replace")
 
-    @mock.patch("cloudinit.util.subp")
+    @mock.patch("cloudinit.subp.subp")
     def test_blkid_no_cache_uses_no_cache(self, m_subp):
         """blkid should turn off cache if disable_cache is true."""
         m_subp.return_value = (
@@ -351,7 +352,7 @@ class TestBlkid(CiTestCase):
                                   capture=True, decode="replace")
 
 
-@mock.patch('cloudinit.util.subp')
+@mock.patch('cloudinit.subp.subp')
 class TestUdevadmSettle(CiTestCase):
     def test_with_no_params(self, m_subp):
         """called with no parameters."""
@@ -396,8 +397,8 @@ class TestUdevadmSettle(CiTestCase):
              '--timeout=%s' % timeout])
 
     def test_subp_exception_raises_to_caller(self, m_subp):
-        m_subp.side_effect = util.ProcessExecutionError("BOOM")
-        self.assertRaises(util.ProcessExecutionError, util.udevadm_settle)
+        m_subp.side_effect = subp.ProcessExecutionError("BOOM")
+        self.assertRaises(subp.ProcessExecutionError, util.udevadm_settle)
 
 
 @mock.patch('os.path.exists')