Merge ~troyanov/maas:node-release-scriptset into maas:master

[Anton Troyanov]

feat: machine release scripts

[MAAS Lander (maas-lander)]

UNIT TESTS
-b node-release-scriptset lp:~troyanov/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 5672c8464f22aa4de1ee90d7bcb88fcf8cf6e02d

review: Approve

[Christian Grabowski (cgrabowski)]

One minor nit inline, otherwise looks good, +1

review: Approve

diff --git a/src/maasserver/api/machines.py b/src/maasserver/api/machines.py
index f8cddf0..2c0a116 100644
--- a/src/maasserver/api/machines.py
+++ b/src/maasserver/api/machines.py
@@ -973,12 +973,23 @@ class MachineHandler(NodeHandler, WorkloadAnnotationsMixin, PowerMixin):
             # postcondition is achieved, so call this success.
             pass
         elif machine.status in RELEASABLE_STATUSES:
-            machine.release_or_erase(
-                request.user,
-                form.cleaned_data["comment"],
-                erase=form.cleaned_data["erase"],
-                secure_erase=form.cleaned_data["secure_erase"],
-                quick_erase=form.cleaned_data["quick_erase"],
+            scripts = form.cleaned_data["scripts"]
+
+            params = {}
+            if form.cleaned_data["erase"]:
+                params["wipe-disks"] = {}
+                params["wipe-disks"]["secure_erase"] = form.cleaned_data[
+                    "secure_erase"
+                ]
+                params["wipe-disks"]["quick_erase"] = form.cleaned_data[
+                    "quick_erase"
+                ]
+            params = params | form.get_script_param_dict(scripts)
+            machine.start_releasing(
+                user=request.user,
+                comment=form.cleaned_data["comment"],
+                scripts=scripts,
+                script_input=params,
                 force=form.cleaned_data["force"],
             )
         else:
@@ -2225,7 +2236,10 @@ class MachinesHandler(NodesHandler, PowersMixin):
                 # Nothing to do.
                 pass
             elif machine.status in RELEASABLE_STATUSES:
-                machine.release_or_erase(request.user, comment)
+                machine.start_releasing(
+                    user=request.user,
+                    comment=comment,
+                )
                 released_ids.append(machine.system_id)
             else:
                 failed.append(
diff --git a/src/maasserver/api/tests/test_machine.py b/src/maasserver/api/tests/test_machine.py
index f5a2b33..42b3511 100644
--- a/src/maasserver/api/tests/test_machine.py
+++ b/src/maasserver/api/tests/test_machine.py
@@ -1683,17 +1683,16 @@ class TestMachineAPI(APITestCase.ForUser):
         )
         self.become_admin()
         comment = factory.make_name("comment")
-        machine_release = self.patch(node_module.Machine, "release_or_erase")
+        machine_release = self.patch(node_module.Machine, "start_releasing")
         self.client.post(
             self.get_machine_uri(machine),
             {"op": "release", "comment": comment},
         )
         machine_release.assert_called_once_with(
-            self.user,
-            comment,
-            erase=False,
-            quick_erase=False,
-            secure_erase=False,
+            user=self.user,
+            comment=comment,
+            scripts=[],
+            script_input={},
             force=False,
         )
 
@@ -1707,7 +1706,7 @@ class TestMachineAPI(APITestCase.ForUser):
         self.become_admin()
         secure_erase = factory.pick_bool()
         quick_erase = factory.pick_bool()
-        machine_release = self.patch(node_module.Machine, "release_or_erase")
+        machine_release = self.patch(node_module.Machine, "start_releasing")
         self.client.post(
             self.get_machine_uri(machine),
             {
@@ -1718,11 +1717,15 @@ class TestMachineAPI(APITestCase.ForUser):
             },
         )
         machine_release.assert_called_once_with(
-            self.user,
-            "",
-            erase=True,
-            quick_erase=quick_erase,
-            secure_erase=secure_erase,
+            user=self.user,
+            comment="",
+            scripts=["wipe-disks"],
+            script_input={
+                "wipe-disks": {
+                    "secure_erase": secure_erase,
+                    "quick_erase": quick_erase,
+                }
+            },
             force=False,
         )
 
@@ -1735,16 +1738,15 @@ class TestMachineAPI(APITestCase.ForUser):
         )
         self.become_admin()
         force = factory.pick_bool()
