Merge lp:~pwlars/uci-engine/unmodified-imagebuild into lp:uci-engine

[Paul Larson]

Strip the image builder down to just ensure we have the proper image in glance, rather than trying to modify it.

Putting this out as an MP so it's easier to look at, but this requires some other changes to the test runner also:
https://code.launchpad.net/~vila/uci-engine/new-tr-api-lander/+merge/227510
https://code.launchpad.net/~vila/uci-engine/tr-new-api/+merge/227542

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:694
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/1128/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/1128/rebuild

review: Approve (continuous-integration)

[Vincent Ladeuil (vila)]

Nice cleanup !

See comments inline.

There is still one thing surprising me here, shouldn't you change the lander too to remove the now unused parameters ?

I'll test that branch with my MPs but I expect it to work, so +1 for now ;)

Once we're both ok with them, we should look at the API transition stuff Evan mentioned.

review: Approve

[Paul Larson (pwlars)]

I think I'm going to supersede this version in favor of https://code.launchpad.net/~pwlars/uci-engine/versioned-imagebuild/+merge/227666 after our discussion about API versioning today. Also, the other version should have the lander changes needed.

Unmerged revisions

694. By Paul Larson on 2014-07-18

Modification of cloud images is no longer necessary. Now all we need to do is ensure the image is in glance, and adt-run takes care of the rest

=== modified file 'image-builder/imagebuilder/cloud_image.py'
--- image-builder/imagebuilder/cloud_image.py	2014-07-17 16:00:41 +0000
+++ image-builder/imagebuilder/cloud_image.py	2014-07-21 13:55:48 +0000
@@ -15,23 +15,17 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import argparse
-import fcntl
 import logging
 import os
-import re
-import shutil
 import subprocess
-import textwrap
-import time
 import urllib
 import urlparse
 import uuid
 
 import ci_utils
 
-from imagebuilder import ImageException
 from ci_utils.image_store import ImageStore
-from ci_utils import dump_stack, data_store, launchpad
+from ci_utils import dump_stack
 
 
 log = logging.getLogger('Imagebuilder')
@@ -42,144 +36,11 @@
     parser.add_argument(
         "-i", "--image", required=True,
         help="URL for the cloud image to modify"),
-    parser.add_argument(
-        "-r", "--repository", required=True, action='append',
-        help="List of PPAs to add")
-    parser.add_argument(
-        "-p", "--package", required=True, action='append',
-        help="List of extra packages to install")
-    parser.add_argument(
-        "-s", "--series", required=True,
-        help="Series of Ubuntu the image is based on")
 
     args = parser.parse_args()
     return args
 
 
