api: Pre-query not deleted members in server groups

When retrieving multiple - or all - server groups, the code tries to
find not deleted members for each server group in every cell
individually. This is highly inefficient, which is especially noticable
when the number of server groups rises.

We change this to query all members of all server-groups we will reply
with (i.e. from the already limited list) in advance and pass this set
of existing uuids into the function formatting the server group. This is
more efficient, because we only do one large query instead of up to 1000
times the number of cells.

Change-Id: I3459ce7a8bec9a9e6f3a3b496a3e441078b86af0
Signed-off-by: Johannes Kulik <johannes.kulik@sap.com>
Partial-Bug: #2122109

nova/api/openstack/compute/server_groups.py

@@ -16,6 +16,7 @@
 """The Server Group API Extension."""
 
 import collections
+import itertools
 
 from oslo_log import log as logging
 import webob
@@ -87,7 +88,16 @@ def _should_enable_custom_max_server_rules(context, rules):
 class ServerGroupController(wsgi.Controller):
     """The Server group API controller for the OpenStack API."""
 
-    def _format_server_group(self, context, group, req):
+    def _format_server_group(self, context, group, req,
+                             not_deleted_inst_uuids=None):
+        """Format ServerGroup according to API version.
+
+        Displays only not-deleted members.
+
+        :param:not_deleted_inst_uuids: Pre-built set of instance-uuids for
+                                       multiple server-groups that are found to
+                                       be not deleted.
+        """
         # the id field has its value as the uuid of the server group
         # There is no 'uuid' key in server_group seen by clients.
         # In addition, clients see policies as a ["policy-name"] list;
@@ -106,7 +116,12 @@ class ServerGroupController(wsgi.Controller):
         members = []
         if group.members:
             # Display the instances that are not deleted.
-            members = _get_not_deleted(context, group.members)
+            if not_deleted_inst_uuids is not None:
+                # short-cut if we already pre-built a list of not deleted
+                # instances to be more efficient
+                members = list(set(group.members) & not_deleted_inst_uuids)
+            else:
+                members = _get_not_deleted(context, group.members)
         server_group['members'] = members
         # Add project id information to the response data for
         # API version v2.13
@@ -181,7 +196,11 @@ class ServerGroupController(wsgi.Controller):
             sgs = objects.InstanceGroupList.get_by_project_id(
                     context, project_id)
         limited_list = common.limited(sgs.objects, req)
-        result = [self._format_server_group(context, group, req)
+
+        all_members = list(itertools.chain.from_iterable(
+            sg.members for sg in limited_list if sg.members))
+        not_deleted = set(_get_not_deleted(context, all_members))
+        result = [self._format_server_group(context, group, req, not_deleted)
                   for group in limited_list]
         return {'server_groups': result}
 

nova/tests/unit/api/openstack/compute/test_server_groups.py

@@ -14,6 +14,7 @@
 #    under the License.
 
 import copy
+import itertools
 from unittest import mock
 
 from oslo_utils.fixture import uuidsentinel
@@ -389,6 +390,37 @@ class ServerGroupTestV21(test.NoDBTestCase):
         self.assertEqual(2, len(result_members))
         self.assertIn(instances[0].uuid, result_members)
 
+    def test_display_active_members_only_when_listing(self):
+        ctx = context.RequestContext('fake_user', fakes.FAKE_PROJECT_ID,
+                                     roles=['member', 'reader'])
+        (ig1_uuid, ig1_instances, ig1_members) = \
+                self._create_groups_and_instances(ctx)
+        (ig2_uuid, ig2_instances, ig2_members) = \
+                self._create_groups_and_instances(ctx)
+
+        # delete an instance per ig
+        for instance in (ig1_instances[1], ig2_instances[0]):
+            im = objects.InstanceMapping.get_by_instance_uuid(
+                ctx, instance.uuid)
+            with context.target_cell(ctx, im.cell_mapping) as cctxt:
+                instance._context = cctxt
+                instance.destroy()
+            # check that the instance does not exist
+            self.assertRaises(exception.InstanceNotFound,
+                              objects.Instance.get_by_uuid,
+                              ctx, instance.uuid)
+
+        res_dict = self.controller.index(self.reader_req)
+        result_members = sorted(itertools.chain.from_iterable(
+            sg['members'] for sg in res_dict['server_groups']))
+
+        # check that only the active instances are displayed
+        self.assertEqual(4, len(result_members))
+        expected_members = sorted([
+            ig1_instances[0].uuid, ig1_instances[2].uuid,
+            ig2_instances[1].uuid, ig2_instances[2].uuid])
+        self.assertEqual(expected_members, result_members)
+
     def test_display_members_rbac_default(self):
         ctx = context.RequestContext('fake_user', fakes.FAKE_PROJECT_ID)
         ig_uuid = self._create_groups_and_instances(ctx)[0]

releasenotes/notes/bug-2122109-dac145799fc8f58c.yaml

@@ -0,0 +1,9 @@
+---
+fixes:
+  - |
+    `Bug #2122109`_: Fix API performance when getting multiple server groups.
+    We now pre-fetch the not-deleted members of all requested server groups in
+    a single query per cell instead of executing a query per server group per
+    cell.
+
+    .. _Bug #2122109: https://launchpad.net/bugs/2122109