Merge ~ltrager/maas:remove_netplan_interface_lock into maas:master

[Lee Trager]

LP: 1835275 - Remove netplan interface lock.

Netplan creates a configuration file which locks the interface used to boot.
This ensures netbooted environments don't get disconnected. MAAS may send
a netplan configuration file which will reconfigure the boot interface. The
script runner will ensure the machine reconnects.

[Newell Jensen (newell-jensen)]

+1 but don't really see the need to match the naming convention of cloud-init unless there is an actual functional advantage of doing it this way (which was not mentioned in your comment).

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b remove_netplan_interface_lock lp:~ltrager/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/6021/consoleText

[MAAS Lander (maas-lander)]

LANDING
-b remove_netplan_interface_lock lp:~ltrager/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/6022/consoleText

eb65754... by Lee Trager on 2019-07-19

Fix failing tests

diff --git a/src/metadataserver/tests/test_api.py b/src/metadataserver/tests/test_api.py
index c6979ff..800f437 100644
--- a/src/metadataserver/tests/test_api.py
+++ b/src/metadataserver/tests/test_api.py
@@ -117,7 +117,6 @@ from testtools.matchers import (
     ContainsAll,
     ContainsDict,
     Equals,
-    HasLength,
     KeysEqual,
     StartsWith,
 )
@@ -858,7 +857,7 @@ class TestMetadataCommon(MAASServerTestCase):
         self.assertThat(content, LooksLikeCloudInit)
         self.assertThat(
             yaml.safe_load(content['cloud-init']),
-            KeysEqual("system_info"))
+            KeysEqual("system_info", "runcmd"))
 
     def test_vendor_data_node_without_def_user_includes_no_system_info(self):
         # Test vendor_data includes no system_info when the node has an owner
@@ -874,7 +873,7 @@ class TestMetadataCommon(MAASServerTestCase):
         self.assertThat(content, LooksLikeCloudInit)
         self.assertThat(
             yaml.safe_load(content['cloud-init']),
-            HasLength(0))
+            KeysEqual('runcmd'))
 
     def test_vendor_data_for_node_without_owner_includes_no_system_info(self):
         view_name = self.get_metadata_name('-meta-data')
@@ -886,7 +885,7 @@ class TestMetadataCommon(MAASServerTestCase):
         self.assertThat(content, LooksLikeCloudInit)
         self.assertThat(
             yaml.safe_load(content['cloud-init']),
-            HasLength(0))
+            KeysEqual('runcmd'))
 
     def test_vendor_data_calls_through_to_get_vendor_data(self):
         # i.e. for further information, see `get_vendor_data`.
diff --git a/src/metadataserver/tests/test_vendor_data.py b/src/metadataserver/tests/test_vendor_data.py
index a5db4fc..ad770d2 100644
--- a/src/metadataserver/tests/test_vendor_data.py
+++ b/src/metadataserver/tests/test_vendor_data.py
@@ -5,10 +5,14 @@
 
 __all__ = []
 
+import random
+
+from maasserver.enum import NODE_STATUS
 from maasserver.models import (
     Config,
     NodeMetadata,
 )
+from maasserver.node_status import COMMISSIONING_LIKE_STATUSES
 from maasserver.server_address import get_maas_facing_server_host
 from maasserver.testing.factory import factory
 from maasserver.testing.fixtures import RBACEnabled
@@ -237,7 +241,8 @@ class TestGenerateRackControllerConfiguration(MAASServerTestCase):
             }))
 
     def test_yields_configuration_when_machine_install_kvm_true(self):
-        node = factory.make_Node(osystem='ubuntu', netboot=False)
+        node = factory.make_Node(
+            status=NODE_STATUS.DEPLOYING, osystem='ubuntu', netboot=False)
         node.install_kvm = True
         configuration = get_vendor_data(node, None)
         config = str(dict(configuration))
@@ -271,7 +276,8 @@ class TestGenerateRackControllerConfiguration(MAASServerTestCase):
 
     def test_includes_smt_off_for_install_kvm_on_ppc64(self):
         node = factory.make_Node(
-            osystem='ubuntu', netboot=False, architecture='ppc64el/generic')
+            status=NODE_STATUS.DEPLOYING, osystem='ubuntu', netboot=False,
+            architecture='ppc64el/generic')
         node.install_kvm = True
         configuration = get_vendor_data(node, None)
         config = dict(configuration)
