Merge lp:~rackspace-titan/nova/metadata_v1.1 into lp:~hudson-openstack/nova/trunk

[Dan Prince]

Implement metadata resource for Openstack API v1.1. Includes:
      -GET /servers/id/meta
      -POST /servers/id/meta
      -GET /servers/id/meta/key
      -PUT /servers/id/meta/key
      -DELETE /servers/id/meta/key

[justinsb (justin-fathomdb)]

It looks like I can post multiple keys as long as one of those is the 'right' key e.g.

PUT /servers/1/meta/key1
{ "key1": "value1", "key2": "value2" }

That's fine with me, but might be unexpected.

I'm really confused by the spec. Sometimes it's meta, sometimes metadata, sometimes there's a values collection, sometimes there's not. So it's a bit difficult to verify this against the spec. But as long as you're consistent (and it looks like you are), we can always fix the spec to match...

This ones a total nit-pick: do you think that delete_instance_metadata should filter by deleted=False, so that we don't keep marking a key as deleted once it is deleted?

However, as a consequence: What happens if I delete a key, and then try to create (or update) the same key. It looks like you'll get a PK violation?

A general discussion point (not for this patch, probably): should our DB layer be throwing exceptions when it doesn't find things? It seems to cause complexity... I like the convention that get_X throws, whereas find_X returns None if not found.

What are we doing about XML formatting? I don't see how this gets marshalled to the correct XML (though it might be - I didn't actually try this) I'd rather see this merged and then XML fixed separately if there are problems. In general, I'd like to see us testing the XML, because the use of schema means that we get a lot of tests "for free".

They're all a bit nit-picky, but I'm marking as "Needs information" for now because collectively I think they're sort of important.

review: Needs Information

[Dan Prince (dan-prince)]

Hi Justin,

I made a couple of changes based on your comments:

--Regarding 'collections' within the openstack API. I didn't update the JSON/XML responses to nest things in 'values' collections yet. We'll handle this specific issue globally across the API. What I've done here should be consistent with what already exists within the API's.

--The PUT call now guards against requests with multiple keys.

--My update delete SQL command now filters by deleted=False. I've tested the PUT/DELETE/GET commands extensively and I we should be fine if someone deletes a key and then recreates the same key again. I did however modify the update command so that it sets 'deleted=0' on an update (thus reactivating a key). I think 'update_or_create_instance_metadata' should be good to go now.

--Regarding the XML serialization stuff. Nova already has issues here. A call to get metadata in XML format with return something that looks like this:

 <metadata> <key2> value2 </key2> <key1> value1 </key1> </metadata>

Again this isn't "too the spec" but we'll handle proper serialization as a separate issue across the board.

--Regarding the exception throwing. I throw an exception in the 'get_instance_metadata_item' if the key isn't found. In 'get_instance_metadata' I simply return an empty dict if there is not data sense no metadata implies that metadata is just empty. I don't see any calls with find in the name so I suppose we could start doing this as a convention but I'm not sure I need to change anything to follow it currently.

Let me know what you think with the latest updates. I think we should be good to go.

[justinsb (justin-fathomdb)]

Great changes, thanks Dan.

Agree that the 'off topic' issues like XML details are best dealt with separately.

review: Approve

[Sandy Walsh (sandy-walsh)]

Now that openstack-api-versioned-controllers I think we need a freshen up (hard to tell what code is from what branch).

Also, do you have any information on what the expected metadata for a server would be?

Is this a user operation or a service provider operation?

I guess I'm not really clear on the purpose of the branch.

review: Needs Information

[Dan Prince (dan-prince)]

> Now that openstack-api-versioned-controllers I think we need a freshen up
> (hard to tell what code is from what branch).

Sandy. Good call. I just synced up with the latest lp:nova code again.

> Also, do you have any information on what the expected metadata for a server
> would be?
>
> Is this a user operation or a service provider operation?

This branch implements the v1.1 metadata API such that a user can manipulate server specific metadata. A request to /v1.1/servers/1/meta might return something like this:

{
  "metadata" : {
    "Server Label" : "Web Head 1",
    "Image Version" : "2.1"
  }
}

