Merge ~adam-collard/maas:websocket-create-deployed-machine into maas:master

[Adam Collard]

Allow creating a machine as deployed via websocket handler

[Alberto Donato (ack)]

nice!

just one question inline

review: Needs Information

[MAAS Lander (maas-lander)]

UNIT TESTS
-b websocket-create-deployed-machine lp:~adam-collard/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 1633be5330aca1675c83e97634f75ec918c97f5a

review: Approve

[MAAS Lander (maas-lander)]

UNIT TESTS
-b websocket-create-deployed-machine lp:~adam-collard/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/10713/console
COMMIT: 08c2fc6141b4500ebd4c582b27f9218f12989816

review: Needs Fixing

[Alberto Donato (ack)]

+1!

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b websocket-create-deployed-machine lp:~adam-collard/maas/+git/maas into -b master lp:~maas-committers/maas

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

[MAAS Lander (maas-lander)]

UNIT TESTS
-b websocket-create-deployed-machine lp:~adam-collard/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/10715/console
COMMIT: 6de8d707da29ec79c569e46de5b01d9cd4e5a7ec

review: Needs Fixing

e038d4d... by Adam Collard on 2021-08-20

Flaky test - we don't care about the order

diff --git a/src/maasserver/api/machines.py b/src/maasserver/api/machines.py
index fe60680..5c234f2 100644
--- a/src/maasserver/api/machines.py
+++ b/src/maasserver/api/machines.py
@@ -71,10 +71,7 @@ from maasserver.forms import (
     MachineForm,
 )
 from maasserver.forms.clone import CloneForm
-from maasserver.forms.ephemeral import (
-    CommissionForm,
-    CreateScriptsForDeployedForm,
-)
+from maasserver.forms.ephemeral import CommissionForm
 from maasserver.forms.filesystem import (
     MountNonStorageFilesystemForm,
     UnmountNonStorageFilesystemForm,
@@ -1968,6 +1965,10 @@ class MachinesHandler(NodesHandler, PowersMixin):
         time out. Machines created by administrators will be commissioned
         unless set to false.
 
+        @param (boolean) "deployed" [required=false,formatting=true] Request
+        the newly created machine to be created with status set to
+        DEPLOYED.
+
         @param (int) "enable_ssh" [required=false]  Whether to enable SSH for
         the commissioning environment using the user's SSH key(s). '1' == True,
         '0' == False.
@@ -2011,21 +2012,14 @@ class MachinesHandler(NodesHandler, PowersMixin):
             request.data, "deployed", default=False, validator=StringBool
         )
         machine = create_machine(request)
-        if request.user.is_superuser:
-            if deployed:
-                form = CreateScriptsForDeployedForm(
-                    instance=machine,
-                    data=request.data,
-                )
-                form.save()
-            elif commission:
-                form = CommissionForm(
-                    instance=machine, user=request.user, data=request.data
-                )
-                # Silently ignore errors to prevent 500 errors. The commissioning
-                # callbacks have their own logging. This fixes LP:1600328.
-                if form.is_valid():
-                    machine = form.save()
+        if request.user.is_superuser and commission and not deployed:
+            form = CommissionForm(
+                instance=machine, user=request.user, data=request.data
+            )
+            # Silently ignore errors to prevent 500 errors. The commissioning
+            # callbacks have their own logging. This fixes LP:1600328.
+            if form.is_valid():
+                machine = form.save()
 
         return machine
 
diff --git a/src/maasserver/forms/__init__.py b/src/maasserver/forms/__init__.py
index d4d35df..f5feb77 100644
--- a/src/maasserver/forms/__init__.py
+++ b/src/maasserver/forms/__init__.py
@@ -748,6 +748,7 @@ class MachineForm(NodeForm):
             "architecture", choices
         )
         required = requires_macs_and_architecture(self.data) or requires_arch
