Merge lp:~rackspace-titan/nova/openstack-api-versioned-controllers into lp:~hudson-openstack/nova/trunk

[Naveed Massjouni]

Added a mechanism for versioned controllers for openstack api versions 1.0/1.1.
Create servers in the 1.1 api now supports imageRef/flavorRef instead of imageId/flavorId.

[Brian Waldon (bcwaldon)]

Right now your imageRef parsing for v1.1 requests is very simple. All it does is use everything after the last slash as the image id. What will happen if you get an unexpected imageRef, such as this:

    http://servers.api.openstack.org/ v1.1/1234/images?limit=1&marker=743

I ask, because this is the format of the "next" link returned in a request to /images. I understand this shouldn't be valid, but it appears it may have unintended consequences.

I like what you did with the versioned APIRouters and Controllers.

review: Needs Fixing

[Naveed Massjouni (ironcamel)]

> Right now your imageRef parsing for v1.1 requests is very simple. All it does
> is use everything after the last slash as the image id. What will happen if
> you get an unexpected imageRef, such as this:
>
> http://servers.api.openstack.org/ v1.1/1234/images?limit=1&marker=743
>
> I ask, because this is the format of the "next" link returned in a request to
> /images. I understand this shouldn't be valid, but it appears it may have
> unintended consequences.
>

Commit 806 addresses this. Thanks.

[Brian Waldon (bcwaldon)]

Looks good! Thanks for taking a look at that.

review: Approve

[Joshua McKenty (joshua-mckenty)]

Overall thoughts:

 - Using an Integer for the API version number - convenient, but needs to be well documented (docstrings, etc), so that we don't end up with collisions between 1.0 and 10.0, for instance.

 - Using a general assumption that <object_id> is the last portion of the URL, and an integer, seems really dangerous. If I understand correctly, the point of using ref for image is that it could potentially refer to a remote glance server, is that right? Even if this is reliably on the same domain, can we use a proper url parsing library (wsgilib from paste, perhaps) for grabbing portions of the path? What happens if the image ref is sftp:// or git:// schema instead of http?

Why the change to isDetail=True?
Missing docstrings on classes and private methods.
430, instead of the if, use:
    for item in inst.get('metadata', []):

449 and 457 - we don't want to guarantee that those keys are set? Otherwise use .get() again.

468 and 474 - do we really want == None checks here? E.g., are the calls valid if image id and flavour aren't set? Would an exception thrown here be appropriate?

597 Content-Type header should, I believe, be "Accept" header on request. Also, it's case-insensitive according to the RFC (see http://www.ietf.org/rfc/rfc2616.txt section 4.2), so we should probably support it that way.

622 - Why the mix of dict() and {} ?

review: Needs Information

[Brian Waldon (bcwaldon)]

I addressed some of your concerns, as Naveed is out today:

> - Using an Integer for the API version number - convenient, but needs to be well documented
> (docstrings, etc), so that we don't end up with collisions between 1.0 and 10.0, for instance.

I'll let Naveed address this.

> - Using a general assumption that <object_id> is the last portion of the URL,
> and an integer, seems really dangerous. If I understand correctly, the point
> of using ref for image is that it could potentially refer to a remote glance
> server, is that right? Even if this is reliably on the same domain, can we use
> a proper url parsing library (wsgilib from paste, perhaps) for grabbing
> portions of the path? What happens if the image ref is sftp:// or git://
> schema instead of http?

Our main goal was to get as much of the openstack api v1.1 interface defined in code. We know we don't handle the intended functionality, but we also haven't officially announced it. We want to get the interface defined as well as possible for cactus with the intention of filling in the holes in future releases (maybe even as bugs before we release).

> Why the change to isDetail=True?

Assuming you mean line 214: when creating a server you are supposed to get a "detail" response back. It was a bug to use is_detail=False. Check the spec if you want to see more info.

> Missing docstrings on classes and private methods.

I'll let Naveed address this.

> 430, instead of the if, use:
> for item in inst.get('metadata', []):

Good call. Fixed.

> 449 and 457 - we don't want to guarantee that those keys are set? Otherwise
> use .get() again.

The absence of those values from "inst" could indicate an error, as a server should always have an image id and instance type. I don't want to communicate those as NULL or an empty string if that should never actually happen. How would you suggest handling that?

> 468 and 474 - do we really want == None checks here? E.g., are the calls valid
> if image id and flavour aren't set? Would an exception thrown here be
> appropriate?

This goes along with the above statement.

> 597 Content-Type header should, I believe, be "Accept" header on request.
> Also, it's case-insensitive according to the RFC (see
> http://www.ietf.org/rfc/rfc2616.txt section 4.2), so we should probably
> support it that way.