The full spec is here:

https://blueprints.launchpad.net/nova/+spec/openstack-api-1-1

[Matt Dietz (cerberus)]

Your database API method names aren't consistent with the rest of the DB API

159 +def delete_instance_metadata(context, instance_id, key):

Should be, for example:

159 +def instance_metadata_delete(context, instance_id, key):

With that said, I actually have reservations about whether or not this branch truly belongs in Nova. It seems like it would be better situated in a metadata service somewhere at the top of the stack, possibly attached to the API, and simply mapped to instance_ids.

Since I missed this spec originally I won't block it, but I'd like to maybe discuss moving the responsibility for this to a different layer post Cactus

review: Needs Fixing

[Dan Prince (dan-prince)]

> With that said, I actually have reservations about whether or not this branch
> truly belongs in Nova. It seems like it would be better situated in a metadata
> service somewhere at the top of the stack, possibly attached to the API, and
> simply mapped to instance_ids.
>
> Since I missed this spec originally I won't block it, but I'd like to maybe
> discuss moving the responsibility for this to a different layer post Cactus

--

Hey Matt. We are looking into renaming things now.

So on whether this should exist in nova... We had previously bounced the idea around about whether metadata should be a separate service. It certainly could be and that might work better in some cases. Probably best to discuss that as a separate issue/blueprint.

All this branch does is basically extends the v1.0 implementation (which already used the database) to make metadata a RESTful sub-resource under /servers in the API to support the v1.1 spec.

[Dan Prince (dan-prince)]

Hey Matt. Mark just updated the database API names so we should be good to go.

[Matt Dietz (cerberus)]

Thanks for the fixes :-)

And yes, I agree that it should be a separate blueprint. We can talk about it at the summit next month :-D

review: Approve

[Sandy Walsh (sandy-walsh)]

Looks good ...

109 - is there a risk that 'metadata' key doesn't exist?

review: Needs Information

[Dan Prince (dan-prince)]

> Looks good ...
>
> 109 - is there a risk that 'metadata' key doesn't exist?

Hey Sandy,

The _get_metadata method always returns a dict with 'metadata' so I think we are good.

[Sandy Walsh (sandy-walsh)]

Thanks for that

review: Approve

[OpenStack Infra (hudson-openstack)]

Attempt to merge into lp:nova failed due to conflicts:

text conflict in nova/compute/api.py

[Dan Prince (dan-prince)]

Merged with trunk to resolve compute API conflict. Should be good to go again.

[Dan Prince (dan-prince)]

Whoops. Accidentally set to merged. Set back to needs review again.

[OpenStack Infra (hudson-openstack)]

No proposals found for merge of lp:~rackspace-titan/nova/openstack-api-versioned-controllers into lp:nova.

[Dan Prince (dan-prince)]

Marking this as merged. Looks like Hudson failed because of a dependent branch or something.

=== modified file 'Authors'
--- Authors	2011-03-24 22:43:46 +0000
+++ Authors	2011-03-24 22:51:33 +0000
@@ -21,6 +21,7 @@
 Eric Day <[e-mail]>
 Eric Windisch <[e-mail]>
 Ewan Mellor <[e-mail]>
+Gabe Westmaas <[e-mail]>
 Hisaharu Ishii <[e-mail]>
 Hisaki Ohara <[e-mail]>
 Ilya Alekseyev <[e-mail]>
 
=== modified file 'nova/api/openstack/__init__.py'
--- nova/api/openstack/__init__.py	2011-03-24 22:26:47 +0000
+++ nova/api/openstack/__init__.py	2011-03-24 22:51:33 +0000
@@ -35,6 +35,7 @@
 from nova.api.openstack import images
 from nova.api.openstack import limits
 from nova.api.openstack import servers
+from nova.api.openstack import server_metadata
 from nova.api.openstack import shared_ip_groups
 from nova.api.openstack import users
 from nova.api.openstack import zones
@@ -148,6 +149,10 @@
                         controller=servers.ControllerV11(),
                         collection={'detail': 'GET'},
                         member=self.server_members)
