Merge lp:~jaypipes/glance/parallax-register-image into lp:~hudson-openstack/glance/trunk

[Jay Pipes]

Implements Parallax API call to register a new image

* Adds unit test for Parallax API controller
* Adds stubouts for glance.parallax.db.sqlalchemy.api calls
  regarding images
* Adds unittest for bad status on image creation
* Adds --logging-clear-handlers arg to nosetests in run_tests.sh to
  prevent extra output in running tests when tests complete successfully

[Michael Gundlach (gundlach)]

Why does ImageController.__init__ exist?

57: should be
  if 'status' not in image_data:
or possibly 56/57/58 could be replaced with
  image_data.setdefault('status', 'available')

101: i assume you meant to remove support for GET /images/detail.

Otherwise, lgtm.

review: Approve

[Jay Pipes (jaypipes)]

> Why does ImageController.__init__ exist?

lol. :) Leftover from when I had some debugging statements in there. ::blush::

> 57: should be
> if 'status' not in image_data:
> or possibly 56/57/58 could be replaced with
> image_data.setdefault('status', 'available')

Ah, nice. Thanks.

> 101: i assume you meant to remove support for GET /images/detail.

Yes, that does not comply with the Atom publishing protocol. No need for it AFAICT.

> Otherwise, lgtm.

Thanks! I'll push those little fixes later today :)

[Michael Gundlach (gundlach)]

On Tue, Oct 12, 2010 at 4:36 PM, Jay Pipes <email address hidden> wrote:

> > 101: i assume you meant to remove support for GET /images/detail.
>
> Yes, that does not comply with the Atom publishing protocol. No need for
> it AFAICT.
>

We need it in order to comply as much as possible with the current Rackspace
API. GET /images should just return id and name, while /images/detail
returns all other available data. (same for GET /images/4 and GET
/images/4/detail .)

I think our goal was to conform to the RS API, not to conform to the Atom
protocol. Then again, we're planning to iterate on the API shortly after
Austin, so I'm not positive what this buys us.

cc'ing a couple of people from whom I've heard the "conform to the RS API"
message, to get their second opinions.

Michael

[Rick Clark (dendrobates)]

On 10/13/2010 11:00 AM, Michael Gundlach wrote:
> I think our goal was to conform to the RS API, not to conform to the Atom
> protocol. Then again, we're planning to iterate on the API shortly after
> Austin, so I'm not positive what this buys us.

We need to conform as much as possible to the RS API until they release
1.1 and hand off API development to us. At that point we can
diverge/fix in future versions.

I am not commenting on the technical merits of this discussion though.

> cc'ing a couple of people from whom I've heard the "conform to the RS API"
> message, to get their second opinions.
>
> Michael
>

[Rick Harris (rconradharris)]

++ on gundlach's cleanup suggestions, otherwise, lgtm.

review: Approve

[Jay Pipes (jaypipes)]

cool, understood as far as RS API. fix coming shortly.

[Jay Pipes (jaypipes)]

Alrighty, added the /images/detail call, a unit test for it, and gundlach's cleanups. Please mark the merge prop Approved if all meets expectations. :)

[lp:~jaypipes/glance/parallax-register-image]

15. By Jay Pipes on 2010-10-13

Adds a /images/detail route to the Parallax controller, adds a unit test for it, and cleans up Michael's suggestions.

[Michael Gundlach (gundlach)]

yeehaw

review: Approve

=== modified file 'glance/common/wsgi.py'
--- glance/common/wsgi.py	2010-10-01 23:07:46 +0000
+++ glance/common/wsgi.py	2010-10-13 18:50:33 +0000
@@ -224,7 +224,6 @@
         Uses self._serialization_metadata if it exists, which is a dict mapping
         MIME types to information needed to serialize to that type.
         """
-        # FIXME(sirp): type(self) should just be `self`
         _metadata = getattr(type(self), "_serialization_metadata", {})
         serializer = Serializer(request.environ, _metadata)
         return serializer.to_content_type(data)

=== modified file 'glance/parallax/controllers.py'
--- glance/parallax/controllers.py	2010-10-11 18:28:35 +0000
+++ glance/parallax/controllers.py	2010-10-13 18:50:33 +0000
@@ -18,26 +18,52 @@
 Parllax Image controller
 """
 
