sts-sponsors team mailing list archive

[Alexsander de Souza <mp+433897@xxxxxxxxxxxxxxxxxx>]

Alexsander de Souza has proposed merging ~alexsander-souza/maas:bulk_node_actions into maas:master.

Commit message:
improve OverrideFailedTesting Action

allow suppressing failed scripts results 

Requested reviews:
  MAAS Lander (maas-lander): unittests
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~alexsander-souza/maas/+git/maas/+merge/433897

Also fixes:

- don't compile all actions when we know which one we want
- use the queryset with annotations
-- 
Your team MAAS Maintainers is requested to review the proposed merge of ~alexsander-souza/maas:bulk_node_actions into maas:master.

diff --git a/src/maasserver/forms/ephemeral.py b/src/maasserver/forms/ephemeral.py
index be17d7b..893ef37 100644
--- a/src/maasserver/forms/ephemeral.py
+++ b/src/maasserver/forms/ephemeral.py
@@ -19,7 +19,7 @@ from django.forms import (
 from django.http import QueryDict
 
 from maasserver.enum import NODE_STATUS
-from maasserver.node_action import compile_node_actions
+from maasserver.node_action import get_node_action
 from maasserver.utils.forms import set_form_error
 from metadataserver.enum import SCRIPT_TYPE
 from metadataserver.models import Script
@@ -160,8 +160,7 @@ class TestForm(Form):
         self._set_up_script_fields()
 
     def clean(self):
-        actions = compile_node_actions(self.instance, self.user)
-        action = actions.get(self._name)
+        action = get_node_action(self.instance, self._name, self.user)
         if action is None:
             raise ValidationError(
                 "%s is not available because of the current state of the node."
diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index e223aad..e1c6923 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -176,6 +176,7 @@ from maasserver.worker_user import get_worker_user
 from metadataserver.enum import (
     RESULT_TYPE,
     SCRIPT_STATUS,
+    SCRIPT_STATUS_FAILED,
     SCRIPT_STATUS_RUNNING_OR_PENDING,
 )
 from metadataserver.user_data import generate_user_data_for_status
@@ -3965,6 +3966,38 @@ class Node(CleanSave, TimestampedModel):
         self.current_installation_script_set = None
         self.save()
 
+    def get_latest_failed_testing_script_results(self) -> List[int]:
+        from metadataserver.models import ScriptResult
+
+        script_results = (
+            ScriptResult.objects.filter(
+                script_set__node__system_id=self.system_id,
+                script_set__result_type=RESULT_TYPE.TESTING,
+            )
+            .values(
+                "id",
+                "status",
+                "script_set__node_id",
+                "script_name",
+                "physical_blockdevice_id",
+            )
+            .order_by(
+                "script_set__node_id",
+                "script_name",
+                "physical_blockdevice_id",
+                "-id",
+            )
+            .distinct(
+                "script_set__node_id", "script_name", "physical_blockdevice_id"
+            )
+        )
+        node_script_results = [
+            s["id"]
+            for s in script_results
+            if s["status"] in SCRIPT_STATUS_FAILED
+        ]
+        return node_script_results
+
     def override_failed_testing(self, user, comment=None):
         """Reset a node with failed tests into a working state."""
         self._register_request_event(
diff --git a/src/maasserver/node_action.py b/src/maasserver/node_action.py
index 2306403..d60da4b 100644
--- a/src/maasserver/node_action.py
+++ b/src/maasserver/node_action.py
@@ -50,6 +50,7 @@ from maasserver.utils.osystems import (
     validate_osystem_and_distro_series,
 )
 from metadataserver.enum import SCRIPT_STATUS
+from metadataserver.models.scriptresult import ScriptResult
 from provisioningserver.events import EVENT_TYPES
 from provisioningserver.rpc.exceptions import (
     NoConnectionsAvailable,
@@ -851,8 +852,16 @@ class OverrideFailedTesting(NodeAction):
         """Retrieve the node action audit description."""
         return self.audit_description % action.node.hostname
 
-    def _execute(self):
+    def _execute(self, suppress_failed_script_results=False):
         """See `NodeAction.execute`."""
+        if suppress_failed_script_results:
+            script_result_ids = (
+                self.node.get_latest_failed_testing_script_results()
+            )
+            ScriptResult.objects.filter(
+                script_set__node__system_id=self.node.system_id,
+                id__in=script_result_ids,
+            ).update(suppressed=True)
         self.node.override_failed_testing(self.user, "via web interface")
 
 
@@ -1104,6 +1113,23 @@ ACTION_CLASSES = (
 ACTIONS_DICT = OrderedDict((action.name, action) for action in ACTION_CLASSES)
 
 
+def get_node_action(node, action_name, user, request=None):
+    """Get Action for node, if actionable
+
+    :param node: The :class:`Node` that the request pertains to.
+    :param action_name: The action name.
+    :param user: The :class:`User` making the request.
+    :param request: The :class:`HttpRequest` being serviced.  It may be used
+        to obtain information about the OAuth token being used.
+    :return: An Action object if it's actionable, None otherwise
+    """
+    act_cls = ACTIONS_DICT.get(action_name)
+    if act_cls is None:
+        return None
+    action = act_cls(node, user, request)
+    return action if action.is_actionable() else None
+
+
 def compile_node_actions(node, user, request=None, classes=ACTION_CLASSES):
     """Provide :class:`NodeAction` objects for given request.
 
diff --git a/src/maasserver/tests/test_node_action.py b/src/maasserver/tests/test_node_action.py
index dca8ccf..c30a7ab 100644
--- a/src/maasserver/tests/test_node_action.py
+++ b/src/maasserver/tests/test_node_action.py
@@ -71,7 +71,12 @@ from maasserver.testing.testcase import (
 from maasserver.utils.orm import post_commit, post_commit_hooks, reload_object
 from maastesting.matchers import MockCalledOnce, MockCalledOnceWith
 from metadataserver.builtin_scripts import load_builtin_scripts
-from metadataserver.enum import RESULT_TYPE, SCRIPT_STATUS, SCRIPT_TYPE
+from metadataserver.enum import (
+    RESULT_TYPE,
+    SCRIPT_STATUS,
+    SCRIPT_STATUS_FAILED,
+    SCRIPT_TYPE,
+)
 from metadataserver.models import ScriptSet
 from provisioningserver.events import AUDIT
 from provisioningserver.utils.shell import ExternalProcessError
@@ -1634,6 +1639,32 @@ class TestMarkFixedAction(MAASServerTestCase):
 
 
 class TestOverrideFailedTesting(MAASServerTestCase):
+    def create_script_results(self, nodes, count=5):
+        script_results = []
+        for node in nodes:
+            script = factory.make_Script()
+            for _ in range(count):
+                script_set = factory.make_ScriptSet(
+                    result_type=script.script_type, node=node
+                )
+                factory.make_ScriptResult(script=script, script_set=script_set)
+
+            script_set = factory.make_ScriptSet(
+                result_type=script.script_type, node=node
+            )
+            script_result = factory.make_ScriptResult(
+                script=script,
+                script_set=script_set,
+                status=random.choice(
+                    list(SCRIPT_STATUS_FAILED.union({SCRIPT_STATUS.PASSED}))
+                ),
+            )
+            if script.script_type == SCRIPT_TYPE.TESTING and (
+                script_result.status in SCRIPT_STATUS_FAILED
+            ):
+                script_results.append(script_result)
+        return script_results
+
     def test_ignore_tests_sets_status_to_ready(self):
         owner = factory.make_admin()
         request = factory.make_fake_request("/")
@@ -1645,9 +1676,10 @@ class TestOverrideFailedTesting(MAASServerTestCase):
             error_description=description,
             osystem="",
         )
+        failed_scripts = self.create_script_results(nodes=[node])
         action = OverrideFailedTesting(node, owner, request)
         self.assertTrue(action.is_permitted())
-        action.execute()
+        action.execute(suppress_failed_script_results=True)
         node = reload_object(node)
         self.assertEqual(NODE_STATUS.READY, node.status)
         self.assertEqual("", node.osystem)
@@ -1657,6 +1689,9 @@ class TestOverrideFailedTesting(MAASServerTestCase):
             audit_event.description,
             "Overrode failed testing on '%s'." % node.hostname,
         )
+        for script_result in failed_scripts:
+            script_result = reload_object(script_result)
+            self.assertTrue(script_result.suppressed)
 
     def test_ignore_tests_sets_status_to_deployed(self):
         owner = factory.make_admin()
diff --git a/src/maasserver/websockets/handlers/controller.py b/src/maasserver/websockets/handlers/controller.py
index 87b575e..0e54ff9 100644
--- a/src/maasserver/websockets/handlers/controller.py
+++ b/src/maasserver/websockets/handlers/controller.py
@@ -22,7 +22,7 @@ from maasserver.exceptions import NodeActionError
 from maasserver.forms import ControllerForm
 from maasserver.models import Controller, Event, RackController, VLAN
 from maasserver.models.controllerinfo import get_target_version
-from maasserver.node_action import compile_node_actions
+from maasserver.node_action import get_node_action
 from maasserver.permissions import NodePermission
 from maasserver.secrets import SecretManager
 from maasserver.websockets.base import (
@@ -154,12 +154,13 @@ class ControllerHandler(NodeHandler):
 
     def action(self, params):
         """Perform the action on the object."""
-        # `compile_node_actions` handles the permission checking internally
+        # `get_node_action` handles the permission checking internally
         # the default view permission check is enough at this level.
         obj = self.get_object(params)
         action_name = params.get("action")
-        actions = compile_node_actions(obj, self.user, request=self.request)
-        action = actions.get(action_name)
+        action = get_node_action(
+            obj, action_name, self.user, request=self.request
+        )
         if action is None:
             raise NodeActionError(
                 f"{action_name} action is not available for this node."
diff --git a/src/maasserver/websockets/handlers/device.py b/src/maasserver/websockets/handlers/device.py
index ac00df7..4d98b8c 100644
--- a/src/maasserver/websockets/handlers/device.py
+++ b/src/maasserver/websockets/handlers/device.py
@@ -18,7 +18,7 @@ from maasserver.models.interface import Interface
 from maasserver.models.node import Device
 from maasserver.models.staticipaddress import StaticIPAddress
 from maasserver.models.subnet import Subnet
-from maasserver.node_action import compile_node_actions
+from maasserver.node_action import get_node_action
 from maasserver.permissions import NodePermission
 from maasserver.utils.orm import reload_object
 from maasserver.websockets.base import HandlerError, HandlerValidationError
@@ -418,8 +418,9 @@ class DeviceHandler(NodeHandler):
         """Perform the action on the object."""
         obj = self.get_object(params, permission=self._meta.edit_permission)
         action_name = params.get("action")
-        actions = compile_node_actions(obj, self.user, request=self.request)
-        action = actions.get(action_name)
+        action = get_node_action(
+            obj, action_name, self.user, request=self.request
+        )
         if action is None:
             raise NodeActionError(
                 "%s action is not available for this device." % action_name
diff --git a/src/maasserver/websockets/handlers/machine.py b/src/maasserver/websockets/handlers/machine.py
index 380616b..d5ae491 100644
--- a/src/maasserver/websockets/handlers/machine.py
+++ b/src/maasserver/websockets/handlers/machine.py
@@ -76,7 +76,7 @@ from maasserver.models import (
     Subnet,
     VolumeGroup,
 )
-from maasserver.node_action import compile_node_actions
+from maasserver.node_action import get_node_action
 from maasserver.permissions import NodePermission
 from maasserver.storage_layouts import (
     StorageLayoutError,
@@ -94,7 +94,6 @@ from maasserver.websockets.base import (
 )
 from maasserver.websockets.handlers.node import node_prefetch, NodeHandler
 from metadataserver.enum import HARDWARE_TYPE, RESULT_TYPE
-from metadataserver.models.scriptresult import ScriptResult
 from provisioningserver.certificates import Certificate
 from provisioningserver.logger import LegacyLogger
 from provisioningserver.rpc.exceptions import UnknownPowerType
@@ -252,7 +251,6 @@ class MachineHandler(NodeHandler):
             "filter_groups",
             "filter_options",
             "count",
-            "suppress_failed_script_results",
         ]
         form = AdminMachineWithMACAddressesForm
         exclude = [
@@ -1023,8 +1021,9 @@ class MachineHandler(NodeHandler):
             )
 
     def _action(self, obj, action_name, extra_params):
-        actions = compile_node_actions(obj, self.user, request=self.request)
-        action = actions.get(action_name)
+        action = get_node_action(
+            obj, action_name, self.user, request=self.request
+        )
         if action is None:
             raise NodeActionError(
                 f"{action_name} action is not available for this node."
@@ -1035,7 +1034,9 @@ class MachineHandler(NodeHandler):
         self, filter_params, action_name, extra_params
     ) -> tuple[int, list[str]]:
         """Find nodes that match the filter, then apply the given action to them."""
-        machines = self._filter(self._meta.queryset, None, filter_params)
+        machines = self._filter(
+            self.get_queryset(for_list=True), None, filter_params
+        )
         success_count = 0
         failed_system_ids = []
         for machine in machines:
@@ -1053,9 +1054,12 @@ class MachineHandler(NodeHandler):
 
     def _bulk_clone(self, source, filter_params, extra_params):
         """Bulk clone - special case of bulk_action."""
-        actions = compile_node_actions(source, self.user, request=self.request)
-        clone_action = actions.get("clone")
-        destinations = self._filter(self._meta.queryset, None, filter_params)
+        clone_action = get_node_action(
+            source, "clone", self.user, request=self.request
+        )
+        destinations = self._filter(
+            self.get_queryset(for_list=True), None, filter_params
+        )
         destination_ids = [node.system_id for node in destinations]
         return clone_action.execute(
             destinations=destination_ids, **extra_params
@@ -1063,8 +1067,6 @@ class MachineHandler(NodeHandler):
 
     def action(self, params):
         """Perform the action on the object."""
-        # `compile_node_actions` handles the permission checking internally
-        # the default view permission check is enough at this level.
         action_name = params.get("action")
         extra_params = params.get("extra", {})
         if action_name == "clone" and "filter" in params:
@@ -1221,21 +1223,3 @@ class MachineHandler(NodeHandler):
         if "filter" in params:
             qs = self._filter(qs, "list", params["filter"])
         return {"count": qs.count()}
-
-    def suppress_failed_script_results(self, params):
-        qs = self.get_queryset(for_list=True)
-        system_ids = self._filter(
-            qs, "list", params.get("filter")
-        ).values_list("system_id", flat=True)
-        script_results = self._get_latest_failed_testing_script_results(
-            system_ids
-        )
-        script_result_ids = [
-            script_result.id for script_result in script_results
-        ]
-        ScriptResult.objects.filter(
-            id__in=script_result_ids,
-        ).update(suppressed=True)
-        return {
-            "success_count": len(script_result_ids),
-        }
diff --git a/src/maasserver/websockets/handlers/tests/test_machine.py b/src/maasserver/websockets/handlers/tests/test_machine.py
index 5e90c0a..08afc62 100644
--- a/src/maasserver/websockets/handlers/tests/test_machine.py
+++ b/src/maasserver/websockets/handlers/tests/test_machine.py
@@ -5715,16 +5715,6 @@ class TestMachineHandlerSuppressScriptResult(MAASServerTestCase):
             params,
         )
 
-    def test_suppress_failed_script_results(self):
-        owner = factory.make_User()
-        handler = MachineHandler(owner, {}, None)
-        nodes = [factory.make_Node(owner=owner) for _ in range(10)]
-        failed_results = self.create_script_results(nodes)
-        result = handler.suppress_failed_script_results(
-            {"filter": {"owner": owner.username}}
-        )
-        self.assertEqual(len(failed_results), result["success_count"])
-
     def test_set_script_result_unsuppressed(self):
         owner = factory.make_admin()
         node = factory.make_Node(owner=owner)