Merge lp:~blamar/nova/openstack-api-1-1-images into lp:~hudson-openstack/nova/trunk

[Brian Lamar]

Adds support for versioned requests on /images through the OpenStack API.

[Brian Waldon (bcwaldon)]

Many of your method-level comments refer to 'webob.Request'. This should actually be 'wsgi.Request'.

155: The _builder_dispatch dict can probably go away now. _get_builder takes care of it.

209: Looks like you called get_builder and build incorrectly: "return self.get_builder().build(req, image, True)" should be "return self.get_builder(req).build(image, True)"

review: Needs Fixing

[Brian Lamar (blamar)]

Changes reflect your comments. I found out I wasn't even testing show(), which should now be fully tested.

Moving back to ready for review.

[Brian Lamar (blamar)]

Working on merging trunk this morning after a couple of relevant merges which happened last night. Shouldn't take long (hopefully).

[Brian Waldon (bcwaldon)]

Looking good! What would you think about changing the generate_href method from:
    "%s/images/%s" % (self._url, image_id)
to:
    os.path.join(self._url, "images", image_id)

[Brian Waldon (bcwaldon)]

I should also point out to anybody reviewing this that the prerequisite branch has been merged into trunk.

[Jay Pipes (jaypipes)]

Hi! Good stuff! A few suggestions from me:

173 + self.__compute = compute_service or compute.API()
174 + self.__image = image_service or _default_service

Please use zero or a single underscore, not two, for attributes. One underscore indicates a "private" attribute/method (even though Python has no concept of a private attribute, really... Generally, two underscores indicate that the attribute/method is special to the system (like __dict__, etc).

154 - "serverId", "progress"]}}}
...
158 + "serverId", "progress"],
159 + "link": ["rel", "type", "href"],
160 + },
161 + },
162 + }

This will break pep8 0.5.0. Just an FYI. If you run your tests with ./run_tests.sh -V, you would see this pop up. Whether pep8 0.6 should be in tools/pip-requires is a totally separate issue, but until it is, running tests in a virtualenv will fail with stuff like the above. Frankly, I think this particular pep8 rule is stupid and makes the code less-readable, but until pep8 0.6 is the standard used in tools/pip-requires, there's not much I can do about that...

While the rest of the code looks fine, I'd like Rick H. to take a looksie, because I have the feeling that his related_images branch will cause this branch to conflict, and the related_images branch has a number of improvements in it re: how we handle OS API /images keys, image properties, and how the translation from image service dicts to OS API dicts works...

-jay

review: Needs Fixing

[Brian Lamar (blamar)]

Hey Jay! Thanks for the comments:

> Please use zero or a single underscore, not two, for attributes.

Two leading underscores is Python's way of avoiding conflict with related sub/super class variables through 'name mangling'. I use this pretty regularly in my personal projects which is why it slipped in to this change. (http://docs.python.org/tutorial/classes.html#private-variables)

While I'll remove it if you/others think it doesn't belong, I technically think that any class member variable which isn't being used outside of the class it's defined should be 'Python private' to avoid conflicts...but maybe it's just a personal thing, are there any PEPs associated with the use of privates?

> This will break pep8 0.5.0.

We had a good discussion the other day about using specific versions in tools/pip-requires and I'd like to bring it up again since the last time you changed my mind about specifying specific versions.

Can I file a ticket to increase this version or are we linking it to the latest package release in Ubuntu? If any program we use should be the latest version, IMO it should be pep8.

[Brian Lamar (blamar)]

> If you run your tests with ./run_tests.sh -V

My results from ./run_tests.sh -V show:

Ran 532 tests in 394.149s

OK

and no pep8 failures.

[Brian Lamar (blamar)]

After thinking about it I'm going to rename those variables to something more appropriate and use a single underscore. It's more in line with other styles used in the project and an overall improvement. Thanks Jay!

[Rick Harris (rconradharris)]

> related_images branch will cause this branch to conflict

Yeah we ended up touching a lot of the same code, so this will almost certainly conflict.

Brian--

could you take a ganders at `related_images`:
https://code.launchpad.net/~rconradharris/nova/related_images/+merge/53374
and see how much you think the two will conflict?

[Brian Waldon (bcwaldon)]

Thanks for addressing my concerns

review: Approve

[Jay Pipes (jaypipes)]

On Thu, Mar 24, 2011 at 11:48 AM, Brian Lamar <email address hidden> wrote:
>> If you run your tests with ./run_tests.sh -V
>
> My results from ./run_tests.sh -V show:
>
> Ran 532 tests in 394.149s
>
> OK
>
> and no pep8 failures.

Heh, well, I must be out of my mind, then :) I ran your branch locally
and got this, totally non-related to this patch, error:

nova/tests/test_volume.py:359:63: E202 whitespace before ')'
                                    "--tid=%(tid)d" % locals()
                                                              ^
    Avoid extraneous whitespace in the following situations:

    - Immediately inside parentheses, brackets or braces.

    - Immediately before a comma, semicolon, or colon.

    Okay: spam(ham[1], {eggs: 2})
    E201: spam( ham[1], {eggs: 2})
    E201: spam(ham[ 1], {eggs: 2})
    E201: spam(ham[1], { eggs: 2})
    E202: spam(ham[1], {eggs: 2} )
    E202: spam(ham[1 ], {eggs: 2})
    E202: spam(ham[1], {eggs: 2 })

    E203: if x == 4: print x, y; x, y = y , x
    E203: if x == 4: print x, y ; x, y = y, x
    E203: if x == 4 : print x, y; x, y = y, x
jpipes@serialcoder:~/repos/nova/openstack-api-1-1-images$

It would *seem* that your code in /nova/api/openstack/images.py would
trigger the same error, but it doesn't! :)

-jay

[Rick Harris (rconradharris)]

Nice work Brian. A few comments:

General
=======

