Merge lp:~rackspace-titan/glance/api-pagination-limiting into lp:~hudson-openstack/glance/trunk

[Brian Waldon]

Adding support for marker/limit query params from api, through registry client/api, and implementing at registry db api layer

[Jay Pipes (jaypipes)]

Hi Brian,

Once again, great work. Couple little things:

1) Something I did not think about for the results filtering MP, as well as this one, is that we will want to bump the minor version since we are adding functionality to the API in these two MPs.

You can do this in a separate merge proposal, I suppose. Basically, what needs to happen is that we need a way of describing the 1.1 API (1.0 + filters and limits) in the /versions endpoint.

The reason this is important is that the API result for /images has changed: we go from returning ALL public images in no particular order to returning SOME public images in a particular order (by created_at DESC). So, we should bump the minor version of the API to indicate to clients that this has been done.

Since we are in the middle of the D1 milestone, I'm not too concerned about having the results-filter branch, then this branch, then the API 1.1 branch going into trunk in that particular order. I'll handle the versioning stuff next week when I return from vacation.

2) I'd prefer to set the default limit to < 1000. I'm thinking 25 or 50 would be a more appropriate default limit. Also, please make this a constant somewhere, not just a number in the method signature...

3) Unless I'm mistaken these functions:

50 +def limit_collection(items, marker, limit):
243 + def _limit_collection(self, req, images):

in glance.common.wsgi and glance.registry.server, respectively, can be removed. You've pushed (correctly) the limiting and offset down into the registry db query.

4) This:

114 if filters != None:
115 - action = "/images?%s" % urllib.urlencode(filters)
116 + params = filters
117 else:
118 - action = "/images"
119 + params = {}

Might be better as:

params = filters or {}

5)

184 + order_by(models.Image.id)

I think I would prefer:

order_by(desc(models.Image.created_at))

6)

192 + if marker != None:
193 + query = query.filter(models.Image.id > marker)

Should be:

if marker != None:
    query = query.offset(marker)

That way, SQLAlchemy will pass the marker as-is to the query as a LIMIT X OFFSET Y. This allows more than just the Image.id column to be used for ordering, and will allow you to add custom ordering to the API ;)

-jay

review: Needs Fixing

[Brian Waldon (bcwaldon)]

> Hi Brian,
>
> Once again, great work. Couple little things:
>
> 1) Something I did not think about for the results filtering MP, as well as
> this one, is that we will want to bump the minor version since we are adding
> functionality to the API in these two MPs.
>
> You can do this in a separate merge proposal, I suppose. Basically, what needs
> to happen is that we need a way of describing the 1.1 API (1.0 + filters and
> limits) in the /versions endpoint.
>
> The reason this is important is that the API result for /images has changed:
> we go from returning ALL public images in no particular order to returning
> SOME public images in a particular order (by created_at DESC). So, we should
> bump the minor version of the API to indicate to clients that this has been
> done.
>
> Since we are in the middle of the D1 milestone, I'm not too concerned about
> having the results-filter branch, then this branch, then the API 1.1 branch
> going into trunk in that particular order. I'll handle the versioning stuff
> next week when I return from vacation.

According to the last versioning-related discussion we had, these changes were going to be part of v1.0. We haven't cut an official v1.0 release yet, and the limiting changes are definitely a major version change. I say we let the filtering and pagination branches in then call it v1.0. Any new feature (sets?) bump the version.

> 2) I'd prefer to set the default limit to < 1000. I'm thinking 25 or 50 would
> be a more appropriate default limit. Also, please make this a constant
> somewhere, not just a number in the method signature...

I meant to do that before I pushed the branch. Now it's a constant in registry/server.py, set to 25

> 3) Unless I'm mistaken these functions:
>
> 50 +def limit_collection(items, marker, limit):
> 243 + def _limit_collection(self, req, images):
>
> in glance.common.wsgi and glance.registry.server, respectively, can be
> removed. You've pushed (correctly) the limiting and offset down into the
> registry db query.

Good catch. Removed them.

> 4) This:
>
> 114 if filters != None:
> 115 - action = "/images?%s" % urllib.urlencode(filters)
> 116 + params = filters
> 117 else:
> 118 - action = "/images"
> 119 + params = {}
>
> Might be better as:
>
> params = filters or {}

Yep. Did this in a bunch of places.

> 5)
>
> 184 + order_by(models.Image.id)
>
> I think I would prefer:
>
> order_by(desc(models.Image.created_at))

Done. I kept the secondary order_by id for predictability in the behavior of marker b/c created_at is not unique.

Shoot for custom ordering in a future milestone (for v1.1)?

> 6)
>
> 192 + if marker != None:
> 193 + query = query.filter(models.Image.id > marker)
>
> Should be:
>
> if marker != None:
> query = query.offset(marker)
>
> That way, SQLAlchemy will pass the marker as-is to the query as a LIMIT X
> OFFSET Y. This allows more than just the Image.id column to be used for
> ordering, and will allow you to add custom ordering to the API ;)

Actually, the term "offset" means something different than "marker". Offset is an index in a list of images, while marker is the actual id of ...

[Jay Pipes (jaypipes)]

On Tue, May 17, 2011 at 8:42 PM, Brian Waldon
<email address hidden> wrote:
> According to the last versioning-related discussion we had, these changes were going to be part of v1.0. We haven't cut an official v1.0 release yet, and the limiting changes are definitely a major version change. I say we let the filtering and pagination branches in then call it v1.0. Any new feature (sets?) bump the version.

Cool, yep, that sounds fine.

