Merge lp:~bcwaldon/nova/osapi-flavors-1_1 into lp:~hudson-openstack/nova/trunk

[Brian Waldon]

Openstack api 1.0 flavors resource now implemented to match the spec. This also addresses the instance_type.id != flavor_id issue in bug #736343.

Resubmitting w/ prereq removed

[Paul Voccio (pvo)]

lgtm

review: Approve

[Rick Harris (rconradharris)]

Really nice job, Brian.

> 142 + "ram": "256",
> 143 + "disk": "10",

Looking at the CS API spec, it looks like JSON format uses ints for `ram` and `disk`. Is this correct?

Strangely, the XML format uses strings for all the attrs with no type qualifier.

review: Needs Information

[Brian Waldon (bcwaldon)]

> Really nice job, Brian.
>
> > 142 + "ram": "256",
> > 143 + "disk": "10",
>
> Looking at the CS API spec, it looks like JSON format uses ints for `ram` and
> `disk`. Is this correct?
>
> Strangely, the XML format uses strings for all the attrs with no type
> qualifier.

So I have noticed this across the project; integer values are represented as strings in xml and ints in json. I decided to make everything a string for now until we have some actual xml schemas just for consistency across the formats. The other option would be to accept the misrepresentation in xml and use ints in json. Thoughts?

[Rick Harris (rconradharris)]

> I have noticed this across the project

Gotcha, if it's that widespread, I like your go-for-consistency approach. If this turns out to be a legit problem, we can get clarification, make a bug, and fix it all at once.

Might be a good idea to add a TODO or a Wishlist-bug to investigate this down the road.

review: Approve

[justinsb (justin-fathomdb)]

>
> > I have noticed this across the project
>
> Gotcha, if it's that widespread, I like your go-for-consistency approach.
> If this turns out to be a legit problem, we can get clarification, make a
> bug, and fix it all at once.
>
> Might be a good idea to add a TODO or a Wishlist-bug to investigate this
> down the road.

Ick... can I clarify which schema we're looking at? I'm looking at this
one:
http://docs.rackspacecloud.com/servers/api/v1.0/xsd/flavor.xsd

<http://docs.rackspacecloud.com/servers/api/v1.0/xsd/flavor.xsd>Where it
looks like ram is an xsd:int ?

Totally agree this is probably not best fixed in this particular patch given
it's cross-cutting. Am I looking at the right schema though? Do we have a
bug / blueprint?

[Rick Harris (rconradharris)]

> Ick... can I clarify which schema we're looking at? I'm looking at this
one:

I wasn't looking at a particular schema, I was just looking in the `Flavors` section of

http://docs.rackspacecloud.com/servers/api/cs-devguide-latest.pdf

I noticed that the JSON version didn't quote the ram/disk whereas this patch does.

[Brian Waldon (bcwaldon)]

> >
> > > I have noticed this across the project
> >
> > Gotcha, if it's that widespread, I like your go-for-consistency approach.
> > If this turns out to be a legit problem, we can get clarification, make a
> > bug, and fix it all at once.
> >
> > Might be a good idea to add a TODO or a Wishlist-bug to investigate this
> > down the road.
>
>
> Ick... can I clarify which schema we're looking at? I'm looking at this
> one:
> http://docs.rackspacecloud.com/servers/api/v1.0/xsd/flavor.xsd
>
> <http://docs.rackspacecloud.com/servers/api/v1.0/xsd/flavor.xsd>Where it
> looks like ram is an xsd:int ?

I hadn't even looked at that schema. You're definitely right. Is there a v1.1 schema somewhere?

>
> Totally agree this is probably not best fixed in this particular patch given
> it's cross-cutting. Am I looking at the right schema though? Do we have a
> bug / blueprint?

Like I said earlier, I think there is some inconsistency across the project as a whole between xml/json integers. I do hate feeling like I am contributing to the problem. Would you like me to file a bug and address this very soon?

[justinsb (justin-fathomdb)]

>
> Like I said earlier, I think there is some inconsistency across the project
> as a whole between xml/json integers. I do hate feeling like I am
> contributing to the problem. Would you like me to file a bug and address
> this very soon?
>

I don't know if there's a 1.1 schema yet. I think filing the bug is
definitely the right place to start, probably also the ML - I'm guessing
there will need to be some discussion around the possible approaches.

I wouldn't worry about contributing to the problem - I think you've brought
it to our collective attention. I'm thinking the right way to solve this
will be with a dedicated cleanup patch / patch series, rather than a
patch-by-patch, call-by-call approach. It might even be best to do this
once all the Cactus feature branches have been merged. I don't want XML to
block this or other important patches from merging.

[Brian Waldon (bcwaldon)]

> It might even be best to do this
> once all the Cactus feature branches have been merged. I don't want XML to
> block this or other important patches from merging.

Definitely. I'll be sure to keep this in mind next week.

[Rick Harris (rconradharris)]

> I don't want XML to block this or other important patches from merging.

+1

[OpenStack Infra (hudson-openstack)]

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

[Rick Harris (rconradharris)]

> No proposals found for merge of lp:~rackspace-titan/nova/openstack-api-version-split into lp:nova.

Uh, the pre-req was merged in at rev821.

[Brian Waldon (bcwaldon)]

> > No proposals found for merge of lp:~rackspace-titan/nova/openstack-api-
> version-split into lp:nova.
>
> Uh, the pre-req was merged in at rev821.

That prereq was valid when I filed this merge prop. Do I need to manually remove it now that it has been resolved?

