From 683005df39cbb825ff0702cd91773ad16d7b3998 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. Conflicts: nova/api/openstack/compute/servers.py nova/compute/api.py nova/tests/unit/api/openstack/compute/test_server_actions.py Modified: nova/tests/unit/compute/test_compute_api.py NOTE(elod.illes): * conflicts in servers.py and test_server_actions.py are due to bug fixing patch I25eff0271c856a8d3e83867b448e1dec6f6732ab is not backported to stable/train * api.py conflict is due to Ic2ad1468d31b7707b7f8f2b845a9cf47d9d076d5 is part of a feature introduced in Ussuri * modification of test_compute_api.py is also required due to patch I25eff0271c856a8d3e83867b448e1dec6f6732ab is not backported and another patch, Ide8eb9e09d22f20165474d499ef0524aefc67854, that cannot be backported to stable/train Change-Id: I7f93cfd18f948134c9cb429dea55740d2cf97994 Closes-Bug: #1489304 (cherry picked from commit 10e9a9b9fc62a3cf72c3717e3621ed95d3cf5519) (cherry picked from commit bcbeae2c605f4ab4ad805dddccac802928a180b6) --- nova/api/openstack/compute/servers.py | 1 + nova/compute/api.py | 23 +++++-- .../openstack/compute/test_server_actions.py | 15 +++++ nova/tests/unit/compute/test_compute_api.py | 60 +++++++++++++++++++ 4 files changed, 95 insertions(+), 4 deletions(-) diff --git a/nova/api/openstack/compute/servers.py b/nova/api/openstack/compute/servers.py index 42308af93123..68af4750d446 100644 --- a/nova/api/openstack/compute/servers.py +++ b/nova/api/openstack/compute/servers.py @@ -1139,6 +1139,7 @@ class ServersController(wsgi.Controller): exception.ImageNotActive, exception.ImageUnacceptable, exception.InvalidMetadata, + 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 69837f62d760..08ca453d04a0 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -3346,6 +3346,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 @@ -3445,6 +3451,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. @@ -3963,10 +3980,8 @@ 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) + if compute_utils.is_volume_backed_instance(context, instance, bdms): reason = _("Cannot rescue a volume-backed instance") raise exception.InstanceNotRescuable(instance_id=instance.uuid, 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 2c5feb9c4bb0..1f6d632dfd0f 100644 --- a/nova/tests/unit/api/openstack/compute/test_server_actions.py +++ b/nova/tests/unit/api/openstack/compute/test_server_actions.py @@ -642,6 +642,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 d17a9b54925f..8db729b77468 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -3612,6 +3612,66 @@ class _ComputeAPIUnitTestMixIn(object): "new password", auto_disk_config=True) + @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_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, + 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) + _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')