>> 2) I'd prefer to set the default limit to < 1000. I'm thinking 25 or 50 would
>> be a more appropriate default limit. Also, please make this a constant
>> somewhere, not just a number in the method signature...
>
> I meant to do that before I pushed the branch. Now it's a constant in registry/server.py, set to 25

Cheers.

>> 3) Unless I'm mistaken these functions:
>>
>> 50      +def limit_collection(items, marker, limit):
>> 243     + def _limit_collection(self, req, images):
>>
>> in glance.common.wsgi and glance.registry.server, respectively, can be
>> removed. You've pushed (correctly) the limiting and offset down into the
>> registry db query.
>
> Good catch. Removed them.

Cool.

>> 4) This:
>>
>> 114     if filters != None:
>> 115     - action = "/images?%s" % urllib.urlencode(filters)
>> 116     + params = filters
>> 117     else:
>> 118     - action = "/images"
>> 119     + params = {}
>>
>> Might be better as:
>>
>> params = filters or {}
>
> Yep. Did this in a bunch of places.

Thanks.

>> 5)
>>
>> 184     + order_by(models.Image.id)
>>
>> I think I would prefer:
>>
>> order_by(desc(models.Image.created_at))
>
> Done. I kept the secondary order_by id for predictability in the behavior of marker b/c created_at is not unique.

Hmm, so you actually don't need created_at to be unique if you replace
the concept of marker with that of offset. See more below...

> Shoot for custom ordering in a future milestone (for v1.1)?

Cool with me :)

>> 6)
>>
>> 192     + if marker != None:
>> 193     + query = query.filter(models.Image.id > marker)
>>
>> Should be:
>>
>> if marker != None:
>>     query = query.offset(marker)
>>
>> That way, SQLAlchemy will pass the marker as-is to the query as a LIMIT X
>> OFFSET Y. This allows more than just the Image.id column to be used for
>> ordering, and will allow you to add custom ordering to the API ;)
>
> Actually, the term "offset" means something different than "marker". Offset is an index in a list of images, while marker is the actual id of an image in that list. I did have to change the code to support the new created_at sort, but I kept the original functionality.

I understand that marker is different from offset. I was recommending
that instead of using the concept of a marker, that you use the
concept of an offset. A marker isn't particularly useful unless you
have do ordering with a unique column. In addition, using a marker
requires you to do the pagination in code, whereas using an offset
allows you to do the pagination in the SQL query itself, which will be
much more efficient than in Python.

The code I demonstrated above would send the offset to the SQL query
and allow you to do pagination in the query instead of this:

398 + images = sorted(images, k...

[Brian Waldon (bcwaldon)]

I updated the code to actually implement marker correctly. This branch should implement what was agreed upon (or proposed and waiting to be approved?) on the mailing list:

Quoted from that email:
1) Push the LIMIT variable into the database API layer
2) Ensure that all queries that return a set of results have an ORDER
BY expression to them
3) Push the marker into the database API layer. Continue to have the
marker variable be a value of a unique key (primary key for now at
least). Use a WHERE field > $marker LIMIT $pagesize construct.

[Brian Waldon (bcwaldon)]

sqlalchemy query is still bad. I didn't test it thoroughly enough

[Brian Waldon (bcwaldon)]

Washenberger helped me out. Should work now. Blame him if it doesn't.

[Jay Pipes (jaypipes)]

Heya!

Lookin' good...

One thing though; we decided on the ML thread that we would *not* do the additional filter on the created_at date; that the possibility of a page of results changing over time was not a big deal and the marker would just be used as the start of the subset.

So, this code:

173 + # images returned should be created before the image defined by marker
174 + marker_created_at = image_get(context, marker).created_at
175 + query = query.filter(
176 + or_(models.Image.created_at < marker_created_at,
177 + and_(models.Image.created_at == marker_created_at,
178 + models.Image.id < marker)))

Should just be:

query = query.filter(models.Image.id > marker)

marker should be the primary key (or a unique index) of the last record on the previous page, or None for the first page.

Make sense?

-jay

[Brian Waldon (bcwaldon)]

> One thing though; we decided on the ML thread that we would *not* do the
> additional filter on the created_at date; that the possibility of a page of
> results changing over time was not a big deal and the marker would just be
> used as the start of the subset.

[snip snip]

> query = query.filter(models.Image.id > marker)
>
> marker should be the primary key (or a unique index) of the last record on the
> previous page, or None for the first page.

So are you suggesting we sort by id asc (assuming your comparison isn't a typo)? My code does impose a default sort of created_at desc.

[Jay Pipes (jaypipes)]

> So are you suggesting we sort by id asc (assuming your comparison isn't a
> typo)? My code does impose a default sort of created_at desc.

Hmm, good point. Should have thought of that... :)

I'd keep the default order as ORDER BY created_at DESC, id DESC, but change the WHERE id > $marker to the other direction: WHERE id < $marker. When we do custom ordering, the direction (> or <) will be determined by the direction of the sort order. Am I correct in that thinking?

-jay

[Brian Waldon (bcwaldon)]

> I'd keep the default order as ORDER BY created_at DESC, id DESC, but change
> the WHERE id > $marker to the other direction: WHERE id < $marker. When we do
> custom ordering, the direction (> or <) will be determined by the direction of
> the sort order. Am I correct in that thinking?

Yes sir. So I think what I have should be good, assuming you understand why I am first doing a comparison on created_at.

[Jay Pipes (jaypipes)]

