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

diff --git a/neutron/agent/linux/async_process.py b/neutron/agent/linux/async_process.py
index 42705b4fca68dc607291897d7de027b92c5b0b9c..63bfa939ebcebe0c9716a404d7bb52815f53bf8a 100644 (file)
--- 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 f292906fd90c9fac93acbcb2f4cf497affc3663c..d6ab603bbe943f44b3239c5abc377b7011e1972e 100644 (file)
--- 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 93bc06fe3665ccf9bfc4785fd4ea9ea2bff8a8b9..51284c8ea7ed98cf3306b19ce62e7a1b8c3f0da1 100644 (file)
--- 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 d57a3015ed3ec9010d56bcb480c6238b3467967e..0ea63655a86496054e46707631f9944688057b9a 100644 (file)
--- 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 6b7fbbfd00d9263984d9541408fae869f1a64130..cccbf2024f622f4e0872dea9061ecb91a10a7490 100644 (file)
--- 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 c1f4b908229592cd1e09df4c038c87a5624d9177..703dc6341e861f226ce2d04abd973b4e3f15bf7b 100644 (file)
--- 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