Merge lp:~matsubara/curtin/simplestreams into lp:~curtin-dev/curtin/trunk

[Diogo Matsubara]

Decouple image download from serving images to tests

This branch decouples ImageStore action of updating the cache of images and providing the images to tests. This is useful for debugging without updating the images as they might introduce changes that cause other failures, masking the original failure and making debugging harder.

[Ryan Harper (raharper)]

+1 to decoupling the download from the test run.

Couple of comments though:

Don't drop the checksum verification of the image; we need to keep that otherwise we can't trust the cache. If we need to be able to supply unsigned images, I think we can introduce a separate flag to disable checksums when that is needed.

It appears that the sync pulls down all images in one go, which is nice for the case where we're running the entire test suite. But if I just want to run a single test on just one release, I have to pay for all of the sync. I'd like to retain the ability to sync and run just one image.

We now have ssquery and sync data (what versions, what releases) in two places (vmtest/__init__.py and tools/vmtest-sync-images).

So, I think we need to following features:

1. a make target to pre-sync all required images
   - it needs to extract from the test cases which images and releases to pull down
     so that when we add a new test-case that requires a new release or kver or something
     this program knows that and syncs that without having to modify the source
   - images should be checksummed and the hash written out for validation on subsequent invocations.

2. ability to sync a single image based on the test-case that's being run
   - I should be able to run nosetest3 on a single test and the images should get synced if needed

3. I should be able to disable (syncing/downloading a newer image) via a flag. As you mentioned in the commit message, sometimes we don't want to sync a new image while we're debugging; in which case we want to optionally skip the sync; since this is most useful in debugging, it shouldn't be the default behavior, instead surface it through a flag.

review: Needs Fixing

[Scott Moser (smoser)]

a couple comments.
using sstream-mirror will allow you to just pass on the releases that you need.
   sstream-mirror .... release=wily
will just then filter to only wily things.

second, i think that Christian is looking at also getting us some hwe-X kernels and initramfses.
so there is likely overlap there.

[Diogo Matsubara (matsubara)]

Some additional comments from Scott over IRC:

<smoser> cpaelzer, matsubara wrt your sstream stuff, https://code.launchpad.net/~matsubara/curtin/simplestreams/+merge/275836
<smoser> it'd be wonderful if sstream-mirror realized it had the same hash and size and just reused it magically.
<smoser> it doesnt do that, but it could and shoudl. sstream-mirror wont re-download the path if it has it, but it is kind of dumb in doing that. it just says "dont download if path exists". and paths are supposed to be immutable.
<smoser> would be nice if it recognized matches even across paths though.
<smoser> as an image hash might appear in daily/foo/bar and released/foo/bar

[lp:~matsubara/curtin/simplestreams]

284. By Diogo Matsubara on 2015-11-04

address review comments: sync images on demand, fix tools/sync-images script to sync all and keep DEFAULT_FILTERS in a single place, add flag to disable/enable syncing

285. By Diogo Matsubara on 2015-11-04

merge from trunk

286. By Diogo Matsubara on 2015-11-05

sync if there's no local image or if ImageStore says so

287. By Diogo Matsubara on 2015-11-05

find which releases to tests based on the existing test cases

288. By Diogo Matsubara on 2015-11-05

add image checksum back

[Diogo Matsubara (matsubara)]

> +1 to decoupling the download from the test run.
>
> Couple of comments though:
>
> Don't drop the checksum verification of the image; we need to keep that
> otherwise we can't trust the cache. If we need to be able to supply unsigned
> images, I think we can introduce a separate flag to disable checksums when
> that is needed.

I assumed sstream-mirror would always give the correct images, that's why I dropped the verification. More on this below.

>
> It appears that the sync pulls down all images in one go, which is nice for
> the case where we're running the entire test suite. But if I just want to run
> a single test on just one release, I have to pay for all of the sync. I'd
> like to retain the ability to sync and run just one image.
>
> We now have ssquery and sync data (what versions, what releases) in two places
> (vmtest/__init__.py and tools/vmtest-sync-images).
>
> So, I think we need to following features:
>
> 1. a make target to pre-sync all required images
> - it needs to extract from the test cases which images and releases to pull
> down
> so that when we add a new test-case that requires a new release or kver
> or something
> this program knows that and syncs that without having to modify the
> source

