diff --git a/nova/compute/api.py b/nova/compute/api.py index e6f22a388c6f..e835a3de8ddf 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -1346,6 +1346,13 @@ class API(base.Base): # compatibility can be removed after Ocata EOL. self._check_attach(context, volume, instance) bdm.volume_size = volume.get('size') + + # NOTE(mnaser): If we end up reserving the volume, it will + # not have an attachment_id which is needed + # for cleanups. This can be removed once + # all calls to reserve_volume are gone. + if 'attachment_id' not in bdm: + bdm.attachment_id = None except (exception.CinderConnectionFailed, exception.InvalidVolume): raise diff --git a/nova/tests/functional/wsgi/test_servers.py b/nova/tests/functional/wsgi/test_servers.py index b0a69991259c..3c2aea783baa 100644 --- a/nova/tests/functional/wsgi/test_servers.py +++ b/nova/tests/functional/wsgi/test_servers.py @@ -276,10 +276,7 @@ class ServersPreSchedulingTestCase(test.TestCase, # The volume should no longer have any attachments as instance delete # should have removed them. - # self.assertNotIn(volume_id, cinder.reserved_volumes) - # FIXME(mnaser): This is part of bug 1750666 where the BDMs aren't - # properly deleted because they don't exist in the database. - self.assertIn(volume_id, cinder.reserved_volumes) + self.assertNotIn(volume_id, cinder.reserved_volumes) def test_bfv_delete_build_request_pre_scheduling(self): cinder = self.useFixture( diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index aff32ec674db..a48ca6b557c9 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -4028,6 +4028,46 @@ class _ComputeAPIUnitTestMixIn(object): self.context, bdms, legacy_bdm=True) + @mock.patch.object(objects.service, 'get_minimum_version_all_cells', + return_value=17) + @mock.patch.object(objects.Service, 'get_minimum_version', + return_value=17) + @mock.patch.object(cinder.API, 'get') + @mock.patch.object(cinder.API, 'reserve_volume') + def test_validate_bdm_returns_attachment_id(self, mock_reserve_volume, + mock_get, mock_get_min_ver, + mock_get_min_ver_all): + # Tests that bdm validation *always* returns an attachment_id even if + # it's None. + instance = self._create_instance_obj() + instance_type = self._create_flavor() + volume_id = 'e856840e-9f5b-4894-8bde-58c6e29ac1e8' + volume_info = {'status': 'available', + 'attach_status': 'detached', + 'id': volume_id, + 'multiattach': False} + mock_get.return_value = volume_info + + # NOTE(mnaser): We use the AnonFakeDbBlockDeviceDict to make sure that + # the attachment_id field does not get any defaults to + # properly test this function. + bdms = [objects.BlockDeviceMapping( + **fake_block_device.AnonFakeDbBlockDeviceDict( + { + 'boot_index': 0, + 'volume_id': volume_id, + 'source_type': 'volume', + 'destination_type': 'volume', + 'device_name': 'vda', + }))] + self.compute_api._validate_bdm(self.context, instance, instance_type, + bdms) + self.assertIsNone(bdms[0].attachment_id) + + mock_get.assert_called_once_with(self.context, volume_id) + mock_reserve_volume.assert_called_once_with( + self.context, volume_id) + @mock.patch.object(objects.service, 'get_minimum_version_all_cells', return_value=17) @mock.patch.object(objects.Service, 'get_minimum_version',