Merge lp:~ltrager/maas/fix_exception into lp:~maas-committers/maas/trunk

[Lee Trager]

In restore methods use assert on the model and raise NodeStateViolation on the API

As per the discussion in the stand up this turns the validation in set_initial_networking to be an assert which will be logged no matter where it is called from. The API now checks the node state and raises a NodeStateViolation if the state is not READY.

[Blake Rouse (blake-rouse)]

Looks good. Need to fix one of the API validation messages and add a message to the assert as well.

review: Approve

[Andres Rodriguez (andreserl)]

Lee, before you alnd this, pelase attach this barnch to https://bugs.launchpad.net/maas/+bug/1580405 and fix the bug title and description to reflect this bugfix.

review: Approve

=== modified file 'src/maasserver/api/devices.py'
--- src/maasserver/api/devices.py	2016-05-04 21:14:04 +0000
+++ src/maasserver/api/devices.py	2016-05-11 21:24:10 +0000
@@ -141,7 +141,7 @@
         device = self.model.objects.get_node_or_404(
             system_id=system_id, user=request.user,
             perm=NODE_PERMISSION.ADMIN)
-        device.restore_default_configuration()
+        device.set_initial_networking_configuration()
         return reload_object(device)
 
     @classmethod

=== modified file 'src/maasserver/api/machines.py'
--- src/maasserver/api/machines.py	2016-05-09 20:13:38 +0000
+++ src/maasserver/api/machines.py	2016-05-11 21:24:10 +0000
@@ -595,6 +595,10 @@
         machine = self.model.objects.get_node_or_404(
             system_id=system_id, user=request.user,
             perm=NODE_PERMISSION.ADMIN)
+        if machine.status != NODE_STATUS.READY:
+            raise NodeStateViolation(
+                "Machine must be in a ready state to restore networking "
+                "configuration")
         machine.set_initial_networking_configuration()
         return reload_object(machine)
 
@@ -608,6 +612,10 @@
         machine = self.model.objects.get_node_or_404(
             system_id=system_id, user=request.user,
             perm=NODE_PERMISSION.ADMIN)
+        if machine.status != NODE_STATUS.READY:
+            raise NodeStateViolation(
+                "Machine must be in a ready state to restore storage "
+                "configuration.")
         machine.set_default_storage_layout()
         return reload_object(machine)
 
@@ -621,7 +629,12 @@
         machine = self.model.objects.get_node_or_404(
             system_id=system_id, user=request.user,
             perm=NODE_PERMISSION.ADMIN)
-        machine.restore_default_configuration()
+        if machine.status != NODE_STATUS.READY:
+            raise NodeStateViolation(
+                "Machine must be in a ready state to restore default "
+                "networking and storage configuration.")
+        machine.set_default_storage_layout()
+        machine.set_initial_networking_configuration()
         return reload_object(machine)
 
     @operation(idempotent=False)

=== modified file 'src/maasserver/api/tests/test_devices.py'
--- src/maasserver/api/tests/test_devices.py	2016-05-04 21:14:04 +0000
+++ src/maasserver/api/tests/test_devices.py	2016-05-11 21:24:10 +0000
@@ -376,7 +376,7 @@
         self.become_admin()
         device = factory.make_Device()
         mock_set_initial_networking_config = self.patch(
-            node_module.Device, 'restore_default_configuration')
+            node_module.Device, 'set_initial_networking_configuration')
         response = self.client.post(
             get_device_uri(device),
             {'op': 'restore_default_configuration'})

=== modified file 'src/maasserver/api/tests/test_machine.py'
--- src/maasserver/api/tests/test_machine.py	2016-05-09 23:54:19 +0000
+++ src/maasserver/api/tests/test_machine.py	2016-05-11 21:24:10 +0000
@@ -60,6 +60,7 @@
     HasLength,
     MockCalledOnce,
     MockCalledOnceWith,
