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

@@ -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,

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

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

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,

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()

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.