From ecbc2e3ed36964ed8944b3a128cde6850e250dd5 Mon Sep 17 00:00:00 2001 From: Jakub Libosvar Date: Thu, 17 Sep 2015 13:26:05 +0000 Subject: [PATCH] Introduce kill_signal parameter to AsynProcess.stop() All stop() calls of instances of AsyncProcess class were sending hardcoded SIGKILL signal to its process. This patch leaves the default behavior to SIGKILL but offers any number to be sent to kill command. Note: Internal private methods also got a new parameter which is not appended. Given that those methods are private and thus not used outside of the class, we can afford it. Change-Id: Ib7b0273c134d59c6a50173d4c2eb35761fcd3d62 Related-Bug: #1487548 --- neutron/agent/linux/async_process.py | 19 +++++++++----- .../agent/linux/test_ovsdb_monitor.py | 4 ++- .../unit/agent/linux/test_async_process.py | 26 ++++++++++++------- 3 files changed, 31 insertions(+), 18 deletions(-) diff --git a/neutron/agent/linux/async_process.py b/neutron/agent/linux/async_process.py index b11c263f2..c31210bd4 100644 --- a/neutron/agent/linux/async_process.py +++ b/neutron/agent/linux/async_process.py @@ -12,6 +12,8 @@ # License for the specific language governing permissions and limitations # under the License. +import signal + import eventlet import eventlet.event import eventlet.queue @@ -103,16 +105,18 @@ class AsyncProcess(object): if block: utils.wait_until_true(self.is_active) - def stop(self, block=False): + def stop(self, block=False, kill_signal=signal.SIGKILL): """Halt the process and watcher threads. :param block: Block until the process has stopped. + :param kill_signal: Number of signal that will be sent to the process + when terminating the process :raises eventlet.timeout.Timeout if blocking is True and the process did not stop in time. """ if self._kill_event: LOG.debug('Halting async process [%s].', self.cmd) - self._kill() + self._kill(kill_signal) else: raise AsyncProcessException(_('Process is not running.')) @@ -142,7 +146,7 @@ class AsyncProcess(object): self._process.pid, run_as_root=self.run_as_root) - def _kill(self, respawning=False): + def _kill(self, kill_signal, respawning=False): """Kill the process and the associated watcher greenthreads. :param respawning: Optional, whether respawn will be subsequently @@ -153,18 +157,19 @@ class AsyncProcess(object): pid = self.pid if pid: - self._kill_process(pid) + self._kill_process(pid, kill_signal) if not respawning: # Clear the kill event to ensure the process can be # explicitly started again. self._kill_event = None - def _kill_process(self, pid): + def _kill_process(self, pid, kill_signal): try: # A process started by a root helper will be running as # root and need to be killed via the same helper. - utils.execute(['kill', '-9', pid], run_as_root=self.run_as_root) + utils.execute(['kill', '-%d' % kill_signal, pid], + run_as_root=self.run_as_root) except Exception as ex: stale_pid = (isinstance(ex, RuntimeError) and 'No such process' in str(ex)) @@ -185,7 +190,7 @@ class AsyncProcess(object): respawning = True else: respawning = False - self._kill(respawning=respawning) + self._kill(signal.SIGKILL, respawning=respawning) if respawning: eventlet.sleep(self.respawn_interval) LOG.debug('Respawning async process [%s].', self.cmd) diff --git a/neutron/tests/functional/agent/linux/test_ovsdb_monitor.py b/neutron/tests/functional/agent/linux/test_ovsdb_monitor.py index e88329df4..53a074ccd 100644 --- a/neutron/tests/functional/agent/linux/test_ovsdb_monitor.py +++ b/neutron/tests/functional/agent/linux/test_ovsdb_monitor.py @@ -22,6 +22,8 @@ Tests in this module will be skipped unless: - sudo testing is enabled (see neutron.tests.functional.base for details) """ +import signal + import eventlet from oslo_config import cfg @@ -82,7 +84,7 @@ class TestOvsdbMonitor(BaseMonitorTest): old_pid = self.monitor._process.pid output1 = self.collect_initial_output() pid = utils.get_root_helper_child_pid(old_pid, run_as_root=True) - self.monitor._kill_process(pid) + self.monitor._kill_process(pid, signal.SIGKILL) self.monitor._reset_queues() while (self.monitor._process.pid == old_pid): eventlet.sleep(0.01) diff --git a/neutron/tests/unit/agent/linux/test_async_process.py b/neutron/tests/unit/agent/linux/test_async_process.py index 7d116d14b..5bf909759 100644 --- a/neutron/tests/unit/agent/linux/test_async_process.py +++ b/neutron/tests/unit/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.event import eventlet.queue import eventlet.timeout @@ -57,7 +59,7 @@ class TestAsyncProcess(base.BaseTestCase): with mock.patch.object(self.proc, '_kill') as kill: self.proc._handle_process_error() - kill.assert_has_calls([mock.call(respawning=False)]) + kill.assert_has_calls([mock.call(signal.SIGKILL, respawning=False)]) def test__handle_process_error_kills_without_respawn(self): self.proc.respawn_interval = 1 @@ -66,7 +68,7 @@ class TestAsyncProcess(base.BaseTestCase): with mock.patch('eventlet.sleep') as sleep: self.proc._handle_process_error() - kill.assert_has_calls([mock.call(respawning=True)]) + kill.assert_has_calls([mock.call(signal.SIGKILL, respawning=True)]) sleep.assert_has_calls([mock.call(self.proc.respawn_interval)]) spawn.assert_called_once_with() @@ -161,7 +163,7 @@ class TestAsyncProcess(base.BaseTestCase): mock.patch.object(self.proc, '_kill_process' ) as mock_kill_process,\ mock.patch.object(self.proc, '_process'): - self.proc._kill(respawning) + self.proc._kill(signal.SIGKILL, respawning) if respawning: self.assertIsNotNone(self.proc._kill_event) @@ -170,7 +172,7 @@ class TestAsyncProcess(base.BaseTestCase): mock_kill_event.send.assert_called_once_with() if pid: - mock_kill_process.assert_called_once_with(pid) + mock_kill_process.assert_called_once_with(pid, signal.SIGKILL) def test__kill_when_respawning_does_not_clear_kill_event(self): self._test__kill(True) @@ -181,7 +183,8 @@ class TestAsyncProcess(base.BaseTestCase): def test__kill_targets_process_for_pid(self): self._test__kill(False, pid='1') - def _test__kill_process(self, pid, expected, exception_message=None): + def _test__kill_process(self, pid, expected, exception_message=None, + kill_signal=signal.SIGKILL): self.proc.run_as_root = True if exception_message: exc = RuntimeError(exception_message) @@ -189,10 +192,10 @@ class TestAsyncProcess(base.BaseTestCase): exc = None with mock.patch.object(utils, 'execute', side_effect=exc) as mock_execute: - actual = self.proc._kill_process(pid) + actual = self.proc._kill_process(pid, kill_signal) self.assertEqual(expected, actual) - mock_execute.assert_called_with(['kill', '-9', pid], + mock_execute.assert_called_with(['kill', '-%d' % kill_signal, pid], run_as_root=self.proc.run_as_root) def test__kill_process_returns_true_for_valid_pid(self): @@ -204,11 +207,14 @@ class TestAsyncProcess(base.BaseTestCase): def test__kill_process_returns_false_for_execute_exception(self): self._test__kill_process('1', False, 'Invalid') - def test_stop_calls_kill(self): + def test_kill_process_with_different_signal(self): + self._test__kill_process('1', True, kill_signal=signal.SIGTERM) + + def test_stop_calls_kill_with_provided_signal_number(self): self.proc._kill_event = True with mock.patch.object(self.proc, '_kill') as mock_kill: - self.proc.stop() - mock_kill.assert_called_once_with() + self.proc.stop(kill_signal=signal.SIGTERM) + mock_kill.assert_called_once_with(signal.SIGTERM) def test_stop_raises_exception_if_already_started(self): with testtools.ExpectedException(async_process.AsyncProcessException): -- 2.45.2