Change arguments to volume_detach()

Most compute API calls accept an instance object.  To be compatible with
cells, volume_detach() should as well.  Also change it to accept the
volume object, instead of just a volume ID.  This lets us proxy the
volume object to child cells a bit more easily.

This removes 2 skipTests that we have related to volumes and cells.

Fixes bug 1157408

There was also a check that if instance_get_by_uuid() returned no
instance, it VolumeUnattached should be raised.  However, this check
would never work as InstanceNotFound would have been raised.  This
patch also corrects that.

Change-Id: Ide35ad5d8e147f6bd21b5840b901363dab0eae38

5 files changed, 34 insertions, 41 deletions

diff --git a/nova/tests/api/ec2/test_cinder_cloud.py b/nova/tests/api/ec2/test_cinder_cloud.py
index 7b66da749..fd01c8bce 100644
--- a/nova/tests/api/ec2/test_cinder_cloud.py
+++ b/nova/tests/api/ec2/test_cinder_cloud.py
@@ -840,7 +840,7 @@ class CinderCloudTestCase(test.TestCase):
         self._assert_volume_attached(vol2, instance_uuid, '/dev/sdc')
 
         self.cloud.compute_api.detach_volume(self.context,
-                                             volume_id=vol1_uuid)
+                                             instance, vol1)
 
         vol1 = self.volume_api.get(self.context, vol1_uuid)
         self._assert_volume_detached(vol1)
diff --git a/nova/tests/api/openstack/compute/contrib/test_volumes.py b/nova/tests/api/openstack/compute/contrib/test_volumes.py
index 90e226eac..83372b251 100644
--- a/nova/tests/api/openstack/compute/contrib/test_volumes.py
+++ b/nova/tests/api/openstack/compute/contrib/test_volumes.py
@@ -68,11 +68,15 @@ def fake_get_instance(self, context, instance_id):
     return({'uuid': instance_id})
 
 
+def fake_get_volume(self, context, id):
+    return({'id': 'woot'})
+
+
 def fake_attach_volume(self, context, instance, volume_id, device):
     return()
 
 
-def fake_detach_volume(self, context, volume_id):
+def fake_detach_volume(self, context, instance, volume):
     return()
 
 
