Differential D983

shell_directive: fix race condition when running commands
ClosedPublic
Actions

Authored by kparal on Aug 22 2016, 2:58 PM.

Details

Reviewers
lbrabec
Group Reviewers
libtaskotron
Commits
rLTRN355c415d4f98: shell_directive: fix race condition when running commands
Summary

Even though some commands exited with non-zero exit code, we sometimes
believed it exited with zero code due to a race condition. This patch
fixes that and also moves the code to a separate function
run_and_print in os_utils.

Test Plan

in order to test this, first reproduce the issue on existing code base. Apply this patch:

diff --git a/testing/functest_shell_directive.py b/testing/functest_shell_directive.py
index a1cc0f6..81a19e9 100644
--- a/testing/functest_shell_directive.py
+++ b/testing/functest_shell_directive.py
@@ -69,7 +69,11 @@ class TestCheckCommand(object):

         assert output == OUTPUT

-    def test_multi_command_error(self):
+    @pytest.fixture(params=xrange(1000))
+    def moore(self, request):
+        return
+
+    def test_multi_command_error(self, moore):
         test_params = ['true',
                        'echo -n %s' % OUTPUT[::-1],
                        'false',

and then run the test suite like this:

py.test -F testing/functest_shell_directive.py -x -k test_multi_command_error

During the execution, open up a new terminal and run yes command in order to simulate some load on your machine (or do something else to cause larger load on the machine). In my case, the race condition is triggered (and the test suite fails) immediately after running yes.

Now apply the patch and try the same with new codebase. It should no longer happen.

Diff Detail

Repository
rLTRN libtaskotron
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kparal retitled this revision from to shell_directive: fix race condition when running commands.Aug 22 2016, 2:58 PM
kparal updated this object.
kparal edited the test plan for this revision. (Show Details)
kparal added reviewers: libtaskotron, lbrabec.
kparal updated this object.Aug 22 2016, 2:59 PM
kparal updated this revision to Diff 2508.Aug 23 2016, 8:11 AM
  • bikeshedding: rename run_and_print to popen_rt
This revision is now accepted and ready to land.Aug 23 2016, 9:14 AM
Closed by commit rLTRN355c415d4f98: shell_directive: fix race condition when running commands (authored by kparal). · Explain WhyAug 23 2016, 9:32 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

Diff 2509

View Options

libtaskotron/directives/shell_directive.py

1# -*- coding: utf-8 -*- 1​# -*- coding: utf-8 -*-
2# Copyright 2009-2016, Red Hat, Inc. 2​# Copyright 2009-2016, Red Hat, Inc.
3# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+> 3​# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>
4# See the LICENSE file for more details on Licensing 4​# See the LICENSE file for more details on Licensing
5 5​
6from __future__ import absolute_import 6​from __future__ import absolute_import
7 7​
8import os 8​import os
9import subprocess 9​import subprocess
10 10​
11from libtaskotron.directives import BaseDirective 11​from libtaskotron.directives import BaseDirective
12from libtaskotron.exceptions import TaskotronDirectiveError 12​from libtaskotron.exceptions import TaskotronDirectiveError
13from libtaskotron.logger import log 13​from libtaskotron.logger import log
14​from libtaskotron import os_utils
14 15​
15DOCUMENTATION = """ 16​DOCUMENTATION = """
16module: shell_directive 17​module: shell_directive
17short_description: execute one ore more shell commands or direct processes 18​short_description: execute one ore more shell commands or direct processes
18 19​
19description: | 20​description: |
20 Execute a command in shell or as a direct process. There are no parameters for this directive, 21​ Execute a command in shell or as a direct process. There are no parameters for this directive,
21 it uses a slightly different format: 22​ it uses a slightly different format:
▲ Show 20 Lines • Show All 68 Lines • ▼ Show 20 Line(s)90 - name: load file to formula variable
90 shell: 91​ shell:
91 - cat some_file 92​ - cat some_file
92 export: loaded_file 93​ export: loaded_file
93""" 94​"""
94 95​
95directive_class = 'ShellDirective' 96​directive_class = 'ShellDirective'
96 97​
97 98​
98def check_output(cmd, **kwargs): ​
99 """Custom check_output that prints output in real-time""" ​
100 ​
101 proc = subprocess.Popen(cmd, **kwargs) ​
102 output = [] ​
103 for line in iter(proc.stdout.readline, ''): ​
104 print line, ​
105 output.append(line) ​
106 ​
107 retcode = proc.poll() ​
108 if retcode: ​
109 raise subprocess.CalledProcessError(retcode, cmd, ''.join(output)) ​
110 ​
111 return ''.join(output) ​
112 ​
113 ​
114class ShellDirective(BaseDirective): 99​class ShellDirective(BaseDirective):
115 def process(self, params, arg_data, ignorereturn=False): 100​ def process(self, params, arg_data, ignorereturn=False):
116 101​
117 if not isinstance(params, list): 102​ if not isinstance(params, list):
118 log.debug('Shell directive received invalid input: %r', params) 103​ log.debug('Shell directive received invalid input: %r', params)
119 raise TaskotronDirectiveError("Shell directive must contain a list of commands") 104​ raise TaskotronDirectiveError("Shell directive must contain a list of commands")
120 105​
121 basedir = os.path.dirname(os.path.abspath(arg_data['task'])) 106​ basedir = os.path.dirname(os.path.abspath(arg_data['task']))
122 output = "" 107​ output = ""
123 108​
124 for cmd in params: 109​ for cmd in params:
125 if isinstance(cmd, dict) and 'ignorereturn' in cmd: 110​ if isinstance(cmd, dict) and 'ignorereturn' in cmd:
126 output = self.process(cmd['ignorereturn'], arg_data, ignorereturn=True) 111​ output = self.process(cmd['ignorereturn'], arg_data, ignorereturn=True)
127 continue 112​ continue
128 113​
129 try: 114​ try:
130 if isinstance(cmd, list) and all([isinstance(c, basestring) for c in cmd]): 115​ if isinstance(cmd, list) and all([isinstance(c, basestring) for c in cmd]):
131 log.info("Executing process: %s", cmd) 116​ log.info("Executing process: %s", cmd)
132 output = check_output(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, 117​ output, _ = os_utils.popen_rt(cmd, cwd=basedir)
133 bufsize=1, close_fds=True, cwd=basedir) ​
134 elif isinstance(cmd, basestring): 118​ elif isinstance(cmd, basestring):
135 log.info("Executing shell: %s", cmd) 119​ log.info("Executing shell: %s", cmd)
136 output = check_output(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, 120​ output, _ = os_utils.popen_rt(cmd, shell=True, cwd=basedir)
137 bufsize=1, close_fds=True, cwd=basedir, shell=True) ​
138 else: 121​ else:
139 cmd_type = [type(c) for c in cmd] if isinstance(cmd, list) else type(cmd) 122​ cmd_type = [type(c) for c in cmd] if isinstance(cmd, list) else type(cmd)
140 raise TaskotronDirectiveError("Command %r is in invalid type %r, use string or" 123​ raise TaskotronDirectiveError("Command %r is in invalid type %r, use string or"
141 " list of strings" % (cmd, cmd_type)) 124​ " list of strings" % (cmd, cmd_type))
142 125​
143 except subprocess.CalledProcessError as e: 126​ except subprocess.CalledProcessError as e:
144 if not ignorereturn: 127​ if not ignorereturn:
145 raise TaskotronDirectiveError(e) 128​ raise TaskotronDirectiveError(e)
146 except OSError as e: 129​ except OSError as e:
147 raise TaskotronDirectiveError(e) 130​ raise TaskotronDirectiveError(e)
148 131​
149 return output 132​ return output
View Options

libtaskotron/os_utils.py

1# -*- coding: utf-8 -*- 1​# -*- coding: utf-8 -*-
2# Copyright 2009-2016, Red Hat, Inc. 2​# Copyright 2009-2016, Red Hat, Inc.
3# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+> 3​# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>
4# See the LICENSE file for more details on Licensing 4​# See the LICENSE file for more details on Licensing
5 5​
6''' Utility methods related to an operating system ''' 6​''' Utility methods related to an operating system '''
7 7​
8from __future__ import absolute_import 8​from __future__ import absolute_import
9import os 9​import os
10import subprocess 10​import subprocess
11 11​
12from libtaskotron.logger import log 12​from libtaskotron.logger import log
13​import libtaskotron.exceptions as exc
13 14​
14 15​
15def is_root(): 16​def is_root():
16 '''Determine whether we're currently running under the root account. 17​ '''Determine whether we're currently running under the root account.
17 18​
18 :rtype: bool 19​ :rtype: bool
19 ''' 20​ '''
20 return os.geteuid() == 0 21​ return os.geteuid() == 0
Show All 18 Lines39 try:
39 subprocess.check_output(cmd, stderr=subprocess.STDOUT) 40​ subprocess.check_output(cmd, stderr=subprocess.STDOUT)
40 except subprocess.CalledProcessError as e: 41​ except subprocess.CalledProcessError as e:
41 log.debug(u"✘ Sudo access is not available. Received exit code %d and output:\n%s", 42​ log.debug(u"✘ Sudo access is not available. Received exit code %d and output:\n%s",
42 e.returncode, e.output.rstrip()) 43​ e.returncode, e.output.rstrip())
43 return False 44​ return False
44 else: 45​ else:
45 log.debug(u'✔ Sudo access is available') 46​ log.debug(u'✔ Sudo access is available')
46 return True 47​ return True
48​
49​
50​def popen_rt(cmd, stderr=subprocess.STDOUT, bufsize=1, **kwargs):
51​ """This is similar to :func:`subprocess.check_output`, but with real-time printing to console
52​ as well. It is useful for longer-running tasks for which you'd like to both capture the output
53​ and also see it printed in terminal continuously.
54​
55​ Please note that by default both ``stdout`` and ``stderr`` are merged together. You can use
56​ ``stderr=subprocess.PIPE``, and it will be returned to you separated, but it won't be printed
57​ out to console (only ``stdout`` will).
58​
59​ The parameters are the same as for :class:`subprocess.Popen`. You can't use ``stdout``
60​ parameter, that one is hardcoded to :attr:`subprocess.PIPE`.
61​
62​ :return: tuple of ``(stdoutdata, stderrdata)``. ``stderrdata`` will be ``None`` by default
63​ (because ``stderr`` is redirected to ``stdout``).
64​ :raise TaskotronValueError: if you provide ``stdout`` parameter
65​ :raise subprocess.CalledProcessError: if the command exits with non-zero exit code (helpful
66​ attributes are provided, study its documentation)
67​ """
68​ if 'stdout' in kwargs:
69​ raise exc.TaskotronValueError('stdout parameter not allowed, it will be overridden')
70​
71​ proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=stderr, bufsize=bufsize, **kwargs)
72​ output = []
73​ for line in iter(proc.stdout.readline, ''):
74​ print line,
75​ output.append(line)
76​
77​ (stdoutdata, stderrdata) = proc.communicate()
78​ assert proc.returncode is not None
79​ assert not stdoutdata
80​ if stderr == subprocess.STDOUT:
81​ assert not stderrdata
82​
83​ stdoutdata = ''.join(output)
84​
85​ if proc.returncode:
86​ raise subprocess.CalledProcessError(proc.returncode, cmd, stdoutdata)
87​
88​ return (stdoutdata, stderrdata)
View Options