+        mapper.resource("server_meta", "meta",
+                        controller=server_metadata.Controller(),
+                        parent_resource=dict(member_name='server',
+                        collection_name='servers'))
 
 
 class Versions(wsgi.Application):
 
=== added file 'nova/api/openstack/server_metadata.py'
--- nova/api/openstack/server_metadata.py	1970-01-01 00:00:00 +0000
+++ nova/api/openstack/server_metadata.py	2011-03-24 22:51:33 +0000
@@ -0,0 +1,78 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2011 OpenStack LLC.
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+from webob import exc
+
+from nova import compute
+from nova import wsgi
+from nova.api.openstack import faults
+
+
+class Controller(wsgi.Controller):
+    """ The server metadata API controller for the Openstack API """
+
+    def __init__(self):
+        self.compute_api = compute.API()
+        super(Controller, self).__init__()
+
+    def _get_metadata(self, context, server_id):
+        metadata = self.compute_api.get_instance_metadata(context, server_id)
+        meta_dict = {}
+        for key, value in metadata.iteritems():
+            meta_dict[key] = value
+        return dict(metadata=meta_dict)
+
+    def index(self, req, server_id):
+        """ Returns the list of metadata for a given instance """
+        context = req.environ['nova.context']
+        return self._get_metadata(context, server_id)
+
+    def create(self, req, server_id):
+        context = req.environ['nova.context']
+        body = self._deserialize(req.body, req.get_content_type())
+        self.compute_api.update_or_create_instance_metadata(context,
+                                                            server_id,
+                                                            body['metadata'])
+        return req.body
+
+    def update(self, req, server_id, id):
+        context = req.environ['nova.context']
+        body = self._deserialize(req.body, req.get_content_type())
+        if not id in body:
+            expl = _('Request body and URI mismatch')
+            raise exc.HTTPBadRequest(explanation=expl)
+        if len(body) > 1:
+            expl = _('Request body contains too many items')
+            raise exc.HTTPBadRequest(explanation=expl)
+        self.compute_api.update_or_create_instance_metadata(context,
+                                                            server_id,
+                                                            body)
+        return req.body
+
+    def show(self, req, server_id, id):
+        """ Return a single metadata item """
+        context = req.environ['nova.context']
+        data = self._get_metadata(context, server_id)
+        if id in data['metadata']:
+            return {id: data['metadata'][id]}
+        else:
+            return faults.Fault(exc.HTTPNotFound())
+
+    def delete(self, req, server_id, id):
+        """ Deletes an existing metadata """
+        context = req.environ['nova.context']
+        self.compute_api.delete_instance_metadata(context, server_id, id)
 
=== modified file 'nova/compute/api.py'
--- nova/compute/api.py	2011-03-24 21:50:27 +0000
+++ nova/compute/api.py	2011-03-24 22:51:33 +0000
@@ -673,3 +673,18 @@
         self.network_api.associate_floating_ip(context,
                                                floating_ip=address,
                                                fixed_ip=instance['fixed_ip'])
+
+    def get_instance_metadata(self, context, instance_id):
+        """Get all metadata associated with an instance."""
+        rv = self.db.instance_metadata_get(context, instance_id)
+        return dict(rv.iteritems())
+
+    def delete_instance_metadata(self, context, instance_id, key):
+        """Delete the given metadata item"""
+        self.db.instance_metadata_delete(context, instance_id, key)
+
+    def update_or_create_instance_metadata(self, context, instance_id,
+                                            metadata):
+        """Updates or creates instance metadata"""
+        self.db.instance_metadata_update_or_create(context, instance_id,
+                                                    metadata)
 
=== modified file 'nova/db/api.py'
--- nova/db/api.py	2011-03-24 19:04:24 +0000
+++ nova/db/api.py	2011-03-24 22:51:33 +0000
@@ -1172,3 +1172,21 @@
 def zone_get_all(context):
     """Get all child Zones."""
     return IMPL.zone_get_all(context)