@@ -288,6 +294,26 @@ class TestGenerateRackControllerConfiguration(MAASServerTestCase):
         self.assertThat(config['runcmd'], Contains(['/etc/rc.local']))
 
 
+class TestGenerateEphemeralNetplanLockRemoval(MAASServerTestCase):
+    """Tests for `generate_ephemeral_netplan_lock_removal`."""
+
+    def test__does_nothing_if_deploying(self):
+        # MAAS transitions a machine from DEPLOYING to DEPLOYED after
+        # user_data has been requested. Make sure deploying nodes don't
+        # get this config.
+        node = factory.make_Node(status=NODE_STATUS.DEPLOYING)
+        configuration = get_vendor_data(node, None)
+        config = dict(configuration)
+        self.assertNotIn('runcmd', config)
+
+    def test__removes_lock_when_ephemeral(self):
+        node = factory.make_Node(
+            status=random.choice(COMMISSIONING_LIKE_STATUSES))
+        configuration = get_vendor_data(node, None)
+        config = dict(configuration)
+        self.assertThat(config['runcmd'], Contains('rm -rf /run/netplan'))
+
+
 class TestGenerateEphemeralDeploymentNetworkConfiguration(MAASServerTestCase):
     """Tests for `generate_ephemeral_deployment_network_configuration`."""
 
@@ -303,8 +329,10 @@ class TestGenerateEphemeralDeploymentNetworkConfiguration(MAASServerTestCase):
         config = dict(configuration)
         self.assertThat(
             config['write_files'][0]['path'],
-            Contains("/etc/netplan/config.yaml"))
-        self.assertThat(config['runcmd'], Contains("netplan apply"))
+            Contains("/etc/netplan/50-maas.yaml"))
+        # Make sure netplan's lock is removed before applying the config
+        self.assertEquals(config['runcmd'][0], 'rm -rf /run/netplan')
+        self.assertEquals(config['runcmd'][1], 'netplan apply --debug')
 
 
 class TestGenerateVcenterConfiguration(MAASServerTestCase):
@@ -312,19 +340,24 @@ class TestGenerateVcenterConfiguration(MAASServerTestCase):
 
     def test_does_nothing_if_not_vmware(self):
         mock_get_configs = self.patch(Config.objects, 'get_configs')
-        node = factory.make_Node(owner=factory.make_admin())
+        node = factory.make_Node(
+            status=NODE_STATUS.DEPLOYING, owner=factory.make_admin())
         config = get_vendor_data(node, None)
         self.assertThat(mock_get_configs, MockNotCalled())
         self.assertDictEqual({}, config)
 
     def test_returns_nothing_if_no_values_set(self):
-        node = factory.make_Node(osystem='esxi', owner=factory.make_admin())
+        node = factory.make_Node(
+            status=NODE_STATUS.DEPLOYING, osystem='esxi',
+            owner=factory.make_admin())
         node.nodemetadata_set.create(key='vcenter_registration', value='True')
         config = get_vendor_data(node, None)
         self.assertDictEqual({}, config)
 
     def test_returns_vcenter_yaml(self):