-def _parse_ppas(lp, ppalist, series):
-    aptlines = []
-    keys = {}
-    for ppa in ppalist:
-        ppamatch = re.match("ppa:(.+)/(.+)", ppa)
-        # This will throw an exception for malformed ppas, intentional for now
-        (team, repo) = ppamatch.groups()
-        ppa = lp.people[team].getPPAByName(name=repo)
-        keys[ppa.name] = ppa.signing_key_fingerprint
-        if ppa.private:
-            # lp.me won't work, you have to get yourself by person
-            user = lp.people[launchpad.get_lp_user_name()]
-            url = user.getArchiveSubscriptionURL(archive=ppa)
-        else:
-            url = 'http://ppa.launchpad.net/{}/{}/ubuntu'.format(team, repo)
-        aptlines.append('deb {} {} main'.format(url, series))
-        aptlines.append('deb-src {} {} main'.format(url, series))
-    return aptlines, keys
-
-
-def _chrooted(chroot, cmd):
-    log.info('Running command in image: {0}'.format(cmd))
-    if type(cmd) is not list:
-        cmd = cmd.split(' ')
-    output = subprocess.check_output(
-        ['chroot', chroot] + cmd, stderr=subprocess.STDOUT)
-    if output:
-        log.info(output)
-
-
-def _apt_get_update(chroot):
-    # apt-get update isn't always reliable, give it a few tries
-    retries = 5
-    for x in range(retries):
-        try:
-            _chrooted(chroot, "/usr/bin/apt-get update")
-            break
-        except subprocess.CalledProcessError as e:
-            x += 1
-            if x == retries:
-                raise e
-            else:
-                # retry delays will be 1, 16, 81, 256, 625
-                # ie about 1s, 16s, 1.5m, 4m, 10m
-                timeout = x * x * x * x
-                fmt = 'apt-get update failed, wait %ds and try %d more times'
-                log.info(fmt, timeout, retries - x)
-                time.sleep(timeout)
-
-
-def _setup_apt_mirror(status_cb, fname):
-    try:
-        mirror = ci_utils.unit_config.get('apt_mirror')
-    except KeyError:
-        status_cb('no apt mirror configured')
-        return False
-    status_cb('updating apt mirror to: {}'.format(mirror))
-    with open(fname) as f:
-        lines = f.readlines()
-    with open(fname, 'w') as f:
-        for line in lines:
-            f.write(line.replace('archive.ubuntu.com', mirror))
-    return True
-
-
-def _setup_ppas(chroot, repos, series, status_cb):
-    status_cb('importing ppa signing keys...')
-    lp = launchpad.lp_login()
-
-    status_cb('grabbing ppa subscription urls and keys...')
-    deblines, keys = _parse_ppas(lp, repos, series)
-
-    for ppa, key in keys.iteritems():
-        status_cb('importing signing key for: %s' % ppa)
-        cmd_base = 'apt-key adv --keyserver keyserver.ubuntu.com --recv-keys '
-        _chrooted(chroot, cmd_base + key)
-
-    status_cb('adding ppas to apt list...')
-    with open('%s/etc/apt/sources.list.d/ci.list' % chroot, 'a+') as f:
-        for debline in deblines:
-            f.write('\n%s' % debline)
-
-    status_cb('running apt-get-update...')
-    _apt_get_update(chroot)
-
-
-def _run_chroot_cmds(chroot, pkglist, status_cb):
-    '''Run commands in the chroot to do update the sources and add packages
-    '''
-    status_cb('Adding requested packages to the image...')
-    os.environ['DEBIAN_FRONTEND'] = 'noninteractive'
-    _chrooted(chroot, 'apt-get install -y {}'.format(' '.join(pkglist)))
-    _chrooted(chroot, "/usr/bin/apt-get clean")
-
-    # create a big null data file and remove it to make it more compressable
-    status_cb('updating chroot to be more compressible...')
-    zerofile = '%s/zero.tmp' % chroot
-    filldata = '\0' * 4096
-    with open(zerofile, 'wb') as f:
-        while True:
-            try:
-                f.write(filldata)
-            except IOError:
-                break
-    os.unlink(zerofile)
-
-
-def _setup_grub(mountpoint, status_cb):
-    '''ensure grub uses the proper rootfs
-
-    When installing kernels grub will reconfigure but set the wrong root
-    device to boot from. This ensures its probed correctly
-    '''
-    status_cb('ensuring grub uses proper rootfs')
-    path = os.path.join(mountpoint, 'etc/default/grub.d/99-ci-eng.cfg')
-    if not os.path.exists(path):
-        # FIXME: On precise, this all appears to have changed, but I
-        # don't know yet whether this will break kernel installs
-        return
-    with open(path, 'w') as f:
-        f.write('LINUX_ROOT_DEVICE="LABEL=cloudimg-rootfs"\n')
-        f.write('GRUB_DEVICE="LABEL=cloudimg-rootfs"\n')
-
-
 def _download(url, path):
     urlpath = urlparse.urlsplit(url).path
     filename = os.path.basename(urlpath)
@@ -188,160 +49,24 @@
     return filename
 
 
