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
This commit is contained in:
Mohammed Naser 2018-02-20 17:11:37 -05:00
parent 3120627d98
commit 16c2c8b3ee
3 changed files with 48 additions and 4 deletions

View File

@ -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

View File

@ -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(

View File

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