Fix rescue volume-based instance

As of now, when attempting to rescue a volume-based instance
using an image without the hw_rescue_device and/or hw_rescue_bus
properties set, the rescue api call fails (as non-stable rescue
for volume-based instances are not supported) leaving the instance
in error state.

This change checks for hw_rescue_device/hw_rescue_bus image
properties before attempting to rescue and if the property
is not set, then fail with proper error message, without changing
instance state.

Related-Bug: #1978958
Closes-Bug: #1926601
Change-Id: Id4c8c5f3b32985ac7d3d7c833b82e0876f7367c1
(cherry picked from commit 6eed55bf55469f4ceaa7d4d4eb1be635e14bc73b)

4 files changed, 220 insertions, 11 deletions

diff --git a/nova/compute/api.py b/nova/compute/api.py
index c06fefdd3c..eebfb2cbe4 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -4697,6 +4697,7 @@ class API:
                allow_bfv_rescue=False):
         """Rescue the given instance."""
 
+        image_meta = None
         if rescue_image_ref:
             try:
                 image_meta = image_meta_obj.ImageMeta.from_image_ref(
@@ -4717,6 +4718,8 @@ class API:
                             "image properties set")
                 raise exception.UnsupportedRescueImage(
                     image=rescue_image_ref)
+        else:
+            image_meta = instance.image_meta
 
         bdms = objects.BlockDeviceMappingList.get_by_instance_uuid(
                     context, instance.uuid)
@@ -4725,6 +4728,9 @@ class API:
         volume_backed = compute_utils.is_volume_backed_instance(
             context, instance, bdms)
 
+        allow_bfv_rescue &= 'hw_rescue_bus' in image_meta.properties and \
+            'hw_rescue_device' in image_meta.properties
+
         if volume_backed and allow_bfv_rescue:
             cn = objects.ComputeNode.get_by_host_and_nodename(
                 context, instance.host, instance.node)
diff --git a/nova/tests/functional/test_server_rescue.py b/nova/tests/functional/test_server_rescue.py
index fa96c10344..8f5b912943 100644
--- a/nova/tests/functional/test_server_rescue.py
+++ b/nova/tests/functional/test_server_rescue.py
@@ -10,6 +10,10 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
+import datetime
+
+from oslo_utils.fixture import uuidsentinel as uuids
+
 from nova.tests import fixtures as nova_fixtures
 from nova.tests.functional.api import client
 from nova.tests.functional import integrated_helpers
@@ -23,7 +27,37 @@ class BFVRescue(integrated_helpers.ProviderUsageBaseTestCase):
         self.useFixture(nova_fixtures.CinderFixture(self))
         self._start_compute(host='host1')
 
-    def _create_bfv_server(self):
+    def _create_image(self, metadata=None):
+        image = {
+            'id': uuids.stable_rescue_image,
+            'name': 'fake-image-rescue-property',
+            'created_at': datetime.datetime(2011, 1, 1, 1, 2, 3),
+            'updated_at': datetime.datetime(2011, 1, 1, 1, 2, 3),
+            'deleted_at': None,
+            'deleted': False,
+            'status': 'active',
+            'is_public': False,
+            'container_format': 'raw',
+            'disk_format': 'raw',
+            'size': '25165824',
+            'min_ram': 0,
+            'min_disk': 0,
+            'protected': False,
+            'visibility': 'public',
+            'tags': ['tag1', 'tag2'],
+            'properties': {
+                'kernel_id': 'nokernel',
+                'ramdisk_id': 'nokernel',
+                'hw_rescue_device': 'disk',
+                'hw_rescue_bus': 'scsi',
+            },
+        }
+        if metadata:
+            image['properties'].update(metadata)
+        return self.glance.create(None, image)
+
+    def _create_bfv_server(self, metadata=None):
+        image = self._create_image(metadata=metadata)
         server_request = self._build_server(networks=[])
         server_request.pop('imageRef')
         server_request['block_device_mapping_v2'] = [{
@@ -33,7 +67,7 @@ class BFVRescue(integrated_helpers.ProviderUsageBaseTestCase):
             'destination_type': 'volume'}]
         server = self.api.post_server({'server': server_request})
         self._wait_for_state_change(server, 'ACTIVE')
-        return server
+        return server, image
 
 
 class DisallowBFVRescuev286(BFVRescue):
@@ -43,10 +77,10 @@ class DisallowBFVRescuev286(BFVRescue):
     microversion = '2.86'
 
     def test_bfv_rescue_not_supported(self):