-        machine_release = self.patch(node_module.Machine, "release_or_erase")
+        machine_release = self.patch(node_module.Machine, "start_releasing")
         self.client.post(
             self.get_machine_uri(machine), {"op": "release", "force": force}
         )
         machine_release.assert_called_once_with(
-            self.user,
-            "",
-            erase=False,
-            quick_erase=False,
-            secure_erase=False,
+            user=self.user,
+            comment="",
+            scripts=[],
+            script_input={},
             force=force,
         )
 
@@ -1756,14 +1758,13 @@ class TestMachineAPI(APITestCase.ForUser):
             power_state=POWER_STATE.OFF,
         )
         self.become_admin()
-        machine_release = self.patch(node_module.Machine, "release_or_erase")
+        machine_release = self.patch(node_module.Machine, "start_releasing")
         self.client.post(self.get_machine_uri(machine), {"op": "release"})
         machine_release.assert_called_once_with(
-            self.user,
-            "",
-            erase=False,
-            quick_erase=False,
-            secure_erase=False,
+            user=self.user,
+            comment="",
+            scripts=[],
+            script_input={},
             force=False,
         )
 
diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index 0e66b44..e34f5d4 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -19,6 +19,7 @@ from itertools import chain, count
 import json
 import logging
 from operator import attrgetter
+import os
 import random
 import re
 import socket
@@ -174,7 +175,11 @@ from metadataserver.enum import (
     SCRIPT_STATUS_FAILED,
     SCRIPT_STATUS_RUNNING_OR_PENDING,
 )
-from metadataserver.user_data import generate_user_data_for_status
+from metadataserver.user_data import (
+    generate_user_data,
+    generate_user_data_for_status,
+)
+from metadataserver.user_data.snippets import get_userdata_template_dir
 from provisioningserver.drivers.osystem import BOOT_IMAGE_PURPOSE
 from provisioningserver.drivers.pod import Capabilities
 from provisioningserver.drivers.power.ipmi import IPMI_BOOT_TYPE
@@ -3623,6 +3628,144 @@ class Node(CleanSave, TimestampedModel):
                 % (self.system_id, NODE_STATUS_CHOICES_DICT[self.status])
             )
 
