Merge ~newell-jensen/maas:lp1548402 into maas:master

[Newell Jensen]

LP: #1548402 - Handle errors in preseeds

Ensure that MAAS prevents starting the deployment of a machine if the preseeds fail to render.

[Andres Rodriguez (andreserl)]

lgtm!

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b lp1548402 lp:~newell-jensen/maas into -b master lp:~maas-committers/maas

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

9a16d51... by Newell Jensen on 2017-07-20

Add fix for failing test in websocket handler machine_test

diff --git a/src/maasserver/api/machines.py b/src/maasserver/api/machines.py
index 58d9dfd..dfca107 100644
--- a/src/maasserver/api/machines.py
+++ b/src/maasserver/api/machines.py
@@ -443,6 +443,13 @@ class MachineHandler(NodeHandler, OwnerDataMixin, PowerMixin):
             form.save()
         else:
             raise MAASAPIValidationError(form.errors)
+        # Check that the curtin preseeds renders correctly.
+        try:
+            get_curtin_merged_config(machine)
+        except Exception as e:
+            raise MAASAPIBadRequest(
+                "Failed to render preseed: %s" % e)
+
         return self.power_on(request, system_id)
 
     @operation(idempotent=False)
diff --git a/src/maasserver/api/tests/test_machine.py b/src/maasserver/api/tests/test_machine.py
index 4f9fa5d..9d4abf0 100644
--- a/src/maasserver/api/tests/test_machine.py
+++ b/src/maasserver/api/tests/test_machine.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the Machine API."""
@@ -326,6 +326,7 @@ class TestMachineAPI(APITestCase.ForUser):
 
     def test_POST_deploy_sets_osystem_and_distro_series(self):
         self.patch(node_module.Node, "_start")
+        self.patch(machines_module, "get_curtin_merged_config")
         machine = factory.make_Node(
             owner=self.user, interface=True,
             power_type='manual',
@@ -367,6 +368,7 @@ class TestMachineAPI(APITestCase.ForUser):
 
     def test_POST_deploy_sets_license_key(self):
         self.patch(node_module.Node, "_start")
+        self.patch(machines_module, "get_curtin_merged_config")
         machine = factory.make_Node(
             owner=self.user, interface=True,
             power_type='manual',
@@ -415,6 +417,7 @@ class TestMachineAPI(APITestCase.ForUser):
 
     def test_POST_deploy_sets_default_distro_series(self):
         self.patch(node_module.Node, "_start")
+        self.patch(machines_module, "get_curtin_merged_config")
         machine = factory.make_Node(
             owner=self.user, interface=True,
             power_type='manual',
@@ -432,6 +435,7 @@ class TestMachineAPI(APITestCase.ForUser):
 
     def test_POST_deploy_works_if_series_already_set(self):
         self.patch(node_module.Node, "_start")
+        self.patch(machines_module, "get_curtin_merged_config")
         osystem = Config.objects.get_config('default_osystem')
         distro_series = Config.objects.get_config('default_distro_series')
         make_usable_osystem(
@@ -467,6 +471,26 @@ class TestMachineAPI(APITestCase.ForUser):
             ),
             (response.status_code, json_load_bytes(response.content)))
 
+    def test_POST_deploy_fails_when_preseed_not_rendered(self):
+        mock_get_curtin_merged_config = self.patch(
+            machines_module, "get_curtin_merged_config")
+        mock_get_curtin_merged_config.side_effect = Exception('error')
+        osystem = Config.objects.get_config('default_osystem')
+        distro_series = Config.objects.get_config('default_distro_series')
+        make_usable_osystem(
+            self, osystem_name=osystem, releases=[distro_series])
+        machine = factory.make_Node(
+            owner=self.user, interface=True,
+            status=NODE_STATUS.ALLOCATED,
+            power_type='manual',
+            distro_series=distro_series,
+            osystem=osystem,
+            architecture=make_usable_architecture(self))
+        response = self.client.post(
+            self.get_machine_uri(machine), {'op': 'deploy'})
+        self.assertEqual(http.client.BAD_REQUEST, response.status_code)
+        self.assertEqual(b"Failed to render preseed: error", response.content)
+
     def test_POST_deploy_validates_hwe_kernel_with_default_distro_series(self):
         architecture = make_usable_architecture(self, subarch_name="generic")
         machine = factory.make_Node(
@@ -495,6 +519,7 @@ class TestMachineAPI(APITestCase.ForUser):
 
     def test_POST_deploy_may_be_repeated(self):
         self.patch(node_module.Node, "_start")
+        self.patch(machines_module, "get_curtin_merged_config")
         machine = factory.make_Node(
             owner=self.user, interface=True,
             power_type='manual',
@@ -515,6 +540,7 @@ class TestMachineAPI(APITestCase.ForUser):
             node_module.RackControllerManager, "filter_by_url_accessible"
             ).return_value = [rack_controller]
         self.patch(node_module.Node, "_power_control_node")
+        self.patch(machines_module, "get_curtin_merged_config")
         machine = factory.make_Node(
             owner=self.user, interface=True,
             power_type='virsh', architecture=make_usable_architecture(self),
@@ -536,6 +562,7 @@ class TestMachineAPI(APITestCase.ForUser):
 
     def test_POST_deploy_passes_comment(self):
         self.patch(node_module.Node, "_start")
+        self.patch(machines_module, "get_curtin_merged_config")
         rack_controller = factory.make_RackController()
         machine = factory.make_Node(
             owner=self.user, interface=True,
@@ -565,6 +592,7 @@ class TestMachineAPI(APITestCase.ForUser):
         osystem = make_usable_osystem(self)
         distro_series = osystem['default_release']
         machine_start = self.patch(node_module.Machine, 'start')
+        self.patch(machines_module, "get_curtin_merged_config")
         machine_start.return_value = False
         self.client.post(
             self.get_machine_uri(machine), {
@@ -578,6 +606,7 @@ class TestMachineAPI(APITestCase.ForUser):
     def test_POST_deploy_doesnt_reset_power_options_bug_1569102(self):
         self.become_admin()
         self.patch(node_module.Node, "_start")
+        self.patch(machines_module, "get_curtin_merged_config")
         rack_controller = factory.make_RackController()
         machine = factory.make_Node(
             owner=self.user, interface=True,
@@ -600,6 +629,7 @@ class TestMachineAPI(APITestCase.ForUser):
 
     def test_POST_deploy_allocates_ready_machines(self):
         self.patch(node_module.Node, "_start")
+        self.patch(machines_module, "get_curtin_merged_config")
         machine = factory.make_Node(
             status=NODE_STATUS.READY, interface=True,
             power_type='manual',
@@ -631,6 +661,7 @@ class TestMachineAPI(APITestCase.ForUser):
 
     def test_POST_deploy_passes_agent_name(self):
         self.patch(node_module.Node, "_start")
+        self.patch(machines_module, "get_curtin_merged_config")
         machine = factory.make_Node(
             status=NODE_STATUS.READY, interface=True,
             power_type='manual',
@@ -650,6 +681,7 @@ class TestMachineAPI(APITestCase.ForUser):
 
     def test_POST_deploy_passes_comment_on_acquire(self):
         self.patch(node_module.Node, "_start")
+        self.patch(machines_module, "get_curtin_merged_config")
         machine_method = self.patch(node_module.Machine, 'acquire')
         machine = factory.make_Node(
             status=NODE_STATUS.READY, owner=self.user, interface=True,
@@ -673,6 +705,7 @@ class TestMachineAPI(APITestCase.ForUser):
 
     def test_POST_deploy_passes_bridge_settings(self):
         self.patch(node_module.Node, "_start")
+        self.patch(machines_module, "get_curtin_merged_config")
         machine_method = self.patch(node_module.Machine, 'acquire')
         machine = factory.make_Node(
             status=NODE_STATUS.READY, owner=self.user, interface=True,
diff --git a/src/maasserver/node_action.py b/src/maasserver/node_action.py
index fc48bb0..564abd5 100644
--- a/src/maasserver/node_action.py
+++ b/src/maasserver/node_action.py
@@ -44,6 +44,7 @@ from maasserver.node_status import (
     is_failed_status,
     NON_MONITORED_STATUSES,
 )
+from maasserver.preseed import get_curtin_config
 from maasserver.utils.orm import post_commit_do
 from maasserver.utils.osystems import (
     validate_hwe_kernel,
@@ -346,6 +347,12 @@ class Deploy(NodeAction):
             raise NodeActionError(e)
 
         try:
+            get_curtin_config(self.node)
+        except Exception as e:
+            raise NodeActionError(
+                "Failed to retrieve curtin config: %s" % e)
+
+        try:
             self.node.start(self.user)
         except StaticIPAddressExhaustion:
             raise NodeActionError(
diff --git a/src/maasserver/tests/test_node_action.py b/src/maasserver/tests/test_node_action.py
index 4f0c0bd..bad38a2 100644
--- a/src/maasserver/tests/test_node_action.py
+++ b/src/maasserver/tests/test_node_action.py
@@ -49,6 +49,7 @@ from maasserver.node_action import (
     SetZone,
     Test,
 )
+import maasserver.node_action as node_action_module
 from maasserver.node_status import (
     MONITORED_STATUSES,
     NODE_TESTING_RESET_READY_TRANSITIONS,
@@ -499,10 +500,27 @@ class TestDeployAction(MAASServerTestCase):
         node = factory.make_Node(
             interface=True, status=NODE_STATUS.ALLOCATED,
             power_type='manual', owner=user)
-        node_start = self.patch(node, 'start')
+        mock_get_curtin_config = self.patch(
+            node_action_module, 'get_curtin_config')
+        mock_node_start = self.patch(node, 'start')
         Deploy(node, user).execute()
-        self.assertThat(
-            node_start, MockCalledOnceWith(user))
+        self.expectThat(
+            mock_get_curtin_config, MockCalledOnceWith(node))
+        self.expectThat(
+            mock_node_start, MockCalledOnceWith(user))
+
+    def test_Deploy_raises_NodeActionError_for_no_curtin_config(self):
+        user = factory.make_User()
+        node = factory.make_Node(
+            interface=True, status=NODE_STATUS.ALLOCATED,
+            power_type='manual', owner=user)
+        mock_get_curtin_config = self.patch(
+            node_action_module, 'get_curtin_config')
+        mock_get_curtin_config.side_effect = NodeActionError('error')
+        error = self.assertRaises(
+            NodeActionError, Deploy(node, user).execute)
+        self.assertEqual(
+            "Failed to retrieve curtin config: error", str(error))
 
     def test_Deploy_raises_NodeActionError_for_invalid_os(self):
         user = factory.make_User()
@@ -527,7 +545,9 @@ class TestDeployAction(MAASServerTestCase):
         node = factory.make_Node(
             interface=True, status=NODE_STATUS.ALLOCATED,
             power_type='manual', owner=user)
-        self.patch(node, 'start')
+        mock_get_curtin_config = self.patch(
+            node_action_module, 'get_curtin_config')
+        mock_node_start = self.patch(node, 'start')
         osystem = make_usable_osystem(self)
         os_name = osystem["name"]
         release_name = osystem["releases"][0]["name"]
@@ -536,6 +556,10 @@ class TestDeployAction(MAASServerTestCase):
             "distro_series": release_name
         }
         Deploy(node, user).execute(**extra)
+        self.expectThat(
+            mock_get_curtin_config, MockCalledOnceWith(node))
+        self.expectThat(
+            mock_node_start, MockCalledOnceWith(user))
         self.expectThat(node.osystem, Equals(os_name))
         self.expectThat(
             node.distro_series, Equals(release_name))
@@ -545,7 +569,9 @@ class TestDeployAction(MAASServerTestCase):
         node = factory.make_Node(
             interface=True, status=NODE_STATUS.ALLOCATED,
             power_type='manual', owner=user)
-        self.patch(node, 'start')
+        mock_get_curtin_config = self.patch(
+            node_action_module, 'get_curtin_config')
+        mock_node_start = self.patch(node, 'start')
         osystem = make_usable_osystem(self)
         os_name = osystem["name"]
         release_name = osystem["releases"][0]["name"]
@@ -554,6 +580,10 @@ class TestDeployAction(MAASServerTestCase):
             "distro_series": release_name + '*'
         }
         Deploy(node, user).execute(**extra)
+        self.expectThat(
+            mock_get_curtin_config, MockCalledOnceWith(node))
+        self.expectThat(
+            mock_node_start, MockCalledOnceWith(user))
         self.expectThat(node.osystem, Equals(os_name))
         self.expectThat(
             node.distro_series, Equals(release_name))
@@ -563,12 +593,18 @@ class TestDeployAction(MAASServerTestCase):
         node = factory.make_Node(
             interface=True, status=NODE_STATUS.ALLOCATED,
             power_type='manual', owner=user)
-        self.patch(node, 'start')
+        mock_get_curtin_config = self.patch(
+            node_action_module, 'get_curtin_config')
+        mock_node_start = self.patch(node, 'start')
         osystem = make_osystem_with_releases(self)
         extra = {
             "distro_series": osystem["releases"][0]["name"],
         }
         Deploy(node, user).execute(**extra)
+        self.expectThat(
+            mock_get_curtin_config, MockCalledOnceWith(node))
+        self.expectThat(
+            mock_node_start, MockCalledOnceWith(user))
         self.expectThat(node.osystem, Equals(""))
         self.expectThat(node.distro_series, Equals(""))
 
@@ -577,24 +613,36 @@ class TestDeployAction(MAASServerTestCase):
         node = factory.make_Node(
             interface=True, status=NODE_STATUS.ALLOCATED,
             power_type='manual', owner=user)
-        self.patch(node, 'start')
+        mock_get_curtin_config = self.patch(
+            node_action_module, 'get_curtin_config')
+        mock_node_start = self.patch(node, 'start')
         osystem = make_osystem_with_releases(self)
         extra = {
             "osystem": osystem["name"],
         }
         Deploy(node, user).execute(**extra)
+        self.expectThat(
+            mock_get_curtin_config, MockCalledOnceWith(node))
+        self.expectThat(
+            mock_node_start, MockCalledOnceWith(user))
         self.expectThat(node.osystem, Equals(""))
         self.expectThat(node.distro_series, Equals(""))
 
     def test_Deploy_allocates_node_if_node_not_already_allocated(self):
         user = factory.make_User()
         node = factory.make_Node(status=NODE_STATUS.READY, with_boot_disk=True)
-        self.patch(node, 'start')
+        mock_get_curtin_config = self.patch(
+            node_action_module, 'get_curtin_config')
+        mock_node_start = self.patch(node, 'start')
         action = Deploy(node, user)
         action.execute()
 
-        self.assertEqual(user, node.owner)
-        self.assertEqual(NODE_STATUS.ALLOCATED, node.status)
+        self.expectThat(
+            mock_get_curtin_config, MockCalledOnceWith(node))
+        self.expectThat(
+            mock_node_start, MockCalledOnceWith(user))
+        self.expectThat(user, Equals(node.owner))
+        self.expectThat(NODE_STATUS.ALLOCATED, Equals(node.status))
 
 
 class TestDeployActionTransactional(MAASTransactionServerTestCase):
diff --git a/src/maasserver/websockets/handlers/tests/test_machine.py b/src/maasserver/websockets/handlers/tests/test_machine.py
index 80ab253..0732ba1 100644
--- a/src/maasserver/websockets/handlers/tests/test_machine.py
+++ b/src/maasserver/websockets/handlers/tests/test_machine.py
@@ -56,6 +56,7 @@ from maasserver.models.partition import (
     PARTITION_ALIGNMENT_SIZE,
 )
 from maasserver.node_action import compile_node_actions
+import maasserver.node_action as node_action_module
 from maasserver.testing.architecture import make_usable_architecture
 from maasserver.testing.factory import factory
 from maasserver.testing.osystems import make_usable_osystem
@@ -2059,6 +2060,7 @@ class TestMachineHandler(MAASServerTestCase):
         self.patch(Machine, 'on_network').return_value = True
         node = factory.make_Node(status=NODE_STATUS.ALLOCATED, owner=user)
         self.patch(Machine, "_start").return_value = None
+        self.patch(node_action_module, 'get_curtin_config')
         osystem = make_usable_osystem(self)
         handler = MachineHandler(user, {})
         handler.action({