* Not much we can do here, but looks like this is gonna conflict pretty heavily with `related_images`

* I'm seeing quite a bit of dependency-injection being used. This can usually be avoid by stubbing out using Fakes and Mocks. This keeps the implementation clean since it won't have to take `client`, `image_service`, `compute_service` args. This isn't a deal-breaker for this patch (by any means), but wanted to raise it for discussion.

Specifics
=========

> 235 + ex = webob.exc.HTTPNotFound(explanation="Image not found.")

Needs i18n treatment.

> 238 + return dict(image=self.get_builder(req).build(image, True))

Might be a little clearer if a kwarg is used, like:

    return dict(image=self.get_builder(req).build(image, detail=True))

> 464 now = datetime.datetime.utcnow()

It's usually not a good idea to use now-now in tests, since the tests, in a way, change each time. For example, you can see a test pass everyday but fail on Feb. 29th. Rather, you might want to do something like:

    NOW_DATETIME = datetime.datetime(2010, 10, 11, 10, 32, 23)
    NOW_STR = "2010-10-11T10:32:23Z"

review: Needs Fixing

[Brian Lamar (blamar)]

Thanks for the input Rick.

Unfortunately you're right in that this is going to conflict very heavily with the work you have been doing. I'd love to discuss how this can be avoided in the future. I try to keep up on Nova blueprints and the associated branches when they sound API specific but I think that you were potentially not working off a blueprint?

> I'm seeing quite a bit of dependency-injection being used.

Wow, I've never been told that DI is bad, can you potentially give me more details?

> 235 + ex = webob.exc.HTTPNotFound(explanation="Image not found.")

Updated, good catch.

> 238 + return dict(image=self.get_builder(req).build(image, True))

I don't agree in the general case, but it's the second time someone has mentioned this specific case to me. This has been fixed.

> It's usually not a good idea to use now-now in tests.

I'm not quite sure I follow the logic here as none of the tests do time-based comparison (which I agree is a no-no). Can you elaborate?

Thanks!

[Rick Harris (rconradharris)]

> Wow, I've never been told that DI is bad, can you potentially give me more details?

Sure. DI is a pretty common pattern static languages since it gives you the flexibility to test against something other than the original implementation.

With a dynamic language like Python, you get this for free, no dependency injection needed. All you need to do is re-bind the dependent-class in the setUp of the test. I believe we have plenty of examples of this pattern across our test suite. Here's an example:

DI-Style
========

class Dependent(object):
  pass

class DoesSomething(object):
  def __init__(self, dependent=None)
    self.dependent = dependent or Dependent()

class FakeDependent(object):
  pass

# test it
test = DoesSomething(dependent=FakeDependent)

Non-DI-Style
============

class DoesSomething(object):
  def __init__(self)
    self.dependent = Dependent()

# test it
Dependent = FakeDependent() # Rebind, use the `stubs` library in the real case
test = DoesSomething()

Notice that both ways achieve the same thing, the code ultimately uses FakeDependent. But with the Non-DI style, we don't have to add N number of dependents to the constructor.

> I'm not quite sure I follow the logic here as none of the tests do time-based comparison

In this case, that's true. This is more of a suggestion as a best-practice: tests should be invariant over time. I'm just pointing out that there is a hidden dependency on time here which might be come a problem in the future as new tests are added.

FWIW, I've been burnt by this in the past, that's why I'm raising the concern here.

> think that you were potentially not working off a blueprint?

Yeah, my fault here :/ There *is* a blueprint but it's under Glance since that's where most of the work was planning to take place. However, it turned out, it was much cleaner if the changes were made to Nova; at that point I should created a Nova blueprint and rejected the Glance prop. My apologies.

[Brian Lamar (blamar)]

Potential Reviewers,

I request this branch be merged in as a Feature Freeze Exception due to it's link to an approved Cactus blueprint (OpenStack API v1.1). The branch took a great deal of merge-conflict resolution but now the /images resource should be version-compatible.

Branch Benefits
===============
I believe a goal of Cactus was to make the OpenStack API more stable and complete, which is what this branch helps. Myself and my team will be looking at *throughly* testing the OpenStack API up until the Cactus release to ensure API stability and accuracy. Many tests are already included in this branch, but we are still lacking complete integration and acceptance testing.

Regression Risk
===============
I'm not sure how to quantify this, but we have multiple branches which were merged using the same versioning patter I've used here (OpenStack API v1.1 Flavors and Servers come to mind). I would rate regression risk quite low.

[Thierry Carrez (ttx)]

FFe granted, but please get this in asap and before Tuesday night.

review: Approve

[Rick Harris (rconradharris)]

Nice work, Brian. LGTM.

Caveat: wasn't able to get tests to pass, but I think that was a result of my hosed env. Could be a while before I can get that rebuilt, so approving anyway.

One small nit:

> 1036 + print self.fixtures

Accidentally left in?

review: Approve

[justinsb (justin-fathomdb)]

V1.1 requires an imageRef when creating a server. Are callers now supposed to dig through the links collection to find the imageRef?

Do we have a schema for V1.1 that we can check this against?

This is exactly why I think we need a client that we're maintaining, by the way. Our API should be a joy, and the way we find out where the rough edges are is when we code to it.

review: Needs Fixing

[Brian Lamar (blamar)]

@justinsb

> V1.1 requires an imageRef when creating a server

Yes.

> Are callers now supposed to dig through the links collection to find the imageRef?

GET /v1.1/images/1 would return an image which has a list of "links". One of those links would be a "self" link with a fully qualified URL which can be used as "imageRef" in a server creation.

> Do we have a schema for V1.1 that we can check this against?

No.

> This is exactly why I think we need a client that we're maintaining, by the way. Our API should be
> a joy, and the way we find out where the rough edges are is when we code to it.

Great thought, it's not something that we have right now, but it's something I'd love to have. A full set of end-to-end API tests along with XML (and JSON?) schema validation would help the API tremendously.