+import json
 import routes
+from webob import exc
+
 from glance.common import wsgi
 from glance.common import exception
 from glance.parallax import db
-from webob import exc
 
 
 class ImageController(wsgi.Controller):
+
     """Image Controller """
     
     def index(self, req):
-        """Return data for all public, non-deleted images """
+        """Return basic information for all public, non-deleted images
+        
+        :param req: the Request object coming from the wsgi layer
+        :retval a mapping of the following form::
+
+            dict(images=[image_list])
+
+        Where image_list is a sequence of mappings::
+
+            {'id': image_id, 'name': image_name}
+        
+        """
+        images = db.image_get_all_public(None)
+        image_dicts = [dict(id=i['id'], name=i['name']) for i in images]
+        return dict(images=image_dicts)
+
+    def detail(self, req):
+        """Return detailed information for all public, non-deleted images
+        
+        :param req: the Request object coming from the wsgi layer
+        :retval a mapping of the following form::
+
+            dict(images=[image_list])
+
+        Where image_list is a sequence of mappings containing
+        all image model fields.
+        
+        """
         images = db.image_get_all_public(None)
         image_dicts = [self._make_image_dict(i) for i in images]
         return dict(images=image_dicts)
 
-    def detail(self, req):
-        """Detail is not currently supported """
-        raise exc.HTTPNotImplemented()
-
     def show(self, req, id):
         """Return data about the given image id."""
         try:
@@ -52,8 +78,24 @@
         raise exc.HTTPNotImplemented()
 
     def create(self, req):
-        """Create is not currently supported """
-        raise exc.HTTPNotImplemented()
+        """Registers a new image with the registry.
+
+        :param req: Request body.  A JSON-ified dict of information about
+                    the image.
+
+        :retval Returns the newly-created image information as a mapping,
+                which will include the newly-created image's internal id
+                in the 'id' field
+
+        """
+        image_data = json.loads(req.body)
+
+        # Ensure the image has a status set
+        image_data.setdefault('status', 'available')
+
+        context = None
+        new_image = db.image_create(context, image_data)
+        return dict(new_image)
 
     def update(self, req, id):
         """Update is not currently supported """
@@ -64,23 +106,23 @@
         """ Create a dict represenation of an image which we can use to
         serialize the image.
         """
-        def _fetch_attrs(obj, attrs):
-            return dict([(a, getattr(obj, a)) for a in attrs])
+        def _fetch_attrs(d, attrs):
+            return dict([(a, d[a]) for a in attrs])
 
         # attributes common to all models
         base_attrs = set(['id', 'created_at', 'updated_at', 'deleted_at',
                           'deleted'])
 
         file_attrs = base_attrs | set(['location', 'size'])
-        files = [_fetch_attrs(f, file_attrs) for f in image.files]
+        files = [_fetch_attrs(f, file_attrs) for f in image['files']]
 
         # TODO(sirp): should this be a dict, or a list of dicts?
         # A plain dict is more convenient, but list of dicts would provide
         # access to created_at, etc
-        metadata = dict((m.key, m.value) for m in image.metadata 
-                        if not m.deleted)
+        metadata = dict((m['key'], m['value']) for m in image['metadata'] 
+                        if not m['deleted'])
 
-        image_attrs = base_attrs | set(['name', 'image_type', 'state', 'public'])
+        image_attrs = base_attrs | set(['name', 'image_type', 'status', 'is_public'])
         image_dict = _fetch_attrs(image, image_attrs)
 
         image_dict['files'] = files
@@ -95,5 +137,6 @@
         # TODO(sirp): should we add back the middleware for parallax?
         mapper = routes.Mapper()
         mapper.resource("image", "images", controller=ImageController(),
-                        collection={'detail': 'GET'})
+                       collection={'detail': 'GET'})
+        mapper.connect("/", controller=ImageController(), action="index")
         super(API, self).__init__(mapper)