-def _nbd_loaded():
-    log.info('Checking to see if nbd is loaded')
-    with open('/proc/modules') as f:
-        if '\nnbd ' in f.read():
-            return True
-        else:
-            return False
-
-
-def _load_nbd():
-    '''load the nbd kernel module unless it's loaded already'''
-    if not _nbd_loaded():
-        runcmd('modprobe nbd')
-    if not _nbd_loaded():
-        raise ImageException(
-            'nbd module is unavailable and could not be loaded')
-
-
-def runcmd(cmd):
-    log.info('Running command: {0}'.format(cmd))
-    output = subprocess.check_output(cmd.split(' '))
-    if output:
-        log.info(output)
-
-
-class CloudImage(object):
-    def __init__(self, image_path, tmpdir):
-        image_size = ci_utils.unit_config.get('image_size')
-
-        # we need to ensure only one of these runs on the system
-        log.info('waiting for cloud image lock')
-        self.lockfile = open('/tmp/cloudimage.lck', 'a+')
-        fcntl.lockf(self.lockfile, fcntl.LOCK_EX)
-
-        log.info('resizing image to be %s', image_size)
-        subprocess.check_call(['qemu-img', 'resize', image_path, image_size])
-        self.mountpoint = self._mount_image(image_path, tmpdir)
-
-    def __enter__(self):
-        return self.mountpoint
-
-    def __exit__(self, exc, value, tb):
-        self._umount_image()
-        fcntl.lockf(self.lockfile, fcntl.LOCK_UN)
-        self.lockfile.close()
-
-    def _mount_image(self, image_path, tmpdir):
-        # Find all the unused network block devices
-        mountpoint = '%s/mountpoint' % tmpdir
-        os.mkdir(mountpoint)
-        nbd_devs = [
-            dev for dev in os.listdir('/sys/block') if dev.startswith('nbd')
-            and not os.path.exists('/sys/block/%s/pid' % dev)
-        ]
-        self.nbd_dev = nbd_devs[0]
-        runcmd('qemu-nbd -c /dev/%s %s' % (self.nbd_dev, image_path))
-        for x in xrange(10):
-            if os.path.exists('/sys/block/%s/pid' % self.nbd_dev):
-                break
-            time.sleep(1)
-        else:
-            raise ImageException('Block device %s never became available' %
-                                 self.nbd_dev)
-        runcmd('udevadm settle')
-        runcmd('mount /dev/%sp1 %s' % (self.nbd_dev, mountpoint))
-
-        # Mount /proc /sys and /dev in the image
-        for bmount in ['/proc', '/sys', '/dev']:
-            runcmd('mount --bind %s %s/%s' % (bmount, mountpoint, bmount))
-
-        # Creating /run/lock
-        run_lock = os.path.join(mountpoint, 'run', 'lock')
-        os.mkdir(run_lock)
-        runcmd(
-            'mount -t tmpfs -o nodev,noexec,nosuid,size=5242880 none '
-            '{}'.format(run_lock))
-        # Backup resolv.conf and copy in the one from the local system
-        if os.path.lexists('%s/etc/resolv.conf' % mountpoint):
-            shutil.move('%s/etc/resolv.conf' % mountpoint,
-                        '%s/etc/resolv.conf.orig' % mountpoint)
-        shutil.copy('/etc/resolv.conf', '%s/etc/resolv.conf' % mountpoint)
-        policyfile = textwrap.dedent("""\
-            #!/bin/sh
-            while true; do
-            case "$1" in
-                -*) shift ;;
-                makedev) exit 0;;
-                x11-common) exit 0;;
-                *) exit 101;;
-            esac
-            done""")
-        with open('%s/usr/sbin/policy-rc.d' % mountpoint, 'w') as f:
-            f.write(policyfile)
-        os.chmod('%s/usr/sbin/policy-rc.d' % mountpoint, 0o755)
-
-        return mountpoint
-
-    def _umount_image(self):
-        for bmount in ['/proc', '/sys', '/dev']:
-            runcmd('umount %s/%s' % (self.mountpoint, bmount))
-        # Removing /run/lock
-        run_lock = os.path.join(self.mountpoint, 'run', 'lock')
-        runcmd('umount {}'.format(run_lock))
-        shutil.rmtree(run_lock)
-        if os.path.lexists('%s/etc/resolv.conf.orig' % self.mountpoint):
-            shutil.move('%s/etc/resolv.conf.orig' % self.mountpoint,
-                        '%s/etc/resolv.conf' % self.mountpoint)
-        os.unlink('%s/usr/sbin/policy-rc.d' % self.mountpoint)
-        runcmd('umount %s' % self.mountpoint)
-        runcmd('qemu-nbd -d /dev/%s' % self.nbd_dev)
-
-
-def modify_image(image, repos, series, packages, ds, status_cb=None):
-    '''Add packges and PPAs to a cloud image mounted in a chroot'''
+def get_image(image, status_cb=None):
+    '''Copy the requested cloud image to glance'''
     if not status_cb:
         status_cb = lambda x: 0   # just provide a no-op
 
