openstack
/
cinder
Code Issues Proposed changes

Fix _attachment_reserve to not allow attaching an invalid status volume 

It is currently possible to create a volume attachment for a server
when the volume is in error status because of the override logic
in the _attachment_reserve method. What results is that the volume
attach operation fails in nova-compute which rolls back and deletes
the volume attachment, which puts the volume into 'available' status
because it no longer has any attachments, which in fact it should
have never allowed the attachment create/reserve in the first place.

This updates the override logic such that a volume without any
attachments which is in an invalid status will result in an error
being raised.

Change-Id: Id9cf2f510684cd296ffbcaf53d11889cfe8973b9
Closes-Bug: #1785050
This commit is contained in:
Matt Riedemann 2018-08-08 16:44:33 -04:00
parent 9bc9a528ef
commit 9c0123eb70
2 changed files with 24 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -1859,6 +1859,24 @@ class VolumeTestCase(base.BaseVolumeTestCase):
self.assertIn("status must be available or downloading",
six.text_type(ex))
def test_attachment_reserve_with_instance_uuid_error_volume(self):
# Tests that trying to create an attachment (with an instance_uuid
# provided) on a volume that's not 'available' or 'downloading' status
# will fail if the volume does not have any attachments, similar to how
# the volume reserve action works.
volume = tests_utils.create_volume(self.context, status='error')
# 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))
# Try attaching an instance to the volume which should fail based on
# the volume status.
ex = self.assertRaises(exception.InvalidVolume,
self.volume_api._attachment_reserve,
self.context, volume, fake.UUID1)
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')

11
cinder/volume/api.py
View File

@ -2086,18 +2086,19 @@ class API(base.Base):
result = vref.conditional_update({'status': 'reserved'}, expected)
if not result:
# Make sure we're not going to the same instance, in which case
# it could be a live-migrate or similar scenario (LP BUG: 1694530)
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
# If we're attaching the same volume to the same instance,
# we could be migrating the instance to another host in
# which case we want to allow the reservation.
# (LP BUG: 1694530)
if attachment.instance_uuid == instance_uuid:
override = True
break
if not override: