From eb3da9f5a1c212ed36c0594c91f24f3a18cec8f0 Mon Sep 17 00:00:00 2001 From: Dan Smith Date: Thu, 10 May 2018 09:37:57 -0700 Subject: [PATCH] Address feedback from instance_list smart-cell behavior This addresses minor feedback from the preceding patch to enable smart instance listing behavior across cells. Change-Id: I90d5f4bbe18913749f3aa4d209731be4a7ba27d9 --- nova/compute/instance_list.py | 8 +++++--- nova/compute/multi_cell_list.py | 9 +++++---- nova/conf/api.py | 7 ++++--- nova/tests/functional/db/test_cell_mapping.py | 3 ++- nova/tests/unit/compute/test_instance_list.py | 4 ++-- ...t-limit-to-cells-config-f72701ac68444e95.yaml | 16 ++++++++++++++++ 6 files changed, 34 insertions(+), 13 deletions(-) create mode 100644 releasenotes/notes/instance-list-limit-to-cells-config-f72701ac68444e95.yaml diff --git a/nova/compute/instance_list.py b/nova/compute/instance_list.py index a6e59f8b8f1f..05c8322a38b6 100644 --- a/nova/compute/instance_list.py +++ b/nova/compute/instance_list.py @@ -100,13 +100,15 @@ def get_instance_objects_sorted(ctx, filters, limit, marker, expected_attrs, """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. + # where if we're not admin we're restricted to our context's project if query_cell_subset and not ctx.is_admin: + # We are not admin, and configured to only query the subset of cells + # we could possibly have instances in. cell_mappings = objects.CellMappingList.get_by_project_id( ctx, ctx.project_id) else: - # If we're admin then query all cells + # Either we are admin, or configured to always hit all cells, + # so don't limit the list to a subset. cell_mappings = None columns_to_join = instance_obj._expected_cols(expected_attrs) instance_generator = get_instances_sorted(ctx, filters, limit, marker, diff --git a/nova/compute/multi_cell_list.py b/nova/compute/multi_cell_list.py index 96be8658c627..780881e11cb3 100644 --- a/nova/compute/multi_cell_list.py +++ b/nova/compute/multi_cell_list.py @@ -67,10 +67,11 @@ class CrossCellLister(object): """An implementation of a cross-cell efficient lister. This primarily provides a listing implementation for fetching - records from across all cells, paginated and sorted appropriately. - The external interface is the get_records_sorted() method. You should - implement this if you need to efficiently list your data type from - cell databases. + records from across multiple cells, paginated and sorted + appropriately. The external interface is the get_records_sorted() + method. You should implement this if you need to efficiently list + your data type from cell databases. + """ def __init__(self, sort_ctx, cells=None): self.sort_ctx = sort_ctx diff --git a/nova/conf/api.py b/nova/conf/api.py index 6b4098487f4e..d2662ccb9f6c 100644 --- a/nova/conf/api.py +++ b/nova/conf/api.py @@ -255,8 +255,8 @@ Possible values: * Any string, including an empty string (the default). """), cfg.BoolOpt("instance_list_per_project_cells", - default=False, - help=""" + 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 @@ -264,7 +264,8 @@ in which the tenant has mapped instances. This requires an additional 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. """), +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/functional/db/test_cell_mapping.py b/nova/tests/functional/db/test_cell_mapping.py index 835f2f1e2a82..68ec0d765112 100644 --- a/nova/tests/functional/db/test_cell_mapping.py +++ b/nova/tests/functional/db/test_cell_mapping.py @@ -146,7 +146,8 @@ class CellMappingListTestCase(test.NoDBTestCase): project_id='proj2') im.create() - # One mapping has no cell because it's a BuildRequest + # One mapping has no cell. This helps ensure that our query + # filters out any mappings that aren't tied to a cell. im = objects.InstanceMapping(context=ctxt, instance_uuid=uuidutils.generate_uuid(), cell_mapping=None, diff --git a/nova/tests/unit/compute/test_instance_list.py b/nova/tests/unit/compute/test_instance_list.py index d93cbee44682..4ab2b2976429 100644 --- a/nova/tests/unit/compute/test_instance_list.py +++ b/nova/tests/unit/compute/test_instance_list.py @@ -106,7 +106,7 @@ class TestInstanceList(test.NoDBTestCase): mock_gi.assert_called_once_with(admin_context, {}, None, None, [], None, None, cell_mappings=None) - self.assertFalse(mock_cm.called) + mock_cm.assert_not_called() @mock.patch('nova.objects.BuildRequestList.get_by_filters') @mock.patch('nova.compute.instance_list.get_instances_sorted') @@ -136,4 +136,4 @@ class TestInstanceList(test.NoDBTestCase): mock_gi.assert_called_once_with(admin_context, {}, None, None, [], None, None, cell_mappings=None) - self.assertFalse(mock_cm.called) + mock_cm.assert_not_called() diff --git a/releasenotes/notes/instance-list-limit-to-cells-config-f72701ac68444e95.yaml b/releasenotes/notes/instance-list-limit-to-cells-config-f72701ac68444e95.yaml new file mode 100644 index 000000000000..a7661bdffc08 --- /dev/null +++ b/releasenotes/notes/instance-list-limit-to-cells-config-f72701ac68444e95.yaml @@ -0,0 +1,16 @@ +--- +other: + - | + The `[api]/instance_list_per_project_cells` configuration option + was added, which controls whether or not an instance list for + non-admin users checks all cell databases for results. If + disabled (the default), then a list will always contact each cell + database looking for instances. This is appropriate if you have a + small number of cells, and/or if you spread instances from tenants + evenly across cells. If you confine tenants to a subset of cells, + then enabling this will result in fewer cell database calls, as + nova will only query the cells for which the tenant has instances + mapped. Doing this requires one more (fast) call to the API + database to get the relevant subset of cells, so if that is likely + to always be the same, disabling this feature will provide better + performance.