Merge ~blake-rouse/maas:fix-1820998 into maas:master

[Blake Rouse]

Fixes LP: #1820998 - Reset skip_storage and skip_networking on running of commissioning hooks as well on failure for starting of commissioning and testing.

[Newell Jensen (newell-jensen)]

LGTM

review: Approve

[MAAS Lander (maas-lander)]

UNIT TESTS
-b fix-1820998 lp:~blake-rouse/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: b6c50d27b3b703ad58b78a4d3148b7a2ac7a0298

review: Approve

[Lee Trager (ltrager)]

LGTM!

review: Approve

[Andres Rodriguez (andreserl)]

lgtm! please backport this!

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b fix-1820998 lp:~blake-rouse/maas/+git/maas into -b master lp:~maas-committers/maas

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

diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index 205b45e..f4d04a9 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -2025,6 +2025,10 @@ class Node(CleanSave, TimestampedModel):
                 allow_power_cycle=True, config=config)
         except Exception as error:
             self.status = old_status
+            self.enable_ssh = False
+            self.skip_bmc_config = False
+            self.skip_networking = False
+            self.skip_storage = False
             self.save()
             maaslog.error(
                 "%s: Could not start node for commissioning: %s",
@@ -2146,6 +2150,7 @@ class Node(CleanSave, TimestampedModel):
             cycling = self._power_cycle()
         except Exception as error:
             self.status = old_status
+            self.enable_ssh = False
             self.save()
             maaslog.error(
                 "%s: Could not start testing for node: %s",
diff --git a/src/maasserver/models/tests/test_node.py b/src/maasserver/models/tests/test_node.py
index c29dd5a..7254699 100644
--- a/src/maasserver/models/tests/test_node.py
+++ b/src/maasserver/models/tests/test_node.py
@@ -3018,7 +3018,9 @@ class TestNode(MAASServerTestCase):
 
         try:
             with transaction.atomic():
-                node.start_commissioning(admin)
+                node.start_commissioning(
+                    admin, enable_ssh=True, skip_bmc_config=True,
+                    skip_networking=True, skip_storage=True)
         except node_start.side_effect.__class__:
             # We don't care about the error here, so suppress it. It
             # exists only to cause the transaction to abort.
@@ -3030,6 +3032,10 @@ class TestNode(MAASServerTestCase):
                 admin, generate_user_data_for_status.return_value,
                 NODE_STATUS.NEW, allow_power_cycle=True, config=config))
         self.assertEqual(NODE_STATUS.NEW, node.status)
+        self.assertFalse(node.enable_ssh)
+        self.assertFalse(node.skip_bmc_config)
+        self.assertFalse(node.skip_networking)
+        self.assertFalse(node.skip_storage)
 
     def test_start_commissioning_logs_and_raises_errors_in_starting(self):
         admin = factory.make_admin()
@@ -3380,7 +3386,8 @@ class TestNode(MAASServerTestCase):
 
         try:
             with transaction.atomic():
-                node.start_testing(admin, testing_scripts=[script.name])
+                node.start_testing(
+                    admin, enable_ssh=True, testing_scripts=[script.name])
         except mock_node_power_cycle.side_effect.__class__:
             # We don't care about the error here, so suppress it. It
             # exists only to cause the transaction to abort.
@@ -3388,6 +3395,7 @@ class TestNode(MAASServerTestCase):
 
         self.expectThat(mock_node_power_cycle, MockCalledOnceWith())
         self.expectThat(NODE_STATUS.DEPLOYED, Equals(node.status))
+        self.assertFalse(node.enable_ssh)
 
     def test_start_testing_logs_and_raises_errors(self):
         script = factory.make_Script(script_type=SCRIPT_TYPE.TESTING)
diff --git a/src/metadataserver/builtin_scripts/hooks.py b/src/metadataserver/builtin_scripts/hooks.py
index 0012e40..e0509ca 100644
--- a/src/metadataserver/builtin_scripts/hooks.py
+++ b/src/metadataserver/builtin_scripts/hooks.py
@@ -164,6 +164,9 @@ def update_node_network_information(node, output, exit_status):
 
     # Skip network configuration if set by the user.
     if node.skip_networking:
+        # Turn off skip_networking now that the hook has been called.
+        node.skip_networking = False
+        node.save(update_fields=['skip_networking'])
         return
 
     # Get the MAC addresses of all connected interfaces.
@@ -498,6 +501,9 @@ def update_node_physical_block_devices(node, output, exit_status):
 
     # Skip storage configuration if set by the user.
     if node.skip_storage:
+        # Turn off skip_storage now that the hook has been called.
+        node.skip_storage = False
+        node.save(update_fields=['skip_storage'])
         return
 
     try:
diff --git a/src/metadataserver/builtin_scripts/tests/test_hooks.py b/src/metadataserver/builtin_scripts/tests/test_hooks.py
index bf99b95..13c0f0e 100644
--- a/src/metadataserver/builtin_scripts/tests/test_hooks.py
+++ b/src/metadataserver/builtin_scripts/tests/test_hooks.py
@@ -1039,6 +1039,7 @@ class TestUpdateNodePhysicalBlockDevices(MAASServerTestCase):
         block_device = factory.make_PhysicalBlockDevice(node=node)
         update_node_physical_block_devices(node, b"garbage", exit_status=0)
         self.assertIsNotNone(reload_object(block_device))
+        self.assertFalse(reload_object(node).skip_storage)
 
     def test__removes_previous_physical_block_devices(self):
         node = factory.make_Node()
@@ -1452,6 +1453,7 @@ class TestUpdateNodeNetworkInformation(MAASServerTestCase):
         boot_interface = node.get_boot_interface()
         update_node_network_information(node, self.IP_ADDR_OUTPUT, 0)
         self.assertIsNotNone(reload_object(boot_interface))
+        self.assertFalse(reload_object(node).skip_networking)
 
     def test__add_all_interfaces(self):
         """Test a node that has no previously known interfaces on which we