[Paul Voccio (pvo)]

lgtm. Was previously approved as well.

review: Approve

[Rick Harris (rconradharris)]

>That prereq was valid when I filed this merge prop. Do I need to manually remove it now that it has been resolved?

Hmm I would have thought LP was smart enough to handle that. But, just in case, guess we could remove the pre-req.

=== modified file 'nova/api/openstack/flavors.py'
--- nova/api/openstack/flavors.py	2011-03-16 19:43:57 +0000
+++ nova/api/openstack/flavors.py	2011-03-21 18:06:07 +0000
@@ -22,6 +22,7 @@
 from nova.api.openstack import faults
 from nova.api.openstack import common
 from nova.compute import instance_types
+from nova.api.openstack.views import flavors as flavors_views
 from nova import wsgi
 import nova.api.openstack
 
@@ -36,7 +37,7 @@
 
     def index(self, req):
         """Return all flavors in brief."""
-        return dict(flavors=[dict(id=flavor['flavorid'], name=flavor['name'])
+        return dict(flavors=[dict(id=flavor['id'], name=flavor['name'])
                              for flavor in self.detail(req)['flavors']])
 
     def detail(self, req):
@@ -47,14 +48,18 @@
     def show(self, req, id):
         """Return data about the given flavor id."""
         ctxt = req.environ['nova.context']
-        values = db.instance_type_get_by_flavor_id(ctxt, id)
-        values['id'] = values['flavorid']
+        flavor = db.api.instance_type_get_by_flavor_id(ctxt, id)
+        values = {
+            "id": flavor["flavorid"],
+            "name": flavor["name"],
+            "ram": flavor["memory_mb"],
+            "disk": flavor["local_gb"],
+        }
         return dict(flavor=values)
-        raise faults.Fault(exc.HTTPNotFound())
 
     def _all_ids(self, req):
         """Return the list of all flavorids."""
         ctxt = req.environ['nova.context']
-        inst_types = db.instance_type_get_all(ctxt)
+        inst_types = db.api.instance_type_get_all(ctxt)
         flavor_ids = [inst_types[i]['flavorid'] for i in inst_types.keys()]
         return sorted(flavor_ids)

=== modified file 'nova/tests/api/openstack/test_flavors.py'
--- nova/tests/api/openstack/test_flavors.py	2011-03-16 19:40:16 +0000
+++ nova/tests/api/openstack/test_flavors.py	2011-03-21 18:06:07 +0000
@@ -22,11 +22,32 @@
 from nova import test
 import nova.api
 from nova import context
+from nova.api.openstack import flavors
 from nova import db
-from nova.api.openstack import flavors
 from nova.tests.api.openstack import fakes
 
 
+def stub_flavor(flavorid, name, memory_mb="256", local_gb="10"):
+    return {
+        "flavorid": str(flavorid),
+        "name": name,
+        "memory_mb": memory_mb,
+        "local_gb": local_gb,
+    }
+
+
+def return_instance_type_by_flavor_id(context, flavorid):
+    return stub_flavor(flavorid, "flavor %s" % (flavorid,))
+
+
+def return_instance_types(context, num=2):
+    instance_types = {}
+    for i in xrange(1, num + 1):
+        name = "flavor %s" % (i,)
+        instance_types[name] = stub_flavor(i, name)
+    return instance_types
+
+
 class FlavorsTest(test.TestCase):
     def setUp(self):
         super(FlavorsTest, self).setUp()
@@ -36,6 +57,10 @@
         fakes.stub_out_networking(self.stubs)
         fakes.stub_out_rate_limiting(self.stubs)
         fakes.stub_out_auth(self.stubs)
+        self.stubs.Set(nova.db.api, "instance_type_get_all",
+                       return_instance_types)
+        self.stubs.Set(nova.db.api, "instance_type_get_by_flavor_id",
+                       return_instance_type_by_flavor_id)
         self.context = context.get_admin_context()
 
     def tearDown(self):
@@ -46,10 +71,49 @@
         req = webob.Request.blank('/v1.0/flavors')
         res = req.get_response(fakes.wsgi_app())
         self.assertEqual(res.status_int, 200)
+        flavors = json.loads(res.body)["flavors"]
+        expected = [
+            {
+                "id": "1",
+                "name": "flavor 1",
+            },
+            {
+                "id": "2",
+                "name": "flavor 2",
+            },
+        ]
+        self.assertEqual(flavors, expected)
+
+    def test_get_flavor_list_detail(self):
+        req = webob.Request.blank('/v1.0/flavors/detail')
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 200)
+        flavors = json.loads(res.body)["flavors"]
+        expected = [
+            {
+                "id": "1",
+                "name": "flavor 1",
+                "ram": "256",
+                "disk": "10",
+            },
+            {
+                "id": "2",
+                "name": "flavor 2",
+                "ram": "256",
+                "disk": "10",
+            },
+        ]
+        self.assertEqual(flavors, expected)
 
     def test_get_flavor_by_id(self):
-        req = webob.Request.blank('/v1.0/flavors/1')
+        req = webob.Request.blank('/v1.0/flavors/12')
         res = req.get_response(fakes.wsgi_app())
         self.assertEqual(res.status_int, 200)
-        body = json.loads(res.body)
-        self.assertEqual(body['flavor']['id'], 1)
+        flavor = json.loads(res.body)["flavor"]
+        expected = {
+            "id": "12",
+            "name": "flavor 12",
+            "ram": "256",
+            "disk": "10",
+        }
+        self.assertEqual(flavor, expected)