Merge "Support pagination in instance_list"

nova/compute/instance_list.py

@@ -10,10 +10,14 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import copy
 import heapq
+import itertools
 
 from nova import context
 from nova import db
+from nova import exception
+from nova import objects
 
 
 class InstanceSortContext(object):
@@ -58,6 +62,29 @@ class InstanceWrapper(object):
         return r == -1
 
 
+def _get_marker_instance(ctx, marker):
+    """Get the marker instance from its cell.
+
+    This returns the marker instance from the cell in which it lives
+    """
+
+    try:
+        im = objects.InstanceMapping.get_by_instance_uuid(ctx, marker)
+    except exception.InstanceMappingNotFound:
+        raise exception.MarkerNotFound(marker=marker)
+
+    elevated = ctx.elevated(read_deleted='yes')
+    with context.target_cell(elevated, im.cell_mapping) as cctx:
+        try:
+            db_inst = db.instance_get_by_uuid(cctx, marker,
+                                              columns_to_join=[])
+        except exception.InstanceNotFound:
+            db_inst = None
+    if not db_inst:
+        raise exception.MarkerNotFound(marker=marker)
+    return db_inst
+
+
 def get_instances_sorted(ctx, filters, limit, marker, columns_to_join,
                          sort_keys, sort_dirs):
     """Get a cross-cell list of instances matching filters.
@@ -78,8 +105,6 @@ def get_instances_sorted(ctx, filters, limit, marker, columns_to_join,
     to each database query, although the limit will be enforced in the
     output of this function. Meaning, we will still query $limit from each
     database, but only return $limit total results.
-
-    FIXME: Make marker work
     """
 
     if not sort_keys:
