Change get_root_helper_child_pid to stop when it finds cmd

get_root_helper_child_pid recursively finds the child of pid,
until it can no longer find a child. However, the intention is
not to find the deepest child, but to strip away root helpers.
For example 'sudo neutron-rootwrap x' is supposed to find the
pid of x. However, in cases 'x' spawned quick lived children of
its own (For example: ip / brctl / ovs invocations),
get_root_helper_child_pid returned those pids if called in
the wrong time.

Change-Id: I582aa5c931c8bfe57f49df6899445698270bb33e
Closes-Bug: #1558819

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):

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
 

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):

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

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)

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)