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

Make cinder.API methods accept volume_id instead of the whole volume object.
This will remove redundant roundtrip to get the volume before
passing it to other methods as in fact they only need the id.

Fixes bug 1172297

Change-Id: I5e7f944c1c29b2f211ece2ef86c0959c81e806df

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 ffb2b4948..89d32a49d 100644
--- a/nova/tests/api/ec2/test_cinder_cloud.py
+++ b/nova/tests/api/ec2/test_cinder_cloud.py
@@ -371,12 +371,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
@@ -441,7 +441,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):
@@ -746,10 +746,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 83372b251..3ab0d379e 100644
--- a/nova/tests/api/openstack/compute/contrib/test_volumes.py
+++ b/nova/tests/api/openstack/compute/contrib/test_volumes.py
@@ -199,7 +199,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)
@@ -212,7 +212,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 c5d57ecbb..ef3474249 100644
--- a/nova/tests/api/openstack/compute/test_server_actions.py
+++ b/nova/tests/api/openstack/compute/test_server_actions.py
@@ -797,7 +797,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 0b1ba5a6c..14e499a31 100644
--- a/nova/tests/api/openstack/fakes.py
+++ b/nova/tests/api/openstack/fakes.py
@@ -571,7 +571,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)
 
 
@@ -601,13 +601,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 4878397cc..edf4a5261 100644
--- a/nova/tests/compute/test_compute.py
+++ b/nova/tests/compute/test_compute.py
@@ -7044,11 +7044,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)
@@ -7060,7 +7055,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):
@@ -7076,18 +7070,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 607f1444d..bec9bfb15 100644
--- a/nova/tests/fake_volume.py
+++ b/nova/tests/fake_volume.py
@@ -177,9 +177,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':
@@ -199,9 +200,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'
@@ -215,9 +216,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'
@@ -237,7 +238,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)
@@ -255,32 +257,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 188f96055..d331b5aaf 100644
--- a/nova/tests/integrated/test_api_samples.py
+++ b/nova/tests/integrated/test_api_samples.py
@@ -3636,7 +3636,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,