Merge lp:~rackspace-titan/nova/fix_rebuild into lp:~hudson-openstack/nova/trunk

[Dan Prince]

Update the rebuild_instance function in the compute manager so that it accepts the arguments that our current compute API sends.

Fixes to the SQLAlchmeny API such that metadata is saved on an instance_update. Added integration test to ensure that instance metadata is updated on a rebuild.

[Jay Pipes (jaypipes)]

ah, metadata. :) lgtm.

review: Approve

[Dan Prince (dan-prince)]

Add 'name' to that list as well.

v1.1 server rebuilds should also support the ability to optionally update the name of the server as well.

Setting to WIP until we get that in there too.

[Dan Prince (dan-prince)]

Updated so that 'name' can be set when doing a rebuild. Also added a fix to the way we update metadata so that it can be reset when doing a rebuild (if an empty dict is posted).

[Devin Carlen (devcamcar)]

lgtm, nice work :)

review: Approve

[Mark Washenberger (markwash)]

I don't love the "purge" flag. Do you think it would be cleaner if there were an instance_metadata_delete_all(context, instance_id)? Or perhaps you would prefer not to change the db/api.py interface.

[lp:~rackspace-titan/nova/fix_rebuild]

1123. By Dan Prince on 2011-05-31

Use a new instance_metadata_delete_all DB api call to delete existing
metadata when updating a server.

[Dan Prince (dan-prince)]

Mark,

Just swapped the "purge" flag for a instance_metadata_delete_all. I like this better too. Give it a look and let me know. Thanks.

[Mark Washenberger (markwash)]

Looking good to me. Also merges clean with recent trunk and all tests are passing.

review: Approve

=== modified file 'nova/api/openstack/servers.py'
--- nova/api/openstack/servers.py	2011-05-25 17:55:51 +0000
+++ nova/api/openstack/servers.py	2011-05-31 18:41:32 +0000
@@ -708,14 +708,16 @@
 
         image_id = common.get_id_from_href(image_ref)
         personalities = info["rebuild"].get("personality", [])
-        metadata = info["rebuild"].get("metadata", {})
+        metadata = info["rebuild"].get("metadata")
+        name = info["rebuild"].get("name")
 
-        self._validate_metadata(metadata)
+        if metadata:
+            self._validate_metadata(metadata)
         self._decode_personalities(personalities)
 
         try:
-            self.compute_api.rebuild(context, instance_id, image_id, metadata,
-                                     personalities)
+            self.compute_api.rebuild(context, instance_id, image_id, name,
+                                     metadata, personalities)
         except exception.BuildInProgress:
             msg = _("Instance %d is currently being rebuilt.") % instance_id
             LOG.debug(msg)

=== modified file 'nova/compute/api.py'
--- nova/compute/api.py	2011-05-25 20:58:40 +0000
+++ nova/compute/api.py	2011-05-31 18:41:32 +0000
@@ -530,7 +530,7 @@
         """Reboot the given instance."""
         self._cast_compute_message('reboot_instance', context, instance_id)
 
