From 16c2c8b3ee9d70e928a61ceb1ef5931d40e509a4 Mon Sep 17 00:00:00 2001
From: Mohammed Naser <mnaser@vexxhost.com>
Date: Tue, 20 Feb 2018 17:11:37 -0500
Subject: [PATCH] Ensure attachment_id always exists for block device mapping

If an instance is deleted before it is scheduled, the BDM
clean-up code uses the mappings from the build request as
they don't exist in the database yet.

When using the older attachment flow with reserve_volume,
there is no attachment_id bound to the block device mapping
and because it is not loaded from database but rather from
the build request, accessing the attachment_id field raises
an exception with 'attachment_id not lazy-loadable'

If we did a new style attach, _validate_bdm will add the
attachment_id from Cinder.  If we did not, then this patch
will make sure to set it to 'None' to avoid raising an
exception when checking if we have an attachment_id set in
the BDM clean-up code

Change-Id: I3cc775fc7dafe691b97a15e50ae2e93c92f355be
Closes-Bug: #1750666
---
 nova/compute/api.py                         |  7 ++++
 nova/tests/functional/wsgi/test_servers.py  |  5 +--
 nova/tests/unit/compute/test_compute_api.py | 40 +++++++++++++++++++++
 3 files changed, 48 insertions(+), 4 deletions(-)

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',