@@ -92,6 +117,16 @@ def get_instances_sorted(ctx, filters, limit, marker, columns_to_join,
 
     sort_ctx = InstanceSortContext(sort_keys, sort_dirs)
 
+    if marker:
+        # A marker UUID was provided from the API. Call this the 'global'
+        # marker as it determines where we start the process across
+        # all cells. Look up the instance in whatever cell it is in and
+        # record the values for the sort keys so we can find the marker
+        # instance in each cell (called the 'local' marker).
+        global_marker_instance = _get_marker_instance(ctx, marker)
+        global_marker_values = [global_marker_instance[key]
+                                for key in sort_keys]
+
     def do_query(ctx):
         """Generate InstanceWrapper(Instance) objects from a cell.
 
@@ -102,13 +137,65 @@ def get_instances_sorted(ctx, filters, limit, marker, columns_to_join,
         routine.
         """
 
-        return (InstanceWrapper(sort_ctx, inst) for inst in
+        # The local marker is a uuid of an instance in a cell that is found
-                db.instance_get_all_by_filters_sort(
+        # by the special method instance_get_by_sort_filters(). It should
-                    ctx, filters,
+        # be the next instance in order according to the sort provided,
-                    limit=limit, marker=marker,
+        # but after the marker instance which may have been in another cell.
+        local_marker = None
+
+        # Since the regular DB query routines take a marker and assume that
+        # the marked instance was the last entry of the previous page, we
+        # may need to prefix it to our result query if we're not the cell
+        # that had the actual marker instance.
+        local_marker_prefix = []
+
+        if marker:
+            # FIXME(danms): If we knew which cell we were in here, we could
+            # avoid looking up the marker again. But, we don't currently.
+
+            local_marker = db.instance_get_by_sort_filters(
+                ctx, sort_keys, sort_dirs, global_marker_values)
+            if local_marker:
+                if local_marker != marker:
+                    # We did find a marker in our cell, but it wasn't
+                    # the global marker. Thus, we will use it as our
+                    # marker in the main query below, but we also need
+                    # to prefix that result with this marker instance
+                    # since the result below will not return it and it
+                    # has not been returned to the user yet. Note that
+                    # we do _not_ prefix the marker instance if our
+                    # marker was the global one since that has already
+                    # been sent to the user.
+                    local_marker_filters = copy.copy(filters)
+                    if 'uuid' not in local_marker_filters:
+                        # If a uuid filter was provided, it will
+                        # have included our marker already if this instance
+                        # is desired in the output set. If it wasn't, we
+                        # specifically query for it. If the other filters would
+                        # have excluded it, then we'll get an empty set here
+                        # and not include it in the output as expected.
+                        local_marker_filters['uuid'] = [local_marker]
+                    local_marker_prefix = db.instance_get_all_by_filters_sort(
+                        ctx, local_marker_filters, limit=1, marker=None,
                         columns_to_join=columns_to_join,
                         sort_keys=sort_keys,
-                sort_dirs=sort_dirs))
+                        sort_dirs=sort_dirs)
+            else:
+                # There was a global marker but everything in our cell is
+                # _before_ that marker, so we return nothing. If we didn't
+                # have this clause, we'd pass marker=None to the query below
+                # and return a full unpaginated set for our cell.
+                return []
+
+        main_query_result = db.instance_get_all_by_filters_sort(
+            ctx, filters,
+            limit=limit, marker=local_marker,
+            columns_to_join=columns_to_join,
+            sort_keys=sort_keys,
+            sort_dirs=sort_dirs)
+
+        return (InstanceWrapper(sort_ctx, inst) for inst in
+                itertools.chain(local_marker_prefix, main_query_result))
 
     # FIXME(danms): If we raise or timeout on a cell we need to handle
     # that here gracefully. The below routine will provide sentinels

nova/tests/functional/compute/test_instance_list.py

@@ -14,8 +14,11 @@ import datetime
 
 from nova.compute import instance_list
 from nova import context
+from nova import db
+from nova import exception
 from nova import objects
 from nova import test
+from nova.tests import uuidsentinel as uuids
 
 
 class InstanceListTestCase(test.TestCase):
@@ -31,11 +34,11 @@ class InstanceListTestCase(test.TestCase):
         dt = datetime.datetime(1985, 10, 25, 1, 21, 0)
         spread = datetime.timedelta(minutes=10)
 
-        cells = objects.CellMappingList.get_all(self.context)
+        self.cells = objects.CellMappingList.get_all(self.context)
         # Create three instances in each of the real cells. Leave the
         # first cell empty to make sure we don't break with an empty
         # one.
-        for cell in cells[1:]:
+        for cell in self.cells[1:]:
             for i in range(0, self.num_instances):
                 with context.target_cell(self.context, cell) as cctx:
                     inst = objects.Instance(
@@ -127,3 +130,185 @@ class InstanceListTestCase(test.TestCase):
         uuids = [inst['uuid'] for inst in insts]
         self.assertEqual(sorted(uuids), uuids)
         self.assertEqual(len(self.instances), len(uuids))
+
+    def _test_get_sorted_with_limit_marker(self, sort_by, pages=2, pagesize=2):
+        insts = []
+
+        page = 0
+        while True:
+            if page >= pages:
+                limit = None
+            else:
+                limit = pagesize
+            if insts:
+                marker = insts[-1]['uuid']
+            else:
+                marker = None
+            batch = list(
+                instance_list.get_instances_sorted(self.context, {},
+                                                   limit, marker,
+                                                   [], [sort_by], ['asc']))
+            if not batch:
+                break
+            insts.extend(batch)
+            page += 1
+
+        # We should have requested exactly (or one more unlimited) pages
+        self.assertIn(page, (pages, pages + 1))
+
+        # Make sure the full set matches what we know to be true
+        found = [x[sort_by] for x in insts]
+        had = [x[sort_by] for x in self.instances]
+
+        if sort_by == 'launched_at':
+            # We're comparing objects and database entries, so we need to
+            # squash the tzinfo of the object ones so we can compare
+            had = [x.replace(tzinfo=None) for x in had]
+
+        self.assertEqual(sorted(had), found)
+
+    def test_get_sorted_with_limit_marker_stable(self):
+        """Test sorted by hostname.
+
+        This will be a stable sort that won't change on each run.
+        """
+        self._test_get_sorted_with_limit_marker(sort_by='hostname')
+
+    def test_get_sorted_with_limit_marker_stable_different_pages(self):
+        """Test sorted by hostname with different page sizes.
+
+        Just do the above with page seams in different places.
+        """
+        self._test_get_sorted_with_limit_marker(sort_by='hostname',
+                                                pages=3, pagesize=1)
+
+    def test_get_sorted_with_limit_marker_random(self):
+        """Test sorted by uuid.
+
+        This will not be stable and the actual ordering will depend on
+        uuid generation and thus be different on each run. Do this in
+        addition to the stable sort above to keep us honest.
+        """
+        self._test_get_sorted_with_limit_marker(sort_by='uuid')
+
+    def test_get_sorted_with_limit_marker_random_different_pages(self):
+        """Test sorted by uuid with different page sizes.
+
+        Just do the above with page seams in different places.
+        """
+        self._test_get_sorted_with_limit_marker(sort_by='uuid',
+                                                pages=3, pagesize=2)
+
+    def test_get_sorted_with_limit_marker_datetime(self):
+        """Test sorted by launched_at.
+
+        This tests that we can do all of this, but with datetime
+        fields.
+        """
+        self._test_get_sorted_with_limit_marker(sort_by='launched_at')
+
+    def test_get_sorted_with_deleted_marker(self):
+        marker = self.instances[1]['uuid']
+
+        before = list(
+            instance_list.get_instances_sorted(self.context, {},
+                                               None, marker,
+                                               [], None, None))
+
+        db.instance_destroy(self.context, marker)
+
+        after = list(
+            instance_list.get_instances_sorted(self.context, {},
+                                               None, marker,
+                                               [], None, None))
+
+        self.assertEqual(before, after)
+
+    def test_get_sorted_with_invalid_marker(self):
+        self.assertRaises(exception.MarkerNotFound,
+                          list, instance_list.get_instances_sorted(
+                              self.context, {}, None, 'not-a-marker',
+                              [], None, None))
+
+    def test_get_sorted_with_purged_instance(self):
+        """Test that we handle a mapped but purged instance."""
+        im = objects.InstanceMapping(self.context,
+                                     instance_uuid=uuids.missing,
+                                     project_id=self.context.project_id,
+                                     user_id=self.context.user_id,
+                                     cell=self.cells[0])
+        im.create()
+        self.assertRaises(exception.MarkerNotFound,
+                          list, instance_list.get_instances_sorted(
+                              self.context, {}, None, uuids.missing,
+                              [], None, None))
+
+    def _test_get_paginated_with_filter(self, filters):
+
+        found_uuids = []
+        marker = None
+        while True:
+            # Query for those instances, sorted by a different key in
+            # pages of one until we've consumed them all
+            batch = list(
+                instance_list.get_instances_sorted(self.context,
+                                                   filters,
+                                                   1, marker, [],
+                                                   ['hostname'],
+                                                   ['asc']))
+            if not batch:
+                break
+            found_uuids.extend([x['uuid'] for x in batch])
+            marker = found_uuids[-1]
+
+        return found_uuids
+
+    def test_get_paginated_with_uuid_filter(self):
+        """Test getting pages with uuid filters.
+
+        This runs through the results of a uuid-filtered query in pages of
+        length one to ensure that we land on markers that are filtered out
+        of the query and are not accidentally returned.
+        """
+        # Pick a set of the instances by uuid, when sorted by uuid
+        all_uuids = [x['uuid'] for x in self.instances]
+        filters = {'uuid': sorted(all_uuids)[:7]}
+
+        found_uuids = self._test_get_paginated_with_filter(filters)
+
+        # Make sure we found all (and only) the instances we asked for
+        self.assertEqual(set(found_uuids), set(filters['uuid']))
+        self.assertEqual(7, len(found_uuids))
+
+    def test_get_paginated_with_other_filter(self):
+        """Test getting pages with another filter.
+
+        This runs through the results of a filtered query in pages of
+        length one to ensure we land on markers that are filtered out
+        of the query and are not accidentally returned.
+        """
+        expected = [inst['uuid'] for inst in self.instances
+                    if inst['instance_type_id'] == 1]
+        filters = {'instance_type_id': 1}
+
+        found_uuids = self._test_get_paginated_with_filter(filters)
+
+        self.assertEqual(set(expected), set(found_uuids))
+
+    def test_get_paginated_with_uuid_and_other_filter(self):
+        """Test getting pages with a uuid and other type of filter.
+
+        We do this to make sure that we still find (but exclude) the
+        marker even if one of the other filters would have included
+        it.
+        """
+        # Pick a set of the instances by uuid, when sorted by uuid
+        all_uuids = [x['uuid'] for x in self.instances]
+        filters = {'uuid': sorted(all_uuids)[:7],
+                   'user_id': 'fake'}
+
+        found_uuids = self._test_get_paginated_with_filter(filters)
+
+        # Make sure we found all (and only) the instances we asked for
+        self.assertEqual(set(found_uuids), set(filters['uuid']))
+        self.assertEqual(7, len(found_uuids))