From a98f5c86cdf3bd5eb4615b1b25f9b08aadc450f3 Mon Sep 17 00:00:00 2001 From: Steven Kaufer Date: Wed, 28 May 2014 07:33:58 +0000 Subject: [PATCH] GET servers API sorting compute/instance/DB updates This change is to support updating the v2 and v3 /servers and /servers/detail APIs to support multiple sort keys and sort directions (using the 'sort_key' and 'sort_dir' parameters); these parameters can be specified multiple times to create a list of sort keys and directions. Contains the changes that will be used by the REST API layer: * Compute API updates on the get_all function to support sort keys and directions * InstanceList updates on the get_by_filters function. The new 'sort_keys' and 'sort_dirs' kwargs are added but the existing 'sort_key' and 'sort_dir' kwargs must remain for backward compatibility. * DB API has a new 'instance_get_all_by_filters_sort' function that supports multiple keys/directions. The existing 'instance_get_all_by_filters' remains for backward compatibility and calls the new sort function with the single sort values wrapped in a list. Note that the defaulting of the sort keys and directions is done in the dependent patch set in the new 'process_sort_params' function (invoked in db.sqlalchemy.api); by default, the sort keys are 'created_at' and 'id' in the 'desc' direction. The REST API changes that consume these updated will be pushed in another change set. Change-Id: Iec2bba18c8894302e3ba35af69379ea9c1da04fc Partially implements: blueprint nova-pagination --- nova/api/ec2/cloud.py | 3 +- nova/compute/api.py | 29 ++-- nova/db/api.py | 18 +++ nova/db/sqlalchemy/api.py | 36 +++-- nova/objects/instance.py | 21 ++- nova/tests/unit/db/test_db_api.py | 169 +++++++++++++++++++++++ nova/tests/unit/objects/test_instance.py | 51 +++++++ nova/tests/unit/objects/test_objects.py | 2 +- 8 files changed, 299 insertions(+), 30 deletions(-) diff --git a/nova/api/ec2/cloud.py b/nova/api/ec2/cloud.py index 63d44ba62e3..0b180f86684 100644 --- a/nova/api/ec2/cloud.py +++ b/nova/api/ec2/cloud.py @@ -1173,7 +1173,8 @@ def _format_instances(self, context, instance_id=None, use_v6=False, search_opts['deleted'] = False instances = self.compute_api.get_all(context, search_opts=search_opts, - sort_dir='asc', + sort_keys=['created_at'], + sort_dirs=['asc'], want_objects=True) except exception.NotFound: instances = [] diff --git a/nova/compute/api.py b/nova/compute/api.py index 853866346ae..bc93e391ae5 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -1913,9 +1913,9 @@ def get(self, context, instance_id, want_objects=False, instance = obj_base.obj_to_primitive(instance) return instance - def get_all(self, context, search_opts=None, sort_key='created_at', - sort_dir='desc', limit=None, marker=None, want_objects=False, - expected_attrs=None): + def get_all(self, context, search_opts=None, limit=None, marker=None, + want_objects=False, expected_attrs=None, sort_keys=None, + sort_dirs=None): """Get all instances filtered by one of the given parameters. If there is no filter and the context is an admin, it will retrieve @@ -1924,8 +1924,10 @@ def get_all(self, context, search_opts=None, sort_key='created_at', Deleted instances will be returned by default, unless there is a search option that says otherwise. - The results will be returned sorted in the order specified by the - 'sort_dir' parameter using the key specified in the 'sort_key' + The results will be sorted based on the list of sort keys in the + 'sort_keys' parameter (first value is primary sort key, second value is + seconardy sort ket, etc.). For each sort key, the associated sort + direction is based on the list of sort directions in the 'sort_dirs' parameter. """ @@ -1995,8 +1997,8 @@ def _remap_system_metadata_filter(metadata): return [] inst_models = self._get_instances_by_filters(context, filters, - sort_key, sort_dir, limit=limit, marker=marker, - expected_attrs=expected_attrs) + limit=limit, marker=marker, expected_attrs=expected_attrs, + sort_keys=sort_keys, sort_dirs=sort_dirs) if 'ip6' in filters or 'ip' in filters: inst_models = self._ip_filter(inst_models, filters) @@ -2031,16 +2033,15 @@ def _ip_filter(inst_models, filters): return objects.InstanceList(objects=result_objs) def _get_instances_by_filters(self, context, filters, - sort_key, sort_dir, - limit=None, - marker=None, expected_attrs=None): + limit=None, marker=None, expected_attrs=None, + sort_keys=None, sort_dirs=None): fields = ['metadata', 'system_metadata', 'info_cache', 'security_groups'] if expected_attrs: fields.extend(expected_attrs) return objects.InstanceList.get_by_filters( - context, filters=filters, sort_key=sort_key, sort_dir=sort_dir, - limit=limit, marker=marker, expected_attrs=fields) + context, filters=filters, limit=limit, marker=marker, + expected_attrs=fields, sort_keys=sort_keys, sort_dirs=sort_dirs) # NOTE(melwitt): We don't check instance lock for backup because lock is # intended to prevent accidental change/delete of instances @@ -3089,8 +3090,8 @@ def _filter_metadata(instance, search_filt, input_metadata): formatted_metadata_list = [] instances = self._get_instances_by_filters(context, filters={}, - sort_key='created_at', - sort_dir='desc') + sort_keys=['created_at'], + sort_dirs=['desc']) for instance in instances: try: check_policy(context, 'get_all_instance_%s' % metadata_type, diff --git a/nova/db/api.py b/nova/db/api.py index 08abe9baed9..711dccc9ec5 100644 --- a/nova/db/api.py +++ b/nova/db/api.py @@ -655,6 +655,10 @@ def instance_get_all_by_filters(context, filters, sort_key='created_at', sort_dir='desc', limit=None, marker=None, columns_to_join=None, use_slave=False): """Get all instances that match all filters.""" + # Note: This function exists for backwards compatibility since calls to + # the instance layer coming in over RPC may specify the single sort + # key/direction values; in this case, this function is invoked instead + # of the 'instance_get_all_by_filters_sort' function. return IMPL.instance_get_all_by_filters(context, filters, sort_key, sort_dir, limit=limit, marker=marker, @@ -662,6 +666,20 @@ def instance_get_all_by_filters(context, filters, sort_key='created_at', use_slave=use_slave) +def instance_get_all_by_filters_sort(context, filters, limit=None, + marker=None, columns_to_join=None, + use_slave=False, sort_keys=None, + sort_dirs=None): + """Get all instances that match all filters sorted by multiple keys. + + sort_keys and sort_dirs must be a list of strings. + """ + return IMPL.instance_get_all_by_filters_sort( + context, filters, limit=limit, marker=marker, + columns_to_join=columns_to_join, use_slave=use_slave, + sort_keys=sort_keys, sort_dirs=sort_dirs) + + def instance_get_active_by_window_joined(context, begin, end=None, project_id=None, host=None, use_slave=False, diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py index b3209c97195..e9fbe94c8ba 100644 --- a/nova/db/sqlalchemy/api.py +++ b/nova/db/sqlalchemy/api.py @@ -1850,9 +1850,27 @@ def instance_get_all(context, columns_to_join=None): def instance_get_all_by_filters(context, filters, sort_key, sort_dir, limit=None, marker=None, columns_to_join=None, use_slave=False): - """Return instances that match all filters. Deleted instances - will be returned by default, unless there's a filter that says - otherwise. + """Return instances matching all filters sorted by the primary key. + + See instance_get_all_by_filters_sort for more information. + """ + # Invoke the API with the multiple sort keys and directions using the + # single sort key/direction + return instance_get_all_by_filters_sort(context, filters, limit=limit, + marker=marker, + columns_to_join=columns_to_join, + use_slave=use_slave, + sort_keys=[sort_key], + sort_dirs=[sort_dir]) + + +@require_context +def instance_get_all_by_filters_sort(context, filters, limit=None, marker=None, + columns_to_join=None, use_slave=False, + sort_keys=None, sort_dirs=None): + """Return instances that match all filters sorted the the given keys. + Deleted instances will be returned by default, unless there's a filter that + says otherwise. Depending on the name of a filter, matching for that filter is performed using either exact matching or as regular expression @@ -1889,7 +1907,9 @@ def instance_get_all_by_filters(context, filters, sort_key, sort_dir, if limit == 0: return [] - sort_fn = {'desc': desc, 'asc': asc} + sort_keys, sort_dirs = process_sort_params(sort_keys, + sort_dirs, + default_dir='desc') if CONF.database.slave_connection == '': use_slave = False @@ -1906,8 +1926,8 @@ def instance_get_all_by_filters(context, filters, sort_key, sort_dir, for column in columns_to_join: query_prefix = query_prefix.options(joinedload(column)) - query_prefix = query_prefix.order_by(sort_fn[sort_dir]( - getattr(models.Instance, sort_key))) + # Note: order_by is done in the sqlalchemy.utils.py paginate_query(), + # no need to do it here as well # Make a copy of the filters dictionary to use going forward, as we'll # be modifying it and we shouldn't affect the caller's use of it. @@ -1982,9 +2002,9 @@ def instance_get_all_by_filters(context, filters, sort_key, sort_dir, raise exception.MarkerNotFound(marker) query_prefix = sqlalchemyutils.paginate_query(query_prefix, models.Instance, limit, - [sort_key, 'created_at', 'id'], + sort_keys, marker=marker, - sort_dir=sort_dir) + sort_dirs=sort_dirs) return _instances_fill_metadata(context, query_prefix.all(), manual_joins) diff --git a/nova/objects/instance.py b/nova/objects/instance.py index 533b8ea186c..e6a7ffb0236 100644 --- a/nova/objects/instance.py +++ b/nova/objects/instance.py @@ -681,7 +681,8 @@ class InstanceList(base.ObjectListBase, base.NovaObject): # Version 1.8: Instance <= version 1.14 # Version 1.9: Instance <= version 1.15 # Version 1.10: Instance <= version 1.16 - VERSION = '1.10' + # Version 1.11: Added sort_keys and sort_dirs to get_by_filters + VERSION = '1.11' fields = { 'objects': fields.ListOfObjectsField('Instance'), @@ -698,16 +699,24 @@ class InstanceList(base.ObjectListBase, base.NovaObject): '1.8': '1.14', '1.9': '1.15', '1.10': '1.16', + '1.11': '1.16', } @base.remotable_classmethod def get_by_filters(cls, context, filters, sort_key='created_at', sort_dir='desc', limit=None, - marker=None, expected_attrs=None, use_slave=False): - db_inst_list = db.instance_get_all_by_filters( - context, filters, sort_key, sort_dir, limit=limit, marker=marker, - columns_to_join=_expected_cols(expected_attrs), - use_slave=use_slave) + marker=None, expected_attrs=None, use_slave=False, + sort_keys=None, sort_dirs=None): + if sort_keys or sort_dirs: + db_inst_list = db.instance_get_all_by_filters_sort( + context, filters, limit=limit, marker=marker, + columns_to_join=_expected_cols(expected_attrs), + use_slave=use_slave, sort_keys=sort_keys, sort_dirs=sort_dirs) + else: + db_inst_list = db.instance_get_all_by_filters( + context, filters, sort_key, sort_dir, limit=limit, + marker=marker, columns_to_join=_expected_cols(expected_attrs), + use_slave=use_slave) return _make_instance_list(context, cls(), db_inst_list, expected_attrs) diff --git a/nova/tests/unit/db/test_db_api.py b/nova/tests/unit/db/test_db_api.py index f103dd49ce5..fc7f624426b 100644 --- a/nova/tests/unit/db/test_db_api.py +++ b/nova/tests/unit/db/test_db_api.py @@ -251,6 +251,161 @@ def test_instance_get_all_by_filters_paginate(self): self.context, {'display_name': '%test%'}, marker=str(stdlib_uuid.uuid4())) + def _assert_equals_inst_order(self, correct_order, filters, + sort_keys=None, sort_dirs=None, + limit=None, marker=None, + match_keys=['uuid', 'vm_state', + 'display_name', 'id']): + '''Retrieves instances based on the given filters and sorting + information and verifies that the instances are returned in the + correct sorted order by ensuring that the supplied keys match. + ''' + result = db.instance_get_all_by_filters_sort( + self.context, filters, limit=limit, marker=marker, + sort_keys=sort_keys, sort_dirs=sort_dirs) + self.assertEqual(len(correct_order), len(result)) + for inst1, inst2 in zip(result, correct_order): + for key in match_keys: + self.assertEqual(inst1.get(key), inst2.get(key)) + return result + + def test_instance_get_all_by_filters_sort_keys(self): + '''Verifies sort order and direction for multiple instances.''' + # Instances that will reply to the query + test1_active = self.create_instance_with_args( + display_name='test1', + vm_state=vm_states.ACTIVE) + test1_error = self.create_instance_with_args( + display_name='test1', + vm_state=vm_states.ERROR) + test1_error2 = self.create_instance_with_args( + display_name='test1', + vm_state=vm_states.ERROR) + test2_active = self.create_instance_with_args( + display_name='test2', + vm_state=vm_states.ACTIVE) + test2_error = self.create_instance_with_args( + display_name='test2', + vm_state=vm_states.ERROR) + test2_error2 = self.create_instance_with_args( + display_name='test2', + vm_state=vm_states.ERROR) + # Other instances in the DB, will not match name filter + other_error = self.create_instance_with_args( + display_name='other', + vm_state=vm_states.ERROR) + other_active = self.create_instance_with_args( + display_name='other', + vm_state=vm_states.ACTIVE) + filters = {'display_name': '%test%'} + + # Verify different sort key/direction combinations + sort_keys = ['display_name', 'vm_state', 'created_at'] + sort_dirs = ['asc', 'asc', 'asc'] + correct_order = [test1_active, test1_error, test1_error2, + test2_active, test2_error, test2_error2] + self._assert_equals_inst_order(correct_order, filters, + sort_keys=sort_keys, + sort_dirs=sort_dirs) + + sort_dirs = ['asc', 'desc', 'asc'] + correct_order = [test1_error, test1_error2, test1_active, + test2_error, test2_error2, test2_active] + self._assert_equals_inst_order(correct_order, filters, + sort_keys=sort_keys, + sort_dirs=sort_dirs) + + sort_dirs = ['desc', 'desc', 'asc'] + correct_order = [test2_error, test2_error2, test2_active, + test1_error, test1_error2, test1_active] + self._assert_equals_inst_order(correct_order, filters, + sort_keys=sort_keys, + sort_dirs=sort_dirs) + + # created_at is added by default if not supplied, descending order + sort_keys = ['display_name', 'vm_state'] + sort_dirs = ['desc', 'desc'] + correct_order = [test2_error2, test2_error, test2_active, + test1_error2, test1_error, test1_active] + self._assert_equals_inst_order(correct_order, filters, + sort_keys=sort_keys, + sort_dirs=sort_dirs) + + # Now created_at should be in ascending order (defaults to the first + # sort dir direction) + sort_dirs = ['asc', 'asc'] + correct_order = [test1_active, test1_error, test1_error2, + test2_active, test2_error, test2_error2] + self._assert_equals_inst_order(correct_order, filters, + sort_keys=sort_keys, + sort_dirs=sort_dirs) + + # Remove name filter, get all instances + correct_order = [other_active, other_error, + test1_active, test1_error, test1_error2, + test2_active, test2_error, test2_error2] + self._assert_equals_inst_order(correct_order, {}, + sort_keys=sort_keys, + sort_dirs=sort_dirs) + + # Default sorting, 'created_at' then 'id' in desc order + correct_order = [other_active, other_error, + test2_error2, test2_error, test2_active, + test1_error2, test1_error, test1_active] + self._assert_equals_inst_order(correct_order, {}) + + def test_instance_get_all_by_filters_sort_keys_paginate(self): + '''Verifies sort order with pagination.''' + # Instances that will reply to the query + test1_active = self.create_instance_with_args( + display_name='test1', + vm_state=vm_states.ACTIVE) + test1_error = self.create_instance_with_args( + display_name='test1', + vm_state=vm_states.ERROR) + test1_error2 = self.create_instance_with_args( + display_name='test1', + vm_state=vm_states.ERROR) + test2_active = self.create_instance_with_args( + display_name='test2', + vm_state=vm_states.ACTIVE) + test2_error = self.create_instance_with_args( + display_name='test2', + vm_state=vm_states.ERROR) + test2_error2 = self.create_instance_with_args( + display_name='test2', + vm_state=vm_states.ERROR) + # Other instances in the DB, will not match name filter + self.create_instance_with_args(display_name='other') + self.create_instance_with_args(display_name='other') + filters = {'display_name': '%test%'} + # Common sort information for every query + sort_keys = ['display_name', 'vm_state', 'created_at'] + sort_dirs = ['asc', 'desc', 'asc'] + # Overall correct instance order based on the sort keys + correct_order = [test1_error, test1_error2, test1_active, + test2_error, test2_error2, test2_active] + + # Limits of 1, 2, and 3, verify that the instances returned are in the + # correct sorted order, update the marker to get the next correct page + for limit in range(1, 4): + marker = None + # Include the maximum number of instances (ie, 6) to ensure that + # the last query (with marker pointing to the last instance) + # returns 0 servers + for i in range(0, 7, limit): + if i == len(correct_order): + correct = [] + else: + correct = correct_order[i:i + limit] + insts = self._assert_equals_inst_order( + correct, filters, + sort_keys=sort_keys, sort_dirs=sort_dirs, + limit=limit, marker=marker) + if correct: + marker = insts[-1]['uuid'] + self.assertEqual(correct[-1]['uuid'], marker) + def test_convert_objects_related_datetimes(self): t1 = timeutils.utcnow() @@ -787,6 +942,20 @@ def test_instance_get_active_by_window_joined(self): self.assertIn('info_cache', result[0]) self.assertEqual(network_info, result[0]['info_cache']['network_info']) + @mock.patch('nova.db.sqlalchemy.api.instance_get_all_by_filters_sort') + def test_instance_get_all_by_filters_calls_sort(self, + mock_get_all_filters_sort): + '''Verifies instance_get_all_by_filters calls the sort function.''' + # sort parameters should be wrapped in a list, all other parameters + # should be passed through + ctxt = context.get_admin_context() + sqlalchemy_api.instance_get_all_by_filters(ctxt, {'foo': 'bar'}, + 'sort_key', 'sort_dir', limit=100, marker='uuid', + columns_to_join='columns', use_slave=True) + mock_get_all_filters_sort.assert_called_once_with(ctxt, {'foo': 'bar'}, + limit=100, marker='uuid', columns_to_join='columns', + use_slave=True, sort_keys=['sort_key'], sort_dirs=['sort_dir']) + class ProcessSortParamTestCase(test.TestCase): diff --git a/nova/tests/unit/objects/test_instance.py b/nova/tests/unit/objects/test_instance.py index b24fd0143db..0df29adcbf9 100644 --- a/nova/tests/unit/objects/test_instance.py +++ b/nova/tests/unit/objects/test_instance.py @@ -991,6 +991,57 @@ def test_get_all_by_filters(self): self.assertEqual(inst_list.objects[i].uuid, fakes[i]['uuid']) self.assertRemotes() + def test_get_all_by_filters_sorted(self): + fakes = [self.fake_instance(1), self.fake_instance(2)] + self.mox.StubOutWithMock(db, 'instance_get_all_by_filters_sort') + db.instance_get_all_by_filters_sort(self.context, {'foo': 'bar'}, + limit=None, marker=None, + columns_to_join=['metadata'], + use_slave=False, + sort_keys=['uuid'], + sort_dirs=['asc']).AndReturn(fakes) + self.mox.ReplayAll() + inst_list = instance.InstanceList.get_by_filters( + self.context, {'foo': 'bar'}, expected_attrs=['metadata'], + use_slave=False, sort_keys=['uuid'], sort_dirs=['asc']) + + for i in range(0, len(fakes)): + self.assertIsInstance(inst_list.objects[i], instance.Instance) + self.assertEqual(inst_list.objects[i].uuid, fakes[i]['uuid']) + self.assertRemotes() + + @mock.patch.object(db, 'instance_get_all_by_filters_sort') + @mock.patch.object(db, 'instance_get_all_by_filters') + def test_get_all_by_filters_calls_non_sort(self, + mock_get_by_filters, + mock_get_by_filters_sort): + '''Verifies InstanceList.get_by_filters calls correct DB function.''' + # Single sort key/direction is set, call non-sorted DB function + instance.InstanceList.get_by_filters( + self.context, {'foo': 'bar'}, sort_key='key', sort_dir='dir', + limit=100, marker='uuid', use_slave=True) + mock_get_by_filters.assert_called_once_with( + self.context, {'foo': 'bar'}, 'key', 'dir', limit=100, + marker='uuid', columns_to_join=None, use_slave=True) + self.assertEqual(0, mock_get_by_filters_sort.call_count) + + @mock.patch.object(db, 'instance_get_all_by_filters_sort') + @mock.patch.object(db, 'instance_get_all_by_filters') + def test_get_all_by_filters_calls_sort(self, + mock_get_by_filters, + mock_get_by_filters_sort): + '''Verifies InstanceList.get_by_filters calls correct DB function.''' + # Multiple sort keys/directions are set, call sorted DB function + instance.InstanceList.get_by_filters( + self.context, {'foo': 'bar'}, limit=100, marker='uuid', + use_slave=True, sort_keys=['key1', 'key2'], + sort_dirs=['dir1', 'dir2']) + mock_get_by_filters_sort.assert_called_once_with( + self.context, {'foo': 'bar'}, limit=100, + marker='uuid', columns_to_join=None, use_slave=True, + sort_keys=['key1', 'key2'], sort_dirs=['dir1', 'dir2']) + self.assertEqual(0, mock_get_by_filters.call_count) + def test_get_all_by_filters_works_for_cleaned(self): fakes = [self.fake_instance(1), self.fake_instance(2, updates={'deleted': 2, diff --git a/nova/tests/unit/objects/test_objects.py b/nova/tests/unit/objects/test_objects.py index f7eb53808b4..a39a53db54b 100644 --- a/nova/tests/unit/objects/test_objects.py +++ b/nova/tests/unit/objects/test_objects.py @@ -960,7 +960,7 @@ def test_object_serialization_iterables(self): 'InstanceGroup': '1.9-95ece99f092e8f4f88327cdbb44162c9', 'InstanceGroupList': '1.6-c6b78f3c9d9080d33c08667e80589817', 'InstanceInfoCache': '1.5-ef64b604498bfa505a8c93747a9d8b2f', - 'InstanceList': '1.10-03dd7839cd11cff75c3661c9e4227900', + 'InstanceList': '1.11-bb0b3a74a1c82791330247a963a7d6a9', 'InstanceNUMACell': '1.1-8d2a13c8360cc9ea1b68c9c6c4476857', 'InstanceNUMATopology': '1.1-86b95d263c4c68411d44c6741b8d2bb0', 'InstancePCIRequest': '1.1-e082d174f4643e5756ba098c47c1510f',