diff --git a/neutron/agent/linux/async_process.py b/neutron/agent/linux/async_process.py index 3acec0d8655..f9b27d405b9 100644 --- a/neutron/agent/linux/async_process.py +++ b/neutron/agent/linux/async_process.py @@ -155,6 +155,7 @@ class AsyncProcess(object): if self._process: return utils.get_root_helper_child_pid( self._process.pid, + self.cmd_without_namespace, run_as_root=self.run_as_root) def _kill(self, kill_signal): diff --git a/neutron/agent/linux/utils.py b/neutron/agent/linux/utils.py index 9fc43e0ef93..30d6a206df3 100644 --- a/neutron/agent/linux/utils.py +++ b/neutron/agent/linux/utils.py @@ -217,15 +217,16 @@ def remove_conf_files(cfg_root, uuid): os.unlink(file_path) -def get_root_helper_child_pid(pid, run_as_root=False): +def get_root_helper_child_pid(pid, expected_cmd, run_as_root=False): """ - Get the lowest child pid in the process hierarchy + Get the first non root_helper child pid in the process hierarchy. If root helper was used, two or more processes would be created: - a root helper process (e.g. sudo myscript) - possibly a rootwrap script (e.g. neutron-rootwrap) - a child process (e.g. myscript) + - possibly its child processes Killing the root helper process will leave the child process running, re-parented to init, so the only way to ensure that both @@ -233,18 +234,17 @@ def get_root_helper_child_pid(pid, run_as_root=False): """ pid = str(pid) if run_as_root: - try: - pid = find_child_pids(pid)[0] - except IndexError: - # Process is already dead - return None while True: try: # We shouldn't have more than one child per process # so keep getting the children of the first one pid = find_child_pids(pid)[0] except IndexError: - # Last process in the tree, return it + return # We never found the child pid with expected_cmd + + # If we've found a pid with no root helper, return it. + # If we continue, we can find transient children. + if pid_invoked_with_cmdline(pid, expected_cmd): break return pid diff --git a/neutron/tests/common/net_helpers.py b/neutron/tests/common/net_helpers.py index 695414aa17e..9f5f67dd2d8 100644 --- a/neutron/tests/common/net_helpers.py +++ b/neutron/tests/common/net_helpers.py @@ -250,7 +250,7 @@ class RootHelperProcess(subprocess.Popen): sleep=CHILD_PROCESS_SLEEP): def child_is_running(): child_pid = utils.get_root_helper_child_pid( - self.pid, run_as_root=True) + self.pid, self.cmd, run_as_root=True) if utils.pid_invoked_with_cmdline(child_pid, self.cmd): return True @@ -260,7 +260,7 @@ class RootHelperProcess(subprocess.Popen): exception=RuntimeError("Process %s hasn't been spawned " "in %d seconds" % (self.cmd, timeout))) self.child_pid = utils.get_root_helper_child_pid( - self.pid, run_as_root=True) + self.pid, self.cmd, run_as_root=True) @property def is_running(self): diff --git a/neutron/tests/contrib/functional-testing.filters b/neutron/tests/contrib/functional-testing.filters index 7a94728e2d9..70eaf1a92c2 100644 --- a/neutron/tests/contrib/functional-testing.filters +++ b/neutron/tests/contrib/functional-testing.filters @@ -37,3 +37,7 @@ touch_filter2: RegExpFilter, /usr/bin/touch, root, touch, /etc/netns/qdhcp-[0-9a # needed by test_ovs_flows which runs ovs-appctl ofproto/trace ovstrace_filter: RegExpFilter, ovs-appctl, root, ovs-appctl, ofproto/trace, .*, .* + +# needed for TestGetRootHelperChildPid +bash_filter: RegExpFilter, /bin/bash, root, bash, -c, \(sleep 100\) +sleep_kill: KillFilter, root, sleep, -9 diff --git a/neutron/tests/functional/agent/linux/test_utils.py b/neutron/tests/functional/agent/linux/test_utils.py index 5508457218e..c5c07e72c04 100644 --- a/neutron/tests/functional/agent/linux/test_utils.py +++ b/neutron/tests/functional/agent/linux/test_utils.py @@ -12,12 +12,15 @@ # License for the specific language governing permissions and limitations # under the License. +import functools + import eventlet import testtools from neutron.agent.linux import async_process from neutron.agent.linux import utils from neutron.tests.functional.agent.linux import test_async_process +from neutron.tests.functional import base as functional_base class TestPIDHelpers(test_async_process.AsyncProcessTestFramework): @@ -38,3 +41,61 @@ class TestPIDHelpers(test_async_process.AsyncProcessTestFramework): def test_wait_until_true_predicate_fails(self): with testtools.ExpectedException(eventlet.timeout.Timeout): utils.wait_until_true(lambda: False, 2) + + +class TestGetRootHelperChildPid(functional_base.BaseSudoTestCase): + def _addcleanup_sleep_process(self, parent_pid): + sleep_pid = utils.execute( + ['ps', '--ppid', parent_pid, '-o', 'pid=']).strip() + self.addCleanup( + utils.execute, + ['kill', '-9', sleep_pid], + check_exit_code=False, + run_as_root=True) + + def test_get_root_helper_child_pid_returns_first_child(self): + """Test that the first child, not lowest child pid is returned. + + Test creates following proccess tree: + sudo + + | + +--rootwrap + + | + +--bash+ + | + +--sleep 100 + + and tests that pid of `bash' command is returned. + """ + + def wait_for_sleep_is_spawned(parent_pid): + proc_tree = utils.execute( + ['pstree', parent_pid], check_exit_code=False) + processes = [command.strip() for command in proc_tree.split('---') + if command] + return 'sleep' == processes[-1] + + cmd = ['bash', '-c', '(sleep 100)'] + proc = async_process.AsyncProcess(cmd, run_as_root=True) + proc.start() + + # root helpers spawn their child processes asynchronously, and we + # don't want to use proc.start(block=True) as that uses + # get_root_helper_child_pid (The method under test) internally. + sudo_pid = proc._process.pid + utils.wait_until_true( + functools.partial( + wait_for_sleep_is_spawned, + sudo_pid), + sleep=0.1) + + child_pid = utils.get_root_helper_child_pid( + sudo_pid, cmd, run_as_root=True) + self.assertIsNotNone( + child_pid, + "get_root_helper_child_pid is expected to return the pid of the " + "bash process") + self._addcleanup_sleep_process(child_pid) + with open('/proc/%s/cmdline' % child_pid, 'r') as f_proc_cmdline: + cmdline = f_proc_cmdline.readline().split('\0')[0] + self.assertIn('bash', cmdline) diff --git a/neutron/tests/unit/agent/linux/test_utils.py b/neutron/tests/unit/agent/linux/test_utils.py index c93bc097249..362c512b2af 100644 --- a/neutron/tests/unit/agent/linux/test_utils.py +++ b/neutron/tests/unit/agent/linux/test_utils.py @@ -206,7 +206,8 @@ class TestFindChildPids(base.BaseTestCase): class TestGetRoothelperChildPid(base.BaseTestCase): def _test_get_root_helper_child_pid(self, expected=_marker, - run_as_root=False, pids=None): + run_as_root=False, pids=None, + cmds=None): def _find_child_pids(x): if not pids: return [] @@ -214,9 +215,17 @@ class TestGetRoothelperChildPid(base.BaseTestCase): return pids mock_pid = object() + pid_invoked_with_cmdline = {} + if cmds: + pid_invoked_with_cmdline['side_effect'] = cmds + else: + pid_invoked_with_cmdline['return_value'] = False with mock.patch.object(utils, 'find_child_pids', - side_effect=_find_child_pids): - actual = utils.get_root_helper_child_pid(mock_pid, run_as_root) + side_effect=_find_child_pids), \ + mock.patch.object(utils, 'pid_invoked_with_cmdline', + **pid_invoked_with_cmdline): + actual = utils.get_root_helper_child_pid( + mock_pid, mock.ANY, run_as_root) if expected is _marker: expected = str(mock_pid) self.assertEqual(expected, actual) @@ -226,12 +235,21 @@ class TestGetRoothelperChildPid(base.BaseTestCase): def test_returns_child_pid_as_root(self): self._test_get_root_helper_child_pid(expected='2', pids=['1', '2'], - run_as_root=True) + run_as_root=True, + cmds=[True]) def test_returns_last_child_pid_as_root(self): self._test_get_root_helper_child_pid(expected='3', pids=['1', '2', '3'], - run_as_root=True) + run_as_root=True, + cmds=[False, True]) + + def test_returns_first_non_root_helper_child(self): + self._test_get_root_helper_child_pid( + expected='2', + pids=['1', '2', '3'], + run_as_root=True, + cmds=[True, False]) def test_returns_none_as_root(self): self._test_get_root_helper_child_pid(expected=None, run_as_root=True)