Done. I added a helpers.find_releases() function to find the releases based on the test classes we have defined.

> - images should be checksummed and the hash written out for validation on
> subsequent invocations.

I re-added the checksum verification but ideally this would be fixed by sstream-mirror. I filed bug 1513625 and left a comment to that effect so this could be dropped once sstream-mirror does the verification for us.

>
> 2. ability to sync a single image based on the test-case that's being run
> - I should be able to run nosetest3 on a single test and the images should
> get synced if needed

Done. By default the images will get synced on deman when you run nosetests3 on a single test. Image syncing is disabled for the make vmtests target (i.e. running all tests) so we can use that in CI.

>
> 3. I should be able to disable (syncing/downloading a newer image) via a flag.
> As you mentioned in the commit message, sometimes we don't want to sync a new
> image while we're debugging; in which case we want to optionally skip the
> sync; since this is most useful in debugging, it shouldn't be the default
> behavior, instead surface it through a flag.

Added that flag.

Please take another look at my changes addressing your review comments.

Thanks for the review!

[Ryan Harper (raharper)]

I really like the usage changes. +1

Only a few comments below.

[lp:~matsubara/curtin/simplestreams]

289. By Diogo Matsubara on 2015-11-10

address review comments

[Diogo Matsubara (matsubara)]

Hi Ryan,

I addressed your comments. Thanks!

[lp:~matsubara/curtin/simplestreams]

290. By Diogo Matsubara on 2015-11-10

Incorporate Scott's changes to simplestreams branch
 - add a new get_env_var_bool() function that does the right things with env vars
 - add qemu as a dependency for tools/vmtest-system-setup
 - make IMAGE_SRC_URL settable from a env var
 - add progress output if sstream-mirror supports it
 - fix filters to be a list rather than string

[Diogo Matsubara (matsubara)]

This changes plus Scott's changes was merged in by Scott into trunk's revno 293

=== modified file 'Makefile'
--- Makefile	2015-09-18 18:55:35 +0000
+++ Makefile	2015-11-10 19:57:52 +0000
@@ -1,6 +1,8 @@
 TOP := $(abspath $(dir $(lastword $(MAKEFILE_LIST))))
 CWD := $(shell pwd)
 PYTHON ?= python3
+CURTIN_VMTEST_IMAGE_SYNC ?= False
+export CURTIN_VMTEST_IMAGE_SYNC
 
 build:
 
@@ -28,11 +30,15 @@
 		  echo "   apt-get install -qy python3-sphinx"; exit 1; } 1>&2
 	make -C doc html
 
+# By default don't sync images when running all tests.
 vmtest:
 	nosetests3 $(noseopts) tests/vmtests
 
 vmtest-deps:
 	@$(CWD)/tools/vmtest-system-setup
 
+sync-images:
+	@$(CWD)/tools/vmtest-sync-images
+
 
 .PHONY: all test pyflakes pyflakes3 pep8 build

=== modified file 'doc/devel/README-vmtest.txt'
--- doc/devel/README-vmtest.txt	2015-10-26 10:07:30 +0000
+++ doc/devel/README-vmtest.txt	2015-11-10 19:57:52 +0000
@@ -52,8 +52,8 @@
     sudo ./backdoor-image -v --user=<USER> --password-auth --password=<PW> IMG
 At step 6 -> 7
   - You might want to keep all the temporary images around.
-    To do so you can set KEEP_VMTEST_DATA to true:
-    export KEEP_VMTEST_DATA=true
+    To do so you can set CURTIN_VMTEST_KEEP_DATA to true:
+    export CURTIN_VMTEST_KEEP_DATA=true
     That will keep the /tmp/tmpXXXXX directories and all files in there for
     further execution.
 At step 7
@@ -95,6 +95,6 @@
     look at the host's apt config and read 'Acquire::HTTP::Proxy'
 
 If required for further debugging one can set the environment variable
