Implement "kill" method using os.kill()

Implement the "kill" method (send a signal to a process) using the
Python native library "os".

In functional tests, "RootHelperProcess.kill" method should not fail if
the process does not exist.

Closes-Bug: #1843446
Closes-Bug: #1843418

Change-Id: Iee97a83779dd3e20eb3a223fb8557a94b8f15dc0

neutron/agent/l3/ha_router.py

@@ -440,12 +440,12 @@ class HaRouter(router.RouterInfo):
         pm = self._get_state_change_monitor_process_manager()
         process_monitor.unregister(
             self.router_id, IP_MONITOR_PROCESS_SERVICE)
-        pm.disable(sig=str(int(signal.SIGTERM)))
+        pm.disable(sig=signal.SIGTERM)
         try:
             common_utils.wait_until_true(lambda: not pm.active,
                                          timeout=SIGTERM_TIMEOUT)
         except common_utils.WaitTimeout:
-            pm.disable(sig=str(int(signal.SIGKILL)))
+            pm.disable(sig=signal.SIGKILL)
 
     @staticmethod
     def _gateway_ports_equal(port1, port2):

neutron/agent/linux/external_process.py

@@ -15,6 +15,7 @@
 import abc
 import collections
 import os.path
+import signal
 
 import eventlet
 from oslo_concurrency import lockutils
@@ -96,9 +97,10 @@ class ProcessManager(MonitoredProcess):
         if self.custom_reload_callback:
             self.disable(get_stop_command=self.custom_reload_callback)
         else:
-            self.disable('HUP')
+            self.disable(signal.SIGHUP)
 
-    def disable(self, sig='9', get_stop_command=None):
+    def disable(self, sig=signal.SIGKILL, get_stop_command=None):
+        sig = int(sig)
         pid = self.pid
 
         if self.active:
@@ -109,11 +111,9 @@ class ProcessManager(MonitoredProcess):
                                          run_as_root=self.run_as_root,
                                          privsep_exec=True)
             else:
-                cmd = self.get_kill_cmd(sig, pid)
+                self._kill_process(sig, pid)
-                utils.execute(cmd, run_as_root=self.run_as_root,
-                              privsep_exec=True)
                 # In the case of shutting down, remove the pid file
-                if sig == '9':
+                if sig == signal.SIGKILL:
                     utils.delete_if_exists(self.get_pid_file_name(),
                                            run_as_root=self.run_as_root)
         elif pid:
@@ -125,12 +125,14 @@ class ProcessManager(MonitoredProcess):
             LOG.debug('No %(service)s process started for %(uuid)s',
                       {'service': self.service, 'uuid': self.uuid})
 
-    def get_kill_cmd(self, sig, pid):
+    def _kill_process(self, sig, pid):
         if self.kill_scripts_path:
             kill_file = "%s-kill" % self.service
             if os.path.isfile(os.path.join(self.kill_scripts_path, kill_file)):
-                return [kill_file, sig, pid]
+                utils.execute([kill_file, sig, pid],
-        return ['kill', '-%s' % (sig), pid]
+                              run_as_root=self.run_as_root)
+                return
+        utils.kill_process(pid, sig, run_as_root=self.run_as_root)
 
     def get_pid_file_name(self):
         """Returns the file name for a given kind of config file."""

neutron/agent/linux/keepalived.py

@@ -479,7 +479,7 @@ class KeepalivedManager(object):
                                         service_name=KEEPALIVED_SERVICE_NAME)
 
         pm = self.get_process()
-        pm.disable(sig=str(int(signal.SIGTERM)))
+        pm.disable(sig=signal.SIGTERM)
         try:
             utils.wait_until_true(lambda: not pm.active,
                                   timeout=SIGTERM_TIMEOUT)
@@ -487,7 +487,7 @@ class KeepalivedManager(object):
             LOG.warning('Keepalived process %s did not finish after SIGTERM '
                         'signal in %s seconds, sending SIGKILL signal',
                         pm.pid, SIGTERM_TIMEOUT)
