Merge lp:~jtv/maas-test/bootresources-config into lp:maas-test

[Jeroen T. Vermeulen]

Generate bootresources.yaml and upload it to the VM before importing images.

This should make the import script import just the images that are required. This includes i386/generic images as well as those for the requested architecture, all for the requested Ubuntu release series.

At least one change is completely unrelated: a utils test was in the wrong TestCase. I gave it a TestCase of its own. Along the way I also added another utility function to the test factory module.

I initially planned to upload the bootresources config into the ubuntu user's home directory on the VM, and then use the import script's --config option to load it; but Raphaël mentioned that he might like to replace the script invocation with an API call, and then I found that just uploading to /etc/maas/ wasn't much easier anyway. It's an extra sudo invocation, but it saves us having to pass that --config option. The complications for existing tests weren't so bad.

One thing that is not covered by tests, though I don't expect it will matter, is the fact that import_maas_images passes the architectures it gets from mipf_arch_list (not in __all__, so I added it, then actually sorted __all__) and not just [self.architecture].

Jeroen

[Raphaël Badin (rvb)]

[0]

Looks like this doesn't work. I extracted the generated bootresources.yaml generated by this change (http://d-jenkins.ubuntu-ci:8080/view/MAAS/job/maas-test-manual/60/console) and ran the import script manually on canonistack and got http://paste.ubuntu.com/7225943/.

[1]

fwiw, I disagree with the approach taken here. I understand it's been approved by Julian so I won't block this but I really think this is wrong.

Having maas-test generate bootresources.yaml instead of just taking its content from a command line argument will force us to change maas-test every single time we will want to get it to tweak bootresources.yaml: to use a different label, to tweak the keyring or the simplestream path. This is the same mistake we've done with the power parameters settings. maas-test should try to do as little as possible and just pass things around, act as a dumb intermediate whenever possible.

[2]

47 + self.kvm_fixture.upload_file(config.name, 'bootresources.yaml')
48 + self.kvm_fixture.run_command(
49 + ['sudo', 'mv', 'bootresources.yaml', '/etc/maas/'],
50 + check_call=True)

Shouldn't we put the file in /tmp/bootresources.yaml? This looks like it will work just fine but maybe it's best to have the entire path under our control, just to be on the safe side.

[3]

> This includes i386/generic images as well as those for the requested architecture, all for the requested Ubuntu
> release series.

This was https://bugs.launchpad.net/ubuntu/+source/maas/+bug/1181334 and it's fixed now. Since maas-test can only be used to run MAAS 1.5 and up, we might as well be rid of this and avoid downloading the i386 image when testing an amd64 node. I know it has nothing to do with you branch per se, I'm just mentioning it here so that we don't forget.

review: Needs Fixing

[Gavin Panella (allenap)]

Raphaël and I had a long conversation about this. Ultimately we agreed
that this branch out to be put on hold, and we should discuss it
properly in Austin in a few weeks. Can you live with that Jeroen?

Transcript:

allenap: rvba: I think bootresources.yaml is different to power
         parameters. The latter have a number of schemas associated with
         them. Trying to flatten that into a command-line is nearly
         impossible.
   rvba: allenap: my suggestion is to pass the entire file content to
         maas-test.
   rvba: All at once.
allenap: rvba: bootresources.yaml will be transient, but we can use
         capabilities to detect when we should use its successor.
   rvba: allenap: maas-test --bootresources <file>
allenap: rvba: Would that be optional?
   rvba: allenap: yes, of course
allenap: rvba: What’s the purpose?
   rvba: allenap: customize the boot image import: use a different
         label, use a different path, etc
allenap: rvba: I imagine we still need the code in this branch when
         it’s not supplied?
   rvba: allenap: no, we can use the default one.
allenap: rvba: Does that import everything? Using the default one makes
         sense - we’re trying to test how hardware works with MAAS
         out-of-the-box - but if it’s really slow then folk are going
         to tire of it.