+        self._need_boot_images = required
         self.fields["architecture"] = forms.ChoiceField(
             choices=choices,
             required=required,
@@ -853,7 +854,7 @@ class MachineForm(NodeForm):
         is_valid = super().is_valid()
         if not is_valid:
             return False
-        if len(list_all_usable_architectures()) == 0:
+        if self._need_boot_images and not list_all_usable_architectures():
             set_form_error(self, "architecture", NO_ARCHITECTURES_AVAILABLE)
             is_valid = False
         return is_valid
@@ -1172,6 +1173,23 @@ class AdminMachineForm(MachineForm, AdminNodeForm, WithPowerTypeMixin):
         cleaned_data = WithPowerTypeMixin.check_driver(self, cleaned_data)
         return cleaned_data
 
+    def _setup_deployed_machine(self, machine):
+        """Configure the Machine before it has been saved."""
+        from metadataserver.models import NodeKey
+        from metadataserver.models.scriptset import ScriptSet
+
+        machine.status = NODE_STATUS.DEPLOYED
+        # Foreign relations need to have an id to relate to, have to
+        # save here
+        machine.save()
+        script_set = ScriptSet.objects.create_deployed_machine_script_set(
+            machine
+        )
+        machine.current_commissioning_script_set = script_set
+
+        # ensure a token is available for the machine
+        NodeKey.objects.get_token_for_node(machine)
+
     def save(self, *args, **kwargs):
         """Persist the node into the database."""
         machine = super().save(commit=False)
@@ -1182,7 +1200,7 @@ class AdminMachineForm(MachineForm, AdminNodeForm, WithPowerTypeMixin):
         if pool:
             machine.pool = pool
         if self.cleaned_data.get("deployed"):
-            machine.status = NODE_STATUS.DEPLOYED
+            self._setup_deployed_machine(machine)
         WithPowerTypeMixin.set_values(self, machine)
         if kwargs.get("commit", True):
             machine.save(*args, **kwargs)
diff --git a/src/maasserver/forms/ephemeral.py b/src/maasserver/forms/ephemeral.py
index 39d28e2..b736eaa 100644
--- a/src/maasserver/forms/ephemeral.py
+++ b/src/maasserver/forms/ephemeral.py
@@ -21,8 +21,8 @@ from django.http import QueryDict
 from maasserver.enum import NODE_STATUS
 from maasserver.node_action import compile_node_actions
 from maasserver.utils.forms import set_form_error
-from metadataserver.enum import RESULT_TYPE, SCRIPT_TYPE
-from metadataserver.models import NodeKey, Script, ScriptSet
+from metadataserver.enum import SCRIPT_TYPE
+from metadataserver.models import Script
 
 
 class TestForm(Form):
@@ -322,31 +322,3 @@ class CommissionForm(TestForm):
             ),
         )
         return self.instance
-
-
-class CreateScriptsForDeployedForm(Form):
-    def __init__(self, instance, **kwargs):
-        super().__init__(**kwargs)
-        self.instance = instance
-
-    def save(self):
-        node = self.instance
-        scripts = list(
-            Script.objects.filter(
-                script_type=SCRIPT_TYPE.COMMISSIONING,
-                default=True,
-                tags__contains=["deploy-info"],
-            )
-        )
-        script_set = ScriptSet.objects.create(
-            node=node,
-            result_type=RESULT_TYPE.COMMISSIONING,
-            requested_scripts=[script.name for script in scripts],
-        )
-        for script in scripts:
-            script_set.add_pending_script(script)
-        node.current_commissioning_script_set = script_set
-        node.save()
-        # ensure a token is available for the machine
-        NodeKey.objects.get_token_for_node(node)
-        return script_set
diff --git a/src/maasserver/forms/tests/test_ephemeral.py b/src/maasserver/forms/tests/test_ephemeral.py
index a5c2e56..fde9528 100644
--- a/src/maasserver/forms/tests/test_ephemeral.py
+++ b/src/maasserver/forms/tests/test_ephemeral.py
@@ -12,16 +12,11 @@ from maasserver.enum import (
     NODE_TYPE_CHOICES,
     POWER_STATE,
 )
-from maasserver.forms.ephemeral import (
-    CommissionForm,
-    CreateScriptsForDeployedForm,
-    TestForm,
-)
+from maasserver.forms.ephemeral import CommissionForm, TestForm
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
 from maastesting.matchers import MockCalledOnceWith
-from metadataserver.enum import SCRIPT_STATUS, SCRIPT_TYPE
-from metadataserver.models import NodeKey
+from metadataserver.enum import SCRIPT_TYPE
 
 
 class TestTestForm(MAASServerTestCase):
@@ -988,54 +983,3 @@ class TestCommissionForm(MAASServerTestCase):
                 script_input={},
             ),
         )
