From b5173b419219437b50f49c88bce9727ed0ed1ee8 Mon Sep 17 00:00:00 2001 From: Amit Uniyal Date: Fri, 5 Jan 2024 08:41:29 +0000 Subject: [PATCH] Fixes: bfv vm reboot ends up in an error state. we only need to verify if bdm has attachment id and it should be present in both nova and cinde DB. For tests coverage, added tests for bfv server to test different bdm source type. Closes-Bug: 2048154 Closes-Bug: 2048184 Change-Id: Icffcbad27d99a800e3f285565c0b823f697e388c --- nova/compute/manager.py | 3 +- nova/tests/fixtures/cinder.py | 61 ++++++++++++++++--- nova/tests/functional/integrated_helpers.py | 37 +++++++++++ .../tests/functional/test_boot_from_volume.py | 16 +++++ 4 files changed, 108 insertions(+), 9 deletions(-) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index c7b25ac26a..a477f154d6 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -4222,8 +4222,7 @@ class ComputeManager(manager.Manager): nova_attachments = [] bdms_to_delete = [] for bdm in bdms.objects: - if bdm.volume_id and bdm.source_type == 'volume' and \ - bdm.destination_type == 'volume': + if bdm.volume_id and bdm.attachment_id: try: self.volume_api.attachment_get(context, bdm.attachment_id) except exception.VolumeAttachmentNotFound: diff --git a/nova/tests/fixtures/cinder.py b/nova/tests/fixtures/cinder.py index 0ae7df38d8..049d22ebc2 100644 --- a/nova/tests/fixtures/cinder.py +++ b/nova/tests/fixtures/cinder.py @@ -101,7 +101,9 @@ class CinderFixture(fixtures.Fixture): # multi-attach, as some flows create a blank 'reservation' attachment # before deleting another attachment. However, a non-multiattach volume # can only have at most one attachment with a host connector at a time. + self.volumes = collections.defaultdict(dict) self.volume_to_attachment = collections.defaultdict(dict) + self.volume_snapshots = collections.defaultdict(dict) def setUp(self): super().setUp() @@ -165,6 +167,15 @@ class CinderFixture(fixtures.Fixture): self.useFixture(fixtures.MockPatch( 'nova.volume.cinder.API.attachment_get_all', side_effect=self.fake_attachment_get_all, autospec=False)) + self.useFixture(fixtures.MockPatch( + 'nova.volume.cinder.API.create_snapshot_force', + side_effect=self.fake_create_snapshot_force, autospec=False)) + self.useFixture(fixtures.MockPatch( + 'nova.volume.cinder.API.get_snapshot', + side_effect=self.fake_get_snapshot, autospec=False)) + self.useFixture(fixtures.MockPatch( + 'nova.volume.cinder.API.create', + side_effect=self.fake_vol_create, autospec=False)) def _is_multiattach(self, volume_id): return volume_id in [ @@ -218,13 +229,16 @@ class CinderFixture(fixtures.Fixture): return {'save_volume_id': new_volume_id} def fake_get(self, context, volume_id, microversion=None): - volume = { - 'display_name': volume_id, - 'id': volume_id, - 'size': 1, - 'multiattach': self._is_multiattach(volume_id), - 'availability_zone': self.az - } + if volume_id in self.volumes: + volume = self.volumes[volume_id] + else: + volume = { + 'display_name': volume_id, + 'id': volume_id, + 'size': 1, + 'multiattach': self._is_multiattach(volume_id), + 'availability_zone': self.az + } # Add any attachment details the fixture has fixture_attachments = self.volume_to_attachment[volume_id] @@ -466,3 +480,36 @@ class CinderFixture(fixtures.Fixture): def delete_vol_attachment(self, vol_id): del self.volume_to_attachment[vol_id] + + def fake_create_snapshot_force(self, _ctxt, volume_id, name, description): + _id = uuidutils.generate_uuid() + snapshot = { + 'id': _id, + 'volume_id': volume_id, + 'display_name': name, + 'display_description': description, + 'status': 'creating', + } + self.volume_snapshots[_id] = snapshot + return snapshot + + def fake_get_snapshot(self, _ctxt, snap_id): + if snap_id in self.volume_snapshots: + # because instance is getting unquiesce_instance + self.volume_snapshots[snap_id]['status'] = 'available' + return self.volume_snapshots[snap_id] + + def fake_vol_create(self, _ctxt, size, name, description, + snapshot=None, image_id=None, volume_type=None, metadata=None, + availability_zone=None): + _id = uuidutils.generate_uuid() + volume = { + 'id': _id, + 'status': 'available', + 'display_name': name or 'fake-cinder-vol', + 'attach_status': 'detached', + 'size': size, + 'display_description': description or 'fake-description', + } + self.volumes[_id] = volume + return volume diff --git a/nova/tests/functional/integrated_helpers.py b/nova/tests/functional/integrated_helpers.py index 5166e7bdd3..41f0e7f1bc 100644 --- a/nova/tests/functional/integrated_helpers.py +++ b/nova/tests/functional/integrated_helpers.py @@ -27,6 +27,7 @@ import time from oslo_concurrency import lockutils from oslo_log import log as logging import oslo_messaging as messaging +from oslo_utils.fixture import uuidsentinel as uuids from nova.compute import instance_actions from nova.compute import rpcapi as compute_rpcapi @@ -702,6 +703,42 @@ class InstanceHelperMixin: self.cinder.create_vol_attachment( volume_id, server['id']) + def _create_server_boot_from_volume(self): + bfv_image_id = uuids.bfv_image_uuid + timestamp = datetime.datetime(2011, 1, 1, 1, 2, 3) + + image = { + 'id': bfv_image_id, + 'name': 'fake_image_name', + 'created_at': timestamp, + 'updated_at': timestamp, + 'deleted_at': None, + 'deleted': False, + 'status': 'active', + 'container_format': 'raw', + 'disk_format': 'raw', + 'min_disk': 0 + } + + self.glance.create(None, image) + + # for bfv, image is not required in server request + server = self._build_server() + server.pop('imageRef') + + # as bfv-image will be used as source in block_device_mapping_v2 + # here block device will be created based on bfv-image + # i.e bfvimage_id + server['block_device_mapping_v2'] = [{ + 'source_type': 'image', + 'destination_type': 'volume', + 'boot_index': 0, + 'uuid': bfv_image_id, + 'volume_size': 1, + }] + server = self.api.post_server({'server': server}) + return self._wait_for_state_change(server, 'ACTIVE') + class PlacementHelperMixin: """A helper mixin for interacting with placement.""" diff --git a/nova/tests/functional/test_boot_from_volume.py b/nova/tests/functional/test_boot_from_volume.py index 6396954bf4..a95167542e 100644 --- a/nova/tests/functional/test_boot_from_volume.py +++ b/nova/tests/functional/test_boot_from_volume.py @@ -200,6 +200,22 @@ class BootFromVolumeTest(integrated_helpers._IntegratedTestBase): self.assertIn('You specified more local devices than the limit allows', str(ex)) + def test_reboot_bfv_instance(self): + # verify bdm 'source_type': 'image' and 'destination_type': 'volume' + server = self._create_server_boot_from_volume() + server = self._reboot_server(server, hard=True) + self.assertEqual(server['status'], 'ACTIVE') + + def test_reboot_bfv_instance_snapshot(self): + # verify bdm 'source_type': 'snapshot' and 'destination_type': 'volume' + server = self._create_server_boot_from_volume() + self._snapshot_server(server, 'snap1') + images = self.api.get_images() + snap_img = [img for img in images if img['name'] == 'snap1'][0] + server1 = self._create_server(image_uuid=snap_img['id']) + self._reboot_server(server1, hard=True) + self.assertEqual(server['status'], 'ACTIVE') + class BootFromVolumeLargeRequestTest(test.TestCase, integrated_helpers.InstanceHelperMixin):