I think you might have Accept and Content-Type a little confused. With a request to a server, you use Content-Type to describe the mimetype of the data you are providing. You use Accept to ask the server for a certain mimetype in the response. When the server responds, it uses Content-Type to describe the mimetype of the response entity. In this case, the test is setting the Content-Type of the request body.

> 622 - Why the mix of dict() and {} ?

Looks like it was copy/pasted in without thinking about it. Fixed.

[Todd Willey (xtoddx)]

> I addressed some of your concerns, as Naveed is out today:
>
> > - Using an Integer for the API version number - convenient, but needs to be
> well documented
> > (docstrings, etc), so that we don't end up with collisions between 1.0 and
> 10.0, for instance.
>
> I'll let Naveed address this.
>

Josh, do you mean in naming the classes? Paste.deploy maps urls into the classes, so the actual names are not very consequential.

>
> > - Using a general assumption that <object_id> is the last portion of the
> URL,
> > and an integer, seems really dangerous. If I understand correctly, the point
> > of using ref for image is that it could potentially refer to a remote glance
> > server, is that right? Even if this is reliably on the same domain, can we
> use
> > a proper url parsing library (wsgilib from paste, perhaps) for grabbing
> > portions of the path? What happens if the image ref is sftp:// or git://
> > schema instead of http?
>
> Our main goal was to get as much of the openstack api v1.1 interface defined
> in code. We know we don't handle the intended functionality, but we also
> haven't officially announced it. We want to get the interface defined as well
> as possible for cactus with the intention of filling in the holes in future
> releases (maybe even as bugs before we release).
>

I'd also like to use PATH_INFO or SCRIPT_NAME or the like (set by wsgi) at the very least.

>
> > Why the change to isDetail=True?
>
> Assuming you mean line 214: when creating a server you are supposed to get a
> "detail" response back. It was a bug to use is_detail=False. Check the spec if
> you want to see more info.
>
> > Missing docstrings on classes and private methods.
>
> I'll let Naveed address this.
>
>
> > 430, instead of the if, use:
> > for item in inst.get('metadata', []):
>
> Good call. Fixed.
>
>
> > 449 and 457 - we don't want to guarantee that those keys are set? Otherwise
> > use .get() again.
>
> The absence of those values from "inst" could indicate an error, as a server
> should always have an image id and instance type. I don't want to communicate
> those as NULL or an empty string if that should never actually happen. How
> would you suggest handling that?
>

But inst comes back from compute_api, so it should be a reliable source of data. If for whatever reason it isn't, I think the place to perform this check is in the controller, not in the view builder. You can raise whatever fault is appropriate there.

>
> > 468 and 474 - do we really want == None checks here? E.g., are the calls
> valid
> > if image id and flavour aren't set? Would an exception thrown here be
> > appropriate?
>
> This goes along with the above statement.
>
>
> > 597 Content-Type header should, I believe, be "Accept" header on request.
> > Also, it's case-insensitive according to the RFC (see
> > http://www.ietf.org/rfc/rfc2616.txt section 4.2), so we should probably
> > support it that way.
>
> I think you might have Accept and Content-Type a little confused. With a
> request to a server, you use Content-Type to describe the mimetype of the data
> you are providing. You use Accept to ask the server for a certain mimetype in
> th...

[Naveed Massjouni (ironcamel)]

> Overall thoughts:
>
> - Using an Integer for the API version number - convenient, but needs to be
> well documented (docstrings, etc), so that we don't end up with collisions
> between 1.0 and 10.0, for instance.

Added docstrings. Also, by the time we get to 10.0, the code will probably be totally different anyway.

>
> - Using a general assumption that <object_id> is the last portion of the URL,
> and an integer, seems really dangerous. If I understand correctly, the point
> of using ref for image is that it could potentially refer to a remote glance
> server, is that right? Even if this is reliably on the same domain, can we use
> a proper url parsing library (wsgilib from paste, perhaps) for grabbing
> portions of the path? What happens if the image ref is sftp:// or git://
> schema instead of http?
>

Good point. I am now using the urlparse module to parse the id out of the url.

>
> Why the change to isDetail=True?
> Missing docstrings on classes and private methods.
> 430, instead of the if, use:
> for item in inst.get('metadata', []):

Nice, thanks for show me this idiom.

>
> 449 and 457 - we don't want to guarantee that those keys are set? Otherwise
> use .get() again.
>
> 468 and 474 - do we really want == None checks here? E.g., are the calls valid
> if image id and flavour aren't set? Would an exception thrown here be
> appropriate?

You are right. The image_id and instance_type should always exist in an instance entity. Fixed.

[Todd Willey (xtoddx)]

Can you please read over Pep-257 for docstring conventions? http://www.python.org/dev/peps/pep-0257/