Is there something about this code specifically you're troubled with? I'd love to work it out as the primary purpose of this branch is to bring versioning to the "images" resource.

[termie (termie)]

- please format docstrings according to HACKING
- in tests, don't import parseString directly from minidom, import the module instead, as referenced in HACKING, and hte space before it is not necessary

I'll bow out of commenting on the design as it looks like others have already discussed it

review: Needs Fixing

[justinsb (justin-fathomdb)]

> GET /v1.1/images/1 would return an image which has a list of "links". One of those links would be a "self" link with a fully qualified URL which can be used as "imageRef" in a server creation.

I think we should return the imageRef as a simple attribute, so that it's easy for callers. We're still returning the image ID, right, but that's now useless? Or are we supposed to use the ID when doing REST operations through the OpenStack API? But then why aren't we using the imageRef - do we now have two URLs for an image (which will often be the same, until they're not, when people that have assumed they're the same will break?) If we're admitting the possibility of external images by using an imageRef, why is there an images controller in the OpenStack API at all?

This isn't your fault, but I find the introduction of imageRefs very confusing. It feels like we're trying to mix a simple 'owned image' model with a more powerful 'federated images' model, but we don't seem to have thought that through and figured out a way to fuse the two nicely.

We need to get this merged, and I guess this is on-spec, so I'm going to change to Approve. I'm not a fan of the spec though.

If we could return the imageRef as a simple attribute so that clients don't have to dig through the atom links, that would make me happier. Normally that would break the XSD, but as we don't have one... :-)

review: Approve

[Brian Lamar (blamar)]

@termie

> please format docstrings according to HACKING

Thanks for the review! Can you give me a bit more of a push with regards to docstrings? Are all of them wrong? PEP-257 is pretty lenient as to docstrings, is the :param foo and :returns: format required? HACKING is a little vauge sometimes for me. Some things seem like format suggestions and not standard requirements. Thanks!

> in tests, don't import parseString directly from minidom, import the module instead

Good catch, I've aliased xml.dom.minidom to "minidom" as I find xml.dom.minidom.parseString a painfully long call, I hope that is acceptable. Changes are in r826 and pushed.

Thanks again!

[Thierry Carrez (ttx)]

review: Approve (ffe)

[Jay Pipes (jaypipes)]

review: Approve

[Vish Ishaya (vishvananda)]

'the :param foo and :returns: format' works well with sphinx

For short docstrings:

"""This is much nicer."""

"""
Than This.
"""

The format that nova is using for multiline strings is:

"""One line description.

Other Info.

"""

[Brian Lamar (blamar)]

@termie

Docstrings have been update, can you please review to ensure HACKING compatibility? Thanks!

[Brian Lamar (blamar)]

Merged trunk and fixed conflict.

[Jay Pipes (jaypipes)]

Did this get messed up in merge?

52 +import webob.exc
53 import datetime
54
55 -from webob import exc

Should be:

import datetime

import webob.exc

-jay

review: Needs Fixing

[Brian Lamar (blamar)]

Hmm, nope. Pretty sure it just wasn't caught before. Fixed now.

[Jay Pipes (jaypipes)]

ok by me.

review: Approve

[OpenStack Infra (hudson-openstack)]

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

[termie (termie)]

still missed a few, but i'm going to do a style sweep anyway so I guess no biggie for now.

[Brian Lamar (blamar)]

@termie

Can you *please* be more specific? It would really help me, especially with something as common as docstrings, to indicate specifics and line numbers when commenting. Thanks!

=== modified file 'nova/api/openstack/__init__.py'
--- nova/api/openstack/__init__.py	2011-03-28 20:57:35 +0000
+++ nova/api/openstack/__init__.py	2011-03-29 16:12:48 +0000
@@ -111,9 +111,6 @@
                         parent_resource=dict(member_name='server',
                         collection_name='servers'))
 
-        mapper.resource("image", "images", controller=images.Controller(),
-                        collection={'detail': 'GET'})
-
         _limits = limits.LimitsController()
         mapper.resource("limit", "limits", controller=_limits)
 
@@ -128,6 +125,10 @@
                         collection={'detail': 'GET'},
                         member=self.server_members)
 
+        mapper.resource("image", "images",
+                        controller=images.ControllerV10(),
+                        collection={'detail': 'GET'})
+
         mapper.resource("flavor", "flavors",
                         controller=flavors.ControllerV10(),
                         collection={'detail': 'GET'})
@@ -152,6 +153,10 @@
                         collection={'detail': 'GET'},
                         member=self.server_members)
 