-
-
-class TestCreateScriptsForDeployedForm(MAASServerTestCase):
-    def test_create_scripts(self):
-        script1 = factory.make_Script(
-            script_type=SCRIPT_TYPE.COMMISSIONING,
-            default=True,
-            tags=["foo", "deploy-info"],
-        )
-        script2 = factory.make_Script(
-            script_type=SCRIPT_TYPE.COMMISSIONING,
-            default=True,
-            tags=["bar", "deploy-info"],
-        )
-        # other scripts that are not matched
-        factory.make_Script(  # not for commissioning
-            script_type=SCRIPT_TYPE.TESTING,
-            default=True,
-            tags=["bar", "deploy-info"],
-        )
-        factory.make_Script(  # not a builtin script
-            script_type=SCRIPT_TYPE.COMMISSIONING,
-            default=False,
-            tags=["deploy-info"],
-        )
-        factory.make_Script(  # no deploy-info tag
-            script_type=SCRIPT_TYPE.COMMISSIONING,
-            default=True,
-            tags=["foo"],
-        )
-        node = factory.make_Node(status=NODE_STATUS.DEPLOYED)
-        form = CreateScriptsForDeployedForm(instance=node, data={})
-        self.assertTrue(form.is_valid())
-        script_set = form.save()
-        self.assertEqual(script_set.node, node)
-        script_results = list(script_set.scriptresult_set.order_by("id"))
-        self.assertEqual(
-            [script_result.script for script_result in script_results],
-            [script1, script2],
-        )
-        for script_result in script_results:
-            self.assertEqual(script_result.status, SCRIPT_STATUS.PENDING)
-        self.assertIs(node.current_commissioning_script_set, script_set)
-
-    def test_create_node_token(self):
-        node = factory.make_Node(status=NODE_STATUS.DEPLOYED)
-        self.assertFalse(NodeKey.objects.filter(node=node).exists())
-        form = CreateScriptsForDeployedForm(instance=node, data={})
-        self.assertTrue(form.is_valid())
-        form.save()
-        self.assertTrue(NodeKey.objects.filter(node=node).exists())
diff --git a/src/maasserver/forms/tests/test_machine.py b/src/maasserver/forms/tests/test_machine.py
index cef7cbe..bf61313 100644
--- a/src/maasserver/forms/tests/test_machine.py
+++ b/src/maasserver/forms/tests/test_machine.py
@@ -24,6 +24,7 @@ from maasserver.testing.osystems import (
     patch_usable_osystems,
 )
 from maasserver.testing.testcase import MAASServerTestCase
+from metadataserver.models import NodeKey
 from provisioningserver.rpc.exceptions import (
     NoConnectionsAvailable,
     NoSuchOperatingSystem,
@@ -409,11 +410,9 @@ class TestAdminMachineForm(MAASServerTestCase):
 
     def test_AdminMachineForm_new_machine_deployed(self):
         hostname = factory.make_string()
-        arch = make_usable_architecture(self)
         form = AdminMachineForm(
             data={
                 "hostname": hostname,
-                "architecture": arch,
                 "deployed": True,
             },
         )
@@ -518,3 +517,25 @@ class TestAdminMachineForm(MAASServerTestCase):
                 ]
             },
         )