allenap: rvba: This is pertinent w.r.t. the “directive” we had about
         default-to-everything.
   rvba: allenap: It imports [i386,amd64]/release/precise
         [i386/amd64]/daily/trusty by default.
allenap: rvba: Is that MAAS’s default, or maas-test’s?
   rvba: MAAS's default
allenap: rvba: When we transition away from bootresources.yaml, what
         will we do with maas-test then?
   rvba: allenap: maybe we won't transition away from
         bootresources.yaml.  Maybe the region will simply generate an
         ad-hoc bootresources.yaml every time the import script it run.
   rvba: allenap: and if we do get away with it, then that's one more
         reason no to spend time getting maas-test capable of writing
         that file.
allenap: rvba: Either way, bootresources.yaml is an implementation
         detail, temporarily exposed to users while we scurry to provide
         API+UI for it. Jeroen has written the code to generate it. I
         think we should use it for now. I suspect we’ll have to
         change maas-test again in any case.
   rvba: allenap: well, I don't agree but I guess I'm outnumbered :)
   rvba: allenap: bootresources.yaml being an implementation detail
         should be yet another reason not to spend time working on code
         to generate it.  We should expose it to user temporarily to
         give them maximum control over what gets imported.  And change
         that when MAAS itself changes.
allenap: rvba: I think we can revisit this at some point. In the
         meantime I think jtv’s branch is an improvement, and is okay
         to land.
   rvba: allenap: we won't be able to offer a unified interface in maas-
         test anyway.
   rvba: allenap: well, apart from the fact that it doesn't work at all.
allenap: rvba: Doesn’t it?
   rvba: allenap: see my point [0]
allenap: rvba: Oh yes...

[Julian Edwards (julian-edwards)]

On Wednesday 09 Apr 2014 12:19:25 you wrote:
> fwiw, I disagree with the approach taken here. I understand it's been
> approved by Julian so I won't block this but I really think this is wrong.
>
> Having maas-test generate bootresources.yaml instead of just taking its
> content from a command line argument will force us to change maas-test
> every single time we will want to get it to tweak bootresources.yaml: to
> use a different label, to tweak the keyring or the simplestream path. This
> is the same mistake we've done with the power parameters
> settings. maas-test should try to do as little as possible and just pass
> things around, act as a dumb intermediate whenever possible.

bootresources.yaml is going to disappear very soon, so I advised jtv to take
the route that involved the least amount of work.

[Julian Edwards (julian-edwards)]

On Wednesday 09 Apr 2014 14:11:32 you wrote:
> allenap: rvba: I don’t know what Julian said, but I’m not thrilled
> about adding it either. It depends somewhat on how you see
> maas-test. I see it as a front-end for non-MAAS users to test
> hardware with MAAS. As such, I don’t think asking them to
> provide config files is good UI.

This is the nub of it, yes.

bootresources.yaml is a shit user-facing interface, I mean really shit. I was
rather frustrated to see the simplestreams filter options appearing in a
config file in the original script work, but that was out of our hands as it
turns out.

We need to be moving as much of the maas-test functionality as possible over
to maas itself so that maas-test ends up being a wrapper to drive some tests
inside maas.

When we have an API/UI to control which boot resources are required (which we
will design with maas-test in mind) then this all becomes much easier.

[Jeroen T. Vermeulen (jtv)]

This is madness. We want to make people download gigabytes extra in the release version because we're arguing over a hundred lines of code we could save with a temporary solution with an unfriendly UI that is also doomed from the start? I'm not buying it.

I agree that just passing a bootresources.yaml would be easier (and take about half the code, not “10× less”) but that is not the desired user experience. The difference in code size really isn't that dramatic in absolute numbers, for a user-friendliness issue.

Also, it should be noted for the record that the branch did work properly in the experimental run, but because of it, revealed an outdated default setting in MAAS itself (which was fixed within the hour after Gavin pasted the conversation). That, not a problem with my branch, was what stopped the experiment before it could be completed — but by that point my changes had already completed their work, and MAAS had already accepted it. So far as we know or have reason to expect, no fixes to my branch are needed.