review: Needs Fixing

[Eric Day (eday)]

Looks good! I definitely think the view building is a lot cleaner now. One thing:

119-122: See the HACKING file for more, but import order should be core-modules, 3rd party modules, and then local modules with each section separated by a line. Also, I don't think you are using re?

review: Needs Fixing

[Naveed Massjouni (ironcamel)]

> Looks good! I definitely think the view building is a lot cleaner now. One
> thing:
>
> 119-122: See the HACKING file for more, but import order should be core-
> modules, 3rd party modules, and then local modules with each section separated
> by a line. Also, I don't think you are using re?

Fixed.

[Naveed Massjouni (ironcamel)]

> Can you please read over Pep-257 for docstring conventions?
> http://www.python.org/dev/peps/pep-0257/

Thanks for pointing me to PEP 257. Fixed.

[Todd Willey (xtoddx)]

review: Approve

[Sandy Walsh (sandy-walsh)]

137, 148 - use super (new style classes)
APIRouter._setup_routes(self, mapper) should be
super(APIRouterV10, self)._setup_routes(mapper) / super(APIRouterV11, self)._setup_routes(mapper)

Otherwise, looks good!

review: Needs Fixing

[Naveed Massjouni (ironcamel)]

> 137, 148 - use super (new style classes)
> APIRouter._setup_routes(self, mapper) should be
> super(APIRouterV10, self)._setup_routes(mapper) / super(APIRouterV11,
> self)._setup_routes(mapper)

Fixed.

>
> Otherwise, looks good!

[Sandy Walsh (sandy-walsh)]

lgtm

review: Approve

[lp:~rackspace-titan/nova/openstack-api-versioned-controllers]

815. By Naveed Massjouni on 2011-03-23

Should not call super __init__ twice in APIRouter

816. By Naveed Massjouni on 2011-03-24

Merge from trunk

817. By Naveed Massjouni on 2011-03-24

Merge from trunk

818. By Naveed Massjouni on 2011-03-24

Fixed the docstring for common.get_id_from_href

Unmerged revisions

=== modified file 'etc/api-paste.ini'
--- etc/api-paste.ini	2011-03-18 17:16:14 +0000
+++ etc/api-paste.ini	2011-03-23 18:10:37 +0000
@@ -67,11 +67,14 @@
 [composite:osapi]
 use = egg:Paste#urlmap
 /: osversions
-/v1.0: openstackapi
-/v1.1: openstackapi
-
-[pipeline:openstackapi]
-pipeline = faultwrap auth ratelimit osapiapp
+/v1.0: openstackapi10
+/v1.1: openstackapi11
+
+[pipeline:openstackapi10]
+pipeline = faultwrap auth ratelimit osapiapp10
+
+[pipeline:openstackapi11]
+pipeline = faultwrap auth ratelimit osapiapp11
 
 [filter:faultwrap]
 paste.filter_factory = nova.api.openstack:FaultWrapper.factory
@@ -82,8 +85,11 @@
 [filter:ratelimit]
 paste.filter_factory = nova.api.openstack.limits:RateLimitingMiddleware.factory
 
-[app:osapiapp]
-paste.app_factory = nova.api.openstack:APIRouter.factory
+[app:osapiapp10]
+paste.app_factory = nova.api.openstack:APIRouterV10.factory
+
+[app:osapiapp11]
+paste.app_factory = nova.api.openstack:APIRouterV11.factory
 
 [pipeline:osversions]
 pipeline = faultwrap osversionapp

=== modified file 'nova/api/openstack/__init__.py'
--- nova/api/openstack/__init__.py	2011-03-18 17:16:14 +0000
+++ nova/api/openstack/__init__.py	2011-03-23 18:10:37 +0000
@@ -72,9 +72,14 @@
         return cls()
 
     def __init__(self):
+        self.server_members = {}
         mapper = routes.Mapper()
+        self._setup_routes(mapper)
+        super(APIRouter, self).__init__(mapper)
 
-        server_members = {'action': 'POST'}
+    def _setup_routes(self, mapper):
+        server_members = self.server_members
+        server_members['action'] = 'POST'
         if FLAGS.allow_admin_api:
             LOG.debug(_("Including admin operations in API."))
 
@@ -99,10 +104,6 @@
                             controller=accounts.Controller(),
                             collection={'detail': 'GET'})
 
