diff --git a/api-ref/source/os-volume-attachments.inc b/api-ref/source/os-volume-attachments.inc index 8146faf922f1..3304c88cc066 100644 --- a/api-ref/source/os-volume-attachments.inc +++ b/api-ref/source/os-volume-attachments.inc @@ -177,6 +177,9 @@ Policy defaults enable only users with the administrative role to perform this operation. Cloud providers can change these permissions through the ``policy.json`` file. +Updating, or what is commonly referred to as "swapping", volume attachments +with volumes that have more than one read/write attachment, is not supported. + Normal response codes: 202 Error response codes: badRequest(400), unauthorized(401), forbidden(403), itemNotFound(404), conflict(409) diff --git a/nova/api/openstack/compute/volumes.py b/nova/api/openstack/compute/volumes.py index 59cf001e7717..aa1c0b4fd4a2 100644 --- a/nova/api/openstack/compute/volumes.py +++ b/nova/api/openstack/compute/volumes.py @@ -405,7 +405,8 @@ class VolumeAttachmentController(wsgi.Controller): new_volume) except exception.VolumeBDMNotFound as e: raise exc.HTTPNotFound(explanation=e.format_message()) - except exception.InvalidVolume as e: + except (exception.InvalidVolume, + exception.MultiattachSwapVolumeNotSupported) as e: raise exc.HTTPBadRequest(explanation=e.format_message()) except exception.InstanceIsLocked as e: raise exc.HTTPConflict(explanation=e.format_message()) diff --git a/nova/compute/api.py b/nova/compute/api.py index 5dd8eae1f289..f63a66a0f083 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -4447,6 +4447,52 @@ class API(base.Base): else: self._detach_volume(context, instance, volume) + def _count_attachments_for_swap(self, ctxt, volume): + """Counts the number of attachments for a swap-related volume. + + Attempts to only count read/write attachments if the volume attachment + records exist, otherwise simply just counts the number of attachments + regardless of attach mode. + + :param ctxt: nova.context.RequestContext - user request context + :param volume: nova-translated volume dict from nova.volume.cinder. + :returns: count of attachments for the volume + """ + # This is a dict, keyed by server ID, to a dict of attachment_id and + # mountpoint. + attachments = volume.get('attachments', {}) + # Multiattach volumes can have more than one attachment, so if there + # is more than one attachment, attempt to count the read/write + # attachments. + if len(attachments) > 1: + count = 0 + for attachment in attachments.values(): + attachment_id = attachment['attachment_id'] + # Get the attachment record for this attachment so we can + # get the attach_mode. + # TODO(mriedem): This could be optimized if we had + # GET /attachments/detail?volume_id=volume['id'] in Cinder. + try: + attachment_record = self.volume_api.attachment_get( + ctxt, attachment_id) + # Note that the attachment record from Cinder has + # attach_mode in the top-level of the resource but the + # nova.volume.cinder code translates it and puts the + # attach_mode in the connection_info for some legacy + # reason... + if attachment_record.get( + 'connection_info', {}).get( + # attachments are read/write by default + 'attach_mode', 'rw') == 'rw': + count += 1 + except exception.VolumeAttachmentNotFound: + # attachments are read/write by default so count it + count += 1 + else: + count = len(attachments) + + return count + @check_instance_lock @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.PAUSED, vm_states.RESIZED]) @@ -4470,6 +4516,20 @@ class API(base.Base): except exception.InvalidInput as exc: raise exception.InvalidVolume(reason=exc.format_message()) + # Disallow swapping from multiattach volumes that have more than one + # read/write attachment. We know the old_volume has at least one + # attachment since it's attached to this server. The new_volume + # can't have any attachments because of the attach_status check above. + # We do this count after calling "begin_detaching" to lock against + # concurrent attachments being made while we're counting. + try: + if self._count_attachments_for_swap(context, old_volume) > 1: + raise exception.MultiattachSwapVolumeNotSupported() + except Exception: # This is generic to handle failures while counting + # We need to reset the detaching status before raising. + with excutils.save_and_reraise_exception(): + self.volume_api.roll_detaching(context, old_volume['id']) + # Get the BDM for the attached (old) volume so we can tell if it was # attached with the new-style Cinder 3.44 API. bdm = objects.BlockDeviceMapping.get_by_volume_and_instance( diff --git a/nova/exception.py b/nova/exception.py index 65598516e131..64dc557d2581 100644 --- a/nova/exception.py +++ b/nova/exception.py @@ -304,6 +304,11 @@ class MultiattachToShelvedNotSupported(Invalid): "shelved-offloaded instances.") +class MultiattachSwapVolumeNotSupported(Invalid): + msg_fmt = _('Swapping multi-attach volumes with more than one read/write ' + 'attachment is not supported.') + + class VolumeNotCreated(NovaException): msg_fmt = _("Volume %(volume_id)s did not finish being created" " even after we waited %(seconds)s seconds or %(attempts)s" diff --git a/nova/tests/unit/api/openstack/compute/test_volumes.py b/nova/tests/unit/api/openstack/compute/test_volumes.py index 687b49f6db53..c52e72f570e2 100644 --- a/nova/tests/unit/api/openstack/compute/test_volumes.py +++ b/nova/tests/unit/api/openstack/compute/test_volumes.py @@ -973,6 +973,81 @@ class VolumeAttachTestsV260(test.NoDBTestCase): 'shelved-offloaded instances.', six.text_type(ex)) +class SwapVolumeMultiattachTestCase(test.NoDBTestCase): + + @mock.patch('nova.api.openstack.common.get_instance') + @mock.patch('nova.volume.cinder.API.begin_detaching') + @mock.patch('nova.volume.cinder.API.roll_detaching') + def test_swap_multiattach_multiple_readonly_attachments_fails( + self, mock_roll_detaching, mock_begin_detaching, + mock_get_instance): + """Tests that trying to swap from a multiattach volume with + multiple read/write attachments will return an error. + """ + + def fake_volume_get(_context, volume_id): + if volume_id == uuids.old_vol_id: + return { + 'id': volume_id, + 'size': 1, + 'multiattach': True, + 'attachments': { + uuids.server1: { + 'attachment_id': uuids.attachment_id1, + 'mountpoint': '/dev/vdb' + }, + uuids.server2: { + 'attachment_id': uuids.attachment_id2, + 'mountpoint': '/dev/vdb' + } + } + } + if volume_id == uuids.new_vol_id: + return { + 'id': volume_id, + 'size': 1, + 'attach_status': 'detached' + } + raise exception.VolumeNotFound(volume_id=volume_id) + + def fake_attachment_get(_context, attachment_id): + return {'connection_info': {'attach_mode': 'rw'}} + + ctxt = context.get_admin_context() + instance = fake_instance.fake_instance_obj( + ctxt, uuid=uuids.server1, vm_state=vm_states.ACTIVE, + task_state=None, launched_at=datetime.datetime(2018, 6, 6)) + mock_get_instance.return_value = instance + controller = volumes_v21.VolumeAttachmentController() + with test.nested( + mock.patch.object(controller.volume_api, 'get', + side_effect=fake_volume_get), + mock.patch.object(controller.compute_api.volume_api, + 'attachment_get', + side_effect=fake_attachment_get)) as ( + mock_volume_get, mock_attachment_get + ): + req = fakes.HTTPRequest.blank( + '/servers/%s/os-volume_attachments/%s' % + (uuids.server1, uuids.old_vol_id)) + req.headers['content-type'] = 'application/json' + req.environ['nova.context'] = ctxt + body = { + 'volumeAttachment': { + 'volumeId': uuids.new_vol_id + } + } + ex = self.assertRaises( + webob.exc.HTTPBadRequest, controller.update, req, + uuids.server1, uuids.old_vol_id, body=body) + self.assertIn('Swapping multi-attach volumes with more than one ', + six.text_type(ex)) + mock_attachment_get.assert_has_calls([ + mock.call(ctxt, uuids.attachment_id1), + mock.call(ctxt, uuids.attachment_id2)], any_order=True) + mock_roll_detaching.assert_called_once_with(ctxt, uuids.old_vol_id) + + class CommonBadRequestTestCase(object): resource = None diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index 367909c009c2..333d033bb1b5 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -2857,6 +2857,58 @@ class _ComputeAPIUnitTestMixIn(object): _do_test() + def test_count_attachments_for_swap_not_found_and_readonly(self): + """Tests that attachment records that aren't found are considered + read/write by default. Also tests that read-only attachments are + not counted. + """ + ctxt = context.get_admin_context() + volume = { + 'attachments': { + uuids.server1: { + 'attachment_id': uuids.attachment1 + }, + uuids.server2: { + 'attachment_id': uuids.attachment2 + } + } + } + + def fake_attachment_get(_context, attachment_id): + if attachment_id == uuids.attachment1: + raise exception.VolumeAttachmentNotFound( + attachment_id=attachment_id) + return {'connection_info': {'attach_mode': 'ro'}} + + with mock.patch.object(self.compute_api.volume_api, 'attachment_get', + side_effect=fake_attachment_get) as mock_get: + self.assertEqual( + 1, self.compute_api._count_attachments_for_swap(ctxt, volume)) + mock_get.assert_has_calls([ + mock.call(ctxt, uuids.attachment1), + mock.call(ctxt, uuids.attachment2)], any_order=True) + + @mock.patch('nova.volume.cinder.API.attachment_get', + new_callable=mock.NonCallableMock) # asserts not called + def test_count_attachments_for_swap_no_query(self, mock_attachment_get): + """Tests that if the volume has <2 attachments, we don't query + the attachments for their attach_mode value. + """ + volume = {} + self.assertEqual( + 0, self.compute_api._count_attachments_for_swap( + mock.sentinel.context, volume)) + volume = { + 'attachments': { + uuids.server: { + 'attachment_id': uuids.attach1 + } + } + } + self.assertEqual( + 1, self.compute_api._count_attachments_for_swap( + mock.sentinel.context, volume)) + @mock.patch.object(compute_utils, 'is_volume_backed_instance') @mock.patch.object(objects.Instance, 'save') @mock.patch.object(image_api.API, 'create') diff --git a/releasenotes/notes/bug-1775418-754fc50261f5d7c3.yaml b/releasenotes/notes/bug-1775418-754fc50261f5d7c3.yaml new file mode 100644 index 000000000000..683422b4012e --- /dev/null +++ b/releasenotes/notes/bug-1775418-754fc50261f5d7c3.yaml @@ -0,0 +1,10 @@ +--- +fixes: + - | + The `os-volume_attachments`_ update API, commonly referred to as the swap + volume API will now return a ``400`` (BadRequest) error when attempting to + swap from a multi attached volume with more than one active read/write + attachment resolving `bug #1775418`_. + + .. _os-volume_attachments: https://developer.openstack.org/api-ref/compute/?expanded=update-a-volume-attachment-detail + .. _bug #1775418: https://launchpad.net/bugs/1775418