-            pm.disable(sig=str(int(signal.SIGKILL)))
+            pm.disable(sig=signal.SIGKILL)
 
     def check_processes(self):
         keepalived_pm = self.get_process()

neutron/agent/linux/utils.py

@@ -214,14 +214,33 @@ def find_fork_top_parent(pid):
             return pid
 
 
-def kill_process(pid, signal, run_as_root=False):
+def kill_process(pid, signal, run_as_root=False, extra_ok_codes=None,
-    """Kill the process with the given pid using the given signal."""
+                 raise_exception=True):
+    """Kill the process with the given pid using the given signal.
+
+    :param pid: (str, int) process PID
+    :param signal: (str, int) signal to send
+    :param run_as_root: (bool) execute the command as root user
+    :param extra_ok_codes: (list of int) list of "errno" codes
+    :param raise_exception: (bool) if True, if an exception occurs, it is
+                            raised
+    :return: 0 if OK, "errno" code in case of muted exception (see
+             "extra_ok_codes")
+    """
     try:
-        execute(['kill', '-%d' % signal, pid], run_as_root=run_as_root,
+        LOG.debug("Start killing process %s, signal %s", pid, signal)
-                privsep_exec=True)
+        if run_as_root:
-    except exceptions.ProcessExecutionError:
+            priv_utils.kill_process(pid, signal)
-        if process_is_running(pid):
+        else:
-            raise
+            os.kill(int(pid), int(signal))
+        LOG.debug("Finish killing process %s, signal %s", pid, signal)
+    except OSError as exc:
+        with excutils.save_and_reraise_exception() as ctxt:
+            extra_ok_codes = extra_ok_codes or []
+            if exc.errno in extra_ok_codes or not raise_exception:
+                ctxt.reraise = False
+                return exc.errno
+    return 0
 
 
 def _get_conf_base(cfg_root, uuid, ensure_conf_dir):

neutron/privileged/__init__.py

@@ -26,7 +26,8 @@ default = priv_context.PrivContext(
                   caps.CAP_NET_ADMIN,
                   caps.CAP_DAC_OVERRIDE,
                   caps.CAP_DAC_READ_SEARCH,
-                  caps.CAP_SYS_PTRACE],
+                  caps.CAP_SYS_PTRACE,
+                  caps.CAP_KILL],
 )
 
 

neutron/privileged/agent/linux/utils.py

@@ -82,3 +82,9 @@ def _create_process(cmd, addl_env=None):
     obj = subprocess.Popen(cmd, shell=False, stdin=subprocess.PIPE,
                            stdout=subprocess.PIPE, stderr=subprocess.PIPE)
     return obj, cmd
+
+
+@privileged.default.entrypoint
+def kill_process(pid, signal):
+    """Kill the process with the given pid using the given signal."""
+    os.kill(int(pid), int(signal))

neutron/tests/common/net_helpers.py

@@ -16,6 +16,7 @@
 import abc
 from concurrent import futures
 import contextlib
+import errno
 import os
 import random
 import re
@@ -307,7 +308,10 @@ class RootHelperProcess(subprocess.Popen):
 
     def kill(self, sig=signal.SIGKILL):
         pid = self.child_pid or str(self.pid)
-        utils.execute(['kill', '-%d' % sig, pid], run_as_root=True)
+        if utils.kill_process(pid, sig, run_as_root=True,
+                              extra_ok_codes=[errno.ESRCH]):
+            LOG.debug('Process %s did not exists already so it could not be '
+                      'killed', pid)
 
     def read_stdout(self, timeout=None):
         return self._read_stream(self.stdout, timeout)
