Prevent metadata updates until instance is active.

This avoids a race condition where setting metadata too early in the
build process can cause a build failure due to the VM not existing
in Xen.

bug 1100866

Change-Id: I702128799997d91d5f0c0ac189f619bfdf011988

4 files changed, 90 insertions, 5 deletions

diff --git a/nova/api/openstack/compute/server_metadata.py b/nova/api/openstack/compute/server_metadata.py
index 023a054d0..0de5d536f 100644
--- a/nova/api/openstack/compute/server_metadata.py
+++ b/nova/api/openstack/compute/server_metadata.py
@@ -136,6 +136,10 @@ class Controller(object):
             raise exc.HTTPRequestEntityTooLarge(explanation=unicode(error),
                                                 headers={'Retry-After': 0})
 
+        except exception.InstanceInvalidState as state_error:
+            common.raise_http_conflict_for_instance_invalid_state(state_error,
+                    'update metadata')
+
     @wsgi.serializers(xml=common.MetaItemTemplate)
     def show(self, req, server_id, id):
         """Return a single metadata item."""
@@ -162,10 +166,15 @@ class Controller(object):
         try:
             server = self.compute_api.get(context, server_id)
             self.compute_api.delete_instance_metadata(context, server, id)
+
         except exception.InstanceNotFound:
             msg = _('Server does not exist')
             raise exc.HTTPNotFound(explanation=msg)
 
+        except exception.InstanceInvalidState as state_error:
+            common.raise_http_conflict_for_instance_invalid_state(state_error,
+                    'delete metadata')
+
 
 def create_resource():
     return wsgi.Resource(Controller())
diff --git a/nova/compute/api.py b/nova/compute/api.py
index 22ee82bbf..a08a01242 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -2160,6 +2160,9 @@ 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],
+                          task_state=None)
     def delete_instance_metadata(self, context, instance, key):
         """Delete the given metadata item from an instance."""
         self.db.instance_metadata_delete(context, instance['uuid'], key)
