Merge ~ltrager/maas:2.3_ws_cache_current_scriptresults_only into maas:2.3

[Lee Trager]

Only cache processed ScriptResults for the current nodes being dehydrated.

Previously MAAS would cache the current ScriptResults for all nodes
to minimize database lookups. This caused all ScriptResults to be
loaded into RAM regardless of how many nodes are actually being
loaded. Now only the latest ScriptResults from the current set of
nodes being dehydrates are loaded. As nodes are dehydrated their
ScriptResults are cleared from cache to reduce memory usage on the
region.

Backport of 15a4c14 for LP: #1830365

[Blake Rouse (blake-rouse)]

Looks good

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b 2.3_ws_cache_current_scriptresults_only lp:~ltrager/maas/+git/maas into -b 2.3 lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/5797/consoleText

740a4d8... by Lee Trager on 2019-05-30

Merge branch '2.3' into 2.3_ws_cache_current_scriptresults_only

diff --git a/src/maasserver/websockets/base.py b/src/maasserver/websockets/base.py
index 11b7fe4..53fda92 100644
--- a/src/maasserver/websockets/base.py
+++ b/src/maasserver/websockets/base.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """The base class that all handlers must extend."""
@@ -335,6 +335,11 @@ class Handler(metaclass=HandlerMetaclass):
         else:
             raise HandlerNoSuchMethodError(method_name)
 
+    def _cache_pks(self, objs):
+        """Cache all loaded object pks."""
+        getpk = attrgetter(self._meta.pk)
+        self.cache["loaded_pks"].update(getpk(obj) for obj in objs)
+
     def list(self, params):
         """List objects.
 
@@ -353,9 +358,7 @@ class Handler(metaclass=HandlerMetaclass):
         if "limit" in params:
             queryset = queryset[:params["limit"]]
         objs = list(queryset)
-
-        getpk = attrgetter(self._meta.pk)
-        self.cache["loaded_pks"].update(getpk(obj) for obj in objs)
+        self._cache_pks(objs)
         return [
             self.full_dehydrate(obj, for_list=True)
             for obj in objs
@@ -367,8 +370,7 @@ class Handler(metaclass=HandlerMetaclass):
         :param pk: Object with primary key to return.
         """
         obj = self.get_object(params)
-        getpk = attrgetter(self._meta.pk)
-        self.cache["loaded_pks"].add(getpk(obj))
+        self._cache_pks([obj])
         return self.full_dehydrate(obj)
 
     def create(self, params):
diff --git a/src/maasserver/websockets/handlers/device.py b/src/maasserver/websockets/handlers/device.py
index c8af5de..7469821 100644
--- a/src/maasserver/websockets/handlers/device.py
+++ b/src/maasserver/websockets/handlers/device.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """The device handler for the WebSocket connection."""
@@ -7,6 +7,8 @@ __all__ = [
     "DeviceHandler",
     ]
 
+from operator import attrgetter
+
 from django.core.exceptions import ValidationError
 from maasserver.enum import (
     DEVICE_IP_ASSIGNMENT_TYPE,
@@ -136,6 +138,12 @@ class DeviceHandler(NodeHandler):
             "device",
             ]
 
