Merge "Refactor nova.volume.cinder.API to reduce roundtrips with Cinder"

8 files changed, 58 insertions, 61 deletions

diff --git a/nova/tests/api/ec2/test_cinder_cloud.py b/nova/tests/api/ec2/test_cinder_cloud.py
index 60797c8f7..5d02a28d7 100644
--- a/nova/tests/api/ec2/test_cinder_cloud.py
+++ b/nova/tests/api/ec2/test_cinder_cloud.py
@@ -401,12 +401,12 @@ class CinderCloudTestCase(test.TestCase):
                                                         **kwargs)
                 if 'snapshot_id' in values:
                     self.volume_api.create_snapshot(self.context,
-                                                    vol,
+                                                    vol['id'],
                                                     'snapshot-bdm',
                                                     'fake snap for bdm tests',
                                                     values['snapshot_id'])
 
-                self.volume_api.attach(self.context, vol,
+                self.volume_api.attach(self.context, vol['id'],
                                        instance_uuid, bdm['device_name'])
                 volumes.append(vol)
         return volumes
@@ -471,7 +471,7 @@ class CinderCloudTestCase(test.TestCase):
 
     def _tearDownBlockDeviceMapping(self, inst1, inst2, volumes):
         for vol in volumes:
-            self.volume_api.delete(self.context, vol)
+            self.volume_api.delete(self.context, vol['id'])
         for uuid in (inst1['uuid'], inst2['uuid']):
             for bdm in db.block_device_mapping_get_all_by_instance(
                 self.context, uuid):
@@ -776,10 +776,10 @@ class CinderCloudTestCase(test.TestCase):
             self.assertTrue(str(vol['id']) == str(vol1_uuid) or
                 str(vol['id']) == str(vol2_uuid))
             if str(vol['id']) == str(vol1_uuid):
-                self.volume_api.attach(self.context, vol,
+                self.volume_api.attach(self.context, vol['id'],
                                        instance_uuid, '/dev/sdb')
             elif str(vol['id']) == str(vol2_uuid):
-                self.volume_api.attach(self.context, vol,
+                self.volume_api.attach(self.context, vol['id'],
                                        instance_uuid, '/dev/sdc')
 
         vol = self.volume_api.get(self.context, vol1_uuid)
diff --git a/nova/tests/api/openstack/compute/contrib/test_volumes.py b/nova/tests/api/openstack/compute/contrib/test_volumes.py
index ebac609f6..d1d7210f0 100644
--- a/nova/tests/api/openstack/compute/contrib/test_volumes.py
+++ b/nova/tests/api/openstack/compute/contrib/test_volumes.py
@@ -217,7 +217,7 @@ class VolumeApiTest(test.TestCase):
         self.assertEqual(resp.status_int, 200)
 
     def test_volume_show_no_volume(self):
-        self.stubs.Set(cinder.API, "get", fakes.stub_volume_get_notfound)
+        self.stubs.Set(cinder.API, "get", fakes.stub_volume_notfound)
 
         req = webob.Request.blank('/v2/fake/os-volumes/456')
         resp = req.get_response(self.app)
@@ -230,7 +230,7 @@ class VolumeApiTest(test.TestCase):
         self.assertEqual(resp.status_int, 202)
 
     def test_volume_delete_no_volume(self):
-        self.stubs.Set(cinder.API, "get", fakes.stub_volume_get_notfound)
+        self.stubs.Set(cinder.API, "delete", fakes.stub_volume_notfound)
 
         req = webob.Request.blank('/v2/fake/os-volumes/456')
         req.method = 'DELETE'
diff --git a/nova/tests/api/openstack/compute/test_server_actions.py b/nova/tests/api/openstack/compute/test_server_actions.py
index 7347aa169..3b8833dd7 100644
--- a/nova/tests/api/openstack/compute/test_server_actions.py
+++ b/nova/tests/api/openstack/compute/test_server_actions.py
@@ -838,7 +838,7 @@ class ServerActionsControllerTest(test.TestCase):
         self.mox.StubOutWithMock(self.controller.compute_api, 'volume_api')
         volume_api = self.controller.compute_api.volume_api
         volume_api.get(mox.IgnoreArg(), volume['id']).AndReturn(volume)
-        volume_api.create_snapshot_force(mox.IgnoreArg(), volume,
+        volume_api.create_snapshot_force(mox.IgnoreArg(), volume['id'],
                 mox.IgnoreArg(), mox.IgnoreArg()).AndReturn(snapshot)
 
         self.mox.ReplayAll()
diff --git a/nova/tests/api/openstack/fakes.py b/nova/tests/api/openstack/fakes.py
index 26b837f52..6139da3f4 100644
--- a/nova/tests/api/openstack/fakes.py
+++ b/nova/tests/api/openstack/fakes.py
@@ -581,7 +581,7 @@ def stub_volume_get(self, context, volume_id):
     return stub_volume(volume_id)
 
 
-def stub_volume_get_notfound(self, context, volume_id):
+def stub_volume_notfound(self, context, volume_id):
     raise exc.VolumeNotFound(volume_id=volume_id)
 
 
@@ -611,13 +611,13 @@ def stub_snapshot(id, **kwargs):
     return snapshot
 
 
-def stub_snapshot_create(self, context, volume, name, description):
-    return stub_snapshot(100, volume_id=volume['id'], display_name=name,
+def stub_snapshot_create(self, context, volume_id, name, description):
+    return stub_snapshot(100, volume_id=volume_id, display_name=name,
                          display_description=description)
 
 
-def stub_snapshot_delete(self, context, snapshot):
-    if snapshot['id'] == '-1':
+def stub_snapshot_delete(self, context, snapshot_id):
+    if snapshot_id == '-1':
         raise exc.NotFound
 
 
diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py
index 2adb07bdb..cef9d2cb3 100644
--- a/nova/tests/compute/test_compute.py
+++ b/nova/tests/compute/test_compute.py
@@ -7268,11 +7268,6 @@ class ComputeAPITestCase(BaseTestCase):
         def fake_roll_detaching(*args, **kwargs):
             called['fake_roll_detaching'] = True
 
-        def fake_volume_get(self, context, volume_id):
-            called['fake_volume_get'] = True
-            return {'id': volume_id, 'attach_status': 'in-use'}
-
-        self.stubs.Set(cinder.API, 'get', fake_volume_get)
         self.stubs.Set(cinder.API, 'roll_detaching', fake_roll_detaching)
         self.stubs.Set(self.compute, "_get_instance_volume_bdm",
                        fake_get_instance_volume_bdm)
@@ -7284,7 +7279,6 @@ class ComputeAPITestCase(BaseTestCase):
         self.assertRaises(AttributeError, self.compute.detach_volume,
                           self.context, 1, instance)
         self.assertTrue(called.get('fake_libvirt_driver_instance_exists'))
-        self.assertTrue(called.get('fake_volume_get'))
         self.assertTrue(called.get('fake_roll_detaching'))
 
     def test_terminate_with_volumes(self):
@@ -7300,18 +7294,18 @@ class ComputeAPITestCase(BaseTestCase):
                   }
         db.block_device_mapping_create(admin, values)
 
-        def fake_volume_get(self, context, volume):
+        def fake_volume_get(self, context, volume_id):
             return {'id': volume_id}
         self.stubs.Set(cinder.API, "get", fake_volume_get)
 
         # Stub out and record whether it gets detached
         result = {"detached": False}
 
-        def fake_detach(self, context, volume):
-            result["detached"] = volume["id"] == volume_id
+        def fake_detach(self, context, volume_id_param):
+            result["detached"] = volume_id_param == volume_id
         self.stubs.Set(cinder.API, "detach", fake_detach)
 
-        def fake_terminate_connection(self, context, volume, connector):
+        def fake_terminate_connection(self, context, volume_id, connector):
             return {}
         self.stubs.Set(cinder.API, "terminate_connection",
                        fake_terminate_connection)
diff --git a/nova/tests/fake_volume.py b/nova/tests/fake_volume.py
index 9d9eb1fb6..8bb3258b6 100644
--- a/nova/tests/fake_volume.py
+++ b/nova/tests/fake_volume.py
@@ -178,9 +178,10 @@ class API(object):
     def get_all(self, context):
         return self.volume_list
 
-    def delete(self, context, volume):
-        LOG.info('deleting volume %s', volume['id'])
-        self.volume_list = [v for v in self.volume_list if v != volume]
+    def delete(self, context, volume_id):
+        LOG.info('deleting volume %s', volume_id)
+        self.volume_list = [v for v in self.volume_list
+                            if v['id'] != volume_id]
 
     def check_attach(self, context, volume, instance=None):
         if volume['status'] != 'available':
@@ -200,9 +201,9 @@ class API(object):
             msg = _("already detached")
             raise exception.InvalidVolume(reason=msg)
 
-    def attach(self, context, volume, instance_uuid, mountpoint):
-        LOG.info('attaching volume %s', volume['id'])
-        volume = self.get(context, volume['id'])
+    def attach(self, context, volume_id, instance_uuid, mountpoint):
+        LOG.info('attaching volume %s', volume_id)
+        volume = self.get(context, volume_id)
         volume['status'] = 'in-use'
         volume['mountpoint'] = mountpoint
         volume['attach_status'] = 'attached'
@@ -216,9 +217,9 @@ class API(object):
         del self.volume_list[:]
         del self.snapshot_list[:]
 
-    def detach(self, context, volume):
-        LOG.info('detaching volume %s', volume['id'])
-        volume = self.get(context, volume['id'])
+    def detach(self, context, volume_id):
+        LOG.info('detaching volume %s', volume_id)
+        volume = self.get(context, volume_id)
         volume['status'] = 'available'
         volume['mountpoint'] = None
         volume['attach_status'] = 'detached'
@@ -238,7 +239,8 @@ class API(object):
     def get_all_snapshots(self, context):
         return self.snapshot_list
 
-    def create_snapshot(self, context, volume, name, description, id=None):
+    def create_snapshot(self, context, volume_id, name, description, id=None):
+        volume = self.get(context, volume_id)
         snapshot = fake_snapshot(volume['id'], volume['size'],
                                  name, description, id)
         self.snapshot_list.append(snapshot.snap)
@@ -256,32 +258,34 @@ class API(object):
         self.snapshot_list.append(snapshot.snap)
         return snapshot.snap
 
-    def create_snapshot_force(self, context, volume,
+    def create_snapshot_force(self, context, volume_id,
                               name, description, id=None):
+        volume = self.get(context, volume_id)
         snapshot = fake_snapshot(volume['id'], volume['size'],
                                  name, description, id)
         self.snapshot_list.append(snapshot.snap)
         return snapshot.snap
 
-    def delete_snapshot(self, context, snapshot):
-        self.snapshot_list = [s for s in self.snapshot_list if s != snapshot]
+    def delete_snapshot(self, context, snapshot_id):
+        self.snapshot_list = [s for s in self.snapshot_list
+                              if s['id'] != snapshot_id]
 
-    def reserve_volume(self, context, volume):
-        LOG.info('reserving volume %s', volume['id'])
-        volume = self.get(context, volume['id'])
+    def reserve_volume(self, context, volume_id):
+        LOG.info('reserving volume %s', volume_id)
+        volume = self.get(context, volume_id)
         volume['status'] = 'attaching'
 
-    def unreserve_volume(self, context, volume):
-        LOG.info('unreserving volume %s', volume['id'])
-        volume = self.get(context, volume['id'])
+    def unreserve_volume(self, context, volume_id):
+        LOG.info('unreserving volume %s', volume_id)
+        volume = self.get(context, volume_id)
         volume['status'] = 'available'
 
-    def begin_detaching(self, context, volume):
-        LOG.info('beging detaching volume %s', volume['id'])
-        volume = self.get(context, volume['id'])
+    def begin_detaching(self, context, volume_id):
+        LOG.info('beging detaching volume %s', volume_id)
+        volume = self.get(context, volume_id)
         volume['status'] = 'detaching'
 
-    def roll_detaching(self, context, volume):
-        LOG.info('roll detaching volume %s', volume['id'])
-        volume = self.get(context, volume['id'])
+    def roll_detaching(self, context, volume_id):
+        LOG.info('roll detaching volume %s', volume_id)
+        volume = self.get(context, volume_id)
         volume['status'] = 'in-use'
diff --git a/nova/tests/integrated/test_api_samples.py b/nova/tests/integrated/test_api_samples.py
index dd18a4400..d33cda028 100644
--- a/nova/tests/integrated/test_api_samples.py
+++ b/nova/tests/integrated/test_api_samples.py
@@ -3654,7 +3654,6 @@ class SnapshotsSampleJsonTests(ApiSampleTestBase):
     def _create_snapshot(self):
         self.stubs.Set(cinder.API, "create_snapshot",
                        fakes.stub_snapshot_create)
-        self.stubs.Set(cinder.API, "get", fakes.stub_volume_get)
 
         response = self._do_post("os-snapshots",
                                  "snapshot-create-req",
diff --git a/nova/tests/volume/test_cinder.py b/nova/tests/volume/test_cinder.py
index 420fa2373..a235a526c 100644
--- a/nova/tests/volume/test_cinder.py
+++ b/nova/tests/volume/test_cinder.py
@@ -80,10 +80,10 @@ class CinderApiTestCase(test.TestCase):
         self.api.create(self.ctx, 1, '', '')
 
     def test_create_failed(self):
-        cinder.cinderclient(self.ctx).AndRaise(cinder_exception.NotFound(''))
+        cinder.cinderclient(self.ctx).AndRaise(cinder_exception.BadRequest(''))
         self.mox.ReplayAll()
 
-        self.assertRaises(exception.VolumeNotFound,
+        self.assertRaises(exception.InvalidInput,
                           self.api.create, self.ctx, 1, '', '')
 
     def test_get_all(self):
@@ -142,7 +142,7 @@ class CinderApiTestCase(test.TestCase):
         self.cinderclient.volumes.reserve('id1')
         self.mox.ReplayAll()
 
-        self.api.reserve_volume(self.ctx, {'id': 'id1'})
+        self.api.reserve_volume(self.ctx, 'id1')
 
     def test_unreserve_volume(self):
         cinder.cinderclient(self.ctx).AndReturn(self.cinderclient)
@@ -151,7 +151,7 @@ class CinderApiTestCase(test.TestCase):
         self.cinderclient.volumes.unreserve('id1')
         self.mox.ReplayAll()
 
-        self.api.unreserve_volume(self.ctx, {'id': 'id1'})
+        self.api.unreserve_volume(self.ctx, 'id1')
 
     def test_begin_detaching(self):
         cinder.cinderclient(self.ctx).AndReturn(self.cinderclient)
@@ -160,7 +160,7 @@ class CinderApiTestCase(test.TestCase):
         self.cinderclient.volumes.begin_detaching('id1')
         self.mox.ReplayAll()
 
-        self.api.begin_detaching(self.ctx, {'id': 'id1'})
+        self.api.begin_detaching(self.ctx, 'id1')
 
     def test_roll_detaching(self):
         cinder.cinderclient(self.ctx).AndReturn(self.cinderclient)
@@ -169,7 +169,7 @@ class CinderApiTestCase(test.TestCase):
         self.cinderclient.volumes.roll_detaching('id1')
         self.mox.ReplayAll()
 
-        self.api.roll_detaching(self.ctx, {'id': 'id1'})
+        self.api.roll_detaching(self.ctx, 'id1')
 
     def test_attach(self):
         cinder.cinderclient(self.ctx).AndReturn(self.cinderclient)
@@ -178,7 +178,7 @@ class CinderApiTestCase(test.TestCase):
         self.cinderclient.volumes.attach('id1', 'uuid', 'point')
         self.mox.ReplayAll()
 
-        self.api.attach(self.ctx, {'id': 'id1'}, 'uuid', 'point')
+        self.api.attach(self.ctx, 'id1', 'uuid', 'point')
 
     def test_detach(self):
         cinder.cinderclient(self.ctx).AndReturn(self.cinderclient)
@@ -187,7 +187,7 @@ class CinderApiTestCase(test.TestCase):
         self.cinderclient.volumes.detach('id1')
         self.mox.ReplayAll()
 
-        self.api.detach(self.ctx, {'id': 'id1'})
+        self.api.detach(self.ctx, 'id1')
 
     def test_initialize_connection(self):
         cinder.cinderclient(self.ctx).AndReturn(self.cinderclient)
@@ -196,7 +196,7 @@ class CinderApiTestCase(test.TestCase):
         self.cinderclient.volumes.initialize_connection('id1', 'connector')
         self.mox.ReplayAll()
 
-        self.api.initialize_connection(self.ctx, {'id': 'id1'}, 'connector')
+        self.api.initialize_connection(self.ctx, 'id1', 'connector')
 
     def test_terminate_connection(self):
         cinder.cinderclient(self.ctx).AndReturn(self.cinderclient)
@@ -205,7 +205,7 @@ class CinderApiTestCase(test.TestCase):
         self.cinderclient.volumes.terminate_connection('id1', 'connector')
         self.mox.ReplayAll()
 
-        self.api.terminate_connection(self.ctx, {'id': 'id1'}, 'connector')
+        self.api.terminate_connection(self.ctx, 'id1', 'connector')
 
     def test_delete(self):
         cinder.cinderclient(self.ctx).AndReturn(self.cinderclient)
@@ -214,7 +214,7 @@ class CinderApiTestCase(test.TestCase):
         self.cinderclient.volumes.delete('id1')
         self.mox.ReplayAll()
 
-        self.api.delete(self.ctx, {'id': 'id1'})
+        self.api.delete(self.ctx, 'id1')
 
     def test_update(self):
         self.assertRaises(NotImplementedError,
@@ -270,7 +270,7 @@ class CinderApiTestCase(test.TestCase):
         self.cinderclient.volume_snapshots.delete('id1')
         self.mox.ReplayAll()
 
-        self.api.delete_snapshot(self.ctx, {'id': 'id1'})
+        self.api.delete_snapshot(self.ctx, 'id1')
 
     def test_get_volume_metadata(self):
         self.assertRaises(NotImplementedError,