@@ -2171,6 +2174,9 @@ 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],
+                          task_state=None)
     def update_instance_metadata(self, context, instance,
                                  metadata, delete=False):
         """Updates or creates instance metadata.
diff --git a/nova/tests/api/openstack/compute/test_server_metadata.py b/nova/tests/api/openstack/compute/test_server_metadata.py
index 1e992c2a3..71fa9f3f3 100644
--- a/nova/tests/api/openstack/compute/test_server_metadata.py
+++ b/nova/tests/api/openstack/compute/test_server_metadata.py
@@ -21,6 +21,7 @@ import webob
 
 from nova.api.openstack.compute import server_metadata
 from nova.compute import rpcapi as compute_rpcapi
+from nova.compute import vm_states
 import nova.db
 from nova import exception
 from nova.openstack.common import cfg
@@ -75,14 +76,16 @@ def return_server(context, server_id):
     return {'id': server_id,
             'uuid': '0cc3346e-9fef-4445-abe6-5d2b2690ec64',
             'name': 'fake',
-            'locked': False}
+            'locked': False,
+            'vm_state': vm_states.ACTIVE}
 
 
 def return_server_by_uuid(context, server_uuid):
     return {'id': 1,
             'uuid': '0cc3346e-9fef-4445-abe6-5d2b2690ec64',
             'name': 'fake',
-            'locked': False}
+            'locked': False,
+            'vm_state': vm_states.ACTIVE}
 
 
 def return_server_nonexistent(context, server_id):
@@ -93,10 +96,9 @@ def fake_change_instance_metadata(self, context, instance, diff):
     pass
 
 
-class ServerMetaDataTest(test.TestCase):
-
+class BaseTest(test.TestCase):
     def setUp(self):
-        super(ServerMetaDataTest, self).setUp()
+        super(BaseTest, self).setUp()
         fakes.stub_out_key_pair_funcs(self.stubs)
         self.stubs.Set(nova.db, 'instance_get', return_server)
         self.stubs.Set(nova.db, 'instance_get_by_uuid',
@@ -112,6 +114,9 @@ class ServerMetaDataTest(test.TestCase):
         self.uuid = str(uuid.uuid4())
         self.url = '/v1.1/fake/servers/%s/metadata' % self.uuid
 
+
+class ServerMetaDataTest(BaseTest):
+
     def test_index(self):
         req = fakes.HTTPRequest.blank(self.url)
         res_dict = self.controller.index(req, self.uuid)
@@ -510,3 +515,50 @@ class ServerMetaDataTest(test.TestCase):
         req.body = jsonutils.dumps(data)
         self.assertRaises(webob.exc.HTTPBadRequest,
                           self.controller.update_all, req, self.uuid, data)
+
+
+class BadStateServerMetaDataTest(BaseTest):
+
+    def setUp(self):
+        super(BadStateServerMetaDataTest, self).setUp()
+        self.stubs.Set(nova.db, 'instance_get', self._return_server_in_build)
+        self.stubs.Set(nova.db, 'instance_get_by_uuid',
+                self._return_server_in_build_by_uuid)
+        self.stubs.Set(nova.db, 'instance_metadata_delete',
+                       delete_server_metadata)
+
+    def test_invalid_state_on_delete(self):
+        req = fakes.HTTPRequest.blank(self.url + '/key2')
+        req.method = 'DELETE'
+        self.assertRaises(webob.exc.HTTPConflict, self.controller.delete,
+                          req, self.uuid, 'key2')
+
+    def test_invalid_state_on_update_metadata(self):
+        self.stubs.Set(nova.db, 'instance_metadata_update',
+                       return_create_instance_metadata)
+        req = fakes.HTTPRequest.blank(self.url)
+        req.method = 'POST'
+        req.content_type = 'application/json'
+        expected = {
+            'metadata': {
+                'key1': 'updatedvalue',
+                'key29': 'newkey',
+            }
+        }
+        req.body = jsonutils.dumps(expected)
+        self.assertRaises(webob.exc.HTTPConflict, self.controller.update_all,
+                req, self.uuid, expected)
+
+    def _return_server_in_build(self, context, server_id):
+        return {'id': server_id,
+                'uuid': '0cc3346e-9fef-4445-abe6-5d2b2690ec64',
+                'name': 'fake',
+                'locked': False,
+                'vm_state': vm_states.BUILDING}
+
+    def _return_server_in_build_by_uuid(self, context, server_uuid):
+        return {'id': 1,
+                'uuid': '0cc3346e-9fef-4445-abe6-5d2b2690ec64',
+                'name': 'fake',
+                'locked': False,
+                'vm_state': vm_states.BUILDING}
diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py
index 596668048..5894e07b0 100644
--- a/nova/tests/compute/test_compute.py
+++ b/nova/tests/compute/test_compute.py
@@ -5302,6 +5302,24 @@ class ComputeAPITestCase(BaseTestCase):
 
         db.instance_destroy(_context, instance['uuid'])
 
+    def test_disallow_metadata_changes_during_building(self):
+        def fake_change_instance_metadata(inst, ctxt, diff, instance=None,
+                                          instance_uuid=None):
+            pass
+        self.stubs.Set(compute_rpcapi.ComputeAPI, 'change_instance_metadata',
+                       fake_change_instance_metadata)
+
+        instance = self._create_fake_instance({'vm_state': vm_states.BUILDING})
+        instance = dict(instance)
+
+        self.assertRaises(exception.InstanceInvalidState,
+                self.compute_api.delete_instance_metadata, self.context,
+                instance, "key")
+
+        self.assertRaises(exception.InstanceInvalidState,
+                self.compute_api.update_instance_metadata, self.context,
+                instance, "key")
+
     def test_get_instance_faults(self):
         # Get an instances latest fault.
         instance = self._create_fake_instance()