Merge "Replace subprocess "check_output" with "Popen""

6 files changed, 267 insertions, 46 deletions

diff --git a/tests/functional/utils.py b/tests/functional/utils.py
index b96d2dd50..d889c0dfa 100755
--- a/tests/functional/utils.py
+++ b/tests/functional/utils.py
@@ -7,14 +7,12 @@
 # http://www.apache.org/licenses/LICENSE-2.0
 ##############################################################################
 
-from __future__ import absolute_import
-
 import copy
 import os
-import subprocess
 
 from oslo_serialization import jsonutils
-from oslo_utils import encodeutils
+
+from yardstick.common import process
 
 
 class Yardstick(object):
@@ -26,38 +24,22 @@ class Yardstick(object):
 
     """
 
-    def __init__(self, fake=False):
-
-        self.args = ["yardstick"]
+    def __init__(self):
+        self._args = ["yardstick"]
         self.env = copy.deepcopy(os.environ)
 
-    def __del__(self):
-        pass
-
-    def __call__(self, cmd, getjson=False, report_path=None, raw=False,
-                 suffix=None, extension=None, keep_old=False,
-                 write_report=False):
+    def __call__(self, cmd, getjson=False):
         """Call yardstick in the shell
 
-        :param cmd: yardstick command
-        :param getjson: in cases, when yardstick prints JSON, you can catch
-         output deserialized
-        TO DO:
-        :param report_path: if present, yardstick command and its output will
-         be written to file with passed file name
-        :param raw: don't write command itself to report file. Only output
-            will be written
+        :param cmd: Yardstick command.
+        :param getjson: If the output is a JSON object, it's deserialized.
+        :return Command output string.
         """
 
         if not isinstance(cmd, list):
             cmd = cmd.split(" ")
-        try:
-            output = encodeutils.safe_decode(subprocess.check_output(
-                self.args + cmd, stderr=subprocess.STDOUT, env=self.env),
-                'utf-8')
-
-            if getjson:
-                return jsonutils.loads(output)
-            return output
-        except subprocess.CalledProcessError as e:
-            raise e
+        cmd = self._args + cmd
+        output = process.execute(cmd=cmd)
+        if getjson:
+            return jsonutils.loads(output)
+        return output
diff --git a/tests/unit/common/test_process.py b/tests/unit/common/test_process.py
index 5eee55bcc..1c6dfec27 100644
--- a/tests/unit/common/test_process.py
+++ b/tests/unit/common/test_process.py
@@ -11,10 +11,13 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
-import unittest
 
 import mock
+import unittest
+
+from oslo_utils import encodeutils
 
+from yardstick.common import exceptions
 from yardstick.common import process
 
 
@@ -44,3 +47,104 @@ class TerminateChildrenTestcase(unittest.TestCase):
     def test_no_children(self, mock_multiprocessing):
         mock_multiprocessing.active_children.return_value = []
         process.terminate_children()
+
+
+class ExecuteTestCase(unittest.TestCase):
+
+    RET_CODE_OK = 0
+    RET_CODE_WRONG = 1
+
+    def setUp(self):
+        self._mock_create_process = mock.patch.object(process,
+                                                      'create_process')
+        self.mock_create_process = self._mock_create_process.start()
+        self.obj = mock.Mock()
+        self.cmd = mock.Mock()
+        self.obj.communicate = mock.Mock()
+        self.stdout = 'std out'
+        self.stderr = 'std err'
+        self.obj.communicate.return_value = (self.stdout, self.stderr)
+        self.mock_create_process.return_value = (self.obj, self.cmd)
+        self.input_cmd = 'input cmd'
+        self.additional_env = mock.Mock()
+
+    def test_execute_with_input(self):
+        process_input = 'process input'
+        self.obj.returncode = self.RET_CODE_OK
+        out = process.execute(self.input_cmd, process_input=process_input,
+                              additional_env=self.additional_env)
+        self.obj.communicate.assert_called_once_with(
+            encodeutils.to_utf8(process_input))
+        self.mock_create_process.assert_called_once_with(
+            self.input_cmd, run_as_root=False,
+            additional_env=self.additional_env)
+        self.assertEqual(self.stdout, out)
+
+    def test_execute_no_input(self):
+        self.obj.returncode = self.RET_CODE_OK
+        out = process.execute(self.input_cmd,
+                              additional_env=self.additional_env)
+        self.obj.communicate.assert_called_once_with(None)
+        self.mock_create_process.assert_called_once_with(
+            self.input_cmd, run_as_root=False,
+            additional_env=self.additional_env)
+        self.assertEqual(self.stdout, out)
+
+    def test_execute_exception(self):
+        self.obj.returncode = self.RET_CODE_WRONG
+        self.assertRaises(exceptions.ProcessExecutionError, process.execute,
+                          self.input_cmd, additional_env=self.additional_env)
+        self.obj.communicate.assert_called_once_with(None)
+
+    def test_execute_with_extra_code(self):
+        self.obj.returncode = self.RET_CODE_WRONG
+        out = process.execute(self.input_cmd,
+                              additional_env=self.additional_env,
+                              extra_ok_codes=[self.RET_CODE_WRONG])
+        self.obj.communicate.assert_called_once_with(None)
+        self.mock_create_process.assert_called_once_with(
+            self.input_cmd, run_as_root=False,
+            additional_env=self.additional_env)
+        self.assertEqual(self.stdout, out)
+
+    def test_execute_exception_no_check(self):
+        self.obj.returncode = self.RET_CODE_WRONG
+        out = process.execute(self.input_cmd,
+                              additional_env=self.additional_env,
+                              check_exit_code=False)
+        self.obj.communicate.assert_called_once_with(None)
+        self.mock_create_process.assert_called_once_with(
+            self.input_cmd, run_as_root=False,
+            additional_env=self.additional_env)
+        self.assertEqual(self.stdout, out)
+
+
+class CreateProcessTestCase(unittest.TestCase):
+
+    @mock.patch.object(process, 'subprocess_popen')
+    def test_process_string_command(self, mock_subprocess_popen):
+        cmd = 'command'
+        obj = mock.Mock()
+        mock_subprocess_popen.return_value = obj
+        out1, out2 = process.create_process(cmd)
+        self.assertEqual(obj, out1)
+        self.assertEqual([cmd], out2)
+
+    @mock.patch.object(process, 'subprocess_popen')
+    def test_process_list_command(self, mock_subprocess_popen):
+        cmd = ['command']
+        obj = mock.Mock()
+        mock_subprocess_popen.return_value = obj
+        out1, out2 = process.create_process(cmd)
+        self.assertEqual(obj, out1)
+        self.assertEqual(cmd, out2)
+
+    @mock.patch.object(process, 'subprocess_popen')
+    def test_process_with_env(self, mock_subprocess_popen):
+        cmd = ['command']
+        obj = mock.Mock()
+        additional_env = {'var1': 'value1'}
+        mock_subprocess_popen.return_value = obj
+        out1, out2 = process.create_process(cmd, additional_env=additional_env)
+        self.assertEqual(obj, out1)
+        self.assertEqual(['env', 'var1=value1'] + cmd, out2)
diff --git a/tests/unit/common/test_utils.py b/tests/unit/common/test_utils.py
index 42b75d1f0..452b93a56 100644
--- a/tests/unit/common/test_utils.py
+++ b/tests/unit/common/test_utils.py
@@ -11,15 +11,15 @@
 
 from __future__ import absolute_import
 
-import ipaddress
-import os
-import unittest
 from copy import deepcopy
-from itertools import product, chain
-
 import errno
+import ipaddress
+from itertools import product, chain
 import mock
+import os
+import six
 from six.moves import configparser
+import unittest
 
 import yardstick
 from yardstick.common import utils
@@ -775,7 +775,8 @@ class RemoveFileTestCase(unittest.TestCase):
     def test_remove_file(self):
         try:
             utils.remove_file('notexistfile.txt')
-        except Exception as e:
+        except Exception as e:  # pylint: disable=broad-except
+            # NOTE(ralonsoh): to narrow the scope of this exception.
             self.assertTrue(isinstance(e, OSError))
 
 
@@ -997,7 +998,8 @@ class TestUtilsIpAddrMethods(unittest.TestCase):
             self.assertEqual(utils.safe_ip_address(addr), expected, addr)
 
     @mock.patch("yardstick.common.utils.logging")
-    def test_safe_ip_address_negative(self, mock_logging):
+    def test_safe_ip_address_negative(self, *args):
+        # NOTE(ralonsoh): check the calls to mocked functions.
         for value in self.INVALID_IP_ADDRESS_STR_LIST:
             self.assertIsNone(utils.safe_ip_address(value), value)
 
@@ -1026,7 +1028,8 @@ class TestUtilsIpAddrMethods(unittest.TestCase):
             self.assertEqual(utils.get_ip_version(addr), 6, addr)
 
     @mock.patch("yardstick.common.utils.logging")
-    def test_get_ip_version_negative(self, mock_logging):
+    def test_get_ip_version_negative(self, *args):
+        # NOTE(ralonsoh): check the calls to mocked functions.
         for value in self.INVALID_IP_ADDRESS_STR_LIST:
             self.assertIsNone(utils.get_ip_version(value), value)
 
@@ -1055,7 +1058,8 @@ class TestUtilsIpAddrMethods(unittest.TestCase):
             self.assertEqual(utils.ip_to_hex(value), value)
 
     @mock.patch("yardstick.common.utils.logging")
-    def test_ip_to_hex_negative(self, mock_logging):
+    def test_ip_to_hex_negative(self, *args):
+        # NOTE(ralonsoh): check the calls to mocked functions.
         addr_list = self.GOOD_IP_V4_ADDRESS_STR_LIST
         mask_list = self.GOOD_IP_V4_MASK_STR_LIST
         value_iter = (''.join(pair) for pair in product(addr_list, mask_list))
@@ -1063,6 +1067,17 @@ class TestUtilsIpAddrMethods(unittest.TestCase):
             self.assertEqual(utils.ip_to_hex(value), value)
 
 
+class SafeDecodeUtf8TestCase(unittest.TestCase):
+
+    @unittest.skipIf(six.PY2,
+                     'This test should only be launched with Python 3.x')
+    def test_safe_decode_utf8(self):
+        _bytes = b'this is a byte array'
+        out = utils.safe_decode_utf8(_bytes)
+        self.assertIs(type(out), str)
+        self.assertEqual('this is a byte array', out)
+
+
 def main():
     unittest.main()
 
diff --git a/yardstick/common/exceptions.py b/yardstick/common/exceptions.py
new file mode 100644
index 000000000..9c0ec2c19
--- /dev/null
+++ b/yardstick/common/exceptions.py
@@ -0,0 +1,19 @@
+# Copyright (c) 2017 Intel Corporation
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+
+class ProcessExecutionError(RuntimeError):
+    def __init__(self, message, returncode):
+        super(ProcessExecutionError, self).__init__(message)
+        self.returncode = returncode
diff --git a/yardstick/common/process.py b/yardstick/common/process.py
index 812ddea94..ede6cddac 100644
--- a/yardstick/common/process.py
+++ b/yardstick/common/process.py
@@ -11,10 +11,19 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
+
 import logging
 import multiprocessing
+import signal
+import subprocess
+import time
 
 import os
+from oslo_utils import encodeutils
+
+from yardstick.common import exceptions
+from yardstick.common import utils
+
 
 LOG = logging.getLogger(__name__)
 
@@ -45,3 +54,85 @@ def terminate_children(timeout=3):
     for child in active_children:
         LOG.debug("%s %s %s, after terminate child: %s %s", current_proccess.name,
                   current_proccess.pid, os.getpid(), child, child.pid)
+
+
+def _additional_env_args(additional_env):
+    """Build arguments for adding additional environment vars with env"""
+    if additional_env is None:
+        return []
+    return ['env'] + ['%s=%s' % pair for pair in additional_env.items()]
+
+
+def _subprocess_setup():
+    # Python installs a SIGPIPE handler by default. This is usually not what
+    # non-Python subprocesses expect.
+    signal.signal(signal.SIGPIPE, signal.SIG_DFL)
+
+
+def subprocess_popen(args, stdin=None, stdout=None, stderr=None, shell=False,
+                     env=None, preexec_fn=_subprocess_setup, close_fds=True):
+    return subprocess.Popen(args, shell=shell, stdin=stdin, stdout=stdout,
+                            stderr=stderr, preexec_fn=preexec_fn,
+                            close_fds=close_fds, env=env)
+
+
+def create_process(cmd, run_as_root=False, additional_env=None):
+    """Create a process object for the given command.
+
+    The return value will be a tuple of the process object and the
+    list of command arguments used to create it.
+    """
+    if not isinstance(cmd, list):
+        cmd = [cmd]
+    cmd = list(map(str, _additional_env_args(additional_env) + cmd))
+    if run_as_root:
+        # NOTE(ralonsoh): to handle a command executed as root, using
+        # a root wrapper, instead of using "sudo".
+        pass
+    LOG.debug("Running command: %s", cmd)
+    obj = subprocess_popen(cmd, shell=False, stdin=subprocess.PIPE,
+                           stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    return obj, cmd
+
+
+def execute(cmd, process_input=None, additional_env=None,
+            check_exit_code=True, return_stderr=False, log_fail_as_error=True,
+            extra_ok_codes=None, run_as_root=False):
+    try:
+        if process_input is not None:
+            _process_input = encodeutils.to_utf8(process_input)
+        else:
+            _process_input = None
+
+        # NOTE(ralonsoh): to handle the execution of a command as root,
+        # using a root wrapper, instead of using "sudo".
+        obj, cmd = create_process(cmd, run_as_root=run_as_root,
+                                  additional_env=additional_env)
+        _stdout, _stderr = obj.communicate(_process_input)
+        returncode = obj.returncode
+        obj.stdin.close()
+        _stdout = utils.safe_decode_utf8(_stdout)
+        _stderr = utils.safe_decode_utf8(_stderr)
+
+        extra_ok_codes = extra_ok_codes or []
+        if returncode and returncode not in extra_ok_codes:
+            msg = ("Exit code: %(returncode)d; "
+                   "Stdin: %(stdin)s; "
+                   "Stdout: %(stdout)s; "
+                   "Stderr: %(stderr)s") % {'returncode': returncode,
+                                            'stdin': process_input or '',
+                                            'stdout': _stdout,
+                                            'stderr': _stderr}
+            if log_fail_as_error:
+                LOG.error(msg)
+            if check_exit_code:
+                raise exceptions.ProcessExecutionError(msg,
+                                                       returncode=returncode)
+
+    finally:
+        # This appears to be necessary in order for the subprocess to clean up
+        # something between call; without it, the second process hangs when two
+        # execute calls are made in a row.
+        time.sleep(0)
+
+    return (_stdout, _stderr) if return_stderr else _stdout
diff --git a/yardstick/common/utils.py b/yardstick/common/utils.py
index 51f6e1360..82e20bec7 100644
--- a/yardstick/common/utils.py
+++ b/yardstick/common/utils.py
@@ -76,7 +76,7 @@ def import_modules_from_package(package):
     """
     yardstick_root = os.path.dirname(os.path.dirname(yardstick.__file__))
     path = os.path.join(yardstick_root, *package.split("."))
