openstack/rally
Code Issues Proposed changes

[Plugins] Fix vm.utils._run_command_over_ssh

Browse Source 
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
This commit is contained in:
Alexander Maretskiy 2016-06-03 11:56:59 +03:00
parent 0da699c1a3
commit 567b9d975b
4 changed files with 79 additions and 73 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

53
rally/plugins/openstack/scenarios/vm/utils.py
View File

@ -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)

21
rally/task/validation.py
View File

@ -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:

8
tests/unit/plugins/openstack/scenarios/vm/test_utils.py
View File

@ -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()
mock_popen.return_value.wait.assert_called_once_with()

70
tests/unit/task/test_validation.py
View File

@ -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")