From 090b1a166bd19bdb98b0311d58b85582bd1676ed Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Wed, 8 Nov 2017 12:41:36 +0000 Subject: Replace subprocess "check_output" with "Popen" "check_output" is a blocking wrapper for "Popen" which returns the output of the command execution or raises an exception in case of error. "Popen" is a non-blocking function that allows to create asynchronous tasks. It returns any possible execution error but doesn't raise an exception; this is delegated to the developer. This code is used in the Yardstick CLI base test class. Change-Id: Ie3e1228b2d40cb306354447653678bf581bc9697 Signed-off-by: Rodolfo Alonso Hernandez --- tests/functional/utils.py | 44 +++++----------- tests/unit/common/test_process.py | 106 +++++++++++++++++++++++++++++++++++++- tests/unit/common/test_utils.py | 33 ++++++++---- yardstick/common/exceptions.py | 19 +++++++ yardstick/common/process.py | 91 ++++++++++++++++++++++++++++++++ yardstick/common/utils.py | 20 +++++-- 6 files changed, 267 insertions(+), 46 deletions(-) create mode 100644 yardstick/common/exceptions.py 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): -- cgit 1.2.3-korg