> > I'd keep the default order as ORDER BY created_at DESC, id DESC, but change
> > the WHERE id > $marker to the other direction: WHERE id < $marker. When we
> do
> > custom ordering, the direction (> or <) will be determined by the direction
> of
> > the sort order. Am I correct in that thinking?
>
> Yes sir. So I think what I have should be good, assuming you understand why I
> am first doing a comparison on created_at.

Yep.

And you understand in the future for custom ordering, you only need to do the or_ expression when the order is on a field that does not have a unique index :)

-jay

review: Approve

[Brian Waldon (bcwaldon)]

Added support to glance/client.py

[Dan Prince (dan-prince)]

Looks good to me.

review: Approve

[Mark Washenberger (markwash)]

This sqlalchemy looks exactly like what we talked about. Clear eyes full hearts can't lose.

review: Approve

=== modified file 'glance/api/v1/images.py'
--- glance/api/v1/images.py	2011-05-17 13:27:09 +0000
+++ glance/api/v1/images.py	2011-05-31 13:23:37 +0000
@@ -92,8 +92,15 @@
                  'size': <SIZE>}, ...
             ]}
         """
-        filters = self._get_filters(req)
-        images = registry.get_images_list(self.options, filters)
+        params = {'filters': self._get_filters(req)}
+
+        if 'limit' in req.str_params:
+            params['limit'] = req.str_params.get('limit')
+
+        if 'marker' in req.str_params:
+            params['marker'] = req.str_params.get('marker')
+
+        images = registry.get_images_list(self.options, **params)
         return dict(images=images)
 
     def detail(self, req):
@@ -118,8 +125,15 @@
                  'properties': {'distro': 'Ubuntu 10.04 LTS', ...}}, ...
             ]}
         """
-        filters = self._get_filters(req)
-        images = registry.get_images_detail(self.options, filters)
+        params = {'filters': self._get_filters(req)}
+
+        if 'limit' in req.str_params:
+            params['limit'] = req.str_params.get('limit')
+
+        if 'marker' in req.str_params:
+            params['marker'] = req.str_params.get('marker')
+
+        images = registry.get_images_detail(self.options, **params)
         return dict(images=images)
 
     def _get_filters(self, req):

=== modified file 'glance/client.py'
--- glance/client.py	2011-05-24 19:47:59 +0000
+++ glance/client.py	2011-05-31 13:23:37 +0000
@@ -209,19 +209,41 @@
         return super(V1Client, self).do_request(method, action, body,
                                                 headers, params)
 
-    def get_images(self, filters=None):
+    def get_images(self, filters=None, marker=None, limit=None):
         """
         Returns a list of image id/name mappings from Registry
+
+        :param filters: dictionary of attributes by which the resulting
+                        collection of images should be filtered
+        :param marker: id after which to start the page of images
+        :param limit: maximum number of items to return
         """
-        res = self.do_request("GET", "/images", params=filters)
+
+        params = filters or {}
+        if marker:
+            params['marker'] = marker
+        if limit:
+            params['limit'] = limit
+        res = self.do_request("GET", "/images", params=params)
         data = json.loads(res.read())['images']
         return data
 
-    def get_images_detailed(self, filters=None):
+    def get_images_detailed(self, filters=None, marker=None, limit=None):
         """
         Returns a list of detailed image data mappings from Registry
+
+        :param filters: dictionary of attributes by which the resulting
+                        collection of images should be filtered
+        :param marker: id after which to start the page of images
+        :param limit: maximum number of items to return
         """
-        res = self.do_request("GET", "/images/detail", params=filters)
+
+        params = filters or {}
+        if marker:
+            params['marker'] = marker
+        if limit:
+            params['limit'] = limit
+        res = self.do_request("GET", "/images/detail", params=params)
         data = json.loads(res.read())['images']
         return data
 

=== modified file 'glance/registry/__init__.py'
--- glance/registry/__init__.py	2011-05-15 01:05:04 +0000
+++ glance/registry/__init__.py	2011-05-31 13:23:37 +0000
@@ -32,14 +32,14 @@
     return client.RegistryClient(host, port)
 
 
-def get_images_list(options, filters):
-    c = get_registry_client(options)
-    return c.get_images(filters)
-
-
-def get_images_detail(options, filters):
-    c = get_registry_client(options)
-    return c.get_images_detailed(filters)
+def get_images_list(options, **kwargs):
+    c = get_registry_client(options)
+    return c.get_images(**kwargs)
+
+
+def get_images_detail(options, **kwargs):
+    c = get_registry_client(options)
+    return c.get_images_detailed(**kwargs)
 
 
 def get_image_metadata(options, image_id):

=== modified file 'glance/registry/client.py'
--- glance/registry/client.py	2011-05-24 19:47:59 +0000
+++ glance/registry/client.py	2011-05-31 13:23:37 +0000
@@ -44,19 +44,43 @@
         port = port or self.DEFAULT_PORT
         super(RegistryClient, self).__init__(host, port, use_ssl)
 
-    def get_images(self, filters=None):
+    def get_images(self, filters=None, marker=None, limit=None):
         """
         Returns a list of image id/name mappings from Registry
+
+        :param filters: dict of keys & expected values to filter results
+        :param marker: image id after which to start page
+        :param limit: max number of images to return
         """
-        res = self.do_request("GET", "/images", params=filters)
+        params = filters or {}
+
+        if marker != None:
+            params['marker'] = marker
+
+        if limit != None:
+            params['limit'] = limit
+
+        res = self.do_request("GET", "/images", params=params)
         data = json.loads(res.read())['images']
         return data
 
