zuul/nodepool
Code Issues Proposed changes

Add parent and abstract flags for diskimages

Browse Source 
While YAML does have inbuilt support for anchors to greatly reduce
duplicated sections, anchors have no support for merging values.  For
diskimages, this can result in a lot of duplicated values for each
image which you can not otherwise avoid.

This provides two new values for diskimages; a "parent" and
"abstract".

Specifying a parent means you inherit all the configuration values
from that image.  Anything specified within the child image overwrites
the parent values as you would expect; caveats, as described in the
documentation, are that the elements field appends and the env-vars
field has update() semantics.

An "abstract" diskimage is not instantiated into a real image, it is
only used for configuration inheritance.  This way you can make a
abstrat "base" image with common values and inherit that everywhere
without having to worry about bringing in values you don't want.

You can also chain parents together and the inheritance flows through.

Documentation is updated, and several tests are added to ensure the
correct parenting, merging and override behaviour of the new values.

Change-Id: I170016ef7d8443b9830912b9b0667370e6afcde7
This commit is contained in:
Ian Wienand 2020-03-16 13:13:54 +11:00
parent 340df68a7b
commit b5b20b6e2c
9 changed files with 619 additions and 51 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

92
doc/source/configuration.rst
View File

@ -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

10
nodepool/cmd/config_validator.py
View File

@ -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:

94
nodepool/config.py
View File

@ -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):

74
nodepool/tests/fake-image-create
View File

@ -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)

209
nodepool/tests/fixtures/config_validate/missing_parent_diskimage.yaml vendored Normal file
View File

@ -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

85
nodepool/tests/fixtures/node_diskimage_parents.yaml vendored Normal file
View File

@ -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

90
nodepool/tests/unit/test_builder.py
View File

@ -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')

9
nodepool/tests/unit/test_config_validator.py
View File

@ -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',

7
releasenotes/notes/diskimages-parent-182d518232b5649b.yaml Normal file
View File

@ -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.