Merge lp:~jtv/maas/curtin-copy into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Change the way network preseeds are installed in Curtin: instead of writing /etc files to individual temporary locations and then moving them into place individually, write them all to one /tmp/maas/etc/ and copy the whole directory tree in one go.

This is preparation for a change that writes each network interface's configuration to a separate fragment file in /etc/network/interfaces.d. Splitting up the job makes for easier reviewing, more confident progress, and more targeted Q/A. The new approach works on my test rig.

Jeroen

[Julian Edwards (julian-edwards)]

This will need release-noting, as the last time we changed the curtin preseed we had a couple of complaints about clashing changes.

Otherwise, good stuff, this makes far more sense than the old way! Nits inline to prove I read it all.

review: Approve

[Jeroen T. Vermeulen (jtv)]

This does not change the Curtin preseed yet, but the next branch will. What you see here is just a different way of getting (parts of) it onto the installed system.

=== modified file 'src/provisioningserver/drivers/osystem/tests/test_ubuntu.py'
--- src/provisioningserver/drivers/osystem/tests/test_ubuntu.py	2014-10-01 03:58:43 +0000
+++ src/provisioningserver/drivers/osystem/tests/test_ubuntu.py	2014-11-13 08:19:50 +0000
@@ -28,6 +28,7 @@
 from provisioningserver.drivers.osystem.ubuntu import UbuntuOS
 import provisioningserver.drivers.osystem.ubuntu as ubuntu_module
 from provisioningserver.udev import compose_network_interfaces_udev_rules
+from provisioningserver.utils.curtin import compose_recursive_copy
 from testtools.matchers import (
     AllMatch,
     HasLength,
@@ -92,6 +93,13 @@
 
 class TestComposeCurtinNetworkPreseed(MAASTestCase):
 
+    def find_preseed(self, preseeds, key):
+        """Extract from list of `preseeds` the first one containing `key`."""
+        for preseed in preseeds:
+            if key in preseed:
+                return preseed
+        return None
+
     def test__returns_list_of_dicts(self):
         preseed = UbuntuOS().compose_curtin_network_preseed([], [], {}, {})
         self.assertIsInstance(preseed, list)
@@ -112,35 +120,33 @@
             late_commands['late_commands'].values(),
             AllMatch(IsInstance(list)))
 
-    def test__installs_and_moves_network_interfaces_file(self):
+    def test__writes_network_interfaces_file(self):
         interfaces_file = compose_network_interfaces([], [], {}, {})
         write_text_file = self.patch_autospec(
             ubuntu_module, 'compose_write_text_file')
-        mv_command = self.patch_autospec(ubuntu_module, 'compose_mv_command')
 
         UbuntuOS().compose_curtin_network_preseed([], [], {}, {})
 
-        temp_path = '/tmp/maas-etc-network-interfaces'
+        temp_path = '/tmp/maas/etc/network/interfaces'
         self.expectThat(
             write_text_file,
             MockAnyCall(temp_path, interfaces_file, permissions=0644))
-        self.expectThat(
-            mv_command,
-            MockAnyCall(temp_path, '/etc/network/interfaces'))
 
-    def test__installs_and_moves_udev_rules_file(self):
+    def test__writes_udev_rules_file(self):
         udev_file = compose_network_interfaces_udev_rules([])
         write_text_file = self.patch_autospec(
             ubuntu_module, 'compose_write_text_file')
-        mv_command = self.patch_autospec(ubuntu_module, 'compose_mv_command')
 
         UbuntuOS().compose_curtin_network_preseed([], [], {}, {})
 
-        temp_path = '/tmp/maas-udev-70-persistent-net.rules'
+        temp_path = '/tmp/maas/etc/udev/rules.d/70-persistent-net.rules'
         self.expectThat(
             write_text_file,
             MockAnyCall(temp_path, udev_file, permissions=0644))
-        self.expectThat(
-            mv_command,
-            MockAnyCall(
-                temp_path, '/etc/udev/rules.d/70-persistent-net.rules'))
+
+    def test__copies_temp_etc_to_real_etc(self):
+        preseed = UbuntuOS().compose_curtin_network_preseed([], [], {}, {})
+        late_commands = self.find_preseed(preseed, 'late_commands')
+        self.assertEqual(
+            {'copy_etc': compose_recursive_copy('/tmp/maas/etc', '/')},
+            late_commands['late_commands'])

=== modified file 'src/provisioningserver/drivers/osystem/ubuntu.py'
--- src/provisioningserver/drivers/osystem/ubuntu.py	2014-10-10 06:24:35 +0000
+++ src/provisioningserver/drivers/osystem/ubuntu.py	2014-11-13 08:19:50 +0000
@@ -26,7 +26,7 @@
     )
 from provisioningserver.udev import compose_network_interfaces_udev_rules
 from provisioningserver.utils.curtin import (
-    compose_mv_command,
+    compose_recursive_copy,
     compose_write_text_file,
     )
 