-        node = factory.make_Node(osystem='esxi', owner=factory.make_admin())
+        node = factory.make_Node(
+            status=NODE_STATUS.DEPLOYING, osystem='esxi',
+            owner=factory.make_admin())
         node.nodemetadata_set.create(key='vcenter_registration', value='True')
         vcenter = {
             'vcenter_server': factory.make_name('vcenter_server'),
@@ -343,7 +376,9 @@ class TestGenerateVcenterConfiguration(MAASServerTestCase):
 
     def test_returns_vcenter_yaml_if_rbac_admin(self):
         rbac = self.useFixture(RBACEnabled())
-        node = factory.make_Node(osystem='esxi', owner=factory.make_User())
+        node = factory.make_Node(
+            status=NODE_STATUS.DEPLOYING, osystem='esxi',
+            owner=factory.make_User())
         node.nodemetadata_set.create(key='vcenter_registration', value='True')
         rbac.store.add_pool(node.pool)
         rbac.store.allow(node.owner.username, node.pool, 'admin-machines')
@@ -364,7 +399,9 @@ class TestGenerateVcenterConfiguration(MAASServerTestCase):
 
     def test_returns_nothing_if_rbac_user(self):
         rbac = self.useFixture(RBACEnabled())
-        node = factory.make_Node(osystem='esxi', owner=factory.make_User())
+        node = factory.make_Node(
+            status=NODE_STATUS.DEPLOYING, osystem='esxi',
+            owner=factory.make_User())
         node.nodemetadata_set.create(key='vcenter_registration', value='True')
         rbac.store.add_pool(node.pool)
         rbac.store.allow(node.owner.username, node.pool, 'deploy-machines')
@@ -380,7 +417,7 @@ class TestGenerateVcenterConfiguration(MAASServerTestCase):
         self.assertDictEqual({}, config)
 
     def test_returns_nothing_if_no_user(self):
-        node = factory.make_Node(osystem='esxi')
+        node = factory.make_Node(status=NODE_STATUS.DEPLOYING, osystem='esxi')
         for i in ['server', 'username', 'password', 'datacenter']:
             key = 'vcenter_%s' % i
             Config.objects.set_config(key, factory.make_name(key))
@@ -388,7 +425,9 @@ class TestGenerateVcenterConfiguration(MAASServerTestCase):
         self.assertDictEqual({}, config)
 
     def test_returns_nothing_if_user(self):
-        node = factory.make_Node(osystem='esxi', owner=factory.make_User())
+        node = factory.make_Node(
+            status=NODE_STATUS.DEPLOYING, osystem='esxi',
+            owner=factory.make_User())
         for i in ['server', 'username', 'password', 'datacenter']:
             key = 'vcenter_%s' % i
             Config.objects.set_config(key, factory.make_name(key))
@@ -396,7 +435,9 @@ class TestGenerateVcenterConfiguration(MAASServerTestCase):
         self.assertDictEqual({}, config)
 
     def test_returns_nothing_if_vcenter_registration_not_set(self):
-        node = factory.make_Node(osystem='esxi', owner=factory.make_admin())
+        node = factory.make_Node(
+            status=NODE_STATUS.DEPLOYING, osystem='esxi',
+            owner=factory.make_admin())
         for i in ['server', 'username', 'password', 'datacenter']:
             key = 'vcenter_%s' % i
             Config.objects.set_config(key, factory.make_name(key))
diff --git a/src/metadataserver/vendor_data.py b/src/metadataserver/vendor_data.py
index b4c7adf..1e992d9 100644
--- a/src/metadataserver/vendor_data.py
+++ b/src/metadataserver/vendor_data.py
@@ -17,6 +17,7 @@ from maasserver.models import (
     Config,
     NodeMetadata,
 )
+from maasserver.node_status import COMMISSIONING_LIKE_STATUSES
 from maasserver.permissions import NodePermission
 from maasserver.preseed import get_network_yaml_settings
 from maasserver.preseed_network import NodeNetworkConfiguration
@@ -34,6 +35,7 @@ def get_vendor_data(node, proxy):
         generate_ntp_configuration(node),
         generate_rack_controller_configuration(node, proxy),
         generate_kvm_pod_configuration(node),
+        generate_ephemeral_netplan_lock_removal(node),
         generate_ephemeral_deployment_network_configuration(node),
         generate_vcenter_configuration(node),
     ))
@@ -110,6 +112,20 @@ def generate_rack_controller_configuration(node, proxy):
         ]
 
 
+def generate_ephemeral_netplan_lock_removal(node):
+    """Remove netplan's interface lock.
+
+    When booting a machine over the network netplan creates a configuration
+    file in /run/netplan for the interface used to boot. This contains the
+    settings used on boot(DHCP), what renderer was used(networkd), and marks
+    the interface as critical. Netplan will ensure interfaces marked critical
+    will always have the specified configuration applied. This overrides
+    anything put in /etc/netplan breaking custom network configuration."""
+
+    if node.status in COMMISSIONING_LIKE_STATUSES:
+        yield 'runcmd', ['rm -rf /run/netplan']
+
+
 def generate_ephemeral_deployment_network_configuration(node):
     """Generate cloud-init network configuration for ephemeral deployment."""
     if node.ephemeral_deployment:
@@ -125,10 +141,13 @@ def generate_ephemeral_deployment_network_configuration(node):
         yield "write_files", [
             {
                 'content': network_config_yaml,
-                'path': "/etc/netplan/config.yaml",
+                'path': "/etc/netplan/50-maas.yaml",
             }
         ]
-        yield "runcmd", ["netplan apply"]
+        yield "runcmd", [
+            "rm -rf /run/netplan",
+            "netplan apply --debug",
+        ]
 
 
 def generate_kvm_pod_configuration(node):