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

Proposed by Brian Lamar
Status: Merged
Merged at revision: 907
Proposed branch: lp:~blamar/nova/openstack-api-1-1-images
Merge into: lp:~hudson-openstack/nova/trunk
Prerequisite: lp:~rackspace-titan/nova/openstack-api-versioned-controllers
Diff against target: 986 lines (+595/-254)
4 files modified
nova/api/openstack/__init__.py (+8/-3)
nova/api/openstack/images.py (+102/-209)
nova/api/openstack/views/images.py (+91/-11)
nova/tests/api/openstack/test_images.py (+394/-31)
To merge this branch: bzr merge lp:~blamar/nova/openstack-api-1-1-images
Reviewer Review Type Date Requested Status
Jay Pipes (community) Approve
Thierry Carrez (community) ffe Approve
justinsb (community) Approve
termie (community) Needs Fixing
Rick Harris (community) Approve
Brian Waldon (community) Approve
Review via email: mp+53942@code.launchpad.net

Description of the change

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

To post a comment you must log in.
Revision history for this message
Brian Waldon (bcwaldon) wrote :

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
Revision history for this message
Brian Lamar (blamar) wrote :

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.

Revision history for this message
Brian Lamar (blamar) wrote :

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

Revision history for this message
Brian Waldon (bcwaldon) wrote :

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)

Revision history for this message
Brian Waldon (bcwaldon) wrote :

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

Revision history for this message
Jay Pipes (jaypipes) wrote :

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
Revision history for this message
Brian Lamar (blamar) wrote :

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.

Revision history for this message
Brian Lamar (blamar) 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.

Revision history for this message
Brian Lamar (blamar) wrote :

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!

Revision history for this message
Rick Harris (rconradharris) wrote :

> 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?

Revision history for this message
Brian Waldon (bcwaldon) wrote :

Thanks for addressing my concerns

review: Approve
Revision history for this message
Jay Pipes (jaypipes) wrote :

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

Revision history for this message
Rick Harris (rconradharris) wrote :

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
Revision history for this message
Brian Lamar (blamar) wrote :

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!

Revision history for this message
Rick Harris (rconradharris) wrote :

> 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.

Revision history for this message
Brian Lamar (blamar) wrote :

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.

Revision history for this message
Thierry Carrez (ttx) wrote :

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

review: Approve
Revision history for this message
Rick Harris (rconradharris) wrote :

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
Revision history for this message
justinsb (justin-fathomdb) wrote :

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
Revision history for this message
Brian Lamar (blamar) wrote :

@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.

Revision history for this message
termie (termie) wrote :

- 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
Revision history for this message
justinsb (justin-fathomdb) wrote :

> 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
Revision history for this message
Brian Lamar (blamar) wrote :

@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!

Revision history for this message
Thierry Carrez (ttx) :
review: Approve (ffe)
Revision history for this message
Jay Pipes (jaypipes) :
review: Approve
Revision history for this message
Vish Ishaya (vishvananda) wrote :

'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.

