Merge ~cgrabowski/maas:add_update_interface_back_to_controller_ws_handler into maas:master

[Christian Grabowski]

ensure node is up to date before dehydrating

include controller interface form

use boot interface in test

move _update_obj_tags to the Node handler for both Controller and Machine handlers

move form import to correct file

move _get_node_or_permission_error to Node handler for use with Controller and Machine handlers

move update_interface into Node handler for controller to inherit

[MAAS Lander (maas-lander)]

UNIT TESTS
-b add_update_interface_back_to_controller_ws_handler lp:~cgrabowski/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas-tester/176/consoleText
COMMIT: c04185ec71067a6e098994170d848d5506a57fe1

review: Needs Fixing

[Alexsander de Souza (alexsander-souza)]

+1

review: Approve

[MAAS Lander (maas-lander)]

UNIT TESTS
-b add_update_interface_back_to_controller_ws_handler lp:~cgrabowski/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas-tester/178/consoleText
COMMIT: 624798b11123c4696d80371ff66ab1b3487e76e8

review: Needs Fixing

[Christian Grabowski (cgrabowski)]

jenkins: !test

[MAAS Lander (maas-lander)]

UNIT TESTS
-b add_update_interface_back_to_controller_ws_handler lp:~cgrabowski/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 624798b11123c4696d80371ff66ab1b3487e76e8

review: Approve

diff --git a/src/maasserver/websockets/handlers/controller.py b/src/maasserver/websockets/handlers/controller.py
index dad5976..609526a 100644
--- a/src/maasserver/websockets/handlers/controller.py
+++ b/src/maasserver/websockets/handlers/controller.py
@@ -90,6 +90,7 @@ class ControllerHandler(NodeHandler):
             "set_script_result_suppressed",
             "set_script_result_unsuppressed",
             "get_latest_failed_testing_script_results",
+            "update_interface",
         ]
         form = ControllerForm
         exclude = [
@@ -168,7 +169,11 @@ class ControllerHandler(NodeHandler):
         data = super().dehydrate(obj, data, for_list=for_list)
 
         vlan_counts = Counter()
-        for vlan_id in obj.vlan_ids:
+        vlan_ids = [
+            interface.vlan_id
+            for interface in obj.current_config.interface_set.all()
+        ]
+        for vlan_id in vlan_ids:
             vlan_counts[self._vlans_ha[vlan_id]] += 1
 
         data.update(
@@ -184,10 +189,7 @@ class ControllerHandler(NodeHandler):
             }
         )
         if not for_list:
-            data["vlan_ids"] = [
-                interface.vlan_id
-                for interface in obj.current_config.interface_set.all()
-            ]
+            data["vlan_ids"] = vlan_ids
 
             # include certificate info if present
             certificate = obj.power_parameters.get("certificate")
diff --git a/src/maasserver/websockets/handlers/machine.py b/src/maasserver/websockets/handlers/machine.py
index 5a67786..a64d51d 100644
--- a/src/maasserver/websockets/handlers/machine.py
+++ b/src/maasserver/websockets/handlers/machine.py
@@ -45,8 +45,6 @@ from maasserver.forms.interface import (
     AcquiredBridgeInterfaceForm,
     BondInterfaceForm,
     BridgeInterfaceForm,
-    DeployedInterfaceForm,
-    InterfaceForm,
     PhysicalInterfaceForm,
     VLANInterfaceForm,
 )
@@ -655,11 +653,6 @@ class MachineHandler(NodeHandler):
                 else:
                     form.save()
 