-    status_cb('loading nbd...')
-    _load_nbd()
     with ci_utils.TmpDir() as tmpdir:
         status_cb('downloading %s...' % image)
         image_path = _download(image, tmpdir)
-        with CloudImage(image_path, tmpdir) as mountpoint:
-            status_cb('updating image %s...' % image)
-
-            _setup_grub(mountpoint, status_cb)
-
-            _setup_apt_mirror(
-                status_cb, os.path.join(mountpoint, 'etc/apt/sources.list'))
-            _setup_ppas(mountpoint, repos, series, status_cb)
-
-            _run_chroot_cmds(mountpoint, packages, status_cb)
 
         glance = ImageStore(ci_utils.unit_config.get_auth_config())
-        converted_image_path = os.path.join(tmpdir, 'new.img')
-        status_cb('Shrinking the converted image...')
-        runcmd('qemu-img convert -O qcow2 -c %s %s' % (
-            image_path, converted_image_path))
         upload_filename = str(uuid.uuid1()) + '.img'
         status_cb(
-            'Uploading converted image to glance as %s' % upload_filename)
-        glance.image_create(upload_filename, converted_image_path)
-        try:
-            ds.change_visibility()
-            with open(converted_image_path, 'r') as fp:
-                location = ds.put_file(upload_filename, fp)
-        except data_store.DataStoreException:
-            status_cb('ERROR: unable to upload image to swift')
-        return upload_filename, location
+            'Uploading image to glance as %s' % upload_filename)
+        glance.image_create(upload_filename, image_path)
+        return upload_filename
 
 
 def main():
-    import mock
-    ds = mock.Mock()
     dump_stack.install_stack_dump_signal()
     args = _get_args()
 
@@ -349,8 +74,7 @@
         print(msg)
 
     try:
-        return modify_image(args.image, args.repository, args.series,
-                            args.package, ds, cb)
+        return get_image(args.image, cb)
     except subprocess.CalledProcessError as e:
         print(e.output)
 

=== modified file 'image-builder/imagebuilder/run_worker.py'
--- image-builder/imagebuilder/run_worker.py	2014-07-15 20:20:00 +0000
+++ image-builder/imagebuilder/run_worker.py	2014-07-21 13:55:48 +0000
@@ -18,7 +18,7 @@
 
 from ci_utils import amqp_utils, amqp_worker
 
-from imagebuilder.cloud_image import modify_image
+from imagebuilder.cloud_image import get_image
 
 
 class ImageBuildWorker(amqp_worker.AMQPWorker):
@@ -48,21 +48,16 @@
 
     def handle_request(self, params, log):
         image = params['image']
-        repos = params['ppa_list']
-        series = params['series']
-        packages = params['package_list']
+        # FIXME: We will need the series again soon
+        # series = params['series']
         trigger = params['progress_trigger']
