Merge "Cleanup unnecessary logic in os-volume_attachments controller code"

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(
+        try:
-                context, instance.uuid)
+            bdm = objects.BlockDeviceMapping.get_by_volume_and_instance(
-
+                context, volume_id, instance.uuid)
-        if not bdms:
+        except exception.VolumeBDMNotFound:
-            msg = _("Instance %s is not attached.") % server_id
+            msg = (_("Instance %(instance)s is not attached "
-            raise exc.HTTPNotFound(explanation=msg)
+                     "to volume %(volume)s") %
-
+                   {'instance': server_id, 'volume': volume_id})
-        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
             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:
+            self.compute_api.swap_volume(context, instance, old_volume,
-                if bdm.volume_id != old_volume_id:
+                                         new_volume)
-                    continue
+        except exception.VolumeBDMNotFound as e:
-                try:
+            raise exc.HTTPNotFound(explanation=e.format_message())
-                    self.compute_api.swap_volume(context, instance, old_volume,
+        except exception.InvalidVolume as e:
-                                                 new_volume)
+            raise exc.HTTPBadRequest(explanation=e.format_message())
-                    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())
         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(
+        try:
-                context, instance.uuid)
+            bdm = objects.BlockDeviceMapping.get_by_volume_and_instance(
-        if not bdms:
+                context, volume_id, instance.uuid)
-            msg = _("Instance %s is not attached.") % server_id
+        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
+        if bdm.is_root:
-        try:
+            msg = _("Cannot detach a root device volume")
-            for bdm in bdms:
+            raise exc.HTTPForbidden(explanation=msg)
-                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())
 
+        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."""

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_out(
-        self._stub_db_bdms_get_all_by_instance(server_id)
+            '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_out(
-        self._stub_db_bdms_get_all_by_instance(server_id)
+            '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_out(
-        self._stub_db_bdms_get_all_by_instance(server_id)
+            '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)

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,
+    if id == FAKE_UUID_A:
-            'status': 'available',
+        status = 'in-use'
-            'attach_status': 'detached'
+        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,
+def fake_swap_volume(self, context, instance, old_volume, new_volume):
-                     old_volume_id, new_volume_id):
+    if old_volume['id'] != FAKE_UUID_A:
-    pass
+        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):
+def fake_bdm_get_by_volume_and_instance(cls, ctxt, volume_id, instance_uuid):
-    db_list = [fake_block_device.FakeDbBlockDeviceDict(
+    if volume_id != FAKE_UUID_A:
-            {'id': 1,
+        raise exception.VolumeBDMNotFound(volume_id=volume_id)
-             'instance_uuid': instance_uuid,
+    db_bdm = fake_block_device.FakeDbBlockDeviceDict(
-             'device_name': '/dev/fake0',
+        {'id': 1,
-             'delete_on_termination': 'False',
+         'instance_uuid': instance_uuid,
-             'source_type': 'volume',
+         'device_name': '/dev/fake0',
-             'destination_type': 'volume',
+         'delete_on_termination': 'False',
-             'snapshot_id': None,
+         'source_type': 'volume',
-             'volume_id': FAKE_UUID_A,
+         'destination_type': 'volume',
-             'volume_size': 1}),
+         'snapshot_id': None,
-            fake_block_device.FakeDbBlockDeviceDict(
+         'volume_id': FAKE_UUID_A,
-            {'id': 2,
+         'volume_size': 1})
-             'instance_uuid': instance_uuid,
+    return objects.BlockDeviceMapping._from_db_object(
-             'device_name': '/dev/fake1',
+        ctxt, objects.BlockDeviceMapping(), db_bdm)
-             '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)
 
 
 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'
+        self.stub_out('nova.objects.BlockDeviceMapping'
-                      '.get_by_instance_uuid',
+                      '.get_by_volume_and_instance',
-                      fake_bdm_list_get_by_instance_uuid)
+                      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,
+    @mock.patch.object(objects.BlockDeviceMapping,
-                       'get_by_instance_uuid', return_value=None)
+                       '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,