From 1f950e2c1e6a107e2231722228eb755ac50f4983 Mon Sep 17 00:00:00 2001 From: Dan Smith Date: Mon, 18 Sep 2017 16:26:17 -0700 Subject: [PATCH] Support pagination in instance_list This adds pagination support to the efficient cross-cell listing routines. Change-Id: I3d6f6d88f562b469391723d8ab4c38eede1e9755 --- nova/compute/instance_list.py | 103 +++++++++- .../functional/compute/test_instance_list.py | 189 +++++++++++++++++- 2 files changed, 282 insertions(+), 10 deletions(-) diff --git a/nova/compute/instance_list.py b/nova/compute/instance_list.py index be3726c323..2dc359d48a 100644 --- a/nova/compute/instance_list.py +++ b/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. """ + # The local marker is a uuid of an instance in a cell that is found + # by the special method instance_get_by_sort_filters(). It should + # be the next instance in order according to the sort provided, + # 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) + 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 - db.instance_get_all_by_filters_sort( - ctx, filters, - limit=limit, marker=marker, - columns_to_join=columns_to_join, - sort_keys=sort_keys, - sort_dirs=sort_dirs)) + 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 diff --git a/nova/tests/functional/compute/test_instance_list.py b/nova/tests/functional/compute/test_instance_list.py index 85126be1ce..11ebb53b71 100644 --- a/nova/tests/functional/compute/test_instance_list.py +++ b/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))