-    def rebuild(self, context, instance_id, image_id, metadata=None,
+    def rebuild(self, context, instance_id, image_id, name=None, metadata=None,
                 files_to_inject=None):
         """Rebuild the given instance with the provided metadata."""
         instance = db.api.instance_get(context, instance_id)
@@ -539,13 +539,16 @@
             msg = _("Instance already building")
             raise exception.BuildInProgress(msg)
 
-        metadata = metadata or {}
-        self._check_metadata_properties_quota(context, metadata)
-
         files_to_inject = files_to_inject or []
         self._check_injected_file_quota(context, files_to_inject)
 
-        self.db.instance_update(context, instance_id, {"metadata": metadata})
+        values = {}
+        if metadata is not None:
+            self._check_metadata_properties_quota(context, metadata)
+            values['metadata'] = metadata
+        if name is not None:
+            values['display_name'] = name
+        self.db.instance_update(context, instance_id, values)
 
         rebuild_params = {
             "image_id": image_id,

=== modified file 'nova/compute/manager.py'
--- nova/compute/manager.py	2011-05-25 19:05:20 +0000
+++ nova/compute/manager.py	2011-05-31 18:41:32 +0000
@@ -331,7 +331,7 @@
 
     @exception.wrap_exception
     @checks_instance_lock
-    def rebuild_instance(self, context, instance_id, image_id):
+    def rebuild_instance(self, context, instance_id, **kwargs):
         """Destroy and re-make this instance.
 
         A 'rebuild' effectively purges all existing data from the system and
@@ -349,7 +349,8 @@
         self._update_state(context, instance_id, power_state.BUILDING)
 
         self.driver.destroy(instance_ref)
-        instance_ref.image_id = image_id
+        instance_ref.image_id = kwargs.get('image_id')
+        instance_ref.injected_files = kwargs.get('injected_files', [])
         self.driver.spawn(instance_ref)
 
         self._update_image_id(context, instance_id, image_id)

=== modified file 'nova/db/sqlalchemy/api.py'
--- nova/db/sqlalchemy/api.py	2011-05-27 04:37:39 +0000
+++ nova/db/sqlalchemy/api.py	2011-05-31 18:41:32 +0000
@@ -771,24 +771,25 @@
 
 
 ###################
-
-
-@require_context
-def instance_create(context, values):
-    """Create a new Instance record in the database.
-
-    context - request context object
-    values - dict containing column values.
-    """
-    metadata = values.get('metadata')
+def _metadata_refs(metadata_dict):
     metadata_refs = []
-    if metadata:
-        for k, v in metadata.iteritems():
+    if metadata_dict:
+        for k, v in metadata_dict.iteritems():
             metadata_ref = models.InstanceMetadata()
             metadata_ref['key'] = k
             metadata_ref['value'] = v
             metadata_refs.append(metadata_ref)
-    values['metadata'] = metadata_refs
+    return metadata_refs
+
+
+@require_context
+def instance_create(context, values):
+    """Create a new Instance record in the database.
+
+    context - request context object
+    values - dict containing column values.
+    """
+    values['metadata'] = _metadata_refs(values.get('metadata'))
 
     instance_ref = models.Instance()
     instance_ref.update(values)
@@ -1010,6 +1011,11 @@
 @require_context
 def instance_update(context, instance_id, values):
     session = get_session()
+    metadata = values.get('metadata')
+    if metadata is not None:
+        instance_metadata_delete_all(context, instance_id)
+        instance_metadata_update_or_create(context, instance_id,
+                                           values.pop('metadata'))
     with session.begin():
         instance_ref = instance_get(context, instance_id, session=session)
         instance_ref.update(values)
@@ -2626,6 +2632,17 @@
 
 
 @require_context
+def instance_metadata_delete_all(context, instance_id):
+    session = get_session()
+    session.query(models.InstanceMetadata).\
+        filter_by(instance_id=instance_id).\
+        filter_by(deleted=False).\
+        update({'deleted': True,
+                'deleted_at': datetime.datetime.utcnow(),
+                'updated_at': literal_column('updated_at')})
+
+
+@require_context
 def instance_metadata_get_item(context, instance_id, key):
     session = get_session()
 
@@ -2644,6 +2661,9 @@
 @require_context
 def instance_metadata_update_or_create(context, instance_id, metadata):
     session = get_session()
+
+    original_metadata = instance_metadata_get(context, instance_id)
+
     meta_ref = None
     for key, value in metadata.iteritems():
         try:
@@ -2655,4 +2675,5 @@
                             "instance_id": instance_id,
                             "deleted": 0})
         meta_ref.save(session=session)
+
     return metadata

=== modified file 'nova/tests/integrated/api/client.py'
--- nova/tests/integrated/api/client.py	2011-03-28 23:54:17 +0000
+++ nova/tests/integrated/api/client.py	2011-05-31 18:41:32 +0000
@@ -152,7 +152,10 @@
     def _decode_json(self, response):
         body = response.read()
         LOG.debug(_("Decoding JSON: %s") % (body))
-        return json.loads(body)
+        if body:
+            return json.loads(body)
+        else:
+            return ""
 
     def api_get(self, relative_uri, **kwargs):
         kwargs.setdefault('check_response_status', [200])
@@ -166,7 +169,7 @@
             headers['Content-Type'] = 'application/json'
             kwargs['body'] = json.dumps(body)
 
-        kwargs.setdefault('check_response_status', [200])
+        kwargs.setdefault('check_response_status', [200, 202])
         response = self.api_request(relative_uri, **kwargs)
         return self._decode_json(response)
 
@@ -185,6 +188,9 @@
     def post_server(self, server):
         return self.api_post('/servers', server)['server']
 
+    def post_server_action(self, server_id, data):
+        return self.api_post('/servers/%s/action' % server_id, data)
+
     def delete_server(self, server_id):
         return self.api_delete('/servers/%s' % server_id)
 

=== modified file 'nova/tests/integrated/test_servers.py'
--- nova/tests/integrated/test_servers.py	2011-04-15 19:36:52 +0000
+++ nova/tests/integrated/test_servers.py	2011-05-31 18:41:32 +0000
@@ -179,6 +179,112 @@
         # Cleanup
         self._delete_server(created_server_id)
 
+    def test_create_and_rebuild_server(self):
+        """Rebuild a server."""
+
+        # create a server with initially has no metadata
+        server = self._build_minimal_create_server_request()
+        server_post = {'server': server}
+        created_server = self.api.post_server(server_post)
+        LOG.debug("created_server: %s" % created_server)
+        self.assertTrue(created_server['id'])
+        created_server_id = created_server['id']
+
+        # rebuild the server with metadata
+        post = {}
+        post['rebuild'] = {
+            "imageRef": "https://localhost/v1.1/32278/images/2",
+            "name": "blah"
+        }
+
+        self.api.post_server_action(created_server_id, post)
+        LOG.debug("rebuilt server: %s" % created_server)
+        self.assertTrue(created_server['id'])
+
+        found_server = self.api.get_server(created_server_id)
+        self.assertEqual(created_server_id, found_server['id'])
+        self.assertEqual({}, found_server.get('metadata'))
+        self.assertEqual('blah', found_server.get('name'))
+
+        # Cleanup
+        self._delete_server(created_server_id)
+
+    def test_create_and_rebuild_server_with_metadata(self):
+        """Rebuild a server with metadata."""
+
+        # create a server with initially has no metadata
+        server = self._build_minimal_create_server_request()
+        server_post = {'server': server}
+        created_server = self.api.post_server(server_post)
+        LOG.debug("created_server: %s" % created_server)
+        self.assertTrue(created_server['id'])
+        created_server_id = created_server['id']
+
+        # rebuild the server with metadata
+        post = {}
+        post['rebuild'] = {
+            "imageRef": "https://localhost/v1.1/32278/images/2",
+            "name": "blah"
+        }
+
+        metadata = {}
+        for i in range(30):
+            metadata['key_%s' % i] = 'value_%s' % i
+
+        post['rebuild']['metadata'] = metadata
+
+        self.api.post_server_action(created_server_id, post)
+        LOG.debug("rebuilt server: %s" % created_server)
+        self.assertTrue(created_server['id'])
+
+        found_server = self.api.get_server(created_server_id)
+        self.assertEqual(created_server_id, found_server['id'])
+        self.assertEqual(metadata, found_server.get('metadata'))
+        self.assertEqual('blah', found_server.get('name'))
+
+        # Cleanup
+        self._delete_server(created_server_id)
+
+    def test_create_and_rebuild_server_with_metadata_removal(self):
+        """Rebuild a server with metadata."""
+
+        # create a server with initially has no metadata
+        server = self._build_minimal_create_server_request()
+        server_post = {'server': server}
+
+        metadata = {}
+        for i in range(30):
+            metadata['key_%s' % i] = 'value_%s' % i
+
+        server_post['server']['metadata'] = metadata
+
+        created_server = self.api.post_server(server_post)
+        LOG.debug("created_server: %s" % created_server)
+        self.assertTrue(created_server['id'])
+        created_server_id = created_server['id']
+
+        # rebuild the server with metadata
+        post = {}
+        post['rebuild'] = {
+            "imageRef": "https://localhost/v1.1/32278/images/2",
+            "name": "blah"
+        }
+
+        metadata = {}
+        post['rebuild']['metadata'] = metadata
+
+        self.api.post_server_action(created_server_id, post)
+        LOG.debug("rebuilt server: %s" % created_server)
+        self.assertTrue(created_server['id'])
+
+        found_server = self.api.get_server(created_server_id)
+        self.assertEqual(created_server_id, found_server['id'])
+        self.assertEqual(metadata, found_server.get('metadata'))
+        self.assertEqual('blah', found_server.get('name'))
+
+        # Cleanup
+        self._delete_server(created_server_id)
+
 
 if __name__ == "__main__":
     unittest.main()