From bcbeae2c605f4ab4ad805dddccac802928a180b6 Mon Sep 17 00:00:00 2001 From: sunhao Date: Wed, 10 Jun 2020 10:26:49 +0800 Subject: [PATCH] Add checks for volume status when rebuilding When rebuilding, we should only allow detaching the volume with 'in-use' status, volume in status such as 'retyping' should not allowed. Change-Id: I7f93cfd18f948134c9cb429dea55740d2cf97994 Closes-Bug: #1489304 (cherry picked from commit 10e9a9b9fc62a3cf72c3717e3621ed95d3cf5519) --- nova/api/openstack/compute/servers.py | 1 + nova/compute/api.py | 22 +++++-- .../openstack/compute/test_server_actions.py | 15 +++++ nova/tests/unit/compute/test_compute_api.py | 64 +++++++++++++++++++ 4 files changed, 98 insertions(+), 4 deletions(-) diff --git a/nova/api/openstack/compute/servers.py b/nova/api/openstack/compute/servers.py index d70bd8d45f56..985c9475e93a 100644 --- a/nova/api/openstack/compute/servers.py +++ b/nova/api/openstack/compute/servers.py @@ -1130,6 +1130,7 @@ class ServersController(wsgi.Controller): exception.ImageUnacceptable, exception.InvalidMetadata, exception.InvalidArchitectureName, + exception.InvalidVolume, ) as error: raise exc.HTTPBadRequest(explanation=error.format_message()) except INVALID_FLAVOR_IMAGE_EXCEPTIONS as error: diff --git a/nova/compute/api.py b/nova/compute/api.py index d12523af597f..2519e18f7a52 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -3438,6 +3438,12 @@ class API(base.Base): self._checks_for_create_and_rebuild(context, image_id, image, flavor, metadata, files_to_inject, root_bdm) + # Check the state of the volume. If it is not in-use, an exception + # will occur when creating attachment during reconstruction, + # resulting in the failure of reconstruction and the instance + # turning into an error state. + self._check_volume_status(context, bdms) + # NOTE(sean-k-mooney): When we rebuild with a new image we need to # validate that the NUMA topology does not change as we do a NOOP claim # in resource tracker. As such we cannot allow the resource usage or @@ -3537,6 +3543,17 @@ class API(base.Base): preserve_ephemeral=preserve_ephemeral, host=host, request_spec=request_spec) + def _check_volume_status(self, context, bdms): + """Check whether the status of the volume is "in-use". + + :param context: A context.RequestContext + :param bdms: BlockDeviceMappingList of BDMs for the instance + """ + for bdm in bdms: + if bdm.volume_id: + vol = self.volume_api.get(context, bdm.volume_id) + self.volume_api.check_attached(context, vol) + @staticmethod def _validate_numa_rebuild(instance, image, flavor): """validates that the NUMA constraints do not change on rebuild. @@ -4207,10 +4224,7 @@ class API(base.Base): bdms = objects.BlockDeviceMappingList.get_by_instance_uuid( context, instance.uuid) - for bdm in bdms: - if bdm.volume_id: - vol = self.volume_api.get(context, bdm.volume_id) - self.volume_api.check_attached(context, vol) + self._check_volume_status(context, bdms) volume_backed = compute_utils.is_volume_backed_instance( context, instance, bdms) diff --git a/nova/tests/unit/api/openstack/compute/test_server_actions.py b/nova/tests/unit/api/openstack/compute/test_server_actions.py index 4a25a3cd669e..e707b8e18184 100644 --- a/nova/tests/unit/api/openstack/compute/test_server_actions.py +++ b/nova/tests/unit/api/openstack/compute/test_server_actions.py @@ -672,6 +672,21 @@ class ServerActionsControllerTestV21(test.TestCase): self.controller._action_rebuild, self.req, FAKE_UUID, body=body) + @mock.patch.object(compute_api.API, 'rebuild') + def test_rebuild_raise_invalid_volume_exc(self, mock_rebuild): + """Make sure that we can't rebuild with an InvalidVolume exception.""" + body = { + "rebuild": { + "imageRef": self._image_href, + }, + } + + mock_rebuild.side_effect = exception.InvalidVolume('error') + + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller._action_rebuild, + self.req, FAKE_UUID, body=body) + def test_resize_server(self): body = dict(resize=dict(flavorRef="http://localhost/3")) diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index 54bb6c88cadc..c3a1b37907a3 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -3669,6 +3669,70 @@ class _ComputeAPIUnitTestMixIn(object): "new password") self.assertIsNone(instance.task_state) + @mock.patch.object(objects.RequestSpec, 'get_by_instance_uuid') + @mock.patch.object(objects.Instance, 'save') + @mock.patch.object(objects.Instance, 'get_flavor') + @mock.patch.object(objects.BlockDeviceMappingList, 'get_by_instance_uuid') + @mock.patch.object(compute_api.API, '_get_image') + @mock.patch.object(compute_api.API, '_check_image_arch') + @mock.patch.object(compute_api.API, '_check_auto_disk_config') + @mock.patch.object(compute_api.API, '_checks_for_create_and_rebuild') + @mock.patch.object(compute_api.API, '_record_action_start') + def test_rebuild_with_invalid_volume(self, _record_action_start, + _checks_for_create_and_rebuild, _check_auto_disk_config, + _check_image_arch, mock_get_image, + mock_get_bdms, get_flavor, + instance_save, req_spec_get_by_inst_uuid): + """Test a negative scenario where the instance has an + invalid volume. + """ + instance = fake_instance.fake_instance_obj( + self.context, vm_state=vm_states.ACTIVE, cell_name='fake-cell', + launched_at=timeutils.utcnow(), + system_metadata={}, image_ref='foo', + expected_attrs=['system_metadata']) + + bdms = objects.BlockDeviceMappingList(objects=[ + objects.BlockDeviceMapping( + boot_index=None, image_id=None, + source_type='volume', destination_type='volume', + volume_type=None, snapshot_id=None, + volume_id=uuids.volume_id, volume_size=None)]) + mock_get_bdms.return_value = bdms + + get_flavor.return_value = test_flavor.fake_flavor + flavor = instance.get_flavor() + + image_href = 'foo' + image = { + "min_ram": 10, "min_disk": 1, + "properties": { + 'architecture': fields_obj.Architecture.X86_64}} + mock_get_image.return_value = (None, image) + + fake_spec = objects.RequestSpec() + req_spec_get_by_inst_uuid.return_value = fake_spec + + fake_volume = {'id': uuids.volume_id, 'status': 'retyping'} + with mock.patch.object(self.compute_api.volume_api, 'get', + return_value=fake_volume) as mock_get_volume: + self.assertRaises(exception.InvalidVolume, + self.compute_api.rebuild, + self.context, + instance, + image_href, + "new password") + self.assertIsNone(instance.task_state) + mock_get_bdms.assert_called_once_with(self.context, + instance.uuid) + mock_get_volume.assert_called_once_with(self.context, + uuids.volume_id) + _check_auto_disk_config.assert_called_once_with( + image=image, auto_disk_config=None) + _check_image_arch.assert_called_once_with(image=image) + _checks_for_create_and_rebuild.assert_called_once_with( + self.context, None, image, flavor, {}, [], None) + @mock.patch.object(objects.RequestSpec, 'get_by_instance_uuid') @mock.patch.object(objects.Instance, 'save') @mock.patch.object(objects.Instance, 'get_flavor')