openstack
/
nova
Code Issues Proposed changes

Robustify attachment tracking in CinderFixtureNewAttachFlow 

Cinder allows more than one attachment record on a non-multiattach
volume as long as only one attachment is "connected" (has a host
connector).

When you create an empty (no connector) attachment for an in-use
volume, the volume status changes to "attaching". If you try to
update the empty attachment before deleting the previous attachment,
Cinder will return a 400 like:

  Invalid volume: Volume b2aba195-6570-40c4-82bb-46d3557fceeb status
  must be available or downloading to reserve, but the current status
  is attaching.

This change refactors the attachment tracking in the
CinderFixtureNewAttachFlow fixture to be able to track the connector
per attachment and if code tries to update an attachment on an
already connected volume it will fail.

Change-Id: I369f82245465e96fc15d4fc71a79a8a71f6f2c6d
(cherry picked from commit 1d3ca5a3a0)
(cherry picked from commit 1e1bbdc4cd)
This commit is contained in:
Matt Riedemann 2019-05-13 14:42:25 -04:00 committed by Lee Yarwood
parent b84eb40a66
commit c1b372fc92
1 changed files with 37 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

57
nova/tests/fixtures.py
View File

@ -1588,16 +1588,21 @@ class CinderFixtureNewAttachFlow(fixtures.Fixture):
self.swap_volume_instance_uuid = None self.swap_volume_instance_uuid = None
self.swap_volume_instance_error_uuid = None self.swap_volume_instance_error_uuid = None
self.attachment_error_id = None self.attachment_error_id = None
# A map of volumes to a list of (attachment_id, instance_uuid). # A dict, keyed by volume id, to a dict, keyed by attachment id,
# with keys:
# - id: the attachment id
# - instance_uuid: uuid of the instance attached to the volume
# - connector: host connector dict; None if not connected
# Note that a volume can have multiple attachments even without # Note that a volume can have multiple attachments even without
# multi-attach, as some flows create a blank 'reservation' attachment # multi-attach, as some flows create a blank 'reservation' attachment
# before deleting another attachment. # before deleting another attachment. However, a non-multiattach volume
self.volume_to_attachment = collections.defaultdict(list) # can only have at most one attachment with a host connector at a time.
self.volume_to_attachment = collections.defaultdict(dict)
def volume_ids_for_instance(self, instance_uuid): def volume_ids_for_instance(self, instance_uuid):
for volume_id, attachments in self.volume_to_attachment.items(): for volume_id, attachments in self.volume_to_attachment.items():
for _, _instance_uuid in attachments: for attachment in attachments.values():
if _instance_uuid == instance_uuid: if attachment['instance_uuid'] == instance_uuid:
# we might have multiple volumes attached to this instance # we might have multiple volumes attached to this instance
# so yield rather than return # so yield rather than return
yield volume_id yield volume_id
@ -1629,14 +1634,14 @@ class CinderFixtureNewAttachFlow(fixtures.Fixture):
else self.swap_volume_instance_error_uuid) else self.swap_volume_instance_error_uuid)
if attachments: if attachments:
attachment_id, instance_uuid = attachments[0] attachment = list(attachments.values())[0]
volume.update({ volume.update({
'status': 'in-use', 'status': 'in-use',
'attachments': { 'attachments': {
instance_uuid: { instance_uuid: {
'mountpoint': '/dev/vdb', 'mountpoint': '/dev/vdb',
'attachment_id': attachment_id 'attachment_id': attachment['id']
} }
}, },
'attach_status': 'attached' 'attach_status': 'attached'
@ -1646,7 +1651,7 @@ class CinderFixtureNewAttachFlow(fixtures.Fixture):
# Check to see if the volume is attached. # Check to see if the volume is attached.
if attachments: if attachments:
# The volume is attached. # The volume is attached.
attachment_id, instance_uuid = attachments[0] attachment = list(attachments.values())[0]
volume = { volume = {
'status': 'in-use', 'status': 'in-use',
'display_name': volume_id, 'display_name': volume_id,
@ -1655,8 +1660,8 @@ class CinderFixtureNewAttachFlow(fixtures.Fixture):
'multiattach': volume_id == self.MULTIATTACH_VOL, 'multiattach': volume_id == self.MULTIATTACH_VOL,
'size': 1, 'size': 1,
'attachments': { 'attachments': {
instance_uuid: { attachment['instance_uuid']: {
'attachment_id': attachment_id, 'attachment_id': attachment['id'],
'mountpoint': '/dev/vdb' 'mountpoint': '/dev/vdb'
} }
} }
@ -1700,14 +1705,13 @@ class CinderFixtureNewAttachFlow(fixtures.Fixture):
"""Find attachment corresponding to ``attachment_id``. """Find attachment corresponding to ``attachment_id``.
Returns: Returns:
A tuple of the volume ID, an attachment-instance mapping tuple A tuple of the volume ID, an attachment dict
for the given attachment ID, and a list of attachment-instance for the given attachment ID, and a dict (keyed by attachment
mapping tuples for the volume. id) of attachment dicts for the volume.
""" """
for volume_id, attachments in self.volume_to_attachment.items(): for volume_id, attachments in self.volume_to_attachment.items():
for attachment in attachments: for attachment in attachments.values():
_attachment_id, instance_uuid = attachment if attachment_id == attachment['id']:
if attachment_id == _attachment_id:
return volume_id, attachment, attachments return volume_id, attachment, attachments
raise exception.VolumeAttachmentNotFound( raise exception.VolumeAttachmentNotFound(
attachment_id=attachment_id) attachment_id=attachment_id)
@ -1718,8 +1722,10 @@ class CinderFixtureNewAttachFlow(fixtures.Fixture):
if self.attachment_error_id is not None: if self.attachment_error_id is not None:
attachment_id = self.attachment_error_id attachment_id = self.attachment_error_id
attachment = {'id': attachment_id, 'connection_info': {'data': {}}} attachment = {'id': attachment_id, 'connection_info': {'data': {}}}
self.volume_to_attachment[volume_id].append( self.volume_to_attachment[volume_id][attachment_id] = {
(attachment_id, instance_uuid)) 'id': attachment_id,
'instance_uuid': instance_uuid,
'connector': connector}
LOG.info('Created attachment %s for volume %s. Total ' LOG.info('Created attachment %s for volume %s. Total '
'attachments for volume: %d', attachment_id, volume_id, 'attachments for volume: %d', attachment_id, volume_id,
len(self.volume_to_attachment[volume_id])) len(self.volume_to_attachment[volume_id]))
@ -1730,7 +1736,7 @@ class CinderFixtureNewAttachFlow(fixtures.Fixture):
# 'attachment' is a tuple defining a attachment-instance mapping # 'attachment' is a tuple defining a attachment-instance mapping
volume_id, attachment, attachments = ( volume_id, attachment, attachments = (
_find_attachment(attachment_id)) _find_attachment(attachment_id))
attachments.remove(attachment) del attachments[attachment_id]
LOG.info('Deleted attachment %s for volume %s. Total attachments ' LOG.info('Deleted attachment %s for volume %s. Total attachments '
'for volume: %d', attachment_id, volume_id, 'for volume: %d', attachment_id, volume_id,
len(attachments)) len(attachments))
@ -1738,7 +1744,18 @@ class CinderFixtureNewAttachFlow(fixtures.Fixture):
def fake_attachment_update(_self, context, attachment_id, connector, def fake_attachment_update(_self, context, attachment_id, connector,
mountpoint=None): mountpoint=None):
# Ensure the attachment exists # Ensure the attachment exists
_find_attachment(attachment_id) volume_id, attachment, attachments = (
_find_attachment(attachment_id))
# Cinder will only allow one "connected" attachment per
# non-multiattach volume at a time.
if volume_id != self.MULTIATTACH_VOL:
for _attachment in attachments.values():
if _attachment['connector'] is not None:
raise exception.InvalidInput(
'Volume %s is already connected with attachment '
'%s on host %s' % (volume_id, _attachment['id'],
_attachment['connector'].get('host')))
attachment['connector'] = connector
LOG.info('Updating volume attachment: %s', attachment_id) LOG.info('Updating volume attachment: %s', attachment_id)
attachment_ref = {'driver_volume_type': 'fake_type', attachment_ref = {'driver_volume_type': 'fake_type',
'id': attachment_id, 'id': attachment_id,