+        mapper.resource("image", "images",
+                        controller=images.ControllerV11(),
+                        collection={'detail': 'GET'})
+
         mapper.resource("image_meta", "meta",
                         controller=image_metadata.Controller(),
                         parent_resource=dict(member_name='image',
 
=== modified file 'nova/api/openstack/images.py'
--- nova/api/openstack/images.py	2011-03-24 21:13:55 +0000
+++ nova/api/openstack/images.py	2011-03-29 16:12:48 +0000
@@ -1,6 +1,4 @@
-# vim: tabstop=4 shiftwidth=4 softtabstop=4
-
-# Copyright 2010 OpenStack LLC.
+# Copyright 2011 OpenStack LLC.
 # All Rights Reserved.
 #
 #    Licensed under the Apache License, Version 2.0 (the "License"); you may
@@ -17,7 +15,7 @@
 
 import datetime
 
-from webob import exc
+import webob.exc
 
 from nova import compute
 from nova import exception
@@ -25,238 +23,133 @@
 from nova import log
 from nova import utils
 from nova import wsgi
-import nova.api.openstack
 from nova.api.openstack import common
 from nova.api.openstack import faults
-import nova.image.service
+from nova.api.openstack.views import images as images_view
 
 
 LOG = log.getLogger('nova.api.openstack.images')
-
 FLAGS = flags.FLAGS
 
 
-def _translate_keys(item):
-    """
-    Maps key names to Rackspace-like attributes for return
-    also pares down attributes to those we want
-    item is a dict
-
-    Note: should be removed when the set of keys expected by the api
-    and the set of keys returned by the image service are equivalent
-
-    """
-    # TODO(tr3buchet): this map is specific to s3 object store,
-    # replace with a list of keys for _filter_keys later
-    mapped_keys = {'status': 'imageState',
-                   'id': 'imageId',
-                   'name': 'imageLocation'}
-
-    mapped_item = {}
-    # TODO(tr3buchet):
-    # this chunk of code works with s3 and the local image service/glance
-    # when we switch to glance/local image service it can be replaced with
-    # a call to _filter_keys, and mapped_keys can be changed to a list
-    try:
-        for k, v in mapped_keys.iteritems():
-            # map s3 fields
-            mapped_item[k] = item[v]
-    except KeyError:
-        # return only the fields api expects
-        mapped_item = _filter_keys(item, mapped_keys.keys())
-
-    return mapped_item
-
-
-def _translate_status(item):
-    """
-    Translates status of image to match current Rackspace api bindings
-    item is a dict
-
-    Note: should be removed when the set of statuses expected by the api
-    and the set of statuses returned by the image service are equivalent
-
-    """
-    status_mapping = {
-        'pending': 'queued',
-        'decrypting': 'preparing',
-        'untarring': 'saving',
-        'available': 'active'}
-    try:
-        item['status'] = status_mapping[item['status']]
-    except KeyError:
-        # TODO(sirp): Performing translation of status (if necessary) here for
-        # now. Perhaps this should really be done in EC2 API and
-        # S3ImageService
-        pass
-
-
-def _filter_keys(item, keys):
-    """
-    Filters all model attributes except for keys
-    item is a dict
-
-    """
-    return dict((k, v) for k, v in item.iteritems() if k in keys)
-
-
-def _convert_image_id_to_hash(image):
-    if 'imageId' in image:
-        # Convert EC2-style ID (i-blah) to Rackspace-style (int)
-        image_id = abs(hash(image['imageId']))
-        image['imageId'] = image_id
-        image['id'] = image_id
-
-
-def _translate_s3_like_images(image_metadata):
-    """Work-around for leaky S3ImageService abstraction"""
-    api_metadata = image_metadata.copy()
-    _convert_image_id_to_hash(api_metadata)
-    api_metadata = _translate_keys(api_metadata)
-    _translate_status(api_metadata)
-    return api_metadata
-
-
-def _translate_from_image_service_to_api(image_metadata):
-    """Translate from ImageService to OpenStack API style attribute names
-
-    This involves 4 steps:
-
-        1. Filter out attributes that the OpenStack API doesn't need
-
-        2. Translate from base image attributes from names used by
-           BaseImageService to names used by OpenStack API
-
-        3. Add in any image properties
-
-        4. Format values according to API spec (for example dates must
-           look like "2010-08-10T12:00:00Z")
-    """
-    service_metadata = image_metadata.copy()
-    properties = service_metadata.pop('properties', {})
-
-    # 1. Filter out unecessary attributes
-    api_keys = ['id', 'name', 'updated_at', 'created_at', 'status']
-    api_metadata = utils.subset_dict(service_metadata, api_keys)
-
-    # 2. Translate base image attributes
-    api_map = {'updated_at': 'updated', 'created_at': 'created'}
-    api_metadata = utils.map_dict_keys(api_metadata, api_map)
-
-    # 3. Add in any image properties
-    # 3a. serverId is used for backups and snapshots
-    try:
-        api_metadata['serverId'] = int(properties['instance_id'])
-    except KeyError:
-        pass  # skip if it's not present
-    except ValueError:
-        pass  # skip if it's not an integer
-
-    # 3b. Progress special case
-    # TODO(sirp): ImageService doesn't have a notion of progress yet, so for
-    # now just fake it
-    if service_metadata['status'] == 'saving':
-        api_metadata['progress'] = 0
-
-    # 4. Format values
-    # 4a. Format Image Status (API requires uppercase)
-    api_metadata['status'] = _format_status_for_api(api_metadata['status'])
-
-    # 4b. Format timestamps
-    for attr in ('created', 'updated'):
-        if attr in api_metadata:
-            api_metadata[attr] = _format_datetime_for_api(
-                api_metadata[attr])
-
-    return api_metadata
-
-
-def _format_status_for_api(status):
-    """Return status in a format compliant with OpenStack API"""
-    mapping = {'queued': 'QUEUED',
-               'preparing': 'PREPARING',
-               'saving': 'SAVING',
-               'active': 'ACTIVE',
-               'killed': 'FAILED'}
-    return mapping[status]
-
-
-def _format_datetime_for_api(datetime_):
-    """Stringify datetime objects in a format compliant with OpenStack API"""
-    API_DATETIME_FMT = '%Y-%m-%dT%H:%M:%SZ'
-    return datetime_.strftime(API_DATETIME_FMT)
-
-
-def _safe_translate(image_metadata):
-    """Translate attributes for OpenStack API, temporary workaround for
-    S3ImageService attribute leakage.
-    """
-    # FIXME(sirp): The S3ImageService appears to be leaking implementation
-    # details, including its internal attribute names, and internal
-    # `status` values. Working around it for now.
-    s3_like_image = ('imageId' in image_metadata)
-    if s3_like_image:
-        translate = _translate_s3_like_images
-    else:
-        translate = _translate_from_image_service_to_api
-    return translate(image_metadata)
-
-
 class Controller(wsgi.Controller):
+    """Base `wsgi.Controller` for retrieving/displaying images."""
 
     _serialization_metadata = {
         'application/xml': {
             "attributes": {
                 "image": ["id", "name", "updated", "created", "status",
-                          "serverId", "progress"]}}}
-
-    def __init__(self):
-        self._service = utils.import_object(FLAGS.image_service)
+                          "serverId", "progress"],
+                "link": ["rel", "type", "href"],
+            },
+        },
+    }
+
+    def __init__(self, image_service=None, compute_service=None):
+        """Initialize new `ImageController`.
+
+        :param compute_service: `nova.compute.api:API`
+        :param image_service: `nova.image.service:BaseImageService`
+        """
+        _default_service = utils.import_object(flags.FLAGS.image_service)
+
+        self._compute_service = compute_service or compute.API()
+        self._image_service = image_service or _default_service
 
     def index(self, req):