+
+
+####################
+
+
+def instance_metadata_get(context, instance_id):
+    """Get all metadata for an instance"""
+    return IMPL.instance_metadata_get(context, instance_id)
+
+
+def instance_metadata_delete(context, instance_id, key):
+    """Delete the given metadata item"""
+    IMPL.instance_metadata_delete(context, instance_id, key)
+
+
+def instance_metadata_update_or_create(context, instance_id, metadata):
+    """Create or update instance metadata"""
+    IMPL.instance_metadata_update_or_create(context, instance_id, metadata)
 
=== modified file 'nova/db/sqlalchemy/api.py'
--- nova/db/sqlalchemy/api.py	2011-03-24 18:09:13 +0000
+++ nova/db/sqlalchemy/api.py	2011-03-24 22:51:33 +0000
@@ -2466,3 +2466,65 @@
 def zone_get_all(context):
     session = get_session()
     return session.query(models.Zone).all()
+
+
+####################
+
+@require_context
+def instance_metadata_get(context, instance_id):
+    session = get_session()
+
+    meta_results = session.query(models.InstanceMetadata).\
+                    filter_by(instance_id=instance_id).\
+                    filter_by(deleted=False).\
+                    all()
+
+    meta_dict = {}
+    for i in meta_results:
+        meta_dict[i['key']] = i['value']
+    return meta_dict
+
+
+@require_context
+def instance_metadata_delete(context, instance_id, key):
+    session = get_session()
+    session.query(models.InstanceMetadata).\
+        filter_by(instance_id=instance_id).\
+        filter_by(key=key).\
+        filter_by(deleted=False).\
+        update({'deleted': 1,
+                '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()
+
+    meta_result = session.query(models.InstanceMetadata).\
+                    filter_by(instance_id=instance_id).\
+                    filter_by(key=key).\
+                    filter_by(deleted=False).\
+                    first()
+
+    if not meta_result:
+        raise exception.NotFound(_('Invalid metadata key for instance %s') %
+                                    instance_id)
+    return meta_result
+
+
+@require_context
+def instance_metadata_update_or_create(context, instance_id, metadata):
+    session = get_session()
+    meta_ref = None
+    for key, value in metadata.iteritems():
+        try:
+            meta_ref = instance_metadata_get_item(context, instance_id, key,
+                                                        session)
+        except:
+            meta_ref = models.InstanceMetadata()
+        meta_ref.update({"key": key, "value": value,
+                            "instance_id": instance_id,
+                            "deleted": 0})
+        meta_ref.save(session=session)
+    return metadata
 
=== added file 'nova/tests/api/openstack/test_server_metadata.py'
--- nova/tests/api/openstack/test_server_metadata.py	1970-01-01 00:00:00 +0000
+++ nova/tests/api/openstack/test_server_metadata.py	2011-03-24 22:51:33 +0000
@@ -0,0 +1,164 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2011 OpenStack LLC.
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+import json
+import stubout
+import unittest
+import webob
+
+
+from nova.api import openstack
+from nova.tests.api.openstack import fakes
+import nova.wsgi
+
+
+def return_create_instance_metadata(context, server_id, metadata):
+    return stub_server_metadata()
+
+
+def return_server_metadata(context, server_id):
+    return stub_server_metadata()
+
+
+def return_empty_server_metadata(context, server_id):
+    return {}
+
+
+def delete_server_metadata(context, server_id, key):
+    pass
+
+
+def stub_server_metadata():
+    metadata = {
+            "key1": "value1",
+            "key2": "value2",
+            "key3": "value3",
+            "key4": "value4",
+            "key5": "value5"
+        }
+    return metadata
+
+
+class ServerMetaDataTest(unittest.TestCase):
+
+    def setUp(self):
+        super(ServerMetaDataTest, self).setUp()
+        self.stubs = stubout.StubOutForTesting()
+        fakes.FakeAuthManager.auth_data = {}
+        fakes.FakeAuthDatabase.data = {}
+        fakes.stub_out_auth(self.stubs)
+        fakes.stub_out_key_pair_funcs(self.stubs)
+
+    def tearDown(self):
+        self.stubs.UnsetAll()
+        super(ServerMetaDataTest, self).tearDown()
+
+    def test_index(self):
+        self.stubs.Set(nova.db.api, 'instance_metadata_get',
+                        return_server_metadata)
+        req = webob.Request.blank('/v1.1/servers/1/meta')
+        req.environ['api.version'] = '1.1'
+        res = req.get_response(fakes.wsgi_app())
+        res_dict = json.loads(res.body)
+        self.assertEqual(200, res.status_int)
+        self.assertEqual('value1', res_dict['metadata']['key1'])
+
+    def test_index_no_data(self):
+        self.stubs.Set(nova.db.api, 'instance_metadata_get',
+                        return_empty_server_metadata)
+        req = webob.Request.blank('/v1.1/servers/1/meta')
+        req.environ['api.version'] = '1.1'
+        res = req.get_response(fakes.wsgi_app())
+        res_dict = json.loads(res.body)
+        self.assertEqual(200, res.status_int)
+        self.assertEqual(0, len(res_dict['metadata']))
+
+    def test_show(self):
+        self.stubs.Set(nova.db.api, 'instance_metadata_get',
+                        return_server_metadata)
+        req = webob.Request.blank('/v1.1/servers/1/meta/key5')
+        req.environ['api.version'] = '1.1'
+        res = req.get_response(fakes.wsgi_app())
+        res_dict = json.loads(res.body)
+        self.assertEqual(200, res.status_int)
+        self.assertEqual('value5', res_dict['key5'])
+
+    def test_show_meta_not_found(self):
+        self.stubs.Set(nova.db.api, 'instance_metadata_get',
+                        return_empty_server_metadata)
+        req = webob.Request.blank('/v1.1/servers/1/meta/key6')
+        req.environ['api.version'] = '1.1'
+        res = req.get_response(fakes.wsgi_app())
+        res_dict = json.loads(res.body)
+        self.assertEqual(404, res.status_int)
+
+    def test_delete(self):
+        self.stubs.Set(nova.db.api, 'instance_metadata_delete',
+                        delete_server_metadata)
+        req = webob.Request.blank('/v1.1/servers/1/meta/key5')
+        req.environ['api.version'] = '1.1'
+        req.method = 'DELETE'
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(200, res.status_int)
+
+    def test_create(self):
+        self.stubs.Set(nova.db.api, 'instance_metadata_update_or_create',
+                        return_create_instance_metadata)
+        req = webob.Request.blank('/v1.1/servers/1/meta')
+        req.environ['api.version'] = '1.1'
+        req.method = 'POST'
+        req.body = '{"metadata": {"key1": "value1"}}'
+        req.headers["content-type"] = "application/json"
+        res = req.get_response(fakes.wsgi_app())
+        res_dict = json.loads(res.body)
+        self.assertEqual(200, res.status_int)
+        self.assertEqual('value1', res_dict['metadata']['key1'])
+
+    def test_update_item(self):
+        self.stubs.Set(nova.db.api, 'instance_metadata_update_or_create',
+                        return_create_instance_metadata)
+        req = webob.Request.blank('/v1.1/servers/1/meta/key1')
+        req.environ['api.version'] = '1.1'
+        req.method = 'PUT'
+        req.body = '{"key1": "value1"}'
+        req.headers["content-type"] = "application/json"
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(200, res.status_int)
+        res_dict = json.loads(res.body)
+        self.assertEqual('value1', res_dict['key1'])
+
+    def test_update_item_too_many_keys(self):
+        self.stubs.Set(nova.db.api, 'instance_metadata_update_or_create',
+                        return_create_instance_metadata)
+        req = webob.Request.blank('/v1.1/servers/1/meta/key1')
+        req.environ['api.version'] = '1.1'
+        req.method = 'PUT'
+        req.body = '{"key1": "value1", "key2": "value2"}'
+        req.headers["content-type"] = "application/json"
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(400, res.status_int)
+
+    def test_update_item_body_uri_mismatch(self):
+        self.stubs.Set(nova.db.api, 'instance_metadata_update_or_create',
+                        return_create_instance_metadata)
+        req = webob.Request.blank('/v1.1/servers/1/meta/bad')
+        req.environ['api.version'] = '1.1'
+        req.method = 'PUT'
+        req.body = '{"key1": "value1"}'
+        req.headers["content-type"] = "application/json"
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(400, res.status_int)