-"KEEP_VMTEST_DATA" like:
-export KEEP_VMTEST_DATA=true
+"CURTIN_VMTEST_KEEP_DATA" like:
+export CURTIN_VMTEST_KEEP_DATA=true
 This will keep the temporary disk files around by skipping __init__.py:__del__

=== modified file 'tests/vmtests/__init__.py'
--- tests/vmtests/__init__.py	2015-10-26 19:54:04 +0000
+++ tests/vmtests/__init__.py	2015-11-10 19:57:52 +0000
@@ -4,16 +4,26 @@
 import logging
 import json
 import os
+import pathlib
 import re
 import shutil
 import subprocess
 import tempfile
 import textwrap
+import urllib
 import curtin.net as curtin_net
 
 from .helpers import check_call
 
+IMAGE_SRC_URL = os.environ.get(
+    'IMAGE_SRC_URL',
+    "http://maas.ubuntu.com/images/ephemeral-v2/daily/streams/v1/index.sjson")
+
 IMAGE_DIR = os.environ.get("IMAGE_DIR", "/srv/images")
+DEFAULT_SSTREAM_OPTS = [
+    '--max=1', '--keyring=/usr/share/keyrings/ubuntu-cloudimage-keyring.gpg']
+DEFAULT_FILTERS = ['arch=amd64', 'item_name=root-image.gz']
+
 
 DEVNULL = open(os.devnull, 'w')
 
@@ -39,53 +49,90 @@
 
 
 class ImageStore:
-    def __init__(self, base_dir):
+    """Local mirror of MAAS images simplestreams data."""
+
+    # By default sync on demand.
+    sync = True
+
+    def __init__(self, source_url, base_dir):
+        """Initialize the ImageStore.
+
+        source_url is the simplestreams source from where the images will be
+        downloaded.
+        base_dir is the target dir in the filesystem to keep the mirror.
+        """
+        self.source_url = source_url
         self.base_dir = base_dir
         if not os.path.isdir(self.base_dir):
             os.makedirs(self.base_dir)
-
-    def get_image(self, repo, release, arch):
-        # query sstream for root image
+        self.url = pathlib.Path(self.base_dir).as_uri()
+
+    def convert_image(self, root_image_path):
+        """Use maas2roottar to unpack root-image, kernel and initrd."""
+        logger.debug('Converting image format')
+        subprocess.check_call(["tools/maas2roottar", root_image_path])
+
+    def sync_images(self, filters=DEFAULT_FILTERS):
+        """Sync MAAS images from source_url simplestreams."""
+        try:
+            out = subprocess.check_output(
+                ['sstream-mirror', '--help'], stderr=subprocess.STDOUT)
+            if isinstance(out, bytes):
+                out = out.decode()
+            if '--progress' in out:
+                progress = ['--progress']
+        except subprocess.CalledProcessError:
+            progress = []
+
+        cmd = ['sstream-mirror'] + DEFAULT_SSTREAM_OPTS + progress + [
+            self.source_url, self.base_dir] + filters
+        logger.debug('Syncing images {}'.format(cmd))
+        out = subprocess.check_output(cmd)
+        logger.debug(out)
+
+    def get_image(self, release, arch, filters=None):
+        """Return local path for root image, kernel and initrd."""
+        if filters is None:
+            filters = [
+                'release=%s' % release, 'krel=%s' % release, 'arch=%s' % arch]
+        # Query local sstream for compressed root image.
         logger.debug(
-            'Query simplestreams for root image: '
-            'release={release} arch={arch}'.format(release=release, arch=arch))
-        cmd = ["tools/usquery", "--max=1", repo, "release=%s" % release,
-               "krel=%s" % release, "arch=%s" % arch,
-               "item_name=root-image.gz"]
+            'Query simplestreams for root image: %s', filters)
+        cmd = ['sstream-query'] + DEFAULT_SSTREAM_OPTS + [
+            self.url, 'item_name=root-image.gz'] + filters
         logger.debug(" ".join(cmd))
         out = subprocess.check_output(cmd)
         logger.debug(out)