testing/functest_os_utils.py

  • This file was added.
1​# -*- coding: utf-8 -*-
2​# Copyright 2016, Red Hat, Inc.
3​# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>
4​# See the LICENSE file for more details on Licensing
5​
6​'''Functional tests for libtaskotron.os_utils'''
7​
8​import subprocess
9​import pytest
10​
11​from libtaskotron.os_utils import popen_rt
12​
13​
14​class TestPopenRT(object):
15​ '''Test os_utils.popen_rt()'''
16​
17​ def test_exitcode_zero(self):
18​ out, err = popen_rt(['true'])
19​ assert not out
20​ assert not err
21​
22​ def test_exitcode_nonzero(self):
23​ cmd = ['false']
24​ with pytest.raises(subprocess.CalledProcessError) as excinfo:
25​ popen_rt(cmd)
26​ err = excinfo.value
27​ assert err.returncode > 0
28​ assert err.cmd == cmd
29​ assert not err.output
30​
31​ def test_stdout(self):
32​ text = 'Lorem ipsum'
33​ out, err = popen_rt(['echo', '-n', text])
34​ assert out == text
35​ assert not err
36​
37​ def test_stderr(self):
38​ text = 'Lorem ipsum'
39​ out, err = popen_rt('echo -n %s >&2' % text, shell=True, stderr=subprocess.PIPE)
40​ assert not out
41​ assert err == text
42​
43​ def test_stream_merge(self):
44​ text_out = 'this is stdout'
45​ text_err = 'this is stderr'
46​ out, err = popen_rt('echo -n %s; echo -n %s >&2' % (text_out, text_err), shell=True)
47​ assert out == text_out + text_err
48​ assert not err
49​
50​ def test_stream_separate(self):
51​ text_out = 'this is stdout'
52​ text_err = 'this is stderr'
53​ out, err = popen_rt('echo -n %s; echo -n %s >&2' % (text_out, text_err), shell=True,
54​ stderr=subprocess.PIPE)
55​ assert out == text_out
56​ assert err == text_err
57​
58​ def test_shell(self):
59​ popen_rt('true && true', shell=True)
60​
61​ def test_stdin(self, tmpdir):
62​ text = 'foo\nbar'
63​ infile = tmpdir.join('stdin')
64​ infile.write(text)
65​ out, err = popen_rt(['cat'], stdin=infile.open())
66​ assert out == text
67​ assert not err
View Options

