From d00a848a735f98b028f5930798ee69ef205c8e2e Mon Sep 17 00:00:00 2001 From: Rajesh Tailor Date: Wed, 10 Aug 2022 18:15:04 +0530 Subject: 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) --- nova/compute/api.py | 6 + nova/tests/functional/test_server_rescue.py | 86 +++++++++++-- nova/tests/unit/compute/test_api.py | 133 ++++++++++++++++++++- ...cue-volume-based-instance-c6e3fba236d90be7.yaml | 6 + 4 files changed, 220 insertions(+), 11 deletions(-) create mode 100644 releasenotes/notes/rescue-volume-based-instance-c6e3fba236d90be7.yaml 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. -- cgit v1.2.1