-        ticket_id = params['ticket_id']
-        ds = self._create_data_store(ticket_id)
 
         def status_cb(msg):
             log.info(msg)
             amqp_utils.progress_update(trigger, {'message': msg})
 
-        image_id, location = modify_image(image, repos, series, packages,
-                                          ds, status_cb)
+        image_id = get_image(image, status_cb)
         retval = {'image_id': image_id}
-        self._create_artifact(image_id, location, retval, type='IMAGE')
         return amqp_utils.progress_completed, retval
 
 

=== removed file 'image-builder/imagebuilder/tests/test_modify_cloud_image.py'
--- image-builder/imagebuilder/tests/test_modify_cloud_image.py	2014-04-16 18:36:03 +0000
+++ image-builder/imagebuilder/tests/test_modify_cloud_image.py	1970-01-01 00:00:00 +0000
@@ -1,108 +0,0 @@
-# Ubuntu CI Engine
-# Copyright 2014 Canonical Ltd.
-
-# This program is free software: you can redistribute it and/or modify it
-# under the terms of the GNU Affero General Public License version 3, as
-# published by the Free Software Foundation.
-
-# This program is distributed in the hope that it will be useful, but
-# WITHOUT ANY WARRANTY; without even the implied warranties of
-# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
-# PURPOSE.  See the GNU Affero General Public License for more details.
-
-# You should have received a copy of the GNU Affero General Public License
-# along with this program.  If not, see <http://www.gnu.org/licenses/>.
-
-import subprocess
-import tempfile
-import textwrap
-import unittest
-
-import mock
-
-from imagebuilder import cloud_image
-
-
-class TestCloudImage(unittest.TestCase):
-    def setUp(self):
-        super(TestCloudImage, self).setUp()
-
-    @mock.patch('ci_utils.unit_config')
-    def test_apt_mirror_not_configured(self, unit_config):
-        unit_config.get.side_effect = KeyError('apt_mirror')
-        status_cb = mock.Mock()
-        self.assertFalse(cloud_image._setup_apt_mirror(status_cb, '/ignored'))
-
-    @mock.patch('ci_utils.unit_config')
-    def test_apt_mirror_configured(self, unit_config):
-        unit_config.get.return_value = 'foo.archive.ubuntu.com'
-        status_cb = mock.Mock()
-
-        orig = textwrap.dedent('''\
-        deb http://archive.ubuntu.com/ubuntu saucy main restricted universe
-        deb http://archive.ubuntu.com/ubuntu saucy-updates main restricted
-        deb http://security.ubuntu.com/ubuntu saucy-security main restricted
-        ''')
-        expected = textwrap.dedent('''\
-        deb http://foo.archive.ubuntu.com/ubuntu saucy main restricted universe
-        deb http://foo.archive.ubuntu.com/ubuntu saucy-updates main restricted
-        deb http://security.ubuntu.com/ubuntu saucy-security main restricted
-        ''')
-
-        with tempfile.NamedTemporaryFile() as f:
-            f.write(orig)
-            f.flush()
-            self.assertTrue(cloud_image._setup_apt_mirror(status_cb, f.name))
-            with open(f.name) as f:
-                self.assertEqual(expected, f.read())
-
-    @mock.patch('imagebuilder.cloud_image.launchpad')
-    def test_parse_ppas(self, launchpad):
-        launchpad.get_lp_user_name.return_value = 'foo'
-
-        lp = mock.MagicMock()
-        ppa = mock.Mock()
-        ppa.name = 'foo'
-        ppa.signing_key_fingerprint = 'foofp'
-        ppa.private = True
-        lp.people['foo'].getPPAByName.return_value = ppa
-        lp.people['foo'].getArchiveSubscriptionURL.return_value = 'foo_url'
-
-        series = 'precise'
-        deblines, keys = cloud_image._parse_ppas(lp, ['ppa:/foo/bar'], series)
-        self.assertEqual({'foo': 'foofp'}, keys)
-        expected = ['deb foo_url precise main', 'deb-src foo_url precise main']
-        self.assertListEqual(expected, deblines)
-
-    @mock.patch('time.sleep')
-    @mock.patch('imagebuilder.cloud_image._chrooted')
-    def test_apt_get_update_fails(self, chrooted, sleep):
-        chrooted.side_effect = subprocess.CalledProcessError(1, 'cmd', 'out')
-        with self.assertRaises(subprocess.CalledProcessError):
-            cloud_image._apt_get_update('/blah')
-
-    @mock.patch('time.sleep')
-    @mock.patch('imagebuilder.cloud_image._chrooted')
-    def test_apt_get_update_retries(self, chrooted, sleep):
-        '''ensure it can apt-get update works after retrying'''
-        chrooted.side_effect = [
-            subprocess.CalledProcessError(1, 'cmd', 'out'),
-            subprocess.CalledProcessError(1, 'cmd', 'out'),
-            None,
-        ]
-        cloud_image._apt_get_update('/blah')
-        self.assertEqual(3, chrooted.call_count)
-        self.assertEqual(2, sleep.call_count)
-
-    @mock.patch('imagebuilder.cloud_image.launchpad')
-    @mock.patch('imagebuilder.cloud_image._chrooted')
-    @mock.patch('__builtin__.open')
-    def test_setup_ppas(self, mopen, chroot, lp):
-        status_cb = mock.Mock()
-        series = 'saucy'
-        ppas = ['ppa:user/foo', 'ppa:team/bar']
-        mopen.return_value = mock.MagicMock(spec=file)
-        fd = mopen.return_value.__enter__.return_value
-        cloud_image._setup_ppas('/chroot', ppas, series, status_cb)
-        self.assertTrue(fd.write.call_count)
-        # todo look at chroot calls