+    def _cache_pks(self, objs):
+        """Cache all loaded object pks."""
+        # Copy from base.py as devices don't have ScriptResults
+        getpk = attrgetter(self._meta.pk)
+        self.cache["loaded_pks"].update(getpk(obj) for obj in objs)
+
     def get_queryset(self):
         """Return `QuerySet` for devices only viewable by `user`."""
         return Device.objects.get_nodes(
diff --git a/src/maasserver/websockets/handlers/machine.py b/src/maasserver/websockets/handlers/machine.py
index 4f16b5f..cd0e6db 100644
--- a/src/maasserver/websockets/handlers/machine.py
+++ b/src/maasserver/websockets/handlers/machine.py
@@ -83,7 +83,6 @@ from metadataserver.enum import (
     HARDWARE_TYPE,
     RESULT_TYPE,
 )
-from metadataserver.models import ScriptResult
 from metadataserver.models.scriptset import get_status_from_qs
 from provisioningserver.logger import LegacyLogger
 from provisioningserver.rpc.exceptions import UnknownPowerType
@@ -192,16 +191,6 @@ class MachineHandler(NodeHandler):
         self.default_osystem = Config.objects.get_config('default_osystem')
         self.default_distro_series = Config.objects.get_config(
             'default_distro_series')
-
-        qs = ScriptResult.objects.all()
-        qs = qs.select_related('script_set', 'script')
-        qs = qs.order_by(
-            'script_name', 'physical_blockdevice_id', 'script_set__node_id',
-            '-id')
-        qs = qs.distinct(
-            'script_name', 'physical_blockdevice_id', 'script_set__node_id')
-        self._refresh_script_result_cache(qs)
-
         return super(MachineHandler, self).list(params)
 
     def dehydrate(self, obj, data, for_list=False):
diff --git a/src/maasserver/websockets/handlers/node.py b/src/maasserver/websockets/handlers/node.py
index a2bc88d..d53e445 100644
--- a/src/maasserver/websockets/handlers/node.py
+++ b/src/maasserver/websockets/handlers/node.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """The node handler for the WebSocket connection."""
@@ -51,6 +51,7 @@ from metadataserver.enum import (
     SCRIPT_STATUS,
     SCRIPT_STATUS_CHOICES,
 )
+from metadataserver.models.scriptresult import ScriptResult
 from metadataserver.models.scriptset import get_status_from_qs
 from provisioningserver.tags import merge_details_cleanly
 
@@ -312,22 +313,26 @@ class NodeHandler(TimestampedModelHandler):
 
         return data
 
-    def _refresh_script_result_cache(self, qs):
-        """Refresh the ScriptResult cache from the given qs.
-
-        If a node_id is given only that node is refreshed.
-        """
-        cleared_node_ids = []
-        for script_result in qs:
+    def _cache_pks(self, nodes):
+        """Refresh the ScriptResult cache from the given node."""
+        super()._cache_pks(nodes)
+        script_results = ScriptResult.objects.filter(
+            script_set__node__in=nodes)
+        script_results = script_results.select_related('script_set', 'script')
+        script_results = script_results.order_by(
+            'script_name', 'physical_blockdevice_id', 'script_set__node_id',
+            '-id')
+        script_results = script_results.distinct(
+            'script_name', 'physical_blockdevice_id', 'script_set__node_id')
+        for script_result in script_results:
             node_id = script_result.script_set.node_id
             if script_result.script is not None:
                 hardware_type = script_result.script.hardware_type
             else:
                 hardware_type = HARDWARE_TYPE.NODE
 
-            if node_id not in cleared_node_ids:
+            if node_id not in self._script_results:
                 self._script_results[node_id] = {}
-                cleared_node_ids.append(node_id)
 
             if hardware_type not in self._script_results[node_id]:
                 self._script_results[node_id][hardware_type] = []
@@ -341,7 +346,7 @@ class NodeHandler(TimestampedModelHandler):
                 for i, cached_script_result in enumerate(
                         self._script_results[node_id][hardware_type]):
                     if cached_script_result.id == script_result.id:
-                        self._script_results[node_id].pop(i)
+                        self._script_results[node_id][hardware_type].pop(i)
                         break
             else:
                 self._script_results[node_id][hardware_type].append(
@@ -629,8 +634,6 @@ class NodeHandler(TimestampedModelHandler):
     def get_object(self, params):
         """Get object by using the `pk` in `params`."""
         obj = super(NodeHandler, self).get_object(params)
-        # Get the object and update update the script_result_cache.
-        self._refresh_script_result_cache(obj.get_latest_script_results)
 
         if self.user.is_superuser:
             return obj.as_self()
@@ -706,12 +709,13 @@ class NodeHandler(TimestampedModelHandler):
         node = self.get_object(params)
         # Produce a "clean" composite details document.
         details_template = dict.fromkeys(script_output_nsmap.values())
-        for hw_type in self._script_results.get(node.id, {}).values():
-            for script_result in hw_type:
-                if (script_result.name in script_output_nsmap and
-                        script_result.status == SCRIPT_STATUS.PASSED):
-                    namespace = script_output_nsmap[script_result.name]
-                    details_template[namespace] = script_result.stdout
+        for script_result in ScriptResult.objects.filter(
+                script_name__in=script_output_nsmap.keys(),
+                status=SCRIPT_STATUS.PASSED,
+                script_set__node=node).order_by(
+                    'script_name', '-updated').distinct('script_name'):
+            namespace = script_output_nsmap[script_result.name]
+            details_template[namespace] = script_result.stdout
         probed_details = merge_details_cleanly(details_template)
 
         # We check here if there's something to show instead of after
@@ -728,12 +732,13 @@ class NodeHandler(TimestampedModelHandler):
         node = self.get_object(params)
         # Produce a "clean" composite details document.
         details_template = dict.fromkeys(script_output_nsmap.values())
-        for hw_type in self._script_results.get(node.id, {}).values():
-            for script_result in hw_type:
-                if (script_result.name in script_output_nsmap and
-                        script_result.status == SCRIPT_STATUS.PASSED):
-                    namespace = script_output_nsmap[script_result.name]
-                    details_template[namespace] = script_result.stdout
+        for script_result in ScriptResult.objects.filter(
+                script_name__in=script_output_nsmap.keys(),
+                status=SCRIPT_STATUS.PASSED,
+                script_set__node=node).order_by(
+                    'script_name', '-updated').distinct('script_name'):
+            namespace = script_output_nsmap[script_result.name]
+            details_template[namespace] = script_result.stdout
         probed_details = merge_details_cleanly(details_template)
 
         # We check here if there's something to show instead of after
diff --git a/src/maasserver/websockets/handlers/tests/test_device.py b/src/maasserver/websockets/handlers/tests/test_device.py
index f7af49c..c26e303 100644
--- a/src/maasserver/websockets/handlers/tests/test_device.py
+++ b/src/maasserver/websockets/handlers/tests/test_device.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for `maasserver.websockets.handlers.device`"""
diff --git a/src/maasserver/websockets/handlers/tests/test_machine.py b/src/maasserver/websockets/handlers/tests/test_machine.py
index 299a5e8..c6bbbbf 100644
--- a/src/maasserver/websockets/handlers/tests/test_machine.py
+++ b/src/maasserver/websockets/handlers/tests/test_machine.py
@@ -1,4 +1,4 @@
-# Copyright 2016-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2016-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for `maasserver.websockets.handlers.node`"""
@@ -146,7 +146,7 @@ class TestMachineHandler(MAASServerTestCase):
     def dehydrate_node(self, node, handler, for_list=False):
         # Prime handler._script_results
         handler._script_results = {}
-        handler._refresh_script_result_cache(node.get_latest_script_results)
+        handler._cache_pks([node])
 
         boot_interface = node.get_boot_interface()
         pxe_mac_vendor = node.get_pxe_mac_vendor()
@@ -407,7 +407,7 @@ class TestMachineHandler(MAASServerTestCase):
                 factory.make_Node(
                     node_type=NODE_TYPE.DEVICE, parent=node, interface=True)
 
-    def test_get_object_refresh_script_result_cache(self):
+    def test_get_refresh_script_result_cache(self):
         owner = factory.make_User()
         node = factory.make_Node(owner=owner)
         script_result = factory.make_ScriptResult(
@@ -429,7 +429,7 @@ class TestMachineHandler(MAASServerTestCase):
         }
         handler = MachineHandler(owner, {})
         handler._script_results[cached_node.id] = cached_content
-        handler.get_object({'system_id': node.system_id})
+        handler._cache_pks([node])
 
         self.assertEquals(
             script_result.id,
@@ -444,7 +444,7 @@ class TestMachineHandler(MAASServerTestCase):
         self.assertEquals(
             cached_content, handler._script_results[cached_node.id])
 
-    def test_get_object_refresh_script_result_cache_clears_aborted(self):
+    def test_get_refresh_script_result_cache_clears_aborted(self):
         # Regression test for LP:1731350
         owner = factory.make_User()
         node = factory.make_Node(owner=owner)
@@ -459,11 +459,45 @@ class TestMachineHandler(MAASServerTestCase):
         # Simulate aborting commissioning/testing
         script_result.status = SCRIPT_STATUS.ABORTED
         script_result.save()
-        handler.get_object({'system_id': node.system_id})
+        handler._cache_pks([node])
 
         self.assertItemsEqual([], handler._script_results[node.id][
             script_result.script.hardware_type])
 
+    def test_list_num_queries_is_the_expected_number(self):
+        owner = factory.make_User()
+        for _ in range(10):
+            node = factory.make_Node(owner=owner)
+            commissioning_script_set = factory.make_ScriptSet(
+                node=node, result_type=RESULT_TYPE.COMMISSIONING)
+            testing_script_set = factory.make_ScriptSet(
+                node=node, result_type=RESULT_TYPE.TESTING)
+            node.current_commissioning_script_set = commissioning_script_set
+            node.current_testing_script_set = testing_script_set
+            node.save()
+            for __ in range(10):
+                factory.make_ScriptResult(
+                    status=SCRIPT_STATUS.PASSED,
+                    script_set=commissioning_script_set)
+                factory.make_ScriptResult(
+                    status=SCRIPT_STATUS.PASSED,
+                    script_set=testing_script_set)
+
+        handler = MachineHandler(owner, {})
+        queries_one, _ = count_queries(handler.list, {'limit': 1})
+        queries_total, _ = count_queries(handler.list, {})
+        # This check is to notify the developer that a change was made that
+        # affects the number of queries performed when doing a node listing.
+        # It is important to keep this number as low as possible. A larger
+        # number means regiond has to do more work slowing down its process
+        # and slowing down the client waiting for the response.
+        self.assertEqual(
+            queries_one, 12,
+            "Number of queries has changed; make sure this is expected.")
+        self.assertEqual(
+            queries_total, 12,
+            "Number of queries has changed; make sure this is expected.")
+
     def test_dehydrate_owner_empty_when_None(self):
         owner = factory.make_User()
         handler = MachineHandler(owner, {})
@@ -1502,7 +1536,7 @@ class TestMachineHandler(MAASServerTestCase):
         user = factory.make_User()
         handler = MachineHandler(user, {})
         node = factory.make_Node(status=NODE_STATUS.ALLOCATED, owner=user)
-        factory.make_ScriptResult(
+        script_result = factory.make_ScriptResult(
             script_set=factory.make_ScriptSet(node=node),
             status=SCRIPT_STATUS.PASSED)
         factory.make_PhysicalBlockDevice(node)
@@ -1510,7 +1544,10 @@ class TestMachineHandler(MAASServerTestCase):
         self.assertItemsEqual(
             [self.dehydrate_node(node, handler, for_list=True)],
             handler.list({}))
-        self.assertIn(node.id, handler._script_results.keys())
+        self.assertDictEqual(
+            {node.id: {
+                script_result.script.hardware_type: [script_result]}},
+            handler._script_results)
 
     def test_list_ignores_devices(self):
         owner = factory.make_User()
@@ -2673,7 +2710,7 @@ class TestMachineHandler(MAASServerTestCase):
         new_name = factory.make_name("name")
         new_vlan = factory.make_VLAN()
         handler._script_results = {}
-        handler._refresh_script_result_cache(node.get_latest_script_results)
+        handler._cache_pks([node])
         handler.update_interface({
             "system_id": node.system_id,
             "interface_id": interface.id,
@@ -2691,7 +2728,7 @@ class TestMachineHandler(MAASServerTestCase):
         interface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL, node=node)
         new_name = factory.make_name("name")
         handler._script_results = {}
-        handler._refresh_script_result_cache(node.get_latest_script_results)
+        handler._cache_pks([node])
         handler.update_interface({
             "system_id": node.system_id,
             "interface_id": interface.id,