Julian says it's acceptable to keep working with the release images (which does mean maas-test will break until Trusty's release) and add an option to use pre-release images later, using a simpler user interface than letting the user configure paths and labels. It's not very hard, and doesn't differ much from the work we would need to do to pass a configuration file in from the command line.

Our chosen direction is to abolish bootresources.yaml. That file is universally regarded as a problem. We can execute on that plan, or we can keep second-guessing it, but either way my branch does not stand in its way. On the other hand, introducing bootresources.yaml in full detail into the user interface is committing publicly to the status quo.

[Julian Edwards (julian-edwards)]

On Thursday 10 Apr 2014 03:36:53 you wrote:
> Our chosen direction is to abolish bootresources.yaml. That file is
> universally regarded as a problem. We can execute on that plan, or we can
> keep second-guessing it, but either way my branch does not stand in its
> way. On the other hand, introducing bootresources.yaml in full detail into
> the user interface is committing publicly to the status quo.

Nail on head.

The user interface is the most important part. Underneath, this will start as
bootresources.yaml and silently migrate to some API calls when the work to fix
maas is done.

[Julian Edwards (julian-edwards)]

review: Approve

[lp:~jtv/maas-test/bootresources-config]

157. By Jeroen T. Vermeulen on 2014-04-10

Test for coming change: pass user/host spec to scp as part of destination name. Tests: 2 failures.

158. By Jeroen T. Vermeulen on 2014-04-10

Satisfy tests.

159. By Jeroen T. Vermeulen on 2014-04-10

Merge trunk, resolve conflicts.

160. By Jeroen T. Vermeulen on 2014-04-10

Test for coming change: make bootresources.yamll world-readable. Tests: 1 failure.

161. By Jeroen T. Vermeulen on 2014-04-10

Satisfy test.

[Jeroen T. Vermeulen (jtv)]

Turns out there were two small problems in the upload code (Raphaël, did you find these and patch around them in your Q/A run? It would have been useful to know!) but it's passing Q/A tests now. Landing.

For the record, the problems were all in the part that uploads bootresources.yaml — so all work that we would have had to do anyway even if we let users pass in a full bootresources.yaml file. The rest of the code has been no trouble at all.

