Merge "Check shellinabox process during stopping console"

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)
+        os.kill(console_pid, signal.SIGTERM)
-        utils.execute('kill', str(console_pid), check_exit_code=[0, 99])
+    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)

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,
+        mock_kill.assert_called_once_with(mock_pid.return_value,
-                                             check_exit_code=[0, 99])
+                                          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_pid.return_value = 12345
-        mock_execute.side_effect = iter([processutils.ProcessExecutionError()])
+        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,
+        mock_kill.assert_called_once_with(mock_pid.return_value,
-                                             check_exit_code=[0, 99])
+                                          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'])