Prevent volume-attach/detach from instances in rescue state

Rescue is supposed to just be a way to log into a wayward instance
if something goes wrong with the base image that may have had some data
(logfiles etc.) and make it possible to grab that  - block devices are
assumed to be accessible by re-attaching them, and are considered
persistant so no need for rescue there.

Since there is no need to attach a volume in rescue mode, and detaching a
volume that was previously attached doesn't work, just ban volume
attach/detach in rescue state.

Fixes bug 1126187

Change-Id: Ifdf164155942cdeb2bbdfcd1dce0dd2e125b507c

4 files changed, 74 insertions, 9 deletions

diff --git a/nova/api/openstack/compute/contrib/volumes.py b/nova/api/openstack/compute/contrib/volumes.py
index 760dc953a..b58a7b442 100644
--- a/nova/api/openstack/compute/contrib/volumes.py
+++ b/nova/api/openstack/compute/contrib/volumes.py
@@ -402,6 +402,9 @@ class VolumeAttachmentController(wsgi.Controller):
                                                     volume_id, device)
         except exception.NotFound:
             raise exc.HTTPNotFound()
+        except exception.InstanceInvalidState as state_error:
+            common.raise_http_conflict_for_instance_invalid_state(state_error,
+                    'attach_volume')
 
         # The attach is async
         attachment = {}
@@ -446,12 +449,16 @@ class VolumeAttachmentController(wsgi.Controller):
             raise exc.HTTPNotFound()
 
         found = False
-        for bdm in bdms:
-            if bdm['volume_id'] == volume_id:
-                self.compute_api.detach_volume(context,
-                    volume_id=volume_id)
-                found = True
-                break
+        try:
+            for bdm in bdms:
+                if bdm['volume_id'] == volume_id:
+                    self.compute_api.detach_volume(context,
+                        volume_id=volume_id)
+                    found = True
+                    break
+        except exception.InstanceInvalidState as state_error:
+            common.raise_http_conflict_for_instance_invalid_state(state_error,
+                    'detach_volume')
 
         if not found:
             raise exc.HTTPNotFound()
diff --git a/nova/compute/api.py b/nova/compute/api.py
index 0603b929d..4082e3f3e 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -2283,6 +2283,10 @@ class API(base.Base):
 
     @wrap_check_policy
     @check_instance_lock
+    @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.PAUSED,
+                                    vm_states.SUSPENDED, vm_states.STOPPED,
+                                    vm_states.RESIZED, vm_states.SOFT_DELETED],
+                          task_state=None)
     def attach_volume(self, context, instance, volume_id, device=None):
         """Attach an existing volume to an existing instance."""
         # NOTE(vish): Fail fast if the device is not going to pass. This
@@ -2312,6 +2316,10 @@ class API(base.Base):
         return device
 
     @check_instance_lock
+    @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.PAUSED,
+                                    vm_states.SUSPENDED, vm_states.STOPPED,
+                                    vm_states.RESIZED, vm_states.SOFT_DELETED],
+                          task_state=None)
     def _detach_volume(self, context, instance, volume_id):
         check_policy(context, 'detach_volume', instance)
 
@@ -2362,7 +2370,7 @@ class API(base.Base):
     @wrap_check_policy
     @check_instance_lock
     @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.PAUSED,
-                          vm_states.SUSPENDED, vm_states.STOPPED],
+                                    vm_states.SUSPENDED, vm_states.STOPPED],
                           task_state=None)
     def delete_instance_metadata(self, context, instance, key):
         """Delete the given metadata item from an instance."""
@@ -2376,7 +2384,7 @@ class API(base.Base):
     @wrap_check_policy
     @check_instance_lock
     @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.PAUSED,
-                          vm_states.SUSPENDED, vm_states.STOPPED],
+                                    vm_states.SUSPENDED, vm_states.STOPPED],
                           task_state=None)
     def update_instance_metadata(self, context, instance,
                                  metadata, delete=False):
diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py
index e94d8b788..6fc24764b 100644
--- a/nova/tests/compute/test_compute.py
+++ b/nova/tests/compute/test_compute.py
@@ -5982,10 +5982,56 @@ class ComputeAPITestCase(BaseTestCase):
         self.assertRaises(exception.InvalidDevicePath,
                 self.compute_api.attach_volume,
                 self.context,
-                {'locked': False},
+                {'locked': False, 'vm_state': vm_states.ACTIVE},
                 None,
                 '/invalid')
 
+    def test_no_attach_volume_in_rescue_state(self):
+        def fake(*args, **kwargs):
+            pass
+
+        def fake_volume_get(self, context, volume_id):
+            return {'id': volume_id}
+
+        self.stubs.Set(cinder.API, 'get', fake_volume_get)
+        self.stubs.Set(cinder.API, 'check_attach', fake)
+        self.stubs.Set(cinder.API, 'reserve_volume', fake)
+
+        self.assertRaises(exception.InstanceInvalidState,
+                self.compute_api.attach_volume,
+                self.context,
+                {'uuid': 'fake_uuid', 'locked': False,
+                'vm_state': vm_states.RESCUED},
+                None,
+                '/dev/vdb')
+
+    def test_no_detach_volume_in_rescue_state(self):
+        # Ensure volume can be detached from instance
+
+        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)
+
+        self.assertRaises(exception.InstanceInvalidState,
+                self.compute_api.detach_volume,
+                self.context, 1)
+
     def test_vnc_console(self):
         # Make sure we can a vnc console for an instance.
 
diff --git a/nova/tests/compute/test_compute_cells.py b/nova/tests/compute/test_compute_cells.py
index 8ba35e033..cbc7aba43 100644
--- a/nova/tests/compute/test_compute_cells.py
+++ b/nova/tests/compute/test_compute_cells.py
@@ -168,6 +168,10 @@ class CellsComputeAPITestCase(test_compute.ComputeAPITestCase):
         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.")