+        # No output means we have no images locally, so try to sync.
+        # If there's output, ImageStore.sync decides if images should be
+        # synced or not.
+        if not out or self.sync:
+            self.sync_images(filters=filters)
+            out = subprocess.check_output(cmd)
         sstream_data = ast.literal_eval(bytes.decode(out))
-
-        # Check if we already have the image
-        logger.debug('Checking cache for image')
-        checksum = ""
-        release_dir = os.path.join(self.base_dir, repo, release, arch,
-                                   sstream_data['version_name'])
-
-        def p(x):
-            return os.path.join(release_dir, x)
-
-        if os.path.isdir(release_dir) and os.path.exists(p("root-image.gz")):
-            # get checksum of cached image
-            with open(p("root-image.gz"), "rb") as fp:
-                checksum = hashlib.sha256(fp.read()).hexdigest()
-        if not os.path.exists(p("root-image.gz")) or \
-                checksum != sstream_data['sha256'] or \
-                not os.path.exists(p("root-image")) or \
-                not os.path.exists(p("root-image-kernel")) or \
-                not os.path.exists(p("root-image-initrd")):
-            # clear dir, download, and extract image
-            logger.debug('Image not found, downloading...')
-            shutil.rmtree(release_dir, ignore_errors=True)
-            os.makedirs(release_dir)
-            subprocess.check_call(
-                ["wget", "-c", sstream_data['item_url'], "-O",
-                 p("root-image.gz")])
-            logger.debug('Converting image format')
-            subprocess.check_call(["tools/maas2roottar", p("root-image.gz")])
-        return (p("root-image"), p("root-image-kernel"),
-                p("root-image-initrd"))
+        root_image_gz = urllib.parse.urlsplit(sstream_data['item_url']).path
+        # Make sure the image checksums correctly. Ideally
+        # sstream-[query,mirror] would make the verification for us.
+        # See https://bugs.launchpad.net/simplestreams/+bug/1513625
+        with open(root_image_gz, "rb") as fp:
+            checksum = hashlib.sha256(fp.read()).hexdigest()
+        if checksum != sstream_data['sha256']:
+            self.sync_images(filters=filters)
+            out = subprocess.check_output(cmd)
+            sstream_data = ast.literal_eval(bytes.decode(out))
+            root_image_gz = urllib.parse.urlsplit(
+                sstream_data['item_url']).path
+
+        image_dir = os.path.dirname(root_image_gz)
+        root_image_path = os.path.join(image_dir, 'root-image')
+        kernel_path = os.path.join(image_dir, 'root-image-kernel')
+        initrd_path = os.path.join(image_dir, 'root-image-initrd')
+        # Check if we need to unpack things from the compressed image.
+        if (not os.path.exists(root_image_path) or
+                not os.path.exists(kernel_path) or
+                not os.path.exists(initrd_path)):
+            self.convert_image(root_image_gz)
+        return (root_image_path, kernel_path, initrd_path)
 
 
 class TempDir:
@@ -134,7 +181,7 @@
                               stdout=DEVNULL, stderr=subprocess.STDOUT)
 
     def __del__(self):
-        if (os.getenv('KEEP_VMTEST_DATA', "false") != "true"):
+        if get_env_var_bool('CURTIN_VMTEST_KEEP_DATA', False):
             # remove tempdir
             shutil.rmtree(self.tmpdir)
 
@@ -148,9 +195,13 @@
     def setUpClass(self):
         logger.debug('Acquiring boot image')
         # get boot img
-        image_store = ImageStore(IMAGE_DIR)
+        image_store = ImageStore(IMAGE_SRC_URL, IMAGE_DIR)
+        # Disable sync if env var is set.
+        if get_env_var_bool('CURTIN_VMTEST_IMAGE_SYNC', False):
+            logger.debug("Images not synced.")
+            image_store.sync = False
         (boot_img, boot_kernel, boot_initrd) = image_store.get_image(
-            self.repo, self.release, self.arch)
+            self.release, self.arch)
 
         # set up tempdir
         logger.debug('Setting up tempdir')
@@ -431,3 +482,18 @@
         logger.debug('Cloud config archive content (pre-json):' + part)
         parts.append({'content': part, 'type': 'text/x-shellscript'})
     return '#cloud-config-archive\n' + json.dumps(parts, indent=1)