-        """Return all public images in brief"""
+        """Return an index listing of images available to the request.
+
+        :param req: `wsgi.Request` object
+        """
         context = req.environ['nova.context']
-        image_metas = self._service.index(context)
-        image_metas = common.limited(image_metas, req)
-        return dict(images=image_metas)
+        images = self._image_service.index(context)
+        images = common.limited(images, req)
+        builder = self.get_builder(req).build
+        return dict(images=[builder(image, detail=False) for image in images])
 
     def detail(self, req):
-        """Return all public images in detail"""
+        """Return a detailed index listing of images available to the request.
+
+        :param req: `wsgi.Request` object.
+        """
         context = req.environ['nova.context']
-        image_metas = self._service.detail(context)
-        image_metas = common.limited(image_metas, req)
-        api_image_metas = [_safe_translate(image_meta)
-                           for image_meta in image_metas]
-        return dict(images=api_image_metas)
+        images = self._image_service.detail(context)
+        images = common.limited(images, req)
+        builder = self.get_builder(req).build
+        return dict(images=[builder(image, detail=True) for image in images])
 
     def show(self, req, id):
-        """Return data about the given image id"""
+        """Return detailed information about a specific image.
+
+        :param req: `wsgi.Request` object
+        :param id: Image identifier (integer)
+        """
         context = req.environ['nova.context']
-        try:
-            image_id = common.get_image_id_from_image_hash(
-                self._service, context, id)
+
+        try:
+            image_id = int(id)
+        except ValueError:
+            explanation = _("Image not found.")
+            raise faults.Fault(webob.exc.HTTPNotFound(explanation=explanation))
+
+        try:
+            image = self._image_service.show(context, image_id)
         except exception.NotFound:
-            raise faults.Fault(exc.HTTPNotFound())
+            explanation = _("Image '%d' not found.") % (image_id)
+            raise faults.Fault(webob.exc.HTTPNotFound(explanation=explanation))
 
-        image_meta = self._service.show(context, image_id)
-        api_image_meta = _safe_translate(image_meta)
-        return dict(image=api_image_meta)
+        return dict(image=self.get_builder(req).build(image, detail=True))
 
     def delete(self, req, id):
-        # Only public images are supported for now.
-        raise faults.Fault(exc.HTTPNotFound())
+        """Delete an image, if allowed.
+
+        :param req: `wsgi.Request` object
+        :param id: Image identifier (integer)
+        """
+        image_id = id
+        context = req.environ['nova.context']
+        self._image_service.delete(context, image_id)
+        return webob.exc.HTTPNoContent()
 
     def create(self, req):
+        """Snapshot a server instance and save the image.
+
+        :param req: `wsgi.Request` object
+        """
         context = req.environ['nova.context']
-        env = self._deserialize(req.body, req.get_content_type())
-        instance_id = env["image"]["serverId"]
-        name = env["image"]["name"]
-        image_meta = compute.API().snapshot(
-            context, instance_id, name)
-        api_image_meta = _safe_translate(image_meta)
-        return dict(image=api_image_meta)
-
-    def update(self, req, id):
-        # Users may not modify public images, and that's all that
-        # we support for now.
-        raise faults.Fault(exc.HTTPNotFound())
+        content_type = req.get_content_type()
+        image = self._deserialize(req.body, content_type)
+
+        if not image:
+            raise webob.exc.HTTPBadRequest()
+
+        try:
+            server_id = image["image"]["serverId"]
+            image_name = image["image"]["name"]
+        except KeyError:
+            raise webob.exc.HTTPBadRequest()
+
+        image = self._compute_service.snapshot(context, server_id, image_name)
+        return self.get_builder(req).build(image, detail=True)
+
+    def get_builder(self, request):
+        """Indicates that you must use a Controller subclass."""
+        raise NotImplementedError
+
+
+class ControllerV10(Controller):
+    """Version 1.0 specific controller logic."""
+
+    def get_builder(self, request):
+        """Property to get the ViewBuilder class we need to use."""
+        base_url = request.application_url
+        return images_view.ViewBuilderV10(base_url)
+
+
+class ControllerV11(Controller):
+    """Version 1.1 specific controller logic."""
+
+    def get_builder(self, request):
+        """Property to get the ViewBuilder class we need to use."""
+        base_url = request.application_url
+        return images_view.ViewBuilderV11(base_url)
 
=== modified file 'nova/api/openstack/views/images.py'
--- nova/api/openstack/views/images.py	2011-03-17 07:41:01 +0000
+++ nova/api/openstack/views/images.py	2011-03-29 16:12:48 +0000
@@ -15,20 +15,100 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-from nova.api.openstack import common
+import os.path
 
 
 class ViewBuilder(object):
-    def __init__(self):
-        pass
-
-    def build(self, image_obj):
-        raise NotImplementedError()
-
-
-class ViewBuilderV11(ViewBuilder):
+    """Base class for generating responses to OpenStack API image requests."""
+
     def __init__(self, base_url):