testing/functest_shell_directive.py

1# -*- coding: utf-8 -*- 1​# -*- coding: utf-8 -*-
2# Copyright 2009-2016, Red Hat, Inc. 2​# Copyright 2009-2016, Red Hat, Inc.
3# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+> 3​# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>
4# See the LICENSE file for more details on Licensing 4​# See the LICENSE file for more details on Licensing
5 5​
6from libtaskotron.directives import shell_directive 6​from libtaskotron.directives import shell_directive
7from libtaskotron.exceptions import TaskotronDirectiveError 7​from libtaskotron.exceptions import TaskotronDirectiveError
8 8​
9import os 9​import os
10import pytest 10​import pytest
11 11​
12OUTPUT = "Piko a Rybi prsty" 12​OUTPUT = "Happy panda"
13PATH = os.path.realpath(__file__) 13​PATH = os.path.realpath(__file__)
14 14​
15 15​
16class TestCheckCommand(object): 16​class TestCheckCommand(object):
17 17​
18 def test_existing_command(self): 18​ def test_existing_command(self):
19 test_params = ['echo -n %s' % OUTPUT] 19​ test_params = ['echo -n %s' % OUTPUT]
20 test_arg_data = {'task': PATH} 20​ test_arg_data = {'task': PATH}
▲ Show 20 Lines • Show All 73 Lines • Show Last 20 Lines
View Options