=== modified file 'glance/parallax/db/sqlalchemy/api.py'
--- glance/parallax/db/sqlalchemy/api.py	2010-10-01 20:27:48 +0000
+++ glance/parallax/db/sqlalchemy/api.py	2010-10-13 18:50:33 +0000
@@ -95,7 +95,7 @@
                  ).options(joinedload(models.Image.files)
                  ).options(joinedload(models.Image.metadata)
                  ).filter_by(deleted=_deleted(context)
-                 ).filter_by(public=public
+                 ).filter_by(is_public=public
                  ).all()
 
 

=== modified file 'glance/parallax/db/sqlalchemy/models.py'
--- glance/parallax/db/sqlalchemy/models.py	2010-10-01 19:51:29 +0000
+++ glance/parallax/db/sqlalchemy/models.py	2010-10-13 18:50:33 +0000
@@ -23,16 +23,13 @@
 import sys
 import datetime
 
-# TODO(vish): clean up these imports
-from sqlalchemy.orm import relationship, backref, exc, object_mapper
+from sqlalchemy.orm import relationship, backref, exc, object_mapper, validates
 from sqlalchemy import Column, Integer, String
 from sqlalchemy import ForeignKey, DateTime, Boolean, Text
 from sqlalchemy.ext.declarative import declarative_base
 
 from glance.common.db.sqlalchemy.session import get_session
 
-# FIXME(sirp): confirm this is not needed
-#from common import auth
 from glance.common import exception
 from glance.common import flags
 
@@ -40,6 +37,7 @@
 
 BASE = declarative_base()
 
+
 #TODO(sirp): ModelBase should be moved out so Glance and Nova can share it
 class ModelBase(object):
     """Base class for Nova and Glance Models"""
@@ -128,18 +126,22 @@
     
     id = Column(Integer, primary_key=True)
     name = Column(String(255))
-    image_type = Column(String(255))
-    state = Column(String(255))
-    public = Column(Boolean, default=False)
+    image_type = Column(String(30))
+    status = Column(String(30))
+    is_public = Column(Boolean, default=False)
 
-    #@validates('image_type')
-    #def validate_image_type(self, key, image_type):
-    #    assert(image_type in ('machine', 'kernel', 'ramdisk', 'raw'))
-    #
-    #@validates('state')
-    #def validate_state(self, key, state):
-    #    assert(state in ('available', 'pending', 'disabled'))
-    #
+    @validates('image_type')
+    def validate_image_type(self, key, image_type):
+        if not image_type in ('machine', 'kernel', 'ramdisk', 'raw'):
+            raise exception.Invalid("Invalid image type '%s' for image." % image_type)
+        return image_type
+    
+    @validates('status')
+    def validate_status(self, key, state):
+        if not state in ('available', 'pending', 'disabled'):
+            raise exception.Invalid("Invalid status '%s' for image." % status)
+        return image_type
+    
     # TODO(sirp): should these be stored as metadata?
     #user_id = Column(String(255))
     #project_id = Column(String(255))

=== modified file 'glance/teller/controllers.py'
--- glance/teller/controllers.py	2010-10-11 18:28:35 +0000
+++ glance/teller/controllers.py	2010-10-13 18:50:33 +0000
@@ -95,7 +95,7 @@
 
 
 class API(wsgi.Router):
-    """WSGI entry point for all Parallax requests."""
+    """WSGI entry point for all Teller requests."""
 
     def __init__(self):
         mapper = routes.Mapper()

=== modified file 'run_tests.sh'
--- run_tests.sh	2010-10-08 20:42:13 +0000
+++ run_tests.sh	2010-10-13 18:50:33 +0000
@@ -41,12 +41,12 @@
 
 if [ $never_venv -eq 1 ]; then
   # Just run the test suites in current environment
-  python run_tests.py
+  nosetests --logging-clear-handlers
   exit
 fi
 
 if [ -e ${venv} ]; then
-  ${with_venv} nosetests
+  ${with_venv} nosetests --logging-clear-handlers
 else  
   if [ $always_venv -eq 1 ]; then
     # Automatically install the virtualenv