=== modified file 'maastest/kvmfixture.py'
--- maastest/kvmfixture.py	2014-04-09 06:19:08 +0000
+++ maastest/kvmfixture.py	2014-04-10 06:19:43 +0000
@@ -425,8 +425,9 @@
         """
         if dest is None:
             dest = os.path.basename(source)
+        remote_dest = self.identity() + ':' + dest
         run_command(
-            ['scp'] + self._get_base_ssh_options() + [source, dest],
+            ['scp'] + self._get_base_ssh_options() + [source, remote_dest],
             check_call=True)
 
     def get_ip_from_network_scan(self, mac_address):
 
=== modified file 'maastest/maasfixture.py'
--- maastest/maasfixture.py	2014-04-09 11:15:36 +0000
+++ maastest/maasfixture.py	2014-04-10 06:19:43 +0000
@@ -23,7 +23,9 @@
 import logging
 from random import choice
 from string import ascii_letters
+from tempfile import NamedTemporaryFile
 from textwrap import dedent
+import yaml
 
 from apiclient.creds import convert_string_to_tuple
 from apiclient.maas_client import (
@@ -217,6 +219,43 @@
             'metadataserver.NodeCommissionResult'
             ])
 
+    def make_selection(self, series, architecture):
+        """Return a `selections` dict for the `bootresources.yaml` config."""
+        arch, subarch = architecture.split('/')
+        return {
+            'release': series,
+            'arches': [arch],
+            'subarches': [subarch],
+            }
+
+    def compose_import_config(self, series, architectures):
+        """Return a `bootresources.yaml` config file for the image imports."""
+        return yaml.safe_dump({
+            'boot': {
+                'sources': [
+                    {
+                        'selections': [
+                            self.make_selection(series, arch)
+                            for arch in architectures
+                            ],
+                    },
+                    ],
+                },
+            })
+
+    def upload_import_config(self, bootresources_yaml):
+        """Upload `bootresources.yaml' into the VM's `/etc/maas/`."""
+        with NamedTemporaryFile() as config:
+            config.write(bootresources_yaml)
+            config.flush()
+            self.kvm_fixture.upload_file(config.name, 'bootresources.yaml')
+        self.kvm_fixture.run_command(
+            ['sudo', 'chmod', 'a+r', 'bootresources.yaml'],
+            check_call=True)
+        self.kvm_fixture.run_command(
+            ['sudo', 'mv', 'bootresources.yaml', '/etc/maas/'],
+            check_call=True)
+
     def import_maas_images(self, series, architecture,
                            simplestreams_filter=None):
         """Import boot images into the MAAS instance.
@@ -229,13 +268,11 @@
             subarchitecture isn't specified, 'generic' is assumed.
         """
         arch_list = utils.mipf_arch_list(architecture)
-        arch_string = ' '.join(arch_list)
         logging.info(
             "Importing boot images series=%s, architectures=%s..." % (
-                series, arch_string))
-        # Import boot images.
-        # XXX jtv 2014-04-03: Configure /etc/maas/bootresources.yaml to import
-        # just the right series & architecture.
+                series, ', '.join(arch_list)))
+        self.upload_import_config(
+            self.compose_import_config(self.series, arch_list))
         self.kvm_fixture.run_command(
             [
                 'sudo',
 
=== modified file 'maastest/testing/factory.py'
--- maastest/testing/factory.py	2014-04-09 06:19:53 +0000
+++ maastest/testing/factory.py	2014-04-10 06:19:43 +0000
@@ -13,11 +13,18 @@
 
 __metaclass__ = type
 __all__ = [
+    'make_arch_with_subarch',
     'make_file',
     ]
 
 from fixtures import TempDir
 import os.path
+from random import randint
+
+
+def make_arch_with_subarch():
+    """Return an arbitrary `arch/subarch` string."""
+    return 'arch-%d/subarch-%d' % (randint(1, 10000000), randint(1, 10000000))
 
 
 def make_file(testcase, name=None, content=None):
 
=== modified file 'maastest/tests/test_kvmfixture.py'
--- maastest/tests/test_kvmfixture.py	2014-04-09 06:19:08 +0000
+++ maastest/tests/test_kvmfixture.py	2014-04-10 06:19:43 +0000
@@ -700,8 +700,11 @@
     def test_upload_file_uploads_file(self):
         self.patch(kvmfixture, 'run_command', mock.MagicMock())
         fixture = self.make_KVMFixture(kvm_timeout=None)
+        ip = '10.11.12.%d' % randint(1, 254)
+        fixture.identity = lambda: 'ubuntu@%s' % ip
         local_file = make_file(self)
         remote_file = os.path.join(gettempdir(), self.getUniqueString())
+        remote_path = 'ubuntu@%s:%s' % (ip, remote_file)
         fixture.upload_file(local_file, remote_file)
         self.assertEqual(
             [
@@ -709,7 +712,7 @@
                     (
                         ['scp'] +
                         fixture._get_base_ssh_options() +
-                        [local_file, remote_file]
+                        [local_file, remote_path]
                     ),
                     check_call=True)
             ],
@@ -718,12 +721,15 @@
     def test_upload_file_defaults_to_base_name_in_default_dir(self):
         self.patch(kvmfixture, 'run_command', mock.MagicMock())
         fixture = self.make_KVMFixture(kvm_timeout=None)
+        fixture.identity = lambda: 'ubuntu@192.168.254.99'
         local_file = make_file(self)
         fixture.upload_file(local_file)
         [command] = kvmfixture.run_command.mock_calls
         name, args, kwargs = command
         (command_line,) = args
-        self.assertEqual(os.path.basename(local_file), command_line[-1])
+        self.assertEqual(
+            fixture.identity() + ':' + os.path.basename(local_file),
+            command_line[-1])
 
 
 def xml_normalize(snippet):
 
=== modified file 'maastest/tests/test_maasfixture.py'
--- maastest/tests/test_maasfixture.py	2014-04-09 11:15:36 +0000
+++ maastest/tests/test_maasfixture.py	2014-04-10 06:19:43 +0000
@@ -22,7 +22,13 @@
 import json
 from random import randint
 from subprocess import check_call
+from testtools.matchers import (
+    FileExists,
+    HasLength,
+    Not,
+    )
 from textwrap import dedent
+import yaml
 
 from apiclient.maas_client import MAASClient
 from maastest import (
@@ -32,7 +38,10 @@
     )
 from maastest.maas_enums import NODEGROUPINTERFACE_MANAGEMENT
 from maastest.maasfixture import NoBootImagesError
-from maastest.testing.factory import make_file
+from maastest.testing.factory import (
+    make_arch_with_subarch,
+    make_file,
+    )
 import mock
 import netaddr
 from six import text_type
@@ -68,11 +77,12 @@
             'droopy', 'i860', proxy_url="http://example.com:8080")
         fixture.run_command = mock.MagicMock(
             return_value=(0, 'stdout output', 'stderr output'))
+        fixture.upload_file = mock.MagicMock()
         ip_address = '.'.join('%d' % randint(1, 254) for octets in range(4))
         fixture.ip_address = mock.MagicMock(return_value=ip_address)
         return fixture
 
-    def make_maas_fixture(self, kvm_fixture=None, proxy_url=None):
+    def make_maas_fixture(self, kvm_fixture=None, proxy_url=None, arch=None):
         """Create a `MAASFixture` with arbitrary series/architecture.
 
         The maasfixture.MAASClient's post/get/put methods are replaced by a
@@ -82,7 +92,8 @@
         if kvm_fixture is None:
             kvm_fixture = self.make_kvm_fixture()
         series = self.getUniqueString()
-        arch = self.getUniqueString()
+        if arch is None:
+            arch = self.getUniqueString()
         if proxy_url is None:
             proxy_url = self.getUniqueString()
         filter = self.getUniqueString()
@@ -751,7 +762,7 @@
         series = self.getUniqueString()
         arch = self.getUniqueString()
         filter = self.getUniqueString()
-        returned_arches = ['amd64', 'i386']
+        returned_arches = ['amd64/generic', 'i386/generic']
         mock_mipf_arch_list = mock.MagicMock(return_value=returned_arches)
         self.patch(
             utils, 'mipf_arch_list', mock_mipf_arch_list)
@@ -781,6 +792,95 @@
                 'metadataserver.NodeCommissionResult']),
             maas_fixture.kvm_fixture.run_command.mock_calls)
 
+    def test_compose_import_config_uses_defaults(self):
+        maas_fixture = self.make_maas_fixture()
+        config = yaml.safe_load(
+            maas_fixture.compose_import_config(
+                maas_fixture.series, [make_arch_with_subarch()]))
+        # This config only has the bare essentials.  All other keys are left
+        # out so that MAAS can provide defaults.
+        self.assertItemsEqual(['boot'], config.keys())
+        self.assertItemsEqual(['sources'], config['boot'].keys())
+        self.assertThat(config['boot']['sources'], HasLength(1))
+        [source] = config['boot']['sources']
+        self.assertItemsEqual(['selections'], source.keys())
+
+    def test_compose_import_config_imports_requested_images(self):
+        full_arch = make_arch_with_subarch()
+        arch, subarch = full_arch.split('/')
+        maas_fixture = self.make_maas_fixture(arch=full_arch)
+        config = yaml.safe_load(maas_fixture.compose_import_config(
+            maas_fixture.series, [full_arch]))
+        [source] = config['boot']['sources']
+        self.assertEqual(
+            [{
+                'arches': [arch],
+                'subarches': [subarch],
+                'release': maas_fixture.series,
+            }],
+            source['selections'])
+
+    def test_compose_import_config_generates_one_selection_per_arch(self):
+        arches = ['arch1/subarch1', 'arch2/subarch2']
+        maas_fixture = self.make_maas_fixture()
+        config = yaml.safe_load(maas_fixture.compose_import_config(
+            maas_fixture.series, arches))
+        [source] = config['boot']['sources']
+        self.assertEqual(
+            [
+                {
+                    'arches': ['arch1'],
+                    'subarches': ['subarch1'],
+                    'release': maas_fixture.series,
+                },
+                {
+                    'arches': ['arch2'],
+                    'subarches': ['subarch2'],
+                    'release': maas_fixture.series,
+                },
+            ],
+            source['selections'])
+
+    def test_upload_import_config_writes_config_to_temp_file(self):
+
+        class RecordingUpload:
+            """Fake `upload_file` for testing.
+
+            Records the given files path, and its contents.
+            """
+            def __call__(self, local_file, remote_file=None):
+                self.path = local_file
+                with open(self.path, 'rb') as open_file:
+                    self.content = open_file.read()
+
+        maas_fixture = self.make_maas_fixture()
+        fake_upload = RecordingUpload()
+        maas_fixture.kvm_fixture.upload_file = fake_upload
+        fake_config = self.getUniqueString()
+        maas_fixture.upload_import_config(fake_config)
+
+        # The uploaded file was temporary; it no longer exists.
+        self.assertThat(fake_upload.path, Not(FileExists()))
+        # But its contents were as given.
+        self.assertEqual(fake_config, fake_upload.content)
+
+    def test_upload_import_config_uploads_bootresources_config(self):
+        maas_fixture = self.make_maas_fixture()
+        maas_fixture.upload_import_config(self.getUniqueString())
+        self.assertEqual(
+            [mock.call(mock.ANY, 'bootresources.yaml')],
+            maas_fixture.kvm_fixture.upload_file.mock_calls)
+        self.assertEqual(
+            [
+                mock.call(
+                    ['sudo', 'chmod', 'a+r', 'bootresources.yaml'],
+                    check_call=True),
+                mock.call(
+                    ['sudo', 'mv', 'bootresources.yaml', '/etc/maas/'],
+                    check_call=True),
+            ],
+            maas_fixture.kvm_fixture.run_command.mock_calls)
+
     def test_wait_until_boot_images_scanned_times_out_eventually(self):
         maas_fixture = self.make_maas_fixture()
         self.patch(
 
=== modified file 'maastest/tests/test_utils.py'
--- maastest/tests/test_utils.py	2014-04-04 17:05:59 +0000
+++ maastest/tests/test_utils.py	2014-04-10 06:19:43 +0000
@@ -35,7 +35,7 @@
     )
 
 
-class TestRunCommand(testtools.TestCase):
+class TestReadFile(testtools.TestCase):
 
     def test_read_file_returns_file_contents_as_bytes(self):
         temp_dir = self.useFixture(TempDir())
@@ -49,6 +49,9 @@
         self.assertEqual(contents, read_contents)
         self.assertIsInstance(contents, bytes)
 
+
+class TestRunCommand(testtools.TestCase):
+
     def test_run_command_calls_Popen(self):
         mock_Popen = mock.MagicMock()
         expected_retcode = self.getUniqueInteger()
 
=== modified file 'maastest/utils.py'
--- maastest/utils.py	2014-04-08 06:49:23 +0000
+++ maastest/utils.py	2014-04-10 06:19:43 +0000
@@ -13,15 +13,16 @@
 __all__ = [
     "BINARY",
     "binary_content",
+    "CasesLoader",
+    "DEFAULT_PIDFILE_DIR",
     "DEFAULT_STATE_DIR",
+    "determine_vm_architecture",
     "determine_vm_series",
-    "determine_vm_architecture",
     "get_uri",
-    "DEFAULT_PIDFILE_DIR",
+    "mipf_arch_list",
     "read_file",
     "retries",
     "run_command",
-    "CasesLoader",
     ]