Merge ~ltrager/maas:limit_listing_queries into maas:master

[Lee Trager]

LP: #1759091 - Limit the amount of queries used during a list operation.

Handlers can now specify a list_queryset which is used when the client calls
list(). The machine, device, and controller handlers have been updated to
only output data needed for the listing page for that node type. All node
types now have their query count tested for list and get.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b limit_listing_queries lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/2198/console
COMMIT: 9c3d32c4d91bc6339b0c191d370d9083f37ce9d0

review: Needs Fixing

[Blake Rouse (blake-rouse)]

review: Needs Fixing

37b692f... by Lee Trager on 2018-03-29

Merge branch 'master' into limit_listing_queries

c38c9fb... by Lee Trager on 2018-03-29

Add get_queryset tests

[Lee Trager (ltrager)]

Thanks for the review. We never had query_count tests for get(), only list(). The reason get() takes so many more queries then list() is due to storage and network interface dehydration. Get queries have gone down with this branch. get() on the MachineHandler went from 61 queries to 48.

I was relying on the query_count tests to test get_queryset, I've added explicit tests for get_queryset.

[Blake Rouse (blake-rouse)]

Thanks for the extra test and the clarification.

review: Approve

diff --git a/src/maasserver/third_party_drivers.py b/src/maasserver/third_party_drivers.py
index ff2039a..6c7311b 100644
--- a/src/maasserver/third_party_drivers.py
+++ b/src/maasserver/third_party_drivers.py
@@ -134,13 +134,14 @@ def populate_kernel_opts(driver):
     return driver
 
 
-def get_third_party_driver(node):
+def get_third_party_driver(node, detected_aliases=None):
     """Determine which, if any, third party driver is required.
 
     Use the node's modaliases strings to determine if a third party
     driver is required.
     """
-    detected_aliases = node.modaliases
+    if detected_aliases is None:
+        detected_aliases = node.modaliases
 
     third_party_drivers_config = DriversConfig.load_from_cache()
     third_party_drivers = third_party_drivers_config['drivers']
diff --git a/src/maasserver/websockets/base.py b/src/maasserver/websockets/base.py
index 636d50a..d7327e4 100644
--- a/src/maasserver/websockets/base.py
+++ b/src/maasserver/websockets/base.py
@@ -81,6 +81,7 @@ class HandlerOptions(object):
     handler_name = None
     object_class = None
     queryset = None
+    list_queryset = None
     pk = 'id'
     pk_type = int
     fields = None
@@ -281,19 +282,22 @@ class Handler(metaclass=HandlerMetaclass):
             })
         pk = params[self._meta.pk]
         try:
