Move failed instances to error state

On instance creation there is the possibility of
an instance raising. This would not cause the
instance to be moved to the error state. This
patch fixes that. lp885323

update 1: fixing exception handling
update 2: preserving the individual messages
update 3: rebase on master & fix spacing

(cherry picked from commit c04b431cd63f4d934f40dd1f62a9107ae6dfde90)

Change-Id: I7584b527e408c08014f1b6a8abda343f1e2aa3b8

2 files changed, 76 insertions, 14 deletions

diff --git a/nova/compute/manager.py b/nova/compute/manager.py
index 69dea2e6af..1196d98630 100644
--- a/nova/compute/manager.py
+++ b/nova/compute/manager.py
@@ -35,6 +35,7 @@ terminating it.
 
 """
 
+import contextlib
 import os
 import socket
 import sys
@@ -396,6 +397,23 @@ class ComputeManager(manager.SchedulerDependentManager):
                 self.network_api.deallocate_for_instance(context,
                                     instance)
 
+        def _cleanup():
+            with utils.save_and_reraise_exception():
+                self._instance_update(context,
+                                      instance_id,
+                                      vm_state=vm_states.ERROR)
+                if network_info is not None:
+                    _deallocate_network()
+
+        @contextlib.contextmanager
+        def _logging_error(instance_id, message):
+            try:
+                yield
+            except Exception as error:
+                with utils.save_and_reraise_exception():
+                    LOG.exception(_("Instance '%(instance_id)s' "
+                                   "failed %(message)s.") % locals())
+
         context = context.elevated()
         instance = self.db.instance_get(context, instance_id)
 
@@ -418,14 +436,17 @@ class ComputeManager(manager.SchedulerDependentManager):
         instance['admin_pass'] = kwargs.get('admin_password', None)
 
         is_vpn = instance['image_ref'] == str(FLAGS.vpn_image_id)
-        network_info = _make_network_info()
         try:
+            network_info = None
+            with _logging_error(instance_id, "network setup"):
+                network_info = _make_network_info()
+
             self._instance_update(context,
                                   instance_id,
                                   vm_state=vm_states.BUILDING,
                                   task_state=task_states.BLOCK_DEVICE_MAPPING)
-
-            block_device_info = _make_block_device_info()
+            with _logging_error(instance_id, "block device setup"):
+                block_device_info = _make_block_device_info()
 
             self._instance_update(context,
                                   instance_id,
@@ -433,16 +454,9 @@ class ComputeManager(manager.SchedulerDependentManager):
                                   task_state=task_states.SPAWNING)
 
             # TODO(vish) check to make sure the availability zone matches
-            try:
+            with _logging_error(instance_id, "failed to spawn"):
                 self.driver.spawn(context, instance,
                                   network_info, block_device_info)
-            except Exception as ex:  # pylint: disable=W0702
-                msg = _("Instance '%(instance_id)s' failed to spawn. Is "
-                        "virtualization enabled in the BIOS? Details: "
-                        "%(ex)s") % locals()
-                LOG.exception(msg)
-                _deallocate_network()
-                return
 
             current_power_state = self._get_power_state(context, instance)
             self._instance_update(context,
@@ -463,9 +477,8 @@ class ComputeManager(manager.SchedulerDependentManager):
             # deleted before it actually got created.  This should
             # be fixed once we have no-db-messaging
             pass
-        except:
-            with utils.save_and_reraise_exception():
-                _deallocate_network()
+        except Exception:
+            _cleanup()
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
     def run_instance(self, context, instance_id, **kwargs):
diff --git a/nova/tests/test_compute.py b/nova/tests/test_compute.py
index 67dbe5e2bd..d32c5df4b3 100644
--- a/nova/tests/test_compute.py
+++ b/nova/tests/test_compute.py
@@ -20,6 +20,8 @@
 Tests For Compute
 """
 
+import mox
+
 from nova import compute
 from nova.compute import instance_types
 from nova.compute import manager as compute_manager
@@ -38,6 +40,8 @@ from nova import rpc
 from nova import test
 from nova import utils
 from nova.notifier import test_notifier
+from nova.network.quantum import client as quantum_client
+
 
 LOG = logging.getLogger('nova.tests.compute')
 FLAGS = flags.FLAGS
@@ -467,6 +471,51 @@ class ComputeTestCase(test.TestCase):
                           instance_id)
         self.compute.terminate_instance(self.context, instance_id)
 
+    def test_instance_set_to_error_on_uncaught_exception(self):
+        """Test that instance is set to error state when exception is raised"""
+        instance_id = self._create_instance()
+
+        self.mox.StubOutWithMock(self.compute.network_api,
+                                 "allocate_for_instance")
+        self.compute.network_api.allocate_for_instance(mox.IgnoreArg(),
+                                                       mox.IgnoreArg(),
+                                                       requested_networks=None,
+                                                       vpn=False).\
+            AndRaise(quantum_client.QuantumServerException())
+
+        FLAGS.stub_network = False
+
+        self.mox.ReplayAll()
+
+        self.assertRaises(quantum_client.QuantumServerException,
+                          self.compute.run_instance,
+                          self.context,
+                          instance_id)
+
+        instances = db.instance_get_all(context.get_admin_context())
+        self.assertEqual(vm_states.ERROR, instances[0]['vm_state'])
+
+        FLAGS.stub_network = True
+        self.compute.terminate_instance(self.context, instance_id)
+
+    def test_network_is_deallocated_on_spawn_failure(self):
+        """When a spawn fails the network must be deallocated"""
+        instance_id = self._create_instance()
+
+        self.mox.StubOutWithMock(self.compute, "_setup_block_device_mapping")
+        self.compute._setup_block_device_mapping(mox.IgnoreArg(),
+                                                 mox.IgnoreArg()).\
+            AndRaise(rpc.common.RemoteError('', '', ''))
+
+        self.mox.ReplayAll()
+
+        self.assertRaises(rpc.common.RemoteError,
+                          self.compute.run_instance,
+                          self.context,
+                          instance_id)
+
+        self.compute.terminate_instance(self.context, instance_id)
+
     def test_lock(self):
         """ensure locked instance cannot be changed"""
         instance_id = self._create_instance()