testing/test_os_utils.py

1# -*- coding: utf-8 -*- 1​# -*- coding: utf-8 -*-
2# Copyright 2016, Red Hat, Inc. 2​# Copyright 2016, Red Hat, Inc.
3# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+> 3​# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>
4# See the LICENSE file for more details on Licensing 4​# See the LICENSE file for more details on Licensing
5 5​
6'''Unit tests for libtaskotron.os_utils''' 6​'''Unit tests for libtaskotron.os_utils'''
7 7​
8import os 8​import os
9import subprocess 9​import subprocess
10import mock 10​import mock
11​import pytest
11 12​
12from libtaskotron.os_utils import is_root, has_sudo 13​from libtaskotron.os_utils import is_root, has_sudo, popen_rt
14​import libtaskotron.exceptions as exc
13 15​
14 16​
15class TestIsRoot(): 17​class TestIsRoot(object):
16 '''Test os_utils.is_root()''' 18​ '''Test os_utils.is_root()'''
17 19​
18 def test_root(self, monkeypatch): 20​ def test_root(self, monkeypatch):
19 mock_geteuid = mock.Mock(return_value=0) 21​ mock_geteuid = mock.Mock(return_value=0)
20 monkeypatch.setattr(os, 'geteuid', mock_geteuid) 22​ monkeypatch.setattr(os, 'geteuid', mock_geteuid)
21 assert is_root() 23​ assert is_root()
22 24​
23 def test_not_root(self, monkeypatch): 25​ def test_not_root(self, monkeypatch):
24 mock_geteuid = mock.Mock(return_value=1000) 26​ mock_geteuid = mock.Mock(return_value=1000)
25 monkeypatch.setattr(os, 'geteuid', mock_geteuid) 27​ monkeypatch.setattr(os, 'geteuid', mock_geteuid)
26 assert not is_root() 28​ assert not is_root()
27 29​
28 30​
29class TestHasSudo(): 31​class TestHasSudo(object):
30 '''Test os_utils.has_sudo()''' 32​ '''Test os_utils.has_sudo()'''
31 33​
32 def test_sudo(self, monkeypatch): 34​ def test_sudo(self, monkeypatch):
33 mock_check_output = mock.Mock() 35​ mock_check_output = mock.Mock()
34 monkeypatch.setattr(subprocess, 'check_output', mock_check_output) 36​ monkeypatch.setattr(subprocess, 'check_output', mock_check_output)
35 assert has_sudo() 37​ assert has_sudo()
36 38​
37 def test_no_sudo(self, monkeypatch): 39​ def test_no_sudo(self, monkeypatch):
38 err = subprocess.CalledProcessError(1, 'cmd', output='') 40​ err = subprocess.CalledProcessError(1, 'cmd', output='')
39 mock_check_output = mock.Mock(side_effect=err) 41​ mock_check_output = mock.Mock(side_effect=err)
40 monkeypatch.setattr(subprocess, 'check_output', mock_check_output) 42​ monkeypatch.setattr(subprocess, 'check_output', mock_check_output)
41 assert not has_sudo() 43​ assert not has_sudo()
44​
45​
46​class TestPopenRT(object):
47​ '''Test os_utils.popen_rt()'''
48​
49​ def test_raise_stdout(self):
50​ with pytest.raises(exc.TaskotronValueError):
51​ popen_rt(['false'], stdout=None)
View Options