-        server = self._create_bfv_server()
+        server, image = self._create_bfv_server()
         ex = self.assertRaises(client.OpenStackApiException,
             self.api.post_server_action, server['id'], {'rescue': {
-            'rescue_image_ref': '155d900f-4e14-4e4c-a73d-069cbf4541e6'}})
+            'rescue_image_ref': image['id']}})
         self.assertEqual(400, ex.response.status_code)
         self.assertIn('Cannot rescue a volume-backed instance',
                       ex.response.text)
@@ -60,10 +94,10 @@ class DisallowBFVRescuev286WithTrait(BFVRescue):
     microversion = '2.86'
 
     def test_bfv_rescue_not_supported(self):
-        server = self._create_bfv_server()
+        server, image = self._create_bfv_server()
         ex = self.assertRaises(client.OpenStackApiException,
             self.api.post_server_action, server['id'], {'rescue': {
-            'rescue_image_ref': '155d900f-4e14-4e4c-a73d-069cbf4541e6'}})
+            'rescue_image_ref': image['id']}})
         self.assertEqual(400, ex.response.status_code)
         self.assertIn('Cannot rescue a volume-backed instance',
                       ex.response.text)
@@ -77,10 +111,10 @@ class DisallowBFVRescuev287WithoutTrait(BFVRescue):
     microversion = '2.87'
 
     def test_bfv_rescue_not_supported(self):
-        server = self._create_bfv_server()
+        server, image = self._create_bfv_server()
         ex = self.assertRaises(client.OpenStackApiException,
             self.api.post_server_action, server['id'], {'rescue': {
-            'rescue_image_ref': '155d900f-4e14-4e4c-a73d-069cbf4541e6'}})
+            'rescue_image_ref': image['id']}})
         self.assertEqual(400, ex.response.status_code)
         self.assertIn('Host unable to rescue a volume-backed instance',
                       ex.response.text)
@@ -94,7 +128,41 @@ class AllowBFVRescuev287WithTrait(BFVRescue):
     microversion = '2.87'
 
     def test_bfv_rescue_supported(self):
-        server = self._create_bfv_server()
+        server, image = self._create_bfv_server()
         self.api.post_server_action(server['id'], {'rescue': {
+            'rescue_image_ref': image['id']}})
+        self._wait_for_state_change(server, 'RESCUE')
+
+
+class DisallowBFVRescuev287WithoutRescueImageProperties(BFVRescue):
+    """Asserts that BFV rescue requests fail with microversion 2.87 (or later)
+    when the required hw_rescue_device and hw_rescue_bus image properties
+    are not set on the image.
+    """
+    compute_driver = 'fake.MediumFakeDriver'
+    microversion = '2.87'
+
+    def test_bfv_rescue_failed(self):
+        server, image = self._create_bfv_server()
+        # try rescue without hw_rescue_device and hw_rescue_bus properties set
+        ex = self.assertRaises(client.OpenStackApiException,
+            self.api.post_server_action, server['id'], {'rescue': {
             'rescue_image_ref': '155d900f-4e14-4e4c-a73d-069cbf4541e6'}})
+        self.assertEqual(400, ex.response.status_code)
+        self.assertIn('Cannot rescue a volume-backed instance',
+                      ex.response.text)
+
+
+class AllowBFVRescuev287WithRescueImageProperties(BFVRescue):
+    """Asserts that BFV rescue requests pass with microversion 2.87 (or later)
+    when the required hw_rescue_device and hw_rescue_bus image properties
+    are set on the image.
+    """
+    compute_driver = 'fake.RescueBFVDriver'
+    microversion = '2.87'
+
+    def test_bfv_rescue_done(self):
+        server, image = self._create_bfv_server()
+        self.api.post_server_action(server['id'], {'rescue': {
+            'rescue_image_ref': image['id']}})
         self._wait_for_state_change(server, 'RESCUE')
diff --git a/nova/tests/unit/compute/test_api.py b/nova/tests/unit/compute/test_api.py
index ca72474a4c..984b7a033d 100644
--- a/nova/tests/unit/compute/test_api.py
+++ b/nova/tests/unit/compute/test_api.py
@@ -5790,7 +5790,10 @@ class _ComputeAPIUnitTestMixIn(object):
                     destination_type='volume', volume_type=None,
                     snapshot_id=None, volume_id=uuids.volume_id,
                     volume_size=None)])
