diff --git a/doc/source/configuration.rst b/doc/source/configuration.rst index 451f4bcd8..b0c17e96e 100644 --- a/doc/source/configuration.rst +++ b/doc/source/configuration.rst @@ -191,17 +191,17 @@ Options To remove a diskimage from the system entirely, remove all associated entries in :attr:`providers.[openstack].diskimages` and - remove its entry from `diskimages`. All uploads will be deleted as - well as the files on disk. + remove its entry from ``diskimages``. All uploads will be deleted + as well as the files on disk. + + A sample configuration section is illustrated below. .. code-block:: yaml diskimages: - - name: ubuntu-precise - pause: False - rebuild-age: 86400 + - name: base + abstract: True elements: - - ubuntu-minimal - vm - simple-init - openstack-repos @@ -210,40 +210,44 @@ Options - cache-bindep - growroot - infra-package-needs - release: precise - username: zuul env-vars: TMPDIR: /opt/dib_tmp DIB_CHECKSUM: '1' DIB_IMAGE_CACHE: /opt/dib_cache + + - name: ubuntu-bionic + parent: base + pause: False + rebuild-age: 86400 + elements: + - ubuntu-minimal + release: bionic + username: zuul + env-vars: DIB_APT_LOCAL_CACHE: '0' DIB_DISABLE_APT_CLEANUP: '1' FS_TYPE: ext3 - - name: ubuntu-xenial + + - name: ubuntu-focal + base: ubuntu-bionic + release: focal + env-vars: + DIB_DISABLE_APT_CLEANUP: '0' + + - name: centos-8 + parent: base pause: True rebuild-age: 86400 formats: - raw - tar elements: - - ubuntu-minimal - - vm - - simple-init - - openstack-repos - - nodepool-base - - cache-devstack - - cache-bindep - - growroot - - infra-package-needs - release: precise - username: ubuntu + - centos-minimal + - epel + release: '8' + username: centos env-vars: - TMPDIR: /opt/dib_tmp - DIB_CHECKSUM: '1' - DIB_IMAGE_CACHE: /opt/dib_cache - DIB_APT_LOCAL_CACHE: '0' - DIB_DISABLE_APT_CLEANUP: '1' - FS_TYPE: ext3 + FS_TYPE: xfs Each entry is a dictionary with the following keys @@ -254,6 +258,42 @@ Options Identifier to reference the disk image in :attr:`providers.[openstack].diskimages` and :attr:`labels`. + .. attr:: abstract + :type: bool + :default: False + + An ``abstract`` entry is used to group common configuration + together, but will not create any actual image. A ``diskimage`` + marked as ``abstract`` should be inherited from in another + ``diskimage`` via its :attr:`diskimages.parent` attribute. + + An `abstract` entry can have a :attr:`diskimages.parent` + attribute as well; values will merge down. + + .. attr:: parent + :type: str + + A parent ``diskimage`` entry to inherit from. Any values from the + parent will be populated into this image. Setting any fields in + the current image will override the parent values execept for + the following: + + * :attr:`diskimages.env-vars`: new keys are additive, any + existing keys from the parent will be overwritten by values in + the current diskimage (i.e. Python `update()` semantics for a + dictionary). + * :attr:`diskimages.elements`: values are additive; the list of + elements from the parent will be extended with any values in + the current diskimage. Note that the element list passed to + `diskimage-builder` is not ordered; elements specify their own + dependencies and `diskimage-builder` builds a graph from that, + not the command-line order. + + Note that a parent ``diskimage`` may also have it's own parent, + creating a chain of inheritance. See also + :attr:`diskimages.abstract` for defining common configuration + that does not create a diskimage. + .. attr:: formats :type: list diff --git a/nodepool/cmd/config_validator.py b/nodepool/cmd/config_validator.py index f6cefb42c..ae2c2ad86 100644 --- a/nodepool/cmd/config_validator.py +++ b/nodepool/cmd/config_validator.py @@ -42,10 +42,12 @@ class ConfigValidator: diskimage = { v.Required('name'): str, + 'abstract': bool, 'dib-cmd': str, 'pause': bool, 'elements': [str], 'formats': [str], + 'parent': str, 'release': v.Any(str, int), 'rebuild-age': int, 'env-vars': {str: str}, @@ -121,6 +123,14 @@ class ConfigValidator: errors = True diskimages[name] = diskimage + # Now check every parent is defined + for diskimage in config.get('diskimages', []): + if diskimage.get('parent', None): + if diskimage['parent'] not in diskimages: + log.error("diskimage %s has non-existant parent: %s" % + (diskimage['name'], diskimage['parent'])) + errors = True + if errors is True: return 1 else: diff --git a/nodepool/config.py b/nodepool/config.py index 9fe35dd48..80c2b69ec 100644 --- a/nodepool/config.py +++ b/nodepool/config.py @@ -98,28 +98,69 @@ class Config(ConfigValue): if not diskimages_cfg: return + all_diskimages = {} + non_abstract_diskimages = [] + + # create a dict and split out the abstract images which don't + # become final images, but can still be referenced as parent: for diskimage in diskimages_cfg: name = diskimage['name'] - d = DiskImage(name) + all_diskimages[name] = diskimage + if not diskimage.get('abstract', False): + non_abstract_diskimages.append(diskimage) + + def _set_from_config(diskimage, config): + build_timeout = config.get('build-timeout', None) + if build_timeout: + diskimage.build_timeout = build_timeout + dib_cmd = config.get('dib-cmd', None) + if dib_cmd: + diskimage.dib_cmd = dib_cmd + elements = config.get('elements', []) + diskimage.elements.extend(elements) + env_vars = config.get('env-vars', {}) + diskimage.env_vars.update(env_vars) + image_types = config.get('formats', None) + if image_types: + diskimage.image_types = set(image_types) + pause = config.get('pause', None) + if pause: + diskimage.pause = pause + python_path = config.get('python-path', None) + if python_path: + diskimage.python_path = python_path + rebuild_age = config.get('rebuild-age', None) + if rebuild_age: + diskimage.rebuild_age = rebuild_age + release = config.get('release', None) + if release: + diskimage.release = release + + def _merge_image_cfg(diskimage, parent): + parent_cfg = all_diskimages[parent] + if parent_cfg.get('parent', None): + _merge_image_cfg(diskimage, parent_cfg['parent']) + _set_from_config(diskimage, parent_cfg) + + for cfg in non_abstract_diskimages: + d = DiskImage(cfg['name']) + + # Walk the parents, if any, and set their values + if cfg.get('parent', None): + _merge_image_cfg(d, cfg.get('parent')) + + # Now set our config, which overrides any values from + # parents. + _set_from_config(d, cfg) - if 'elements' in diskimage: - d.elements = u' '.join(diskimage['elements']) - else: - d.elements = '' - d.dib_cmd = str(diskimage.get('dib-cmd', 'disk-image-create')) # must be a string, as it's passed as env-var to # d-i-b, but might be untyped in the yaml and # interpreted as a number (e.g. "21" for fedora) - d.release = str(diskimage.get('release', '')) - d.rebuild_age = int(diskimage.get('rebuild-age', 86400)) - d.env_vars = diskimage.get('env-vars', {}) - if not isinstance(d.env_vars, dict): - d.env_vars = {} - d.image_types = set(diskimage.get('formats', [])) - d.pause = bool(diskimage.get('pause', False)) - d.username = diskimage.get('username', 'zuul') - d.python_path = diskimage.get('python-path', 'auto') - d.build_timeout = diskimage.get('build-timeout', (8 * 60 * 60)) + d.release = str(d.release) + + # This is expected as a space-separated string + d.elements = u' '.join(d.elements) + self.diskimages[d.name] = d def setSecureDiskimageEnv(self, diskimages, secure_config_path): @@ -172,18 +213,21 @@ class Label(ConfigValue): class DiskImage(ConfigValue): + BUILD_TIMEOUT = (8 * 60 * 60) # 8 hours + REBUILD_AGE = (24 * 60 * 60) # 24 hours + def __init__(self, name): self.name = name - self.build_timeout = None - self.dib_cmd = None - self.elements = None - self.env_vars = None - self.image_types = None + self.build_timeout = self.BUILD_TIMEOUT + self.dib_cmd = 'disk-image-create' + self.elements = [] + self.env_vars = {} + self.image_types = set([]) self.pause = False - self.python_path = None - self.rebuild_age = None - self.release = None - self.username = None + self.python_path = 'auto' + self.rebuild_age = self.REBUILD_AGE + self.release = '' + self.username = 'zuul' def __eq__(self, other): if isinstance(other, DiskImage): diff --git a/nodepool/tests/fake-image-create b/nodepool/tests/fake-image-create index 5f8918249..f215bad72 100755 --- a/nodepool/tests/fake-image-create +++ b/nodepool/tests/fake-image-create @@ -58,11 +58,85 @@ while true ; do esac done +ELEMENTS="$@" + if [ -z "$outfile" ]; then echo "No output file specified." exit 1 fi +# +# WTF is this testing? That we merge abstract diskimages correctly +# into children and append/override variables correctly. The two +# images in the test config file +# (/nodepool/tests/fixtures/node_diskimage_parents.yaml) set +# PARENT_TEST_FLAG which we gate on here so we only run this for that +# test. The diskimage layout when hitting this path is roughly like +# this: +# +# %%%%% abstract-base %%%%% +# elements: fedora +# env: TMPDIR, DIB_IMAGE_... (and so on) +# PARENT_TEST_ENV_OVERRIDE=abstract-base +# | | +# |-------+ +-------| +# | | +# | | +# | %%%%% abstract-intermediate %%%%% +# | elements: intermediate +# | env: PARENT_TEST_ENV_INTERMEDIATE=abstract-intermediate +# | | +# | | +# %%%%% parent-image-1 %%%%% %%%%% parent-image-2 %%%%%% +# elements: vm elements: vm +# env: PARENT_TEST_FLAG=base env: PARENT_TEST_FLAG=intermediate +# PARENT_TEST_ENV_OVERRIDE=parent-image-2 +# +# So for parent-image-1 we want to see elements "fedora vm" and for +# parent-image-2 we want to see "fedora intermediate vm"; and similar +# updates/overrides for the env-vars. +# +if [[ -n "${PARENT_TEST_FLAG}" ]]; then + + if [[ "${PARENT_TEST_FLAG}" == "base" ]]; then + # Note we've already tested the base-defined env-vars like + # TMPDIR, DIB_*, etc. are merged because they're tested above. + if [[ "${ELEMENTS}" != "fedora vm" ]]; then + echo "Elements list did not merge correctly" + exit 1 + else + echo " -> Base elements list correctly merged" + fi + + elif [[ "${PARENT_TEST_FLAG}" == "intermediate" ]]; then + + if [[ "${ELEMENTS}" != "fedora intermediate vm" ]]; then + echo "Elements list did not merge correctly" + exit 1 + else + echo " -> Intermediate elements list correctly merged" + fi + + if [[ "${PARENT_TEST_ENV_INTERMEDIATE}" != "abstract-intermediate" ]]; then + echo "Did not see intermediate env value in final job" + exit 1 + else + echo " -> Intermediate env value correctly merged" + fi + + if [[ "${PARENT_TEST_ENV_OVERRIDE}" != "parent-image-2" ]]; then + echo "Final job did not override PARENT_TEST_ENV_OVERRIDE correctly" + exit 1 + else + echo " -> Env override correct" + fi + else + echo "Invalid PARENT_TEST_IMAGE?" + exit 1 + fi + +fi + # Tests can pause this script by creating this file before it runs. # No dib files should be created yet at the pause point. tmpdir=$(dirname $outfile) diff --git a/nodepool/tests/fixtures/config_validate/missing_parent_diskimage.yaml b/nodepool/tests/fixtures/config_validate/missing_parent_diskimage.yaml new file mode 100644 index 000000000..1238c7f64 --- /dev/null +++ b/nodepool/tests/fixtures/config_validate/missing_parent_diskimage.yaml @@ -0,0 +1,209 @@ +elements-dir: /etc/nodepool/elements +images-dir: /opt/nodepool_dib + +webapp: + port: 8005 + listen_address: '0.0.0.0' + +zookeeper-servers: + - host: zk1.openstack.org + port: 2181 + chroot: /test + +labels: + - name: trusty + max-ready-age: 3600 + min-ready: 1 + - name: trusty-2-node + min-ready: 0 + - name: trusty-external + min-ready: 1 + - name: trusty-static + - name: kubernetes-namespace + - name: pod-fedora + - name: openshift-project + - name: openshift-pod + - name: centos-ami + +providers: + - name: cloud1 + driver: openstack + cloud: vanilla-cloud + region-name: 'vanilla' + boot-timeout: 120 + max-concurrency: 10 + launch-retries: 3 + port-cleanup-interval: 600 + rate: 1 + diskimages: + - name: trusty + pools: + - name: main + max-servers: 184 + auto-floating-ip: True + host-key-checking: True + node-attributes: + key1: value1 + key2: value2 + networks: + - public + - private + labels: + - name: trusty + diskimage: trusty + min-ram: 8192 + console-log: True + networks: + - public + - name: trusty-2-node + diskimage: trusty + min-ram: 8192 + boot-from-volume: True + volume-size: 100 + instance-properties: + a_key: a_value + b_key: b_value + userdata: | + #cloud-config + password: password + chpasswd: { expire: False } + ssh_pwauth: True + hostname: test + + - name: cloud2 + driver: openstack + cloud: chocolate-cloud + region-name: 'chocolate' + boot-timeout: 120 + rate: 0.001 + port-cleanup-interval: 0 + post-upload-hook: /usr/bin/upload-hook + diskimages: + - name: trusty + pause: False + connection-type: ssh + connection-port: 22 + cloud-images: + - name: trusty-unmanaged + config-drive: true + - name: windows-unmanaged + username: winzuul + python-path: A:/python3.7.exe + connection-type: winrm + connection-port: 5986 + pools: + - name: main + max-servers: 184 + auto-floating-ip: False + host-key-checking: False + security-groups: + - zuul_sg + labels: + - name: trusty + diskimage: trusty + min-ram: 8192 + networks: + - public + - private + - name: trusty-2-node + diskimage: trusty + min-ram: 8192 + - name: trusty-external + cloud-image: trusty-unmanaged + min-ram: 8192 + + - name: static-rack + driver: static + pools: + - name: main + nodes: + - name: trusty.example.com + labels: trusty-static + host-key: fake-key + timeout: 13 + connection-port: 22022 + username: zuul + max-parallel-jobs: 1 + + - name: kubespray + driver: kubernetes + context: admin-cluster.local + pools: + - name: main + labels: + - name: kubernetes-namespace + type: namespace + - name: pod-fedora + type: pod + image: docker.io/fedora:28 + + - name: openshift + driver: openshift + context: "/hostname:8443/self-provisioner-service-account" + pools: + - name: main + labels: + - name: openshift-project + type: project + - name: openshift-pod + type: pod + image: docker.io/fedora:28 + python-path: /usr/bin/python3 + memory: 512 + cpu: 2 + + - name: ec2-us-east-2 + driver: aws + region-name: us-east-2 + profile-name: default + cloud-images: + - name: centos-ami + image-id: ami-cfdafaaa + username: centos + pools: + - name: main + max-servers: 42 + security-group-id: sg-8bfe86352e334a80a + subnet-id: subnet-bb3605b5f0fa40e1b + labels: + - name: centos-ami + cloud-image: centos-ami + instance-type: t2.micro + key-name: zuul + volume-type: gp2 + volume-size: 80 + + - name: openshift-single-project + driver: openshiftpods + context: "/hostname:8443/developer" + pools: + - name: project-name + labels: + - name: openshift-pod + image: docker.io/fedora:28 + +diskimages: + - name: a_parent + - name: a_child + parent: a_parent + + - name: trusty + parent: this_does_not_exist + formats: + - tar + pause: False + elements: + - ubuntu + - vm + - openstack-repos + - puppet + - nodepool-base + - cache-devstack + release: trusty + rebuild-age: 3600 + build-timeout: 3600 + python-path: /bin/python3.6 + env-vars: + TMPDIR: /opt/dib_tmp + DIB_IMAGE_CACHE: /opt/dib_cache + QEMU_IMG_OPTIONS: compat=0.10 diff --git a/nodepool/tests/fixtures/node_diskimage_parents.yaml b/nodepool/tests/fixtures/node_diskimage_parents.yaml new file mode 100644 index 000000000..c9471313f --- /dev/null +++ b/nodepool/tests/fixtures/node_diskimage_parents.yaml @@ -0,0 +1,85 @@ +elements-dir: . +images-dir: '{images_dir}' +build-log-dir: '{build_log_dir}' + +zookeeper-servers: + - host: {zookeeper_host} + port: {zookeeper_port} + chroot: {zookeeper_chroot} + +labels: + - name: fake-image-parent-1 + min-ready: 1 + - name: fake-image-parent-2 + min-ready: 1 + +providers: + - name: fake-provider-1 + cloud: fake + driver: fake + region-name: fake-region + rate: 0.0001 + diskimages: + - name: parent-image-1 + pools: + - name: main + max-servers: 96 + labels: + - name: fake-image-parent-1 + diskimage: parent-image-1 + min-ram: 8192 + - name: fake-provider-2 + cloud: fake + driver: fake + region-name: fake-region + rate: 0.0001 + diskimages: + - name: parent-image-2 + pools: + - name: main + max-servers: 96 + labels: + - name: fake-image-parent-2 + diskimage: parent-image-2 + min-ram: 8192 + +diskimages: + - name: abstract-base + abstract: True + elements: + - fedora + env-vars: + TMPDIR: /opt/dib_tmp + DIB_IMAGE_CACHE: /opt/dib_cache + DIB_CLOUD_IMAGES: http://download.fedoraproject.org/pub/fedora/linux/releases/test/21-Beta/Cloud/Images/x86_64/ + BASE_IMAGE_FILE: Fedora-Cloud-Base-20141029-21_Beta.x86_64.qcow2 + PARENT_TEST_ENV_OVERRIDE: abstract-base + + - name: abstract-intermediate + abstract: True + parent: abstract-base + elements: + - intermediate + env-vars: + PARENT_TEST_ENV_INTERMEDIATE: abstract-intermediate + + - name: parent-image-1 + parent: abstract-base + elements: + # - fedora : should merge from parent! + - vm + release: 21 + dib-cmd: nodepool/tests/fake-image-create + env-vars: + PARENT_TEST_FLAG: 'base' + + - name: parent-image-2 + parent: abstract-intermediate + elements: + # - fedora : should merge from parent + - vm + release: 21 + dib-cmd: nodepool/tests/fake-image-create + env-vars: + PARENT_TEST_FLAG: 'intermediate' + PARENT_TEST_ENV_OVERRIDE: parent-image-2 diff --git a/nodepool/tests/unit/test_builder.py b/nodepool/tests/unit/test_builder.py index 88bc545ab..8b082efb7 100644 --- a/nodepool/tests/unit/test_builder.py +++ b/nodepool/tests/unit/test_builder.py @@ -24,6 +24,7 @@ from pathlib import Path from nodepool import builder, exceptions, tests from nodepool.driver.fake import provider as fakeprovider from nodepool import zk +from nodepool.config import Config from nodepool.nodeutils import iterate_timeout @@ -98,6 +99,89 @@ class TestNodepoolBuilderDibImage(tests.BaseTestCase): self.assertRaises(exceptions.BuilderError, image.to_path, '/imagedir/') +class TestNodepoolBuilderImageInheritance(tests.BaseTestCase): + def test_parent_job(self): + config = Config() + diskimages = [ + { + 'name': 'parent', + 'dib-cmd': 'parent-dib-cmd', + 'elements': ['a', 'b'], + 'env-vars': { + 'A': 'foo', + 'B': 'bar', + }, + 'release': 21, + }, + { + 'name': 'child', + 'parent': 'parent', + 'dib-cmd': 'override-dib-cmd', + 'elements': ['c'], + 'env-vars': { + 'A': 'override_foo', + 'C': 'moo' + }, + }, + + ] + config.setDiskImages(diskimages) + parsed = config.diskimages['child'] + self.assertEqual(parsed.dib_cmd, 'override-dib-cmd') + self.assertEqual(parsed.release, '21') + self.assertEqual(parsed.elements, 'a b c') + self.assertDictEqual({ + 'A': 'override_foo', + 'B': 'bar', + 'C': 'moo', + }, parsed.env_vars) + + def test_abstract_jobs(self): + config = Config() + diskimages = [ + { + 'name': 'abstract', + 'abstract': True, + 'elements': ['a', 'b'], + 'env-vars': { + 'A': 'foo', + 'B': 'bar', + }, + }, + { + 'name': 'another-abstract', + 'abstract': True, + 'parent': 'abstract', + 'elements': ['c'], + 'env-vars': { + 'A': 'override_abstract', + 'C': 'moo' + }, + }, + { + 'name': 'job', + 'parent': 'another-abstract', + 'elements': ['d'], + 'dib-cmd': 'override-dib-cmd', + 'env-vars': { + 'A': 'override_foo_again', + 'D': 'zoo' + }, + }, + + ] + config.setDiskImages(diskimages) + parsed = config.diskimages['job'] + self.assertEqual(parsed.dib_cmd, 'override-dib-cmd') + self.assertEqual(parsed.elements, 'a b c d') + self.assertDictEqual({ + 'A': 'override_foo_again', + 'B': 'bar', + 'C': 'moo', + 'D': 'zoo', + }, parsed.env_vars) + + class TestNodePoolBuilder(tests.DBTestCase): def test_start_stop(self): @@ -399,6 +483,12 @@ class TestNodePoolBuilder(tests.DBTestCase): self.assertReportedStat('nodepool.dib_image_build.' 'fake-image-vhd.vhd.size', '4096', 'g') + def test_diskimage_build_parents(self): + configfile = self.setup_config('node_diskimage_parents.yaml') + self.useBuilder(configfile) + self.waitForBuild('parent-image-1', '0000000001') + self.waitForBuild('parent-image-2', '0000000001') + @mock.patch('select.poll') def test_diskimage_build_timeout(self, mock_poll): configfile = self.setup_config('diskimage_build_timeout.yaml') diff --git a/nodepool/tests/unit/test_config_validator.py b/nodepool/tests/unit/test_config_validator.py index 8eeb54cbc..43e19c430 100644 --- a/nodepool/tests/unit/test_config_validator.py +++ b/nodepool/tests/unit/test_config_validator.py @@ -66,6 +66,15 @@ class TestConfigValidation(tests.BaseTestCase): ret = validator.validate() self.assertEqual(ret, 1) + def test_missing_parent_diskimage(self): + config = os.path.join(os.path.dirname(tests.__file__), + 'fixtures', 'config_validate', + 'missing_parent_diskimage.yaml') + + validator = ConfigValidator(config) + ret = validator.validate() + self.assertEqual(ret, 1) + def test_schema(self): config = os.path.join(os.path.dirname(tests.__file__), 'fixtures', 'config_validate', diff --git a/releasenotes/notes/diskimages-parent-182d518232b5649b.yaml b/releasenotes/notes/diskimages-parent-182d518232b5649b.yaml new file mode 100644 index 000000000..485f2cb86 --- /dev/null +++ b/releasenotes/notes/diskimages-parent-182d518232b5649b.yaml @@ -0,0 +1,7 @@ +--- +features: + - | + Entries in the `diskimages` section can now specify a parent image + to inherit configuration values from. You can also specify images + for configuration use only as `abstract` to consolidate + common values.