+    MockNotCalled,
 )
 from metadataserver.models import (
     NodeKey,
@@ -1894,7 +1895,7 @@
         """Get the API URI for `machine`."""
         return reverse('machine_handler', args=[machine.system_id])
 
-    def test_(self):
+    def test_restore_networking_configuration(self):
         self.become_admin()
         machine = factory.make_Machine(status=NODE_STATUS.READY)
         mock_set_initial_networking_config = self.patch(
@@ -1915,6 +1916,17 @@
         self.assertEqual(
             http.client.FORBIDDEN, response.status_code, response.content)
 
+    def test_restore_networking_configuration_checks_machine_status(self):
+        self.become_admin()
+        machine = factory.make_Machine(status=NODE_STATUS.DEPLOYED)
+        mock_set_initial_networking_config = self.patch(
+            node_module.Machine, 'set_initial_networking_configuration')
+        response = self.client.post(
+            self.get_machine_uri(machine),
+            {'op': 'restore_networking_configuration'})
+        self.assertEqual(http.client.CONFLICT, response.status_code)
+        self.assertThat(mock_set_initial_networking_config, MockNotCalled())
+
 
 class TestRestoreStorageConfiguration(APITestCase):
     """Tests for
@@ -1945,6 +1957,17 @@
         self.assertEqual(
             http.client.FORBIDDEN, response.status_code, response.content)
 
+    def test_restore_storage_configuration_checks_machine_status(self):
+        self.become_admin()
+        machine = factory.make_Machine(status=NODE_STATUS.DEPLOYED)
+        mock_set_default_storage_layout = self.patch(
+            node_module.Machine, 'set_default_storage_layout')
+        response = self.client.post(
+            self.get_machine_uri(machine),
+            {'op': 'restore_storage_configuration'})
+        self.assertEqual(http.client.CONFLICT, response.status_code)
+        self.assertThat(mock_set_default_storage_layout, MockNotCalled())
+
 
 class TestRestoreDefaultConfiguration(APITestCase):
     """Tests for
@@ -1957,23 +1980,37 @@
     def test_restore_default_configuration(self):
         self.become_admin()
         machine = factory.make_Machine(status=NODE_STATUS.READY)
+        mock_set_default_storage_layout = self.patch(
+            node_module.Machine, 'set_default_storage_layout')
+        mock_set_initial_networking_config = self.patch(
+            node_module.Machine, 'set_initial_networking_configuration')
+        response = self.client.post(
+            self.get_machine_uri(machine),
+            {'op': 'restore_default_configuration'})
+        self.assertEqual(http.client.OK, response.status_code)
+        self.assertEqual(
+            machine.system_id, json_load_bytes(response.content)['system_id'])
+        self.assertThat(mock_set_default_storage_layout, MockCalledOnce())
+        self.assertThat(mock_set_initial_networking_config, MockCalledOnce())
+
+    def test_restore_default_configuration_requires_admin(self):
+        machine = factory.make_Machine()
+        response = self.client.post(
+            self.get_machine_uri(machine),
+            {'op': 'restore_default_configuration'})
+        self.assertEqual(
+            http.client.FORBIDDEN, response.status_code, response.content)
+
+    def test_restore_default_configuration_checks_machine_status(self):
+        self.become_admin()
+        machine = factory.make_Machine(status=NODE_STATUS.DEPLOYED)
         mock_restore_default_configuration = self.patch(
             node_module.Machine, 'restore_default_configuration')
         response = self.client.post(
             self.get_machine_uri(machine),
             {'op': 'restore_default_configuration'})
-        self.assertEqual(http.client.OK, response.status_code)
-        self.assertEqual(
-            machine.system_id, json_load_bytes(response.content)['system_id'])
-        self.assertThat(mock_restore_default_configuration, MockCalledOnce())
-
-    def test_restore_default_configuration_requires_admin(self):
-        machine = factory.make_Machine()
-        response = self.client.post(
-            self.get_machine_uri(machine),
-            {'op': 'restore_default_configuration'})
-        self.assertEqual(
-            http.client.FORBIDDEN, response.status_code, response.content)
+        self.assertEqual(http.client.CONFLICT, response.status_code)
+        self.assertThat(mock_restore_default_configuration, MockNotCalled())
 
 
 class TestMarkBroken(APITestCase):

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2016-05-11 02:26:59 +0000
+++ src/maasserver/models/node.py	2016-05-11 21:24:10 +0000
@@ -2424,11 +2424,9 @@
             # No interfaces on the node. Nothing to do.
             return
 
-        if self.node_type == NODE_TYPE.MACHINE and self.status in [
-                NODE_STATUS.DEPLOYING, NODE_STATUS.DEPLOYED]:
-            raise ValidationError(
-                "Machine must be in a new, ready, allocated, or failed "
-                "deployment state to be reset.")
+        assert \
+            self.status not in [NODE_STATUS.DEPLOYING, NODE_STATUS.DEPLOYED], \
+            'Node cannot be in a deploying state when configuring network'
 
         # Set AUTO mode on the boot interface.
         auto_set = False
@@ -3073,17 +3071,6 @@
         super(Machine, self).__init__(
             node_type=NODE_TYPE.MACHINE, *args, **kwargs)
 
-    def restore_default_configuration(self):
-        """Restores a machine's configuration to default settings."""
-        if self.status not in [
-                NODE_STATUS.NEW, NODE_STATUS.READY, NODE_STATUS.ALLOCATED,
-                NODE_STATUS.FAILED_DEPLOYMENT]:
-            raise ValidationError(
-                "Machine must be in a new, ready, allocated, or failed "
-                "deployment state to restore default configuration.")
-        self.set_default_storage_layout()
-        self.set_initial_networking_configuration()
-
 
 class Controller(Node):
     """A node which is either a rack or region controller."""
@@ -3825,10 +3812,6 @@
         super(Device, self).__init__(
             node_type=NODE_TYPE.DEVICE, *args, **kwargs)
 
-    def restore_default_configuration(self):
-        """Restores a device's configuration to default settings."""
-        self.set_initial_networking_configuration()
-
     def clean_architecture(self, prev):
         # Devices aren't required to have a defined architecture
         pass

=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2016-05-11 02:26:59 +0000
+++ src/maasserver/models/tests/test_node.py	2016-05-11 21:24:10 +0000
@@ -296,30 +296,6 @@
             list(Machine.objects.get_available_machines_for_acquisition(user)))
 
 
-class TestMachine(MAASServerTestCase):
-    def test_restore_default_configuration(self):
-        machine = factory.make_Machine(status=NODE_STATUS.NEW)
-        mock_set_default_storage_layout = self.patch(
-            machine, 'set_default_storage_layout')
-        mock_set_initial_networking_configuration = self.patch(
-            machine, 'set_initial_networking_configuration')
-        machine.restore_default_configuration()
-        self.assertThat(mock_set_default_storage_layout, MockCalledOnce())
-        self.assertThat(
-            mock_set_initial_networking_configuration, MockCalledOnce())
-
-    def test_restore_default_configuration_error_wrong_status(self):
-        machine = factory.make_Machine_with_Interface_on_Subnet(
-            status=factory.pick_choice(
-                NODE_STATUS_CHOICES,
-                but_not=[
-                    NODE_STATUS.NEW, NODE_STATUS.READY, NODE_STATUS.ALLOCATED,
-                    NODE_STATUS.FAILED_DEPLOYMENT])
-        )
-        self.assertRaises(
-            ValidationError, machine.restore_default_configuration)
-
-
 class TestControllerManager(MAASServerTestCase):
     def test_controller_lists_node_type_rack_and_region(self):
         racks_and_regions = set()
@@ -508,17 +484,6 @@
         self.assertEqual('', device.architecture)
 
 
-class TestDevice(MAASServerTestCase):
-
-    def test_restore_default_configuration(self):
-        device = factory.make_Device()
-        mock_set_initial_networking_configuration = self.patch(
-            device, 'set_initial_networking_configuration')
-        device.restore_default_configuration()
-        self.assertThat(
-            mock_set_initial_networking_configuration, MockCalledOnce())
-
-
 class TestNode(MAASServerTestCase):
 
     def setUp(self):
@@ -3648,12 +3613,12 @@
             alloc_type=IPADDRESS_TYPE.AUTO).first()
         self.assertIsNone(auto_ip)
 
-    def test_set_initial_net_config_rasies_validation_error_wrong_status(self):
+    def test_set_initial_net_config_asserts_proper_status(self):
         machine = factory.make_Machine_with_Interface_on_Subnet(
             status=random.choice([NODE_STATUS.DEPLOYING, NODE_STATUS.DEPLOYED])
         )
         self.assertRaises(
-            ValidationError, machine.set_initial_networking_configuration)
+            AssertionError, machine.set_initial_networking_configuration)
 
     def test_set_initial_networking_configuration_auto_on_boot_nic(self):
         node = factory.make_Node_with_Interface_on_Subnet()