Review: Approve
Wow thanks for the epic docstring rewrite across all integration tests.
It really is a lot easier to read.
I think all of my comments on the previous branch have been addressed. thank
you.
Since the visual diff is so large it isn't fully rendered here.
Couple inline comments remain and
=I can't comment inline for the following:
tests/cloud_tests/config.py:
- s/[Ss]anatize/[Ss]anitize/
tests/cloud_tests/setup_image.py
- s/#return_value/@return_value/
tests/cloud_tests/util.py:
- s/Utilies/Utilities/ in module docstring
- From a previous branch comment, I still think the tmpdir function defined
here is has limited benefit in redefining a wrapper that passes a
prefix='cloud_test_util', why don't we just call
tempfile.mkdtemp(prefix='cloud_test_util') in the two call sites in
tests/cloud_tests/images/lxd.py?
+1 after that set of changes. Thanks for this herculean effort. With such a
large diff it's hard to catch all the issues effectively. But, since it's
integration testing I'm not worried that we can iron out anything that remains
on subsequent passes/iterations.
Diff comments:
> diff --git a/tests/cloud_tests/bddeb.py b/tests/cloud_tests/bddeb.py
> new file mode 100644
> index 0000000..53dbf74
> --- /dev/null
> +++ b/tests/cloud_tests/bddeb.py
> @@ -0,0 +1,118 @@
> +# This file is part of cloud-init. See LICENSE file for license information.
> +
> +"""Used to build a deb."""
> +
> +from functools import partial
> +import os
> +import tempfile
> +
> +from cloudinit import util as c_util
> +from tests.cloud_tests import (config, LOG)
> +from tests.cloud_tests import (platforms, images, snapshots, instances)
> +from tests.cloud_tests.stage import (PlatformComponent, run_stage,
> run_single)
> +
> +build_deps = ['devscripts', 'equivs', 'git', 'tar']
> +
> +
> +def _out(cmd_res):
> + """Get clean output from cmd result."""
> + return cmd_res[0].strip()
> +
> +
> +def build_deb(args, instance):
> + """Build deb on system and copy out to location at args.deb.
> +
> + @param args: cmdline arguments
> + @return_value: tuple of results and fail count
> + """
> + # update remote system package list and install build deps
> + LOG.debug('installing build deps')
> + pkgs = ' '.join(build_deps)
> + cmd = 'apt-get update && apt-get install --yes {}'.format(pkgs)
> + instance.execute(['/bin/sh', '-c', cmd])
> + # TODO Remove this call once we have a ci-deps Makefile target
> + instance.execute(['mk-build-deps', '--install', '-t',
> + 'apt-get --no-install-recommends --yes', 'cloud-init'])
> +
> + # local tmpfile that must be deleted
> + local_tarball = tempfile.NamedTemporaryFile().name
It looks like we aren't deleting this right? Should it be something like the
following instead:
with tempfile.NamedTemporaryFile() as local_tarfile:
... the rest of the code indented within this context and
s/local_tarball/local_tarball.name/
> +
> + # paths to use in remote system
> + output_link = '/root/cloud-init_all.deb'
> + remote_tarball = _out(instance.execute(['mktemp']))
> + extract_dir = _out(instance.execute(['mktemp', '--directory']))
> + bddeb_path = os.path.join(extract_dir, 'packages', 'bddeb')
> + git_env = {'GIT_DIR': os.path.join(extract_dir, '.git'),
> + 'GIT_WORK_TREE': extract_dir}
> +
> + LOG.debug('creating tarball of cloud-init at: %s', local_tarball)
> + c_util.subp(['tar', 'cf', local_tarball, '--owner', 'root',
> + '--group', 'root', '-C', args.cloud_init, '.'])
> + LOG.debug('copying to remote system at: %s', remote_tarball)
> + instance.push_file(local_tarball, remote_tarball)
> +
> + LOG.debug('extracting tarball in remote system at: %s', extract_dir)
> + instance.execute(['tar', 'xf', remote_tarball, '-C', extract_dir])
> + instance.execute(['git', 'commit', '-a', '-m', 'tmp', '--allow-empty'],
> + env=git_env)
> +
> + LOG.debug('building deb in remote system at: %s', output_link)
> + bddeb_args = args.bddeb_args.split() if args.bddeb_args else []
> + instance.execute([bddeb_path, '-d'] + bddeb_args, env=git_env)
> +
> + # copy the deb back to the host system
> + LOG.debug('copying built deb to host at: %s', args.deb)
> + instance.pull_file(output_link, args.deb)
> +
> +
> +def setup_build(args):
> + """Set build system up then run build.
> +
> + @param args: cmdline arguments
> + @return_value: tuple of results and fail count
> + """
> + res = ({}, 1)
> +
> + # set up platform
> + LOG.info('setting up platform: %s', args.build_platform)
> + platform_config = config.load_platform_config(args.build_platform)
> + platform_call = partial(platforms.get_platform, args.build_platform,
> + platform_config)
> + with PlatformComponent(platform_call) as platform:
> +
> + # set up image
> + LOG.info('acquiring image for os: %s', args.build_os)
> + img_conf = config.load_os_config(platform.platform_name,
> args.build_os)
> + image_call = partial(images.get_image, platform, img_conf)
> + with PlatformComponent(image_call) as image:
> +
> + # set up snapshot
> + snapshot_call = partial(snapshots.get_snapshot, image)
> + with PlatformComponent(snapshot_call) as snapshot:
> +
> + # create instance with cloud-config to set it up
> + LOG.info('creating instance to build deb in')
> + empty_cloud_config = "#cloud-config\n{}"
> + instance_call = partial(
> + instances.get_instance, snapshot, empty_cloud_config,
> + use_desc='build cloud-init deb')
> + with PlatformComponent(instance_call) as instance:
> +
> + # build the deb
> + res = run_single('build deb on system',
> + partial(build_deb, args, instance))
> +
> + return res
> +
> +
> +def bddeb(args):
> + """Entry point for build deb.
> +
> + @param args: cmdline arguments
> + @return_value: fail count
> + """
> + LOG.info('preparing to build cloud-init deb')
> + (res, failed) = run_stage('build deb', [partial(setup_build, args)])
> + return failed
> +
> +# vi: ts=4 expandtab
> diff --git a/tests/cloud_tests/config.py b/tests/cloud_tests/config.py
> index f3a13c9..fc16058 100644
> --- a/tests/cloud_tests/config.py
> +++ b/tests/cloud_tests/config.py
> @@ -1,5 +1,7 @@
> # This file is part of cloud-init. See LICENSE file for license information.
>
> +"""Used to setup test configuration."""
Thanks for these, the more the merrier.
> +
> import glob
> import os
>
> diff --git a/tests/cloud_tests/releases.yaml b/tests/cloud_tests/releases.yaml
> index 183f78c..45deb58 100644
> --- a/tests/cloud_tests/releases.yaml
> +++ b/tests/cloud_tests/releases.yaml
> @@ -1,86 +1,253 @@
> # ============================= Release Config
> ================================
> default_release_config:
> - # all are disabled by default
> - enabled: false
> - # timeout for booting image and running cloud init
> - timeout: 120
> - # platform_ident values for the image, with data to identify the image
> - # on that platform. see platforms.base for more information
> - platform_ident: {}
> - # a script to run after a boot that is used to modify an image, before
> - # making a snapshot of the image. may be useful for removing data left
> - # behind from cloud-init booting, such as logs, to ensure that data from
> - # snapshot.launch() will not include a cloud-init.log from a boot used to
> - # create the snapshot, if cloud-init has not run
> - boot_clean_script: |
> - #!/bin/bash
> - rm -rf /var/log/cloud-init.log /var/log/cloud-init-output.log \
> - /var/lib/cloud/ /run/cloud-init/ /var/log/syslog
> + # global default configuration options
> + default:
> + # all are disabled by default
> + enabled: false
> + # timeout for booting image and running cloud init
> + boot_timeout: 120
> + # a script to run after a boot that is used to modify an image,
> before
> + # making a snapshot of the image. may be useful for removing data
> left
> + # behind from cloud-init booting, such as logs, to ensure that data
> + # from snapshot.launch() will not include a cloud-init.log from a
> boot
> + # used to create the snapshot, if cloud-init has not run
> + boot_clean_script: |
> + #!/bin/bash
> + rm -rf /var/log/cloud-init.log /var/log/cloud-init-output.log \
> + /var/lib/cloud/ /run/cloud-init/ /var/log/syslog
> + # test script to determine if system is booted fully
> + system_ready_script: |
> + # permit running or degraded state as both indicate complete boot
> + [ $(systemctl is-system-running) = 'running' -o
> + $(systemctl is-system-running) = 'degraded' ]
> + # test script to determine if cloud-init has finished
> + cloud_init_ready_script: |
> + [ -f '/run/cloud-init/result.json' ]
> + # currently used features and their uses are:
> + # features groups and additional feature settings
> + feature_groups: []
> + features: {}
> +
> + # lxd specific default configuration options
> + lxd:
> + # default sstreams server to use for lxd image retrieval
> + sstreams_server: https://us.images.linuxcontainers.org:8443
> + # keep base image, avoids downloading again next run
> + cache_base_image: true
> + # lxd images from linuxcontainers.org do not have the nocloud seed
> + # templates in place, so the image metadata must be modified
> + override_templates: true
> + # arg overrides to set image up
> + setup_overrides:
> + # lxd images from linuxcontainers.org do not come with
> + # cloud-init, so must pull cloud-init in from repo using
> + # setup_image.upgrade
> + upgrade: true
> +
> +features:
> + # all currently supported feature flags
> + all:
> + - apt # image supports apt package manager
> + - byobu # byobu is available in repositories
> + - landscape # landscape-client available in repos
> + - lxd # lxd is available in the image
> + - ppa # image supports ppas
> + - rpm # image supports rpms
> + - snap # supports snapd
> + # NOTE: the following feature flags are to work around bugs in the
> + # images, and can be removed when no longer needed
> + - hostname # setting system hostname works
It'd be a nice naming convention if the bug-related feature were named with the
LP bug.#.hostname or something so we know what fix to watch for in order to
drop the feature flag from tests in the future.
> + # NOTE: the following feature flags are to work around issues in the
> + # testcases, and can be removed when no longer needed
> + - apt_src_cont # default contents and format of sources.list matches
> + # ubuntu sources.list
> + - apt_hist_fmt # apt command history entries use full paths to apt
> + # executable rather than relative paths
> + - daylight_time # timezones are daylight not standard time
> + - apt_up_out # 'Calculating upgrade..' present in log output from
> + # apt-get dist-upgrade output
> + - engb_locale # locale en_GB.UTF-8 is available
> + - locale_gen # the /etc/locale.gen file exists
> + - no_ntpdate # 'ntpdate' is not installed by default
> + - no_file_fmt_e # the 'file' utility does not have a formatting error
> + - ppa_file_name # the name of the source file added to
> sources.list.d has
> + # the expected format for newer ubuntu releases
> + - sshd # requires ssh server to be installed by default
> + - ssh_key_fmt # ssh auth keys printed to console have expected
> format
> + - syslog # test case requires syslog to be written by default
> + - ubuntu_ntp # expect ubuntu.pool.ntp.org to be used as ntp server
> + - ubuntu_repos # test case requres ubuntu repositories to be used
> + - ubuntu_user # test case needs user with the name 'ubuntu' to
> exist
> + # NOTE: the following feature flags are to work around issues that
> may
> + # be considered bugs in cloud-init
> + - lsb_release # image has lsb_release installed, maybe should
> install
> + # if missing by default
> + - sudo # image has sudo installed, should not be required
> + # feature flag groups
> + groups:
> + base:
> + hostname: true
> + no_file_fmt_e: true
> + ubuntu_specific:
> + apt_src_cont: true
> + apt_hist_fmt: true
> + byobu: true
> + daylight_time: true
> + engb_locale: true
> + landscape: true
> + locale_gen: true
> + lsb_release: true
> + lxd: true
> + ppa: true
> + ppa_file_name: true
> + snap: true
> + sshd: true
> + ssh_key_fmt: true
> + sudo: true
> + syslog: true
> + ubuntu_ntp: true
> + ubuntu_repos: true
> + ubuntu_user: true
> + debian_base:
> + apt: true
> + apt_up_out: true
> + no_ntpdate: true
> + rhel_base:
> + rpm: true
>
> releases:
> - trusty:
> - enabled: true
> - platform_ident:
> - lxd:
> - # if sstreams_server is omitted, default is used, defined in
> - # tests.cloud_tests.platforms.lxd.DEFAULT_SSTREAMS_SERVER as:
> - # sstreams_server: https://us.images.linuxcontainers.org:8443
> - #alias: ubuntu/trusty/default
> - alias: t
> - sstreams_server: https://cloud-images.ubuntu.com/daily
> - xenial:
> - enabled: true
> - platform_ident:
> - lxd:
> - #alias: ubuntu/xenial/default
> - alias: x
> - sstreams_server: https://cloud-images.ubuntu.com/daily
> - yakkety:
> - enabled: true
> - platform_ident:
> - lxd:
> - #alias: ubuntu/yakkety/default
> - alias: y
> - sstreams_server: https://cloud-images.ubuntu.com/daily
> - zesty:
> - enabled: true
> - platform_ident:
> - lxd:
> - #alias: ubuntu/zesty/default
> - alias: z
> - sstreams_server: https://cloud-images.ubuntu.com/daily
> + # UBUNTU
> =================================================================
> artful:
> - enabled: true
> - platform_ident:
> - lxd:
> - #alias: ubuntu/artful/default
> - alias: a
> - sstreams_server: https://cloud-images.ubuntu.com/daily
> - jessie:
> - platform_ident:
> - lxd:
> - alias: debian/jessie/default
> - sid:
> - platform_ident:
> - lxd:
> - alias: debian/sid/default
> + # EOL: Jul 2018
> + default:
> + enabled: true
> + feature_groups:
> + - base
> + - debian_base
> + - ubuntu_specific
> + lxd:
> + sstreams_server: https://cloud-images.ubuntu.com/daily
> + alias: artful
> + setup_overrides: null
> + override_templates: false
> + zesty:
> + # EOL: Jan 2018
> + default:
> + enabled: true
> + feature_groups:
> + - base
> + - debian_base
> + - ubuntu_specific
> + lxd:
> + sstreams_server: https://cloud-images.ubuntu.com/daily
> + alias: zesty
> + setup_overrides: null
> + override_templates: false
> + yakkety:
> + # EOL: Jul 2017
> + default:
> + enabled: true
> + feature_groups:
> + - base
> + - debian_base
> + - ubuntu_specific
> + lxd:
> + sstreams_server: https://cloud-images.ubuntu.com/daily
> + alias: yakkety
> + setup_overrides: null
> + override_templates: false
> + xenial:
> + # EOL: Apr 2021
> + default:
> + enabled: true
> + feature_groups:
> + - base
> + - debian_base
> + - ubuntu_specific
> + lxd:
> + sstreams_server: https://cloud-images.ubuntu.com/daily
> + alias: xenial
> + setup_overrides: null
> + override_templates: false
> + trusty:
> + # EOL: Apr 2019
> + default:
> + enabled: true
> + feature_groups:
> + - base
> + - debian_base
> + - ubuntu_specific
> + features:
> + apt_up_out: false
> + locale_gen: false
> + lxd: false
> + ppa_file_name: false
> + snap: false
> + ssh_key_fmt: false
> + no_ntpdate: false
> + no_file_fmt_e: false
> + system_ready_script: |
> + #!/bin/bash
> + # upstart based, so use old style runlevels
> + [ $(runlevel | awk '{print $2}') = '2' ]
> + lxd:
> + sstreams_server: https://cloud-images.ubuntu.com/daily
> + alias: trusty
> + setup_overrides: null
> + override_templates: false
> + # DEBIAN
> =================================================================
> stretch:
> - platform_ident:
> - lxd:
> - alias: debian/stretch/default
> - wheezy:
> - platform_ident:
> - lxd:
> - alias: debian/wheezy/default
> + # EOL: Not yet released
> + default:
> + enabled: true
> + feature_groups:
> + - base
> + - debian_base
> + lxd:
> + alias: debian/stretch/default
> + jessie:
> + # EOL: Jun 2020
> + # NOTE: the cloud-init version shipped with jessie is out of date
> + # tests work if an up to date deb is used
> + default:
> + enabled: true
> + feature_groups:
> + - base
> + - debian_base
> + lxd:
> + alias: debian/jessie/default
> + # CENTOS
> =================================================================
> centos70:
> - timeout: 180
> - platform_ident:
> - lxd:
> - alias: centos/7/default
> + # EOL: Jun 2024 (2020 - end of full updates)
> + default:
> + enabled: true
> + feature_groups:
> + - base
> + - rhel_base
> + user_data_overrides:
> + preserve_hostname: true
> + lxd:
> + features:
> + # NOTE: (LP: #1575779)
> + hostname: false
> + alias: centos/7/default
> centos66:
> - timeout: 180
> - platform_ident:
> - lxd:
> - alias: centos/6/default
> + # EOL: Nov 2020
> + default:
> + enabled: true
> + feature_groups:
> + - base
> + - rhel_base
> + # still supported, but only bugfixes after may 2017
> + system_ready_script: |
> + #!/bin/bash
> + [ $(runlevel | awk '{print $2}') = '3' ]
> + user_data_overrides:
> + preserve_hostname: true
> + lxd:
> + features:
> + # NOTE: (LP: #1575779)
> + hostname: false
> + alias: centos/6/default
>
> # vi: ts=4 expandtab
--
https://code.launchpad.net/~powersj/cloud-init/+git/cloud-init/+merge/324136
Your team cloud-init commiters is requested to review the proposed merge of
~powersj/cloud-init:integration-test-revamp into cloud-init:master.
_______________________________________________
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
