Merge "Ensure attachment_id always exists for block device mapping" into stable/queens

Author: Zuul

nova/compute/api.py

@@ -1346,6 +1346,13 @@ def _validate_bdm(self, context, instance, instance_type,
                         # 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,

nova/tests/functional/wsgi/test_servers.py

@@ -276,10 +276,7 @@ def test_bfv_delete_build_request_pre_scheduling_ocata(self, mock_get):
 
         # 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(

nova/tests/unit/compute/test_compute_api.py

@@ -4005,6 +4005,46 @@ def test_get_bdm_image_metadata_with_cinder_down(self, mock_get):
                           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',