+
+    def test_AdminMachineForm_creates_scriptset_for_deployed(self):
+        hostname = factory.make_string()
+        form = AdminMachineForm(
+            data={
+                "hostname": hostname,
+                "deployed": True,
+            },
+        )
+        machine = form.save()
+        self.assertIsNotNone(machine.current_commissioning_script_set)
+
+    def test_AdminMachineForm_creates_node_token_for_deployed(self):
+        hostname = factory.make_string()
+        form = AdminMachineForm(
+            data={
+                "hostname": hostname,
+                "deployed": True,
+            },
+        )
+        machine = form.save()
+        self.assertTrue(NodeKey.objects.filter(node=machine).exists())
diff --git a/src/maasserver/websockets/handlers/machine.py b/src/maasserver/websockets/handlers/machine.py
index 3236647..4425c78 100644
--- a/src/maasserver/websockets/handlers/machine.py
+++ b/src/maasserver/websockets/handlers/machine.py
@@ -22,6 +22,7 @@ from maasserver.enum import (
 from maasserver.exceptions import NodeActionError, NodeStateViolation
 from maasserver.forms import (
     AddPartitionForm,
+    AdminMachineForm,
     AdminMachineWithMACAddressesForm,
     CreateBcacheForm,
     CreateCacheSetForm,
@@ -246,6 +247,10 @@ class MachineHandler(NodeHandler):
         edit_permission = NodePermission.admin
         delete_permission = NodePermission.admin
 
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        self._deployed = False
+
     def get_queryset(self, for_list=False):
         """Return `QuerySet` for devices only viewable by `user`."""
         return Machine.objects.get_nodes(
@@ -391,6 +396,8 @@ class MachineHandler(NodeHandler):
 
     def get_form_class(self, action):
         """Return the form class used for `action`."""
+        if action == "create" and self._deployed:
+            return AdminMachineForm
         if action in ("create", "update"):
             return AdminMachineWithMACAddressesForm
         else:
@@ -408,6 +415,7 @@ class MachineHandler(NodeHandler):
         new_params["description"] = params.get("description")
         new_params["power_type"] = params.get("power_type")
         new_params["power_parameters"] = params.get("power_parameters")
+        new_params["deployed"] = params.get("deployed")
         if "zone" in params:
             new_params["zone"] = params["zone"]["name"]
         if params.get("pool"):
@@ -427,18 +435,18 @@ class MachineHandler(NodeHandler):
         return super().preprocess_form(action, new_params)
 
     def create(self, params):
-        """Create the object from params."""
+        self._deployed = bool(params.get("deployed", False))
         data = super().create(params)
-        node_obj = Node.objects.get(system_id=data["system_id"])
+        if not self._deployed:
+            machine = Node.objects.get(system_id=data["system_id"])
+            # Start the commissioning process right away, which has the
+            # desired side effect of initializing the node's power state.
+            d = machine.start_commissioning(self.user)
+            # Silently ignore errors to prevent tracebacks. The commissioning
+            # callbacks have their own logging. This fixes LP1600328.
+            d.addErrback(lambda _: None)
 
-        # Start the commissioning process right away, which has the
-        # desired side effect of initializing the node's power state.
-        d = node_obj.start_commissioning(self.user)
-        # Silently ignore errors to prevent tracebacks. The commissioning
-        # callbacks have their own logging. This fixes LP1600328.
-        d.addErrback(lambda _: None)
-
-        return self.full_dehydrate(node_obj)
+        return data
 
     def update(self, params):
         """Update the object from params."""
diff --git a/src/maasserver/websockets/handlers/tests/test_machine.py b/src/maasserver/websockets/handlers/tests/test_machine.py
index 391127b..b13f953 100644
--- a/src/maasserver/websockets/handlers/tests/test_machine.py
+++ b/src/maasserver/websockets/handlers/tests/test_machine.py
@@ -114,6 +114,7 @@ from metadataserver.enum import (
     SCRIPT_STATUS_FAILED,
     SCRIPT_TYPE,
 )
+from metadataserver.models.nodekey import NodeKey
 from metadataserver.models.scriptset import get_status_from_qs
 from provisioningserver.refresh.node_info_scripts import (
     LIST_MODALIASES_OUTPUT_NAME,
@@ -2416,6 +2417,33 @@ class TestMachineHandler(MAASServerTestCase):
         )
         self.assertThat(mock_start_commissioning, MockCalledOnceWith(user))
 
+    def test_create_creates_deployed_node(self):
+        user = factory.make_admin()
+        handler = MachineHandler(user, {}, None)
+        hostname = factory.make_name("hostname")
+        description = factory.make_name("description")
+        zone = factory.make_Zone()
+
+        mock_start_commissioning = self.patch(
+            node_model, "start_commissioning"
+        )
+
+        created_node = handler.create(
+            {
+                "hostname": hostname,
+                "description": description,
+                "zone": {"name": zone.name},
+                "deployed": True,
+            }
+        )
+        # the commissioning process is not started
+        mock_start_commissioning.assert_not_called()
+        self.assertEqual(created_node["status"], "Deployed")
+        node = Node.objects.get(system_id=created_node["system_id"])
+        self.assertIsNotNone(node.current_commissioning_script_set)
+        self.assertTrue(NodeKey.objects.filter(node=node).exists())
+        self.assertEqual(node.status, NODE_STATUS.DEPLOYED)
+
     def test_update_raise_permissions_error_for_non_admin(self):
         user = factory.make_User()
         node = factory.make_Node()
diff --git a/src/metadataserver/models/scriptset.py b/src/metadataserver/models/scriptset.py
index e9eed4a..2e8dea6 100644
--- a/src/metadataserver/models/scriptset.py
+++ b/src/metadataserver/models/scriptset.py
@@ -227,6 +227,25 @@ class ScriptSetManager(Manager):
         self._clean_old(node, RESULT_TYPE.INSTALLATION, script_set)
         return script_set
 
+    def create_deployed_machine_script_set(self, machine):
+        """Setup ScriptSet for a brown-field deployment.
+
+        Built-in scripts with deploy-info tag are queued up.
+        """
+        scripts = list(
+            Script.objects.filter(
+                script_type=SCRIPT_TYPE.COMMISSIONING,
+                default=True,
+                tags__contains=["deploy-info"],
+            )
+        )
+        script_set = machine.scriptset_set.create(
+            requested_scripts=[script.name for script in scripts]
+        )
+        for script in scripts:
+            script_set.add_pending_script(script)
+        return script_set
+
     def _find_scripts(self, script_qs, hw_pairs, modaliases):
         for script in script_qs:
             # If a script is not for specific hardware, it is always included
@@ -444,13 +463,10 @@ class ScriptSet(CleanSave, Model):
     def add_pending_script(self, script, script_input=None):
         """Create and add a new ScriptResult for the given Script.
 
-        Creates a new ScriptResult for the given script and assoicates it with
+        Creates a new ScriptResult for the given script and associates it with
         this ScriptSet. Raises a ValidationError if ParametersForm validation
         fails.
         """
-        # Avoid circular dependencies.
-        from metadataserver.models import ScriptResult
-
         if script_input is None:
             script_input = {}
         form = ParametersForm(
@@ -461,8 +477,7 @@ class ScriptSet(CleanSave, Model):
         if not form.is_valid():
             raise ValidationError(form.errors)
         for param in form.cleaned_data["input"]:
-            ScriptResult.objects.create(
-                script_set=self,
+            self.scriptresult_set.create(
                 status=SCRIPT_STATUS.PENDING,
                 script=script,
                 script_name=script.name,
diff --git a/src/metadataserver/models/tests/test_scriptset.py b/src/metadataserver/models/tests/test_scriptset.py
index 68a5b9f..ad37878 100644
--- a/src/metadataserver/models/tests/test_scriptset.py
+++ b/src/metadataserver/models/tests/test_scriptset.py
@@ -10,7 +10,7 @@ from unittest.mock import PropertyMock
 from django.core.exceptions import ValidationError
 from django.db.models import Q
 
-from maasserver.enum import NODE_TYPE
+from maasserver.enum import NODE_STATUS, NODE_TYPE
 from maasserver.exceptions import NoScriptsFound
 from maasserver.models import Config, Event, EventType, Node
 from maasserver.preseed import CURTIN_INSTALL_LOG
@@ -1012,6 +1012,48 @@ class TestScriptSetManager(MAASServerTestCase):
             ).all(),
         )
 
+    def test_create_deployed_machine_script_set_scripts(self):
+        # Avoid builtins muddying the test
+        Script.objects.all().delete()
+        script1 = factory.make_Script(
+            script_type=SCRIPT_TYPE.COMMISSIONING,
+            default=True,
+            tags=["foo", "deploy-info"],
+        )
+        script2 = factory.make_Script(
+            script_type=SCRIPT_TYPE.COMMISSIONING,
+            default=True,
+            tags=["bar", "deploy-info"],
+        )
+        # other scripts that are not matched
+        factory.make_Script(  # not for commissioning
+            script_type=SCRIPT_TYPE.TESTING,
+            default=True,
+            tags=["bar", "deploy-info"],
+        )
+        factory.make_Script(  # not a builtin script
+            script_type=SCRIPT_TYPE.COMMISSIONING,
+            default=False,
+            tags=["deploy-info"],
+        )
+        factory.make_Script(  # no deploy-info tag
+            script_type=SCRIPT_TYPE.COMMISSIONING,
+            default=True,
+            tags=["foo"],
+        )
+        node = factory.make_Node(status=NODE_STATUS.DEPLOYED)
+        script_set = ScriptSet.objects.create_deployed_machine_script_set(node)
+        self.assertItemsEqual(
+            [
+                (script_result.script, script_result.status)
+                for script_result in script_set.scriptresult_set.all()
+            ],
+            [
+                (script1, SCRIPT_STATUS.PENDING),
+                (script2, SCRIPT_STATUS.PENDING),
+            ],
+        )
+
 
 class TestScriptSet(MAASServerTestCase):
     """Test the ScriptSet model."""