Handle spawning error on unshelving

If spawning fails when unshelving, terminate the volumes' connections
with the node, and remove the node reference from the instance entry.

Co-Authored-By: Matt Riedemann <mriedem.os@gmail.com>

Conflicts:
      nova/compute/manager.py
      nova/tests/unit/compute/test_shelve.py

NOTE(mriedem): Conflicts are due to the following changes not in Ocata:

  I2394b0588bc7210efd16456af2fc12dc7071681c

  Id2c7b7b3b4abda8a3b878fdee6806bcfe096e12e

  I3a3caa4c566ecc132aa2699f8c7e5987bbcc863a

Closes-Bug: 1627694
Change-Id: I8cfb2280d956d452ccad1fc711bd814b7258147f
(cherry picked from commit dcdd2c9832c7c60fe9163cd744ca2b5acfe16bcc)
(cherry picked from commit 73c3e4969c108f2eefc77b57021d9b3e17afdc8e)
(cherry picked from commit 06946b7f8ad310e01eff895362ad0f3a164636f5)

2 files changed, 91 insertions, 0 deletions

diff --git a/nova/compute/manager.py b/nova/compute/manager.py
index 2e83f07ae6..8c83167d4a 100644
--- a/nova/compute/manager.py
+++ b/nova/compute/manager.py
@@ -4494,6 +4494,11 @@ class ComputeManager(manager.Manager):
             with excutils.save_and_reraise_exception():
                 LOG.exception(_LE('Instance failed to spawn'),
                               instance=instance)
+                # FIXME: Umm, shouldn't we be rolling back port bindings too?
+                self._terminate_volume_connections(context, instance, bdms)
+                # The reverts_task_state decorator on unshelve_instance will
+                # eventually save these updates.
+                self._nil_out_instance_obj_host_and_node(instance)
 
         if image:
             instance.image_ref = shelved_image_ref
diff --git a/nova/tests/unit/compute/test_shelve.py b/nova/tests/unit/compute/test_shelve.py
index 2d21f092f8..8c74faec2c 100644
--- a/nova/tests/unit/compute/test_shelve.py
+++ b/nova/tests/unit/compute/test_shelve.py
@@ -397,6 +397,92 @@ class ShelveComputeManagerTestCase(test_compute.BaseTestCase):
             self.compute.unshelve_instance(self.context, instance, image=None,
                     filter_properties=filter_properties, node=node)
 
+    @mock.patch('nova.objects.BlockDeviceMappingList.get_by_instance_uuid')
+    @mock.patch('nova.compute.utils.notify_about_instance_action')
+    @mock.patch.object(nova.compute.resource_tracker.ResourceTracker,
+                       'instance_claim')
+    @mock.patch('nova.network.neutronv2.api.API.'
+                'setup_instance_network_on_host')
+    @mock.patch.object(nova.virt.fake.SmallFakeDriver, 'spawn',
+                       side_effect=test.TestingException('oops!'))
+    @mock.patch.object(nova.compute.manager.ComputeManager,
+                       '_prep_block_device', return_value='fake_bdm')
+    @mock.patch.object(nova.compute.manager.ComputeManager,
+                       '_notify_about_instance_usage')
+    @mock.patch('nova.utils.get_image_from_system_metadata')
+    @mock.patch.object(nova.compute.manager.ComputeManager,
+                       '_terminate_volume_connections')
+    def test_unshelve_spawn_fails_cleanup_volume_connections(
+            self, mock_terminate_volume_connections, mock_image_meta,
+            mock_notify_instance_usage, mock_prep_block_device, mock_spawn,
+            mock_setup_network, mock_instance_claim,
+            mock_notify_instance_action, mock_get_bdms):
+        """Tests error handling when a instance fails to unshelve and makes
+        sure that volume connections are cleaned up from the host
+        and that the host/node values are unset on the instance.
+        """
+        mock_bdms = mock.Mock()
+        mock_get_bdms.return_value = mock_bdms
+        instance = self._create_fake_instance_obj()
+        node = test_compute.NODENAME
+        limits = {}
+        filter_properties = {'limits': limits}
+        instance.task_state = task_states.UNSHELVING
+        instance.save()
+        image_meta = {'properties': {'base_image_ref': uuids.image_id}}
+        mock_image_meta.return_value = image_meta
+
+        tracking = {'last_state': instance.task_state}
+
+        def fake_claim(context, instance, node, limits):
+            instance.host = self.compute.host
+            instance.node = node
+            requests = objects.InstancePCIRequests(requests=[])
+            return claims.Claim(context, instance, node,
+                                self.rt, _fake_resources(),
+                                requests, limits=limits)
+        mock_instance_claim.side_effect = fake_claim
+
+        def check_save(expected_task_state=None):
+            if tracking['last_state'] == task_states.UNSHELVING:
+                # This is before we've failed.
+                self.assertEqual(task_states.SPAWNING, instance.task_state)
+                tracking['last_state'] = instance.task_state
+            elif tracking['last_state'] == task_states.SPAWNING:
+                # This is after we've failed.
+                self.assertIsNone(instance.host)
+                self.assertIsNone(instance.node)
+                self.assertIsNone(instance.task_state)
+                tracking['last_state'] = instance.task_state
+            else:
+                self.fail('Unexpected save!')
+
+        with mock.patch.object(instance, 'save') as mock_save:
+            mock_save.side_effect = check_save
+            self.assertRaises(test.TestingException,
+                              self.compute.unshelve_instance,
+                              self.context, instance, image=None,
+                              filter_properties=filter_properties, node=node)
+
+        mock_notify_instance_action.assert_called_once_with(
+            self.context, instance, 'fake-mini', action='unshelve',
+            phase='start')
+        mock_notify_instance_usage.assert_called_once_with(
+            self.context, instance, 'unshelve.start')
+        mock_prep_block_device.assert_called_once_with(
+            self.context, instance, mock_bdms, do_check_attach=False)
+        mock_setup_network.assert_called_once_with(self.context, instance,
+                                                   self.compute.host)
+        mock_instance_claim.assert_called_once_with(self.context, instance,
+                                                    test_compute.NODENAME,
+                                                    limits)
+        mock_spawn.assert_called_once_with(
+            self.context, instance, test.MatchType(objects.ImageMeta),
+            injected_files=[], admin_password=None,
+            network_info=[], block_device_info='fake_bdm')
+        mock_terminate_volume_connections.assert_called_once_with(
+            self.context, instance, mock_bdms)
+
     @mock.patch.object(objects.InstanceList, 'get_by_filters')
     def test_shelved_poll_none_offloaded(self, mock_get_by_filters):
         # Test instances are not offloaded when shelved_offload_time is -1