-        mapper.resource("server", "servers", controller=servers.Controller(),
-                        collection={'detail': 'GET'},
-                        member=server_members)
-
         mapper.resource("backup_schedule", "backup_schedule",
                         controller=backup_schedules.Controller(),
                         parent_resource=dict(member_name='server',
@@ -129,6 +130,28 @@
         super(APIRouter, self).__init__(mapper)
 
 
+class APIRouterV10(APIRouter):
+    """Define routes specific to OpenStack API V1.0."""
+
+    def _setup_routes(self, mapper):
+        super(APIRouterV10, self)._setup_routes(mapper)
+        mapper.resource("server", "servers",
+                        controller=servers.ControllerV10(),
+                        collection={'detail': 'GET'},
+                        member=self.server_members)
+
+
+class APIRouterV11(APIRouter):
+    """Define routes specific to OpenStack API V1.1."""
+
+    def _setup_routes(self, mapper):
+        super(APIRouterV11, self)._setup_routes(mapper)
+        mapper.resource("server", "servers",
+                        controller=servers.ControllerV11(),
+                        collection={'detail': 'GET'},
+                        member=self.server_members)
+
+
 class Versions(wsgi.Application):
     @webob.dec.wsgify(RequestClass=wsgi.Request)
     def __call__(self, req):

=== modified file 'nova/api/openstack/auth.py'
--- nova/api/openstack/auth.py	2011-03-17 17:42:42 +0000
+++ nova/api/openstack/auth.py	2011-03-23 18:10:37 +0000
@@ -69,8 +69,6 @@
             return faults.Fault(webob.exc.HTTPUnauthorized())
 
         req.environ['nova.context'] = context.RequestContext(user, account)
-        version = req.path.split('/')[1].replace('v', '')
-        req.environ['api.version'] = version
         return self.application
 
     def has_authentication(self, req):

=== modified file 'nova/api/openstack/common.py'
--- nova/api/openstack/common.py	2011-03-16 01:26:45 +0000
+++ nova/api/openstack/common.py	2011-03-23 18:10:37 +0000
@@ -15,7 +15,9 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-import webob.exc
+from urlparse import urlparse
+
+import webob
 
 from nova import exception
 
@@ -76,5 +78,14 @@
     raise exception.NotFound(image_hash)
 
 
-def get_api_version(req):
-    return req.environ.get('api.version')
+def get_id_from_href(href):
+    """Return the id portion of a url.
+
+    Given: http://www.foo.com/bar/123?q=4
+    Returns: 4
+
+    """
+    try:
+        return int(urlparse(href).path.split('/')[-1])
+    except:
+        raise webob.exc.HTTPBadRequest(_('could not parse id from href'))

=== modified file 'nova/api/openstack/servers.py'
--- nova/api/openstack/servers.py	2011-03-17 22:24:08 +0000
+++ nova/api/openstack/servers.py	2011-03-23 18:10:37 +0000
@@ -29,8 +29,9 @@
 from nova import utils
 from nova.api.openstack import common
 from nova.api.openstack import faults
-from nova.api.openstack.views import servers as servers_views
-from nova.api.openstack.views import addresses as addresses_views
+import nova.api.openstack.views.addresses
+import nova.api.openstack.views.flavors
+import nova.api.openstack.views.servers
 from nova.auth import manager as auth_manager
 from nova.compute import instance_types
 from nova.compute import power_state
@@ -63,7 +64,7 @@
         except exception.NotFound:
             return faults.Fault(exc.HTTPNotFound())
 
-        builder = addresses_views.get_view_builder(req)
+        builder = self._get_addresses_view_builder(req)
         return builder.build(instance)
 
     def index(self, req):
@@ -81,7 +82,7 @@
         """
         instance_list = self.compute_api.get_all(req.environ['nova.context'])
         limited_list = common.limited(instance_list, req)
-        builder = servers_views.get_view_builder(req)
+        builder = self._get_view_builder(req)
         servers = [builder.build(inst, is_detail)['server']
                 for inst in limited_list]
         return dict(servers=servers)
@@ -90,7 +91,7 @@
         """ Returns server details by server id """
         try:
             instance = self.compute_api.get(req.environ['nova.context'], id)
-            builder = servers_views.get_view_builder(req)
+            builder = self._get_view_builder(req)
             return builder.build(instance, is_detail=True)
         except exception.NotFound:
             return faults.Fault(exc.HTTPNotFound())
@@ -119,8 +120,9 @@
             key_name = key_pair['name']
             key_data = key_pair['public_key']
 
+        requested_image_id = self._image_id_from_req_data(env)
         image_id = common.get_image_id_from_image_hash(self._image_service,
-            context, env['server']['imageId'])
+            context, requested_image_id)
         kernel_id, ramdisk_id = self._get_kernel_ramdisk_from_image(
             req, image_id)
 
@@ -139,10 +141,11 @@
         if personality:
             injected_files = self._get_injected_files(personality)
 
+        flavor_id = self._flavor_id_from_req_data(env)
         try:
-            instances = self.compute_api.create(
+            (inst,) = self.compute_api.create(
                 context,
-                instance_types.get_by_flavor_id(env['server']['flavorId']),
+                instance_types.get_by_flavor_id(flavor_id),
                 image_id,
                 kernel_id=kernel_id,
                 ramdisk_id=ramdisk_id,
@@ -155,8 +158,11 @@
         except QuotaError as error:
             self._handle_quota_errors(error)
 
-        builder = servers_views.get_view_builder(req)
-        server = builder.build(instances[0], is_detail=False)
+        inst['instance_type'] = flavor_id
+        inst['image_id'] = requested_image_id
+
+        builder = self._get_view_builder(req)
+        server = builder.build(inst, is_detail=True)
         password = "%s%s" % (server['server']['name'][:4],
                              utils.generate_password(12))
         server['server']['adminPass'] = password
@@ -511,6 +517,45 @@
         return kernel_id, ramdisk_id
 
 
+class ControllerV10(Controller):
+    def _image_id_from_req_data(self, data):
+        return data['server']['imageId']
+
+    def _flavor_id_from_req_data(self, data):
+        return data['server']['flavorId']
+
+    def _get_view_builder(self, req):
+        addresses_builder = nova.api.openstack.views.addresses.ViewBuilderV10()
+        return nova.api.openstack.views.servers.ViewBuilderV10(
+            addresses_builder)
+
+    def _get_addresses_view_builder(self, req):
+        return nova.api.openstack.views.addresses.ViewBuilderV10(req)
+
+
+class ControllerV11(Controller):
+    def _image_id_from_req_data(self, data):
+        href = data['server']['imageRef']
+        return common.get_id_from_href(href)
+
+    def _flavor_id_from_req_data(self, data):
+        href = data['server']['flavorRef']
+        return common.get_id_from_href(href)
+
+    def _get_view_builder(self, req):
+        base_url = req.application_url
+        flavor_builder = nova.api.openstack.views.flavors.ViewBuilderV11(
+            base_url)
+        image_builder = nova.api.openstack.views.images.ViewBuilderV11(
+            base_url)
+        addresses_builder = nova.api.openstack.views.addresses.ViewBuilderV11()
+        return nova.api.openstack.views.servers.ViewBuilderV11(
+            addresses_builder, flavor_builder, image_builder)
+
+    def _get_addresses_view_builder(self, req):
+        return nova.api.openstack.views.addresses.ViewBuilderV11(req)
+
+
 class ServerCreateRequestXMLDeserializer(object):
     """
     Deserializer to handle xml-formatted server create requests.

=== modified file 'nova/api/openstack/views/addresses.py'
--- nova/api/openstack/views/addresses.py	2011-03-15 23:55:13 +0000
+++ nova/api/openstack/views/addresses.py	2011-03-23 18:10:37 +0000
@@ -19,18 +19,6 @@
 from nova.api.openstack import common
 
 
-def get_view_builder(req):
-    '''
-    A factory method that returns the correct builder based on the version of
-    the api requested.
-    '''
-    version = common.get_api_version(req)
-    if version == '1.1':
-        return ViewBuilder_1_1()
-    else:
-        return ViewBuilder_1_0()
-
-
 class ViewBuilder(object):
     ''' Models a server addresses response as a python dictionary.'''
 
@@ -38,14 +26,14 @@
         raise NotImplementedError()
 
 
-class ViewBuilder_1_0(ViewBuilder):
+class ViewBuilderV10(ViewBuilder):
     def build(self, inst):
         private_ips = utils.get_from_path(inst, 'fixed_ip/address')
         public_ips = utils.get_from_path(inst, 'fixed_ip/floating_ips/address')
         return dict(public=public_ips, private=private_ips)
 
 
-class ViewBuilder_1_1(ViewBuilder):
+class ViewBuilderV11(ViewBuilder):
     def build(self, inst):
         private_ips = utils.get_from_path(inst, 'fixed_ip/address')
         private_ips = [dict(version=4, addr=a) for a in private_ips]

=== modified file 'nova/api/openstack/views/flavors.py'
--- nova/api/openstack/views/flavors.py	2011-03-16 01:26:45 +0000
+++ nova/api/openstack/views/flavors.py	2011-03-23 18:10:37 +0000
@@ -18,19 +18,6 @@
 from nova.api.openstack import common
 
 
-def get_view_builder(req):
-    '''
-    A factory method that returns the correct builder based on the version of
-    the api requested.
-    '''
-    version = common.get_api_version(req)
-    base_url = req.application_url
-    if version == '1.1':
-        return ViewBuilder_1_1(base_url)
-    else:
-        return ViewBuilder_1_0()
-
-
 class ViewBuilder(object):
     def __init__(self):
         pass
@@ -39,13 +26,9 @@
         raise NotImplementedError()
 
 
-class ViewBuilder_1_1(ViewBuilder):
+class ViewBuilderV11(ViewBuilder):
     def __init__(self, base_url):
         self.base_url = base_url
 
     def generate_href(self, flavor_id):
         return "%s/flavors/%s" % (self.base_url, flavor_id)
-
-
-class ViewBuilder_1_0(ViewBuilder):
-    pass

=== modified file 'nova/api/openstack/views/images.py'
--- nova/api/openstack/views/images.py	2011-03-16 01:26:45 +0000
+++ nova/api/openstack/views/images.py	2011-03-23 18:10:37 +0000
@@ -18,19 +18,6 @@
 from nova.api.openstack import common
 
 
-def get_view_builder(req):
-    '''
-    A factory method that returns the correct builder based on the version of
-    the api requested.
-    '''
-    version = common.get_api_version(req)
-    base_url = req.application_url
-    if version == '1.1':
-        return ViewBuilder_1_1(base_url)
-    else:
-        return ViewBuilder_1_0()
-
-
 class ViewBuilder(object):
     def __init__(self):
         pass
@@ -39,13 +26,9 @@
         raise NotImplementedError()
 
 
-class ViewBuilder_1_1(ViewBuilder):
+class ViewBuilderV11(ViewBuilder):
     def __init__(self, base_url):
         self.base_url = base_url
 
     def generate_href(self, image_id):
         return "%s/images/%s" % (self.base_url, image_id)
-
-
-class ViewBuilder_1_0(ViewBuilder):
-    pass

=== modified file 'nova/api/openstack/views/servers.py'
--- nova/api/openstack/views/servers.py	2011-03-15 23:55:13 +0000
+++ nova/api/openstack/views/servers.py	2011-03-23 18:10:37 +0000
@@ -24,45 +24,30 @@
 from nova import utils
 
 
-def get_view_builder(req):
-    '''
-    A factory method that returns the correct builder based on the version of
-    the api requested.
-    '''
-    version = common.get_api_version(req)
-    addresses_builder = addresses_view.get_view_builder(req)
-    if version == '1.1':
-        flavor_builder = flavors_view.get_view_builder(req)
-        image_builder = images_view.get_view_builder(req)
-        return ViewBuilder_1_1(addresses_builder, flavor_builder,
-                               image_builder)
-    else:
-        return ViewBuilder_1_0(addresses_builder)
-
-
 class ViewBuilder(object):
-    '''
-    Models a server response as a python dictionary.
+    """Model a server response as a python dictionary.
+
+    Public methods: build
     Abstract methods: _build_image, _build_flavor
-    '''
+
+    """
 
     def __init__(self, addresses_builder):
         self.addresses_builder = addresses_builder
 
     def build(self, inst, is_detail):
-        """
-        Coerces into dictionary format, mapping everything to
-        Rackspace-like attributes for return
-        """
+        """Return a dict that represenst a server."""
         if is_detail:
             return self._build_detail(inst)
         else:
             return self._build_simple(inst)
 
     def _build_simple(self, inst):
-            return dict(server=dict(id=inst['id'], name=inst['display_name']))
+        """Return a simple model of a server."""
+        return dict(server=dict(id=inst['id'], name=inst['display_name']))
 
     def _build_detail(self, inst):
+        """Returns a detailed model of a server."""
         power_mapping = {
             None: 'build',
             power_state.NOSTATE: 'build',
@@ -76,25 +61,19 @@
             power_state.FAILED: 'error'}
         inst_dict = {}
 
-        #mapped_keys = dict(status='state', imageId='image_id',
-        #    flavorId='instance_type', name='display_name', id='id')
-
-        mapped_keys = dict(status='state', name='display_name', id='id')
-
-        for k, v in mapped_keys.iteritems():
-            inst_dict[k] = inst[v]
-
-        inst_dict['status'] = power_mapping[inst_dict['status']]
+        inst_dict['id'] = int(inst['id'])
+        inst_dict['name'] = inst['display_name']
+        inst_dict['status'] = power_mapping[inst.get('state')]
         inst_dict['addresses'] = self.addresses_builder.build(inst)
 
         # Return the metadata as a dictionary
         metadata = {}
-        for item in inst['metadata']:
+        for item in inst.get('metadata', []):
             metadata[item['key']] = item['value']
         inst_dict['metadata'] = metadata
 
         inst_dict['hostId'] = ''
-        if inst['host']:
+        if inst.get('host'):
             inst_dict['hostId'] = hashlib.sha224(inst['host']).hexdigest()
 
         self._build_image(inst_dict, inst)
@@ -103,21 +82,27 @@
         return dict(server=inst_dict)
 
     def _build_image(self, response, inst):
+        """Return the image sub-resource of a server."""
         raise NotImplementedError()
 
     def _build_flavor(self, response, inst):
+        """Return the flavor sub-resource of a server."""
         raise NotImplementedError()
 
 
-class ViewBuilder_1_0(ViewBuilder):
+class ViewBuilderV10(ViewBuilder):
+    """Model an Openstack API V1.0 server response."""
+
     def _build_image(self, response, inst):
-        response["imageId"] = inst["image_id"]
+        response['imageId'] = inst['image_id']
 
     def _build_flavor(self, response, inst):
-        response["flavorId"] = inst["instance_type"]
-
-
-class ViewBuilder_1_1(ViewBuilder):
+        response['flavorId'] = inst['instance_type']
+
+
+class ViewBuilderV11(ViewBuilder):
+    """Model an Openstack API V1.0 server response."""
+
     def __init__(self, addresses_builder, flavor_builder, image_builder):
         ViewBuilder.__init__(self, addresses_builder)
         self.flavor_builder = flavor_builder

=== modified file 'nova/tests/api/openstack/fakes.py'
--- nova/tests/api/openstack/fakes.py	2011-03-18 18:07:24 +0000
+++ nova/tests/api/openstack/fakes.py	2011-03-23 18:10:37 +0000
@@ -72,14 +72,18 @@
     return self.application
 
 
-def wsgi_app(inner_application=None):
-    if not inner_application:
-        inner_application = openstack.APIRouter()
+def wsgi_app(inner_app10=None, inner_app11=None):
+    if not inner_app10:
+        inner_app10 = openstack.APIRouterV10()
+    if not inner_app11:
+        inner_app11 = openstack.APIRouterV11()
     mapper = urlmap.URLMap()
-    api = openstack.FaultWrapper(auth.AuthMiddleware(
-              limits.RateLimitingMiddleware(inner_application)))
-    mapper['/v1.0'] = api
-    mapper['/v1.1'] = api
+    api10 = openstack.FaultWrapper(auth.AuthMiddleware(
+              limits.RateLimitingMiddleware(inner_app10)))
+    api11 = openstack.FaultWrapper(auth.AuthMiddleware(
+              limits.RateLimitingMiddleware(inner_app11)))
+    mapper['/v1.0'] = api10
+    mapper['/v1.1'] = api11
     mapper['/'] = openstack.FaultWrapper(openstack.Versions())
     return mapper
 

=== modified file 'nova/tests/api/openstack/test_auth.py'
--- nova/tests/api/openstack/test_auth.py	2011-03-20 19:04:51 +0000
+++ nova/tests/api/openstack/test_auth.py	2011-03-23 18:10:37 +0000
@@ -83,8 +83,7 @@
         self.assertEqual(result.headers['X-Storage-Url'], "")
 
         token = result.headers['X-Auth-Token']
-        self.stubs.Set(nova.api.openstack, 'APIRouter',
-            fakes.FakeRouter)
+        self.stubs.Set(nova.api.openstack, 'APIRouterV10', fakes.FakeRouter)
         req = webob.Request.blank('/v1.0/fake')
         req.headers['X-Auth-Token'] = token
         result = req.get_response(fakes.wsgi_app())
@@ -201,8 +200,7 @@
         self.assertEqual(len(result.headers['X-Auth-Token']), 40)
 
         token = result.headers['X-Auth-Token']
-        self.stubs.Set(nova.api.openstack, 'APIRouter',
-            fakes.FakeRouter)
+        self.stubs.Set(nova.api.openstack, 'APIRouterV10', fakes.FakeRouter)
         req = webob.Request.blank('/v1.0/fake')
         req.method = 'POST'
         req.headers['X-Auth-Token'] = token

=== modified file 'nova/tests/api/openstack/test_servers.py'
--- nova/tests/api/openstack/test_servers.py	2011-03-20 19:06:22 +0000
+++ nova/tests/api/openstack/test_servers.py	2011-03-23 18:10:37 +0000
@@ -161,7 +161,7 @@
         req = webob.Request.blank('/v1.0/servers/1')
         res = req.get_response(fakes.wsgi_app())
         res_dict = json.loads(res.body)
-        self.assertEqual(res_dict['server']['id'], '1')
+        self.assertEqual(res_dict['server']['id'], 1)
         self.assertEqual(res_dict['server']['name'], 'server1')
 
     def test_get_server_by_id_with_addresses(self):
@@ -172,7 +172,7 @@
         req = webob.Request.blank('/v1.0/servers/1')
         res = req.get_response(fakes.wsgi_app())
         res_dict = json.loads(res.body)
-        self.assertEqual(res_dict['server']['id'], '1')
+        self.assertEqual(res_dict['server']['id'], 1)
         self.assertEqual(res_dict['server']['name'], 'server1')
         addresses = res_dict['server']['addresses']
         self.assertEqual(len(addresses["public"]), len(public))
@@ -180,7 +180,7 @@
         self.assertEqual(len(addresses["private"]), 1)
         self.assertEqual(addresses["private"][0], private)
 
-    def test_get_server_by_id_with_addresses_v1_1(self):
+    def test_get_server_by_id_with_addresses_v11(self):
         private = "192.168.0.3"
         public = ["1.2.3.4"]
         new_return_server = return_server_with_addresses(private, public)
@@ -189,7 +189,7 @@
         req.environ['api.version'] = '1.1'
         res = req.get_response(fakes.wsgi_app())
         res_dict = json.loads(res.body)
-        self.assertEqual(res_dict['server']['id'], '1')
+        self.assertEqual(res_dict['server']['id'], 1)
         self.assertEqual(res_dict['server']['name'], 'server1')
         addresses = res_dict['server']['addresses']
         self.assertEqual(len(addresses["public"]), len(public))
@@ -239,7 +239,7 @@
         servers = json.loads(res.body)['servers']
         self.assertEqual([s['id'] for s in servers], [1, 2])
 
-    def _test_create_instance_helper(self):
+    def _setup_for_create_instance(self):
         """Shared implementation for tests below that create instance"""
         def instance_create(context, inst):
             return {'id': '1', 'display_name': 'server_test'}
@@ -276,14 +276,17 @@
         self.stubs.Set(nova.api.openstack.common,
             "get_image_id_from_image_hash", image_id_from_hash)
 
+    def _test_create_instance_helper(self):
+        self._setup_for_create_instance()
+
         body = dict(server=dict(
-            name='server_test', imageId=2, flavorId=2,
+            name='server_test', imageId=3, flavorId=2,
             metadata={'hello': 'world', 'open': 'stack'},
             personality={}))
         req = webob.Request.blank('/v1.0/servers')
         req.method = 'POST'
         req.body = json.dumps(body)
-        req.headers["Content-Type"] = "application/json"
+        req.headers["content-type"] = "application/json"
 
         res = req.get_response(fakes.wsgi_app())
 
@@ -291,8 +294,9 @@
         self.assertEqual('serv', server['adminPass'][:4])
         self.assertEqual(16, len(server['adminPass']))
         self.assertEqual('server_test', server['name'])
-        self.assertEqual('1', server['id'])
-
+        self.assertEqual(1, server['id'])
+        self.assertEqual(2, server['flavorId'])
+        self.assertEqual(3, server['imageId'])
         self.assertEqual(res.status_int, 200)
 
     def test_create_instance(self):
@@ -302,6 +306,56 @@
         fakes.stub_out_key_pair_funcs(self.stubs, have_key_pair=False)
         self._test_create_instance_helper()
 
+    def test_create_instance_v11(self):
+        self._setup_for_create_instance()
+
+        imageRef = 'http://localhost/v1.1/images/2'
+        flavorRef = 'http://localhost/v1.1/flavors/3'
+        body = {
+            'server': {
+                'name': 'server_test',
+                'imageRef': imageRef,
+                'flavorRef': flavorRef,
+                'metadata': {
+                    'hello': 'world',
+                    'open': 'stack',
+                },
+                'personality': {},
+            },
+        }
+
+        req = webob.Request.blank('/v1.1/servers')
+        req.method = 'POST'
+        req.body = json.dumps(body)
+        req.headers["content-type"] = "application/json"
+
+        res = req.get_response(fakes.wsgi_app())
+
+        server = json.loads(res.body)['server']
+        self.assertEqual('serv', server['adminPass'][:4])
+        self.assertEqual(16, len(server['adminPass']))
+        self.assertEqual('server_test', server['name'])
+        self.assertEqual(1, server['id'])
+        self.assertEqual(flavorRef, server['flavorRef'])
+        self.assertEqual(imageRef, server['imageRef'])
+        self.assertEqual(res.status_int, 200)
+
+    def test_create_instance_v11_bad_href(self):
+        self._setup_for_create_instance()
+
+        imageRef = 'http://localhost/v1.1/images/asdf'
+        flavorRef = 'http://localhost/v1.1/flavors/3'
+        body = dict(server=dict(
+            name='server_test', imageRef=imageRef, flavorRef=flavorRef,
+            metadata={'hello': 'world', 'open': 'stack'},
+            personality={}))
+        req = webob.Request.blank('/v1.1/servers')
+        req.method = 'POST'
+        req.body = json.dumps(body)
+        req.headers["content-type"] = "application/json"
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 400)
+
     def test_update_no_body(self):
         req = webob.Request.blank('/v1.0/servers/1')
         req.method = 'PUT'