Merge lp:~bjornt/maas/reset-network-config into lp:~maas-committers/maas/trunk

[Björn Tillenius]

When resetting the network configuration over the API, remove all
network interfaces that weren't detected during commissioning.

When resetting the network configuration over the API, remove all
network interfaces that weren't detected during commissioning.

I did it by re-running the hooks for the commissioning script that
detects the network interfaces. It was mentiond that maasserver
shouldn't import from metadata server, but I did that for now, since we
seem to have quite a few such imports already. But I'd be happy to move
code before merging, if you tell me where it should live.

I also fixed a bug where set_initial_networking_configuration would add
an additional IP to the boot interface every time it was called,
resulting in a growing list of alias interfaces when the network
configuration was reset.

[Mike Pontillo (mpontillo)]

Looks good in general. I've got a couple minor issues below, but I'll trust you to resolve them before you land this.

review: Approve

[MAAS Lander (maas-lander)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

[MAAS Lander (maas-lander)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

=== modified file 'src/maasserver/api/machines.py'
--- src/maasserver/api/machines.py	2017-05-05 14:10:46 +0000
+++ src/maasserver/api/machines.py	2017-05-18 09:01:59 +0000
@@ -730,6 +730,7 @@
             raise NodeStateViolation(
                 "Machine must be in a ready state to restore networking "
                 "configuration")
+        machine.restore_network_interfaces()
         machine.set_initial_networking_configuration()
         return reload_object(machine)
 
@@ -765,6 +766,7 @@
                 "Machine must be in a ready state to restore default "
                 "networking and storage configuration.")
         machine.set_default_storage_layout()
+        machine.restore_network_interfaces()
         machine.set_initial_networking_configuration()
         return reload_object(machine)
 

=== modified file 'src/maasserver/api/tests/test_machine.py'
--- src/maasserver/api/tests/test_machine.py	2017-04-21 16:47:02 +0000
+++ src/maasserver/api/tests/test_machine.py	2017-05-18 09:01:59 +0000
@@ -2128,6 +2128,8 @@
     def test_restore_networking_configuration(self):
         self.become_admin()
         machine = factory.make_Machine(status=NODE_STATUS.READY)
+        mock_restore_network_interfaces = self.patch(
+            node_module.Machine, 'restore_network_interfaces')
         mock_set_initial_networking_config = self.patch(
             node_module.Machine, 'set_initial_networking_configuration')
         response = self.client.post(
@@ -2137,6 +2139,7 @@
         self.assertEqual(
             machine.system_id, json_load_bytes(response.content)['system_id'])
         self.assertThat(mock_set_initial_networking_config, MockCalledOnce())
+        self.assertThat(mock_restore_network_interfaces, MockCalledOnce())
 
     def test_restore_networking_configuration_requires_admin(self):
         machine = factory.make_Machine()
@@ -2212,6 +2215,8 @@
         machine = factory.make_Machine(status=NODE_STATUS.READY)
         mock_set_default_storage_layout = self.patch(
             node_module.Machine, 'set_default_storage_layout')
+        mock_restore_network_interfaces = self.patch(
+            node_module.Machine, 'restore_network_interfaces')
         mock_set_initial_networking_config = self.patch(
             node_module.Machine, 'set_initial_networking_configuration')
         response = self.client.post(
@@ -2221,6 +2226,7 @@
         self.assertEqual(
             machine.system_id, json_load_bytes(response.content)['system_id'])
         self.assertThat(mock_set_default_storage_layout, MockCalledOnce())
+        self.assertThat(mock_restore_network_interfaces, MockCalledOnce())
         self.assertThat(mock_set_initial_networking_config, MockCalledOnce())
 
     def test_restore_default_configuration_requires_admin(self):

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2017-05-06 02:05:56 +0000
+++ src/maasserver/models/node.py	2017-05-18 09:01:59 +0000
@@ -176,6 +176,7 @@
     get_sys_info,
     refresh,
 )
+from provisioningserver.refresh.node_info_scripts import IPADDR_OUTPUT_NAME
 from provisioningserver.rpc.cluster import (
     AddChassis,
     DisableAndShutoffRackd,
@@ -3097,6 +3098,16 @@
         for interface in interfaces:
             interface.clear_all_links(clearing_config=True)
 
+    def restore_network_interfaces(self):
+        """Restore the network interface to their commissioned state."""
+        # Local import to avoid circular import problems.
+        from metadataserver.builtin_scripts.hooks import (
+            update_node_network_information)
+        script = self.current_commissioning_script_set.find_script_result(
+            script_name=IPADDR_OUTPUT_NAME)
+        update_node_network_information(
+            self, script.output, script.exit_status)
+
     def set_initial_networking_configuration(self):
         """Set the networking configuration to the default for this node.
 
@@ -3119,6 +3130,9 @@
             self.status not in [NODE_STATUS.DEPLOYING, NODE_STATUS.DEPLOYED], \
             'Node cannot be in a deploying state when configuring network'
 
+        # Clear the configuration, so that we can call this method
+        # multiple times.
+        self._clear_networking_configuration()
         # Set AUTO mode on the boot interface.
         auto_set = False
         discovered_addresses = boot_interface.ip_addresses.filter(

=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2017-05-06 02:05:56 +0000
+++ src/maasserver/models/tests/test_node.py	2017-05-18 09:01:59 +0000
@@ -8,6 +8,7 @@
 import base64
 from datetime import datetime
 import email
+import os
 import random
 import re
 from unittest.mock import (
@@ -153,6 +154,7 @@
     MockCallsMatch,
     MockNotCalled,
 )
+from metadataserver.builtin_scripts.tests import test_hooks
 from metadataserver.enum import (
     SCRIPT_STATUS,
     SCRIPT_TYPE,
@@ -176,7 +178,10 @@
     EVENT_DETAILS,
     EVENT_TYPES,
 )
-from provisioningserver.refresh.node_info_scripts import NODE_INFO_SCRIPTS
+from provisioningserver.refresh.node_info_scripts import (
+    IPADDR_OUTPUT_NAME,
+    NODE_INFO_SCRIPTS,
+)
 from provisioningserver.rpc.cluster import (
     AddChassis,
     DecomposeMachine,
@@ -5185,6 +5190,73 @@
         )
         self.assertItemsEqual(enabled_interfaces, observed_interfaces)
 
+    def test_set_initial_networking_configuration_no_multiple_auto_ips(self):
+        node = factory.make_Node_with_Interface_on_Subnet()
+        boot_interface = node.get_boot_interface()
+        subnet = boot_interface.ip_addresses.filter(
+            alloc_type=IPADDRESS_TYPE.DISCOVERED).first().subnet
+        boot_interface.link_subnet(INTERFACE_LINK_TYPE.AUTO, subnet)
+        node.set_initial_networking_configuration()
+        boot_interface = reload_object(boot_interface)
+        auto_ips = boot_interface.ip_addresses.filter(
+            alloc_type=IPADDRESS_TYPE.AUTO)
+        self.assertEqual(1, auto_ips.count())
+
+    def test_restore_commisioned_network_interfaces(self):
+        node = factory.make_Node()
+        IP_ADDR_OUTPUT_FILE = os.path.join(
+            os.path.dirname(test_hooks.__file__), 'ip_addr_results_xenial.txt')
+        with open(IP_ADDR_OUTPUT_FILE, "rb") as fd:
+            IP_ADDR_OUTPUT_XENIAL = fd.read()
+        script = factory.make_Script(
+            name=IPADDR_OUTPUT_NAME, script_type=SCRIPT_TYPE.COMMISSIONING)
+        commissioning_script_set = (
+            ScriptSet.objects.create_commissioning_script_set(
+                node, scripts=[script.name]))
+        node.current_commissioning_script_set = commissioning_script_set
+        factory.make_ScriptResult(
+            script_set=commissioning_script_set, script=script, exit_status=0,
+            output=IP_ADDR_OUTPUT_XENIAL)
+
+        # restore_network_interfaces() will set up the network intefaces
+        # specified in ip_addr_results_xenial.txt.
+        node.restore_network_interfaces()
+        self.assertEqual(
+            ["ens10", "ens11", "ens12", "ens3"],
+            sorted(interface.name for interface in node.interface_set.all()))
+
+    def test_restore_network_interfaces_extra(self):
+        node = factory.make_Node()
+        IP_ADDR_OUTPUT_FILE = os.path.join(
+            os.path.dirname(test_hooks.__file__), 'ip_addr_results_xenial.txt')
+        with open(IP_ADDR_OUTPUT_FILE, "rb") as fd:
+            IP_ADDR_OUTPUT_XENIAL = fd.read()
+        script = factory.make_Script(
+            name=IPADDR_OUTPUT_NAME, script_type=SCRIPT_TYPE.COMMISSIONING)
+        commissioning_script_set = (
+            ScriptSet.objects.create_commissioning_script_set(
+                node, scripts=[script.name]))
+        node.current_commissioning_script_set = commissioning_script_set
+        factory.make_ScriptResult(
+            script_set=commissioning_script_set, script=script, exit_status=0,
+            output=IP_ADDR_OUTPUT_XENIAL)
+        node.restore_network_interfaces()
+
+        # If extra interfaces are added, they will be removed when
+        # calling restore_network_interfaces().
+        factory.make_Interface(
+            INTERFACE_TYPE.BOND, node=node,
+            parents=node.interface_set.all()[:2])
+        factory.make_Interface(
+            INTERFACE_TYPE.VLAN, node=node,
+            parents=[node.interface_set.all()[3]])
+        factory.make_Interface(INTERFACE_TYPE.PHYSICAL, node=node)
+        factory.make_Interface(INTERFACE_TYPE.PHYSICAL, node=node)
+        node.restore_network_interfaces()
+        self.assertEqual(
+            ["ens10", "ens11", "ens12", "ens3"],
+            sorted(interface.name for interface in node.interface_set.all()))
+
 
 class TestGetBestGuessForDefaultGateways(MAASServerTestCase):
     """Tests for `Node.get_best_guess_for_default_gateways`."""

=== modified file 'src/metadataserver/builtin_scripts/hooks.py'
--- src/metadataserver/builtin_scripts/hooks.py	2017-03-30 23:28:49 +0000
+++ src/metadataserver/builtin_scripts/hooks.py	2017-05-18 09:01:59 +0000
@@ -5,6 +5,7 @@
 
 __all__ = [
     'NODE_INFO_SCRIPTS',
+    'update_node_network_information',
     ]
 
 import fnmatch
@@ -16,11 +17,15 @@
 from lxml import etree
 from maasserver.models import Fabric
 from maasserver.models.blockdevice import MIN_BLOCK_DEVICE_SIZE
-from maasserver.models.interface import PhysicalInterface
+from maasserver.models.interface import (
+    Interface,
+    PhysicalInterface,
+)
 from maasserver.models.physicalblockdevice import PhysicalBlockDevice
 from maasserver.models.tag import Tag
 from maasserver.utils.orm import get_one
 from provisioningserver.refresh.node_info_scripts import (
+    IPADDR_OUTPUT_NAME,
     LIST_MODALIASES_OUTPUT_NAME,
     LSHW_OUTPUT_NAME,
     NODE_INFO_SCRIPTS,
@@ -154,7 +159,7 @@
                     interface.vlan = None
                     interface.save(update_fields=['vlan', 'updated'])
 
-    for iface in PhysicalInterface.objects.filter(node=node):
+    for iface in Interface.objects.filter(node=node):
         if iface not in current_interfaces:
             iface.delete()
 
@@ -647,7 +652,7 @@
 NODE_INFO_SCRIPTS['00-maas-02-virtuality']['hook'] = set_virtual_tag
 NODE_INFO_SCRIPTS['00-maas-07-block-devices']['hook'] = (
     update_node_physical_block_devices)
-NODE_INFO_SCRIPTS['99-maas-03-network-interfaces']['hook'] = (
+NODE_INFO_SCRIPTS[IPADDR_OUTPUT_NAME]['hook'] = (
     update_node_network_information)
 NODE_INFO_SCRIPTS['99-maas-04-network-interfaces-with-sriov']['hook'] = (
     update_node_network_interface_tags)

=== modified file 'src/metadataserver/builtin_scripts/tests/test_hooks.py'
--- src/metadataserver/builtin_scripts/tests/test_hooks.py	2017-03-30 23:41:35 +0000
+++ src/metadataserver/builtin_scripts/tests/test_hooks.py	2017-05-18 09:01:59 +0000
@@ -1087,7 +1087,7 @@
         # ... and ensure that the interface was deleted.
         self.assertItemsEqual([], Interface.objects.filter(id=interface_id))
 
-    def test__does_not_delete_virtual_interfaces_with_shared_mac(self):
+    def test__deletes_virtual_interfaces_with_shared_mac(self):
         # Note: since this VLANInterface will be linked to the default VLAN
         # ("vid 0", which is actually invalid) the VLANInterface will
         # automatically get the name "vlan0".
@@ -1101,7 +1101,7 @@
         eth1 = factory.make_Interface(
             name="eth1", mac_address=ETH1_MAC, node=node)
 
-        vlanif = factory.make_Interface(
+        factory.make_Interface(
             INTERFACE_TYPE.VLAN, mac_address=ETH0_MAC, parents=[eth0],
             node=node)
         factory.make_Interface(
@@ -1110,8 +1110,7 @@
 
         update_node_network_information(node, self.IP_ADDR_OUTPUT, 0)
         node_interfaces = Interface.objects.filter(node=node)
-        self.assert_expected_interfaces_and_macs_exist(
-            node_interfaces, {vlanif.name: ETH0_MAC, BOND_NAME: ETH1_MAC})
+        self.assert_expected_interfaces_and_macs_exist(node_interfaces)
 
     def test__interface_names_changed(self):
         # Note: the MACs here are swapped compared to their expected values.
@@ -1182,26 +1181,22 @@
         # Make sure the interface that no longer exists has been removed.
         self.assertIsNone(reload_object(eth3))
 
-    def test__does_not_delete_virtual_interfaces_with_unique_mac(self):
+    def test__deletes_virtual_interfaces_with_unique_mac(self):
         ETH0_MAC = self.EXPECTED_INTERFACES['eth0'].get_raw()
         ETH1_MAC = self.EXPECTED_INTERFACES['eth1'].get_raw()
         BOND_MAC = '00:00:00:00:01:02'
         node = factory.make_Node()
         eth0 = factory.make_Interface(mac_address=ETH0_MAC, node=node)
         eth1 = factory.make_Interface(mac_address=ETH1_MAC, node=node)
-        vlan = factory.make_Interface(
+        factory.make_Interface(
             INTERFACE_TYPE.VLAN, node=node, parents=[eth0])
-        bond = factory.make_Interface(
+        factory.make_Interface(
             INTERFACE_TYPE.BOND, mac_address=BOND_MAC, node=node,
             parents=[eth1])
 
         update_node_network_information(node, self.IP_ADDR_OUTPUT, 0)
-        # Freshen the other objects, since they may have changed names.
-        vlan = reload_object(vlan)
-        bond = reload_object(bond)
         node_interfaces = Interface.objects.filter(node=node)
-        self.assert_expected_interfaces_and_macs_exist(
-            node_interfaces, {vlan.name: ETH0_MAC, bond.name: BOND_MAC})
+        self.assert_expected_interfaces_and_macs_exist(node_interfaces)
 
     def test__deletes_virtual_interfaces_linked_to_removed_macs(self):
         VLAN_MAC = '00:00:00:00:01:01'

=== modified file 'src/provisioningserver/refresh/node_info_scripts.py'
--- src/provisioningserver/refresh/node_info_scripts.py	2017-04-22 02:54:57 +0000
+++ src/provisioningserver/refresh/node_info_scripts.py	2017-05-18 09:01:59 +0000
@@ -4,6 +4,7 @@
 """Builtin node info scripts."""
 
 __all__ = [
+    'IPADDR_OUTPUT_NAME',
     'NODE_INFO_SCRIPTS',
     'LIST_MODALIASES_OUTPUT_NAME',
     'LLDP_OUTPUT_NAME',
@@ -23,6 +24,9 @@
 # Name of the file where the node info scripts store LLDP output.
 LLDP_OUTPUT_NAME = '99-maas-02-capture-lldp'
 
+# Name of the file where the node info scripts store ip addr output.
+IPADDR_OUTPUT_NAME = '99-maas-03-network-interfaces'
+
 
 def make_function_call_script(function, *args, **kwargs):
     """Compose a Python script that calls the given function.
@@ -642,7 +646,7 @@
         'timeout': timedelta(minutes=3),
         'run_on_controller': False,
     }),
-    ('99-maas-03-network-interfaces', {
+    (IPADDR_OUTPUT_NAME, {
         'content': IPADDR_SCRIPT.encode('ascii'),
         'hook': null_hook,
         'timeout': timedelta(seconds=10),