Merge lp:~newell-jensen/maas/fix-1602487 into lp:~maas-committers/maas/trunk

[Newell Jensen]

Factor out the addErrback for resetting the node's status into _start from start_commissioning. Makes it so if there is an error during deployment, the status will be reset.

[Newell Jensen (newell-jensen)]

This was testing on OIL's integration server.

[Gavin Panella (allenap)]

Looks good.

review: Approve

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2016-07-15 18:57:25 +0000
+++ src/maasserver/models/node.py	2016-07-18 20:09:23 +0000
@@ -1657,7 +1657,7 @@
             # 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, commissioning_user_data)
+            starting = self._start(user, commissioning_user_data, old_status)
         except Exception as error:
             self.status = old_status
             self.save()
@@ -1688,11 +1688,6 @@
                 callOut, self._start_commissioning_async, is_starting,
                 self.hostname)
 
-            # If there's an error, reset the node's status.
-            starting.addErrback(
-                callOutToDatabase, Node._set_status, self.system_id,
-                status=old_status)
-
             def eb_start(failure, hostname):
                 maaslog.error(
                     "%s: Could not start node for commissioning: %s",
@@ -2135,6 +2130,7 @@
 
         # Change the status of the node now to avoid races when starting
         # nodes in bulk.
+        old_status = self.status
         self.status = NODE_STATUS.DISK_ERASING
         self.save()
 
@@ -2142,7 +2138,7 @@
             # 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, disk_erase_user_data)
+            starting = self._start(user, disk_erase_user_data, old_status)
         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
@@ -3019,7 +3015,7 @@
         return client_idents, fallback_idents
 
     @transactional
-    def _start(self, user, user_data=None):
+    def _start(self, user, user_data=None, old_status=None):
         """Request on given user's behalf that the node be started up.
 
         :param user: Requesting user.
@@ -3055,6 +3051,7 @@
         NodeUserData.objects.set_user_data(self, user_data)
 
         if self.status == NODE_STATUS.ALLOCATED:
+            old_status = self.status
             # Claim AUTO IP addresses for the node if it's ALLOCATED.
             # The current state being ALLOCATED is our indication that the node
             # is being deployed for the first time.
@@ -3084,6 +3081,12 @@
                 callOutToDatabase, Node._set_status_expires,
                 self.system_id, deployment_timeout)
 
+        if old_status is not None:
+            # If there's an error, reset the node's status.
+            d.addErrback(
+                callOutToDatabase, Node._set_status, self.system_id,
+                status=old_status)
+
         # If any part of this processes fails be sure to release the grabbed
         # auto IP addresses.
         if claimed_ips:
 
=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2016-07-15 18:57:25 +0000
+++ src/maasserver/models/tests/test_node.py	2016-07-18 20:09:23 +0000
@@ -1414,7 +1414,8 @@
             status=NODE_STATUS.ALLOCATED, owner=owner, agent_name=agent_name)
         node_start = self.patch(node, '_start')
         # Return a post-commit hook from Node.start().
-        node_start.side_effect = lambda user, user_data: post_commit()
+        node_start.side_effect = (
+            lambda user, user_data, old_status: post_commit())
         Config.objects.set_config('disk_erase_with_secure_erase', True)
         Config.objects.set_config('disk_erase_with_quick_erase', True)
         with post_commit_hooks:
@@ -1435,7 +1436,8 @@
             status=NODE_STATUS.ALLOCATED, owner=owner, agent_name=agent_name)
         node_start = self.patch(node, '_start')
         # Return a post-commit hook from Node.start().
-        node_start.side_effect = lambda user, user_data: post_commit()
+        node_start.side_effect = (
+            lambda user, user_data, old_status: post_commit())
         Config.objects.set_config('disk_erase_with_secure_erase', False)
         Config.objects.set_config('disk_erase_with_quick_erase', False)
         with post_commit_hooks:
@@ -1456,26 +1458,28 @@
             status=NODE_STATUS.ALLOCATED, owner=owner, agent_name=agent_name)
         node_start = self.patch(node, '_start')
         # Return a post-commit hook from Node.start().
-        node_start.side_effect = lambda user, user_data: post_commit()
+        node_start.side_effect = (
+            lambda user, user_data, old_status: post_commit())
         with post_commit_hooks:
             node.start_disk_erasing(owner)
         self.expectThat(node.owner, Equals(owner))
         self.expectThat(node.status, Equals(NODE_STATUS.DISK_ERASING))
         self.expectThat(node.agent_name, Equals(agent_name))
         self.assertThat(
-            node_start, MockCalledOnceWith(owner, ANY))
+            node_start, MockCalledOnceWith(owner, ANY, NODE_STATUS.ALLOCATED))
 
     def test_start_disk_erasing_logs_user_request(self):
         owner = factory.make_User()
         node = factory.make_Node(status=NODE_STATUS.ALLOCATED, owner=owner)
         node_start = self.patch(node, '_start')
         # Return a post-commit hook from Node.start().
-        node_start.side_effect = lambda user, user_data: post_commit()
+        node_start.side_effect = (
+            lambda user, user_data, old_status: post_commit())
         register_event = self.patch(node, '_register_request_event')
         with post_commit_hooks:
             node.start_disk_erasing(owner)
         self.assertThat(
-            node_start, MockCalledOnceWith(owner, ANY))
+            node_start, MockCalledOnceWith(owner, ANY, NODE_STATUS.ALLOCATED))
         self.assertThat(register_event, MockCalledOnceWith(
             owner, EVENT_TYPES.REQUEST_NODE_ERASE_DISK,
             action='start disk erasing', comment=None))
@@ -1538,7 +1542,7 @@
 
         self.assertThat(
             node_start, MockCalledOnceWith(
-                admin, generate_user_data.return_value))
+                admin, generate_user_data.return_value, NODE_STATUS.ALLOCATED))
         self.assertEqual(NODE_STATUS.FAILED_DISK_ERASING, node.status)
 
     def test_start_disk_erasing_sets_status_on_post_commit_error(self):
@@ -2159,7 +2163,8 @@
             interface=True, status=NODE_STATUS.NEW, power_type='manual')
         node_start = self.patch(node, '_start')
         # Return a post-commit hook from Node.start().
-        node_start.side_effect = lambda user, user_data: post_commit()
+        node_start.side_effect = (
+            lambda user, user_data, old_status: post_commit())
         admin = factory.make_admin()
         node.start_commissioning(admin)
         post_commit_hooks.reset()  # Ignore these for now.
@@ -2168,7 +2173,8 @@
             'status': NODE_STATUS.COMMISSIONING,
         }
         self.assertAttributes(node, expected_attrs)
-        self.assertThat(node_start, MockCalledOnceWith(admin, ANY))
+        self.assertThat(node_start, MockCalledOnceWith(
+            admin, ANY, NODE_STATUS.NEW))
 
     def test_start_commissioning_sets_options(self):
         rack = factory.make_RackController()
@@ -2177,7 +2183,8 @@
             bmc_connected_to=rack)
         node_start = self.patch(node, '_start')
         # Return a post-commit hook from Node.start().
-        node_start.side_effect = lambda user, user_data: post_commit()
+        node_start.side_effect = (
+            lambda user, user_data, old_status: post_commit())
         admin = factory.make_admin()
         enable_ssh = factory.pick_bool()
         skip_networking = factory.pick_bool()
@@ -2197,7 +2204,8 @@
     def test_start_commissioning_sets_user_data(self):
         node = factory.make_Node(status=NODE_STATUS.NEW)
         node_start = self.patch(node, '_start')
-        node_start.side_effect = lambda user, user_data: post_commit()
+        node_start.side_effect = (
+            lambda user, user_data, old_status: post_commit())
         user_data = factory.make_string().encode('ascii')
         generate_user_data = self.patch(
             commissioning, 'generate_user_data')
@@ -2205,12 +2213,14 @@
         admin = factory.make_admin()
         node.start_commissioning(admin)
         post_commit_hooks.reset()  # Ignore these for now.
-        self.assertThat(node_start, MockCalledOnceWith(admin, user_data))
+        self.assertThat(node_start, MockCalledOnceWith(
+            admin, user_data, NODE_STATUS.NEW))
 
     def test_start_commissioning_sets_min_hwe_kernel(self):
         node = factory.make_Node(status=NODE_STATUS.NEW)
         node_start = self.patch(node, '_start')
-        node_start.side_effect = lambda user, user_data: post_commit()
+        node_start.side_effect = (
+            lambda user, user_data, old_status: post_commit())
         user_data = factory.make_string().encode('ascii')
         generate_user_data = self.patch(
             commissioning, 'generate_user_data')
@@ -2224,7 +2234,8 @@
     def test_start_commissioning_clears_node_commissioning_results(self):
         node = factory.make_Node(status=NODE_STATUS.NEW)
         node_start = self.patch(node, '_start')
-        node_start.side_effect = lambda user, user_data: post_commit()
+        node_start.side_effect = (
+            lambda user, user_data, old_status: post_commit())
         NodeResult.objects.store_data(
             node, factory.make_string(),
             random.randint(0, 10),
@@ -2237,7 +2248,8 @@
     def test_start_commissioning_clears_storage_configuration(self):
         node = factory.make_Node(status=NODE_STATUS.NEW)
         node_start = self.patch(node, '_start')
-        node_start.side_effect = lambda user, user_data: post_commit()
+        node_start.side_effect = (
+            lambda user, user_data, old_status: post_commit())
         clear_storage = self.patch_autospec(
             node, '_clear_full_storage_configuration')
         admin = factory.make_admin()
@@ -2248,7 +2260,8 @@
     def test_start_commissioning_doesnt_clear_storage_configuration(self):
         node = factory.make_Node(status=NODE_STATUS.NEW)
         node_start = self.patch(node, '_start')
-        node_start.side_effect = lambda user, user_data: post_commit()
+        node_start.side_effect = (
+            lambda user, user_data, old_status: post_commit())
         clear_storage = self.patch_autospec(
             node, '_clear_full_storage_configuration')
         admin = factory.make_admin()
@@ -2259,7 +2272,8 @@
     def test_start_commissioning_calls__clear_networking_configuration(self):
         node = factory.make_Node(status=NODE_STATUS.NEW)
         node_start = self.patch(node, '_start')
-        node_start.side_effect = lambda user, user_data: post_commit()
+        node_start.side_effect = (
+            lambda user, user_data, old_status: post_commit())
         clear_networking = self.patch_autospec(
             node, '_clear_networking_configuration')
         admin = factory.make_admin()
@@ -2270,7 +2284,8 @@
     def test_start_commissioning_doesnt_call__clear_networking(self):
         node = factory.make_Node(status=NODE_STATUS.NEW)
         node_start = self.patch(node, '_start')
-        node_start.side_effect = lambda user, user_data: post_commit()
+        node_start.side_effect = (
+            lambda user, user_data, old_status: post_commit())
         clear_networking = self.patch_autospec(
             node, '_clear_networking_configuration')
         admin = factory.make_admin()
@@ -2313,41 +2328,10 @@
 
         self.assertThat(
             node_start,
-            MockCalledOnceWith(admin, generate_user_data.return_value))
+            MockCalledOnceWith(
+                admin, generate_user_data.return_value, NODE_STATUS.NEW))
         self.assertEqual(NODE_STATUS.NEW, node.status)
 
-    def test_start_commissioning_reverts_status_on_post_commit_error(self):
-        # When start_commissioning encounters an error in its post-commit
-        # hook, it will revert the node to its previous status.
-        admin = factory.make_admin()
-        status = random.choice(
-            (NODE_STATUS.NEW, NODE_STATUS.READY,
-             NODE_STATUS.FAILED_COMMISSIONING))
-        node = factory.make_Node(status=status)
-        # Patch out some things that we don't want to do right now.
-        self.patch(Node, '_set_status_expires')
-        self.patch(node, '_start').return_value = None
-        # Fake an error during the post-commit hook.
-        error_message = factory.make_name("error")
-        error_type = factory.make_exception_type()
-        _start_async = self.patch_autospec(node, "_start_commissioning_async")
-        _start_async.side_effect = error_type(error_message)
-        # Capture calls to _set_status.
-        self.patch_autospec(Node, "_set_status")
-
-        with LoggerFixture("maas") as logger:
-            with ExpectedException(error_type):
-                with post_commit_hooks:
-                    node.start_commissioning(admin)
-
-        # The status is set to be reverted to its initial status.
-        self.assertThat(node._set_status, MockCalledOnceWith(
-            node.system_id, status=status))
-        # It's logged too.
-        self.assertThat(logger.output, Contains(
-            "%s: Could not start node for commissioning: %s\n"
-            % (node.hostname, error_message)))
-
     def test_start_commissioning_logs_and_raises_errors_in_starting(self):
         admin = factory.make_admin()
         node = factory.make_Node(status=NODE_STATUS.NEW)
@@ -2368,7 +2352,8 @@
         register_event = self.patch(node, '_register_request_event')
         node_start = self.patch(node, '_start')
         # Return a post-commit hook from Node.start().
-        node_start.side_effect = lambda user, user_data: post_commit()
+        node_start.side_effect = (
+            lambda user, user_data, old_status: post_commit())
         admin = factory.make_admin()
         node.start_commissioning(admin)
         post_commit_hooks.reset()  # Ignore these for now.
@@ -2489,7 +2474,8 @@
             enable_ssh=True)
         node_start = self.patch(node, 'start')
         # Return a post-commit hook from Node.start().
-        node_start.side_effect = lambda user, user_data: post_commit()
+        node_start.side_effect = (
+            lambda user, user_data, old_status: post_commit())
         admin = factory.make_admin()
         node.start_commissioning(admin)
         post_commit_hooks.reset()  # Ignore these for now.
@@ -4632,6 +4618,7 @@
     def test__adds_callbacks_and_errbacks_to_post_commit(self):
         user = factory.make_User()
         node = self.make_acquired_node_with_interface(user)
+        old_status = node.status
 
         post_commit_defer = self.patch(node_module, "post_commit")
         mock_power_control = self.patch(Node, "_power_control_node")
@@ -4645,10 +4632,12 @@
                 callOutToDatabase, Node._set_status_expires,
                 node.system_id, node.get_deployment_time()))
 
-        # Adds errback to release auto ips.
+        # Adds errback to reset status and release auto ips.
         self.assertThat(
-            post_commit_defer.addErrback, MockCalledOnceWith(
-                callOutToDatabase, node.release_auto_ips))
+            post_commit_defer.addErrback, MockCallsMatch(
+                call(callOutToDatabase, node._set_status,
+                     node.system_id, status=old_status),
+                call(callOutToDatabase, node.release_auto_ips)))
 
     def test_storage_layout_issues_returns_invalid_no_boot_arm64_non_efi(self):
         node = factory.make_Node(
 
=== modified file 'src/maasserver/tests/test_node_action.py'
--- src/maasserver/tests/test_node_action.py	2016-07-15 00:42:25 +0000
+++ src/maasserver/tests/test_node_action.py	2016-07-18 20:09:23 +0000
@@ -292,14 +292,15 @@
             interface=True, status=self.status,
             power_type='manual', power_state=POWER_STATE.OFF)
         node_start = self.patch(node, '_start')
-        node_start.side_effect = lambda user, user_data: post_commit()
+        node_start.side_effect = (
+            lambda user, user_data, old_status: post_commit())
         admin = factory.make_admin()
         action = Commission(node, admin)
         with post_commit_hooks:
             action.execute()
         self.assertEqual(NODE_STATUS.COMMISSIONING, node.status)
         self.assertThat(
-            node_start, MockCalledOnceWith(admin, ANY))
+            node_start, MockCalledOnceWith(admin, ANY, ANY))
 
 
 class TestAbortAction(MAASTransactionServerTestCase):
@@ -734,6 +735,7 @@
             interface=True, status=self.actionable_status,
             power_type='ipmi', power_state=POWER_STATE.OFF,
             owner=user, power_parameters=params)
+        old_status = node.status
         node_start = self.patch_autospec(node, '_start')
         node_start.return_value = None
 
@@ -742,7 +744,8 @@
 
         self.expectThat(node.status, Equals(NODE_STATUS.DISK_ERASING))
         self.assertThat(
-            node_start, MockCalledOnceWith(user, user_data=ANY))
+            node_start, MockCalledOnceWith(
+                user, user_data=ANY, old_status=old_status))
 
     def test_Release_passes_secure_erase_and_quick_erase(self):
         user = factory.make_User()