-    for root, dirs, files in os.walk(path):
+    for root, _, files in os.walk(path):
         matches = (filename for filename in files if filename.endswith(".py") and
                    not filename.startswith("__"))
         new_package = os.path.relpath(root, yardstick_root).replace(os.sep, ".")
@@ -251,10 +251,10 @@ def set_dict_value(dic, keys, value):
 
 def get_free_port(ip):
     with closing(socket.socket(socket.AF_INET, socket.SOCK_STREAM)) as s:
-        while True:
+        port = random.randint(5000, 10000)
+        while s.connect_ex((ip, port)) == 0:
             port = random.randint(5000, 10000)
-            if s.connect_ex((ip, port)) != 0:
-                return port
+        return port
 
 
 def mac_address_to_hex_list(mac):
@@ -350,10 +350,13 @@ def config_to_dict(config):
 
 
 def validate_non_string_sequence(value, default=None, raise_exc=None):
+    # NOTE(ralonsoh): refactor this function to check if raise_exc is an
+    # Exception. Remove duplicate code, this function is duplicated in this
+    # repository.
     if isinstance(value, collections.Sequence) and not isinstance(value, six.string_types):
         return value
     if raise_exc:
-        raise raise_exc
+        raise raise_exc  # pylint: disable=raising-bad-type
     return default
 
 
@@ -365,6 +368,13 @@ def join_non_strings(separator, *non_strings):
     return str(separator).join(str(non_string) for non_string in non_strings)
 
 
+def safe_decode_utf8(s):
+    """Safe decode a str from UTF"""
+    if six.PY3 and isinstance(s, bytes):
+        return s.decode('utf-8', 'surrogateescape')
+    return s
+
+
 class ErrorClass(object):
 
     def __init__(self, *args, **kwargs):