diff --git a/neutron/agent/l3/ha_router.py b/neutron/agent/l3/ha_router.py index 2775706f5ef..306e69e49c9 100644 --- a/neutron/agent/l3/ha_router.py +++ b/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): diff --git a/neutron/agent/linux/external_process.py b/neutron/agent/linux/external_process.py index e8437336ae8..d5a3ce45a12 100644 --- a/neutron/agent/linux/external_process.py +++ b/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) - utils.execute(cmd, run_as_root=self.run_as_root, - privsep_exec=True) + self._kill_process(sig, pid) # 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] - return ['kill', '-%s' % (sig), pid] + utils.execute([kill_file, 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.""" diff --git a/neutron/agent/linux/keepalived.py b/neutron/agent/linux/keepalived.py index 910d0be96e2..cf6c6067611 100644 --- a/neutron/agent/linux/keepalived.py +++ b/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() diff --git a/neutron/agent/linux/utils.py b/neutron/agent/linux/utils.py index c7e39d73037..1e24c1873a5 100644 --- a/neutron/agent/linux/utils.py +++ b/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): - """Kill the process with the given pid using the given signal.""" +def kill_process(pid, signal, run_as_root=False, extra_ok_codes=None, + 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, - privsep_exec=True) - except exceptions.ProcessExecutionError: - if process_is_running(pid): - raise + LOG.debug("Start killing process %s, signal %s", pid, signal) + if run_as_root: + priv_utils.kill_process(pid, signal) + else: + 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): diff --git a/neutron/privileged/__init__.py b/neutron/privileged/__init__.py index f4fc471ede6..9c34936f4a5 100644 --- a/neutron/privileged/__init__.py +++ b/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], ) diff --git a/neutron/privileged/agent/linux/utils.py b/neutron/privileged/agent/linux/utils.py index a87edf907bf..58398120eb2 100644 --- a/neutron/privileged/agent/linux/utils.py +++ b/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)) diff --git a/neutron/tests/common/net_helpers.py b/neutron/tests/common/net_helpers.py index bff08b5ae8f..380efd836a6 100644 --- a/neutron/tests/common/net_helpers.py +++ b/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.', diff --git a/neutron/tests/functional/agent/linux/test_async_process.py b/neutron/tests/functional/agent/linux/test_async_process.py index 95317021677..ab4ac9cbb3f 100644 --- a/neutron/tests/functional/agent/linux/test_async_process.py +++ b/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, diff --git a/neutron/tests/functional/agent/linux/test_utils.py b/neutron/tests/functional/agent/linux/test_utils.py index b65233efd58..5eedec6a44b 100644 --- a/neutron/tests/functional/agent/linux/test_utils.py +++ b/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, - ['kill', '-9', sleep_pid], - check_exit_code=False, - run_as_root=True) + utils.kill_process, sleep_pid, signal.SIGKILL, run_as_root=True, + raise_exception=False) 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)) diff --git a/neutron/tests/functional/agent/test_dhcp_agent.py b/neutron/tests/functional/agent/test_dhcp_agent.py index 0eef55d62a3..e8e93b16270 100644 --- a/neutron/tests/functional/agent/test_dhcp_agent.py +++ b/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, diff --git a/neutron/tests/unit/agent/l3/test_ha_router.py b/neutron/tests/unit/agent/l3/test_ha_router.py index 2abd0df59e2..ac2ffc83dff 100644 --- a/neutron/tests/unit/agent/l3/test_ha_router.py +++ b/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( - sig=str(int(signal.SIGTERM))) + mock_pm.disable.assert_called_once_with(sig=signal.SIGTERM) def test_destroy_state_change_monitor_force(self): ri = self._create_router(mock.MagicMock()) diff --git a/neutron/tests/unit/agent/linux/test_external_process.py b/neutron/tests/unit/agent/linux/test_external_process.py index d2aa240098a..fc17595e983 100644 --- a/neutron/tests/unit/agent/linux/test_external_process.py +++ b/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([ - mock.call.execute(['kill', '-9', 4], - run_as_root=False, - privsep_exec=True)]) + utils.kill_process.assert_has_calls([ + mock.call(4, int(signal.SIGKILL), run_as_root=False)]) 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([ - mock.call.execute(['kill', '-9', 4], - run_as_root=True, - privsep_exec=True)]) + utils.kill_process.assert_has_calls([ + mock.call(4, int(signal.SIGKILL), run_as_root=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: - expected_cmd = ['test-service-kill', '9', 4] - else: - expected_cmd = ['kill', '-9', 4] + process_pid = 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( - expected_cmd, run_as_root=bool(namespace), - privsep_exec=True) + if kill_script_exists: + expected_cmd = ['test-service-kill', + 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( diff --git a/neutron/tests/unit/agent/linux/test_keepalived.py b/neutron/tests/unit/agent/linux/test_keepalived.py index 28474f9dc89..1200492db8f 100644 --- a/neutron/tests/unit/agent/linux/test_keepalived.py +++ b/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( - sig=str(int(signal.SIGTERM))) + process.disable.assert_called_once_with(sig=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=str(int(signal.SIGKILL)))]) + mock.call(sig=signal.SIGTERM), + mock.call(sig=signal.SIGKILL)]) diff --git a/neutron/tests/unit/agent/linux/test_utils.py b/neutron/tests/unit/agent/linux/test_utils.py index 4e01bb907cd..2aba6004c08 100644 --- a/neutron/tests/unit/agent/linux/test_utils.py +++ b/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):