Merge "Ensure attachment_id always exists for block device mapping" into stable/queens17.0.0.0rc317.0.0

3 files changed, 48 insertions, 4 deletions

diff --git a/nova/compute/api.py b/nova/compute/api.py
index 622eecc971..9e7387b9bc 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,
                         exception.MultiattachNotSupportedOldMicroversion,
diff --git a/nova/tests/functional/wsgi/test_servers.py b/nova/tests/functional/wsgi/test_servers.py
index b0a6999125..3c2aea783b 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 a00c0a02b8..ad7e03260b 100644
--- a/nova/tests/unit/compute/test_compute_api.py
+++ b/nova/tests/unit/compute/test_compute_api.py
@@ -4010,6 +4010,46 @@ class _ComputeAPIUnitTestMixIn(object):
     @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',
+                       return_value=17)
+    @mock.patch.object(cinder.API, 'get')
     @mock.patch.object(cinder.API, 'reserve_volume',
                        side_effect=exception.InvalidInput(reason='error'))
     def test_validate_bdm_with_error_volume(self, mock_reserve_volume,