testing/test_shell_directive.py

1# -*- coding: utf-8 -*- 1​# -*- coding: utf-8 -*-
2# Copyright 2009-2016, Red Hat, Inc. 2​# Copyright 2009-2016, Red Hat, Inc.
3# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+> 3​# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>
4# See the LICENSE file for more details on Licensing 4​# See the LICENSE file for more details on Licensing
5 5​
6from libtaskotron.directives import shell_directive 6​from libtaskotron.directives import shell_directive
7from libtaskotron.exceptions import TaskotronDirectiveError 7​from libtaskotron.exceptions import TaskotronDirectiveError
8​import libtaskotron.os_utils
8 9​
9import os 10​import os
10import pytest 11​import pytest
11import mock 12​import mock
12 13​
13OUTPUT = "Piko a Rybi prsty" 14​OUTPUT = "Happy panda"
14PATH = os.path.realpath(__file__) 15​PATH = os.path.realpath(__file__)
15 16​
16 17​
17@pytest.mark.usefixtures("setup") 18​@pytest.mark.usefixtures("setup")
18class TestCheckCommand(object): 19​class TestCheckCommand(object):
19 20​
20 @pytest.fixture 21​ @pytest.fixture
21 def setup(self, monkeypatch): 22​ def setup(self, monkeypatch):
22 self.shell_dir = shell_directive.ShellDirective() 23​ self.shell_dir = shell_directive.ShellDirective()
23 self.mock_check_output = mock.Mock() 24​ self.mock_run = mock.Mock()
24 monkeypatch.setattr(shell_directive, 'check_output', self.mock_check_output) 25​ monkeypatch.setattr(libtaskotron.os_utils, 'popen_rt', self.mock_run)
25 self.arg_data = {'task': PATH} 26​ self.arg_data = {'task': PATH}
26 27​
27 def test_a_list(self): 28​ def test_a_list(self):
28 test_params = ['cmd.exe'] 29​ test_params = ['cmd.exe']
29 self.mock_check_output.return_value = OUTPUT 30​ self.mock_run.return_value = (OUTPUT, '')
30 31​
31 output = self.shell_dir.process(test_params, self.arg_data) 32​ output = self.shell_dir.process(test_params, self.arg_data)
32 33​
33 assert output == OUTPUT 34​ assert output == OUTPUT
34 35​
35 def test_not_a_list(self, monkeypatch): 36​ def test_not_a_list(self, monkeypatch):
36 test_params = {'a dict': 'instead of a list'} 37​ test_params = {'a dict': 'instead of a list'}
37 38​
38 with pytest.raises(TaskotronDirectiveError): 39​ with pytest.raises(TaskotronDirectiveError):
39 self.shell_dir.process(test_params, self.arg_data) 40​ self.shell_dir.process(test_params, self.arg_data)
40 41​
41 assert not self.mock_check_output.called 42​ assert not self.mock_run.called
42 43​
43 def test_cmd_not_a_str(self, monkeypatch): 44​ def test_cmd_not_a_str(self, monkeypatch):
44 test_params = [False] 45​ test_params = [False]
45 46​
46 with pytest.raises(TaskotronDirectiveError): 47​ with pytest.raises(TaskotronDirectiveError):
47 self.shell_dir.process(test_params, self.arg_data) 48​ self.shell_dir.process(test_params, self.arg_data)
48 49​
49 assert not self.mock_check_output.called 50​ assert not self.mock_run.called
50 51​
51 def test_cmd_not_a_list_of_strs(self, monkeypatch): 52​ def test_cmd_not_a_list_of_strs(self, monkeypatch):
52 test_params = [['echo', False]] 53​ test_params = [['echo', False]]
53 54​
54 with pytest.raises(TaskotronDirectiveError): 55​ with pytest.raises(TaskotronDirectiveError):
55 self.shell_dir.process(test_params, self.arg_data) 56​ self.shell_dir.process(test_params, self.arg_data)
56 57​
57 assert not self.mock_check_output.called 58​ assert not self.mock_run.called