+    def start_releasing(
+        self, user, comment=None, scripts=None, script_input=None, force=False
+    ):
+        """Start the release process for the machine."""
+        from maasserver.models import ScriptSet
+
+        if scripts is None:
+            scripts = []
+
+        self.maybe_delete_pods(not force)
+
+        config = Config.objects.get_configs(
+            [
+                "commissioning_osystem",
+                "commissioning_distro_series",
+                "default_osystem",
+                "default_distro_series",
+                "disk_erase_with_secure_erase",
+                "disk_erase_with_quick_erase",
+                "enable_disk_erasing_on_release",
+            ]
+        )
+
+        if config.get("enable_disk_erasing_on_release"):
+            scripts.append("wipe-disks")
+
+        if not scripts:
+            self.release(user, comment)
+            return
+
+        self.current_release_script_set = (
+            ScriptSet.objects.create_release_script_set(
+                self, scripts=scripts, script_input=script_input
+            )
+        )
+        self.save()
+
+        template_file = os.path.join(
+            get_userdata_template_dir(), "script_runner.template"
+        )
+        user_data = generate_user_data(self, template_file)
+
+        failed_status = NODE_STATUS.FAILED_RELEASING
+        # Before machine release scripts were introduced there was Erase Disk
+        # functionality. We want to maintain backward compatible statuses
+        # to be reported when only "wipe-disk" script is executed.
+        if len(scripts) == 1 and "wipe-disks" in scripts:
+            self._register_request_event(
+                user,
+                EVENT_TYPES.REQUEST_NODE_ERASE_DISK,
+                action="start disk erasing",
+                comment=comment,
+            )
+            old_status = self.update_status(NODE_STATUS.DISK_ERASING)
+            failed_status = NODE_STATUS.FAILED_DISK_ERASING
+        else:
+            self._register_request_event(
+                user,
+                EVENT_TYPES.REQUEST_NODE_RELEASE,
+                action="start node releasing",
+                comment=comment,
+            )
+
+            old_status = self.update_status(NODE_STATUS.RELEASING)
+
+        self.save()
+
+        try:
+            # Node.start() has synchronous and asynchronous parts, so catch
+            # exceptions arising synchronously, and chain callbacks to the
+            # Deferred it returns for the asynchronous (post-commit) bits.
+            starting = self._start(
+                user,
+                user_data,
+                old_status,
+                allow_power_cycle=True,
+                config=config,
+            )
+        except Exception as error:
+            # We always mark the node as failed here, although we could
+            # potentially move it back to the state it was in previously. For
+            # now, though, this is safer, since it marks the node as needing
+            # attention.
+            self.update_status(failed_status)
+            self.save()
+            maaslog.error(
+                f"{self.hostname}: Could not start node for release: {error}"
+            )
+            # Let the exception bubble up, since the UI or API will have to
+            # deal with it.
+            raise
+        else:
+            # Don't permit naive mocking of start(); it causes too much
+            # confusion when testing. Return a Deferred from side_effect.
+            assert isinstance(starting, Deferred) or starting is None
+
+            if starting is None:
+                starting = post_commit()
+                # MAAS cannot start the node itself.
+                is_starting = False
+            else:
+                # MAAS can direct the node to start.
+                is_starting = True
+
+            @asynchronous
+            def async_start(is_starting, hostname):
+                if is_starting:
+                    maaslog.info(f"{hostname}: Release started")
+                else:
+                    maaslog.warning(
+                        f"{hostname}: Could not start node for release; it "
+                        "must be started manually",
+                    )
+
+            starting.addCallback(
+                callOut,
+                async_start,
+                is_starting,
+                self.hostname,
+            )
+
+            # If there's an error, reset the node's status.
+            starting.addErrback(
+                callOutToDatabase,
+                Node._set_status,
+                self.system_id,
+                status=failed_status,
+            )
+
+            def eb_start(failure, hostname):
+                maaslog.error(
+                    f"{hostname}: Could not start node for "
+                    f"releasing: {failure.getErrorMessage()}",
+                )
+                return failure  # Propagate.
+
+            return starting.addErrback(eb_start, self.hostname)
+
     def release(self, user=None, comment=None):
         self._register_request_event(
             user,
@@ -3758,31 +3901,6 @@ class Node(CleanSave, TimestampedModel):
             # Remove all set owner data.
             OwnerData.objects.filter(node=self).delete()
 
-    def release_or_erase(
-        self,
-        user,
-        comment=None,
-        erase=False,
-        secure_erase=None,
-        quick_erase=None,
-        force=False,
-    ):
-        """Either release the node or erase the node then release it, depending
-        on settings and parameters."""
-        self.maybe_delete_pods(not force)
-        erase_on_release = Config.objects.get_config(
-            "enable_disk_erasing_on_release"
-        )
-        if erase or erase_on_release:
-            self.start_disk_erasing(
-                user,
-                comment,
-                secure_erase=secure_erase,
-                quick_erase=quick_erase,
-            )
-        else:
-            self.release(user, comment)
-
     def maybe_delete_pods(self, dry_run: bool):
         """Check if any pods are associated with this Node.
 
diff --git a/src/maasserver/models/tests/test_node.py b/src/maasserver/models/tests/test_node.py
index 7706930..6195c28 100644
--- a/src/maasserver/models/tests/test_node.py
+++ b/src/maasserver/models/tests/test_node.py
@@ -22,6 +22,7 @@ from fixtures import LoggerFixture
 from netaddr import IPAddress, IPNetwork
 from testscenarios import multiply_scenarios
 from twisted.internet import defer
+from twisted.internet.defer import succeed
 from twisted.internet.error import ConnectionDone
 import yaml
 
@@ -7273,44 +7274,40 @@ class TestNodeManagerGetNodesRBAC(MAASServerTestCase):
 
 
 class TestNodeErase(MAASServerTestCase):
-    def test_release_or_erase_erases_when_enabled(self):
+    def test_start_releasing_erases_when_enabled(self):
         owner = factory.make_User()
         node = factory.make_Node(status=NODE_STATUS.ALLOCATED, owner=owner)
         Config.objects.set_config("enable_disk_erasing_on_release", True)
-        erase_mock = self.patch_autospec(node, "start_disk_erasing")
         release_mock = self.patch_autospec(node, "release")
-        node.release_or_erase(owner)
-        erase_mock.assert_called_once_with(
-            owner, None, secure_erase=None, quick_erase=None
-        )
+        self.patch(node, "_start").return_value = succeed(None)
+        node.start_releasing(owner)
         release_mock.assert_not_called()
 
-    def test_release_or_erase_erases_when_disabled_and_erase_param(self):
+    def test_start_releasing_erases_when_disabled_and_erase_param(self):
         owner = factory.make_User()
         node = factory.make_Node(status=NODE_STATUS.ALLOCATED, owner=owner)
         Config.objects.set_config("enable_disk_erasing_on_release", False)
-        erase_mock = self.patch_autospec(node, "start_disk_erasing")
         release_mock = self.patch_autospec(node, "release")
+        self.patch(node, "_start").return_value = succeed(None)
         secure_erase = factory.pick_bool()
         quick_erase = factory.pick_bool()
-        node.release_or_erase(
-            owner,
-            erase=True,
-            secure_erase=secure_erase,
-            quick_erase=quick_erase,
-        )
-        erase_mock.assert_called_once_with(
-            owner, None, secure_erase=secure_erase, quick_erase=quick_erase
+        node.start_releasing(
+            user=owner,
+            scripts=["wipe-disks"],
+            script_input={
+                "secure_erase": secure_erase,
+                "quick_erase": quick_erase,
+            },
         )
         release_mock.assert_not_called()
 
-    def test_release_or_erase_releases_when_disabled(self):
+    def test_start_releasing_releases_when_disabled(self):
         owner = factory.make_User()
         node = factory.make_Node(status=NODE_STATUS.ALLOCATED, owner=owner)
         Config.objects.set_config("enable_disk_erasing_on_release", False)
         erase_mock = self.patch_autospec(node, "start_disk_erasing")
         release_mock = self.patch_autospec(node, "release")
-        node.release_or_erase(owner)
+        node.start_releasing(owner)
         release_mock.assert_called_once_with(owner, None)
         erase_mock.assert_not_called()
 
diff --git a/src/maasserver/node_action.py b/src/maasserver/node_action.py
index 0c19d9e..ec7a87d 100644
--- a/src/maasserver/node_action.py
+++ b/src/maasserver/node_action.py
@@ -728,11 +728,22 @@ class Release(NodeAction):
         if not form.is_valid():
             raise NodeActionError(form.errors)
         try:
-            self.node.release_or_erase(
-                self.user,
-                erase=form.cleaned_data["erase"],
-                secure_erase=form.cleaned_data["secure_erase"],
-                quick_erase=form.cleaned_data["quick_erase"],
+            scripts = []
+            params = {}
+            if form.cleaned_data["erase"]:
+                scripts.append("wipe-disks")
+                params["wipe-disks"] = {}
+                params["wipe-disks"]["secure_erase"] = form.cleaned_data[
+                    "secure_erase"
+                ]
+                params["wipe-disks"]["quick_erase"] = form.cleaned_data[
+                    "quick_erase"
+                ]
+            params = params | form.get_script_param_dict(scripts)
+            self.node.start_releasing(
+                user=self.user,
+                scripts=scripts,
+                script_input=params,
             )
         except RPC_EXCEPTIONS + (ExternalProcessError,) as exception:
             raise NodeActionError(exception)
diff --git a/src/maasserver/node_status.py b/src/maasserver/node_status.py
index 3a344ef..4ea19f1 100644
--- a/src/maasserver/node_status.py
+++ b/src/maasserver/node_status.py
@@ -312,6 +312,7 @@ COMMISSIONING_LIKE_STATUSES = [
     NODE_STATUS.COMMISSIONING,
     NODE_STATUS.DISK_ERASING,
     NODE_STATUS.ENTERING_RESCUE_MODE,
+    NODE_STATUS.RELEASING,
     NODE_STATUS.RESCUE_MODE,
     NODE_STATUS.TESTING,
 ]
diff --git a/src/maasserver/tests/test_node_action.py b/src/maasserver/tests/test_node_action.py
index 41e0a04..b04105e 100644
--- a/src/maasserver/tests/test_node_action.py
+++ b/src/maasserver/tests/test_node_action.py
@@ -1555,15 +1555,19 @@ class TestReleaseAction(MAASServerTestCase):
             owner=user,
             power_parameters=params,
         )
-        node_release_or_erase = self.patch_autospec(node, "release_or_erase")
+        node_start_releasing = self.patch_autospec(node, "start_releasing")
 
         with post_commit_hooks:
             Release(node, user, request).execute(
                 erase=True, secure_erase=True, quick_erase=True
             )
 
-        node_release_or_erase.assert_called_once_with(
-            user, erase=True, secure_erase=True, quick_erase=True
+        node_start_releasing.assert_called_once_with(
+            user=user,
+            scripts=["wipe-disks"],
+            script_input={
+                "wipe-disks": {"secure_erase": True, "quick_erase": True}
+            },
         )
 
 
diff --git a/src/metadataserver/api.py b/src/metadataserver/api.py
index d2394c2..35f9a9e 100644
--- a/src/metadataserver/api.py
+++ b/src/metadataserver/api.py
@@ -519,6 +519,7 @@ class VersionIndexHandler(MetadataViewHandler):
         NODE_STATUS.FAILED_DEPLOYMENT,
         NODE_STATUS.READY,
         NODE_STATUS.DISK_ERASING,
+        NODE_STATUS.RELEASING,
         NODE_STATUS.ENTERING_RESCUE_MODE,
         NODE_STATUS.RESCUE_MODE,
         NODE_STATUS.FAILED_ENTERING_RESCUE_MODE,
@@ -531,6 +532,7 @@ class VersionIndexHandler(MetadataViewHandler):
         NODE_STATUS.COMMISSIONING,
         NODE_STATUS.DEPLOYING,
         NODE_STATUS.DISK_ERASING,
+        NODE_STATUS.RELEASING,
         NODE_STATUS.ENTERING_RESCUE_MODE,
         NODE_STATUS.RESCUE_MODE,
         NODE_STATUS.TESTING,
@@ -789,6 +791,13 @@ class VersionIndexHandler(MetadataViewHandler):
         self._store_results(
             node, node.current_release_script_set, request, status
         )
+
+        if status == SIGNAL_STATUS.OK:
+            node.release()
+
+        if status == SIGNAL_STATUS.FAILED:
+            node.mark_failed(comment="Failed to release machine.")
+
         return None
 
     @operation(idempotent=False)
diff --git a/src/metadataserver/api_twisted.py b/src/metadataserver/api_twisted.py
index ca432fa..a5c4f9e 100644
--- a/src/metadataserver/api_twisted.py
+++ b/src/metadataserver/api_twisted.py
@@ -503,6 +503,12 @@ class StatusWorkerService(TimerService):
                         comment="Failed to erase disks.", commit=False
                     )
                     save_node = True
+            elif node.status == NODE_STATUS.RELEASING:
+                if failed:
+                    node.mark_failed(
+                        comment="Failed to release machine.", commit=False
+                    )
+                    save_node = True
             # Deallocate the node if we enter any terminal state.
             if node.node_type == NODE_TYPE.MACHINE and node.status in [
                 NODE_STATUS.READY,