diff --git a/nova/api/openstack/compute/volumes.py b/nova/api/openstack/compute/volumes.py index ab97279117b0..190110ecb9e8 100644 --- a/nova/api/openstack/compute/volumes.py +++ b/nova/api/openstack/compute/volumes.py @@ -292,24 +292,16 @@ class VolumeAttachmentController(wsgi.Controller): volume_id = id instance = common.get_instance(self.compute_api, context, server_id) - bdms = objects.BlockDeviceMappingList.get_by_instance_uuid( - context, instance.uuid) - - if not bdms: - msg = _("Instance %s is not attached.") % server_id - raise exc.HTTPNotFound(explanation=msg) - - assigned_mountpoint = None - - for bdm in bdms: - if bdm.volume_id == volume_id: - assigned_mountpoint = bdm.device_name - break - - if assigned_mountpoint is None: - msg = _("volume_id not found: %s") % volume_id + try: + bdm = objects.BlockDeviceMapping.get_by_volume_and_instance( + context, volume_id, instance.uuid) + except exception.VolumeBDMNotFound: + msg = (_("Instance %(instance)s is not attached " + "to volume %(volume)s") % + {'instance': server_id, 'volume': volume_id}) raise exc.HTTPNotFound(explanation=msg) + assigned_mountpoint = bdm.device_name return {'volumeAttachment': _translate_attachment_detail_view( volume_id, instance.uuid, @@ -359,16 +351,6 @@ class VolumeAttachmentController(wsgi.Controller): attachment['serverId'] = server_id attachment['volumeId'] = volume_id attachment['device'] = device - - # NOTE(justinsb): And now, we have a problem... - # The attach is async, so there's a window in which we don't see - # the attachment (until the attachment completes). We could also - # get problems with concurrent requests. I think we need an - # attachment state, and to write to the DB here, but that's a bigger - # change. - # For now, we'll probably have to rely on libraries being smart - - # TODO(justinsb): How do I return "accepted" here? return {'volumeAttachment': attachment} @wsgi.response(202) @@ -399,35 +381,19 @@ class VolumeAttachmentController(wsgi.Controller): instance = common.get_instance(self.compute_api, context, server_id) - bdms = objects.BlockDeviceMappingList.get_by_instance_uuid( - context, instance.uuid) - found = False try: - for bdm in bdms: - if bdm.volume_id != old_volume_id: - continue - try: - self.compute_api.swap_volume(context, instance, old_volume, - new_volume) - found = True - break - except (exception.VolumeBDMNotFound): - # The volume is not attached. Treat it as NotFound - # by falling through. - pass - except exception.InvalidVolume as e: - raise exc.HTTPBadRequest(explanation=e.format_message()) + self.compute_api.swap_volume(context, instance, old_volume, + new_volume) + except exception.VolumeBDMNotFound as e: + raise exc.HTTPNotFound(explanation=e.format_message()) + except exception.InvalidVolume as e: + raise exc.HTTPBadRequest(explanation=e.format_message()) except exception.InstanceIsLocked as e: raise exc.HTTPConflict(explanation=e.format_message()) except exception.InstanceInvalidState as state_error: common.raise_http_conflict_for_instance_invalid_state(state_error, 'swap_volume', server_id) - if not found: - msg = _("The volume was either invalid or not attached to the " - "instance.") - raise exc.HTTPNotFound(explanation=msg) - @wsgi.response(202) @extensions.expected_errors((400, 403, 404, 409)) def delete(self, req, server_id, id): @@ -448,41 +414,33 @@ class VolumeAttachmentController(wsgi.Controller): except exception.VolumeNotFound as e: raise exc.HTTPNotFound(explanation=e.format_message()) - bdms = objects.BlockDeviceMappingList.get_by_instance_uuid( - context, instance.uuid) - if not bdms: - msg = _("Instance %s is not attached.") % server_id + try: + bdm = objects.BlockDeviceMapping.get_by_volume_and_instance( + context, volume_id, instance.uuid) + except exception.VolumeBDMNotFound: + msg = (_("Instance %(instance)s is not attached " + "to volume %(volume)s") % + {'instance': server_id, 'volume': volume_id}) raise exc.HTTPNotFound(explanation=msg) - found = False - try: - for bdm in bdms: - if bdm.volume_id != volume_id: - continue - if bdm.is_root: - msg = _("Can't detach root device volume") - raise exc.HTTPForbidden(explanation=msg) - try: - self.compute_api.detach_volume(context, instance, volume) - found = True - break - except exception.InvalidVolume as e: - raise exc.HTTPBadRequest(explanation=e.format_message()) - except exception.InstanceUnknownCell as e: - raise exc.HTTPNotFound(explanation=e.format_message()) - except exception.InvalidInput as e: - raise exc.HTTPBadRequest(explanation=e.format_message()) + if bdm.is_root: + msg = _("Cannot detach a root device volume") + raise exc.HTTPForbidden(explanation=msg) + try: + self.compute_api.detach_volume(context, instance, volume) + except exception.InvalidVolume as e: + raise exc.HTTPBadRequest(explanation=e.format_message()) + except exception.InstanceUnknownCell as e: + raise exc.HTTPNotFound(explanation=e.format_message()) + except exception.InvalidInput as e: + raise exc.HTTPBadRequest(explanation=e.format_message()) except exception.InstanceIsLocked as e: raise exc.HTTPConflict(explanation=e.format_message()) except exception.InstanceInvalidState as state_error: common.raise_http_conflict_for_instance_invalid_state(state_error, 'detach_volume', server_id) - if not found: - msg = _("volume_id not found: %s") % volume_id - raise exc.HTTPNotFound(explanation=msg) - def _translate_snapshot_detail_view(context, vol): """Maps keys for snapshots details view.""" diff --git a/nova/tests/functional/api_sample_tests/test_volumes.py b/nova/tests/functional/api_sample_tests/test_volumes.py index 3bb36ed667db..36d962f8df2b 100644 --- a/nova/tests/functional/api_sample_tests/test_volumes.py +++ b/nova/tests/functional/api_sample_tests/test_volumes.py @@ -196,13 +196,16 @@ class VolumesSampleJsonTest(test_servers.ServersSampleBase): class VolumeAttachmentsSample(test_servers.ServersSampleBase): sample_dir = "os-volumes" + OLD_VOLUME_ID = 'a26887c6-c47b-4654-abb5-dfadf7d3f803' + NEW_VOLUME_ID = 'a26887c6-c47b-4654-abb5-dfadf7d3f805' + def _stub_db_bdms_get_all_by_instance(self, server_id): def fake_bdms_get_all_by_instance(context, instance_uuid, use_slave=False): bdms = [ fake_block_device.FakeDbBlockDeviceDict( - {'id': 1, 'volume_id': 'a26887c6-c47b-4654-abb5-dfadf7d3f803', + {'id': 1, 'volume_id': self.OLD_VOLUME_ID, 'instance_uuid': server_id, 'source_type': 'volume', 'destination_type': 'volume', 'device_name': '/dev/sdd'}), fake_block_device.FakeDbBlockDeviceDict( @@ -215,6 +218,16 @@ class VolumeAttachmentsSample(test_servers.ServersSampleBase): self.stub_out('nova.db.block_device_mapping_get_all_by_instance', fake_bdms_get_all_by_instance) + def fake_bdm_get_by_volume_and_instance( + self, ctxt, volume_id, instance_uuid, expected_attrs=None): + return objects.BlockDeviceMapping._from_db_object( + ctxt, objects.BlockDeviceMapping(), + fake_block_device.FakeDbBlockDeviceDict( + {'id': 1, 'volume_id': self.OLD_VOLUME_ID, + 'instance_uuid': instance_uuid, 'source_type': 'volume', + 'destination_type': 'volume', 'device_name': '/dev/sdd'}) + ) + def _stub_compute_api_get(self): def fake_compute_api_get(self, context, instance_id, @@ -226,8 +239,6 @@ class VolumeAttachmentsSample(test_servers.ServersSampleBase): def test_attach_volume_to_server(self): self.stub_out('nova.volume.cinder.API.get', fakes.stub_volume_get) - self.stub_out('nova.volume.cinder.API.check_attach', - lambda *a, **k: None) self.stub_out('nova.volume.cinder.API.reserve_volume', lambda *a, **k: None) device_name = '/dev/vdd' @@ -239,12 +250,9 @@ class VolumeAttachmentsSample(test_servers.ServersSampleBase): self.stub_out( 'nova.compute.manager.ComputeManager.attach_volume', lambda *a, **k: None) - self.stub_out( - 'nova.objects.BlockDeviceMapping.get_by_volume_and_instance', - classmethod(lambda *a, **k: None)) volume = fakes.stub_volume_get(None, context.get_admin_context(), - 'a26887c6-c47b-4654-abb5-dfadf7d3f803') + self.OLD_VOLUME_ID) subs = { 'volume_id': volume['id'], 'device': device_name @@ -268,41 +276,44 @@ class VolumeAttachmentsSample(test_servers.ServersSampleBase): def test_volume_attachment_detail(self): server_id = self._post_server() - attach_id = "a26887c6-c47b-4654-abb5-dfadf7d3f803" - self._stub_db_bdms_get_all_by_instance(server_id) + self.stub_out( + 'nova.objects.BlockDeviceMapping.get_by_volume_and_instance', + self.fake_bdm_get_by_volume_and_instance) self._stub_compute_api_get() response = self._do_get('servers/%s/os-volume_attachments/%s' - % (server_id, attach_id)) + % (server_id, self.OLD_VOLUME_ID)) self._verify_response('volume-attachment-detail-resp', {}, response, 200) def test_volume_attachment_delete(self): server_id = self._post_server() - attach_id = "a26887c6-c47b-4654-abb5-dfadf7d3f803" - self._stub_db_bdms_get_all_by_instance(server_id) + self.stub_out( + 'nova.objects.BlockDeviceMapping.get_by_volume_and_instance', + self.fake_bdm_get_by_volume_and_instance) self._stub_compute_api_get() self.stub_out('nova.volume.cinder.API.get', fakes.stub_volume_get) self.stub_out('nova.compute.api.API.detach_volume', lambda *a, **k: None) response = self._do_delete('servers/%s/os-volume_attachments/%s' - % (server_id, attach_id)) + % (server_id, self.OLD_VOLUME_ID)) self.assertEqual(202, response.status_code) self.assertEqual('', response.text) def test_volume_attachment_update(self): self.stub_out('nova.volume.cinder.API.get', fakes.stub_volume_get) subs = { - 'volume_id': 'a26887c6-c47b-4654-abb5-dfadf7d3f805' + 'volume_id': self.NEW_VOLUME_ID } server_id = self._post_server() - attach_id = 'a26887c6-c47b-4654-abb5-dfadf7d3f803' - self._stub_db_bdms_get_all_by_instance(server_id) + self.stub_out( + 'nova.objects.BlockDeviceMapping.get_by_volume_and_instance', + self.fake_bdm_get_by_volume_and_instance) self._stub_compute_api_get() self.stub_out('nova.volume.cinder.API.get', fakes.stub_volume_get) self.stub_out('nova.compute.api.API.swap_volume', lambda *a, **k: None) response = self._do_put('servers/%s/os-volume_attachments/%s' - % (server_id, attach_id), + % (server_id, self.OLD_VOLUME_ID), 'update-volume-req', subs) self.assertEqual(202, response.status_code) diff --git a/nova/tests/unit/api/openstack/compute/test_volumes.py b/nova/tests/unit/api/openstack/compute/test_volumes.py index 025206b8e835..84c69d11ddbd 100644 --- a/nova/tests/unit/api/openstack/compute/test_volumes.py +++ b/nova/tests/unit/api/openstack/compute/test_volumes.py @@ -35,7 +35,6 @@ import nova.conf from nova import context from nova import exception from nova import objects -from nova.objects import base from nova import test from nova.tests.unit.api.openstack import fakes from nova.tests.unit import fake_block_device @@ -44,9 +43,13 @@ from nova.volume import cinder CONF = nova.conf.CONF +# This is the server ID. FAKE_UUID = 'aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa' +# This is the old volume ID (to swap from). FAKE_UUID_A = '00000000-aaaa-aaaa-aaaa-000000000000' +# This is the new volume ID (to swap to). FAKE_UUID_B = 'bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb' +# This is a volume that is not found. FAKE_UUID_C = 'cccccccc-cccc-cccc-cccc-cccccccccccc' IMAGE_UUID = 'c905cedb-7281-47e4-8a62-f26bc5fc4c77' @@ -57,10 +60,15 @@ def fake_get_instance(self, context, instance_id, expected_attrs=None): def fake_get_volume(self, context, id): - return {'id': FAKE_UUID_A, - 'status': 'available', - 'attach_status': 'detached' - } + if id == FAKE_UUID_A: + status = 'in-use' + attach_status = 'attached' + elif id == FAKE_UUID_B: + status = 'available' + attach_status = 'detached' + else: + raise exception.VolumeNotFound(volume_id=id) + return {'id': id, 'status': status, 'attach_status': attach_status} def fake_attach_volume(self, context, instance, volume_id, device, tag=None): @@ -71,9 +79,9 @@ def fake_detach_volume(self, context, instance, volume): pass -def fake_swap_volume(self, context, instance, - old_volume_id, new_volume_id): - pass +def fake_swap_volume(self, context, instance, old_volume, new_volume): + if old_volume['id'] != FAKE_UUID_A: + raise exception.VolumeBDMNotFound(volume_id=old_volume['id']) def fake_create_snapshot(self, context, volume, name, description): @@ -101,29 +109,21 @@ def fake_compute_volume_snapshot_create(self, context, volume_id, @classmethod -def fake_bdm_list_get_by_instance_uuid(cls, context, instance_uuid): - db_list = [fake_block_device.FakeDbBlockDeviceDict( - {'id': 1, - 'instance_uuid': instance_uuid, - 'device_name': '/dev/fake0', - 'delete_on_termination': 'False', - 'source_type': 'volume', - 'destination_type': 'volume', - 'snapshot_id': None, - 'volume_id': FAKE_UUID_A, - 'volume_size': 1}), - fake_block_device.FakeDbBlockDeviceDict( - {'id': 2, - 'instance_uuid': instance_uuid, - 'device_name': '/dev/fake1', - 'delete_on_termination': 'False', - 'source_type': 'volume', - 'destination_type': 'volume', - 'snapshot_id': None, - 'volume_id': FAKE_UUID_B, - 'volume_size': 1})] - item_cls = objects.BlockDeviceMapping - return base.obj_make_list(context, cls(), item_cls, db_list) +def fake_bdm_get_by_volume_and_instance(cls, ctxt, volume_id, instance_uuid): + if volume_id != FAKE_UUID_A: + raise exception.VolumeBDMNotFound(volume_id=volume_id) + db_bdm = fake_block_device.FakeDbBlockDeviceDict( + {'id': 1, + 'instance_uuid': instance_uuid, + 'device_name': '/dev/fake0', + 'delete_on_termination': 'False', + 'source_type': 'volume', + 'destination_type': 'volume', + 'snapshot_id': None, + 'volume_id': FAKE_UUID_A, + 'volume_size': 1}) + return objects.BlockDeviceMapping._from_db_object( + ctxt, objects.BlockDeviceMapping(), db_bdm) class BootFromVolumeTest(test.TestCase): @@ -344,9 +344,9 @@ class VolumeAttachTestsV21(test.NoDBTestCase): def setUp(self): super(VolumeAttachTestsV21, self).setUp() - self.stub_out('nova.objects.BlockDeviceMappingList' - '.get_by_instance_uuid', - fake_bdm_list_get_by_instance_uuid) + self.stub_out('nova.objects.BlockDeviceMapping' + '.get_by_volume_and_instance', + fake_bdm_get_by_volume_and_instance) self.stubs.Set(compute_api.API, 'get', fake_get_instance) self.stubs.Set(cinder.API, 'get', fake_get_volume) self.context = context.get_admin_context() @@ -377,8 +377,10 @@ class VolumeAttachTestsV21(test.NoDBTestCase): FAKE_UUID, FAKE_UUID_A) - @mock.patch.object(objects.BlockDeviceMappingList, - 'get_by_instance_uuid', return_value=None) + @mock.patch.object(objects.BlockDeviceMapping, + 'get_by_volume_and_instance', + side_effect=exception.VolumeBDMNotFound( + volume_id=FAKE_UUID_A)) def test_show_no_bdms(self, mock_mr): self.assertRaises(exc.HTTPNotFound, self.attachments.show,