@@ -223,6 +227,7 @@ class VolumeAttachTests(test.TestCase):
                        'get_instance_bdms',
                        fake_get_instance_bdms)
         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()
         self.expected_show = {'volumeAttachment':
             {'device': '/dev/fake0',
diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py
index 21b5b5c96..6a52402cb 100644
--- a/nova/tests/compute/test_compute.py
+++ b/nova/tests/compute/test_compute.py
@@ -6186,26 +6186,12 @@ class ComputeAPITestCase(BaseTestCase):
         params = {'vm_state': vm_states.RESCUED}
         instance = self._create_fake_instance(params=params)
 
-        def fake(*args, **kwargs):
-            pass
-
-        def fake_volume_get(self, context, volume_id):
-            pass
-            return {'id': volume_id, 'attach_status': 'in-use',
-                    'instance_uuid': instance['uuid']}
-
-        def fake_rpc_detach_volume(self, context, **kwargs):
-            pass
-
-        self.stubs.Set(cinder.API, 'get', fake_volume_get)
-        self.stubs.Set(cinder.API, 'check_detach', fake)
-        self.stubs.Set(cinder.API, 'begin_detaching', fake)
-        self.stubs.Set(compute_rpcapi.ComputeAPI, 'detach_volume',
-                       fake_rpc_detach_volume)
+        volume = {'id': 1, 'attach_status': 'in-use',
+                  'instance_uuid': instance['uuid']}
 
         self.assertRaises(exception.InstanceInvalidState,
                 self.compute_api.detach_volume,
-                self.context, 1)
+                self.context, instance, volume)
 
     def test_vnc_console(self):
         # Make sure we can a vnc console for an instance.
@@ -6437,9 +6423,10 @@ class ComputeAPITestCase(BaseTestCase):
 
     def test_detach_volume(self):
         # Ensure volume can be detached from instance
-
         called = {}
         instance = self._create_fake_instance()
+        volume = {'id': 1, 'attach_status': 'in-use',
+                  'instance_uuid': instance['uuid']}
 
         def fake_check_detach(*args, **kwargs):
             called['fake_check_detach'] = True
@@ -6447,35 +6434,43 @@ class ComputeAPITestCase(BaseTestCase):
         def fake_begin_detaching(*args, **kwargs):
             called['fake_begin_detaching'] = True
 
-        def fake_volume_get(self, context, volume_id):
-            called['fake_volume_get'] = True
-            return {'id': volume_id, 'attach_status': 'in-use',
-                    'instance_uuid': instance['uuid']}
-
         def fake_rpc_detach_volume(self, context, **kwargs):
             called['fake_rpc_detach_volume'] = True
 
-        self.stubs.Set(cinder.API, 'get', fake_volume_get)
         self.stubs.Set(cinder.API, 'check_detach', fake_check_detach)
         self.stubs.Set(cinder.API, 'begin_detaching', fake_begin_detaching)
         self.stubs.Set(compute_rpcapi.ComputeAPI, 'detach_volume',
                        fake_rpc_detach_volume)
 
-        self.compute_api.detach_volume(self.context, 1)
-        self.assertTrue(called.get('fake_volume_get'))
+        self.compute_api.detach_volume(self.context,
+                instance, volume)
         self.assertTrue(called.get('fake_check_detach'))
         self.assertTrue(called.get('fake_begin_detaching'))
         self.assertTrue(called.get('fake_rpc_detach_volume'))
 
     def test_detach_invalid_volume(self):
         # Ensure exception is raised while detaching an un-attached volume
+        instance = {'uuid': 'uuid1',
+                    'locked': False,
+                    'vm_state': vm_states.ACTIVE}
+        volume = {'id': 1, 'attach_status': 'detached'}
 
-        def fake_volume_get(self, context, volume_id):
-            return {'id': volume_id, 'attach_status': 'detached'}
-
-        self.stubs.Set(cinder.API, 'get', fake_volume_get)
         self.assertRaises(exception.InvalidVolume,
-                          self.compute_api.detach_volume, self.context, 1)
+                          self.compute_api.detach_volume, self.context,
+                          instance, volume)
+
+    def test_detach_unattached_volume(self):
+        # Ensure exception is raised when volume's idea of attached
+        # instance doesn't match.
+        instance = {'uuid': 'uuid1',
+                    'locked': False,
+                    'vm_state': vm_states.ACTIVE}
+        volume = {'id': 1, 'attach_status': 'in-use',
+                  'instance_uuid': 'uuid2'}
+
+        self.assertRaises(exception.VolumeUnattached,
+                          self.compute_api.detach_volume, self.context,
+                          instance, volume)
 
     def test_detach_volume_libvirt_is_down(self):
         # Ensure rollback during detach if libvirt goes down
diff --git a/nova/tests/compute/test_compute_cells.py b/nova/tests/compute/test_compute_cells.py
index 0b93652f9..e358e278e 100644
--- a/nova/tests/compute/test_compute_cells.py
+++ b/nova/tests/compute/test_compute_cells.py
@@ -186,14 +186,6 @@ class CellsComputeAPITestCase(test_compute.ComputeAPITestCase):
     def test_backup(self):
         return super(CellsComputeAPITestCase, self).test_backup()
 
-    def test_detach_volume(self):
-        self.skipTest("This test is failing due to TypeError: "
-                      "detach_volume() takes exactly 3 arguments (4 given).")
-
-    def test_no_detach_volume_in_rescue_state(self):
-        self.skipTest("This test is failing due to TypeError: "
-                      "detach_volume() takes exactly 3 arguments (4 given).")
-
     def test_evacuate(self):
         self.skipTest("Test is incompatible with cells.")
 
diff --git a/nova/tests/integrated/test_api_samples.py b/nova/tests/integrated/test_api_samples.py
index c647e0292..8e816766d 100644
--- a/nova/tests/integrated/test_api_samples.py
+++ b/nova/tests/integrated/test_api_samples.py
@@ -3760,6 +3760,7 @@ class VolumeAttachmentsSampleJsonTest(ServersSampleBase):
         attach_id = "a26887c6-c47b-4654-abb5-dfadf7d3f803"
         self._stub_compute_api_get_instance_bdms(server_id)
         self._stub_compute_api_get()
+        self.stubs.Set(cinder.API, 'get', fakes.stub_volume_get)
         self.stubs.Set(compute_api.API, 'detach_volume', lambda *a, **k: None)
         response = self._do_delete('servers/%s/os-volume_attachments/%s'
                                    % (server_id, attach_id))