"""

Revision history for this message
Brian Lamar (blamar) wrote :

@termie

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

Revision history for this message
Brian Lamar (blamar) wrote :

Merged trunk and fixed conflict.

Revision history for this message
Jay Pipes (jaypipes) wrote :

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
Revision history for this message
Brian Lamar (blamar) wrote :

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

Revision history for this message
Jay Pipes (jaypipes) wrote :

ok by me.

review: Approve
Revision history for this message
OpenStack Infra (hudson-openstack) wrote :
Revision history for this message
termie (termie) wrote :

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

Revision history for this message
Brian Lamar (blamar) wrote :

@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!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'nova/api/openstack/__init__.py'
2--- nova/api/openstack/__init__.py 2011-03-28 20:57:35 +0000
3+++ nova/api/openstack/__init__.py 2011-03-29 16:12:48 +0000
4@@ -111,9 +111,6 @@
5 parent_resource=dict(member_name='server',
6 collection_name='servers'))
7
8- mapper.resource("image", "images", controller=images.Controller(),
9- collection={'detail': 'GET'})
10-
11 _limits = limits.LimitsController()
12 mapper.resource("limit", "limits", controller=_limits)
13
14@@ -128,6 +125,10 @@
15 collection={'detail': 'GET'},
16 member=self.server_members)
17
18+ mapper.resource("image", "images",
19+ controller=images.ControllerV10(),
20+ collection={'detail': 'GET'})
21+
22 mapper.resource("flavor", "flavors",
23 controller=flavors.ControllerV10(),
24 collection={'detail': 'GET'})
25@@ -152,6 +153,10 @@
26 collection={'detail': 'GET'},
27 member=self.server_members)
28
29+ mapper.resource("image", "images",
30+ controller=images.ControllerV11(),
31+ collection={'detail': 'GET'})
32+
33 mapper.resource("image_meta", "meta",
34 controller=image_metadata.Controller(),
35 parent_resource=dict(member_name='image',
36
37=== modified file 'nova/api/openstack/images.py'
38--- nova/api/openstack/images.py 2011-03-24 21:13:55 +0000
39+++ nova/api/openstack/images.py 2011-03-29 16:12:48 +0000
40@@ -1,6 +1,4 @@
41-# vim: tabstop=4 shiftwidth=4 softtabstop=4
42-
43-# Copyright 2010 OpenStack LLC.
44+# Copyright 2011 OpenStack LLC.
45 # All Rights Reserved.
46 #
47 # Licensed under the Apache License, Version 2.0 (the "License"); you may
48@@ -17,7 +15,7 @@
49
50 import datetime
51
52-from webob import exc
53+import webob.exc
54
55 from nova import compute
56 from nova import exception
57@@ -25,238 +23,133 @@
58 from nova import log
59 from nova import utils
60 from nova import wsgi
61-import nova.api.openstack
62 from nova.api.openstack import common
63 from nova.api.openstack import faults
64-import nova.image.service
65+from nova.api.openstack.views import images as images_view
66
67
68 LOG = log.getLogger('nova.api.openstack.images')
69-
70 FLAGS = flags.FLAGS
71
72
73-def _translate_keys(item):
74- """
75- Maps key names to Rackspace-like attributes for return
76- also pares down attributes to those we want
77- item is a dict
78-
79- Note: should be removed when the set of keys expected by the api
80- and the set of keys returned by the image service are equivalent
81-
82- """
83- # TODO(tr3buchet): this map is specific to s3 object store,
84- # replace with a list of keys for _filter_keys later
85- mapped_keys = {'status': 'imageState',
86- 'id': 'imageId',
87- 'name': 'imageLocation'}
88-
89- mapped_item = {}
90- # TODO(tr3buchet):
91- # this chunk of code works with s3 and the local image service/glance
92- # when we switch to glance/local image service it can be replaced with
93- # a call to _filter_keys, and mapped_keys can be changed to a list
94- try:
95- for k, v in mapped_keys.iteritems():
96- # map s3 fields
97- mapped_item[k] = item[v]
98- except KeyError:
99- # return only the fields api expects
100- mapped_item = _filter_keys(item, mapped_keys.keys())
101-
102- return mapped_item
103-
104-
105-def _translate_status(item):
106- """
107- Translates status of image to match current Rackspace api bindings
108- item is a dict
109-
110- Note: should be removed when the set of statuses expected by the api
111- and the set of statuses returned by the image service are equivalent
112-
113- """
114- status_mapping = {
115- 'pending': 'queued',
116- 'decrypting': 'preparing',
117- 'untarring': 'saving',
118- 'available': 'active'}
119- try:
120- item['status'] = status_mapping[item['status']]
121- except KeyError:
122- # TODO(sirp): Performing translation of status (if necessary) here for
123- # now. Perhaps this should really be done in EC2 API and
124- # S3ImageService
125- pass
126-
127-
128-def _filter_keys(item, keys):
129- """
130- Filters all model attributes except for keys
131- item is a dict
132-
133- """
134- return dict((k, v) for k, v in item.iteritems() if k in keys)
135-
136-
137-def _convert_image_id_to_hash(image):
138- if 'imageId' in image:
139- # Convert EC2-style ID (i-blah) to Rackspace-style (int)
140- image_id = abs(hash(image['imageId']))
141- image['imageId'] = image_id
142- image['id'] = image_id
143-
144-
145-def _translate_s3_like_images(image_metadata):
146- """Work-around for leaky S3ImageService abstraction"""
147- api_metadata = image_metadata.copy()
148- _convert_image_id_to_hash(api_metadata)
149- api_metadata = _translate_keys(api_metadata)
150- _translate_status(api_metadata)
151- return api_metadata
152-
153-
154-def _translate_from_image_service_to_api(image_metadata):
155- """Translate from ImageService to OpenStack API style attribute names
156-
157- This involves 4 steps:
158-
159- 1. Filter out attributes that the OpenStack API doesn't need
160-
161- 2. Translate from base image attributes from names used by
162- BaseImageService to names used by OpenStack API
163-
164- 3. Add in any image properties
165-
166- 4. Format values according to API spec (for example dates must
167- look like "2010-08-10T12:00:00Z")
168- """
169- service_metadata = image_metadata.copy()
170- properties = service_metadata.pop('properties', {})
171-
172- # 1. Filter out unecessary attributes
173- api_keys = ['id', 'name', 'updated_at', 'created_at', 'status']
174- api_metadata = utils.subset_dict(service_metadata, api_keys)
175-
176- # 2. Translate base image attributes
177- api_map = {'updated_at': 'updated', 'created_at': 'created'}
178- api_metadata = utils.map_dict_keys(api_metadata, api_map)
179-
180- # 3. Add in any image properties
181- # 3a. serverId is used for backups and snapshots
182- try:
183- api_metadata['serverId'] = int(properties['instance_id'])
184- except KeyError:
185- pass # skip if it's not present
186- except ValueError:
187- pass # skip if it's not an integer
188-
189- # 3b. Progress special case
190- # TODO(sirp): ImageService doesn't have a notion of progress yet, so for
191- # now just fake it
192- if service_metadata['status'] == 'saving':
193- api_metadata['progress'] = 0
194-
195- # 4. Format values
196- # 4a. Format Image Status (API requires uppercase)
197- api_metadata['status'] = _format_status_for_api(api_metadata['status'])
198-
199- # 4b. Format timestamps
200- for attr in ('created', 'updated'):
201- if attr in api_metadata:
202- api_metadata[attr] = _format_datetime_for_api(
203- api_metadata[attr])
204-
205- return api_metadata
206-
207-
208-def _format_status_for_api(status):
209- """Return status in a format compliant with OpenStack API"""
210- mapping = {'queued': 'QUEUED',
211- 'preparing': 'PREPARING',
212- 'saving': 'SAVING',
213- 'active': 'ACTIVE',
214- 'killed': 'FAILED'}
215- return mapping[status]
216-
217-
218-def _format_datetime_for_api(datetime_):
219- """Stringify datetime objects in a format compliant with OpenStack API"""
220- API_DATETIME_FMT = '%Y-%m-%dT%H:%M:%SZ'
221- return datetime_.strftime(API_DATETIME_FMT)
222-
223-
224-def _safe_translate(image_metadata):
225- """Translate attributes for OpenStack API, temporary workaround for
226- S3ImageService attribute leakage.
227- """
228- # FIXME(sirp): The S3ImageService appears to be leaking implementation
229- # details, including its internal attribute names, and internal
230- # `status` values. Working around it for now.
231- s3_like_image = ('imageId' in image_metadata)
232- if s3_like_image:
233- translate = _translate_s3_like_images
234- else:
235- translate = _translate_from_image_service_to_api
236- return translate(image_metadata)
237-
238-
239 class Controller(wsgi.Controller):
240+ """Base `wsgi.Controller` for retrieving/displaying images."""
241
242 _serialization_metadata = {
243 'application/xml': {
244 "attributes": {
245 "image": ["id", "name", "updated", "created", "status",
246- "serverId", "progress"]}}}
247-
248- def __init__(self):
249- self._service = utils.import_object(FLAGS.image_service)
250+ "serverId", "progress"],
251+ "link": ["rel", "type", "href"],
252+ },
253+ },
254+ }
255+
256+ def __init__(self, image_service=None, compute_service=None):
257+ """Initialize new `ImageController`.
258+
259+ :param compute_service: `nova.compute.api:API`
260+ :param image_service: `nova.image.service:BaseImageService`
261+ """
262+ _default_service = utils.import_object(flags.FLAGS.image_service)
263+
264+ self._compute_service = compute_service or compute.API()
265+ self._image_service = image_service or _default_service
266
267 def index(self, req):
268- """Return all public images in brief"""
269+ """Return an index listing of images available to the request.
270+
271+ :param req: `wsgi.Request` object
272+ """
273 context = req.environ['nova.context']
274- image_metas = self._service.index(context)
275- image_metas = common.limited(image_metas, req)
276- return dict(images=image_metas)
277+ images = self._image_service.index(context)
278+ images = common.limited(images, req)
279+ builder = self.get_builder(req).build
280+ return dict(images=[builder(image, detail=False) for image in images])
281
282 def detail(self, req):
283- """Return all public images in detail"""
284+ """Return a detailed index listing of images available to the request.
285+
286+ :param req: `wsgi.Request` object.
287+ """
288 context = req.environ['nova.context']
289- image_metas = self._service.detail(context)
290- image_metas = common.limited(image_metas, req)
291- api_image_metas = [_safe_translate(image_meta)
292- for image_meta in image_metas]
293- return dict(images=api_image_metas)
294+ images = self._image_service.detail(context)
295+ images = common.limited(images, req)
296+ builder = self.get_builder(req).build
297+ return dict(images=[builder(image, detail=True) for image in images])
298
299 def show(self, req, id):
300- """Return data about the given image id"""
301+ """Return detailed information about a specific image.
302+
303+ :param req: `wsgi.Request` object
304+ :param id: Image identifier (integer)
305+ """
306 context = req.environ['nova.context']
307- try:
308- image_id = common.get_image_id_from_image_hash(
309- self._service, context, id)
310+
311+ try:
312+ image_id = int(id)
313+ except ValueError:
314+ explanation = _("Image not found.")
315+ raise faults.Fault(webob.exc.HTTPNotFound(explanation=explanation))
316+
317+ try:
318+ image = self._image_service.show(context, image_id)
319 except exception.NotFound:
320- raise faults.Fault(exc.HTTPNotFound())
321+ explanation = _("Image '%d' not found.") % (image_id)
322+ raise faults.Fault(webob.exc.HTTPNotFound(explanation=explanation))
323
324- image_meta = self._service.show(context, image_id)
325- api_image_meta = _safe_translate(image_meta)
326- return dict(image=api_image_meta)
327+ return dict(image=self.get_builder(req).build(image, detail=True))
328
329 def delete(self, req, id):
330- # Only public images are supported for now.
331- raise faults.Fault(exc.HTTPNotFound())
332+ """Delete an image, if allowed.
333+
334+ :param req: `wsgi.Request` object
335+ :param id: Image identifier (integer)
336+ """
337+ image_id = id
338+ context = req.environ['nova.context']
339+ self._image_service.delete(context, image_id)
340+ return webob.exc.HTTPNoContent()
341
342 def create(self, req):
343+ """Snapshot a server instance and save the image.
344+
345+ :param req: `wsgi.Request` object
346+ """
347 context = req.environ['nova.context']
348- env = self._deserialize(req.body, req.get_content_type())
349- instance_id = env["image"]["serverId"]
350- name = env["image"]["name"]
351- image_meta = compute.API().snapshot(
352- context, instance_id, name)
353- api_image_meta = _safe_translate(image_meta)
354- return dict(image=api_image_meta)
355-
356- def update(self, req, id):
357- # Users may not modify public images, and that's all that
358- # we support for now.
359- raise faults.Fault(exc.HTTPNotFound())
360+ content_type = req.get_content_type()
361+ image = self._deserialize(req.body, content_type)
362+
363+ if not image:
364+ raise webob.exc.HTTPBadRequest()
365+
366+ try:
367+ server_id = image["image"]["serverId"]
368+ image_name = image["image"]["name"]
369+ except KeyError:
370+ raise webob.exc.HTTPBadRequest()
371+
372+ image = self._compute_service.snapshot(context, server_id, image_name)
373+ return self.get_builder(req).build(image, detail=True)
374+
375+ def get_builder(self, request):
376+ """Indicates that you must use a Controller subclass."""
377+ raise NotImplementedError
378+
379+
380+class ControllerV10(Controller):
381+ """Version 1.0 specific controller logic."""
382+
383+ def get_builder(self, request):
384+ """Property to get the ViewBuilder class we need to use."""
385+ base_url = request.application_url
386+ return images_view.ViewBuilderV10(base_url)
387+
388+
389+class ControllerV11(Controller):
390+ """Version 1.1 specific controller logic."""
391+
392+ def get_builder(self, request):
393+ """Property to get the ViewBuilder class we need to use."""
394+ base_url = request.application_url
395+ return images_view.ViewBuilderV11(base_url)
396
397=== modified file 'nova/api/openstack/views/images.py'
398--- nova/api/openstack/views/images.py 2011-03-17 07:41:01 +0000
399+++ nova/api/openstack/views/images.py 2011-03-29 16:12:48 +0000
400@@ -15,20 +15,100 @@
401 # License for the specific language governing permissions and limitations
402 # under the License.
403
404-from nova.api.openstack import common
405+import os.path
406
407
408 class ViewBuilder(object):
409- def __init__(self):
410- pass
411-
412- def build(self, image_obj):
413- raise NotImplementedError()
414-
415-
416-class ViewBuilderV11(ViewBuilder):
417+ """Base class for generating responses to OpenStack API image requests."""
418+
419 def __init__(self, base_url):
420- self.base_url = base_url
421+ """Initialize new `ViewBuilder`."""
422+ self._url = base_url
423+
424+ def _format_dates(self, image):
425+ """Update all date fields to ensure standardized formatting."""
426+ for attr in ['created_at', 'updated_at', 'deleted_at']:
427+ if image.get(attr) is not None:
428+ image[attr] = image[attr].strftime('%Y-%m-%dT%H:%M:%SZ')
429+
430+ def _format_status(self, image):
431+ """Update the status field to standardize format."""
432+ status_mapping = {
433+ 'pending': 'queued',
434+ 'decrypting': 'preparing',
435+ 'untarring': 'saving',
436+ 'available': 'active',
437+ 'killed': 'failed',
438+ }
439+
440+ try:
441+ image['status'] = status_mapping[image['status']].upper()
442+ except KeyError:
443+ image['status'] = image['status'].upper()
444
445 def generate_href(self, image_id):
446- return "%s/images/%s" % (self.base_url, image_id)
447+ """Return an href string pointing to this object."""
448+ return os.path.join(self._url, "images", str(image_id))
449+
450+ def build(self, image_obj, detail=False):
451+ """Return a standardized image structure for display by the API."""
452+ properties = image_obj.get("properties", {})
453+
454+ self._format_dates(image_obj)
455+
456+ if "status" in image_obj:
457+ self._format_status(image_obj)
458+
459+ image = {
460+ "id": image_obj["id"],
461+ "name": image_obj["name"],
462+ }
463+
464+ if "instance_id" in properties:
465+ try:
466+ image["serverId"] = int(properties["instance_id"])
467+ except ValueError:
468+ pass
469+
470+ if detail:
471+ image.update({
472+ "created": image_obj["created_at"],
473+ "updated": image_obj["updated_at"],
474+ "status": image_obj["status"],
475+ })
476+
477+ if image["status"] == "SAVING":
478+ image["progress"] = 0
479+
480+ return image
481+
482+
483+class ViewBuilderV10(ViewBuilder):
484+ """OpenStack API v1.0 Image Builder"""
485+ pass
486+
487+
488+class ViewBuilderV11(ViewBuilder):
489+ """OpenStack API v1.1 Image Builder"""
490+
491+ def build(self, image_obj, detail=False):
492+ """Return a standardized image structure for display by the API."""
493+ image = ViewBuilder.build(self, image_obj, detail)
494+ href = self.generate_href(image_obj["id"])
495+
496+ image["links"] = [{
497+ "rel": "self",
498+ "href": href,
499+ },
500+ {
501+ "rel": "bookmark",
502+ "type": "application/json",
503+ "href": href,
504+ },
505+ {
506+ "rel": "bookmark",
507+ "type": "application/xml",
508+ "href": href,
509+ }]
510+
511+ return image
512
513=== modified file 'nova/tests/api/openstack/test_images.py'
514--- nova/tests/api/openstack/test_images.py 2011-03-28 15:05:28 +0000
515+++ nova/tests/api/openstack/test_images.py 2011-03-29 16:12:48 +0000
516@@ -20,11 +20,13 @@
517 and as a WSGI layer
518 """
519
520+import copy
521 import json
522 import datetime
523 import os
524 import shutil
525 import tempfile
526+import xml.dom.minidom as minidom
527
528 import stubout
529 import webob
530@@ -214,12 +216,14 @@
531
532
533 class ImageControllerWithGlanceServiceTest(test.TestCase):
534- """Test of the OpenStack API /images application controller"""
535-
536+ """
537+ Test of the OpenStack API /images application controller w/Glance.
538+ """
539 NOW_GLANCE_FORMAT = "2010-10-11T10:30:22"
540 NOW_API_FORMAT = "2010-10-11T10:30:22Z"
541
542 def setUp(self):
543+ """Run before each test."""
544 super(ImageControllerWithGlanceServiceTest, self).setUp()
545 self.orig_image_service = FLAGS.image_service
546 FLAGS.image_service = 'nova.image.glance.GlanceImageService'
547@@ -230,18 +234,30 @@
548 fakes.stub_out_rate_limiting(self.stubs)
549 fakes.stub_out_auth(self.stubs)
550 fakes.stub_out_key_pair_funcs(self.stubs)
551- fixtures = self._make_image_fixtures()
552- fakes.stub_out_glance(self.stubs, initial_fixtures=fixtures)
553+ self.fixtures = self._make_image_fixtures()
554+ fakes.stub_out_glance(self.stubs, initial_fixtures=self.fixtures)
555
556 def tearDown(self):
557+ """Run after each test."""
558 self.stubs.UnsetAll()
559 FLAGS.image_service = self.orig_image_service
560 super(ImageControllerWithGlanceServiceTest, self).tearDown()
561
562+ def _applicable_fixture(self, fixture, user_id):
563+ """Determine if this fixture is applicable for given user id."""
564+ is_public = fixture["is_public"]
565+ try:
566+ uid = int(fixture["properties"]["user_id"])
567+ except KeyError:
568+ uid = None
569+ return uid == user_id or is_public
570+
571 def test_get_image_index(self):
572- req = webob.Request.blank('/v1.0/images')
573- res = req.get_response(fakes.wsgi_app())
574- image_metas = json.loads(res.body)['images']
575+ request = webob.Request.blank('/v1.0/images')
576+ response = request.get_response(fakes.wsgi_app())
577+
578+ response_dict = json.loads(response.body)
579+ response_list = response_dict["images"]
580
581 expected = [{'id': 123, 'name': 'public image'},
582 {'id': 124, 'name': 'queued backup'},
583@@ -249,32 +265,379 @@
584 {'id': 126, 'name': 'active backup'},
585 {'id': 127, 'name': 'killed backup'}]
586
587- self.assertDictListMatch(image_metas, expected)
588+ self.assertDictListMatch(response_list, expected)
589+
590+ def test_get_image(self):
591+ request = webob.Request.blank('/v1.0/images/123')
592+ response = request.get_response(fakes.wsgi_app())
593+
594+ self.assertEqual(200, response.status_int)
595+
596+ actual_image = json.loads(response.body)
597+
598+ expected_image = {
599+ "image": {
600+ "id": 123,
601+ "name": "public image",
602+ "updated": self.NOW_API_FORMAT,
603+ "created": self.NOW_API_FORMAT,
604+ "status": "ACTIVE",
605+ },
606+ }
607+
608+ self.assertEqual(expected_image, actual_image)
609+
610+ def test_get_image_v1_1(self):
611+ request = webob.Request.blank('/v1.1/images/123')
612+ response = request.get_response(fakes.wsgi_app())
613+
614+ actual_image = json.loads(response.body)
615+
616+ href = "http://localhost/v1.1/images/123"
617+
618+ expected_image = {
619+ "image": {
620+ "id": 123,
621+ "name": "public image",
622+ "updated": self.NOW_API_FORMAT,
623+ "created": self.NOW_API_FORMAT,
624+ "status": "ACTIVE",
625+ "links": [{
626+ "rel": "self",
627+ "href": href,
628+ },
629+ {
630+ "rel": "bookmark",
631+ "type": "application/json",
632+ "href": href,
633+ },
634+ {
635+ "rel": "bookmark",
636+ "type": "application/xml",
637+ "href": href,
638+ }],
639+ },
640+ }
641+
642+ self.assertEqual(expected_image, actual_image)
643+
644+ def test_get_image_xml(self):
645+ request = webob.Request.blank('/v1.0/images/123')
646+ request.accept = "application/xml"
647+ response = request.get_response(fakes.wsgi_app())
648+
649+ actual_image = minidom.parseString(response.body.replace(" ", ""))
650+
651+ expected_now = self.NOW_API_FORMAT
652+ expected_image = minidom.parseString("""
653+ <image id="123"
654+ name="public image"
655+ updated="%(expected_now)s"
656+ created="%(expected_now)s"
657+ status="ACTIVE" />
658+ """ % (locals()))
659+
660+ self.assertEqual(expected_image.toxml(), actual_image.toxml())
661+
662+ def test_get_image_v1_1_xml(self):
663+ request = webob.Request.blank('/v1.1/images/123')
664+ request.accept = "application/xml"
665+ response = request.get_response(fakes.wsgi_app())
666+
667+ actual_image = minidom.parseString(response.body.replace(" ", ""))
668+
669+ expected_href = "http://localhost/v1.1/images/123"
670+ expected_now = self.NOW_API_FORMAT
671+ expected_image = minidom.parseString("""
672+ <image id="123"
673+ name="public image"
674+ updated="%(expected_now)s"
675+ created="%(expected_now)s"
676+ status="ACTIVE">
677+ <links>
678+ <link href="%(expected_href)s" rel="self"/>
679+ <link href="%(expected_href)s" rel="bookmark"
680+ type="application/json" />
681+ <link href="%(expected_href)s" rel="bookmark"
682+ type="application/xml" />
683+ </links>
684+ </image>
685+ """.replace(" ", "") % (locals()))
686+
687+ self.assertEqual(expected_image.toxml(), actual_image.toxml())
688+
689+ def test_get_image_404_json(self):
690+ request = webob.Request.blank('/v1.0/images/NonExistantImage')
691+ response = request.get_response(fakes.wsgi_app())
692+ self.assertEqual(404, response.status_int)
693+
694+ expected = {
695+ "itemNotFound": {
696+ "message": "Image not found.",
697+ "code": 404,
698+ },
699+ }
700+
701+ actual = json.loads(response.body)
702+
703+ self.assertEqual(expected, actual)
704+
705+ def test_get_image_404_xml(self):
706+ request = webob.Request.blank('/v1.0/images/NonExistantImage')
707+ request.accept = "application/xml"
708+ response = request.get_response(fakes.wsgi_app())
709+ self.assertEqual(404, response.status_int)
710+
711+ expected = minidom.parseString("""
712+ <itemNotFound code="404">
713+ <message>
714+ Image not found.
715+ </message>
716+ </itemNotFound>
717+ """.replace(" ", ""))
718+
719+ actual = minidom.parseString(response.body.replace(" ", ""))
720+
721+ self.assertEqual(expected.toxml(), actual.toxml())
722+
723+ def test_get_image_404_v1_1_json(self):
724+ request = webob.Request.blank('/v1.1/images/NonExistantImage')
725+ response = request.get_response(fakes.wsgi_app())
726+ self.assertEqual(404, response.status_int)
727+
728+ expected = {
729+ "itemNotFound": {
730+ "message": "Image not found.",
731+ "code": 404,
732+ },
733+ }
734+
735+ actual = json.loads(response.body)
736+
737+ self.assertEqual(expected, actual)
738+
739+ def test_get_image_404_v1_1_xml(self):
740+ request = webob.Request.blank('/v1.1/images/NonExistantImage')
741+ request.accept = "application/xml"
742+ response = request.get_response(fakes.wsgi_app())
743+ self.assertEqual(404, response.status_int)
744+
745+ expected = minidom.parseString("""
746+ <itemNotFound code="404">
747+ <message>
748+ Image not found.
749+ </message>
750+ </itemNotFound>
751+ """.replace(" ", ""))
752+
753+ actual = minidom.parseString(response.body.replace(" ", ""))
754+
755+ self.assertEqual(expected.toxml(), actual.toxml())
756+
757+ def test_get_image_index_v1_1(self):
758+ request = webob.Request.blank('/v1.1/images')
759+ response = request.get_response(fakes.wsgi_app())
760+
761+ response_dict = json.loads(response.body)
762+ response_list = response_dict["images"]
763+
764+ fixtures = copy.copy(self.fixtures)
765+
766+ for image in fixtures:
767+ if not self._applicable_fixture(image, 1):
768+ fixtures.remove(image)
769+ continue
770+
771+ href = "http://localhost/v1.1/images/%s" % image["id"]
772+ test_image = {
773+ "id": image["id"],
774+ "name": image["name"],
775+ "links": [{
776+ "rel": "self",
777+ "href": "http://localhost/v1.1/images/%s" % image["id"],
778+ },
779+ {
780+ "rel": "bookmark",
781+ "type": "application/json",
782+ "href": href,
783+ },
784+ {
785+ "rel": "bookmark",
786+ "type": "application/xml",
787+ "href": href,
788+ }],
789+ }
790+ self.assertTrue(test_image in response_list)
791+
792+ self.assertEqual(len(response_list), len(fixtures))
793
794 def test_get_image_details(self):
795- req = webob.Request.blank('/v1.0/images/detail')
796- res = req.get_response(fakes.wsgi_app())
797- image_metas = json.loads(res.body)['images']
798-
799- now = self.NOW_API_FORMAT
800- expected = [
801- {'id': 123, 'name': 'public image', 'updated': now,
802- 'created': now, 'status': 'ACTIVE'},
803- {'id': 124, 'name': 'queued backup', 'serverId': 42,
804- 'updated': now, 'created': now,
805- 'status': 'QUEUED'},
806- {'id': 125, 'name': 'saving backup', 'serverId': 42,
807- 'updated': now, 'created': now,
808- 'status': 'SAVING', 'progress': 0},
809- {'id': 126, 'name': 'active backup', 'serverId': 42,
810- 'updated': now, 'created': now,
811- 'status': 'ACTIVE'},
812- {'id': 127, 'name': 'killed backup', 'serverId': 42,
813- 'updated': now, 'created': now,
814- 'status': 'FAILED'}
815- ]
816-
817- self.assertDictListMatch(image_metas, expected)
818+ request = webob.Request.blank('/v1.0/images/detail')
819+ response = request.get_response(fakes.wsgi_app())
820+
821+ response_dict = json.loads(response.body)
822+ response_list = response_dict["images"]
823+
824+ expected = [{
825+ 'id': 123,
826+ 'name': 'public image',
827+ 'updated': self.NOW_API_FORMAT,
828+ 'created': self.NOW_API_FORMAT,
829+ 'status': 'ACTIVE',
830+ },
831+ {
832+ 'id': 124,
833+ 'name': 'queued backup',
834+ 'serverId': 42,
835+ 'updated': self.NOW_API_FORMAT,
836+ 'created': self.NOW_API_FORMAT,
837+ 'status': 'QUEUED',
838+ },
839+ {
840+ 'id': 125,
841+ 'name': 'saving backup',
842+ 'serverId': 42,
843+ 'updated': self.NOW_API_FORMAT,
844+ 'created': self.NOW_API_FORMAT,
845+ 'status': 'SAVING',
846+ 'progress': 0,
847+ },
848+ {
849+ 'id': 126,
850+ 'name': 'active backup',
851+ 'serverId': 42,
852+ 'updated': self.NOW_API_FORMAT,
853+ 'created': self.NOW_API_FORMAT,
854+ 'status': 'ACTIVE'
855+ },
856+ {
857+ 'id': 127,
858+ 'name': 'killed backup', 'serverId': 42,
859+ 'updated': self.NOW_API_FORMAT,
860+ 'created': self.NOW_API_FORMAT,
861+ 'status': 'FAILED',
862+ }]
863+
864+ self.assertDictListMatch(expected, response_list)
865+
866+ def test_get_image_details_v1_1(self):
867+ request = webob.Request.blank('/v1.1/images/detail')
868+ response = request.get_response(fakes.wsgi_app())
869+
870+ response_dict = json.loads(response.body)
871+ response_list = response_dict["images"]
872+
873+ expected = [{
874+ 'id': 123,
875+ 'name': 'public image',
876+ 'updated': self.NOW_API_FORMAT,
877+ 'created': self.NOW_API_FORMAT,
878+ 'status': 'ACTIVE',
879+ "links": [{
880+ "rel": "self",
881+ "href": "http://localhost/v1.1/images/123",
882+ },
883+ {
884+ "rel": "bookmark",
885+ "type": "application/json",
886+ "href": "http://localhost/v1.1/images/123",
887+ },
888+ {
889+ "rel": "bookmark",
890+ "type": "application/xml",
891+ "href": "http://localhost/v1.1/images/123",
892+ }],
893+ },
894+ {
895+ 'id': 124,
896+ 'name': 'queued backup',
897+ 'serverId': 42,
898+ 'updated': self.NOW_API_FORMAT,
899+ 'created': self.NOW_API_FORMAT,
900+ 'status': 'QUEUED',
901+ "links": [{
902+ "rel": "self",
903+ "href": "http://localhost/v1.1/images/124",
904+ },
905+ {
906+ "rel": "bookmark",
907+ "type": "application/json",
908+ "href": "http://localhost/v1.1/images/124",
909+ },
910+ {
911+ "rel": "bookmark",
912+ "type": "application/xml",
913+ "href": "http://localhost/v1.1/images/124",
914+ }],
915+ },
916+ {
917+ 'id': 125,
918+ 'name': 'saving backup',
919+ 'serverId': 42,
920+ 'updated': self.NOW_API_FORMAT,
921+ 'created': self.NOW_API_FORMAT,
922+ 'status': 'SAVING',
923+ 'progress': 0,
924+ "links": [{
925+ "rel": "self",
926+ "href": "http://localhost/v1.1/images/125",
927+ },
928+ {
929+ "rel": "bookmark",
930+ "type": "application/json",
931+ "href": "http://localhost/v1.1/images/125",
932+ },
933+ {
934+ "rel": "bookmark",
935+ "type": "application/xml",
936+ "href": "http://localhost/v1.1/images/125",
937+ }],
938+ },
939+ {
940+ 'id': 126,
941+ 'name': 'active backup',
942+ 'serverId': 42,
943+ 'updated': self.NOW_API_FORMAT,
944+ 'created': self.NOW_API_FORMAT,
945+ 'status': 'ACTIVE',
946+ "links": [{
947+ "rel": "self",
948+ "href": "http://localhost/v1.1/images/126",
949+ },
950+ {
951+ "rel": "bookmark",
952+ "type": "application/json",
953+ "href": "http://localhost/v1.1/images/126",
954+ },
955+ {
956+ "rel": "bookmark",
957+ "type": "application/xml",
958+ "href": "http://localhost/v1.1/images/126",
959+ }],
960+ },
961+ {
962+ 'id': 127,
963+ 'name': 'killed backup', 'serverId': 42,
964+ 'updated': self.NOW_API_FORMAT,
965+ 'created': self.NOW_API_FORMAT,
966+ 'status': 'FAILED',
967+ "links": [{
968+ "rel": "self",
969+ "href": "http://localhost/v1.1/images/127",
970+ },
971+ {
972+ "rel": "bookmark",
973+ "type": "application/json",
974+ "href": "http://localhost/v1.1/images/127",
975+ },
976+ {
977+ "rel": "bookmark",
978+ "type": "application/xml",
979+ "href": "http://localhost/v1.1/images/127",
980+ }],
981+ }]
982+
983+ self.assertDictListMatch(expected, response_list)
984
985 def test_get_image_found(self):
986 req = webob.Request.blank('/v1.0/images/123')