-        self.base_url = base_url
+        """Initialize new `ViewBuilder`."""
+        self._url = base_url
+
+    def _format_dates(self, image):
+        """Update all date fields to ensure standardized formatting."""
+        for attr in ['created_at', 'updated_at', 'deleted_at']:
+            if image.get(attr) is not None:
+                image[attr] = image[attr].strftime('%Y-%m-%dT%H:%M:%SZ')
+
+    def _format_status(self, image):
+        """Update the status field to standardize format."""
+        status_mapping = {
+            'pending': 'queued',
+            'decrypting': 'preparing',
+            'untarring': 'saving',
+            'available': 'active',
+            'killed': 'failed',
+        }
+
+        try:
+            image['status'] = status_mapping[image['status']].upper()
+        except KeyError:
+            image['status'] = image['status'].upper()
 
     def generate_href(self, image_id):
-        return "%s/images/%s" % (self.base_url, image_id)
+        """Return an href string pointing to this object."""
+        return os.path.join(self._url, "images", str(image_id))
+
+    def build(self, image_obj, detail=False):
+        """Return a standardized image structure for display by the API."""
+        properties = image_obj.get("properties", {})
+
+        self._format_dates(image_obj)
+
+        if "status" in image_obj:
+            self._format_status(image_obj)
+
+        image = {
+            "id": image_obj["id"],
+            "name": image_obj["name"],
+        }
+
+        if "instance_id" in properties:
+            try:
+                image["serverId"] = int(properties["instance_id"])
+            except ValueError:
+                pass
+
+        if detail:
+            image.update({
+                "created": image_obj["created_at"],
+                "updated": image_obj["updated_at"],
+                "status": image_obj["status"],
+            })
+
+            if image["status"] == "SAVING":
+                image["progress"] = 0
+
+        return image
+
+
+class ViewBuilderV10(ViewBuilder):
+    """OpenStack API v1.0 Image Builder"""
+    pass
+
+
+class ViewBuilderV11(ViewBuilder):
+    """OpenStack API v1.1 Image Builder"""
+
+    def build(self, image_obj, detail=False):
+        """Return a standardized image structure for display by the API."""
+        image = ViewBuilder.build(self, image_obj, detail)
+        href = self.generate_href(image_obj["id"])
+
+        image["links"] = [{
+            "rel": "self",
+            "href": href,
+        },
+        {
+            "rel": "bookmark",
+            "type": "application/json",
+            "href": href,
+        },
+        {
+            "rel": "bookmark",
+            "type": "application/xml",
+            "href": href,
+        }]
+
+        return image
 
