From 61e9268bd4b696860eeb5a6ce59ae6bf42b1f0c8 Mon Sep 17 00:00:00 2001 From: Terry Wilson Date: Tue, 22 Oct 2013 13:22:18 -0500 Subject: [PATCH] Ensure get_pid_to_kill works with rootwrap script To ensure that correct process is killed when using a rootwrap script, we must recursively list the children of our top-level process and kill the last one. This patch uses the psutil python module which is already used in the heat-cfntools project. Change-Id: I702bb9dd794c08fcaab637284ee303de1778cbb9 --- neutron/agent/linux/async_process.py | 20 ++++++++-------- neutron/agent/linux/utils.py | 14 ----------- .../agent/linux/test_ovsdb_monitor.py | 3 ++- .../unit/agent/linux/test_async_process.py | 16 +++++++++++-- neutron/tests/unit/test_agent_linux_utils.py | 23 ------------------- requirements.txt | 1 + 6 files changed, 28 insertions(+), 49 deletions(-) diff --git a/neutron/agent/linux/async_process.py b/neutron/agent/linux/async_process.py index 42705b4fc..63bfa939e 100644 --- a/neutron/agent/linux/async_process.py +++ b/neutron/agent/linux/async_process.py @@ -18,6 +18,7 @@ import eventlet import eventlet.event import eventlet.queue import eventlet.timeout +import psutil from neutron.agent.linux import utils from neutron.openstack.common import log as logging @@ -129,21 +130,22 @@ class AsyncProcess(object): def _get_pid_to_kill(self): pid = self._process.pid - # If root helper was used, two processes will be created: + # If root helper was used, two or more processes will be created: # # - a root helper process (e.g. sudo myscript) + # - possibly a rootwrap script (e.g. neutron-rootwrap) # - a child process (e.g. myscript) # # Killing the root helper process will leave the child process - # as a zombie, so the only way to ensure that both die is to - # target the child process directly. + # running, re-parented to init, so the only way to ensure that both + # die is to target the child process directly. if self.root_helper: - pids = utils.find_child_pids(pid) - if pids: - # The root helper will only ever launch a single child. - pid = pids[0] - else: - # Process is already dead. + try: + # This assumes that there are not multiple children in any + # level of the process tree under the parent process. + pid = psutil.Process( + self._process.pid).get_children(recursive=True)[-1].pid + except (psutil.NoSuchProcess, IndexError): pid = None return pid diff --git a/neutron/agent/linux/utils.py b/neutron/agent/linux/utils.py index f292906fd..d6ab603bb 100644 --- a/neutron/agent/linux/utils.py +++ b/neutron/agent/linux/utils.py @@ -108,17 +108,3 @@ def replace_file(file_name, data): tmp_file.close() os.chmod(tmp_file.name, 0o644) os.rename(tmp_file.name, file_name) - - -def find_child_pids(pid): - """Retrieve a list of the pids of child processes of the given pid.""" - try: - raw_pids = execute(['ps', '--ppid', pid, '-o', 'pid=']) - except RuntimeError as e: - # Exception has already been logged by execute - no_children_found = 'Exit code: 1' in str(e) - if no_children_found: - return [] - # Unexpected errors are the responsibility of the caller - raise - return [x.strip() for x in raw_pids.split('\n') if x.strip()] diff --git a/neutron/tests/functional/agent/linux/test_ovsdb_monitor.py b/neutron/tests/functional/agent/linux/test_ovsdb_monitor.py index 93bc06fe3..51284c8ea 100644 --- a/neutron/tests/functional/agent/linux/test_ovsdb_monitor.py +++ b/neutron/tests/functional/agent/linux/test_ovsdb_monitor.py @@ -69,7 +69,8 @@ class BaseMonitorTest(base.BaseTestCase): self._check_test_requirements() - self.root_helper = 'sudo' + # Emulate using a rootwrap script with sudo + self.root_helper = 'sudo sudo' self.ovs = ovs_lib.BaseOVS(self.root_helper) self.bridge = create_ovs_resource('test-br-', self.ovs.add_bridge) diff --git a/neutron/tests/unit/agent/linux/test_async_process.py b/neutron/tests/unit/agent/linux/test_async_process.py index d57a3015e..0ea63655a 100644 --- a/neutron/tests/unit/agent/linux/test_async_process.py +++ b/neutron/tests/unit/agent/linux/test_async_process.py @@ -14,6 +14,8 @@ # License for the specific language governing permissions and limitations # under the License. +import collections + import eventlet.event import eventlet.queue import eventlet.timeout @@ -187,11 +189,17 @@ class TestAsyncProcess(base.BaseTestCase): root_helper=None, pids=None): if root_helper: self.proc.root_helper = root_helper + + xpid = collections.namedtuple('xpid', ['pid']) + xpids = [xpid(pid) for pid in pids or []] + with mock.patch.object(self.proc, '_process') as mock_process: with mock.patch.object(mock_process, 'pid') as mock_pid: - with mock.patch.object(utils, 'find_child_pids', - return_value=pids): + with mock.patch('psutil.Process') as mock_ps_process: + instance = mock_ps_process.return_value + instance.get_children.return_value = xpids actual = self.proc._get_pid_to_kill() + if expected is _marker: expected = mock_pid self.assertEqual(expected, actual) @@ -202,6 +210,10 @@ class TestAsyncProcess(base.BaseTestCase): def test__get_pid_to_kill_returns_child_pid_with_root_helper(self): self._test__get_pid_to_kill(expected='1', pids=['1'], root_helper='a') + def test__get_pid_to_kill_returns_last_child_pid_with_root_Helper(self): + self._test__get_pid_to_kill(expected='3', pids=['1', '2', '3'], + root_helper='a') + def test__get_pid_to_kill_returns_none_with_root_helper(self): self._test__get_pid_to_kill(expected=None, root_helper='a') diff --git a/neutron/tests/unit/test_agent_linux_utils.py b/neutron/tests/unit/test_agent_linux_utils.py index 6b7fbbfd0..cccbf2024 100644 --- a/neutron/tests/unit/test_agent_linux_utils.py +++ b/neutron/tests/unit/test_agent_linux_utils.py @@ -17,7 +17,6 @@ import fixtures import mock -import testtools from neutron.agent.linux import utils from neutron.tests import base @@ -107,25 +106,3 @@ class AgentUtilsReplaceFile(base.BaseTestCase): ntf.assert_has_calls(expected) chmod.assert_called_once_with('/baz', 0o644) rename.assert_called_once_with('/baz', '/foo') - - -class TestFindChildPids(base.BaseTestCase): - - def test_returns_empty_list_for_exit_code_1(self): - with mock.patch.object(utils, 'execute', - side_effect=RuntimeError('Exit code: 1')): - self.assertEqual(utils.find_child_pids(-1), []) - - def test_returns_empty_list_for_no_output(self): - with mock.patch.object(utils, 'execute', return_value=''): - self.assertEqual(utils.find_child_pids(-1), []) - - def test_returns_list_of_child_process_ids_for_good_ouput(self): - with mock.patch.object(utils, 'execute', return_value=' 123 \n 185\n'): - self.assertEqual(utils.find_child_pids(-1), ['123', '185']) - - def test_raises_unknown_exception(self): - with testtools.ExpectedException(RuntimeError): - with mock.patch.object(utils, 'execute', - side_effect=RuntimeError()): - utils.find_child_pids(-1) diff --git a/requirements.txt b/requirements.txt index c1f4b9082..703dc6341 100644 --- a/requirements.txt +++ b/requirements.txt @@ -16,6 +16,7 @@ jsonrpclib Jinja2 kombu>=2.4.8 netaddr>=0.7.6 +psutil>=0.6.1,<1.0 python-neutronclient>=2.3.0,<3 SQLAlchemy>=0.7.8,<=0.7.99 WebOb>=1.2.3,<1.3