+
+
+def get_env_var_bool(envname, default=False):
+    """get a boolean environment variable.
+
+    If environment variable is not set, use default.
+    False values are case insensitive 'false', '0', ''."""
+    if not isinstance(default, bool):
+        raise ValueError("default '%s' for '%s' is not a boolean" %
+                         (default, envname))
+    val = os.environ.get(envname)
+    if val is None:
+        return default
+
+    return val.lower() not in ("false", "0", "")

=== modified file 'tests/vmtests/helpers.py'
--- tests/vmtests/helpers.py	2015-09-23 14:59:38 +0000
+++ tests/vmtests/helpers.py	2015-11-10 19:57:52 +0000
@@ -15,9 +15,11 @@
 #
 #   You should have received a copy of the GNU Affero General Public License
 #   along with Curtin.  If not, see <http://www.gnu.org/licenses/>.
-import threading
+import os
 import subprocess
 import signal
+import threading
+from unittest import TestLoader
 
 
 class Command(object):
@@ -93,3 +95,23 @@
 def check_call(cmd, signal=signal.SIGTERM, **kwargs):
     # provide a 'check_call' like interface, but kill with a nice signal
     return Command(cmd, signal).run(**kwargs)
+
+
+def find_releases():
+    """Return a sorted list of releases defined in test cases."""
+    # Use the TestLoader to load all tests cases defined within
+    # tests/vmtests/ and figure out which releases they are testing.
+    loader = TestLoader()
+    # dir with the vmtest modules (i.e. tests/vmtests/)
+    tests_dir = os.path.dirname(__file__)
+    # The root_dir for the curtin branch. (i.e. curtin/)
+    root_dir = os.path.split(os.path.split(tests_dir)[0])[0]
+    # Find all test modules defined in curtin/tests/vmtests/
+    module_test_suites = loader.discover(tests_dir, top_level_dir=root_dir)
+    releases = set()
+    for mts in module_test_suites:
+        for class_test_suite in mts:
+            for test_case in class_test_suite:
+                if getattr(test_case, 'release', ''):
+                    releases.add(getattr(test_case, 'release'))
+    return sorted(releases)

=== added file 'tools/vmtest-sync-images'
--- tools/vmtest-sync-images	1970-01-01 00:00:00 +0000
+++ tools/vmtest-sync-images	2015-11-10 19:57:52 +0000
@@ -0,0 +1,21 @@
+#!/usr/bin/python3
+# This tool keeps a local copy of the maas images used by vmtests.
+# It keeps only the latest copy of the available images.
+import os
+import sys
+
+# Fix path so we can import ImageStore class.
+sys.path.append(os.path.join(os.path.dirname(__file__), '..'))
+
+from tests.vmtests import (
+    ImageStore, IMAGE_DIR, IMAGE_SRC_URL, DEFAULT_FILTERS)
+from tests.vmtests.helpers import find_releases
+
+
+if __name__ == '__main__':
+    # Instantiate the ImageStore object.
+    store = ImageStore(IMAGE_SRC_URL, IMAGE_DIR)
+    release_filter = 'release~{}'.format('|'.join(find_releases()))
+    DEFAULT_FILTERS.append(release_filter)
+    # Sync images.
+    store.sync_images(filters=DEFAULT_FILTERS)

=== modified file 'tools/vmtest-system-setup'
--- tools/vmtest-system-setup	2015-09-21 16:19:01 +0000
+++ tools/vmtest-system-setup	2015-11-10 19:57:52 +0000
@@ -5,6 +5,10 @@
 fail() { [ $# -eq 0 ] || error "$@"; exit 2; }
 
 rel="$(lsb_release -sc)"
+case "$(uname -m)" in
+  i?86|x86_64) qemu="qemu-system-x86";;
+  ppc*) qemu="qemu-system-ppc";;
+esac
 DEPS=(
   cloud-image-utils
   make
@@ -12,6 +16,7 @@
   python3-nose
   python3-yaml
   simplestreams
+  $qemu
   ubuntu-cloudimage-keyring
 )