@@ -625,7 +629,8 @@ class NamespaceFixture(fixtures.Fixture):
                 if self.ip_wrapper.netns.exists(self.name):
                     for pid in ip_lib.list_namespace_pids(self.name):
                         utils.kill_process(pid, signal.SIGKILL,
-                                           run_as_root=True)
+                                           run_as_root=True,
+                                           extra_ok_codes=[errno.ESRCH])
                     self.ip_wrapper.netns.delete(self.name)
             except helpers.TestTimerTimeout:
                 LOG.warning('Namespace %s was not deleted due to a timeout.',

neutron/tests/functional/agent/linux/test_async_process.py

@@ -12,6 +12,8 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import signal
+
 import eventlet
 
 from neutron._i18n import _
@@ -71,7 +73,7 @@ class TestAsyncProcess(AsyncProcessTestFramework):
         # Ensure that the same output is read twice
         self._check_stdout(proc)
         pid = proc.pid
-        utils.execute(['kill', '-9', pid])
+        utils.kill_process(pid, signal.SIGKILL)
         common_utils.wait_until_true(
             lambda: proc.is_active() and pid != proc.pid,
             timeout=5,

neutron/tests/functional/agent/linux/test_utils.py

@@ -12,6 +12,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import errno
 import functools
 import os
 import signal
@@ -41,10 +42,8 @@ class TestGetRootHelperChildPid(functional_base.BaseSudoTestCase):
         sleep_pid = utils.execute(
             ['ps', '--ppid', parent_pid, '-o', 'pid=']).strip()
         self.addCleanup(
-            utils.execute,
+            utils.kill_process, sleep_pid, signal.SIGKILL, run_as_root=True,
-            ['kill', '-9', sleep_pid],
+            raise_exception=False)
-            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.
@@ -172,3 +171,55 @@ class TestFindChildPids(functional_base.BaseSudoTestCase):
         with open('/proc/sys/kernel/pid_max', 'r') as fd:
             pid_max = int(fd.readline().strip())
         self.assertEqual([], utils.find_child_pids(pid_max))
+
+
+class TestKillProcess(functional_base.BaseSudoTestCase):
+
+    # NOTE(ralonsoh): in 64bit systems, PID_MAX_LIMIT is 2^22
+    NON_EXISTING_PID = 2**22 + 1
+
+    def _test_exception(self, pid, error_no, run_as_root=False):
+        try:
+            utils.kill_process(pid, signal.SIGUSR1, run_as_root=run_as_root)
+        except OSError as exc:
+            self.assertEqual(error_no, exc.errno)
+
+    def test_root_no_such_process(self):
+        self._test_exception(self.NON_EXISTING_PID, errno.ESRCH,
+                             run_as_root=True)
+
+    def test_root_no_such_proces_hidden(self):
+        self.assertEqual(
+            errno.ESRCH,
+            utils.kill_process(self.NON_EXISTING_PID, signal.SIGUSR1,
+                               run_as_root=True, extra_ok_codes=[errno.ESRCH]))
+
+    def test_root_exception_not_risen(self):
+        self.assertEqual(
+            errno.ESRCH,
+            utils.kill_process(self.NON_EXISTING_PID, signal.SIGUSR1,
+                               run_as_root=True, raise_exception=False))
+
+    def test_non_root_no_such_process(self):
+        self._test_exception(self.NON_EXISTING_PID, errno.ESRCH)
+
+    def test_non_root_no_such_process_hidden(self):
+        self.assertEqual(
+            errno.ESRCH,
+            utils.kill_process(self.NON_EXISTING_PID, signal.SIGUSR1,
+                               extra_ok_codes=[errno.ESRCH]))
+
+    def test_non_root_operation_not_permitted(self):
+        self._test_exception(1, errno.EPERM)
+
+    def test_non_root_operation_not_permitted_hidden(self):
+        self.assertEqual(
+            errno.EPERM,
+            utils.kill_process(1, signal.SIGUSR1,
+                               extra_ok_codes=[errno.EPERM]))
+
+    def test_non_root_exception_not_risen(self):
+        self.assertEqual(
+            errno.ESRCH,
+            utils.kill_process(self.NON_EXISTING_PID, signal.SIGUSR1,
+                               raise_exception=False))

neutron/tests/functional/agent/test_dhcp_agent.py

@@ -15,6 +15,7 @@
 
 import copy
 import os.path
+import signal
 from unittest import mock
 
 import eventlet
@@ -325,7 +326,7 @@ class DHCPAgentOVSTestCase(DHCPAgentOVSTestFramework):
         pm, network = self._spawn_network_metadata_proxy()
         old_pid = pm.pid
 
-        utils.execute(['kill', '-9', old_pid], run_as_root=True)
+        utils.kill_process(old_pid, signal.SIGKILL, run_as_root=True)
         common_utils.wait_until_true(
             lambda: pm.active and pm.pid != old_pid,
             timeout=5,

neutron/tests/unit/agent/l3/test_ha_router.py

@@ -109,8 +109,7 @@ class TestBasicRouterOperations(base.BaseTestCase):
             mock_pm.active = False
             ri.destroy_state_change_monitor(mock_pm)
 
-        mock_pm.disable.assert_called_once_with(
+        mock_pm.disable.assert_called_once_with(sig=signal.SIGTERM)
-            sig=str(int(signal.SIGTERM)))
 
     def test_destroy_state_change_monitor_force(self):
         ri = self._create_router(mock.MagicMock())

neutron/tests/unit/agent/linux/test_external_process.py

@@ -13,6 +13,7 @@
 #    under the License.
 
 import os.path
+import signal
 from unittest import mock
 
 from neutron_lib import fixture as lib_fixtures
@@ -193,7 +194,7 @@ class TestProcessManager(base.BaseTestCase):
         with mock.patch.object(ep.ProcessManager, 'disable') as disable:
             manager = ep.ProcessManager(self.conf, 'uuid', namespace='ns')
             manager.reload_cfg()
-            disable.assert_called_once_with('HUP')
+            disable.assert_called_once_with(signal.SIGHUP)
 
     def test_reload_cfg_with_custom_reload_callback(self):
         reload_callback = mock.sentinel.callback
@@ -227,10 +228,8 @@ class TestProcessManager(base.BaseTestCase):
 
                 with mock.patch.object(ep, 'utils') as utils:
                     manager.disable()
-                    utils.assert_has_calls([
+                    utils.kill_process.assert_has_calls([
-                        mock.call.execute(['kill', '-9', 4],
+                        mock.call(4, int(signal.SIGKILL), run_as_root=False)])
-                                          run_as_root=False,
-                                          privsep_exec=True)])
 
     def test_disable_namespace(self):
         with mock.patch.object(ep.ProcessManager, 'pid') as pid:
@@ -242,10 +241,23 @@ class TestProcessManager(base.BaseTestCase):
 
                 with mock.patch.object(ep, 'utils') as utils:
                     manager.disable()
-                    utils.assert_has_calls([
+                    utils.kill_process.assert_has_calls([
-                        mock.call.execute(['kill', '-9', 4],
+                        mock.call(4, int(signal.SIGKILL), run_as_root=True)])
-                                          run_as_root=True,
+
-                                          privsep_exec=True)])
+    def test_disable_with_and_without_namespace(self):
+        namespaces = ['ns', None]
+        for namespace in namespaces:
+            with mock.patch.object(ep.ProcessManager, 'pid') as pid:
+                pid.__get__ = mock.Mock(return_value=4)
+                with mock.patch.object(ep.ProcessManager, 'active') as active:
+                    active.__get__ = mock.Mock(return_value=True)
+                    manager = ep.ProcessManager(self.conf, 'uuid',
+                                                namespace=namespace)
+                    with mock.patch.object(ep, 'utils') as utils:
+                        manager.disable()
+                        call = mock.call(4, int(signal.SIGKILL),
+                                         run_as_root=bool(namespace))
+                        utils.kill_process.assert_has_calls([call])
 
     def test_disable_not_active(self):
         with mock.patch.object(ep.ProcessManager, 'pid') as pid:
@@ -270,13 +282,10 @@ class TestProcessManager(base.BaseTestCase):
     def _test_disable_custom_kill_script(self, kill_script_exists, namespace,
                                          kill_scripts_path='test-path/'):
         cfg.CONF.set_override("kill_scripts_path", kill_scripts_path, "AGENT")
-        if kill_script_exists:
+        process_pid = 4
-            expected_cmd = ['test-service-kill', '9', 4]
-        else:
-            expected_cmd = ['kill', '-9', 4]
 
         with mock.patch.object(ep.ProcessManager, 'pid') as pid:
-            pid.__get__ = mock.Mock(return_value=4)
+            pid.__get__ = mock.Mock(return_value=process_pid)
             with mock.patch.object(ep.ProcessManager, 'active') as active:
                 active.__get__ = mock.Mock(return_value=True)
                 manager = ep.ProcessManager(
@@ -286,9 +295,15 @@ class TestProcessManager(base.BaseTestCase):
                         mock.patch.object(os.path, 'isfile',
                                           return_value=kill_script_exists):
                     manager.disable()
-                    utils.execute.assert_called_with(
+                    if kill_script_exists:
-                        expected_cmd, run_as_root=bool(namespace),
+                        expected_cmd = ['test-service-kill',
-                        privsep_exec=True)
+                                        signal.SIGKILL, process_pid]
+                        utils.execute.assert_called_with(
+                            expected_cmd, run_as_root=bool(namespace))
+                    else:
+                        utils.kill_process.assert_called_once_with(
+                            process_pid, signal.SIGKILL,
+                            run_as_root=bool(namespace))
 
     def test_disable_custom_kill_script_no_namespace(self):
         self._test_disable_custom_kill_script(

neutron/tests/unit/agent/linux/test_keepalived.py

@@ -634,8 +634,7 @@ class KeepalivedManagerTestCase(base.BaseTestCase):
         mock_get_process.return_value = process
         process.active = False
         self.keepalived_manager.disable()
-        process.disable.assert_called_once_with(
+        process.disable.assert_called_once_with(sig=int(signal.SIGTERM))
-            sig=str(int(signal.SIGTERM)))
 
     def test_destroy_force(self):
         mock_get_process = self.mock_get_process.start()
@@ -645,5 +644,5 @@ class KeepalivedManagerTestCase(base.BaseTestCase):
             process.active = True
             self.keepalived_manager.disable()
             process.disable.assert_has_calls([
-                mock.call(sig=str(int(signal.SIGTERM))),
+                mock.call(sig=signal.SIGTERM),
-                mock.call(sig=str(int(signal.SIGKILL)))])
+                mock.call(sig=signal.SIGKILL)])

neutron/tests/unit/agent/linux/test_utils.py

@@ -13,12 +13,9 @@
 #    under the License.
 
 import copy
-import signal
 import socket
 from unittest import mock
 
-import testtools
-
 from neutron_lib import exceptions
 from neutron_lib import fixture as lib_fixtures
 from oslo_config import cfg
@@ -227,39 +224,6 @@ class TestFindForkTopParent(base.BaseTestCase):
             pid_invoked_with_cmdline_retvals=[True, True, True])
 
 
-class TestKillProcess(base.BaseTestCase):
-    def _test_kill_process(self, pid, raise_exception=False,
-                           kill_signal=signal.SIGKILL, pid_killed=True):
-        if raise_exception:
-            exc = exceptions.ProcessExecutionError('', returncode=0)
-        else:
-            exc = None
-        with mock.patch.object(utils, 'execute',
-                               side_effect=exc) as mock_execute:
-            with mock.patch.object(utils, 'process_is_running',
-                                   return_value=not pid_killed):
-                utils.kill_process(pid, kill_signal, run_as_root=True)
-
-        mock_execute.assert_called_with(['kill', '-%d' % kill_signal, pid],
-                                        run_as_root=True, privsep_exec=True)
-
-    def test_kill_process_returns_none_for_valid_pid(self):
-        self._test_kill_process('1')
-
-    def test_kill_process_returns_none_for_stale_pid(self):
-        self._test_kill_process('1', raise_exception=True)
-
-    def test_kill_process_raises_exception_for_execute_exception(self):
-        with testtools.ExpectedException(exceptions.ProcessExecutionError):
-            # Simulate that the process is running after trying to kill due to
-            # any reason such as, for example, Permission denied
-            self._test_kill_process('1', raise_exception=True,
-                                    pid_killed=False)
-
-    def test_kill_process_with_different_signal(self):
-        self._test_kill_process('1', kill_signal=signal.SIGTERM)
-
-
 class TestGetCmdlineFromPid(base.BaseTestCase):
 
     def setUp(self):