-        rescue_image_meta_obj = image_meta_obj.ImageMeta.from_dict({})
+        rescue_image_meta_obj = image_meta_obj.ImageMeta.from_dict({
+            'properties': {'hw_rescue_device': 'disk',
+                           'hw_rescue_bus': 'scsi'}
+        })
 
         with test.nested(
             mock.patch.object(self.compute_api.placementclient,
@@ -5842,6 +5845,7 @@ class _ComputeAPIUnitTestMixIn(object):
             # Assert that the instance task state as set in the compute API
             self.assertEqual(task_states.RESCUING, instance.task_state)
 
+    @mock.patch('nova.objects.instance.Instance.image_meta')
     @mock.patch('nova.objects.compute_node.ComputeNode'
                 '.get_by_host_and_nodename')
     @mock.patch('nova.compute.utils.is_volume_backed_instance',
@@ -5850,7 +5854,8 @@ class _ComputeAPIUnitTestMixIn(object):
                 '.get_by_instance_uuid')
     def test_rescue_bfv_without_required_trait(self, mock_get_bdms,
                                                mock_is_volume_backed,
-                                               mock_get_cn):
+                                               mock_get_cn,
+                                               mock_image_meta):
         instance = self._create_instance_obj()
         bdms = objects.BlockDeviceMappingList(objects=[
                 objects.BlockDeviceMapping(
@@ -5858,6 +5863,12 @@ class _ComputeAPIUnitTestMixIn(object):
                     destination_type='volume', volume_type=None,
                     snapshot_id=None, volume_id=uuids.volume_id,
                     volume_size=None)])
+
+        instance.image_meta = image_meta_obj.ImageMeta.from_dict({
+            'properties': {'hw_rescue_device': 'disk',
+                           'hw_rescue_bus': 'scsi'}
+        })
+
         with test.nested(
             mock.patch.object(self.compute_api.placementclient,
                               'get_provider_traits'),
@@ -5895,6 +5906,124 @@ class _ComputeAPIUnitTestMixIn(object):
             mock_get_traits.assert_called_once_with(
                 self.context, uuids.cn)
 
+    @mock.patch('nova.objects.image_meta.ImageMeta.from_image_ref')
+    @mock.patch('nova.objects.compute_node.ComputeNode'
+                '.get_by_host_and_nodename')
+    @mock.patch('nova.compute.utils.is_volume_backed_instance',
+                return_value=True)
+    @mock.patch('nova.objects.block_device.BlockDeviceMappingList'
+                '.get_by_instance_uuid')
+    def test_rescue_bfv_with_required_image_properties(
+            self, mock_get_bdms, mock_is_volume_backed, mock_get_cn,
+            mock_image_meta_obj_from_ref):
+        instance = self._create_instance_obj()
+        bdms = objects.BlockDeviceMappingList(objects=[
+            objects.BlockDeviceMapping(
+                boot_index=0, image_id=uuids.image_id, source_type='image',
+                destination_type='volume', volume_type=None,
+                snapshot_id=None, volume_id=uuids.volume_id,
+                volume_size=None)])
+        rescue_image_meta_obj = image_meta_obj.ImageMeta.from_dict({
+            'properties': {'hw_rescue_device': 'disk',
+                           'hw_rescue_bus': 'scsi'}
+        })
+
+        with test.nested(
+            mock.patch.object(self.compute_api.placementclient,
+                              'get_provider_traits'),
+            mock.patch.object(self.compute_api.volume_api, 'get'),
+            mock.patch.object(self.compute_api.volume_api, 'check_attached'),
+            mock.patch.object(instance, 'save'),
+            mock.patch.object(self.compute_api, '_record_action_start'),
+            mock.patch.object(self.compute_api.compute_rpcapi,
+                              'rescue_instance')
+        ) as (
+                mock_get_traits, mock_get_volume, mock_check_attached,
+                mock_instance_save, mock_record_start, mock_rpcapi_rescue
+        ):
+            # Mock out the returned compute node, image_meta, bdms and volume
+            mock_image_meta_obj_from_ref.return_value = rescue_image_meta_obj
+            mock_get_bdms.return_value = bdms
+            mock_get_volume.return_value = mock.sentinel.volume
+            mock_get_cn.return_value = mock.Mock(uuid=uuids.cn)
+
+            # Ensure the required trait is returned, allowing BFV rescue
+            mock_trait_info = mock.Mock(traits=[ot.COMPUTE_RESCUE_BFV])
+            mock_get_traits.return_value = mock_trait_info
+
+            # Try to rescue the instance
+            self.compute_api.rescue(self.context, instance,
+                                    rescue_image_ref=uuids.rescue_image_id,
+                                    allow_bfv_rescue=True)
+
+            # Assert all of the calls made in the compute API
+            mock_get_bdms.assert_called_once_with(self.context, instance.uuid)
+            mock_get_volume.assert_called_once_with(
+                self.context, uuids.volume_id)
+            mock_check_attached.assert_called_once_with(
+                self.context, mock.sentinel.volume)
+            mock_is_volume_backed.assert_called_once_with(
+                self.context, instance, bdms)
+            mock_get_cn.assert_called_once_with(
+                self.context, instance.host, instance.node)
+            mock_get_traits.assert_called_once_with(self.context, uuids.cn)
+            mock_instance_save.assert_called_once_with(
+                expected_task_state=[None])
+            mock_record_start.assert_called_once_with(
+                self.context, instance, instance_actions.RESCUE)
+            mock_rpcapi_rescue.assert_called_once_with(
+                self.context, instance=instance, rescue_password=None,
+                rescue_image_ref=uuids.rescue_image_id, clean_shutdown=True)
+
+            # Assert that the instance task state as set in the compute API
+            self.assertEqual(task_states.RESCUING, instance.task_state)
+
+    @mock.patch('nova.objects.image_meta.ImageMeta.from_image_ref')
+    @mock.patch('nova.compute.utils.is_volume_backed_instance',
+                return_value=True)
+    @mock.patch('nova.objects.block_device.BlockDeviceMappingList'
+                '.get_by_instance_uuid')
+    def test_rescue_bfv_without_required_image_properties(
+            self, mock_get_bdms, mock_is_volume_backed,
+            mock_image_meta_obj_from_ref):
+        instance = self._create_instance_obj()
+        bdms = objects.BlockDeviceMappingList(objects=[
+            objects.BlockDeviceMapping(
+                boot_index=0, image_id=uuids.image_id, source_type='image',
+                destination_type='volume', volume_type=None,
+                snapshot_id=None, volume_id=uuids.volume_id,
+                volume_size=None)])
+        rescue_image_meta_obj = image_meta_obj.ImageMeta.from_dict({
+            'properties': {}
+        })
+
+        with test.nested(
+            mock.patch.object(self.compute_api.volume_api, 'get'),
+            mock.patch.object(self.compute_api.volume_api, 'check_attached'),
+        ) as (
+            mock_get_volume, mock_check_attached
+        ):
+            # Mock out the returned bdms, volume and image_meta
+            mock_get_bdms.return_value = bdms
+            mock_get_volume.return_value = mock.sentinel.volume
+            mock_image_meta_obj_from_ref.return_value = rescue_image_meta_obj
+
+            # Assert that any attempt to rescue a bfv instance on a compute
+            # node that does not report the COMPUTE_RESCUE_BFV trait fails and
+            # raises InstanceNotRescuable
+            self.assertRaises(exception.InstanceNotRescuable,
+                              self.compute_api.rescue, self.context, instance,
+                              rescue_image_ref=None, allow_bfv_rescue=True)
+
+            # Assert the calls made in the compute API prior to the failure
+            mock_get_bdms.assert_called_once_with(self.context, instance.uuid)
+            mock_get_volume.assert_called_once_with(
+                self.context, uuids.volume_id)
+            mock_check_attached.assert_called_once_with(
+                self.context, mock.sentinel.volume)
+            mock_is_volume_backed.assert_called_once_with(
+                self.context, instance, bdms)
+
     @mock.patch('nova.compute.utils.is_volume_backed_instance',
                 return_value=True)
     @mock.patch('nova.objects.block_device.BlockDeviceMappingList'
diff --git a/releasenotes/notes/rescue-volume-based-instance-c6e3fba236d90be7.yaml b/releasenotes/notes/rescue-volume-based-instance-c6e3fba236d90be7.yaml
new file mode 100644
index 0000000000..7e80059b80
--- /dev/null
+++ b/releasenotes/notes/rescue-volume-based-instance-c6e3fba236d90be7.yaml
@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    Fix rescuing volume based instance by adding a check for 'hw_rescue_disk'
+    and 'hw_rescue_device' properties in image metadata before attempting
+    to rescue instance.