=== modified file 'nova/tests/api/openstack/test_images.py'
--- nova/tests/api/openstack/test_images.py	2011-03-28 15:05:28 +0000
+++ nova/tests/api/openstack/test_images.py	2011-03-29 16:12:48 +0000
@@ -20,11 +20,13 @@
 and as a WSGI layer
 """
 
+import copy
 import json
 import datetime
 import os
 import shutil
 import tempfile
+import xml.dom.minidom as minidom
 
 import stubout
 import webob
@@ -214,12 +216,14 @@
 
 
 class ImageControllerWithGlanceServiceTest(test.TestCase):
-    """Test of the OpenStack API /images application controller"""
-
+    """
+    Test of the OpenStack API /images application controller w/Glance.
+    """
     NOW_GLANCE_FORMAT = "2010-10-11T10:30:22"
     NOW_API_FORMAT = "2010-10-11T10:30:22Z"
 
     def setUp(self):
+        """Run before each test."""
         super(ImageControllerWithGlanceServiceTest, self).setUp()
         self.orig_image_service = FLAGS.image_service
         FLAGS.image_service = 'nova.image.glance.GlanceImageService'
@@ -230,18 +234,30 @@
         fakes.stub_out_rate_limiting(self.stubs)
         fakes.stub_out_auth(self.stubs)
         fakes.stub_out_key_pair_funcs(self.stubs)
-        fixtures = self._make_image_fixtures()
-        fakes.stub_out_glance(self.stubs, initial_fixtures=fixtures)
+        self.fixtures = self._make_image_fixtures()
+        fakes.stub_out_glance(self.stubs, initial_fixtures=self.fixtures)
 
     def tearDown(self):
+        """Run after each test."""
         self.stubs.UnsetAll()
         FLAGS.image_service = self.orig_image_service
         super(ImageControllerWithGlanceServiceTest, self).tearDown()
 
+    def _applicable_fixture(self, fixture, user_id):
+        """Determine if this fixture is applicable for given user id."""
+        is_public = fixture["is_public"]
+        try:
+            uid = int(fixture["properties"]["user_id"])
+        except KeyError:
+            uid = None
+        return uid == user_id or is_public
+
     def test_get_image_index(self):
-        req = webob.Request.blank('/v1.0/images')
-        res = req.get_response(fakes.wsgi_app())
-        image_metas = json.loads(res.body)['images']
+        request = webob.Request.blank('/v1.0/images')
+        response = request.get_response(fakes.wsgi_app())
+
+        response_dict = json.loads(response.body)
+        response_list = response_dict["images"]
 
         expected = [{'id': 123, 'name': 'public image'},
                     {'id': 124, 'name': 'queued backup'},
@@ -249,32 +265,379 @@
                     {'id': 126, 'name': 'active backup'},
                     {'id': 127, 'name': 'killed backup'}]
 
-        self.assertDictListMatch(image_metas, expected)
+        self.assertDictListMatch(response_list, expected)
+
+    def test_get_image(self):
+        request = webob.Request.blank('/v1.0/images/123')
+        response = request.get_response(fakes.wsgi_app())
+
+        self.assertEqual(200, response.status_int)
+
+        actual_image = json.loads(response.body)
+
+        expected_image = {
+            "image": {
+                "id": 123,
+                "name": "public image",
+                "updated": self.NOW_API_FORMAT,
+                "created": self.NOW_API_FORMAT,
+                "status": "ACTIVE",
+            },
+        }
+
+        self.assertEqual(expected_image, actual_image)
+
+    def test_get_image_v1_1(self):
+        request = webob.Request.blank('/v1.1/images/123')
+        response = request.get_response(fakes.wsgi_app())
+
+        actual_image = json.loads(response.body)
+
+        href = "http://localhost/v1.1/images/123"
+
+        expected_image = {
+            "image": {
+                "id": 123,
+                "name": "public image",
+                "updated": self.NOW_API_FORMAT,
+                "created": self.NOW_API_FORMAT,
+                "status": "ACTIVE",
+                "links": [{
+                    "rel": "self",
+                    "href": href,
+                },
+                {
+                    "rel": "bookmark",
+                    "type": "application/json",
+                    "href": href,
+                },
+                {
+                    "rel": "bookmark",
+                    "type": "application/xml",
+                    "href": href,
+                }],
+            },
+        }
+
+        self.assertEqual(expected_image, actual_image)
+
+    def test_get_image_xml(self):
+        request = webob.Request.blank('/v1.0/images/123')
+        request.accept = "application/xml"
+        response = request.get_response(fakes.wsgi_app())
+
+        actual_image = minidom.parseString(response.body.replace("  ", ""))
+
+        expected_now = self.NOW_API_FORMAT
+        expected_image = minidom.parseString("""
+            <image id="123"
+                    name="public image"
+                    updated="%(expected_now)s"
+                    created="%(expected_now)s"
+                    status="ACTIVE" />
+        """ % (locals()))
+
+        self.assertEqual(expected_image.toxml(), actual_image.toxml())
+
+    def test_get_image_v1_1_xml(self):
+        request = webob.Request.blank('/v1.1/images/123')
+        request.accept = "application/xml"
+        response = request.get_response(fakes.wsgi_app())
+
+        actual_image = minidom.parseString(response.body.replace("  ", ""))
+
+        expected_href = "http://localhost/v1.1/images/123"
+        expected_now = self.NOW_API_FORMAT
+        expected_image = minidom.parseString("""
+        <image id="123"
+                name="public image"
+                updated="%(expected_now)s"
+                created="%(expected_now)s"
+                status="ACTIVE">
+            <links>
+                <link href="%(expected_href)s" rel="self"/>
+                <link href="%(expected_href)s" rel="bookmark"
+                    type="application/json" />
+                <link href="%(expected_href)s" rel="bookmark"
+                    type="application/xml" />
+            </links>
+        </image>
+        """.replace("  ", "") % (locals()))
+
+        self.assertEqual(expected_image.toxml(), actual_image.toxml())
+
+    def test_get_image_404_json(self):
+        request = webob.Request.blank('/v1.0/images/NonExistantImage')
+        response = request.get_response(fakes.wsgi_app())
+        self.assertEqual(404, response.status_int)
+
+        expected = {
+            "itemNotFound": {
+                "message": "Image not found.",
+                "code": 404,
+            },
+        }
+
+        actual = json.loads(response.body)
+
+        self.assertEqual(expected, actual)
+
+    def test_get_image_404_xml(self):
+        request = webob.Request.blank('/v1.0/images/NonExistantImage')
+        request.accept = "application/xml"
+        response = request.get_response(fakes.wsgi_app())
+        self.assertEqual(404, response.status_int)
+
+        expected = minidom.parseString("""
+            <itemNotFound code="404">
+                <message>
+                    Image not found.
+                </message>
+            </itemNotFound>
+        """.replace("  ", ""))
+
+        actual = minidom.parseString(response.body.replace("  ", ""))
+
+        self.assertEqual(expected.toxml(), actual.toxml())
+
+    def test_get_image_404_v1_1_json(self):
+        request = webob.Request.blank('/v1.1/images/NonExistantImage')
+        response = request.get_response(fakes.wsgi_app())
+        self.assertEqual(404, response.status_int)
+
+        expected = {
+            "itemNotFound": {
+                "message": "Image not found.",
+                "code": 404,
+            },
+        }
+
+        actual = json.loads(response.body)
+
+        self.assertEqual(expected, actual)
+
+    def test_get_image_404_v1_1_xml(self):
+        request = webob.Request.blank('/v1.1/images/NonExistantImage')
+        request.accept = "application/xml"
+        response = request.get_response(fakes.wsgi_app())
+        self.assertEqual(404, response.status_int)
+
+        expected = minidom.parseString("""
+            <itemNotFound code="404">
+                <message>
+                    Image not found.
+                </message>
+            </itemNotFound>
+        """.replace("  ", ""))
+
+        actual = minidom.parseString(response.body.replace("  ", ""))
+
+        self.assertEqual(expected.toxml(), actual.toxml())
+
+    def test_get_image_index_v1_1(self):
+        request = webob.Request.blank('/v1.1/images')
+        response = request.get_response(fakes.wsgi_app())
+
+        response_dict = json.loads(response.body)
+        response_list = response_dict["images"]
+
+        fixtures = copy.copy(self.fixtures)
+
+        for image in fixtures:
+            if not self._applicable_fixture(image, 1):
+                fixtures.remove(image)
+                continue
+
+            href = "http://localhost/v1.1/images/%s" % image["id"]
+            test_image = {
+                "id": image["id"],
+                "name": image["name"],
+                "links": [{
+                    "rel": "self",
+                    "href": "http://localhost/v1.1/images/%s" % image["id"],
+                },
+                {
+                    "rel": "bookmark",
+                    "type": "application/json",
+                    "href": href,
+                },
+                {
+                    "rel": "bookmark",
+                    "type": "application/xml",
+                    "href": href,
+                }],
+            }
+            self.assertTrue(test_image in response_list)
+
+        self.assertEqual(len(response_list), len(fixtures))
 
     def test_get_image_details(self):
-        req = webob.Request.blank('/v1.0/images/detail')
-        res = req.get_response(fakes.wsgi_app())
-        image_metas = json.loads(res.body)['images']
-
-        now = self.NOW_API_FORMAT
-        expected = [
-            {'id': 123, 'name': 'public image', 'updated': now,
-             'created': now, 'status': 'ACTIVE'},
-            {'id': 124, 'name': 'queued backup', 'serverId': 42,
-             'updated': now, 'created': now,
-             'status': 'QUEUED'},
-            {'id': 125, 'name': 'saving backup', 'serverId': 42,
-             'updated': now, 'created': now,
-             'status': 'SAVING', 'progress': 0},
-            {'id': 126, 'name': 'active backup', 'serverId': 42,
-             'updated': now, 'created': now,
-             'status': 'ACTIVE'},
-            {'id': 127, 'name': 'killed backup', 'serverId': 42,
-             'updated': now, 'created': now,
-             'status': 'FAILED'}
-        ]
-
-        self.assertDictListMatch(image_metas, expected)
+        request = webob.Request.blank('/v1.0/images/detail')
+        response = request.get_response(fakes.wsgi_app())
+
+        response_dict = json.loads(response.body)
+        response_list = response_dict["images"]
+
+        expected = [{
+            'id': 123,
+            'name': 'public image',
+            'updated': self.NOW_API_FORMAT,
+            'created': self.NOW_API_FORMAT,
+            'status': 'ACTIVE',
+        },
+        {
+            'id': 124,
+            'name': 'queued backup',
+            'serverId': 42,
+            'updated': self.NOW_API_FORMAT,
+            'created': self.NOW_API_FORMAT,
+            'status': 'QUEUED',
+        },
+        {
+            'id': 125,
+            'name': 'saving backup',
+            'serverId': 42,
+            'updated': self.NOW_API_FORMAT,
+            'created': self.NOW_API_FORMAT,
+            'status': 'SAVING',
+            'progress': 0,
+        },
+        {
+            'id': 126,
+            'name': 'active backup',
+            'serverId': 42,
+            'updated': self.NOW_API_FORMAT,
+            'created': self.NOW_API_FORMAT,
+            'status': 'ACTIVE'
+        },
+        {
+            'id': 127,
+            'name': 'killed backup', 'serverId': 42,
+            'updated': self.NOW_API_FORMAT,
+            'created': self.NOW_API_FORMAT,
+            'status': 'FAILED',
+        }]
+
+        self.assertDictListMatch(expected, response_list)
+
+    def test_get_image_details_v1_1(self):
+        request = webob.Request.blank('/v1.1/images/detail')
+        response = request.get_response(fakes.wsgi_app())
+
+        response_dict = json.loads(response.body)
+        response_list = response_dict["images"]
+
+        expected = [{
+            'id': 123,
+            'name': 'public image',
+            'updated': self.NOW_API_FORMAT,
+            'created': self.NOW_API_FORMAT,
+            'status': 'ACTIVE',
+            "links": [{
+                "rel": "self",
+                "href": "http://localhost/v1.1/images/123",
+            },
+            {
+                "rel": "bookmark",
+                "type": "application/json",
+                "href": "http://localhost/v1.1/images/123",
+            },
+            {
+                "rel": "bookmark",
+                "type": "application/xml",
+                "href": "http://localhost/v1.1/images/123",
+            }],
+        },
+        {
+            'id': 124,
+            'name': 'queued backup',
+            'serverId': 42,
+            'updated': self.NOW_API_FORMAT,
+            'created': self.NOW_API_FORMAT,
+            'status': 'QUEUED',
+            "links": [{
+                "rel": "self",
+                "href": "http://localhost/v1.1/images/124",
+            },
+            {
+                "rel": "bookmark",
+                "type": "application/json",
+                "href": "http://localhost/v1.1/images/124",
+            },
+            {
+                "rel": "bookmark",
+                "type": "application/xml",
+                "href": "http://localhost/v1.1/images/124",
+            }],
+        },
+        {
+            'id': 125,
+            'name': 'saving backup',
+            'serverId': 42,
+            'updated': self.NOW_API_FORMAT,
+            'created': self.NOW_API_FORMAT,
+            'status': 'SAVING',
+            'progress': 0,
+            "links": [{
+                "rel": "self",
+                "href": "http://localhost/v1.1/images/125",
+            },
+            {
+                "rel": "bookmark",
+                "type": "application/json",
+                "href": "http://localhost/v1.1/images/125",
+            },
+            {
+                "rel": "bookmark",
+                "type": "application/xml",
+                "href": "http://localhost/v1.1/images/125",
+            }],
+        },
+        {
+            'id': 126,
+            'name': 'active backup',
+            'serverId': 42,
+            'updated': self.NOW_API_FORMAT,
+            'created': self.NOW_API_FORMAT,
+            'status': 'ACTIVE',
+            "links": [{
+                "rel": "self",
+                "href": "http://localhost/v1.1/images/126",
+            },
+            {
+                "rel": "bookmark",
+                "type": "application/json",
+                "href": "http://localhost/v1.1/images/126",
+            },
+            {
+                "rel": "bookmark",
+                "type": "application/xml",
+                "href": "http://localhost/v1.1/images/126",
+            }],
+        },
+        {
+            'id': 127,
+            'name': 'killed backup', 'serverId': 42,
+            'updated': self.NOW_API_FORMAT,
+            'created': self.NOW_API_FORMAT,
+            'status': 'FAILED',
+            "links": [{
+                "rel": "self",
+                "href": "http://localhost/v1.1/images/127",
+            },
+            {
+                "rel": "bookmark",
+                "type": "application/json",
+                "href": "http://localhost/v1.1/images/127",
+            },
+            {
+                "rel": "bookmark",
+                "type": "application/xml",
+                "href": "http://localhost/v1.1/images/127",
+            }],
+        }]
+
+        self.assertDictListMatch(expected, response_list)
 
     def test_get_image_found(self):
         req = webob.Request.blank('/v1.0/images/123')