From 24188760c8d261e2f9425900f7deb73ed4a122b3 Mon Sep 17 00:00:00 2001 From: Zhenguo Niu Date: Thu, 12 Nov 2015 14:24:14 +0800 Subject: [PATCH] Check shellinabox process during stopping console Currently we assume shellinabox is running as long as pid file exists, which will cause error while executing kill command, this patch update to use os.kill() and if errno is ESRCH, just log a warning message. Change-Id: Ib8968418a1835a4131f2f22fb3e4df5ecb9b0dc5 Closes-Bug: #1513690 --- ironic/drivers/modules/console_utils.py | 18 ++++-- .../drivers/modules/test_console_utils.py | 59 +++++++++++-------- 2 files changed, 45 insertions(+), 32 deletions(-) diff --git a/ironic/drivers/modules/console_utils.py b/ironic/drivers/modules/console_utils.py index b4172a9622..bcf8fe0cbb 100644 --- a/ironic/drivers/modules/console_utils.py +++ b/ironic/drivers/modules/console_utils.py @@ -19,7 +19,9 @@ Ironic console utilities. """ +import errno import os +import signal import subprocess import time @@ -122,8 +124,16 @@ def _stop_console(node_uuid): try: console_pid = _get_console_pid(node_uuid) - # Allow exitcode 99 (RC_UNAUTHORIZED) - utils.execute('kill', str(console_pid), check_exit_code=[0, 99]) + os.kill(console_pid, signal.SIGTERM) + except OSError as exc: + if exc.errno != errno.ESRCH: + msg = (_("Could not stop the console for node '%(node)s'. " + "Reason: %(err)s.") % {'node': node_uuid, 'err': exc}) + raise exception.ConsoleError(message=msg) + else: + LOG.warning(_LW("Console process for node %s is not running " + "but pid file exists while trying to stop " + "shellinabox console."), node_uuid) finally: utils.unlink_without_raise(_get_console_pid_file(node_uuid)) @@ -261,7 +271,3 @@ def stop_shellinabox_console(node_uuid): except exception.NoConsolePid: LOG.warning(_LW("No console pid found for node %s while trying to " "stop shellinabox console."), node_uuid) - except processutils.ProcessExecutionError as exc: - msg = (_("Could not stop the console for node '%(node)s'. " - "Reason: %(err)s.") % {'node': node_uuid, 'err': exc}) - raise exception.ConsoleError(message=msg) diff --git a/ironic/tests/unit/drivers/modules/test_console_utils.py b/ironic/tests/unit/drivers/modules/test_console_utils.py index 23762e9f85..bddb421621 100644 --- a/ironic/tests/unit/drivers/modules/test_console_utils.py +++ b/ironic/tests/unit/drivers/modules/test_console_utils.py @@ -17,14 +17,15 @@ """Test class for console_utils driver module.""" +import errno import os import random +import signal import string import subprocess import tempfile import mock -from oslo_concurrency import processutils from oslo_config import cfg from oslo_utils import netutils @@ -136,23 +137,23 @@ class ConsoleUtilsTestCase(db_base.DbTestCase): self.info['uuid']) @mock.patch.object(utils, 'unlink_without_raise', autospec=True) - @mock.patch.object(utils, 'execute', autospec=True) + @mock.patch.object(os, 'kill', autospec=True) @mock.patch.object(console_utils, '_get_console_pid', autospec=True) - def test__stop_console(self, mock_pid, mock_execute, mock_unlink): + def test__stop_console(self, mock_pid, mock_kill, mock_unlink): pid_file = console_utils._get_console_pid_file(self.info['uuid']) - mock_pid.return_value = '12345' + mock_pid.return_value = 12345 console_utils._stop_console(self.info['uuid']) mock_pid.assert_called_once_with(self.info['uuid']) - mock_execute.assert_called_once_with('kill', mock_pid.return_value, - check_exit_code=[0, 99]) + mock_kill.assert_called_once_with(mock_pid.return_value, + signal.SIGTERM) mock_unlink.assert_called_once_with(pid_file) @mock.patch.object(utils, 'unlink_without_raise', autospec=True) - @mock.patch.object(utils, 'execute', autospec=True) + @mock.patch.object(os, 'kill', autospec=True) @mock.patch.object(console_utils, '_get_console_pid', autospec=True) - def test__stop_console_nopid(self, mock_pid, mock_execute, mock_unlink): + def test__stop_console_nopid(self, mock_pid, mock_kill, mock_unlink): pid_file = console_utils._get_console_pid_file(self.info['uuid']) mock_pid.side_effect = iter( [exception.NoConsolePid(pid_path="/tmp/blah")]) @@ -162,24 +163,40 @@ class ConsoleUtilsTestCase(db_base.DbTestCase): self.info['uuid']) mock_pid.assert_called_once_with(self.info['uuid']) - self.assertFalse(mock_execute.called) + self.assertFalse(mock_kill.called) mock_unlink.assert_called_once_with(pid_file) @mock.patch.object(utils, 'unlink_without_raise', autospec=True) - @mock.patch.object(utils, 'execute', autospec=True) + @mock.patch.object(os, 'kill', autospec=True) @mock.patch.object(console_utils, '_get_console_pid', autospec=True) - def test__stop_console_nokill(self, mock_pid, mock_execute, mock_unlink): + def test__stop_console_shellinabox_not_running(self, mock_pid, + mock_kill, mock_unlink): pid_file = console_utils._get_console_pid_file(self.info['uuid']) - mock_pid.return_value = '12345' - mock_execute.side_effect = iter([processutils.ProcessExecutionError()]) + mock_pid.return_value = 12345 + mock_kill.side_effect = OSError(errno.ESRCH, 'message') - self.assertRaises(processutils.ProcessExecutionError, + console_utils._stop_console(self.info['uuid']) + + mock_pid.assert_called_once_with(self.info['uuid']) + mock_kill.assert_called_once_with(mock_pid.return_value, + signal.SIGTERM) + mock_unlink.assert_called_once_with(pid_file) + + @mock.patch.object(utils, 'unlink_without_raise', autospec=True) + @mock.patch.object(os, 'kill', autospec=True) + @mock.patch.object(console_utils, '_get_console_pid', autospec=True) + def test__stop_console_exception(self, mock_pid, mock_kill, mock_unlink): + pid_file = console_utils._get_console_pid_file(self.info['uuid']) + mock_pid.return_value = 12345 + mock_kill.side_effect = OSError(2, 'message') + + self.assertRaises(exception.ConsoleError, console_utils._stop_console, self.info['uuid']) mock_pid.assert_called_once_with(self.info['uuid']) - mock_execute.assert_called_once_with('kill', mock_pid.return_value, - check_exit_code=[0, 99]) + mock_kill.assert_called_once_with(mock_pid.return_value, + signal.SIGTERM) mock_unlink.assert_called_once_with(pid_file) def _get_shellinabox_console(self, scheme): @@ -336,13 +353,3 @@ class ConsoleUtilsTestCase(db_base.DbTestCase): console_utils.stop_shellinabox_console(self.info['uuid']) mock_stop.assert_called_once_with(self.info['uuid']) - - @mock.patch.object(console_utils, '_stop_console', autospec=True) - def test_stop_shellinabox_console_fail_nokill(self, mock_stop): - mock_stop.side_effect = iter([processutils.ProcessExecutionError()]) - - self.assertRaises(exception.ConsoleError, - console_utils.stop_shellinabox_console, - self.info['uuid']) - - mock_stop.assert_called_once_with(self.info['uuid'])