From 739a4e3c55ef7572076d9598a07d7e2ab49c630b Mon Sep 17 00:00:00 2001 From: Federico Ressi Date: Fri, 4 Jun 2021 11:39:29 +0200 Subject: [PATCH] Fix default shell for command execution Change-Id: Id783ef736b82615458d918de46d0e2e5ab82f706 --- tobiko/shell/files/_logs.py | 5 +-- tobiko/shell/sh/_local.py | 35 ++++++++++--------- tobiko/shell/sh/_process.py | 27 +++++++------- tobiko/tests/functional/shell/test_execute.py | 35 ++++++++++++++----- 4 files changed, 62 insertions(+), 40 deletions(-) diff --git a/tobiko/shell/files/_logs.py b/tobiko/shell/files/_logs.py index 6ae6c88a5..cd0b28807 100644 --- a/tobiko/shell/files/_logs.py +++ b/tobiko/shell/files/_logs.py @@ -17,6 +17,7 @@ from __future__ import absolute_import import collections import os +import shlex import typing import tobiko @@ -93,10 +94,10 @@ class JournalLogDigger(LogFileDigger): def grep_lines(self, pattern) -> typing.List[str]: try: result = sh.execute(["journalctl", '--no-pager', - "--unit", self.filename, + "--unit", shlex.quote(self.filename), "--since", "30 minutes ago", '--output', 'short-iso', - '--grep', "'%s'" % pattern], + '--grep', shlex.quote(pattern)], **self.execute_params) except sh.ShellCommandFailed as ex: if ex.stdout.endswith('-- No entries --\n'): diff --git a/tobiko/shell/sh/_local.py b/tobiko/shell/sh/_local.py index 0ef681c04..4799697f0 100644 --- a/tobiko/shell/sh/_local.py +++ b/tobiko/shell/sh/_local.py @@ -15,7 +15,6 @@ # under the License. from __future__ import absolute_import -import fcntl import os import subprocess import sys @@ -24,6 +23,7 @@ from oslo_log import log import tobiko from tobiko.shell.sh import _io +from tobiko.shell.sh import _exception from tobiko.shell.sh import _execute from tobiko.shell.sh import _path from tobiko.shell.sh import _process @@ -68,6 +68,7 @@ class LocalExecutePathFixture(_path.ExecutePathFixture): class LocalShellProcessFixture(_process.ShellProcessFixture): path_execute = tobiko.required_fixture(LocalExecutePathFixture) + default_shell = True def create_process(self): tobiko.setup_fixture(self.path_execute) @@ -77,15 +78,23 @@ class LocalShellProcessFixture(_process.ShellProcessFixture): stdout = parameters.stdout and subprocess.PIPE or None stderr = parameters.stderr and subprocess.PIPE or None env = merge_dictionaries(os.environ, parameters.environment) - return subprocess.Popen(args=args, - bufsize=parameters.buffer_size, - shell=False, - universal_newlines=False, - env=env, - cwd=parameters.current_dir, - stdin=stdin, - stdout=stdout, - stderr=stderr) + try: + return subprocess.Popen(args=args, + bufsize=parameters.buffer_size, + shell=False, + universal_newlines=False, + env=env, + cwd=parameters.current_dir, + stdin=stdin, + stdout=stdout, + stderr=stderr) + except FileNotFoundError as ex: + LOG.debug(f"Error executing local command: args={args}") + raise _exception.ShellCommandFailed(command=self.command, + exit_status=-1, + stdin=parameters.stdin, + stdout=None, + stderr=str(ex)) from ex def setup_stdin(self): self.stdin = _io.ShellStdin(delegate=self.process.stdin, @@ -96,7 +105,6 @@ class LocalShellProcessFixture(_process.ShellProcessFixture): buffer_size=self.parameters.buffer_size) def setup_stderr(self): - set_non_blocking(self.process.stderr.fileno()) self.stderr = _io.ShellStderr(delegate=self.process.stderr, buffer_size=self.parameters.buffer_size) @@ -131,11 +139,6 @@ class LocalShellProcessFixture(_process.ShellProcessFixture): return process and process.pid or None -def set_non_blocking(fd): - flag = fcntl.fcntl(fd, fcntl.F_GETFL) - fcntl.fcntl(fd, fcntl.F_SETFL, flag | os.O_NONBLOCK) - - def merge_dictionaries(*dictionaries): merged = {} for d in dictionaries: diff --git a/tobiko/shell/sh/_process.py b/tobiko/shell/sh/_process.py index 9e8148e63..e82d46046 100644 --- a/tobiko/shell/sh/_process.py +++ b/tobiko/shell/sh/_process.py @@ -72,17 +72,17 @@ class ShellProcessParameters(Parameters): environment = None current_dir = None timeout: tobiko.Seconds = None - shell = None stdin = False stdout = True stderr = True buffer_size = io.DEFAULT_BUFFER_SIZE poll_interval = 1. - sudo = None network_namespace = None retry_count: typing.Optional[int] = 3 retry_interval: tobiko.Seconds = 5. retry_timeout: tobiko.Seconds = 120. + shell: typing.Union[None, bool, str] = None + sudo: typing.Union[None, bool, str] = None class ShellProcessFixture(tobiko.SharedFixture): @@ -92,6 +92,7 @@ class ShellProcessFixture(tobiko.SharedFixture): stdin = None stdout = None stderr = None + default_shell: typing.Union[None, bool, str] = None _exit_status = None @@ -122,16 +123,16 @@ class ShellProcessFixture(tobiko.SharedFixture): command = _command.shell_command(self.parameters.command) network_namespace = self.parameters.network_namespace sudo = self.parameters.sudo - shell = self.parameters.shell - if shell: - if shell is True: - shell = default_shell_command() - else: - shell = _command.shell_command(shell) - command = shell + [str(command)] - else: - command = _command.shell_command(command) + if shell is None: + shell = self.default_shell + + if shell is not None: + tobiko.check_valid_type(shell, (bool, str)) + if isinstance(shell, str): + command = _command.shell_command(shell) + [str(command)] + elif shell is True: + command = default_shell_command() + [str(command)] if network_namespace: if sudo is None: @@ -333,7 +334,7 @@ class ShellProcessFixture(tobiko.SharedFixture): poll_interval = self.parameters.poll_interval LOG.debug(f"Waiting for process data {poll_interval} " f"seconds... \n" - f" command: '{self.command}'\n" + f" command: {self.command}\n" f" attempt: {attempt.details}\n" f" streams: {streams}") @@ -432,7 +433,7 @@ def str_from_stream(stream): def default_shell_command(): from tobiko import config CONF = config.CONF - return _command.shell_command(CONF.tobiko.shell.sudo.command) + return _command.shell_command(CONF.tobiko.shell.command) def default_sudo_command(): diff --git a/tobiko/tests/functional/shell/test_execute.py b/tobiko/tests/functional/shell/test_execute.py index 7faf1fbe6..370690347 100644 --- a/tobiko/tests/functional/shell/test_execute.py +++ b/tobiko/tests/functional/shell/test_execute.py @@ -15,6 +15,8 @@ # under the License. from __future__ import absolute_import +import typing + import testtools import tobiko @@ -22,15 +24,23 @@ from tobiko import config from tobiko.openstack import keystone from tobiko.openstack import stacks from tobiko.shell import sh +from tobiko.shell import ssh CONF = config.CONF +SSH_EXPECTED_SHELL = None +LOCAL_EXPECTED_SHELL = '/bin/sh -c' + class ExecuteTest(testtools.TestCase): - ssh_client = None - shell = '/bin/sh -c' + @property + def expected_shell(self) -> typing.Optional[str]: + if ssh.ssh_proxy_client() is None: + return LOCAL_EXPECTED_SHELL + else: + return SSH_EXPECTED_SHELL def test_succeed(self, command='true', stdin=None, stdout=None, stderr=None, expect_exit_status=0, **kwargs): @@ -153,34 +163,41 @@ class ExecuteTest(testtools.TestCase): self.assertEqual(timeout, ex.timeout) def execute(self, **kwargs): - kwargs.setdefault('shell', self.shell) - kwargs.setdefault('ssh_client', self.ssh_client) return sh.execute(**kwargs) def expected_command(self, command): command = sh.shell_command(command) - if self.shell: - command = sh.shell_command(self.shell) + [str(command)] + if self.expected_shell is not None: + command = sh.shell_command(self.expected_shell) + [str(command)] return str(command) class LocalExecuteTest(ExecuteTest): + expected_shell = LOCAL_EXPECTED_SHELL + def execute(self, **kwargs): - kwargs.setdefault('shell', self.shell) return sh.local_execute(**kwargs) @keystone.skip_unless_has_keystone_credentials() class SSHExecuteTest(ExecuteTest): + expected_shell = SSH_EXPECTED_SHELL + server_stack = tobiko.required_setup_fixture( - stacks.CirrosServerStackFixture) + stacks.UbuntuMinimalServerStackFixture) @property def ssh_client(self): return self.server_stack.ssh_client def execute(self, **kwargs): - kwargs.setdefault('shell', self.shell) return sh.ssh_execute(ssh_client=self.ssh_client, **kwargs) + + +@keystone.skip_unless_has_keystone_credentials() +class CirrosSSHExecuteTest(SSHExecuteTest): + + server_stack = tobiko.required_setup_fixture( + stacks.CirrosServerStackFixture)