@@ -110,21 +110,16 @@
         write_files = {
             'write_files': {
                 'etc_network_interfaces': compose_write_text_file(
-                    '/tmp/maas-etc-network-interfaces', interfaces_file,
+                    '/tmp/maas/etc/network/interfaces', interfaces_file,
                     permissions=0644),
                 'udev_persistent_net': compose_write_text_file(
-                    '/tmp/maas-udev-70-persistent-net.rules', udev_rules,
-                    permissions=0644),
+                    '/tmp/maas/etc/udev/rules.d/70-persistent-net.rules',
+                    udev_rules, permissions=0644),
             },
         }
         late_commands = {
             'late_commands': {
-                'etc_network_interfaces': compose_mv_command(
-                    '/tmp/maas-etc-network-interfaces',
-                    '/etc/network/interfaces'),
-                'udev_persistent_net': compose_mv_command(
-                    '/tmp/maas-udev-70-persistent-net.rules',
-                    '/etc/udev/rules.d/70-persistent-net.rules')
+                'copy_etc': compose_recursive_copy('/tmp/maas/etc', '/'),
             },
         }
         return [write_files, late_commands]

=== modified file 'src/provisioningserver/utils/curtin.py'
--- src/provisioningserver/utils/curtin.py	2014-09-29 02:50:52 +0000
+++ src/provisioningserver/utils/curtin.py	2014-11-13 08:19:50 +0000
@@ -14,6 +14,7 @@
 __metaclass__ = type
 __all__ = [
     'compose_mv_command',
+    'compose_recursive_copy',
     'compose_write_text_file',
     ]
 
@@ -41,11 +42,14 @@
     as an entry in `late_commands` dict.
     """
     return [
-        'curtin',
-        'in-target',
-        '--',
-        'mv',
-        '--',
-        source,
-        dest,
+        'curtin', 'in-target', '--',
+        'mv', '--', source, dest,
+        ]
+
+
+def compose_recursive_copy(source, dest):
+    """Return preseed for running a recursive `cp` in the install target."""
+    return [
+        'curtin', 'in-target', '--',
+        'cp', '-r', '-p', '--', source, dest,
         ]

=== modified file 'src/provisioningserver/utils/tests/test_curtin.py'
--- src/provisioningserver/utils/tests/test_curtin.py	2014-09-29 02:40:27 +0000
+++ src/provisioningserver/utils/tests/test_curtin.py	2014-11-13 08:19:50 +0000
@@ -18,6 +18,7 @@
 from maastesting.testcase import MAASTestCase
 from provisioningserver.utils.curtin import (
     compose_mv_command,
+    compose_recursive_copy,
     compose_write_text_file,
     )
 from testtools.matchers import (
@@ -38,14 +39,37 @@
     def test__runs_command_in_target(self):
         command = compose_mv_command(
             factory.make_name('source'), factory.make_name('dest'))
-        self.assertEqual(['curtin', 'in-target', '--'], command[:3])
+        curtin_prefix = ['curtin', 'in-target', '--']
+        self.assertEqual(curtin_prefix, command[:len(curtin_prefix)])
 
     def test__moves_file(self):
         source = factory.make_name('source')
         dest = factory.make_name('dest')
-        self.assertEqual(
-            ['mv', '--', source, dest],
-            compose_mv_command(source, dest)[-4:])
+        command = compose_mv_command(source, dest)
+        mv_suffix = ['mv', '--', source, dest]
+        self.assertEqual(mv_suffix, command[-len(mv_suffix):])
+
+
+class TestComposeRecursiveCopy(MAASTestCase):
+
+    def test__returns_command_list(self):
+        command = compose_recursive_copy(
+            factory.make_name('source'), factory.make_name('dest'))
+        self.expectThat(command, IsInstance(list))
+        self.expectThat(command, AllMatch(IsInstance(unicode)))
+
+    def test__runs_command_in_target(self):
+        command = compose_recursive_copy(
+            factory.make_name('source'), factory.make_name('dest'))
+        curtin_prefix = ['curtin', 'in-target', '--']
+        self.assertEqual(curtin_prefix, command[:len(curtin_prefix)])
+
+    def test__copies(self):
+        source = factory.make_name('source')
+        dest = factory.make_name('dest')
+        command = compose_recursive_copy(source, dest)
+        cp_suffix = ['cp', '-r', '-p', '--', source, dest]
+        self.assertEqual(cp_suffix, command[-len(cp_suffix):])
 
 
 class TestComposeWriteTextFile(MAASTestCase):