-    def get_images_detailed(self, filters=None):
+    def get_images_detailed(self, filters=None, marker=None, limit=None):
         """
         Returns a list of detailed image data mappings from Registry
+
+        :param filters: dict of keys & expected values to filter results
+        :param marker: image id after which to start page
+        :param limit: max number of images to return
         """
-        res = self.do_request("GET", "/images/detail", params=filters)
+        params = filters or {}
+
+        if marker != None:
+            params['marker'] = marker
+
+        if limit != None:
+            params['limit'] = limit
+
+        res = self.do_request("GET", "/images/detail", params=params)
         data = json.loads(res.read())['images']
         return data
 

=== modified file 'glance/registry/db/api.py'
--- glance/registry/db/api.py	2011-05-27 02:24:55 +0000
+++ glance/registry/db/api.py	2011-05-31 13:23:37 +0000
@@ -23,11 +23,12 @@
 
 import logging
 
-from sqlalchemy import create_engine
+from sqlalchemy import create_engine, desc
 from sqlalchemy.ext.declarative import declarative_base
 from sqlalchemy.orm import exc
 from sqlalchemy.orm import joinedload
 from sqlalchemy.orm import sessionmaker
+from sqlalchemy.sql import or_, and_
 
 from glance.common import config
 from glance.common import exception
@@ -127,23 +128,26 @@
         raise exception.NotFound("No image found with ID %s" % image_id)
 
 
-def image_get_all_public(context, filters=None):
+def image_get_all_public(context, filters=None, marker=None, limit=None):
     """Get all public images that match zero or more filters.
 
     :param filters: dict of filter keys and values. If a 'properties'
                     key is present, it is treated as a dict of key/value
                     filters on the image properties attribute
+    :param marker: image id after which to start page
+    :param limit: maximum number of images to return
 
     """
-    if filters == None:
-        filters = {}
+    filters = filters or {}
 
     session = get_session()
     query = session.query(models.Image).\
                    options(joinedload(models.Image.properties)).\
                    filter_by(deleted=_deleted(context)).\
                    filter_by(is_public=True).\
-                   filter(models.Image.status != 'killed')
+                   filter(models.Image.status != 'killed').\
+                   order_by(desc(models.Image.created_at)).\
+                   order_by(desc(models.Image.id))
 
     if 'size_min' in filters:
         query = query.filter(models.Image.size >= filters['size_min'])
@@ -159,6 +163,17 @@
     for (k, v) in filters.items():
         query = query.filter(getattr(models.Image, k) == v)
 
+    if marker != None:
+        # images returned should be created before the image defined by marker
+        marker_created_at = image_get(context, marker).created_at
+        query = query.filter(
+            or_(models.Image.created_at < marker_created_at,
+                and_(models.Image.created_at == marker_created_at,
+                     models.Image.id < marker)))
+
+    if limit != None:
+        query = query.limit(limit)
+
     return query.all()
 
 

=== modified file 'glance/registry/server.py'
--- glance/registry/server.py	2011-05-16 13:20:53 +0000
+++ glance/registry/server.py	2011-05-31 13:23:37 +0000
@@ -39,6 +39,8 @@
 SUPPORTED_FILTERS = ['name', 'status', 'container_format', 'disk_format',
                      'size_min', 'size_max']
 
+MAX_ITEM_LIMIT = 25
+
 
 class Controller(wsgi.Controller):
     """Controller for the reference implementation registry server"""
@@ -67,7 +69,15 @@
             }
 
         """
-        images = db_api.image_get_all_public(None, self._get_filters(req))
+        params = {
+            'filters': self._get_filters(req),
+            'limit': self._get_limit(req),
+        }
+
+        if 'marker' in req.str_params:
+            params['marker'] = self._get_marker(req)
+
+        images = db_api.image_get_all_public(None, **params)
 
         results = []
         for image in images:
@@ -89,7 +99,15 @@
         all image model fields.
 
         """
-        images = db_api.image_get_all_public(None, self._get_filters(req))
+        params = {
+            'filters': self._get_filters(req),
+            'limit': self._get_limit(req),
+        }
+
+        if 'marker' in req.str_params:
+            params['marker'] = self._get_marker(req)
+
+        images = db_api.image_get_all_public(None, **params)
 
         image_dicts = [make_image_dict(i) for i in images]
         return dict(images=image_dicts)
@@ -116,6 +134,26 @@
 
         return filters
 
+    def _get_limit(self, req):
+        """Parse a limit query param into something usable."""
+        try:
+            limit = int(req.str_params.get('limit', MAX_ITEM_LIMIT))
+        except ValueError:
+            raise exc.HTTPBadRequest("limit param must be an integer")
+
+        if limit < 0:
+            raise exc.HTTPBadRequest("limit param must be positive")
+
+        return min(MAX_ITEM_LIMIT, limit)
+
+    def _get_marker(self, req):
+        """Parse a marker query param into something usable."""
+        try:
+            marker = int(req.str_params.get('marker', None))
+        except ValueError:
+            raise exc.HTTPBadRequest("marker param must be an integer")
+        return marker
+
     def show(self, req, id):
         """Return data about the given image id."""
         try:

=== modified file 'tests/functional/test_curl_api.py'
--- tests/functional/test_curl_api.py	2011-05-27 14:29:59 +0000
+++ tests/functional/test_curl_api.py	2011-05-31 13:23:37 +0000
@@ -1001,3 +1001,119 @@
             self.assertEqual(image["name"], "My Image!")
 
         self.stop_servers()
