From a95c9e5668f6a7596e0198cca2b6b7fef20ab3e9 Mon Sep 17 00:00:00 2001 From: Steve Noyes Date: Thu, 31 Aug 2017 19:16:49 -0400 Subject: [PATCH] Fix attachments on volume show when volume is attaching With the new v3 cinder attach flow, nova calls attachment_update and then attachment_complete. During that time the volume has 'attaching' status. Because of that status, cinder show volume will show all the attachments as empty, even if the volume has attachments. Currently this will only happen during migration, but when multi-attach is working, this will be a general issue. The change is to the show volume flow. It will no longer look at the volume attach status, but only at the individual attachment status's. Any attachment that has status of attached will be returned by show. (This will only be an issue in Queens when the new multi-attach flow is used. There is no need for this change in Pike.) Partially Implements: blueprint cinder-new-attach-apis Change-Id: I8a2bff5a668ec58ee80c192cb72326f2b3599c39 Closes-Bug: 1713521 --- cinder/api/v2/views/volumes.py | 26 +++++++------- cinder/tests/unit/api/v3/test_volumes.py | 44 ++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 14 deletions(-) diff --git a/cinder/api/v2/views/volumes.py b/cinder/api/v2/views/volumes.py index 8e9139be034..302aebb46b8 100644 --- a/cinder/api/v2/views/volumes.py +++ b/cinder/api/v2/views/volumes.py @@ -116,20 +116,18 @@ class ViewBuilder(common.ViewBuilder): """Retrieve the attachments of the volume object.""" attachments = [] - if volume['attach_status'] == fields.VolumeAttachStatus.ATTACHED: - attaches = volume.volume_attachment - for attachment in attaches: - if (attachment.get('attach_status') == - fields.VolumeAttachStatus.ATTACHED): - a = {'id': attachment.get('volume_id'), - 'attachment_id': attachment.get('id'), - 'volume_id': attachment.get('volume_id'), - 'server_id': attachment.get('instance_uuid'), - 'host_name': attachment.get('attached_host'), - 'device': attachment.get('mountpoint'), - 'attached_at': attachment.get('attach_time'), - } - attachments.append(a) + for attachment in volume.volume_attachment: + if (attachment.get('attach_status') == + fields.VolumeAttachStatus.ATTACHED): + a = {'id': attachment.get('volume_id'), + 'attachment_id': attachment.get('id'), + 'volume_id': attachment.get('volume_id'), + 'server_id': attachment.get('instance_uuid'), + 'host_name': attachment.get('attached_host'), + 'device': attachment.get('mountpoint'), + 'attached_at': attachment.get('attach_time'), + } + attachments.append(a) return attachments diff --git a/cinder/tests/unit/api/v3/test_volumes.py b/cinder/tests/unit/api/v3/test_volumes.py index 15bb6a620eb..91c7142f2ec 100644 --- a/cinder/tests/unit/api/v3/test_volumes.py +++ b/cinder/tests/unit/api/v3/test_volumes.py @@ -20,12 +20,14 @@ import webob from cinder.api import extensions from cinder.api.openstack import api_version_request as api_version +from cinder.api.v2.views.volumes import ViewBuilder from cinder.api.v3 import volumes from cinder import context from cinder import db from cinder import exception from cinder.group import api as group_api from cinder import objects +from cinder.objects import fields from cinder import test from cinder.tests.unit.api import fakes from cinder.tests.unit.api.v2 import fakes as v2_fakes @@ -572,3 +574,45 @@ class VolumeApiTest(test.TestCase): self.assertRaises(webob.exc.HTTPConflict, self.controller.revert, req, fake_volume['id'], {'revert': {'snapshot_id': fake_snapshot['id']}}) + + def test_view_get_attachments(self): + fake_volume = self._fake_create_volume() + fake_volume['attach_status'] = fields.VolumeAttachStatus.ATTACHING + att_time = datetime.datetime(2017, 8, 31, 21, 55, 7, + tzinfo=iso8601.iso8601.Utc()) + a1 = { + 'id': fake.UUID1, + 'volume_id': fake.UUID2, + 'instance': None, + 'attached_host': None, + 'mountpoint': None, + 'attach_time': None, + 'attach_status': fields.VolumeAttachStatus.ATTACHING + } + a2 = { + 'id': fake.UUID3, + 'volume_id': fake.UUID4, + 'instance_uuid': fake.UUID5, + 'attached_host': 'host1', + 'mountpoint': 'na', + 'attach_time': att_time, + 'attach_status': fields.VolumeAttachStatus.ATTACHED + } + attachment1 = objects.VolumeAttachment(self.ctxt, **a1) + attachment2 = objects.VolumeAttachment(self.ctxt, **a2) + atts = {'objects': [attachment1, attachment2]} + attachments = objects.VolumeAttachmentList(self.ctxt, **atts) + + fake_volume['volume_attachment'] = attachments + + # get_attachments should only return attachments with the + # attached status = ATTACHED + attachments = ViewBuilder()._get_attachments(fake_volume) + + self.assertEqual(1, len(attachments)) + self.assertEqual(fake.UUID3, attachments[0]['attachment_id']) + self.assertEqual(fake.UUID4, attachments[0]['volume_id']) + self.assertEqual(fake.UUID5, attachments[0]['server_id']) + self.assertEqual('host1', attachments[0]['host_name']) + self.assertEqual('na', attachments[0]['device']) + self.assertEqual(att_time, attachments[0]['attached_at'])