=== modified file 'image-builder/imagebuilder/tests/test_worker.py'
--- image-builder/imagebuilder/tests/test_worker.py	2014-06-27 18:05:26 +0000
+++ image-builder/imagebuilder/tests/test_worker.py	2014-07-21 13:55:48 +0000
@@ -24,26 +24,20 @@
         super(TestImagebuildWorker, self).setUp()
         self.worker = run_worker.ImageBuildWorker()
 
-    @mock.patch('imagebuilder.run_worker.modify_image')
+    @mock.patch('imagebuilder.run_worker.get_image')
     @mock.patch('ci_utils.amqp_utils.progress_completed')
-    @mock.patch('ci_utils.amqp_worker.AMQPWorker._create_data_store')
-    def test_data_store_container(self, mocked_create_data_store,
-                                  mocked_progress_completed,
-                                  mocked_modify_image):
+    def test_worker_get_image(self, mocked_progress_completed,
+                              mocked_get_image):
         params = {
             'image': 'http://fake/location',
             'ticket_id': 1,
-            'ppa_list': [],
-            'package_list': [],
             'series': 'saucy',
             'progress_trigger': 'ticket_1_trigger'
         }
 
         mocked_log = mock.MagicMock()
-        mocked_ds = mock.MagicMock()
-        mocked_modify_image.return_value = ('fake_image_id', 'fake_location')
-        mocked_create_data_store.return_value = mocked_ds
+        mocked_get_image.return_value = ('fake_image_id')
 
         self.worker.handle_request(params, mocked_log)
 
-        mocked_create_data_store.assert_called_once_with(params['ticket_id'])
+        mocked_get_image.assert_called_once()

=== modified file 'image-builder/setup.py'
--- image-builder/setup.py	2014-07-17 16:18:14 +0000
+++ image-builder/setup.py	2014-07-21 13:55:48 +0000
@@ -18,7 +18,7 @@
 import sys
 
 requires = [
-    'pyOpenSSL==0.14',
+    #'pyOpenSSL==0.14',
     'PyYAML==3.10',
     'amqplib==1.0.0',
     'launchpadlib==1.10.2',