-    def _update_obj_tags(self, obj, params):
-        if "tags" in params:
-            obj.tags = params["tags"]
-            obj.save(update_fields=["tags"])
-
     def update_disk(self, params):
         """Update disk information."""
         node = self._get_node_or_permission_error(
@@ -1079,28 +1072,6 @@ class MachineHandler(NodeHandler):
         else:
             raise ValidationError(form.errors)
 
-    def update_interface(self, params):
-        """Update the interface."""
-        node = self._get_node_or_permission_error(
-            params, permission=self._meta.edit_permission
-        )
-        interface = Interface.objects.get(
-            node_config__node=node, id=params["interface_id"]
-        )
-        if node.status == NODE_STATUS.DEPLOYED:
-            interface_form = DeployedInterfaceForm
-        else:
-            interface_form = InterfaceForm.get_interface_form(interface.type)
-        form = interface_form(instance=interface, data=params)
-        if form.is_valid():
-            interface = form.save()
-            self._update_obj_tags(interface, params)
-        else:
-            raise ValidationError(form.errors)
-        if "mode" in params:
-            self.link_subnet(params)
-        return self.full_dehydrate(node)
-
     def delete_interface(self, params):
         """Delete the interface."""
         node = self._get_node_or_permission_error(
@@ -1111,45 +1082,6 @@ class MachineHandler(NodeHandler):
         )
         interface.delete()
 
-    def link_subnet(self, params):
-        """Create or update the link."""
-        node = self._get_node_or_permission_error(
-            params, permission=self._meta.edit_permission
-        )
-        interface = Interface.objects.get(
-            node_config__node=node, id=params["interface_id"]
-        )
-        subnet = None
-        if "subnet" in params:
-            subnet = Subnet.objects.get(id=params["subnet"])
-        if "link_id" in params:
-            if interface.ip_addresses.filter(id=params["link_id"]).exists():
-                # We are updating an already existing link.  Which may have
-                # been deleted.
-                interface.update_link_by_id(
-                    params["link_id"],
-                    params["mode"],
-                    subnet,
-                    ip_address=params.get("ip_address", None),
-                )
-        else:
-            # We are creating a new link.
-            interface.link_subnet(
-                params["mode"],
-                subnet,
-                ip_address=params.get("ip_address", None),
-            )
-
-    def unlink_subnet(self, params):
-        """Delete the link."""
-        node = self._get_node_or_permission_error(
-            params, permission=self._meta.edit_permission
-        )
-        interface = Interface.objects.get(
-            node_config__node=node, id=params["interface_id"]
-        )
-        interface.unlink_subnet_by_id(params["link_id"])
-
     @asynchronous(timeout=45)
     def check_power(self, params):
         """Check the power state of the node."""
@@ -1178,12 +1110,6 @@ class MachineHandler(NodeHandler):
         d.addCallback(partial(deferToDatabase, update_state))
         return d
 
-    def _get_node_or_permission_error(self, params, permission=None):
-        node = self.get_object(params, permission=permission)
-        if node.locked:
-            raise HandlerPermissionError()
-        return node
-
     def get_workload_annotations(self, params):
         """Get the owner data for a machine, known as workload annotations."""
         machine = self._get_node_or_permission_error(
diff --git a/src/maasserver/websockets/handlers/node.py b/src/maasserver/websockets/handlers/node.py
index d6ce3f9..77b6dfc 100644
--- a/src/maasserver/websockets/handlers/node.py
+++ b/src/maasserver/websockets/handlers/node.py
@@ -10,6 +10,7 @@ from itertools import chain
 import logging
 from operator import attrgetter, itemgetter
 
+from django.core.exceptions import ValidationError
 from django.db.models import Model, Prefetch
 from lxml import etree
 
@@ -26,6 +27,11 @@ from maasserver.enum import (
     POWER_STATE_CHOICES,
 )
 from maasserver.forms import list_all_usable_architectures
+from maasserver.forms.interface import (
+    ControllerInterfaceForm,
+    DeployedInterfaceForm,
+    InterfaceForm,
+)
 from maasserver.models import (
     BlockDevice,
     BMC,
@@ -1433,3 +1439,78 @@ class NodeHandler(TimestampedModelHandler):
                 if key == "mem":
                     return self._get_dynamic_filter_options("memory")
                 return self._get_dynamic_filter_options(key)
+
+    def update_interface(self, params):
+        """Update the interface."""
+        node = self._get_node_or_permission_error(
+            params, permission=self._meta.edit_permission
+        )
+        interface = Interface.objects.get(
+            node_config__node=node, id=params["interface_id"]
+        )
+        if node.is_controller:
+            interface_form = ControllerInterfaceForm
+        elif node.status == NODE_STATUS.DEPLOYED:
+            interface_form = DeployedInterfaceForm
+        else:
+            interface_form = InterfaceForm.get_interface_form(interface.type)
+        form = interface_form(instance=interface, data=params)
+        if form.is_valid():
+            interface = form.save()
+            self._update_obj_tags(interface, params)
+        else:
+            raise ValidationError(form.errors)
+        if "mode" in params:
+            self.link_subnet(params)
+        node.refresh_from_db()
+        return self.full_dehydrate(node)
+
+    def _get_node_or_permission_error(self, params, permission=None):
+        node = self.get_object(params, permission=permission)
+        if node.locked:
+            raise HandlerPermissionError()
+        return node
+
+    def _update_obj_tags(self, obj, params):
+        if "tags" in params:
+            obj.tags = params["tags"]
+            obj.save(update_fields=["tags"])
+
+    def link_subnet(self, params):
+        """Create or update the link."""
+        node = self._get_node_or_permission_error(
+            params, permission=self._meta.edit_permission
+        )
+        interface = Interface.objects.get(
+            node_config__node=node, id=params["interface_id"]
+        )
+        subnet = None
+        if "subnet" in params:
+            subnet = Subnet.objects.get(id=params["subnet"])
+        if "link_id" in params:
+            if interface.ip_addresses.filter(id=params["link_id"]).exists():
+                # We are updating an already existing link.  Which may have
+                # been deleted.
+                interface.update_link_by_id(
+                    params["link_id"],
+                    params["mode"],
+                    subnet,
+                    ip_address=params.get("ip_address", None),
+                )
+        else:
+            # We are creating a new link.
+            interface.link_subnet(
+                params["mode"],
+                subnet,
+                ip_address=params.get("ip_address", None),
+            )
+
+    def unlink_subnet(self, params):
+        """Delete the link."""
+        node = self._get_node_or_permission_error(
+            params, permission=self._meta.edit_permission
+        )
+        interface = Interface.objects.get(
+            node_config__node=node, id=params["interface_id"]
+        )
+        interface.unlink_subnet_by_id(params["link_id"])
diff --git a/src/maasserver/websockets/handlers/tests/test_controller.py b/src/maasserver/websockets/handlers/tests/test_controller.py
index 59a3278..f961ede 100644
--- a/src/maasserver/websockets/handlers/tests/test_controller.py
+++ b/src/maasserver/websockets/handlers/tests/test_controller.py
@@ -490,3 +490,30 @@ class TestControllerHandler(MAASServerTestCase):
         controller.refresh_from_db()
         # deleting a region+rack controller changes it to just a region controller
         self.assertEqual(controller.node_type, NODE_TYPE.REGION_CONTROLLER)
+
+    def test_update_interface(self):
+        admin = factory.make_admin()
+        vlan = factory.make_VLAN()
+        handler = ControllerHandler(admin, {}, None)
+        controller = factory.make_RegionRackController()
+        request = {
+            "name": controller.boot_interface.name,
+            "mac_address": controller.boot_interface.mac_address,
+            "tags": [],
+            "fabric": vlan.fabric.id,
+            "vlan": vlan.id,
+            "mode": "link_up",
+            "link_connected": True,
+            "link_speed": 0,
+            "interface_speed": 0,
+            "formatted_link_speed": 0,
+            "formatted_interface_speed": 0,
+            "type": controller.boot_interface.type,
+            "parents": controller.boot_interface.parents,
+            "primary": None,
+            "system_id": controller.system_id,
+            "interface_id": controller.boot_interface.id,
+        }
+        handler.update_interface(request)
+        controller.refresh_from_db()
+        self.assertEqual(controller.boot_interface.vlan, vlan)