openstack
/
cinder
Code Issues Proposed changes

Refresh volume when checking for conflicting attachments 

We should only be able to create more than one attachment to the
same volume if it's (1) multiattach=True or (2) it's multiattach=False
AND the attachments are to the same instance. It is not valid to
attach more than one instance to the same multiattach=False volume.

The _attachment_reserve method is checking for this if the
conditional update to the volume status fails, but it does
not refresh the volume before checking the attachments. Since
we could be racing to create attachments concurrently, when
the request that failed the conditional update actually pulled
the volume out of the DB, it might not have had any attachments,
so we need to refresh it before checking the list of attachments
to see if the instance we're trying to attach (reserve the volume)
is the same as what's already attached to the volume.

Change-Id: Iee78555163bbcbb5ff3e0ba008d7c87a3aedfb0f
Closes-Bug: #1762687
This commit is contained in:
Matt Riedemann 2018-04-10 12:28:05 -04:00
parent d9242027c1
commit dd93b0c719
2 changed files with 26 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
cinder/tests/unit/volume/test_volume.py
View File

@ -1804,6 +1804,28 @@ class VolumeTestCase(base.BaseVolumeTestCase):
self.assertEqual(attachment.attach_status, 'reserved')
def test_attachment_reserve_conditional_update_attach_race(self):
# Tests a scenario where two instances are racing to attach the
# same multiattach=False volume. One updates the volume status to
# "reserved" but the other fails the conditional update which is
# then validated to not be the same instance that is already attached
# to the multiattach=False volume which triggers a failure.
volume = tests_utils.create_volume(self.context)
# Assert that we're not dealing with a multiattach volume and that
# it does not have any existing attachments.
self.assertFalse(volume.multiattach)
self.assertEqual(0, len(volume.volume_attachment))
# Attach the first instance which is OK and should update the volume
# status to 'reserved'.
self.volume_api._attachment_reserve(self.context, volume, fake.UUID1)
# Try attaching a different instance to the same volume which should
# fail.
ex = self.assertRaises(exception.InvalidVolume,
self.volume_api._attachment_reserve,
self.context, volume, fake.UUID2)
self.assertIn("status must be available or downloading",
six.text_type(ex))
def test_unreserve_volume_success_in_use(self):
UUID = six.text_type(uuid.uuid4())
volume = tests_utils.create_volume(self.context, status='attaching')

4
cinder/volume/api.py
View File

@ -2094,6 +2094,10 @@ class API(base.Base):
override = False
if instance_uuid:
override = True
# Refresh the volume reference in case multiple instances were
# being concurrently attached to the same non-multiattach
# volume.
vref = objects.Volume.get_by_id(ctxt, vref.id)
for attachment in vref.volume_attachment:
if attachment.instance_uuid != instance_uuid:
override = False