-            obj = self.get_queryset().get(**{
+            obj = self.get_queryset(for_list=False).get(**{
                 self._meta.pk: pk,
                 })
         except self._meta.object_class.DoesNotExist:
             raise HandlerDoesNotExistError(pk)
         return obj
 
-    def get_queryset(self):
+    def get_queryset(self, for_list=False):
         """Return `QuerySet` used by this handler.
 
         Override if you need to modify the queryset based on the current user.
         """
-        return self._meta.queryset
+        if for_list and self._meta.list_queryset is not None:
+            return self._meta.list_queryset
+        else:
+            return self._meta.queryset
 
     def get_form_class(self, action):
         """Return the form class used for `action`.
@@ -350,7 +354,7 @@ class Handler(metaclass=HandlerMetaclass):
         :param offset: Offset into the queryset to return.
         :param limit: Maximum number of objects to return.
         """
-        queryset = self.get_queryset()
+        queryset = self.get_queryset(for_list=True)
         queryset = queryset.order_by(self._meta.batch_key)
         if "start" in params:
             queryset = queryset.filter(**{
diff --git a/src/maasserver/websockets/handlers/controller.py b/src/maasserver/websockets/handlers/controller.py
index 9fbe5c8..636c4d9 100644
--- a/src/maasserver/websockets/handlers/controller.py
+++ b/src/maasserver/websockets/handlers/controller.py
@@ -24,8 +24,12 @@ class ControllerHandler(MachineHandler):
     class Meta(MachineHandler.Meta):
         abstract = False
         queryset = node_prefetch(
-            Controller.controllers.all().prefetch_related("interface_set"),
-            'controllerinfo'
+            Controller.controllers.all().prefetch_related(
+                "service_set"), "controllerinfo")
+        list_queryset = (
+            Controller.controllers.all()
+            .select_related("controllerinfo", "domain")
+            .prefetch_related("service_set")
         )
         allowed_methods = [
             'list',
@@ -90,10 +94,14 @@ class ControllerHandler(MachineHandler):
         else:
             raise HandlerError("Unknown action: %s" % action)
 
-    def get_queryset(self):
+    def get_queryset(self, for_list=False):
         """Return `QuerySet` for controllers only viewable by `user`."""
+        if for_list:
+            qs = self._meta.list_queryset
+        else:
+            qs = self._meta.queryset
         return Controller.controllers.get_nodes(
-            self.user, NODE_PERMISSION.VIEW, from_nodes=self._meta.queryset)
+            self.user, NODE_PERMISSION.VIEW, from_nodes=qs)
 
     def dehydrate(self, obj, data, for_list=False):
         obj = obj.as_self()
@@ -108,14 +116,15 @@ class ControllerHandler(MachineHandler):
             if version.is_snap:
                 long_version += " (snap)"
             data["version__long"] = long_version
-        data["vlan_ids"] = [
-            interface.vlan_id
-            for interface in obj.interface_set.all()
-        ]
         data["service_ids"] = [
             service.id
             for service in obj.service_set.all()
         ]
+        if not for_list:
+            data["vlan_ids"] = [
+                interface.vlan_id
+                for interface in obj.interface_set.all()
+            ]
         return data
 
     def check_images(self, params):
diff --git a/src/maasserver/websockets/handlers/device.py b/src/maasserver/websockets/handlers/device.py
index dc5374f..0407b0c 100644
--- a/src/maasserver/websockets/handlers/device.py
+++ b/src/maasserver/websockets/handlers/device.py
@@ -37,10 +37,7 @@ from maasserver.websockets.base import (
     HandlerPermissionError,
     HandlerValidationError,
 )
-from maasserver.websockets.handlers.node import (
-    node_prefetch,
-    NodeHandler,
-)
+from maasserver.websockets.handlers.node import NodeHandler
 from netaddr import EUI
 from provisioningserver.logger import get_maas_logger
 
@@ -65,7 +62,16 @@ class DeviceHandler(NodeHandler):
 
     class Meta(NodeHandler.Meta):
         abstract = False
-        queryset = node_prefetch(Device.objects.filter(parent=None))
+        queryset = (
+            Device.objects.filter(parent=None).select_related(
+                'boot_interface', 'owner', 'zone', 'domain')
+            .prefetch_related(
+                'interface_set__ip_addresses__subnet__vlan__space')
+            .prefetch_related(
+                'interface_set__ip_addresses__subnet__vlan__fabric')
+            .prefetch_related('interface_set__vlan__fabric')
+            .prefetch_related('tags')
+        )
         allowed_methods = [
             'list',
             'get',
@@ -80,6 +86,7 @@ class DeviceHandler(NodeHandler):
             'update',
             'action']
         exclude = [
+            "bmc",
             "creation_type",
             "type",
             "boot_interface",
@@ -145,10 +152,11 @@ class DeviceHandler(NodeHandler):
         getpk = attrgetter(self._meta.pk)
         self.cache["loaded_pks"].update(getpk(obj) for obj in objs)
 
-    def get_queryset(self):
+    def get_queryset(self, for_list=False):
         """Return `QuerySet` for devices only viewable by `user`."""
         return Device.objects.get_nodes(
-            self.user, NODE_PERMISSION.VIEW, from_nodes=self._meta.queryset)
+            self.user, NODE_PERMISSION.VIEW, from_nodes=super().get_queryset(
+                for_list=for_list))
 
     def dehydrate_parent(self, parent):
         if parent is None:
diff --git a/src/maasserver/websockets/handlers/event.py b/src/maasserver/websockets/handlers/event.py
index b7e4a80..0845b5f 100644
--- a/src/maasserver/websockets/handlers/event.py
+++ b/src/maasserver/websockets/handlers/event.py
@@ -75,7 +75,7 @@ class EventHandler(TimestampedModelHandler):
         """
         node = self.get_node(params)
         self.cache['node_ids'].append(node.id)
-        queryset = self.get_queryset()
+        queryset = self.get_queryset(for_list=True)
         queryset = queryset.filter(node=node)
         queryset = queryset.order_by('-id')
 
diff --git a/src/maasserver/websockets/handlers/machine.py b/src/maasserver/websockets/handlers/machine.py
index 375b0fb..ca5cbeb 100644
--- a/src/maasserver/websockets/handlers/machine.py
+++ b/src/maasserver/websockets/handlers/machine.py
@@ -54,7 +54,6 @@ from maasserver.forms.interface import (
 from maasserver.forms.interface_link import InterfaceLinkForm
 from maasserver.models.blockdevice import BlockDevice
 from maasserver.models.cacheset import CacheSet
-from maasserver.models.config import Config
 from maasserver.models.filesystem import Filesystem
 from maasserver.models.filesystemgroup import VolumeGroup
 from maasserver.models.interface import Interface
@@ -96,7 +95,20 @@ class MachineHandler(NodeHandler):
 
     class Meta(NodeHandler.Meta):
         abstract = False
-        queryset = node_prefetch(Machine.objects.all()).select_related('bmc')
+        queryset = node_prefetch(Machine.objects.all())
+        list_queryset = (
+            Machine.objects.select_related(
+                'boot_interface', 'owner', 'zone', 'domain')
+            .prefetch_related('blockdevice_set__iscsiblockdevice')
+            .prefetch_related('blockdevice_set__physicalblockdevice')
+            .prefetch_related('blockdevice_set__virtualblockdevice')
+            .prefetch_related(
+                'interface_set__ip_addresses__subnet__vlan__space')
+            .prefetch_related(
+                'interface_set__ip_addresses__subnet__vlan__fabric')
+            .prefetch_related('interface_set__vlan__fabric')
+            .prefetch_related('tags')
+        )
         allowed_methods = [
             'list',
             'get',
@@ -177,54 +189,24 @@ class MachineHandler(NodeHandler):
             "machine",
         ]
 
-    def get_queryset(self):
+    def get_queryset(self, for_list=False):
         """Return `QuerySet` for devices only viewable by `user`."""
         return Machine.objects.get_nodes(
-            self.user, NODE_PERMISSION.VIEW, from_nodes=self._meta.queryset)
-
-    def list(self, params):
-        """List objects.
-
-        Caches default_osystem and default_distro_series so only 1 queries are
-        made for the whole list of nodes.
-
-        Caches the hardware status so only one additional query is needed for
-        all nodes.
-        """
-        configs = (
-            Config.objects.get_configs(
-                ['default_osystem', 'default_distro_series']))
-        self.default_osystem = configs['default_osystem']
-        self.default_distro_series = configs['default_distro_series']
-        return super(MachineHandler, self).list(params)
+            self.user, NODE_PERMISSION.VIEW,
+            from_nodes=super().get_queryset(for_list=for_list))
 
     def dehydrate(self, obj, data, for_list=False):
         """Add extra fields to `data`."""
         data = super(MachineHandler, self).dehydrate(
             obj, data, for_list=for_list)
-        data["locked"] = obj.locked
-        bmc = obj.bmc
-        if bmc is not None and bmc.bmc_type == BMC_TYPE.POD:
-            data['pod'] = self.dehydrate_pod(bmc)
-
-        if (getattr(self, 'default_osystem', None) is not None and
-                getattr(self, 'default_distro_series', None) is not None):
-            data["osystem"] = obj.get_osystem(
-                default=self.default_osystem)
-            data["distro_series"] = obj.get_distro_series(
-                default=self.default_distro_series)
-        else:
-            data["osystem"] = obj.get_osystem()
-            data["distro_series"] = obj.get_distro_series()
-        if not for_list:
-            # Add info specific to a machine.
-            data["show_os_info"] = self.dehydrate_show_os_info(obj)
-            devices = [
-                self.dehydrate_device(device)
-                for device in obj.children.all()
-            ]
-            data["devices"] = sorted(
-                devices, key=itemgetter("fqdn"))
+
+        if obj.is_machine or not for_list:
+            boot_interface = obj.get_boot_interface()
+            if boot_interface is not None:
+                data["pxe_mac"] = "%s" % boot_interface.mac_address
+                data["pxe_mac_vendor"] = obj.get_pxe_mac_vendor()
+            else:
+                data["pxe_mac"] = data["pxe_mac_vendor"] = ""
 
         cpu_script_results = [
             script_result for script_result in
@@ -279,6 +261,18 @@ class MachineHandler(NodeHandler):
         else:
             data["status_tooltip"] = ""
 
+        if not for_list:
+            if obj.bmc is not None and obj.bmc.bmc_type == BMC_TYPE.POD:
+                data['pod'] = self.dehydrate_pod(obj.bmc)
+            # Add info specific to a machine.
+            data["show_os_info"] = self.dehydrate_show_os_info(obj)
+            devices = [
+                self.dehydrate_device(device)
+                for device in obj.children.all()
+            ]
+            data["devices"] = sorted(
+                devices, key=itemgetter("fqdn"))
+
         return data
 
     def dehydrate_show_os_info(self, obj):
diff --git a/src/maasserver/websockets/handlers/node.py b/src/maasserver/websockets/handlers/node.py
index 92fc9b5..8f33bdc 100644
--- a/src/maasserver/websockets/handlers/node.py
+++ b/src/maasserver/websockets/handlers/node.py
@@ -54,6 +54,9 @@ from metadataserver.enum import (
 )
 from metadataserver.models.scriptresult import ScriptResult
 from metadataserver.models.scriptset import get_status_from_qs
+from provisioningserver.refresh.node_info_scripts import (
+    LIST_MODALIASES_OUTPUT_NAME,
+)
 from provisioningserver.tags import merge_details_cleanly
 
 
@@ -69,7 +72,8 @@ NODE_TYPE_TO_LINK_TYPE = {
 def node_prefetch(queryset, *args):
     return (
         queryset
-        .select_related('boot_interface', 'owner', 'zone', 'domain', *args)
+        .select_related(
+            'boot_interface', 'owner', 'zone', 'domain', 'bmc', *args)
         .prefetch_related('blockdevice_set__iscsiblockdevice')
         .prefetch_related('blockdevice_set__physicalblockdevice')
         .prefetch_related('blockdevice_set__virtualblockdevice')
@@ -84,9 +88,6 @@ def node_prefetch(queryset, *args):
 
 class NodeHandler(TimestampedModelHandler):
 
-    default_osystem = None
-    default_distro_series = None
-
     class Meta:
         abstract = True
         pk = 'system_id'
@@ -186,35 +187,10 @@ class NodeHandler(TimestampedModelHandler):
         data["node_type_display"] = obj.get_node_type_display()
         data["link_type"] = NODE_TYPE_TO_LINK_TYPE[obj.node_type]
 
-        data["extra_macs"] = [
-            "%s" % mac_address
-            for mac_address in obj.get_extra_macs()
-        ]
-        subnets = self.get_all_subnets(obj)
-        data["subnets"] = [subnet.cidr for subnet in subnets]
-        data["fabrics"] = self.get_all_fabric_names(obj, subnets)
-        data["spaces"] = self.get_all_space_names(subnets)
-
-        data["tags"] = [
-            tag.name
-            for tag in obj.tags.all()
-        ]
-        data["metadata"] = {
-            metadata.key: metadata.value
-            for metadata in obj.nodemetadata_set.all()
-        }
-        if obj.node_type != NODE_TYPE.DEVICE:
-            data["architecture"] = obj.architecture
-            data["memory"] = obj.display_memory()
-            data["status"] = obj.display_status()
-            data["status_code"] = obj.status
-            boot_interface = obj.get_boot_interface()
-            if boot_interface is not None:
-                data["pxe_mac"] = "%s" % boot_interface.mac_address
-                data["pxe_mac_vendor"] = obj.get_pxe_mac_vendor()
-            else:
-                data["pxe_mac"] = data["pxe_mac_vendor"] = ""
-
+        if obj.node_type == NODE_TYPE.MACHINE or (
+                obj.is_controller and not for_list):
+            # Disk count and storage amount is shown on the machine listing
+            # page and the machine and controllers details page.
             blockdevices = self.get_blockdevices_for(obj)
             physical_blockdevices = [
                 blockdevice for blockdevice in blockdevices
@@ -227,16 +203,6 @@ class NodeHandler(TimestampedModelHandler):
                     for blockdevice in physical_blockdevices
                     ) / (1000 ** 3))
             data["storage_tags"] = self.get_all_storage_tags(blockdevices)
-            data["grouped_storages"] = self.get_grouped_storages(
-                physical_blockdevices)
-
-            data["osystem"] = obj.get_osystem(
-                default=self.default_osystem)
-            data["distro_series"] = obj.get_distro_series(
-                default=self.default_distro_series)
-            data["dhcp_on"] = self.get_providing_dhcp(obj)
-
-        if obj.node_type != NODE_TYPE.DEVICE:
             commissioning_script_results = []
             testing_script_results = []
             log_results = set()
@@ -254,12 +220,12 @@ class NodeHandler(TimestampedModelHandler):
                             RESULT_TYPE.COMMISSIONING):
                         commissioning_script_results.append(script_result)
                         if (script_result.name in script_output_nsmap and
-                                script_result.status == SCRIPT_STATUS.PASSED):
+                                script_result.status ==
+                                SCRIPT_STATUS.PASSED):
                             log_results.add(script_result.name)
                     elif (script_result.script_set.result_type ==
                             RESULT_TYPE.TESTING):
                         testing_script_results.append(script_result)
-
             data["commissioning_script_count"] = len(
                 commissioning_script_results)
             data["commissioning_status"] = get_status_from_qs(
@@ -269,11 +235,41 @@ class NodeHandler(TimestampedModelHandler):
                     commissioning_script_results).replace(
                         'test', 'commissioning script'))
             data["testing_script_count"] = len(testing_script_results)
-            data["testing_status"] = get_status_from_qs(testing_script_results)
+            data["testing_status"] = get_status_from_qs(
+                testing_script_results)
             data["testing_status_tooltip"] = (
-                self.dehydrate_hardware_status_tooltip(testing_script_results))
+                self.dehydrate_hardware_status_tooltip(
+                    testing_script_results))
             data["has_logs"] = (
-                log_results.difference(script_output_nsmap.keys()) == set())
+                log_results.difference(script_output_nsmap.keys()) ==
+                set())
+        else:
+            blockdevices = []
+
+        if obj.node_type != NODE_TYPE.DEVICE:
+            # These values are not defined on a device.
+            data["architecture"] = obj.architecture
+            data["osystem"] = obj.osystem
+            data["distro_series"] = obj.distro_series
+            data["memory"] = obj.display_memory()
+            data["status"] = obj.display_status()
+            data["status_code"] = obj.status
+
+        # Filters are only available on machines and devices.
+        if not obj.is_controller:
+            # For filters
+            subnets = self.get_all_subnets(obj)
+            data["subnets"] = [subnet.cidr for subnet in subnets]
+            data["fabrics"] = self.get_all_fabric_names(obj, subnets)
+            data["spaces"] = self.get_all_space_names(subnets)
+            data["tags"] = [
+                tag.name
+                for tag in obj.tags.all()
+            ]
+            data["extra_macs"] = [
+                "%s" % mac_address
+                for mac_address in obj.get_extra_macs()
+            ]
 
         if not for_list:
             data["on_network"] = obj.on_network()
@@ -281,11 +277,18 @@ class NodeHandler(TimestampedModelHandler):
                 # XXX lamont 2017-02-15 Much of this should be split out into
                 # individual methods, rather than having this huge block of
                 # dense code here.
+                # Status of the commissioning, testing, and logs tabs
+                data["metadata"] = {
+                    metadata.key: metadata.value
+                    for metadata in obj.nodemetadata_set.all()
+                }
+
                 # Network
                 data["interfaces"] = [
                     self.dehydrate_interface(interface, obj)
                     for interface in obj.interface_set.all().order_by('name')
                 ]
+                data["dhcp_on"] = self.get_providing_dhcp(obj)
 
                 data["hwe_kernel"] = make_hwe_kernel_ui_text(obj.hwe_kernel)
 
@@ -313,6 +316,8 @@ class NodeHandler(TimestampedModelHandler):
                     self.dehydrate_filesystem(filesystem)
                     for filesystem in obj.special_filesystems.order_by("id")
                 ]
+                data["grouped_storages"] = self.get_grouped_storages(
+                    physical_blockdevices)
 
                 # Events
                 data["events"] = self.dehydrate_events(obj)
@@ -323,7 +328,18 @@ class NodeHandler(TimestampedModelHandler):
 
                 # Third party drivers
                 if Config.objects.get_config('enable_third_party_drivers'):
-                    driver = get_third_party_driver(obj)
+                    # Pull modaliases from the cache
+                    modaliases = []
+                    for script_result in commissioning_script_results:
+                        if script_result.name == LIST_MODALIASES_OUTPUT_NAME:
+                            if script_result.status == SCRIPT_STATUS.PASSED:
+                                # STDOUT is deferred in the cache so load it.
+                                script_result = ScriptResult.objects.filter(
+                                    id=script_result.id).only(
+                                        'id', 'stdout').first()
+                                modaliases = script_result.stdout.decode(
+                                    'utf-8').splitlines()
+                    driver = get_third_party_driver(obj, modaliases)
                     if "module" in driver and "comment" in driver:
                         data["third_party_driver"] = {
                             "module": driver["module"],
diff --git a/src/maasserver/websockets/handlers/notification.py b/src/maasserver/websockets/handlers/notification.py
index 7292e68..d189e75 100644
--- a/src/maasserver/websockets/handlers/notification.py
+++ b/src/maasserver/websockets/handlers/notification.py
@@ -1,4 +1,4 @@
-# Copyright 2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2017-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """The notification handler for the WebSocket connection."""
@@ -22,7 +22,7 @@ class NotificationHandler(TimestampedModelHandler):
         exclude = list_exclude = {"context"}
         listen_channels = {'notification', 'notificationdismissal'}
 
-    def get_queryset(self):
+    def get_queryset(self, for_list=False):
         """Return `Notifications` for the current user."""
         return Notification.objects.find_for_user(self.user)
 
diff --git a/src/maasserver/websockets/handlers/sshkey.py b/src/maasserver/websockets/handlers/sshkey.py
index efac2e6..37503dc 100644
--- a/src/maasserver/websockets/handlers/sshkey.py
+++ b/src/maasserver/websockets/handlers/sshkey.py
@@ -41,7 +41,7 @@ class SSHKeyHandler(TimestampedModelHandler):
             "sshkey",
         ]
 
-    def get_queryset(self):
+    def get_queryset(self, for_list=False):
         """Return `QuerySet` for SSH keys owned by `user`."""
         return self._meta.queryset.filter(user=self.user)
 
diff --git a/src/maasserver/websockets/handlers/switch.py b/src/maasserver/websockets/handlers/switch.py
index e40f779..72accf5 100644
--- a/src/maasserver/websockets/handlers/switch.py
+++ b/src/maasserver/websockets/handlers/switch.py
@@ -1,4 +1,4 @@
-# Copyright 2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2017-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """The switch handler for the WebSocket connection."""
@@ -45,10 +45,14 @@ class SwitchHandler(NodeHandler):
             'switch',
         ]
 
-    def get_queryset(self):
+    def get_queryset(self, for_list=False):
         """Return `QuerySet` for devices only viewable by `user`."""
+        # FIXME - Return a different query set when for_list is true. This
+        # should contain only the items needed to display a switch when listing
+        # in the UI.
         return Node.objects.get_nodes(
-            self.user, NODE_PERMISSION.VIEW, from_nodes=self._meta.queryset)
+            self.user, NODE_PERMISSION.VIEW,
+            from_nodes=self._meta.queryset)
 
     def get_object(self, params):
         """Get object by using the `pk` in `params`."""
diff --git a/src/maasserver/websockets/handlers/tests/test_controller.py b/src/maasserver/websockets/handlers/tests/test_controller.py
index 18d2433..e86c19a 100644
--- a/src/maasserver/websockets/handlers/tests/test_controller.py
+++ b/src/maasserver/websockets/handlers/tests/test_controller.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.controller`"""
@@ -12,6 +12,11 @@ from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
 from maasserver.websockets.base import dehydrate_datetime
 from maasserver.websockets.handlers.controller import ControllerHandler
+from maastesting.djangotestcase import count_queries
+from metadataserver.enum import (
+    RESULT_TYPE,
+    SCRIPT_STATUS,
+)
 from testscenarios import multiply_scenarios
 from testtools.matchers import (
     ContainsDict,
@@ -65,6 +70,69 @@ class TestControllerHandler(MAASServerTestCase):
         self.assertEqual(1, len(result))
         self.assertEqual(NODE_TYPE.RACK_CONTROLLER, result[0].get('node_type'))
 
+    def test_list_num_queries_is_the_expected_number(self):
+        owner = factory.make_admin()
+        for _ in range(10):
+            node = factory.make_RegionRackController(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 = ControllerHandler(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, 3,
+            "Number of queries has changed; make sure this is expected.")
+        self.assertEqual(
+            queries_total, 3,
+            "Number of queries has changed; make sure this is expected.")
+
+    def test_get_num_queries_is_the_expected_number(self):
+        owner = factory.make_admin()
+        node = factory.make_RegionRackController(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 = ControllerHandler(owner, {})
+        queries, _ = count_queries(handler.get, {'system_id': node.system_id})
+        # This check is to notify the developer that a change was made that
+        # affects the number of queries performed when doing a node get.
+        # 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, 28,
+            "Number of queries has changed; make sure this is expected.")
+
     def test_get_form_class_for_create(self):
         user = factory.make_admin()
         handler = ControllerHandler(user, {})
diff --git a/src/maasserver/websockets/handlers/tests/test_device.py b/src/maasserver/websockets/handlers/tests/test_device.py
index ab26620..d6af973 100644
--- a/src/maasserver/websockets/handlers/tests/test_device.py
+++ b/src/maasserver/websockets/handlers/tests/test_device.py
@@ -137,7 +137,6 @@ class TestDeviceHandler(MAASTransactionServerTestCase):
         boot_interface = node.get_boot_interface()
         data = {
             "actions": list(compile_node_actions(node, user).keys()),
-            "bmc": node.bmc_id,
             "created": dehydrate_datetime(node.created),
             "domain": {
                 "id": node.domain.id,
@@ -149,7 +148,6 @@ class TestDeviceHandler(MAASTransactionServerTestCase):
                 ],
             "fqdn": node.fqdn,
             "hostname": node.hostname,
-            "metadata": {},
             "node_type_display": node.get_node_type_display(),
             "link_type": NODE_TYPE_TO_LINK_TYPE[node.node_type],
             "id": node.id,
@@ -195,7 +193,6 @@ class TestDeviceHandler(MAASTransactionServerTestCase):
                 "ip_address",
                 "ip_assignment",
                 "link_type",
-                "metadata",
                 "node_type_display",
                 "primary_mac",
                 "spaces",
@@ -253,6 +250,24 @@ class TestDeviceHandler(MAASTransactionServerTestCase):
             handler.get({"system_id": device.system_id}))
 
     @transactional
+    def test_get_num_queries_is_the_expected_number(self):
+        owner = factory.make_User()
+        handler = DeviceHandler(owner, {})
+        device = self.make_device_with_ip_address(
+            owner=owner, ip_assignment=DEVICE_IP_ASSIGNMENT_TYPE.STATIC)
+        queries, _ = count_queries(
+            handler.get, {"system_id": device.system_id})
+
+        # This check is to notify the developer that a change was made that
+        # affects the number of queries performed when doing a node get.
+        # 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, 19,
+            "Number of queries has changed; make sure this is expected.")
+
+    @transactional
     def test_list(self):
         owner = factory.make_User()
         handler = DeviceHandler(owner, {})
@@ -301,7 +316,7 @@ class TestDeviceHandler(MAASTransactionServerTestCase):
         # number means regiond has to do more work slowing down its process
         # and slowing down the client waiting for the response.
         self.assertEqual(
-            query_10_count, 13,
+            query_10_count, 10,
             "Number of queries has changed; make sure this is expected.")
 
     @transactional
diff --git a/src/maasserver/websockets/handlers/tests/test_machine.py b/src/maasserver/websockets/handlers/tests/test_machine.py
index 27bc195..3b333ed 100644
--- a/src/maasserver/websockets/handlers/tests/test_machine.py
+++ b/src/maasserver/websockets/handlers/tests/test_machine.py
@@ -150,11 +150,12 @@ class TestMachineHandler(MAASServerTestCase):
 
         boot_interface = node.get_boot_interface()
         pxe_mac_vendor = node.get_pxe_mac_vendor()
+        subnets = handler.get_all_subnets(node)
+
         blockdevices = [
             blockdevice.actual_instance
             for blockdevice in node.blockdevice_set.all()
             ]
-        driver = get_third_party_driver(node)
         disks = [
             handler.dehydrate_blockdevice(blockdevice, node)
             for blockdevice in blockdevices
@@ -167,8 +168,10 @@ class TestMachineHandler(MAASServerTestCase):
             for cache_set in CacheSet.objects.get_cache_sets_for_node(node)
         ]
         disks = sorted(disks, key=itemgetter("name"))
-        subnets = handler.get_all_subnets(node)
-        commissioning_scripts = node.get_latest_commissioning_script_results
+        driver = get_third_party_driver(node)
+
+        commissioning_scripts = (
+            node.get_latest_commissioning_script_results)
         commissioning_scripts = commissioning_scripts.exclude(
             status=SCRIPT_STATUS.ABORTED)
         testing_scripts = node.get_latest_testing_script_results
@@ -179,6 +182,7 @@ class TestMachineHandler(MAASServerTestCase):
             if (script_result.name in script_output_nsmap and
                     script_result.status == SCRIPT_STATUS.PASSED):
                 log_results.add(script_result.name)
+
         data = {
             "actions": list(compile_node_actions(node, handler.user).keys()),
             "architecture": node.architecture,
@@ -186,25 +190,27 @@ class TestMachineHandler(MAASServerTestCase):
             "boot_disk": node.boot_disk,
             "bios_boot_method": node.bios_boot_method,
             "commissioning_script_count": commissioning_scripts.count(),
-            "commissioning_status": get_status_from_qs(commissioning_scripts),
+            "commissioning_status": get_status_from_qs(
+                commissioning_scripts),
             "commissioning_status_tooltip": (
                 handler.dehydrate_hardware_status_tooltip(
                     commissioning_scripts).replace(
                         'test', 'commissioning script')),
             "current_commissioning_script_set": (
                 node.current_commissioning_script_set_id),
+            "current_testing_script_set": node.current_testing_script_set_id,
             "testing_script_count": testing_scripts.count(),
             "testing_status": get_status_from_qs(testing_scripts),
             "testing_status_tooltip": (
-                handler.dehydrate_hardware_status_tooltip(testing_scripts)),
-            "current_testing_script_set": node.current_testing_script_set_id,
+                handler.dehydrate_hardware_status_tooltip(
+                    testing_scripts)),
             "current_installation_script_set": (
                 node.current_installation_script_set_id),
             "installation_status": (
                 handler.dehydrate_script_set_status(
                     node.current_installation_script_set)),
-            "has_logs": (
-                log_results.difference(script_output_nsmap.keys()) == set()),
+            "has_logs": (log_results.difference(
+                script_output_nsmap.keys()) == set()),
             "locked": node.locked,
             "cpu_count": node.cpu_count,
             "cpu_speed": node.cpu_speed,
@@ -231,7 +237,7 @@ class TestMachineHandler(MAASServerTestCase):
             "supported_filesystems": [
                 {'key': key, 'ui': ui}
                 for key, ui in FILESYSTEM_FORMAT_TYPE_CHOICES],
-            "distro_series": node.get_distro_series(),
+            "distro_series": node.distro_series,
             "error": node.error,
             "error_description": node.error_description,
             "events": handler.dehydrate_events(node),
@@ -251,10 +257,9 @@ class TestMachineHandler(MAASServerTestCase):
             "license_key": node.license_key,
             "link_type": NODE_TYPE_TO_LINK_TYPE[node.node_type],
             "memory": node.display_memory(),
-            "metadata": {},
             "node_type_display": node.get_node_type_display(),
             "min_hwe_kernel": node.min_hwe_kernel,
-            "osystem": node.get_osystem(),
+            "osystem": node.osystem,
             "owner": handler.dehydrate_owner(node.owner),
             "power_parameters": handler.dehydrate_power_parameters(
                 node.power_parameters),
@@ -292,7 +297,6 @@ class TestMachineHandler(MAASServerTestCase):
             "zone": handler.dehydrate_zone(node.zone),
             "pool": handler.dehydrate_pool(node.pool),
             "default_user": node.default_user,
-            "dhcp_on": node.interface_set.filter(vlan__dhcp_on=True).exists(),
         }
         bmc = node.bmc
         if bmc is not None and bmc.bmc_type == BMC_TYPE.POD:
@@ -333,6 +337,13 @@ class TestMachineHandler(MAASServerTestCase):
             for key in list(data):
                 if key not in allowed_fields:
                     del data[key]
+        else:
+            data.update({
+                "dhcp_on": node.interface_set.filter(
+                    vlan__dhcp_on=True).exists(),
+                "grouped_storages": handler.get_grouped_storages(blockdevices),
+                "metadata": {},
+            })
 
         cpu_script_results = [
             script_result for script_result in
@@ -395,8 +406,6 @@ class TestMachineHandler(MAASServerTestCase):
         else:
             data["status_tooltip"] = ""
 
-        data["grouped_storages"] = handler.get_grouped_storages(blockdevices)
-
         return data
 
     def make_nodes(self, number):
@@ -495,10 +504,39 @@ class TestMachineHandler(MAASServerTestCase):
         # 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, 11,
+            queries_one, 8,
+            "Number of queries has changed; make sure this is expected.")
+        self.assertEqual(
+            queries_total, 8,
             "Number of queries has changed; make sure this is expected.")
+
+    def test_get_num_queries_is_the_expected_number(self):
+        owner = factory.make_User()
+        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, _ = count_queries(handler.get, {'system_id': node.system_id})
+        # This check is to notify the developer that a change was made that
+        # affects the number of queries performed when doing a node get.
+        # 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_total, 11,
+            queries, 48,
             "Number of queries has changed; make sure this is expected.")
 
     def test_trigger_update_updates_script_result_cache(self):
@@ -521,6 +559,7 @@ class TestMachineHandler(MAASServerTestCase):
 
         handler = MachineHandler(owner, {})
         # Simulate a trigger pushing an update to the UI
+        handler.cache = {'active_pk': node.system_id}
         _, _, ret = handler.on_listen_for_active_pk(
             'update', node.system_id, node)
         self.assertEquals(ret['commissioning_script_count'], 10)
diff --git a/src/maasserver/websockets/handlers/tests/test_switch.py b/src/maasserver/websockets/handlers/tests/test_switch.py
index 6313a5f..d3f4689 100644
--- a/src/maasserver/websockets/handlers/tests/test_switch.py
+++ b/src/maasserver/websockets/handlers/tests/test_switch.py
@@ -5,10 +5,7 @@
 
 __all__ = []
 
-from maasserver.enum import (
-    NODE_METADATA,
-    NODE_TYPE,
-)
+from maasserver.enum import NODE_TYPE
 from maasserver.exceptions import NodeActionError
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASTransactionServerTestCase
@@ -67,22 +64,6 @@ class TestSwitchHandler(MAASTransactionServerTestCase):
             [result['system_id'] for result in handler.list({})])
 
     @transactional
-    def test_list_switches_includes_metadata(self):
-        owner = factory.make_User()
-        handler = SwitchHandler(owner, {})
-        machine = factory.make_Machine(owner=owner)
-        metadata = {
-            NODE_METADATA.VENDOR_NAME: "Canonical",
-            NODE_METADATA.PHYSICAL_MODEL_NAME: "Cloud-in-a-box"
-        }
-        for key, value in metadata.items():
-            factory.make_NodeMetadata(node=machine, key=key, value=value)
-        factory.make_Switch(node=machine)
-        self.assertItemsEqual(
-            [metadata],
-            [result['metadata'] for result in handler.list({})])
-
-    @transactional
     def test_list_ignores_nodes_that_arent_switches(self):
         owner = factory.make_User()
         handler = SwitchHandler(owner, {})
diff --git a/src/maasserver/websockets/handlers/user.py b/src/maasserver/websockets/handlers/user.py
index 8c10046..cc8759a 100644
--- a/src/maasserver/websockets/handlers/user.py
+++ b/src/maasserver/websockets/handlers/user.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 user handler for the WebSocket connection."""
@@ -35,9 +35,9 @@ class UserHandler(Handler):
             "user",
         ]
 
-    def get_queryset(self):
+    def get_queryset(self, for_list=False):
         """Return `QuerySet` for users only viewable by `user`."""
-        users = super(UserHandler, self).get_queryset()
+        users = super(UserHandler, self).get_queryset(for_list=for_list)
         if reload_object(self.user).is_superuser:
             # Super users can view all users, except for the built-in users
             return users.exclude(username__in=SYSTEM_USERS)
diff --git a/src/maasserver/websockets/tests/test_base.py b/src/maasserver/websockets/tests/test_base.py
index 9a3001f..ee71feb 100644
--- a/src/maasserver/websockets/tests/test_base.py
+++ b/src/maasserver/websockets/tests/test_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).
 
 """Tests for `maasserver.websockets.base`"""
@@ -337,6 +337,25 @@ class TestHandler(MAASServerTestCase):
             HandlerDoesNotExistError,
             handler.get_object, {"system_id": machine.system_id})
 
+    def test_get_queryset(self):
+        queryset = MagicMock()
+        list_queryset = MagicMock()
+        handler = make_handler(
+            "TestHandler", queryset=queryset, list_queryset=list_queryset)
+        self.assertEqual(queryset, handler.get_queryset())
+
+    def test_get_queryset_list(self):
+        queryset = MagicMock()
+        list_queryset = MagicMock()
+        handler = make_handler(
+            "TestHandler", queryset=queryset, list_queryset=list_queryset)
+        self.assertEqual(list_queryset, handler.get_queryset(for_list=True))
+
+    def test_get_queryset_list_only_if_avail(self):
+        queryset = MagicMock()
+        handler = make_handler("TestHandler", queryset=queryset)
+        self.assertEqual(queryset, handler.get_queryset(for_list=True))
+
     def test_execute_only_allows_meta_allowed_methods(self):
         handler = self.make_nodes_handler(allowed_methods=['list'])
         with ExpectedException(HandlerNoSuchMethodError):