From 567b9d975b6a2e97ed67da59d78e4f1b25687e7d Mon Sep 17 00:00:00 2001 From: Alexander Maretskiy Date: Fri, 3 Jun 2016 11:56:59 +0300 Subject: [PATCH] [Plugins] Fix vm.utils._run_command_over_ssh Method vm.utils._run_command_over_ssh has been reworked, also there are related changes in task.validation: * command parameters are processed in different way, so UnboundLocalError and another potential issues are resolved * removed redundant check `validation.check_command_dict', because scenarios which uses _run_command_over_ssh already check command with validator `validation.valid_command', so `check_command_dict' is run twice * as result, unit test `test__run_command_over_ssh_fails' is removed since it checks invocation of `check_command_dict' by `_run_command_over_ssh' * os.path.expanduser is moved from check_command_dict to _run_command_over_ssh since validation should not modify data, especially when it is being done implicitly * unit test `validation.check_command_dict' is extended and rewritten with DDT Change-Id: Ib14012bf6e91fd6f7b4dd32766daa191c320e314 Closes-Bug: 1587941 --- rally/plugins/openstack/scenarios/vm/utils.py | 53 +++++++------- rally/task/validation.py | 21 ++++-- .../openstack/scenarios/vm/test_utils.py | 8 +-- tests/unit/task/test_validation.py | 70 +++++++++---------- 4 files changed, 79 insertions(+), 73 deletions(-) diff --git a/rally/plugins/openstack/scenarios/vm/utils.py b/rally/plugins/openstack/scenarios/vm/utils.py index eeea1c67c9..e936297275 100644 --- a/rally/plugins/openstack/scenarios/vm/utils.py +++ b/rally/plugins/openstack/scenarios/vm/utils.py @@ -13,6 +13,7 @@ # License for the specific language governing permissions and limitations # under the License. +import os.path import subprocess import sys @@ -28,7 +29,6 @@ from rally.plugins.openstack.scenarios.nova import utils as nova_utils from rally.plugins.openstack.wrappers import network as network_wrapper from rally.task import atomic from rally.task import utils -from rally.task import validation LOG = logging.getLogger(__name__) @@ -113,33 +113,36 @@ class VMScenario(nova_utils.NovaScenario, cinder_utils.CinderScenario): :returns: tuple (exit_status, stdout, stderr) """ - validation.check_command_dict(command) + cmd, stdin = [], None - # NOTE(pboldin): Here we `get' the values and not check for the keys - # due to template-driven configuration generation that can leave keys - # defined but values empty. - if command.get("script_file") or command.get("script_inline"): - cmd = command["interpreter"] - if command.get("script_file"): - stdin = open(command["script_file"], "rb") - elif command.get("script_inline"): - stdin = six.moves.StringIO(command["script_inline"]) - elif command.get("remote_path"): - cmd = command["remote_path"] - stdin = None + interpreter = command.get("interpreter") or [] + if interpreter: + if isinstance(interpreter, six.string_types): + interpreter = [interpreter] + elif type(interpreter) != list: + raise ValueError("command 'interpreter' value must be str " + "or list type") + cmd.extend(interpreter) - if command.get("local_path"): - remote_path = cmd[-1] if isinstance(cmd, (tuple, list)) else cmd - ssh.put_file(command["local_path"], remote_path, - mode=self.USER_RWX_OTHERS_RX_ACCESS_MODE) + remote_path = command.get("remote_path") or [] + if remote_path: + if isinstance(remote_path, six.string_types): + remote_path = [remote_path] + elif type(remote_path) != list: + raise ValueError("command 'remote_path' value must be str " + "or list type") + cmd.extend(remote_path) + if command.get("local_path"): + ssh.put_file(command["local_path"], remote_path[-1], + mode=self.USER_RWX_OTHERS_RX_ACCESS_MODE) - if command.get("command_args"): - if not isinstance(cmd, (list, tuple)): - cmd = [cmd] - # NOTE(pboldin): `ssh.execute' accepts either a string interpreted - # as a command name or the list of strings that are converted into - # single-line command with arguments. - cmd = cmd + list(command["command_args"]) + if command.get("script_file"): + stdin = open(os.path.expanduser(command["script_file"]), "rb") + + elif command.get("script_inline"): + stdin = six.moves.StringIO(command["script_inline"]) + + cmd.extend(command.get("command_args") or []) return ssh.execute(cmd, stdin=stdin) diff --git a/rally/task/validation.py b/rally/task/validation.py index 2db5fc65df..dfbf829551 100755 --- a/rally/task/validation.py +++ b/rally/task/validation.py @@ -171,13 +171,15 @@ def file_exists(config, clients, deployment, param_name, mode=os.R_OK, def check_command_dict(command): """Check command-specifying dict `command', raise ValueError on error.""" + if type(command) != dict: + raise ValueError("Command must be a dictionary") + # NOTE(pboldin): Here we check for the values not for presence of the keys # due to template-driven configuration generation that can leave keys # defined but values empty. if command.get("interpreter"): script_file = command.get("script_file") if script_file: - command["script_file"] = os.path.expanduser(script_file) if "script_inline" in command: raise ValueError( "Exactly one of script_inline or script_file with " @@ -198,6 +200,13 @@ def check_command_dict(command): "Supplied dict specifies no command to execute," " either interpreter or remote_path is required: %r" % command) + unexpected_keys = set(command) - set(["script_file", "script_inline", + "interpreter", "remote_path", + "local_path", "command_args"]) + if unexpected_keys: + raise ValueError( + "Unexpected command parameters: %s" % ", ".join(unexpected_keys)) + @validator def valid_command(config, clients, deployment, param_name, required=True): @@ -209,13 +218,13 @@ def valid_command(config, clients, deployment, param_name, required=True): :param param_name: Name of parameter to validate :param required: Boolean indicating that the command dictionary is required """ - # TODO(pboldin): Make that a `jsonschema' check once generic validator - # is available. + # TODO(amaretskiy): rework this validator into ResourceType, so this + # will allow to validate parameters values as well command = config.get("args", {}).get(param_name) - if command is None: - return ValidationResult(not required, - "Command dictionary is required") + if command is None and not required: + return ValidationResult(True) + try: check_command_dict(command) except ValueError as e: diff --git a/tests/unit/plugins/openstack/scenarios/vm/test_utils.py b/tests/unit/plugins/openstack/scenarios/vm/test_utils.py index 092680cb23..c14abbc4b2 100644 --- a/tests/unit/plugins/openstack/scenarios/vm/test_utils.py +++ b/tests/unit/plugins/openstack/scenarios/vm/test_utils.py @@ -96,12 +96,6 @@ class VMScenarioTestCase(test.ScenarioTestCase): ["foo", "bar", "arg1", "arg2"], stdin=None) - def test__run_command_over_ssh_fails(self): - vm_scenario = utils.VMScenario(self.context) - self.assertRaises(ValueError, - vm_scenario._run_command_over_ssh, - None, command={}) - def test__wait_for_ssh(self): ssh = mock.MagicMock() vm_scenario = utils.VMScenario(self.context) @@ -309,4 +303,4 @@ class HostTestCase(test.TestCase): mock_popen.assert_called_once_with( ["ping6", "-c1", str(host.ip)], stderr=subprocess.PIPE, stdout=subprocess.PIPE) - mock_popen.return_value.wait.assert_called_once_with() \ No newline at end of file + mock_popen.return_value.wait.assert_called_once_with() diff --git a/tests/unit/task/test_validation.py b/tests/unit/task/test_validation.py index bf1b6fa9a9..572f7cad6c 100755 --- a/tests/unit/task/test_validation.py +++ b/tests/unit/task/test_validation.py @@ -122,41 +122,35 @@ class ValidatorsTestCase(test.TestCase): mock__file_access_ok.assert_called_once_with( "test_file", os.R_OK, "p", False) - def test_check_command_valid(self): - - e = self.assertRaises( - ValueError, validation.check_command_dict, - { - "interpreter": "foobar", "script_file": "foo", - "script_inline": "bar" - }) - self.assertIn("Exactly one of ", str(e)) - - e = self.assertRaises( - ValueError, validation.check_command_dict, - {"script_file": "foobar"}) - self.assertIn("Supplied dict specifies no", str(e)) - - command = {"script_inline": "foobar", "interpreter": "foo"} - result = validation.check_command_dict(command) - self.assertIsNone(result) - - e = self.assertRaises( - ValueError, validation.check_command_dict, - { - "script_inline": "foobar", - "interpreter": "foo", - "local_path": "bar" - }) - self.assertIn("When uploading an interpreter its path", str(e)) - - result = validation.check_command_dict({ - "script_inline": "foobar", - "interpreter": ["ENV=bar", "/bin/foo"], - "local_path": "bar", - "remote_path": "/bin/foo" - }) - self.assertIsNone(result) + @ddt.data({"raises_message": "Command must be a dictionary"}, + {"command": "foo", + "raises_message": "Command must be a dictionary"}, + {"command": {"interpreter": "foobar", "script_file": "foo", + "script_inline": "bar"}, + "raises_message": "Exactly one of "}, + {"command": {"script_file": "foobar"}, + "raises_message": "Supplied dict specifies no"}, + {"command": {"script_inline": "foobar", + "interpreter": "foo", + "local_path": "bar"}, + "raises_message": "When uploading an interpreter its path"}, + {"command": {"interpreter": "/bin/bash", + "script_path": "foo"}, + "raises_message": ("Unexpected command parameters: " + "script_path")}, + {"command": {"script_inline": "foobar", + "interpreter": ["ENV=bar", "/bin/foo"], + "local_path": "bar", + "remote_path": "/bin/foo"}}, + {"command": {"script_inline": "foobar", "interpreter": "foo"}}) + @ddt.unpack + def test_check_command_dict(self, command=None, raises_message=None): + if raises_message: + e = self.assertRaises( + ValueError, validation.check_command_dict, command) + self.assertIn(raises_message, str(e)) + else: + self.assertIsNone(validation.check_command_dict(command)) @mock.patch("rally.task.validation._file_access_ok") def test_valid_command(self, mock__file_access_ok): @@ -171,6 +165,12 @@ class ValidatorsTestCase(test.TestCase): filename="foobar", mode=os.R_OK, param_name="p.script_file", required=True) + def test_valid_command_not_required(self): + validator = self._unwrap_validator(validation.valid_command, + param_name="p", required=False) + result = validator({"args": {"p": None}}, None, None) + self.assertTrue(result.is_valid) + def test_valid_command_required(self): validator = self._unwrap_validator(validation.valid_command, param_name="p")