@@ -58,9 +58,9 @@
       # Install the virtualenv and run the test suite in it
       python tools/install_venv.py
     else
-      nosetests
+      nosetests --logging-clear-handlers
       exit
     fi
   fi
-  ${with_venv} nosetests
+  ${with_venv} nosetests --logging-clear-handlers
 fi

=== modified file 'tests/stubs.py'
--- tests/stubs.py	2010-10-11 18:28:35 +0000
+++ tests/stubs.py	2010-10-13 18:50:33 +0000
@@ -17,12 +17,15 @@
 
 """Stubouts, mocks and fixtures for the test suite"""
 
+import datetime
 import httplib
 import StringIO
 
 import stubout
 
+from glance.common import exception
 import glance.teller.backends.swift
+import glance.parallax.db.sqlalchemy.api
 
 def stub_out_http_backend(stubs):
     """Stubs out the httplib.HTTPRequest.getresponse to return
@@ -147,3 +150,87 @@
     fake_parallax_registry = FakeParallax()
     stubs.Set(glance.teller.registries.Parallax, 'lookup',
               fake_parallax_registry.lookup)
+
+
+def stub_out_parallax_db_image_api(stubs):
+    """Stubs out the database set/fetch API calls for Parallax
+    so the calls are routed to an in-memory dict. This helps us
+    avoid having to manually clear or flush the SQLite database.
+
+    The "datastore" always starts with this set of image fixtures.
+
+    :param stubs: Set of stubout stubs
+
+    """
+    class FakeDatastore(object):
+
+        FIXTURES = [
+            {'id': 1,
+                'name': 'fake image #1',
+                'status': 'available',
+                'image_type': 'kernel',
+                'is_public': False,
+                'created_at': datetime.datetime.utcnow(),
+                'updated_at': datetime.datetime.utcnow(),
+                'deleted_at': None,
+                'deleted': False,
+                'files': [],
+                'metadata': []},
+            {'id': 2,
+                'name': 'fake image #2',
+                'status': 'available',
+                'image_type': 'kernel',
+                'is_public': True,
+                'created_at': datetime.datetime.utcnow(),
+                'updated_at': datetime.datetime.utcnow(),
+                'deleted_at': None,
+                'deleted': False,
+                'files': [],
+                'metadata': []}]
+
+        VALID_STATUSES = ('available', 'disabled', 'pending')
+
+        def __init__(self):
+            self.images = self.FIXTURES
+            self.next_id = 3
+
+        def image_create(self, _context, values):
+            values['id'] = self.next_id
+
+            if 'status' not in values.keys():
+                values['status'] = 'available'
+            else:
+                if not values['status'] in self.VALID_STATUSES:
+                    raise exception.Invalid("Invalid status '%s' for image" % values['status'])
+            
+            self.next_id += 1
+            self.images.extend(values)
+            return values
+
+        def image_destroy(self, _context, image_id):
+            try:
+                del self.images[image_id]
+            except KeyError:
+                new_exc = exception.NotFound("No model for id %s" % image_id)
+                raise new_exc.__class__, new_exc, sys.exc_info()[2]
+
+        def image_get(self, _context, image_id):
+            if image_id not in self.images.keys() or self.images[image_id]['deleted']:
+                new_exc = exception.NotFound("No model for id %s" % image_id)
+                raise new_exc.__class__, new_exc, sys.exc_info()[2]
+            else:
+                return self.images[image_id]
+
+        def image_get_all_public(self, _context, public):
+            return [f for f in self.images
+                    if f['is_public'] == public]
+
+    fake_datastore = FakeDatastore()
+    stubs.Set(glance.parallax.db.sqlalchemy.api, 'image_create',
+              fake_datastore.image_create)
+    stubs.Set(glance.parallax.db.sqlalchemy.api, 'image_destroy',
+              fake_datastore.image_destroy)
+    stubs.Set(glance.parallax.db.sqlalchemy.api, 'image_get',
+              fake_datastore.image_get)
+    stubs.Set(glance.parallax.db.sqlalchemy.api, 'image_get_all_public',
+              fake_datastore.image_get_all_public)

=== added file 'tests/unit/test_parallax_api.py'
--- tests/unit/test_parallax_api.py	1970-01-01 00:00:00 +0000
+++ tests/unit/test_parallax_api.py	2010-10-13 18:50:33 +0000
@@ -0,0 +1,142 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2010 OpenStack, LLC
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+import json
+import stubout
+import unittest
+import webob
+
+from glance.common import exception
+from glance.parallax import controllers
+from glance.parallax import db
+from tests import stubs
+
+
+class TestImageController(unittest.TestCase):
+    def setUp(self):
+        """Establish a clean test environment"""
+        self.stubs = stubout.StubOutForTesting()
+        self.image_controller = controllers.ImageController()
+        stubs.stub_out_parallax_db_image_api(self.stubs)
+
+    def tearDown(self):
+        """Clear the test environment"""
+        self.stubs.UnsetAll()
+
+    def test_get_root(self):
+        """Tests that the root parallax API returns "index",
+        which is a list of public images
+        
+        """
+        fixture = {'id': 2,
+                   'name': 'fake image #2'}
+        req = webob.Request.blank('/')
+        res = req.get_response(controllers.API())
+        res_dict = json.loads(res.body)
+        self.assertEquals(res.status_int, 200)
+
+        images = res_dict['images']
+        self.assertEquals(len(images), 1)
+
+        for k,v in fixture.iteritems():
+            self.assertEquals(v, images[0][k])
+
+    def test_get_index(self):
+        """Tests that the /images parallax API returns list of
+        public images
+        
+        """
+        fixture = {'id': 2,
+                   'name': 'fake image #2'}
+        req = webob.Request.blank('/images')
+        res = req.get_response(controllers.API())
+        res_dict = json.loads(res.body)
+        self.assertEquals(res.status_int, 200)
+
+        images = res_dict['images']
+        self.assertEquals(len(images), 1)
+
+        for k,v in fixture.iteritems():
+            self.assertEquals(v, images[0][k])
+
+    def test_get_details(self):
+        """Tests that the /images/detail parallax API returns
+        a mapping containing a list of detailed image information
+        
+        """
+        fixture = {'id': 2,
+                   'name': 'fake image #2',
+                   'is_public': True,
+                   'image_type': 'kernel',
+                   'status': 'available'
+                  }
+        req = webob.Request.blank('/images/detail')
+        res = req.get_response(controllers.API())
+        res_dict = json.loads(res.body)
+        self.assertEquals(res.status_int, 200)
+
+        images = res_dict['images']
+        self.assertEquals(len(images), 1)
+
+        for k,v in fixture.iteritems():
+            self.assertEquals(v, images[0][k])
+
+    def test_create_image(self):
+        """Tests that the /images POST parallax API creates the image"""
+        fixture = {'name': 'fake public image',
+                   'is_public': True,
+                   'image_type': 'kernel'
+                  }
+
+        req = webob.Request.blank('/images')
+            
+        req.method = 'POST'
+        req.body = json.dumps(fixture)
+
+        res = req.get_response(controllers.API())
+
+        self.assertEquals(res.status_int, 200)
+
+        res_dict = json.loads(res.body)
+
+        for k,v in fixture.iteritems():
+            self.assertEquals(v, res_dict[k])
+
+        # Test ID auto-assigned properly
+        self.assertEquals(3, res_dict['id'])
+
+        # Test status was updated properly
+        self.assertEquals('available', res_dict['status'])
+
+    def test_create_image_with_bad_status(self):
+        """Tests proper exception is raised if a bad status is set"""
+        fixture = {'id': 3,
+                   'name': 'fake public image',
+                   'is_public': True,
+                   'image_type': 'kernel',
+                   'status': 'bad status'
+                  }
+
+        req = webob.Request.blank('/images')
+            
+        req.method = 'POST'
+        req.body = json.dumps(fixture)
+
+        # TODO(jaypipes): Port Nova's Fault infrastructure
+        # over to Glance to support exception catching into
+        # standard HTTP errors.
+        self.assertRaises(exception.Invalid, req.get_response, controllers.API())