+
+    def test_limited_images(self):
+        """
+        Ensure marker and limit query params work
+        """
+        self.cleanup()
+        self.start_servers()
+
+        api_port = self.api_port
+        registry_port = self.registry_port
+
+        # 0. GET /images
+        # Verify no public images
+        cmd = "curl http://0.0.0.0:%d/v1/images" % api_port
+
+        exitcode, out, err = execute(cmd)
+
+        self.assertEqual(0, exitcode)
+        self.assertEqual('{"images": []}', out.strip())
+
+        # 1. POST /images with three public images with various attributes
+        cmd = ("curl -i -X POST "
+               "-H 'Expect: ' "  # Necessary otherwise sends 100 Continue
+               "-H 'X-Image-Meta-Name: Image1' "
+               "-H 'X-Image-Meta-Is-Public: True' "
+               "http://0.0.0.0:%d/v1/images") % api_port
+
+        exitcode, out, err = execute(cmd)
+        self.assertEqual(0, exitcode)
+
+        lines = out.split("\r\n")
+        status_line = lines[0]
+
+        self.assertEqual("HTTP/1.1 201 Created", status_line)
+
+        cmd = ("curl -i -X POST "
+               "-H 'Expect: ' "  # Necessary otherwise sends 100 Continue
+               "-H 'X-Image-Meta-Name: Image2' "
+               "-H 'X-Image-Meta-Is-Public: True' "
+               "http://0.0.0.0:%d/v1/images") % api_port
+
+        exitcode, out, err = execute(cmd)
+        self.assertEqual(0, exitcode)
+
+        lines = out.split("\r\n")
+        status_line = lines[0]
+
+        self.assertEqual("HTTP/1.1 201 Created", status_line)
+
+        cmd = ("curl -i -X POST "
+               "-H 'Expect: ' "  # Necessary otherwise sends 100 Continue
+               "-H 'X-Image-Meta-Name: Image3' "
+               "-H 'X-Image-Meta-Is-Public: True' "
+               "http://0.0.0.0:%d/v1/images") % api_port
+
+        exitcode, out, err = execute(cmd)
+        self.assertEqual(0, exitcode)
+
+        lines = out.split("\r\n")
+        status_line = lines[0]
+
+        self.assertEqual("HTTP/1.1 201 Created", status_line)
+
+        # 2. GET /images with limit of 2
+        # Verify only two images were returned
+        cmd = "curl http://0.0.0.0:%d/v1/images?limit=2" % api_port
+
+        exitcode, out, err = execute(cmd)
+
+        self.assertEqual(0, exitcode)
+        images = json.loads(out.strip())['images']
+
+        self.assertEqual(len(images), 2)
+        self.assertEqual(int(images[0]['id']), 3)
+        self.assertEqual(int(images[1]['id']), 2)
+
+        # 3. GET /images with marker
+        # Verify only two images were returned
+        cmd = "curl http://0.0.0.0:%d/v1/images?marker=3" % api_port
+
+        exitcode, out, err = execute(cmd)
+
+        self.assertEqual(0, exitcode)
+        images = json.loads(out.strip())['images']
+
+        self.assertEqual(len(images), 2)
+        self.assertEqual(int(images[0]['id']), 2)
+        self.assertEqual(int(images[1]['id']), 1)
+
+        # 4. GET /images with marker and limit
+        # Verify only one image was returned with the correct id
+        cmd = ("curl 'http://0.0.0.0:%d/v1/images?"
+               "limit=1&marker=2'" % api_port)
+
+        exitcode, out, err = execute(cmd)
+
+        self.assertEqual(0, exitcode)
+        images = json.loads(out.strip())['images']
+
+        self.assertEqual(len(images), 1)
+        self.assertEqual(int(images[0]['id']), 1)
+
+        # 5. GET /images/detail with marker and limit
+        # Verify only one image was returned with the correct id
+        cmd = ("curl 'http://0.0.0.0:%d/v1/images/detail?"
+               "limit=1&marker=3'" % api_port)
+
+        exitcode, out, err = execute(cmd)
+
+        self.assertEqual(0, exitcode)
+        images = json.loads(out.strip())['images']
+
+        self.assertEqual(len(images), 1)
+        self.assertEqual(int(images[0]['id']), 2)
+
+        self.stop_servers()

=== modified file 'tests/stubs.py'
--- tests/stubs.py	2011-05-16 13:20:53 +0000
+++ tests/stubs.py	2011-05-31 13:23:37 +0000
@@ -19,6 +19,7 @@
 
 import datetime
 import httplib
+import operator
 import os
 import shutil
 import StringIO
@@ -322,8 +323,10 @@
             values['deleted'] = False
             values['properties'] = values.get('properties', {})
             values['location'] = values.get('location')
-            values['created_at'] = datetime.datetime.utcnow()
-            values['updated_at'] = datetime.datetime.utcnow()
+
+            now = datetime.datetime.utcnow()
+            values['created_at'] = values.get('created_at', now)
+            values['updated_at'] = values.get('updated_at', now)
             values['deleted_at'] = None
 
             props = []
@@ -334,8 +337,8 @@
                     p['name'] = k
                     p['value'] = v
                     p['deleted'] = False
-                    p['created_at'] = datetime.datetime.utcnow()
-                    p['updated_at'] = datetime.datetime.utcnow()
+                    p['created_at'] = now
+                    p['updated_at'] = now
                     p['deleted_at'] = None
                     props.append(p)
 
@@ -386,7 +389,8 @@
             else:
                 return images[0]
 
-        def image_get_all_public(self, _context, filters):
+        def image_get_all_public(self, _context, filters=None,
+                                 marker=None, limit=1000):
             images = [f for f in self.images if f['is_public'] == True]
 
             if 'size_min' in filters:
