From 913a99c79456abbfef1642e6dc70e6a4ff4e25db Mon Sep 17 00:00:00 2001 From: Dan Smith Date: Mon, 2 Oct 2017 12:48:05 -0700 Subject: [PATCH] Make get_instance_objects_sorted() be smart about cells This makes the get_instance_objects_sorted() method only query for instances in cells that the tenant actually has instances landed in. We do this by getting a list of cell mappings that have instance mappings owned by our project and limiting the scatter/gather operation to just those cells. Per the behavior in db/sqlalchemy/api, we always list all cells for admins. Change-Id: I442621e2b4acd63d2cfc8a66ab5b32b64ebcaea0 --- nova/compute/instance_list.py | 27 +++++++-- nova/compute/multi_cell_list.py | 9 ++- nova/conf/api.py | 11 ++++ nova/tests/fixtures.py | 3 + .../functional/compute/test_instance_list.py | 17 ++++++ nova/tests/unit/api/openstack/fakes.py | 3 + nova/tests/unit/compute/test_instance_list.py | 60 +++++++++++++++++++ 7 files changed, 123 insertions(+), 7 deletions(-) diff --git a/nova/compute/instance_list.py b/nova/compute/instance_list.py index 313d75de47..a6e59f8b8f 100644 --- a/nova/compute/instance_list.py +++ b/nova/compute/instance_list.py @@ -13,6 +13,7 @@ import copy from nova.compute import multi_cell_list +import nova.conf from nova import context from nova import db from nova import exception @@ -20,6 +21,9 @@ from nova import objects from nova.objects import instance as instance_obj +CONF = nova.conf.CONF + + class InstanceSortContext(multi_cell_list.RecordSortContext): def __init__(self, sort_keys, sort_dirs): if not sort_keys: @@ -41,9 +45,9 @@ class InstanceSortContext(multi_cell_list.RecordSortContext): class InstanceLister(multi_cell_list.CrossCellLister): - def __init__(self, sort_keys, sort_dirs): + def __init__(self, sort_keys, sort_dirs, cells=None): super(InstanceLister, self).__init__( - InstanceSortContext(sort_keys, sort_dirs)) + InstanceSortContext(sort_keys, sort_dirs), cells=cells) @property def marker_identifier(self): @@ -85,18 +89,31 @@ class InstanceLister(multi_cell_list.CrossCellLister): # NOTE(danms): These methods are here for legacy glue reasons. We should not # replicate these for every data type we implement. def get_instances_sorted(ctx, filters, limit, marker, columns_to_join, - sort_keys, sort_dirs): - return InstanceLister(sort_keys, sort_dirs).get_records_sorted( + sort_keys, sort_dirs, cell_mappings=None): + return InstanceLister(sort_keys, sort_dirs, + cells=cell_mappings).get_records_sorted( ctx, filters, limit, marker, columns_to_join=columns_to_join) def get_instance_objects_sorted(ctx, filters, limit, marker, expected_attrs, sort_keys, sort_dirs): """Same as above, but return an InstanceList.""" + query_cell_subset = CONF.api.instance_list_per_project_cells + # NOTE(danms): Replicated in part from instance_get_all_by_sort_filters(), + # where if we're not admin we're restricted to our context's project. Use + # this to get a subset of cell mappings. + if query_cell_subset and not ctx.is_admin: + cell_mappings = objects.CellMappingList.get_by_project_id( + ctx, ctx.project_id) + else: + # If we're admin then query all cells + cell_mappings = None columns_to_join = instance_obj._expected_cols(expected_attrs) instance_generator = get_instances_sorted(ctx, filters, limit, marker, columns_to_join, sort_keys, - sort_dirs) + sort_dirs, + cell_mappings=cell_mappings) + if 'fault' in expected_attrs: # We join fault above, so we need to make sure we don't ask # make_instance_list to do it again for us diff --git a/nova/compute/multi_cell_list.py b/nova/compute/multi_cell_list.py index 6af21d1134..96be8658c6 100644 --- a/nova/compute/multi_cell_list.py +++ b/nova/compute/multi_cell_list.py @@ -72,8 +72,9 @@ class CrossCellLister(object): implement this if you need to efficiently list your data type from cell databases. """ - def __init__(self, sort_ctx): + def __init__(self, sort_ctx, cells=None): self.sort_ctx = sort_ctx + self.cells = cells @property @abc.abstractmethod @@ -245,7 +246,11 @@ class CrossCellLister(object): # that here gracefully. The below routine will provide sentinels # to indicate that, which will crash the merge below, but we don't # handle this anywhere yet anyway. - results = context.scatter_gather_all_cells(ctx, do_query) + if self.cells: + results = context.scatter_gather_cells(ctx, self.cells, 60, + do_query) + else: + results = context.scatter_gather_all_cells(ctx, do_query) # If a limit was provided, it was passed to the per-cell query # routines. That means we have NUM_CELLS * limit items across diff --git a/nova/conf/api.py b/nova/conf/api.py index 04bd0ae01c..6b4098487f 100644 --- a/nova/conf/api.py +++ b/nova/conf/api.py @@ -254,6 +254,17 @@ Possible values: * Any string, including an empty string (the default). """), + cfg.BoolOpt("instance_list_per_project_cells", + default=False, + help=""" +When enabled, this will cause the API to only query cell databases +in which the tenant has mapped instances. This requires an additional +(fast) query in the API database before each list, but also +(potentially) limits the number of cell databases that must be queried +to provide the result. If you have a small number of cells, or tenants +are likely to have instances in all cells, then this should be +False. If you have many cells, especially if you confine tenants to a +small subset of those cells, this should be True. """), ] # NOTE(edleafe): I would like to import the value directly from diff --git a/nova/tests/fixtures.py b/nova/tests/fixtures.py index bc88558cad..de0472b38a 100644 --- a/nova/tests/fixtures.py +++ b/nova/tests/fixtures.py @@ -291,6 +291,9 @@ class SingleCellSimple(fixtures.Fixture): self.useFixture(fixtures.MonkeyPatch( 'nova.objects.CellMappingList._get_all_from_db', self._fake_cell_list)) + self.useFixture(fixtures.MonkeyPatch( + 'nova.objects.CellMappingList._get_by_project_id_from_db', + self._fake_cell_list)) self.useFixture(fixtures.MonkeyPatch( 'nova.objects.CellMapping._get_by_uuid_from_db', self._fake_cell_get)) diff --git a/nova/tests/functional/compute/test_instance_list.py b/nova/tests/functional/compute/test_instance_list.py index 01ffe879c3..6e711b64d3 100644 --- a/nova/tests/functional/compute/test_instance_list.py +++ b/nova/tests/functional/compute/test_instance_list.py @@ -427,6 +427,23 @@ class InstanceListTestCase(test.TestCase): faults = [inst['fault'] for inst in insts] self.assertEqual(expected_no_fault, faults.count(None)) + def test_instance_list_minimal_cells(self): + """Get a list of instances with a subset of cell mappings.""" + last_cell = self.cells[-1] + with context.target_cell(self.context, last_cell) as cctxt: + last_cell_instances = db.instance_get_all(cctxt) + last_cell_uuids = [inst['uuid'] for inst in last_cell_instances] + + instances = list( + instance_list.get_instances_sorted(self.context, {}, + None, None, [], + ['uuid'], ['asc'], + cell_mappings=self.cells[:-1])) + found_uuids = [inst['hostname'] for inst in instances] + had_uuids = [inst['hostname'] for inst in self.instances + if inst['uuid'] not in last_cell_uuids] + self.assertEqual(sorted(had_uuids), sorted(found_uuids)) + class TestInstanceListObjects(test.TestCase): def setUp(self): diff --git a/nova/tests/unit/api/openstack/fakes.py b/nova/tests/unit/api/openstack/fakes.py index 21dca72492..0167075afb 100644 --- a/nova/tests/unit/api/openstack/fakes.py +++ b/nova/tests/unit/api/openstack/fakes.py @@ -373,6 +373,9 @@ def fake_instance_get_all_by_filters(num_servers=5, **kwargs): if 'sort_dirs' in kwargs: kwargs.pop('sort_dirs') + if 'cell_mappings' in kwargs: + kwargs.pop('cell_mappings') + for i in range(num_servers): uuid = get_fake_uuid(i) server = stub_instance(id=i + 1, uuid=uuid, diff --git a/nova/tests/unit/compute/test_instance_list.py b/nova/tests/unit/compute/test_instance_list.py index 9fd70426e5..d93cbee446 100644 --- a/nova/tests/unit/compute/test_instance_list.py +++ b/nova/tests/unit/compute/test_instance_list.py @@ -13,6 +13,7 @@ import mock from nova.compute import instance_list +from nova import context as nova_context from nova import objects from nova import test from nova.tests import fixtures @@ -77,3 +78,62 @@ class TestInstanceList(test.NoDBTestCase): insts_two = [inst['hostname'] for inst in insts] self.assertEqual(insts_one, insts_two) + + @mock.patch('nova.objects.BuildRequestList.get_by_filters') + @mock.patch('nova.compute.instance_list.get_instances_sorted') + @mock.patch('nova.objects.CellMappingList.get_by_project_id') + def test_user_gets_subset_of_cells(self, mock_cm, mock_gi, mock_br): + self.flags(instance_list_per_project_cells=True, group='api') + mock_gi.return_value = [] + mock_br.return_value = [] + user_context = nova_context.RequestContext('fake', 'fake') + instance_list.get_instance_objects_sorted( + user_context, {}, None, None, [], None, None) + mock_gi.assert_called_once_with(user_context, {}, None, None, [], + None, None, + cell_mappings=mock_cm.return_value) + + @mock.patch('nova.objects.BuildRequestList.get_by_filters') + @mock.patch('nova.compute.instance_list.get_instances_sorted') + @mock.patch('nova.objects.CellMappingList.get_by_project_id') + def test_admin_gets_all_cells(self, mock_cm, mock_gi, mock_br): + mock_gi.return_value = [] + mock_br.return_value = [] + admin_context = nova_context.RequestContext('fake', 'fake', + is_admin=True) + instance_list.get_instance_objects_sorted( + admin_context, {}, None, None, [], None, None) + mock_gi.assert_called_once_with(admin_context, {}, None, None, [], + None, None, + cell_mappings=None) + self.assertFalse(mock_cm.called) + + @mock.patch('nova.objects.BuildRequestList.get_by_filters') + @mock.patch('nova.compute.instance_list.get_instances_sorted') + @mock.patch('nova.objects.CellMappingList.get_by_project_id') + def test_user_gets_all_cells(self, mock_cm, mock_gi, mock_br): + self.flags(instance_list_per_project_cells=False, group='api') + mock_gi.return_value = [] + mock_br.return_value = [] + user_context = nova_context.RequestContext('fake', 'fake') + instance_list.get_instance_objects_sorted( + user_context, {}, None, None, [], None, None) + mock_gi.assert_called_once_with(user_context, {}, None, None, [], + None, None, + cell_mappings=None) + + @mock.patch('nova.objects.BuildRequestList.get_by_filters') + @mock.patch('nova.compute.instance_list.get_instances_sorted') + @mock.patch('nova.objects.CellMappingList.get_by_project_id') + def test_admin_gets_all_cells_anyway(self, mock_cm, mock_gi, mock_br): + self.flags(instance_list_per_project_cells=True, group='api') + mock_gi.return_value = [] + mock_br.return_value = [] + admin_context = nova_context.RequestContext('fake', 'fake', + is_admin=True) + instance_list.get_instance_objects_sorted( + admin_context, {}, None, None, [], None, None) + mock_gi.assert_called_once_with(admin_context, {}, None, None, [], + None, None, + cell_mappings=None) + self.assertFalse(mock_cm.called)