@@ -411,7 +415,30 @@
             for k, v in filters.items():
                 images = [f for f in images if f[k] == v]
 
-            return images
+            def image_cmp(x, y):
+                if x['created_at'] > y['created_at']:
+                    return 1
+                elif x['created_at'] == y['created_at']:
+                    if x['id'] > y['id']:
+                        return 1
+                    else:
+                        return -1
+                else:
+                    return -1
+
+            images = sorted(images, cmp=image_cmp)
+            images.reverse()
+
+            if marker == None:
+                start_index = 0
+            else:
+                start_index = -1
+                for i, image in enumerate(images):
+                    if image['id'] == marker:
+                        start_index = i + 1
+                        break
+
+            return images[start_index:start_index + limit]
 
     fake_datastore = FakeDatastore()
     stubs.Set(glance.registry.db.api, 'image_create',

=== modified file 'tests/unit/test_api.py'
--- tests/unit/test_api.py	2011-05-27 02:24:55 +0000
+++ tests/unit/test_api.py	2011-05-31 13:23:37 +0000
@@ -15,6 +15,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import datetime
 import hashlib
 import httplib
 import os
@@ -88,6 +89,158 @@
         for k, v in fixture.iteritems():
             self.assertEquals(v, images[0][k])
 
+    def test_get_index_marker(self):
+        """Tests that the /images registry API returns list of
+        public images that conforms to a marker query param
+
+        """
+
+        time1 = datetime.datetime.utcnow() + datetime.timedelta(seconds=5)
+        time2 = datetime.datetime.utcnow()
+
+        extra_fixture = {'id': 3,
+                         'status': 'active',
+                         'is_public': True,
+                         'disk_format': 'vhd',
+                         'container_format': 'ovf',
+                         'name': 'new name! #123',
+                         'size': 19,
+                         'checksum': None,
+                         'created_at': time1}
+
+        glance.registry.db.api.image_create(None, extra_fixture)
+
+        extra_fixture = {'id': 4,
+                         'status': 'active',
+                         'is_public': True,
+                         'disk_format': 'vhd',
+                         'container_format': 'ovf',
+                         'name': 'new name! #123',
+                         'size': 20,
+                         'checksum': None,
+                         'created_at': time1}
+
+        glance.registry.db.api.image_create(None, extra_fixture)
+
+        extra_fixture = {'id': 5,
+                         'status': 'active',
+                         'is_public': True,
+                         'disk_format': 'vhd',
+                         'container_format': 'ovf',
+                         'name': 'new name! #123',
+                         'size': 20,
+                         'checksum': None,
+                         'created_at': time2}
+
+        glance.registry.db.api.image_create(None, extra_fixture)
+
+        req = webob.Request.blank('/images?marker=4')
+        res = req.get_response(self.api)
+        res_dict = json.loads(res.body)
+        self.assertEquals(res.status_int, 200)
+
+        images = res_dict['images']
+        # should be sorted by created_at desc, id desc
+        # page should start after marker 4
+        self.assertEquals(len(images), 3)
+        self.assertEquals(int(images[0]['id']), 3)
+        self.assertEquals(int(images[1]['id']), 5)
+        self.assertEquals(int(images[2]['id']), 2)
+
+    def test_get_index_limit(self):
+        """Tests that the /images registry API returns list of
+        public images that conforms to a limit query param
+
+        """
+        extra_fixture = {'id': 3,
+                         'status': 'active',
+                         'is_public': True,
+                         'disk_format': 'vhd',
+                         'container_format': 'ovf',
+                         'name': 'new name! #123',
+                         'size': 19,
+                         'checksum': None}
+
+        glance.registry.db.api.image_create(None, extra_fixture)
+
+        extra_fixture = {'id': 4,
+                         'status': 'active',
+                         'is_public': True,
+                         'disk_format': 'vhd',
+                         'container_format': 'ovf',
+                         'name': 'new name! #123',
+                         'size': 20,
+                         'checksum': None}
+
+        glance.registry.db.api.image_create(None, extra_fixture)
+
+        req = webob.Request.blank('/images?limit=1')
+        res = req.get_response(self.api)
+        res_dict = json.loads(res.body)
+        self.assertEquals(res.status_int, 200)
+
+        images = res_dict['images']
+        self.assertEquals(len(images), 1)
+
+        # expect list to be sorted by created_at desc
+        self.assertTrue(int(images[0]['id']), 4)
+
+    def test_get_index_limit_negative(self):
+        """Tests that the /images registry API returns list of
+        public images that conforms to a limit query param
+
+        """
+        req = webob.Request.blank('/images?limit=-1')
+        res = req.get_response(self.api)
+        self.assertEquals(res.status_int, 400)
+
+    def test_get_index_limit_non_int(self):
+        """Tests that the /images registry API returns list of
+        public images that conforms to a limit query param
+
+        """
+        req = webob.Request.blank('/images?limit=a')
+        res = req.get_response(self.api)
+        self.assertEquals(res.status_int, 400)
+
+    def test_get_index_limit_marker(self):
+        """Tests that the /images registry API returns list of
+        public images that conforms to limit and marker query params
+
+        """
+        extra_fixture = {'id': 3,
+                         'status': 'active',
+                         'is_public': True,
+                         'disk_format': 'vhd',
+                         'container_format': 'ovf',
+                         'name': 'new name! #123',
+                         'size': 19,
+                         'checksum': None}
+
+        glance.registry.db.api.image_create(None, extra_fixture)
+
+        extra_fixture = {'id': 4,
+                         'status': 'active',
+                         'is_public': True,
+                         'disk_format': 'vhd',
+                         'container_format': 'ovf',
+                         'name': 'new name! #123',
+                         'size': 20,
+                         'checksum': None}
+
+        glance.registry.db.api.image_create(None, extra_fixture)
+
+        req = webob.Request.blank('/images?marker=3&limit=1')
+        res = req.get_response(self.api)
+        res_dict = json.loads(res.body)
+        self.assertEquals(res.status_int, 200)
+
+        images = res_dict['images']
+        self.assertEquals(len(images), 1)
+
+        # expect list to be sorted by created_at desc
+        self.assertEqual(int(images[0]['id']), 2)
+
     def test_get_index_filter_name(self):
         """Tests that the /images registry API returns list of
         public images that have a specific name. This is really a sanity
@@ -157,6 +310,46 @@
         for k, v in fixture.iteritems():
             self.assertEquals(v, images[0][k])
 
+    def test_get_details_limit_marker(self):
+        """Tests that the /images/details registry API returns list of
+        public images that conforms to limit and marker query params.
+        This functionality is tested more thoroughly on /images, this is
+        just a sanity check
+
+        """
+        extra_fixture = {'id': 3,
+                         'status': 'active',
+                         'is_public': True,
+                         'disk_format': 'vhd',
+                         'container_format': 'ovf',
+                         'name': 'new name! #123',
+                         'size': 19,
+                         'checksum': None}
+
+        glance.registry.db.api.image_create(None, extra_fixture)
+
+        extra_fixture = {'id': 4,
+                         'status': 'active',
+                         'is_public': True,
+                         'disk_format': 'vhd',
+                         'container_format': 'ovf',
+                         'name': 'new name! #123',
+                         'size': 20,
+                         'checksum': None}
+
+        glance.registry.db.api.image_create(None, extra_fixture)
+
+        req = webob.Request.blank('/images/detail?marker=3&limit=1')
+        res = req.get_response(self.api)
+        res_dict = json.loads(res.body)
+        self.assertEquals(res.status_int, 200)
+
+        images = res_dict['images']
+        self.assertEquals(len(images), 1)
+
+        # expect list to be sorted by created_at desc
+        self.assertEqual(int(images[0]['id']), 2)
+
     def test_get_details_filter_name(self):
         """Tests that the /images/detail registry API returns list of
         public images that have a specific name

=== modified file 'tests/unit/test_clients.py'
--- tests/unit/test_clients.py	2011-05-24 19:07:19 +0000
+++ tests/unit/test_clients.py	2011-05-31 13:23:37 +0000
@@ -70,6 +70,92 @@
         for k, v in fixture.items():
             self.assertEquals(v, images[0][k])
 
+    def test_get_image_index_marker(self):
+        """Test correct set of images returned with marker param."""
+        extra_fixture = {'id': 3,
+                         'status': 'saving',
+                         'is_public': True,
+                         'disk_format': 'vhd',
+                         'container_format': 'ovf',
+                         'name': 'new name! #123',
+                         'size': 19,
+                         'checksum': None}
+
+        glance.registry.db.api.image_create(None, extra_fixture)
+
+        extra_fixture = {'id': 4,
+                         'status': 'saving',
+                         'is_public': True,
+                         'disk_format': 'vhd',
+                         'container_format': 'ovf',
+                         'name': 'new name! #125',
+                         'size': 19,
+                         'checksum': None}
+
+        glance.registry.db.api.image_create(None, extra_fixture)
+
+        images = self.client.get_images(marker=4)
+        self.assertEquals(len(images), 2)
+
+        for image in images:
+            self.assertTrue(image['id'] < 4)
+
+    def test_get_image_index_limit(self):
+        """Test correct number of images returned with limit param."""
+        extra_fixture = {'id': 3,
+                         'status': 'saving',
+                         'is_public': True,
+                         'disk_format': 'vhd',
+                         'container_format': 'ovf',
+                         'name': 'new name! #123',
+                         'size': 19,
+                         'checksum': None}
+
+        glance.registry.db.api.image_create(None, extra_fixture)
+
+        extra_fixture = {'id': 4,
+                         'status': 'saving',
+                         'is_public': True,
+                         'disk_format': 'vhd',
+                         'container_format': 'ovf',
+                         'name': 'new name! #125',
+                         'size': 19,
+                         'checksum': None}
+
+        glance.registry.db.api.image_create(None, extra_fixture)
+
+        images = self.client.get_images(limit=2)
+        self.assertEquals(len(images), 2)
+
+    def test_get_image_index_marker_limit(self):
+        """Test correct set of images returned with marker/limit params."""
+        extra_fixture = {'id': 3,
+                         'status': 'saving',
+                         'is_public': True,
+                         'disk_format': 'vhd',
+                         'container_format': 'ovf',
+                         'name': 'new name! #123',
+                         'size': 19,
+                         'checksum': None}
+
+        glance.registry.db.api.image_create(None, extra_fixture)
+
+        extra_fixture = {'id': 4,
+                         'status': 'saving',
+                         'is_public': True,
+                         'disk_format': 'vhd',
+                         'container_format': 'ovf',
+                         'name': 'new name! #125',
+                         'size': 19,
+                         'checksum': None}
+
+        glance.registry.db.api.image_create(None, extra_fixture)
+
+        images = self.client.get_images(marker=3, limit=1)
+        self.assertEquals(len(images), 1)
+
+        self.assertEquals(images[0]['id'], 2)
+
     def test_get_image_index_by_name(self):
         """Test correct set of public, name-filtered image returned. This
         is just a sanity check, we test the details call more in-depth."""
@@ -108,6 +194,35 @@
         for k, v in fixture.items():
             self.assertEquals(v, images[0][k])
 
+    def test_get_image_details_marker_limit(self):
+        """Test correct set of images returned with marker/limit params."""
+        extra_fixture = {'id': 3,
+                         'status': 'saving',
+                         'is_public': True,
+                         'disk_format': 'vhd',
+                         'container_format': 'ovf',
+                         'name': 'new name! #123',
+                         'size': 19,
+                         'checksum': None}
+
+        glance.registry.db.api.image_create(None, extra_fixture)
+
+        extra_fixture = {'id': 4,
+                         'status': 'saving',
+                         'is_public': True,
+                         'disk_format': 'vhd',
+                         'container_format': 'ovf',
+                         'name': 'new name! #125',
+                         'size': 19,
+                         'checksum': None}
+
+        glance.registry.db.api.image_create(None, extra_fixture)
+
+        images = self.client.get_images_detailed(marker=3, limit=1)
+        self.assertEquals(len(images), 1)
+
+        self.assertEquals(images[0]['id'], 2)
+
     def test_get_image_details_by_name(self):
         """Tests that a detailed call can be filtered by name"""
         extra_fixture = {'id': 3,
@@ -457,6 +572,92 @@
         for k, v in fixture.items():
             self.assertEquals(v, images[0][k])
 
+    def test_get_image_index_marker(self):
+        """Test correct set of public images returned with marker param."""
+        extra_fixture = {'id': 3,
+                         'status': 'saving',
+                         'is_public': True,
+                         'disk_format': 'vhd',
+                         'container_format': 'ovf',
+                         'name': 'new name! #123',
+                         'size': 19,
+                         'checksum': None}
+
+        glance.registry.db.api.image_create(None, extra_fixture)
+
+        extra_fixture = {'id': 4,
+                         'status': 'saving',
+                         'is_public': True,
+                         'disk_format': 'vhd',
+                         'container_format': 'ovf',
+                         'name': 'new name! #125',
+                         'size': 19,
+                         'checksum': None}
+
+        glance.registry.db.api.image_create(None, extra_fixture)
+
+        images = self.client.get_images(marker=4)
+        self.assertEquals(len(images), 2)
+
+        for image in images:
+            self.assertTrue(image['id'] < 4)
+
+    def test_get_image_index_limit(self):
+        """Test correct number of public images returned with limit param."""
+        extra_fixture = {'id': 3,
+                         'status': 'saving',
+                         'is_public': True,
+                         'disk_format': 'vhd',
+                         'container_format': 'ovf',
+                         'name': 'new name! #123',
+                         'size': 19,
+                         'checksum': None}
+
+        glance.registry.db.api.image_create(None, extra_fixture)
+
+        extra_fixture = {'id': 4,
+                         'status': 'saving',
+                         'is_public': True,
+                         'disk_format': 'vhd',
+                         'container_format': 'ovf',
+                         'name': 'new name! #125',
+                         'size': 19,
+                         'checksum': None}
+
+        glance.registry.db.api.image_create(None, extra_fixture)
+
+        images = self.client.get_images(limit=2)
+        self.assertEquals(len(images), 2)
+
+    def test_get_image_index_marker_limit(self):
+        """Test correct set of images returned with marker/limit params."""
+        extra_fixture = {'id': 3,
+                         'status': 'saving',
+                         'is_public': True,
+                         'disk_format': 'vhd',
+                         'container_format': 'ovf',
+                         'name': 'new name! #123',
+                         'size': 19,
+                         'checksum': None}
+
+        glance.registry.db.api.image_create(None, extra_fixture)
+
+        extra_fixture = {'id': 4,
+                         'status': 'saving',
+                         'is_public': True,
+                         'disk_format': 'vhd',
+                         'container_format': 'ovf',
+                         'name': 'new name! #125',
+                         'size': 19,
+                         'checksum': None}
+
+        glance.registry.db.api.image_create(None, extra_fixture)
+
+        images = self.client.get_images(marker=3, limit=1)
+        self.assertEquals(len(images), 1)
+
+        self.assertEquals(images[0]['id'], 2)
+
     def test_get_image_index_by_base_attribute(self):
         """Tests that an index call can be filtered by a base attribute"""
         extra_fixture = {'id': 3,
@@ -522,6 +723,35 @@
         for k, v in expected.items():
             self.assertEquals(v, images[0][k])
 
+    def test_get_image_details_marker_limit(self):
+        """Test detailed calls are filtered by marker/limit params."""
+        extra_fixture = {'id': 3,
+                         'status': 'saving',
+                         'is_public': True,
+                         'disk_format': 'vhd',
+                         'container_format': 'ovf',
+                         'name': 'new name! #123',
+                         'size': 19,
+                         'checksum': None}
+
+        glance.registry.db.api.image_create(None, extra_fixture)
+
+        extra_fixture = {'id': 4,
+                         'status': 'saving',
+                         'is_public': True,
+                         'disk_format': 'vhd',
+                         'container_format': 'ovf',
+                         'name': 'new name! #125',
+                         'size': 19,
+                         'checksum': None}
+
+        glance.registry.db.api.image_create(None, extra_fixture)
+
+        images = self.client.get_images_detailed(marker=3, limit=1)
+        self.assertEquals(len(images), 1)
+
+        self.assertEquals(images[0]['id'], 2)
+
     def test_get_image_details_by_base_attribute(self):
         """Tests that a detailed call can be filtered by a base attribute"""
         extra_fixture = {'id': 3,