From c8c89f37bf2c33d81dba2490dd3474c5a49c14e5 Mon Sep 17 00:00:00 2001 From: Trond Hasle Amundsen Date: Thu, 10 Sep 2026 10:02:20 +0200 Subject: [PATCH 01/22] docs: add CLAUDE.md with build commands and architecture notes Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A4Y4JQ39uQMSZcBYoTE84y --- CLAUDE.md | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 109 insertions(+) create mode 100644 CLAUDE.md diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..aeb467f --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,109 @@ +# CLAUDE.md + +This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. + +## What this is + +A thin Python CLI wrapper around [Packer](https://packer.io) for building OpenStack (Glance) images in +[NREC](https://www.nrec.no). It exists so users don't have to write Packer templates: it creates the +throwaway OpenStack resources Packer needs, invokes `packer build` against a bundled template, and +cleans up afterwards. + +This fork's `origin` is `trondham/imagebuilder`; `upstream` is `norcams/imagebuilder`. + +## Commands + +```bash +# Lint — this is what CI runs (.github/workflows/python-package.yml), matrix py3.9/3.10/3.11 +ruff check --select=E9,F63,F7,F82 --target-version=py39 . +ruff check --target-version=py39 . + +# Dev install (see DEVELOPMENT.md) +python3 -m venv . && source bin/activate +pip install -r requirements.txt +python setup.py develop + +# Run without installing +./imagebuilder # wrapper around image_builder.imagebuilder:main +``` + +There is no test suite. CI installs `pytest` but never invokes it; do not claim tests pass. + +Running any command requires OpenStack credentials in the environment (`source keystone_rc.sh`) — +`get_openstack_rc()` reads `OS_USERNAME`, `OS_PROJECT_NAME`, `OS_PASSWORD`, `OS_AUTH_URL`, +`OS_IDENTITY_API_VERSION`, `OS_USER_DOMAIN_NAME`, `OS_PROJECT_DOMAIN_NAME`, `OS_REGION_NAME`, +`OS_NO_CACHE` and exits if any are missing (`OS_CACERT` is optional). `packer`, `ssh-keygen`, +`openssl`, and the `glance` CLI must be on `$PATH` — on RHEL/CentOS beware the unrelated +`/usr/sbin/packer`; `/usr/bin/packer` is the right one. + +## Architecture + +`image_builder/imagebuilder.py:main()` is a straight-line procedural driver — all orchestration lives +there, and the other modules are stateless-ish helper classes. Two subcommands, dispatched by +`parsecommands.Commands` (which uses the argparse "git-style subcommand" pattern: `sys.argv[1:2]` +selects the method, `sys.argv[2:]` is parsed by that method's own parser, and `build_args` / +`bootstrap_args` are `False` when unused). + +**`build`** (`build.py:BuildFunctions`) — the core flow, in order: +1. `helpers.make_tmp_dir()` — a `tempfile.mkdtemp(prefix='imagebuilder-')` holding the SSH keypair and + Packer's manifest; everything in the flow is scoped to it. +2. `create_security_group()` / `create_keypairs()` — temporary OpenStack resources named + `imagebuilder-`, allowing SSH ingress on v4 and v6. The keypair is generated by shelling + out to `ssh-keygen`. +3. `find_network_id()` — resolves the network name (default `Dualstack`) to an id via neutron. +4. `run_packer()` — builds a `packer build --var k=v ...` argv against `/template` and + streams stdout through `helpers.log_subprocess_output` into `logging`. +5. `parse_manifest()` — Packer's `manifest` post-processor writes `packer-manifest.json` into the temp + dir; `builds[0].artifact_id` is the resulting Glance image id. This is the only channel by which the + build result comes back from Packer. +6. Optional `download_image()` (shells out to `glance image-download`, then **deletes the image from + Glance**), optional `--purge-source`, then `cleanup()` + `clean_tmp_files()`. + +Three OpenStack clients are used side by side, each region-scoped from the same keystone session: +nova (keypairs), neutron (security groups, networks), glance (image delete). Cleanup is not +signal-safe — if the process dies mid-build the `imagebuilder-*` security group and keypair are +orphaned and must be deleted by hand. + +**`bootstrap`** (`bootstrap.py:BootstrapFunctions`) — downloads a cloud image from a URL, optionally +verifies it against a checksum file (substring match of the computed hexdigest in the fetched file), and +uploads it to Glance with hardware properties set from the flags (`hw_disk_bus=scsi` + +`hw_scsi_model=virtio-scsi` unless `-s`, `hw_firmware_type=uefi` + `hw_machine_type=q35` with `-e`, +always `hw_rng_model=virtio`). On success it writes the bare image id to **stdout** and nothing else — +all other output goes to `logging` — so `bootstrap ... | xargs -I % imagebuilder build -s % ...` works. + +**Config** (`config.py`) — `configparser` over `./config`, `~/.imagebuilder/config`, +`/etc/imagebuilder/config` (later files win). Only `[main] template_dir` and `download_dir` are read, +and `IB_TEMPLATE_DIR` / `IB_DOWNLOAD_DIR` override them. + +### Packer template and provision scripts + +`template` (legacy JSON Packer format, not HCL2) is a *file*, not a directory — `run_packer` joins +`template_dir` with the literal name `template`. Its `provisioners.scripts` list is a fixed pipeline of +`scripts/*.sh` with ``{{user `provision_script`}}`` (the user's `-p` script, default `/bin/true`) +slotted in near the end, before the cleanup scripts. Adding a stage to every build means editing this +list; the paths in it are relative to the working directory Packer runs in, which is why +`example/template` uses `../scripts/`. + +Scripts under `scripts/` run on the builder VM as the cloud-init user via `sudo`, across a wide range of +guests (RHEL 8/9/10, Alma, CentOS Stream, Fedora, Debian, Ubuntu). They share a copy-pasted +Omnitruck-style platform-detection preamble that sets `platform` (`el`/`fedora`/`debian`/`ubuntu`) and +`major_version`; keep that convention when adding one. A script that doesn't apply to the detected +platform prints a message and `exit 0` — it must not fail the build. Note `grub.sh`, `nvidia-grid-driver.sh`, +and `uib-puppet.sh` are not in the default template's list; they are opt-in, used as the `-p` script or +by site-specific templates. + +`nvidia-grid-driver.sh` pulls the vGPU guest driver and per-region licensing tokens from +`download.iaas.uio.no` and selects the BGO/OSL `gridd.conf` by grepping `/run/cloud-init/` for the +availability zone. Recent history is mostly churn here (pinning/unpinning driver versions, refreshing +client tokens) — check `git log -- scripts/nvidia-grid-driver.sh` before changing it. + +`example/` is a self-contained smoke-test setup (its own `config`, a trimmed `template`, a trivial +`provision.sh`, and Terraform to boot the resulting image); use it to sanity-check changes end to end. + +## Conventions + +- Commit messages follow Conventional Commits (`feat:`, `fix:`) for functional changes; script tweaks + are often plain imperative one-liners. +- `pyproject.toml` disables ruff `E501` (line length) and `E722` (bare `except`) — bare excepts are used + deliberately throughout and should not be "fixed" as drive-by cleanup. +- New Python code targets 3.9 (CI's floor). From ff53932dbf49cb849042cd3b161581d231e83386 Mon Sep 17 00:00:00 2001 From: Trond Hasle Amundsen Date: Fri, 11 Sep 2026 09:17:43 +0200 Subject: [PATCH 02/22] feat: migrate Packer template to HCL2 and install plugins with packer init Packer stopped bundling builder plugins in 1.10, so the openstack builder no longer resolves in a stock binary and builds fail outright on any current Packer. Plugins are installed from a required_plugins block via packer init, which only works with HCL2, so the legacy JSON template had to go. - template -> template.pkr.hcl, pinning github.com/hashicorp/openstack ~> 1.1 - run packer init before every build (no-op for a legacy JSON template, which find_template() still accepts so existing template_dir setups keep working) - packer build --var -> -var - script paths use ${path.root} instead of being relative to Packer's cwd - resolve the template and the network before creating the temporary security group and keypair, so those failures no longer orphan OpenStack resources Verified against Packer 1.16.0 with openstack plugin v1.1.4: packer init installs the plugin and packer validate passes on both templates, failing only on the absence of live OpenStack credentials. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A4Y4JQ39uQMSZcBYoTE84y --- CLAUDE.md | 34 ++++++++---- README.md | 14 ++++- example/README.md | 2 + example/template | 28 ---------- example/template.pkr.hcl | 83 ++++++++++++++++++++++++++++ image_builder/build.py | 63 ++++++++++++++++----- image_builder/imagebuilder.py | 27 +++++++-- template | 35 ------------ template.pkr.hcl | 101 ++++++++++++++++++++++++++++++++++ 9 files changed, 292 insertions(+), 95 deletions(-) delete mode 100644 example/template create mode 100644 example/template.pkr.hcl delete mode 100644 template create mode 100644 template.pkr.hcl diff --git a/CLAUDE.md b/CLAUDE.md index aeb467f..78d8c95 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -47,16 +47,21 @@ selects the method, `sys.argv[2:]` is parsed by that method's own parser, and `b **`build`** (`build.py:BuildFunctions`) — the core flow, in order: 1. `helpers.make_tmp_dir()` — a `tempfile.mkdtemp(prefix='imagebuilder-')` holding the SSH keypair and Packer's manifest; everything in the flow is scoped to it. -2. `create_security_group()` / `create_keypairs()` — temporary OpenStack resources named +2. `find_template()` — locates `template.pkr.hcl` in `template_dir`. +3. `find_network_id()` — resolves the network name (default `Dualstack`) to an id via neutron. +4. `run_packer_init()` — `packer init`, which installs the openstack plugin. + + Steps 2–4 deliberately run *before* anything is created in OpenStack: each can fail on bad user + input, and failing here costs nothing to clean up. +5. `create_security_group()` / `create_keypairs()` — temporary OpenStack resources named `imagebuilder-`, allowing SSH ingress on v4 and v6. The keypair is generated by shelling out to `ssh-keygen`. -3. `find_network_id()` — resolves the network name (default `Dualstack`) to an id via neutron. -4. `run_packer()` — builds a `packer build --var k=v ...` argv against `/template` and - streams stdout through `helpers.log_subprocess_output` into `logging`. -5. `parse_manifest()` — Packer's `manifest` post-processor writes `packer-manifest.json` into the temp +6. `run_packer()` — builds a `packer build -var k=v ...` argv and streams stdout through + `helpers.log_subprocess_output` into `logging`. +7. `parse_manifest()` — Packer's `manifest` post-processor writes `packer-manifest.json` into the temp dir; `builds[0].artifact_id` is the resulting Glance image id. This is the only channel by which the build result comes back from Packer. -6. Optional `download_image()` (shells out to `glance image-download`, then **deletes the image from +8. Optional `download_image()` (shells out to `glance image-download`, then **deletes the image from Glance**), optional `--purge-source`, then `cleanup()` + `clean_tmp_files()`. Three OpenStack clients are used side by side, each region-scoped from the same keystone session: @@ -77,12 +82,17 @@ and `IB_TEMPLATE_DIR` / `IB_DOWNLOAD_DIR` override them. ### Packer template and provision scripts -`template` (legacy JSON Packer format, not HCL2) is a *file*, not a directory — `run_packer` joins -`template_dir` with the literal name `template`. Its `provisioners.scripts` list is a fixed pipeline of -`scripts/*.sh` with ``{{user `provision_script`}}`` (the user's `-p` script, default `/bin/true`) -slotted in near the end, before the cleanup scripts. Adding a stage to every build means editing this -list; the paths in it are relative to the working directory Packer runs in, which is why -`example/template` uses `../scripts/`. +`template.pkr.hcl` (HCL2) is a *file*, not a directory — `find_template()` looks for it in +`template_dir`, falling back to a legacy JSON template named `template` if that's all it finds (in +which case `packer init` is skipped and the openstack plugin must be installed by hand). Its +`provisioner "shell"` `scripts` list is a fixed pipeline of `scripts/*.sh` with `var.provision_script` +(the user's `-p` script, default `/bin/true`) slotted in near the end, before the cleanup scripts. +Adding a stage to every build means editing this list. Paths there use `${path.root}` — the directory +holding the template — so builds don't depend on Packer's working directory. + +Packer stopped bundling builder plugins in 1.10, so `packer init` must run before `packer build`; the +`required_plugins` block pins `github.com/hashicorp/openstack`. `required_plugins` is HCL2-only, which +is why the JSON template had to go. Scripts under `scripts/` run on the builder VM as the cloud-init user via `sudo`, across a wide range of guests (RHEL 8/9/10, Alma, CentOS Stream, Fedora, Debian, Ubuntu). They share a copy-pasted diff --git a/README.md b/README.md index 37f248a..05d57a9 100644 --- a/README.md +++ b/README.md @@ -21,9 +21,15 @@ Pull requests are welcome! * [keystoneauth](https://github.com/openstack/keystoneauth) * [python-novaclient](https://github.com/openstack/python-novaclient) * [python-glanceclient](https://github.com/openstack/python-glanceclient) -* [Packer](https://packer.io) +* [Packer](https://packer.io) 1.7 or newer * OpenSSL command line tool +Packer no longer ships with the OpenStack builder built in, so imagebuilder runs +`packer init` before every build to install the +[openstack plugin](https://github.com/hashicorp/packer-plugin-openstack). This +needs network access the first time; afterwards the plugin is cached under +`$HOME/.config/packer/plugins`. + ### Installation - Clone this repository `git clone https://github.com/norcams/imagebuilder` @@ -91,6 +97,12 @@ template_dir = /home/user/.imagebuilder download_dir = /tmp/images ``` +`template_dir` must contain `template.pkr.hcl` along with the `scripts/` +directory it refers to. A legacy JSON template named `template` is still picked +up if no `template.pkr.hcl` is present, but Packer cannot install plugins for it +— run `packer plugins install github.com/hashicorp/openstack` yourself, or +migrate the template with `packer hcl2_upgrade template`. + ### Provision scripts A provision script is (when using imagebuilder) simply a shell script that will be executed on the virtual machine Packer builds an image from. In it's simplest diff --git a/example/README.md b/example/README.md index 2d36a14..377b5d9 100644 --- a/example/README.md +++ b/example/README.md @@ -2,6 +2,8 @@ This is tested on Alma Linux 8.8. Beware of a duplicate packer executable in `/usr/sbin`, the correct one is in `/usr/bin`. Modify the order of $PATH to ensure the correct one is used. +Requires Packer 1.7 or newer. The OpenStack builder is installed automatically by `packer init`, which imagebuilder runs for you. + ### Install ``` bash diff --git a/example/template b/example/template deleted file mode 100644 index cb3e03e..0000000 --- a/example/template +++ /dev/null @@ -1,28 +0,0 @@ -{ - "builders": [{ - "type": "openstack", - "image_name": "{{user `image_name`}}", - "source_image": "{{user `source_image`}}", - "flavor": "{{user `flavor`}}", - "networks": "{{user `network`}}", - "security_groups": "{{user `security_group`}}", - "ssh_username": "{{user `ssh_username`}}", - "ssh_keypair_name": "{{user `ssh_keypair_name`}}", - "ssh_private_key_file": "{{user `ssh_key_path`}}", - "ssh_ip_version": "4", - "ssh_pty": "true", - "availability_zone": "{{user `availability_zone`}}" - }], - "post-processors": [{ - "type": "manifest", - "output": "{{user `manifest_path`}}" - }], - "provisioners": [{ - "scripts": [ - "../scripts/upgrade.sh", - "{{user `provision_script`}}", - "../scripts/cleanup.sh" - ], - "type": "shell" - }] -} diff --git a/example/template.pkr.hcl b/example/template.pkr.hcl new file mode 100644 index 0000000..7812d01 --- /dev/null +++ b/example/template.pkr.hcl @@ -0,0 +1,83 @@ +packer { + required_plugins { + openstack = { + version = "~> 1.1" + source = "github.com/hashicorp/openstack" + } + } +} + +variable "image_name" { + type = string +} + +variable "source_image" { + type = string +} + +variable "flavor" { + type = string +} + +variable "network" { + type = string +} + +variable "security_group" { + type = string +} + +variable "ssh_username" { + type = string +} + +variable "ssh_keypair_name" { + type = string +} + +variable "ssh_key_path" { + type = string +} + +variable "availability_zone" { + type = string +} + +variable "manifest_path" { + type = string +} + +variable "provision_script" { + type = string + default = "/bin/true" +} + +source "openstack" "image" { + image_name = var.image_name + source_image = var.source_image + flavor = var.flavor + networks = [var.network] + security_groups = [var.security_group] + ssh_username = var.ssh_username + ssh_keypair_name = var.ssh_keypair_name + ssh_private_key_file = var.ssh_key_path + ssh_ip_version = "4" + ssh_pty = true + availability_zone = var.availability_zone +} + +build { + sources = ["source.openstack.image"] + + provisioner "shell" { + scripts = [ + "${path.root}/../scripts/upgrade.sh", + var.provision_script, + "${path.root}/../scripts/cleanup.sh", + ] + } + + post-processor "manifest" { + output = var.manifest_path + } +} diff --git a/image_builder/build.py b/image_builder/build.py index fedb642..ac0788a 100644 --- a/image_builder/build.py +++ b/image_builder/build.py @@ -133,6 +133,26 @@ def find_network_id(self, name): logging.info("Cannot find network %s" % name) return network_id + def find_template(self): + """Finds the Packer template in template_dir + + The HCL2 template is preferred. A legacy JSON template (a file simply + named 'template') is still accepted so existing template_dir setups + keep working, but it cannot declare required_plugins. + """ + for name in ('template.pkr.hcl', 'template'): + template_path = os.path.join(self.template_dir, name) + if os.path.isfile(template_path): + if name == 'template': + logging.info("Using legacy JSON template %s" % template_path) + logging.info("Packer no longer bundles the openstack builder. " + "Install it manually with 'packer plugins install " + "github.com/hashicorp/openstack', or migrate to " + "template.pkr.hcl") + return template_path + logging.info("No Packer template found in %s" % self.template_dir) + return None + def parse_manifest(self): """Parses the manifest file generated by packer""" with open(os.path.join(self.tmp_dir, 'packer-manifest.json')) as data_file: @@ -140,7 +160,7 @@ def parse_manifest(self): artifact_id = manifest['builds'][0]['artifact_id'] return artifact_id - def run_packer(self, secgroup_name, key_name, network_id): + def run_packer(self, template_path, secgroup_name, key_name, network_id): """Executes Packer command""" image_name_var = 'image_name=' + self.image_name avail_zone_var = 'availability_zone=' + self.avail_zone @@ -155,21 +175,38 @@ def run_packer(self, secgroup_name, key_name, network_id): manifest_path_var = 'manifest_path=' + os.path.join(self.tmp_dir, 'packer-manifest.json') cmd = ['packer', 'build', '-color=false', - '--var', image_name_var, - '--var', sshuser_var, - '--var', secgroup_var, - '--var', flavor_var, - '--var', network_var, - '--var', source_image_var, - '--var', keyname_var, - '--var', keypath_var, - '--var', avail_zone_var, - '--var', provision_script_var, - '--var', manifest_path_var, - os.path.join(self.template_dir, 'template')] + '-var', image_name_var, + '-var', sshuser_var, + '-var', secgroup_var, + '-var', flavor_var, + '-var', network_var, + '-var', source_image_var, + '-var', keyname_var, + '-var', keypath_var, + '-var', avail_zone_var, + '-var', provision_script_var, + '-var', manifest_path_var, + template_path] logging.debug(cmd) process = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) with process.stdout: helpers.log_subprocess_output(process.stdout) exitcode = process.wait() return exitcode + + def run_packer_init(self, template_path): + """Installs the Packer plugins required by the template + + Packer does not bundle builder plugins anymore, so the openstack + builder has to be installed before the build. Only HCL2 templates can + declare required_plugins, so this is a no-op for a legacy JSON + template. + """ + if not template_path.endswith('.pkr.hcl'): + return 0 + cmd = ['packer', 'init', template_path] + logging.debug(cmd) + process = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) + with process.stdout: + helpers.log_subprocess_output(process.stdout) + return process.wait() diff --git a/image_builder/imagebuilder.py b/image_builder/imagebuilder.py index 12df5f2..7c1ea95 100644 --- a/image_builder/imagebuilder.py +++ b/image_builder/imagebuilder.py @@ -103,19 +103,34 @@ def main(): template_dir, download_dir) - logging.info('Creating Packer security group...') - secgroup_name, secgroup_id = build.create_security_group() - - logging.info('Creating Packer keypair...') - key_name, keypair_id = build.create_keypairs() + # Everything that can fail without creating anything in OpenStack is + # done first, so a bad template or network name doesn't leave a + # security group and a keypair behind + template_path = build.find_template() + if not template_path: + helpers.clean_tmp_files(build.tmp_dir) + sys.exit(1) network_id = build.find_network_id(network_name) if not network_id: + helpers.clean_tmp_files(build.tmp_dir) + sys.exit(1) + + logging.info('Installing Packer plugins...') + if build.run_packer_init(template_path) != 0: + logging.info('Failed to install Packer plugins') + helpers.clean_tmp_files(build.tmp_dir) sys.exit(1) + logging.info('Creating Packer security group...') + secgroup_name, secgroup_id = build.create_security_group() + + logging.info('Creating Packer keypair...') + key_name, keypair_id = build.create_keypairs() + logging.info('Running Packer...') - exitcode = build.run_packer(secgroup_name, key_name, network_id) + exitcode = build.run_packer(template_path, secgroup_name, key_name, network_id) if exitcode == 0: artifact_id = build.parse_manifest() logging.info("Successfully created image id %s" % artifact_id) diff --git a/template b/template deleted file mode 100644 index 3e5c510..0000000 --- a/template +++ /dev/null @@ -1,35 +0,0 @@ -{ - "builders": [{ - "type": "openstack", - "image_name": "{{user `image_name`}}", - "source_image": "{{user `source_image`}}", - "flavor": "{{user `flavor`}}", - "networks": "{{user `network`}}", - "security_groups": "{{user `security_group`}}", - "ssh_username": "{{user `ssh_username`}}", - "ssh_keypair_name": "{{user `ssh_keypair_name`}}", - "ssh_private_key_file": "{{user `ssh_key_path`}}", - "ssh_pty": "true", - "availability_zone": "{{user `availability_zone`}}" - }], - "post-processors": [{ - "type": "manifest", - "output": "{{user `manifest_path`}}" - }], - "provisioners": [{ - "scripts": [ - "scripts/uio-rhel.sh", - "scripts/fstrim.sh", - "scripts/qemu_guest_agent.sh", - "scripts/upgrade.sh", - "scripts/enable_ipv6.sh", - "scripts/autopatch.sh", - "scripts/report.sh", - "scripts/sshd_hardening.sh", - "{{user `provision_script`}}", - "scripts/cleanup.sh", - "scripts/uio-cleanup.sh" - ], - "type": "shell" - }] -} diff --git a/template.pkr.hcl b/template.pkr.hcl new file mode 100644 index 0000000..4b9fd9d --- /dev/null +++ b/template.pkr.hcl @@ -0,0 +1,101 @@ +packer { + required_plugins { + openstack = { + version = "~> 1.1" + source = "github.com/hashicorp/openstack" + } + } +} + +variable "image_name" { + type = string + description = "Name of the resulting image in Glance" +} + +variable "source_image" { + type = string + description = "ID of the image to build from" +} + +variable "flavor" { + type = string + description = "Flavor of the build instance" +} + +variable "network" { + type = string + description = "ID of the network to attach the build instance to" +} + +variable "security_group" { + type = string + description = "Name of the temporary security group" +} + +variable "ssh_username" { + type = string + description = "User created by cloud-init in the source image" +} + +variable "ssh_keypair_name" { + type = string + description = "Name of the temporary keypair as stored in OpenStack" +} + +variable "ssh_key_path" { + type = string + description = "Path to the private key matching ssh_keypair_name" +} + +variable "availability_zone" { + type = string + description = "Availability zone to build in" +} + +variable "manifest_path" { + type = string + description = "Where the manifest post-processor writes the build result" +} + +variable "provision_script" { + type = string + description = "User supplied provision script" + default = "/bin/true" +} + +source "openstack" "image" { + image_name = var.image_name + source_image = var.source_image + flavor = var.flavor + networks = [var.network] + security_groups = [var.security_group] + ssh_username = var.ssh_username + ssh_keypair_name = var.ssh_keypair_name + ssh_private_key_file = var.ssh_key_path + ssh_pty = true + availability_zone = var.availability_zone +} + +build { + sources = ["source.openstack.image"] + + provisioner "shell" { + scripts = [ + "${path.root}/scripts/uio-rhel.sh", + "${path.root}/scripts/fstrim.sh", + "${path.root}/scripts/qemu_guest_agent.sh", + "${path.root}/scripts/upgrade.sh", + "${path.root}/scripts/enable_ipv6.sh", + "${path.root}/scripts/autopatch.sh", + "${path.root}/scripts/report.sh", + "${path.root}/scripts/sshd_hardening.sh", + var.provision_script, + "${path.root}/scripts/cleanup.sh", + "${path.root}/scripts/uio-cleanup.sh", + ] + } + + post-processor "manifest" { + output = var.manifest_path + } +} From 54606734ec7f601929c4da004170a8e80d470f21 Mon Sep 17 00:00:00 2001 From: Trond Hasle Amundsen Date: Fri, 11 Sep 2026 10:14:57 +0200 Subject: [PATCH 03/22] ci: pin ruff to 0.15.22 The workflow installed ruff unpinned, so CI started failing the moment ruff 0.16.0 shipped a wider default rule set: it reports 73 findings on code that predates this change (mostly LOG015 root-logger calls and UP031 percent formatting). Pin to the newest release that lints clean so lint results are reproducible, and adopt the new defaults deliberately instead of on whatever day upstream releases. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A4Y4JQ39uQMSZcBYoTE84y --- .github/workflows/python-package.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/python-package.yml b/.github/workflows/python-package.yml index 263716b..8074b1e 100644 --- a/.github/workflows/python-package.yml +++ b/.github/workflows/python-package.yml @@ -27,7 +27,9 @@ jobs: - name: Install dependencies run: | python -m pip install --upgrade pip - pip install ruff pytest + # ruff is pinned: 0.16.0 widened the default rule set, which flags a + # lot of pre-existing style in this repo. Bump deliberately. + pip install 'ruff==0.15.22' pytest if [ -f requirements.txt ]; then pip install -r requirements.txt; fi - name: Lint with ruff run: | From 6a1ae198e5014ccdbb29fbc3dbfd7a8412f9780a Mon Sep 17 00:00:00 2001 From: Trond Hasle Amundsen Date: Fri, 11 Sep 2026 10:37:09 +0200 Subject: [PATCH 04/22] ci: drop unused pytest install There are no tests in the repository and the workflow never invoked pytest, so installing it only slowed the job down and made CI look like it ran a test suite. Fix the header comment to match what the workflow actually does. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A4Y4JQ39uQMSZcBYoTE84y --- .github/workflows/python-package.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/python-package.yml b/.github/workflows/python-package.yml index 8074b1e..d5de352 100644 --- a/.github/workflows/python-package.yml +++ b/.github/workflows/python-package.yml @@ -1,4 +1,4 @@ -# This workflow will install Python dependencies, run tests and lint with a variety of Python versions +# This workflow will install Python dependencies and lint with a variety of Python versions # For more information see: https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-python name: Python package @@ -29,7 +29,7 @@ jobs: python -m pip install --upgrade pip # ruff is pinned: 0.16.0 widened the default rule set, which flags a # lot of pre-existing style in this repo. Bump deliberately. - pip install 'ruff==0.15.22' pytest + pip install 'ruff==0.15.22' if [ -f requirements.txt ]; then pip install -r requirements.txt; fi - name: Lint with ruff run: | From 30752586e13052d52b3464fe73015c0f9ae8de49 Mon Sep 17 00:00:00 2001 From: Trond Hasle Amundsen Date: Fri, 11 Sep 2026 10:38:37 +0200 Subject: [PATCH 05/22] ci: lint on every branch, not just master Work happens on feature branches, so waiting for a pull request to find out that lint is broken is too late. Build every branch on push, and keep the pull_request trigger so contributions from forks are still covered. Add a concurrency group keyed on the source branch, since building every branch would otherwise mean two runs for every pull request opened from this repository. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A4Y4JQ39uQMSZcBYoTE84y --- .github/workflows/python-package.yml | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/.github/workflows/python-package.yml b/.github/workflows/python-package.yml index d5de352..8c6908e 100644 --- a/.github/workflows/python-package.yml +++ b/.github/workflows/python-package.yml @@ -5,10 +5,17 @@ name: Python package on: push: - branches: [ "master" ] + branches: [ "**" ] pull_request: branches: [ "master" ] +# Without this, a pull request from a branch in this repository is built twice, +# once for the push and once for the pull_request. Keying on the source branch +# puts both in the same group, and a new push supersedes a running build. +concurrency: + group: ${{ github.workflow }}-${{ github.head_ref || github.ref }} + cancel-in-progress: true + jobs: build: From 7a06574e455da14659e10df3b3e02dc41e6e2e9f Mon Sep 17 00:00:00 2001 From: Trond Hasle Amundsen Date: Fri, 11 Sep 2026 10:42:35 +0200 Subject: [PATCH 06/22] ci: bump checkout and setup-python to v7 Both actions were pinned to majors that run on Node 20, which GitHub now forces onto Node 24 and warns about on every run. checkout v4 and setup-python v5 are still Node 20, so the bump has to go further than one major: Node 24 arrives in checkout v5 and setup-python v6, and v7 is current for both. Nothing in the newer majors affects this workflow. checkout v7 only blocks fork checkouts for pull_request_target and workflow_run, neither of which is used here, and setup-python v7 only drops the pip-install input, which is not used either. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A4Y4JQ39uQMSZcBYoTE84y --- .github/workflows/python-package.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/python-package.yml b/.github/workflows/python-package.yml index 8c6908e..3ba69c3 100644 --- a/.github/workflows/python-package.yml +++ b/.github/workflows/python-package.yml @@ -26,9 +26,9 @@ jobs: python-version: ["3.9", "3.10", "3.11"] steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v7 - name: Set up Python ${{ matrix.python-version }} - uses: actions/setup-python@v4 + uses: actions/setup-python@v7 with: python-version: ${{ matrix.python-version }} - name: Install dependencies From 2ccdd92e23fcf4b7e1212f1807d410657edab701 Mon Sep 17 00:00:00 2001 From: Trond Hasle Amundsen Date: Fri, 11 Sep 2026 10:48:40 +0200 Subject: [PATCH 07/22] deps: update OpenStack clients to current releases Every pin was three to five years old. keystoneauth1 4.3.1, novaclient 17.4.0, glanceclient 3.3.0 and neutronclient 7.3.0 all date from around 2021, and pbr 5.11.1 from 2023. Only cryptography was current, because dependabot bumps it. Pins go to the newest releases that still support Python 3.9, which is what the CI matrix tests. keystoneauth1 and python-glanceclient are the two held back by that floor; the rest are now current. pbr goes to 7.0.3 rather than 7.1.2, which is yanked. Verified the pin set resolves for 3.9, 3.10 and 3.11, and that every OpenStack API the code calls still exists with a compatible signature: v3.Password, Session(verify=...), the nova/neutron/glance client constructors, keypairs create and delete, the security group and rule calls, list_networks, and the glance image create, upload and delete. Note that importing neutronclient now prints a deprecation warning: the bindings are slated for removal in favour of openstacksdk, which is already installed as a transitive dependency. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A4Y4JQ39uQMSZcBYoTE84y --- requirements.txt | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/requirements.txt b/requirements.txt index 87939c9..d86c531 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,6 +1,13 @@ -keystoneauth1==4.3.1 -python-novaclient==17.4.0 -python-glanceclient==3.3.0 -python-neutronclient==7.3.0 -cryptography==46.0.7 -pbr==5.11.1 +# Pinned to the newest releases that still support Python 3.9, the floor the CI +# matrix tests. Two packages are held back by that floor: keystoneauth1 5.12 +# requires 3.10 and 5.15 requires 3.11, python-glanceclient 4.9 requires 3.10 +# and 4.13 requires 3.11. Raising the floor to 3.11 would allow keystoneauth1 +# 5.17.0 and python-glanceclient 4.13.0. Everything else is already current. +keystoneauth1==5.11.1 +python-novaclient==18.13.1 +python-glanceclient==4.8.0 +# Deprecated upstream in favour of openstacksdk, which this already pulls in as +# a dependency. The bindings still work but are slated for removal. +python-neutronclient==14.0.0 +cryptography==50.0.1 +pbr==7.0.3 From c193685fd0c1c72f29ee2751f16c6b30aa013ba7 Mon Sep 17 00:00:00 2001 From: Trond Hasle Amundsen Date: Fri, 11 Sep 2026 11:48:36 +0200 Subject: [PATCH 08/22] refactor: use openstacksdk for network calls, drop python-neutronclient python-neutronclient prints a deprecation warning on import and its bindings are slated for removal in favour of openstacksdk, which was already installed as a transitive dependency. All four network calls move over: creating and deleting the temporary security group, its two SSH rules, and the network lookup. The connection rather than the conn.network proxy is stored on the instance. Touching a proxy authenticates and discovers endpoints, so building one in the constructor would have made it do network I/O, which neutronclient never did. find_network_id() uses networks() rather than the more idiomatic find_network(), which raises on a duplicate name where this code has always taken the first match. Behaviour is unchanged, including returning False when nothing matches. Verified against openstacksdk 4.5.0 and 4.19.1 by binding a fake proxy to the real method signatures, so a renamed or re-typed argument fails the check: both rules are still created with the right ethertypes against the new group, the lookup returns the id, a miss still returns False, and cleanup still deletes the group. openstacksdk is now pinned explicitly at 4.5.0, the newest release supporting Python 3.9. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A4Y4JQ39uQMSZcBYoTE84y --- CLAUDE.md | 7 +++-- example/requirements.txt | 2 +- image_builder/build.py | 61 +++++++++++++++------------------------- requirements.txt | 11 ++++---- 4 files changed, 34 insertions(+), 47 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 78d8c95..3fd60b4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -48,7 +48,7 @@ selects the method, `sys.argv[2:]` is parsed by that method's own parser, and `b 1. `helpers.make_tmp_dir()` — a `tempfile.mkdtemp(prefix='imagebuilder-')` holding the SSH keypair and Packer's manifest; everything in the flow is scoped to it. 2. `find_template()` — locates `template.pkr.hcl` in `template_dir`. -3. `find_network_id()` — resolves the network name (default `Dualstack`) to an id via neutron. +3. `find_network_id()` — resolves the network name (default `Dualstack`) to an id via openstacksdk. 4. `run_packer_init()` — `packer init`, which installs the openstack plugin. Steps 2–4 deliberately run *before* anything is created in OpenStack: each can fail on bad user @@ -65,7 +65,10 @@ selects the method, `sys.argv[2:]` is parsed by that method's own parser, and `b Glance**), optional `--purge-source`, then `cleanup()` + `clean_tmp_files()`. Three OpenStack clients are used side by side, each region-scoped from the same keystone session: -nova (keypairs), neutron (security groups, networks), glance (image delete). Cleanup is not +novaclient (keypairs), an openstacksdk `Connection` (security groups, networks), glanceclient (image +delete). Note `self.conn` holds the connection rather than the `conn.network` proxy, because touching +a proxy authenticates and discovers endpoints — the constructor is deliberately free of network +traffic. Cleanup is not signal-safe — if the process dies mid-build the `imagebuilder-*` security group and keypair are orphaned and must be deleted by hand. diff --git a/example/requirements.txt b/example/requirements.txt index d7a68bf..fa60d85 100644 --- a/example/requirements.txt +++ b/example/requirements.txt @@ -2,5 +2,5 @@ keystoneauth1 python-novaclient python-glanceclient -python-neutronclient +openstacksdk python-openstackclient diff --git a/image_builder/build.py b/image_builder/build.py index ac0788a..034a070 100644 --- a/image_builder/build.py +++ b/image_builder/build.py @@ -5,7 +5,7 @@ import time import uuid from novaclient import client as novaclient -from neutronclient.v2_0 import client as neutronclient +from openstack.connection import Connection from glanceclient import Client as glanceclient from .helpers import Helpers as helpers @@ -32,13 +32,16 @@ def __init__(self, self.download_dir = download_dir self.tmp_dir = helpers.make_tmp_dir() self.nova = novaclient.Client("2", session=session, region_name=region) - self.neutron = neutronclient.Client(session=session, region_name=region) + # Only the connection is built here. Reaching for a proxy such as + # conn.network authenticates and discovers endpoints, so that is left + # to the call sites to keep this constructor free of network traffic + self.conn = Connection(session=session, region_name=region) self.glance = glanceclient('2', session=session, region_name=region) def cleanup(self, secgroup_id, keypair_id): """Cleans up the mess we've made""" logging.info('Removing temporary security group...') - self.neutron.delete_security_group(secgroup_id) + self.conn.network.delete_security_group(secgroup_id) logging.info('Removing temporary keypair...') self.nova.keypairs.delete(key=keypair_id) @@ -59,39 +62,19 @@ def create_keypairs(self): def create_security_group(self): """Creates a temporary security group""" secgroup_name = "imagebuilder-" + str(uuid.uuid4().hex) - # pylint: disable=line-too-long - secgroup = self.neutron.create_security_group(body={ - 'security_group': - { - 'name': secgroup_name, - 'description': 'Temporary security group for image building' - } - }) - secgroup_id = secgroup['security_group']['id'] + secgroup = self.conn.network.create_security_group( + name=secgroup_name, + description='Temporary security group for image building') logging.info('Creating rule allowing SSH traffic...') - self.neutron.create_security_group_rule(body={ - 'security_group_rule': - { - 'security_group_id': secgroup_id, - 'direction': 'ingress', - 'protocol': 'tcp', - 'port_range_min': 22, - 'port_range_max': 22, - 'ethertype': 'IPv4' - } - }) - self.neutron.create_security_group_rule(body={ - 'security_group_rule': - { - 'security_group_id': secgroup_id, - 'direction': 'ingress', - 'protocol': 'tcp', - 'port_range_min': 22, - 'port_range_max': 22, - 'ethertype': 'IPv6' - } - }) - return secgroup_name, secgroup_id + for ethertype in ('IPv4', 'IPv6'): + self.conn.network.create_security_group_rule( + security_group_id=secgroup.id, + direction='ingress', + protocol='tcp', + port_range_min=22, + port_range_max=22, + ethertype=ethertype) + return secgroup_name, secgroup.id def delete_image(self, image_id): logging.info('Removing image %s' % image_id) @@ -124,9 +107,11 @@ def download_image(self, artifact_id): return exitcode def find_network_id(self, name): - networks = self.neutron.list_networks(name=name) - if networks['networks']: - network_id = networks['networks'][0]['id'] + # networks() rather than find_network(), which raises on a duplicate + # name where this has always just taken the first match + network = next(self.conn.network.networks(name=name), None) + if network: + network_id = network.id logging.info("Found network %s with id %s" % (name, network_id)) else: network_id = False diff --git a/requirements.txt b/requirements.txt index d86c531..2b899aa 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,13 +1,12 @@ # Pinned to the newest releases that still support Python 3.9, the floor the CI -# matrix tests. Two packages are held back by that floor: keystoneauth1 5.12 +# matrix tests. Three packages are held back by that floor: keystoneauth1 5.12 # requires 3.10 and 5.15 requires 3.11, python-glanceclient 4.9 requires 3.10 -# and 4.13 requires 3.11. Raising the floor to 3.11 would allow keystoneauth1 -# 5.17.0 and python-glanceclient 4.13.0. Everything else is already current. +# and 4.13 requires 3.11, openstacksdk 4.6 requires 3.10 and 4.14 requires +# 3.11. Raising the floor to 3.11 would allow keystoneauth1 5.17.0, +# python-glanceclient 4.13.0 and openstacksdk 4.20.0. keystoneauth1==5.11.1 python-novaclient==18.13.1 python-glanceclient==4.8.0 -# Deprecated upstream in favour of openstacksdk, which this already pulls in as -# a dependency. The bindings still work but are slated for removal. -python-neutronclient==14.0.0 +openstacksdk==4.5.0 cryptography==50.0.1 pbr==7.0.3 From 5ef43a3c996b69bea3a1ac075e55b57f7b940b6e Mon Sep 17 00:00:00 2001 From: Trond Hasle Amundsen Date: Fri, 11 Sep 2026 11:59:10 +0200 Subject: [PATCH 09/22] docs: document the Python 3.9 floor and prepare the 3.11+ pin set Moving off Python 3.9 does not require EL10. EL9 ships newer interpreters as parallel-installable AppStream packages, so the venv can move to python3.12 on the existing host whenever the three capped pins become a problem. Record why the floor exists and when it stops being comfortable: 3.9 is the minimum supported runtime for OpenStack 2025.1, but 2025.2 raised it to 3.10 and only keeps 3.9 fallbacks in its constraints, pinning exactly the versions used here. The 3.11+ set is left commented in requirements.txt so the switch is an uncomment rather than research. Verified it resolves for 3.11, 3.12 and 3.13, is correctly rejected on 3.9, and that keystoneauth1 5.17.0, glanceclient 4.13.0 and openstacksdk 4.20.0 pass the same API and network checks as the current pins. Commenting the capped pins and uncommenting the block yields exactly that set. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A4Y4JQ39uQMSZcBYoTE84y --- DEVELOPMENT.md | 28 ++++++++++++++++++++++++++++ requirements.txt | 24 ++++++++++++++++++------ 2 files changed, 46 insertions(+), 6 deletions(-) diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index c48718e..fb9c728 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -13,3 +13,31 @@ pip install --upgrade setuptools python setup.py develop pip install -r requirements.txt ``` + +## Python version + +`requirements.txt` pins for Python 3.9, the stock interpreter on EL9. Red Hat +maintains 3.9 for the full RHEL 9 life cycle, so it stays patched well past its +upstream end of life, but three pins are capped by it: keystoneauth1, +python-glanceclient and openstacksdk. + +That floor is fine for OpenStack 2025.1, where 3.9 is the minimum supported +runtime. 2025.2 raised the minimum to 3.10, and while its constraints still +carry 3.9 fallbacks pinning exactly the versions used here, those fallbacks +will not last forever. + +Moving off 3.9 does not require EL10. Newer interpreters ship on EL9 as +parallel-installable AppStream packages, alongside the stock 3.9 and with their +own pip and site-packages: + +``` bash +dnf install -y python3.12 +python3.12 -m venv . +source bin/activate +pip install --upgrade pip +pip install -r requirements.txt +``` + +When switching, uncomment the 3.11+ block at the bottom of `requirements.txt` +and comment out the three capped pins above it. Nothing in the code needs +changing; it has been checked on interpreters up to 3.14. diff --git a/requirements.txt b/requirements.txt index 2b899aa..f117fdf 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,12 +1,24 @@ -# Pinned to the newest releases that still support Python 3.9, the floor the CI -# matrix tests. Three packages are held back by that floor: keystoneauth1 5.12 -# requires 3.10 and 5.15 requires 3.11, python-glanceclient 4.9 requires 3.10 -# and 4.13 requires 3.11, openstacksdk 4.6 requires 3.10 and 4.14 requires -# 3.11. Raising the floor to 3.11 would allow keystoneauth1 5.17.0, -# python-glanceclient 4.13.0 and openstacksdk 4.20.0. +# Pinned for Python 3.9, the stock interpreter on EL9. Three packages are +# capped by that floor: keystoneauth1 5.12 requires 3.10 and 5.15 requires +# 3.11, python-glanceclient 4.9 requires 3.10 and 4.13 requires 3.11, +# openstacksdk 4.6 requires 3.10 and 4.14 requires 3.11. +# +# This is the combination OpenStack itself sanctions for 3.9: the 2025.2 +# upper-constraints pin openstacksdk 4.5.0 and python-glanceclient 4.8.0 for +# python_version=='3.9', and everything here meets or exceeds the 2025.1 +# constraints. keystoneauth1==5.11.1 python-novaclient==18.13.1 python-glanceclient==4.8.0 openstacksdk==4.5.0 cryptography==50.0.1 pbr==7.0.3 + +# On Python 3.11 or newer, comment out the three capped pins above and +# uncomment these. python-novaclient, cryptography and pbr are already current +# and stay as they are. See DEVELOPMENT.md for installing a newer interpreter +# on EL9 without waiting for EL10. +# +# keystoneauth1==5.17.0 +# python-glanceclient==4.13.0 +# openstacksdk==4.20.0 From 547a267fdec33111578bf581af4c634f1e81dd62 Mon Sep 17 00:00:00 2001 From: Trond Hasle Amundsen Date: Sun, 13 Sep 2026 21:32:25 +0200 Subject: [PATCH 10/22] fix: stop leaking OpenStack resources and refusing valid credentials Four bugs from the code audit. Two environment variables were mandatory but unused. OS_IDENTITY_API_VERSION and OS_NO_CACHE were read into the settings dict and never looked at again, yet a missing one aborted the run with "Failed to read environment variables". OS_NO_CACHE is absent from many current openrc files, so valid credentials were rejected with a message pointing at the wrong problem. Only the variables auth() and the region lookup consume are required now, and the error names every missing one instead of just the first. Cleanup was positional rather than unconditional. Everything after the first OpenStack resource is created now runs under a try/finally, so an exception or a Ctrl-C during a long build no longer leaves the security group and keypair behind. cleanup() is written for that position: it skips ids that are None and swallows API errors, because raising from a finally would mask the original failure. clean_tmp_files() likewise no longer fails on an already-removed directory. SIGTERM still bypasses all of this, so TODO.md now says that rather than signals generally. A failed ssh-keygen returned ('', 0) and nobody checked, so the build carried on into packer with an empty keypair name. It returns None now and the caller exits through the cleanup path. parse_manifest() indexed the JSON with no error handling, so a successful build whose manifest was missing or malformed ended in a traceback, which by the bug above also skipped cleanup. It reports and returns None, and the caller treats that as a failed build. Verified by driving main() with a fake build for six scenarios: interrupt during packer, unexpected exception, keygen failure, unusable manifest, clean success and a non-zero packer exit. All six now reach cleanup with the right ids and the right exit code, and interrupts still propagate rather than being swallowed. The same harness against the previous code shows the first two scenarios reaching neither cleanup call. Confirmed separately that a finally block runs on SIGINT but not on SIGTERM. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A4Y4JQ39uQMSZcBYoTE84y --- CLAUDE.md | 10 +++- README.md | 7 ++- TODO.md | 4 +- image_builder/build.py | 45 +++++++++++---- image_builder/helpers.py | 4 +- image_builder/imagebuilder.py | 101 +++++++++++++++++++++------------- 6 files changed, 115 insertions(+), 56 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 3fd60b4..53be979 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -68,9 +68,13 @@ Three OpenStack clients are used side by side, each region-scoped from the same novaclient (keypairs), an openstacksdk `Connection` (security groups, networks), glanceclient (image delete). Note `self.conn` holds the connection rather than the `conn.network` proxy, because touching a proxy authenticates and discovers endpoints — the constructor is deliberately free of network -traffic. Cleanup is not -signal-safe — if the process dies mid-build the `imagebuilder-*` security group and keypair are -orphaned and must be deleted by hand. +traffic. + +Everything after the first OpenStack resource is created runs under a `try/finally`, so `cleanup()` is +reached on exceptions and on Ctrl-C (SIGINT becomes `KeyboardInterrupt`, which unwinds). `cleanup()` is +written for that position: it skips ids that are `None` and swallows API errors, because raising there +would mask the original failure. SIGTERM and SIGKILL still bypass it — verified — so those remain the +one case that orphans `imagebuilder-*` resources. **`bootstrap`** (`bootstrap.py:BootstrapFunctions`) — downloads a cloud image from a URL, optionally verifies it against a checksum file (substring match of the computed hexdigest in the fetched file), and diff --git a/README.md b/README.md index 05d57a9..e56d8f5 100644 --- a/README.md +++ b/README.md @@ -56,8 +56,11 @@ image of choice (i.e. centos, ubuntu), -p is the path to your provision script and -d downloads the image after it's been built. imagebuilder creates a temporary security group and keypair named -"imagebuilder-" which will be deleted after completion. Note that you for -now will have to delete these manually if the command is not allowed to finish. +"imagebuilder-" which will be deleted after completion. These are also +removed if the build fails or you interrupt it with Ctrl-C. A process killed +outright (SIGTERM, SIGKILL) cannot run its cleanup, so in that case you will +still have to delete the leftover "imagebuilder-*" security group and keypair +yourself. Run `imagebuilder -h` for a complete list of options. diff --git a/TODO.md b/TODO.md index dbc2810..8ce3ac2 100644 --- a/TODO.md +++ b/TODO.md @@ -1,6 +1,8 @@ * Exception handling * Isn't very good at the moment - * Handle signals (clean up before dying) + * Handle SIGTERM (clean up before dying). Exceptions and Ctrl-C already + unwind through the try/finally in main(), but a SIGTERM kills the process + without running it, so those builds still leak a security group and keypair * Post-process images * Convert image from raw to qcow2 after download diff --git a/image_builder/build.py b/image_builder/build.py index 034a070..83e3b44 100644 --- a/image_builder/build.py +++ b/image_builder/build.py @@ -39,11 +39,24 @@ def __init__(self, self.glance = glanceclient('2', session=session, region_name=region) def cleanup(self, secgroup_id, keypair_id): - """Cleans up the mess we've made""" - logging.info('Removing temporary security group...') - self.conn.network.delete_security_group(secgroup_id) - logging.info('Removing temporary keypair...') - self.nova.keypairs.delete(key=keypair_id) + """Cleans up the mess we've made + + Runs from a finally block, so it has to cope with a build that only got + half way and must not raise on its way out: an exception here would + mask whatever actually went wrong. + """ + if secgroup_id: + logging.info('Removing temporary security group...') + try: + self.conn.network.delete_security_group(secgroup_id) + except Exception as error: + logging.info("Failed to remove security group %s: %s" % (secgroup_id, error)) + if keypair_id: + logging.info('Removing temporary keypair...') + try: + self.nova.keypairs.delete(key=keypair_id) + except Exception as error: + logging.info("Failed to remove keypair %s: %s" % (keypair_id, error)) def create_keypairs(self): """Creates a temporary keypair""" @@ -52,7 +65,8 @@ def create_keypairs(self): cmd = "/usr/bin/ssh-keygen -b 521 -t ecdsa -N '' -f " + os.path.join(self.tmp_dir, keyname) out = subprocess.call(cmd, shell=True) # generate temporary ssh key if out: # something went wrong with the key generation - return '', 0 + logging.info("Failed to generate SSH key, ssh-keygen exited %s" % out) + return None, None # read public key string and store into Openstack with open(os.path.join(self.tmp_dir, keyname) + ".pub", "r") as pubfile: pubkeystring = pubfile.read().replace('\n', '') @@ -139,11 +153,20 @@ def find_template(self): return None def parse_manifest(self): - """Parses the manifest file generated by packer""" - with open(os.path.join(self.tmp_dir, 'packer-manifest.json')) as data_file: - manifest = json.load(data_file) - artifact_id = manifest['builds'][0]['artifact_id'] - return artifact_id + """Parses the manifest file generated by packer + + Packer exiting successfully does not guarantee the manifest + post-processor left a usable file behind, so report that rather than + raising and taking the build down with a traceback. + """ + manifest_path = os.path.join(self.tmp_dir, 'packer-manifest.json') + try: + with open(manifest_path) as data_file: + manifest = json.load(data_file) + return manifest['builds'][0]['artifact_id'] + except (OSError, ValueError, KeyError, IndexError) as error: + logging.info("Failed to read the image id from %s: %s" % (manifest_path, error)) + return None def run_packer(self, template_path, secgroup_name, key_name, network_id): """Executes Packer command""" diff --git a/image_builder/helpers.py b/image_builder/helpers.py index db06506..eae8569 100644 --- a/image_builder/helpers.py +++ b/image_builder/helpers.py @@ -7,8 +7,10 @@ class Helpers(object): @staticmethod def clean_tmp_files(tmp_dir): + # Also called from a finally block, so failing to remove a scratch + # directory must not be what the user ends up seeing logging.info('Removing temporary directory with content...') - shutil.rmtree(tmp_dir) + shutil.rmtree(tmp_dir, ignore_errors=True) @staticmethod def make_tmp_dir(): diff --git a/image_builder/imagebuilder.py b/image_builder/imagebuilder.py index 7c1ea95..44408c9 100644 --- a/image_builder/imagebuilder.py +++ b/image_builder/imagebuilder.py @@ -27,19 +27,32 @@ def auth(rc): sess = session.Session(auth) return sess - @staticmethod - def get_openstack_rc(): - env_var = {} - env_var['username'] = os.environ['OS_USERNAME'] - env_var['project_name'] = os.environ['OS_PROJECT_NAME'] - env_var['cacert'] = os.environ['OS_CACERT'] if "OS_CACERT" in os.environ else None - env_var['password'] = os.environ['OS_PASSWORD'] - env_var['auth_url'] = os.environ['OS_AUTH_URL'] - env_var['api_version'] = os.environ['OS_IDENTITY_API_VERSION'] - env_var['user_domain_name'] = os.environ['OS_USER_DOMAIN_NAME'] - env_var['project_domain_name'] = os.environ['OS_PROJECT_DOMAIN_NAME'] - env_var['region_name'] = os.environ['OS_REGION_NAME'] - env_var['no_cache'] = os.environ['OS_NO_CACHE'] + # Only what auth() and the region lookup actually consume. Asking for more + # than that turns a perfectly good openrc file into a failed login + REQUIRED_ENV = { + 'username': 'OS_USERNAME', + 'project_name': 'OS_PROJECT_NAME', + 'password': 'OS_PASSWORD', + 'auth_url': 'OS_AUTH_URL', + 'user_domain_name': 'OS_USER_DOMAIN_NAME', + 'project_domain_name': 'OS_PROJECT_DOMAIN_NAME', + 'region_name': 'OS_REGION_NAME', + } + + @classmethod + def get_openstack_rc(cls): + """Reads the credentials from the environment + + OS_CACERT is optional. Raises KeyError naming every variable that is + missing rather than just the first one found. + """ + missing = [name for name in cls.REQUIRED_ENV.values() + if name not in os.environ] + if missing: + raise KeyError(', '.join(missing)) + env_var = {key: os.environ[name] + for key, name in cls.REQUIRED_ENV.items()} + env_var['cacert'] = os.environ.get('OS_CACERT') return env_var def main(): @@ -48,11 +61,11 @@ def main(): try: rc = imagebuilder.get_openstack_rc() - except KeyError: - print("""Failed to read environment variables. + except KeyError as missing: + print("""Missing environment variable(s): %s Please run: source -and try again.""") +and try again.""" % missing.args[0]) sys.exit(1) ib_session = imagebuilder.auth(rc) @@ -126,31 +139,43 @@ def main(): logging.info('Creating Packer security group...') secgroup_name, secgroup_id = build.create_security_group() - logging.info('Creating Packer keypair...') - key_name, keypair_id = build.create_keypairs() + keypair_id = None + exitcode = 1 - logging.info('Running Packer...') - exitcode = build.run_packer(template_path, secgroup_name, key_name, network_id) - if exitcode == 0: - artifact_id = build.parse_manifest() - logging.info("Successfully created image id %s" % artifact_id) - if commands.build_args.download: - exitcode = build.download_image(artifact_id) - else: - exitcode = 0 - else: - logging.info('Build failed') - exitcode = 1 - - logging.info('Cleaning up...') - build.cleanup(secgroup_id, keypair_id) - if commands.build_args.purge_source: - if build.delete_image(source_image): - logging.info('Successfully deleted source image') + # From here on something exists in OpenStack, so the rest runs under a + # finally. An interrupt during the build, or any exception on the way, + # has to still remove the security group and the keypair rather than + # leave them behind in the project + try: + logging.info('Creating Packer keypair...') + key_name, keypair_id = build.create_keypairs() + if key_name is None: + sys.exit(1) + + logging.info('Running Packer...') + exitcode = build.run_packer(template_path, secgroup_name, key_name, network_id) + if exitcode == 0: + artifact_id = build.parse_manifest() + if artifact_id is None: + exitcode = 1 + else: + logging.info("Successfully created image id %s" % artifact_id) + if commands.build_args.download: + exitcode = build.download_image(artifact_id) else: - logging.info('Failed to delete source image') + logging.info('Build failed') exitcode = 1 - helpers.clean_tmp_files(build.tmp_dir) + + if commands.build_args.purge_source: + if build.delete_image(source_image): + logging.info('Successfully deleted source image') + else: + logging.info('Failed to delete source image') + exitcode = 1 + finally: + logging.info('Cleaning up...') + build.cleanup(secgroup_id, keypair_id) + helpers.clean_tmp_files(build.tmp_dir) sys.exit(exitcode) From f0ef35f5658d330e7dd60679106b326bb9544fa3 Mon Sep 17 00:00:00 2001 From: Trond Hasle Amundsen Date: Sun, 13 Sep 2026 21:43:01 +0200 Subject: [PATCH 11/22] fix: address the medium findings from the code audit Bootstrap correctness. checksum_file() dispatched digests with a hand-written if/if/elif ladder, so anything outside sha256/sha512/md5 stayed a string and died with AttributeError on the first update(); -t sha1 and -t SHA256 both crashed. It uses hashlib.new() now, which takes every supported algorithm and raises a clear ValueError otherwise, and a bad -t is rejected before the download rather than after it. Checksum verification was a substring test against the repr of the raw bytes, so it was case sensitive and never tied a hash to a filename. Checksum files are now parsed properly in both the coreutils and BSD layouts and matched on the basename, comparing lowercased. Where the file has no entry naming the download, it falls back to the old contains check with a warning, so vendors that name things differently keep working. The size check read content-length, which is absent from chunked responses and describes what the server claimed rather than what arrived; it uses the size on disk now. Both urlopen() calls got a timeout, so a stalled mirror fails instead of hanging forever. Output. Packer's log was emitted through '%r' and arrived as b'...\n' for every line; it is decoded and stripped. Diagnostics moved to stderr, which matters because bootstrap's contract is that stdout carries only the new image id for piping into build -s. One client library. novaclient and glanceclient are gone: keypairs move to conn.compute, images to conn.image. That also replaces the glance CLI subprocess in download_image(), which re-authenticated from OS_* independently of the session already held, and removes glanceclient from the packages capped by Python 3.9. Bootstrap uploads through conn.image.create_image with allow_duplicates=True, since the default returns an existing same-named image instead of creating one, and disable_vendor_agent=False so it does not add image properties of its own. Subcommands use argparse subparsers rather than getattr(self, argv[1]), which made every attribute reachable from the command line and left the command list as unchecked prose in the usage string. --min-ram and --min-disk validate as ints at parse time. ssh-keygen runs as a list without a shell and is found on PATH instead of hardcoded to /usr/bin. Verified with 60 new checks covering digest dispatch, both checksum layouts including the uppercase and wrong-name cases, size and timeout handling, decoded output, stdout staying empty on failure, the subparser behaviour, and download_image writing the file, deleting from Glance on success, and removing the partial file without deleting from Glance on failure. The earlier suites still pass. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A4Y4JQ39uQMSZcBYoTE84y --- CLAUDE.md | 9 +- DEVELOPMENT.md | 4 +- README.md | 3 +- example/requirements.txt | 2 - image_builder/bootstrap.py | 146 ++++++++++++++++------ image_builder/build.py | 72 ++++++----- image_builder/helpers.py | 31 +++-- image_builder/imagebuilder.py | 10 +- image_builder/parsecommands.py | 215 +++++++++++++++------------------ requirements.txt | 21 ++-- 10 files changed, 287 insertions(+), 226 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 53be979..45eaed9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -61,12 +61,13 @@ selects the method, `sys.argv[2:]` is parsed by that method's own parser, and `b 7. `parse_manifest()` — Packer's `manifest` post-processor writes `packer-manifest.json` into the temp dir; `builds[0].artifact_id` is the resulting Glance image id. This is the only channel by which the build result comes back from Packer. -8. Optional `download_image()` (shells out to `glance image-download`, then **deletes the image from +8. Optional `download_image()` (via `conn.image.download_image`, then **deletes the image from Glance**), optional `--purge-source`, then `cleanup()` + `clean_tmp_files()`. -Three OpenStack clients are used side by side, each region-scoped from the same keystone session: -novaclient (keypairs), an openstacksdk `Connection` (security groups, networks), glanceclient (image -delete). Note `self.conn` holds the connection rather than the `conn.network` proxy, because touching +One openstacksdk `Connection` covers everything: `conn.compute` (keypairs), `conn.network` (security +groups, networks), `conn.image` (image create, download, delete). novaclient and glanceclient were +dropped once the last calls moved over. Note `self.conn` holds the connection rather than a proxy +such as `conn.network`, because touching a proxy authenticates and discovers endpoints — the constructor is deliberately free of network traffic. diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index fb9c728..d2ed7d5 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -18,8 +18,8 @@ pip install -r requirements.txt `requirements.txt` pins for Python 3.9, the stock interpreter on EL9. Red Hat maintains 3.9 for the full RHEL 9 life cycle, so it stays patched well past its -upstream end of life, but three pins are capped by it: keystoneauth1, -python-glanceclient and openstacksdk. +upstream end of life, but two pins are capped by it: keystoneauth1 and +openstacksdk. That floor is fine for OpenStack 2025.1, where 3.9 is the minimum supported runtime. 2025.2 raised the minimum to 3.10, and while its constraints still diff --git a/README.md b/README.md index e56d8f5..2c17b8a 100644 --- a/README.md +++ b/README.md @@ -19,8 +19,7 @@ Pull requests are welcome! ### Requirements * Python 3 * [keystoneauth](https://github.com/openstack/keystoneauth) -* [python-novaclient](https://github.com/openstack/python-novaclient) -* [python-glanceclient](https://github.com/openstack/python-glanceclient) +* [openstacksdk](https://github.com/openstack/openstacksdk) * [Packer](https://packer.io) 1.7 or newer * OpenSSL command line tool diff --git a/example/requirements.txt b/example/requirements.txt index fa60d85..e6bcb0f 100644 --- a/example/requirements.txt +++ b/example/requirements.txt @@ -1,6 +1,4 @@ -e git+https://github.com/norcams/imagebuilder.git#egg=imagebuilder keystoneauth1 -python-novaclient -python-glanceclient openstacksdk python-openstackclient diff --git a/image_builder/bootstrap.py b/image_builder/bootstrap.py index 9a2b26f..e60febd 100644 --- a/image_builder/bootstrap.py +++ b/image_builder/bootstrap.py @@ -1,9 +1,17 @@ import logging import os import urllib.request -from glanceclient import Client +from openstack.connection import Connection from .helpers import Helpers as helpers +# Blocking socket operations give up after this long. This is a per-read +# timeout, not a limit on the whole transfer, so a large image is fine as long +# as bytes keep arriving +SOCKET_TIMEOUT = 60 + +# Anything smaller than this is an error page, not a cloud image +MIN_IMAGE_BYTES = 1000 + class BootstrapFunctions(object): def __init__(self, session, @@ -12,13 +20,45 @@ def __init__(self, self.session = session self.avail_zone = avail_zone self.tmp_dir = helpers.make_tmp_dir() - self.glance = Client("2", session=session, region_name=region) + self.conn = Connection(session=session, region_name=region) + + @staticmethod + def find_expected_checksum(checksum_text, file_name): + """Finds the checksum recorded for file_name in a checksum file + + Handles the two layouts in common use: the coreutils one, ' + ', where the name may carry a '*' prefix, and the BSD one, + 'SHA256 () = '. Returns the hash lowercased, or None when + the file has no entry for this name. + """ + for line in checksum_text.splitlines(): + line = line.strip() + if not line or line.startswith('#'): + continue + if '(' in line and ')' in line and '=' in line: + name = line[line.index('(') + 1:line.rindex(')')] + value = line.rsplit('=', 1)[1] + else: + fields = line.split() + if len(fields) < 2: + continue + value, name = fields[0], fields[-1].lstrip('*') + if os.path.basename(name) == file_name: + return value.strip().lower() + return None def download_and_check(self, url, checksum_url=None, checksum_dig='sha256'): user_agent = 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/35.0.1916.47 Safari/537.36' CHUNK = 16 * 1024 file_name = url.split("/")[-1] file_path = os.path.join(self.tmp_dir, file_name) + + # Checked up front so a typo in -t fails now rather than after the + # download has finished + if checksum_url and not helpers.valid_digest(checksum_dig): + logging.info("Unknown checksum digest: %s" % checksum_dig) + return None + req = urllib.request.Request( url, data=None, @@ -26,7 +66,7 @@ def download_and_check(self, url, checksum_url=None, checksum_dig='sha256'): 'User-Agent': user_agent } ) - response = urllib.request.urlopen(req) + response = urllib.request.urlopen(req, timeout=SOCKET_TIMEOUT) with open(file_path, "wb") as f: while True: chunk = response.read(CHUNK) @@ -34,49 +74,75 @@ def download_and_check(self, url, checksum_url=None, checksum_dig='sha256'): break f.write(chunk) response.close() - if int(response.headers["content-length"]) < 1000: - logging.info("File is too small: %s" % url) + + # The size of what actually landed, not what the server claimed in a + # header it is not obliged to send + size = os.path.getsize(file_path) + if size < MIN_IMAGE_BYTES: + logging.info("File is too small (%d bytes): %s" % (size, url)) os.remove(file_path) return None - if checksum_url: - req = urllib.request.Request( - checksum_url, - data=None, - headers = { - 'User-Agent': user_agent - } - ) - logging.info("Verifying checksum of %s..." % file_path) - response = urllib.request.urlopen(req) - checksum_all = str(response.read()) - response.close() - logging.debug(checksum_all) - logging.debug("Checksum type is %s" % checksum_dig) - checksum_file = helpers.checksum_file(file_path, checksum_dig) - if checksum_file in checksum_all: - logging.info("Checksum ok: %s" % checksum_file) + + if not checksum_url: + return file_path + + req = urllib.request.Request( + checksum_url, + data=None, + headers = { + 'User-Agent': user_agent + } + ) + logging.info("Verifying checksum of %s..." % file_path) + response = urllib.request.urlopen(req, timeout=SOCKET_TIMEOUT) + checksum_text = response.read().decode('utf-8', errors='replace') + response.close() + logging.debug(checksum_text) + logging.debug("Checksum type is %s" % checksum_dig) + + actual = helpers.checksum_file(file_path, checksum_dig) + expected = self.find_expected_checksum(checksum_text, file_name) + + if expected is None: + # No entry naming this file. Fall back to the old behaviour of + # looking for the hash anywhere in the file, which is weaker but + # keeps working with checksum files that name things differently + logging.info("No entry for %s in %s, falling back to matching the " + "hash anywhere in the file" % (file_name, checksum_url)) + if actual in checksum_text.lower(): + logging.info("Checksum ok: %s" % actual) return file_path - else: - logging.info("Checksum not ok: %s" % checksum_file) - return None - else: + logging.info("Checksum not found: %s" % actual) + return None + + if actual == expected: + logging.info("Checksum ok: %s" % actual) return file_path + logging.info("Checksum mismatch for %s: expected %s, got %s" + % (file_name, expected, actual)) + return None + def create_glance_image(self, image_file, name, disk_format, min_disk, min_ram, properties): - image = self.glance.images.create(name=name, - visibility="private", - disk_format=disk_format, - min_disk=min_disk, - min_ram=min_ram, - container_format="bare", - **properties) - logging.info("Created image %s" % name) - logging.debug(image) try: - self.glance.images.upload(image.id, open(image_file, "rb")) - logging.info("Successfully uploaded %s" % image_file) - return image.id - except BaseException as error: - logging.debug(error) + # allow_duplicates keeps this a create: left at its default, + # create_image returns an existing same-named image instead, and + # this command's whole contract is to report a new image id. + # disable_vendor_agent would otherwise add properties of its own + image = self.conn.image.create_image(name=name, + filename=image_file, + disk_format=disk_format, + container_format="bare", + visibility="private", + min_disk=min_disk, + min_ram=min_ram, + allow_duplicates=True, + disable_vendor_agent=False, + **properties) + except Exception as error: + logging.info("Failed to upload %s: %s" % (image_file, error)) return None + logging.info("Successfully uploaded %s as image %s" % (image_file, name)) + logging.debug(image) + return image.id diff --git a/image_builder/build.py b/image_builder/build.py index 83e3b44..83177cf 100644 --- a/image_builder/build.py +++ b/image_builder/build.py @@ -4,9 +4,7 @@ import subprocess import time import uuid -from novaclient import client as novaclient from openstack.connection import Connection -from glanceclient import Client as glanceclient from .helpers import Helpers as helpers class BuildFunctions(object): @@ -31,12 +29,10 @@ def __init__(self, self.template_dir = template_dir self.download_dir = download_dir self.tmp_dir = helpers.make_tmp_dir() - self.nova = novaclient.Client("2", session=session, region_name=region) # Only the connection is built here. Reaching for a proxy such as # conn.network authenticates and discovers endpoints, so that is left # to the call sites to keep this constructor free of network traffic self.conn = Connection(session=session, region_name=region) - self.glance = glanceclient('2', session=session, region_name=region) def cleanup(self, secgroup_id, keypair_id): """Cleans up the mess we've made @@ -54,7 +50,7 @@ def cleanup(self, secgroup_id, keypair_id): if keypair_id: logging.info('Removing temporary keypair...') try: - self.nova.keypairs.delete(key=keypair_id) + self.conn.compute.delete_keypair(keypair_id) except Exception as error: logging.info("Failed to remove keypair %s: %s" % (keypair_id, error)) @@ -62,15 +58,20 @@ def create_keypairs(self): """Creates a temporary keypair""" keyname = "imagebuilder-" + str(uuid.uuid4().hex) - cmd = "/usr/bin/ssh-keygen -b 521 -t ecdsa -N '' -f " + os.path.join(self.tmp_dir, keyname) - out = subprocess.call(cmd, shell=True) # generate temporary ssh key + keypath = os.path.join(self.tmp_dir, keyname) + # No shell: there is nothing here a shell is needed for, and letting + # PATH find ssh-keygen beats hardcoding where it lives + cmd = ['ssh-keygen', '-b', '521', '-t', 'ecdsa', '-N', '', '-f', keypath] + logging.debug(cmd) + out = subprocess.call(cmd) # generate temporary ssh key if out: # something went wrong with the key generation logging.info("Failed to generate SSH key, ssh-keygen exited %s" % out) return None, None # read public key string and store into Openstack - with open(os.path.join(self.tmp_dir, keyname) + ".pub", "r") as pubfile: - pubkeystring = pubfile.read().replace('\n', '') - keypair = self.nova.keypairs.create(name=keyname, public_key=pubkeystring) + with open(keypath + ".pub", "r") as pubfile: + pubkeystring = pubfile.read().replace('\n', '') + keypair = self.conn.compute.create_keypair(name=keyname, + public_key=pubkeystring) return keyname, keypair.id def create_security_group(self): @@ -91,34 +92,39 @@ def create_security_group(self): return secgroup_name, secgroup.id def delete_image(self, image_id): + if image_id is None: + logging.info('No image to remove') + return False logging.info('Removing image %s' % image_id) - if(image_id is not None): - try: - logging.info('Removing image %s' % image_id) - self.glance.images.delete(image_id) - return True - except: - logging.info('Removing image failed.') - return False + try: + self.conn.image.delete_image(image_id) + except Exception as error: + logging.info("Removing image %s failed: %s" % (image_id, error)) + return False + return True def download_image(self, artifact_id): - """Downloads image from Glance""" - logging.info('Downloading image...') + """Downloads image from Glance + + Straight through the API on the session we already hold. Shelling out + to the glance CLI meant a second, independent authentication from the + OS_* environment, and that CLI is deprecated upstream. + """ timestr = time.strftime("%Y%m%d") filename = self.image_name + '-' + timestr + '.qcow2' - cmd = ['glance', - 'image-download', - '--file', os.path.join(self.download_dir, filename), - artifact_id] - process = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) - with process.stdout: - helpers.log_subprocess_output(process.stdout) - exitcode = process.wait() - # Not so pretty but will do for now - if exitcode == 0: - logging.info('Download successful, deleting from Glance...') - self.delete_image(artifact_id) - return exitcode + target = os.path.join(self.download_dir, filename) + logging.info("Downloading image to %s..." % target) + try: + with open(target, 'wb') as image_file: + self.conn.image.download_image(artifact_id, output=image_file) + except Exception as error: + logging.info("Failed to download image %s: %s" % (artifact_id, error)) + if os.path.exists(target): + os.remove(target) # don't leave a half-written image behind + return 1 + logging.info('Download successful, deleting from Glance...') + self.delete_image(artifact_id) + return 0 def find_network_id(self, name): # networks() rather than find_network(), which raises on a duplicate diff --git a/image_builder/helpers.py b/image_builder/helpers.py index eae8569..98f8c2c 100644 --- a/image_builder/helpers.py +++ b/image_builder/helpers.py @@ -20,8 +20,22 @@ def make_tmp_dir(): @staticmethod def log_subprocess_output(pipe): + # Decoded, not repr'd: this is the build log the user watches, and + # every line of it used to arrive looking like b'...\n' for line in iter(pipe.readline, b''): - logging.info('%r', line) + logging.info(line.decode('utf-8', errors='replace').rstrip()) + + @staticmethod + def valid_digest(digest): + """Whether hashlib knows this digest name + + Used to reject a bad -t before spending a download on it. + """ + try: + hashlib.new(digest.lower()) + except (ValueError, TypeError): + return False + return True @staticmethod def checksum_file(file_path, digest='sha256', chunk_size=65536): @@ -29,16 +43,15 @@ def checksum_file(file_path, digest='sha256', chunk_size=65536): particularly large files. Also ensures memory usage is kept to a minimum. Testing shows default is a pretty good size.""" assert isinstance(chunk_size, int) and chunk_size > 0 - if digest == 'sha512': - digest = hashlib.sha512() - if digest == 'sha256': - digest = hashlib.sha256() - elif digest == 'md5': - digest = hashlib.md5() + # hashlib.new() takes any algorithm it supports and raises a clear + # ValueError naming a bad one. The hand-written if/elif ladder this + # replaces silently left unknown names as a string and then died with + # AttributeError on the first update() + hasher = hashlib.new(digest.lower()) #pylint: disable=invalid-name with open(file_path, 'rb') as f: for block in iter(lambda: f.read(chunk_size), b''): - digest.update(block) - checksum = digest.hexdigest() + hasher.update(block) + checksum = hasher.hexdigest() logging.debug("hexdigest of %s is %s" % (file_path, checksum)) return checksum diff --git a/image_builder/imagebuilder.py b/image_builder/imagebuilder.py index 44408c9..19ff88c 100644 --- a/image_builder/imagebuilder.py +++ b/image_builder/imagebuilder.py @@ -65,7 +65,7 @@ def main(): print("""Missing environment variable(s): %s Please run: source -and try again.""" % missing.args[0]) +and try again.""" % missing.args[0], file=sys.stderr) sys.exit(1) ib_session = imagebuilder.auth(rc) @@ -79,7 +79,7 @@ def main(): try: template_dir = config.get('main', 'template_dir') except: - print("Failed to read template_dir from config") + print("Failed to read template_dir from config", file=sys.stderr) sys.exit(1) if "IB_DOWNLOAD_DIR" in os.environ: @@ -88,7 +88,7 @@ def main(): try: download_dir = config.get('main', 'download_dir') except: - print("Failed to read download_dir from config") + print("Failed to read download_dir from config", file=sys.stderr) sys.exit(1) if commands.build_args: @@ -186,8 +186,8 @@ def main(): checksum_url = commands.bootstrap_args.checksum_url checksum_digest = commands.bootstrap_args.checksum_digest disk_format = commands.bootstrap_args.disk_format - min_disk = int(commands.bootstrap_args.min_disk) - min_ram = int(commands.bootstrap_args.min_ram) + min_disk = commands.bootstrap_args.min_disk + min_ram = commands.bootstrap_args.min_ram if commands.bootstrap_args.verbose: logging.basicConfig(format="%(message)s", level=logging.INFO) diff --git a/image_builder/parsecommands.py b/image_builder/parsecommands.py index 0677992..bde0029 100644 --- a/image_builder/parsecommands.py +++ b/image_builder/parsecommands.py @@ -1,124 +1,107 @@ import argparse -import sys class Commands(object): - def __init__(self): + def __init__(self, argv=None): parser = argparse.ArgumentParser( - description='Usage', - usage='''imagebuilder [] + prog='imagebuilder', + description='Build fully provisioned images in NREC') + # Subparsers rather than dispatching on getattr(self, argv[1]): that + # made every attribute on this object reachable from the command line, + # and left the list of commands as prose in the usage string with + # nothing keeping it honest + subcommands = parser.add_subparsers(dest='command', required=True, + metavar='') - build Builds an image - bootstrap Downloads a cloud-ready image from a URL and uploads to Glance -''') - parser.add_argument('command', help='Subcommand to run') - args = parser.parse_args(sys.argv[1:2]) - if not hasattr(self, args.command): - print('Unrecognized command') - parser.print_help() - exit(1) - self.build_args = False - self.bootstrap_args = False - getattr(self, args.command)() + build = subcommands.add_parser( + 'build', + help='Builds an image', + description='Build an image') + build.add_argument('-a', '--availability-zone', + help='Availability zone, i.e. bgo-default-1, osl-default-1', + required=True) + build.add_argument('-d', '--download', + help='Download image after build', + action='store_true', + default=False) + build.add_argument('-f', '--flavor', + help='Flavor to use when building (default: m1.small)', + default='m1.small') + build.add_argument('-n', '--name', + help='Name of the image', + required=True) + build.add_argument('-N', '--network-name', + help='Name of the network we want to use', + default='Dualstack') + build.add_argument('-p', '--provision-script', + help='Path to your provision script', + default='/bin/true') + build.add_argument('-s', '--source-image', + help='Name or id of the source image we build from', + required=True) + build.add_argument('-u', '--ssh-username', + help='SSH username as set up by cloud-init (usually named after distro or OS, i.e. centos, ubuntu)', + required=True) + build.add_argument('-v', '--verbose', + help='Be verbose (default is no output)', + action='store_true', + default=False) + build.add_argument('-x', '--purge-source', + help='Purge source image you are building from', + action='store_true', + default=False) + build.add_argument('--debug', + help='Debug mode', + action='store_true', + default=False) - def build(self): - parser = argparse.ArgumentParser(description='Build an image') - parser.add_argument('-a', '--availability-zone', - help='Availability zone, i.e. bgo-default-1, osl-default-1', - default=False, - required=True) - parser.add_argument('-d', '--download', - help='Download image after build', - action='store_true', - default=False) - parser.add_argument('-f', '--flavor', - help='Flavor to use when building (default: m1.small)', - default='m1.small', - required=False) - parser.add_argument('-n', '--name', - help='Name of the image', - default=False, - required=True) - parser.add_argument('-N', '--network-name', - help='Name of the network we want to use', - default='Dualstack', - required=False) - parser.add_argument('-p', '--provision-script', - help='Path to your provision script', - default='/bin/true') - parser.add_argument('-s', '--source-image', - help='Name or id of the source image we build from', - default=False, - required=True) - parser.add_argument('-u', '--ssh-username', - help='SSH username as set up by cloud-init (usually named after distro or OS, i.e. centos, ubuntu)', - default=False, - required=True), - parser.add_argument('-v', '--verbose', - help='Be verbose (default is no output)', - action='store_true', - default=False) - parser.add_argument('-x', '--purge-source', - help='Purge source image you are building from', - action='store_true', - default=False) - parser.add_argument('--debug', - help='Debug mode', - action='store_true', - default=False) - self.build_args = parser.parse_args(sys.argv[2:]) - return self.build_args - - def bootstrap(self): - parser = argparse.ArgumentParser( + bootstrap = subcommands.add_parser( + 'bootstrap', + help='Downloads a cloud-ready image from a URL and uploads to Glance', description='Downloads a cloud-ready image from a URL and uploads to glance') - parser.add_argument('-a', '--availability-zone', - help='Availability zone, i.e. bgo-default-1, osl-default-1', - default=False, - required=True) - parser.add_argument('-u', '--url', - help='URL to upstream image', - default=False, - required=True) - parser.add_argument('-c', '--checksum-url', - help='URL to checksum file', - default=None, - required=False) - parser.add_argument('-t', '--checksum-digest', - help='Checksum digest (defaults to sha256)', - default='sha256', - required=False), - parser.add_argument('-n', '--name', - help='Name of the image', - default=False, - required=True), - parser.add_argument('-r', '--min-ram', - help='Minimum amount of ram in MB', - default=False, - required=True) - parser.add_argument('-d', '--min-disk', - help='Minimum amount of disk in GB', - default=False, - required=True) - parser.add_argument('-f', '--disk-format', - help='Format of the disk', - default=False, - required=True) - parser.add_argument('-s', '--no-scsi-mode', - help='Do not create image with scsi disk properties', - action='store_true', - default=False) - parser.add_argument('-e', '--efi', - help='Boot image with uefi boot firmware', - action='store_true', - default=False) - parser.add_argument('-v', '--verbose', - help='Be verbose', - action='store_true', - default=False) - parser.add_argument('--debug', - help='Debug mode', - action='store_true', - default=False) - self.bootstrap_args = parser.parse_args(sys.argv[2:]) - return self.bootstrap_args + bootstrap.add_argument('-a', '--availability-zone', + help='Availability zone, i.e. bgo-default-1, osl-default-1', + required=True) + bootstrap.add_argument('-u', '--url', + help='URL to upstream image', + required=True) + bootstrap.add_argument('-c', '--checksum-url', + help='URL to checksum file', + default=None) + bootstrap.add_argument('-t', '--checksum-digest', + help='Checksum digest (defaults to sha256)', + default='sha256') + bootstrap.add_argument('-n', '--name', + help='Name of the image', + required=True) + bootstrap.add_argument('-r', '--min-ram', + help='Minimum amount of ram in MB', + type=int, + required=True) + bootstrap.add_argument('-d', '--min-disk', + help='Minimum amount of disk in GB', + type=int, + required=True) + bootstrap.add_argument('-f', '--disk-format', + help='Format of the disk', + required=True) + bootstrap.add_argument('-s', '--no-scsi-mode', + help='Do not create image with scsi disk properties', + action='store_true', + default=False) + bootstrap.add_argument('-e', '--efi', + help='Boot image with uefi boot firmware', + action='store_true', + default=False) + bootstrap.add_argument('-v', '--verbose', + help='Be verbose', + action='store_true', + default=False) + bootstrap.add_argument('--debug', + help='Debug mode', + action='store_true', + default=False) + + args = parser.parse_args(argv) + self.build_args = args if args.command == 'build' else False + self.bootstrap_args = args if args.command == 'bootstrap' else False diff --git a/requirements.txt b/requirements.txt index f117fdf..3bc1888 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,24 +1,19 @@ -# Pinned for Python 3.9, the stock interpreter on EL9. Three packages are -# capped by that floor: keystoneauth1 5.12 requires 3.10 and 5.15 requires -# 3.11, python-glanceclient 4.9 requires 3.10 and 4.13 requires 3.11, +# Pinned for Python 3.9, the stock interpreter on EL9. Two packages are capped +# by that floor: keystoneauth1 5.12 requires 3.10 and 5.15 requires 3.11, # openstacksdk 4.6 requires 3.10 and 4.14 requires 3.11. # # This is the combination OpenStack itself sanctions for 3.9: the 2025.2 -# upper-constraints pin openstacksdk 4.5.0 and python-glanceclient 4.8.0 for -# python_version=='3.9', and everything here meets or exceeds the 2025.1 -# constraints. +# upper-constraints pin openstacksdk 4.5.0 for python_version=='3.9', and +# everything here meets or exceeds the 2025.1 constraints. keystoneauth1==5.11.1 -python-novaclient==18.13.1 -python-glanceclient==4.8.0 openstacksdk==4.5.0 cryptography==50.0.1 pbr==7.0.3 -# On Python 3.11 or newer, comment out the three capped pins above and -# uncomment these. python-novaclient, cryptography and pbr are already current -# and stay as they are. See DEVELOPMENT.md for installing a newer interpreter -# on EL9 without waiting for EL10. +# On Python 3.11 or newer, comment out the two capped pins above and uncomment +# these. cryptography and pbr are already current and stay as they are. See +# DEVELOPMENT.md for installing a newer interpreter on EL9 without waiting for +# EL10. # # keystoneauth1==5.17.0 -# python-glanceclient==4.13.0 # openstacksdk==4.20.0 From 2158aa24bbe64c10d6997b687ce1ce0124212afb Mon Sep 17 00:00:00 2001 From: Trond Hasle Amundsen Date: Sun, 13 Sep 2026 21:52:23 +0200 Subject: [PATCH 12/22] refactor: address the low findings, and add the test suite Findings 09 and 10 turned out to be fixed already as a side effect of the medium batch: delete_image() gained explicit returns and lost its duplicate log line when it moved to conn.image, and switching the upload to create_image() removed both the unclosed file handle and the over-wide except BaseException. Logging. Every module now has its own logger and passes arguments lazily rather than formatting into the message first. Levels carry meaning: progress is info, trouble that does not stop the run is warning, and anything that ends the run is error. Configuration happens once, in one place, instead of being repeated per subcommand, and --debug now wins when both it and --verbose are given. The default level is WARNING rather than nothing at all, so an ordinary run stays quiet but a failure says why instead of just exiting non-zero. Entry points. Both checked-in scripts use env python3, so an activated virtualenv is actually honoured; the hardcoded /usr/bin/python3 ignored it, which mattered increasingly now that DEVELOPMENT.md recommends a python3.12 venv. The shebang on imagebuilder.py is gone, since a module is not a script, and __main__.py no longer runs a build merely on import. Dead code. Config.show_config() was an empty stub. BootstrapFunctions took an availability zone it never read; the -a flag stays on the bootstrap command because removing a required argument is a contract change rather than a cleanup, but it is now visibly unused. Packaging moves to pyproject.toml, so setup.py is gone and the deprecated python setup.py develop becomes pip install -e ., which now also pulls the two libraries actually imported. Tests. 87 of them, covering what the audit went through: credentials, the keypair and security group lifecycle, cleanup from a finally block, manifest handling, image download and its failure path, both checksum file layouts, digests, timeouts, the subcommand parser and stream separation. test_main_flow drives main() with a stand-in build to pin down that an interrupt or an exception still cleans up. They use fake clients and a fake HTTP layer, so they need no credentials and reach no network. CI runs them, with pytest pinned to 8.4.2, the last release supporting Python 3.9. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A4Y4JQ39uQMSZcBYoTE84y --- .github/workflows/python-package.yml | 11 +- CLAUDE.md | 13 +- DEVELOPMENT.md | 17 ++- example/imagebuilder | 2 +- image_builder/__main__.py | 4 +- image_builder/bootstrap.py | 36 ++--- image_builder/build.py | 50 ++++--- image_builder/config.py | 4 - image_builder/helpers.py | 10 +- image_builder/imagebuilder.py | 71 ++++----- imagebuilder | 2 +- pyproject.toml | 32 +++- setup.py | 20 --- tests/conftest.py | 142 ++++++++++++++++++ tests/test_bootstrap.py | 187 +++++++++++++++++++++++ tests/test_build.py | 214 +++++++++++++++++++++++++++ tests/test_cli.py | 106 +++++++++++++ tests/test_credentials.py | 52 +++++++ tests/test_main_flow.py | 128 ++++++++++++++++ 19 files changed, 986 insertions(+), 115 deletions(-) delete mode 100644 setup.py create mode 100644 tests/conftest.py create mode 100644 tests/test_bootstrap.py create mode 100644 tests/test_build.py create mode 100644 tests/test_cli.py create mode 100644 tests/test_credentials.py create mode 100644 tests/test_main_flow.py diff --git a/.github/workflows/python-package.yml b/.github/workflows/python-package.yml index 3ba69c3..15b6dd9 100644 --- a/.github/workflows/python-package.yml +++ b/.github/workflows/python-package.yml @@ -1,4 +1,4 @@ -# This workflow will install Python dependencies and lint with a variety of Python versions +# This workflow will install Python dependencies, lint and run tests with a variety of Python versions # For more information see: https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-python name: Python package @@ -34,13 +34,16 @@ jobs: - name: Install dependencies run: | python -m pip install --upgrade pip - # ruff is pinned: 0.16.0 widened the default rule set, which flags a - # lot of pre-existing style in this repo. Bump deliberately. - pip install 'ruff==0.15.22' + # Both pinned: ruff 0.16.0 widened the default rule set, and pytest + # 9 requires Python 3.10. Bump deliberately. + pip install 'ruff==0.15.22' 'pytest==8.4.2' if [ -f requirements.txt ]; then pip install -r requirements.txt; fi + pip install -e . - name: Lint with ruff run: | # stop the build if there are Python syntax errors or undefined names ruff check --output-format=github --select=E9,F63,F7,F82 --target-version=py39 . # default set of ruff rules with GitHub Annotations ruff check --output-format=github --target-version=py39 . + - name: Run tests + run: pytest -q diff --git a/CLAUDE.md b/CLAUDE.md index 45eaed9..cff7244 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -18,16 +18,25 @@ This fork's `origin` is `trondham/imagebuilder`; `upstream` is `norcams/imagebui ruff check --select=E9,F63,F7,F82 --target-version=py39 . ruff check --target-version=py39 . +# Tests — CI runs these too; ruff is pinned to 0.15.22 and pytest to 8.4.2 +pytest -q +pytest tests/test_main_flow.py -q # a single file +pytest -k "cleanup" -q # a single case + # Dev install (see DEVELOPMENT.md) python3 -m venv . && source bin/activate pip install -r requirements.txt -python setup.py develop +pip install -e . # Run without installing ./imagebuilder # wrapper around image_builder.imagebuilder:main ``` -There is no test suite. CI installs `pytest` but never invokes it; do not claim tests pass. +Tests live in `tests/` and use stand-in OpenStack clients (`tests/conftest.py`) plus a fake HTTP +layer, so they need no credentials and touch no network. `test_main_flow.py` drives `main()` itself +with a fake `BuildFunctions` — that is where the cleanup-on-failure guarantees are pinned down. + +Packaging metadata and the console script live in `pyproject.toml`; there is no `setup.py`. Running any command requires OpenStack credentials in the environment (`source keystone_rc.sh`) — `get_openstack_rc()` reads `OS_USERNAME`, `OS_PROJECT_NAME`, `OS_PASSWORD`, `OS_AUTH_URL`, diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index d2ed7d5..d325be1 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -9,11 +9,24 @@ cd imagebuilder virtualenv . -p /usr/bin/python3 source bin/activate pip install --upgrade pip -pip install --upgrade setuptools -python setup.py develop pip install -r requirements.txt +pip install -e . ``` +`pip install -e .` replaces the old `python setup.py develop`, which setuptools +has deprecated. Package metadata and the console script live in +`pyproject.toml`; there is no `setup.py` any more. + +## Tests + +``` bash +pip install pytest +pytest +``` + +They use stand-in OpenStack clients and a fake HTTP layer, so nothing reaches a +cloud or the network and no credentials are needed. CI runs them on every push. + ## Python version `requirements.txt` pins for Python 3.9, the stock interpreter on EL9. Red Hat diff --git a/example/imagebuilder b/example/imagebuilder index 60aa80a..0766a13 100755 --- a/example/imagebuilder +++ b/example/imagebuilder @@ -1,4 +1,4 @@ -#!/usr/bin/env python +#!/usr/bin/env python3 from image_builder.imagebuilder import main diff --git a/image_builder/__main__.py b/image_builder/__main__.py index a85f7d4..9c5f01d 100644 --- a/image_builder/__main__.py +++ b/image_builder/__main__.py @@ -1,2 +1,4 @@ from .imagebuilder import main -main() + +if __name__ == '__main__': + main() diff --git a/image_builder/bootstrap.py b/image_builder/bootstrap.py index e60febd..b7aab59 100644 --- a/image_builder/bootstrap.py +++ b/image_builder/bootstrap.py @@ -4,6 +4,8 @@ from openstack.connection import Connection from .helpers import Helpers as helpers +log = logging.getLogger(__name__) + # Blocking socket operations give up after this long. This is a per-read # timeout, not a limit on the whole transfer, so a large image is fine as long # as bytes keep arriving @@ -15,10 +17,8 @@ class BootstrapFunctions(object): def __init__(self, session, - region, - avail_zone): + region): self.session = session - self.avail_zone = avail_zone self.tmp_dir = helpers.make_tmp_dir() self.conn = Connection(session=session, region_name=region) @@ -56,7 +56,7 @@ def download_and_check(self, url, checksum_url=None, checksum_dig='sha256'): # Checked up front so a typo in -t fails now rather than after the # download has finished if checksum_url and not helpers.valid_digest(checksum_dig): - logging.info("Unknown checksum digest: %s" % checksum_dig) + log.error("Unknown checksum digest: %s", checksum_dig) return None req = urllib.request.Request( @@ -79,7 +79,7 @@ def download_and_check(self, url, checksum_url=None, checksum_dig='sha256'): # header it is not obliged to send size = os.path.getsize(file_path) if size < MIN_IMAGE_BYTES: - logging.info("File is too small (%d bytes): %s" % (size, url)) + log.error("File is too small (%d bytes): %s", size, url) os.remove(file_path) return None @@ -93,12 +93,12 @@ def download_and_check(self, url, checksum_url=None, checksum_dig='sha256'): 'User-Agent': user_agent } ) - logging.info("Verifying checksum of %s..." % file_path) + log.info("Verifying checksum of %s...", file_path) response = urllib.request.urlopen(req, timeout=SOCKET_TIMEOUT) checksum_text = response.read().decode('utf-8', errors='replace') response.close() - logging.debug(checksum_text) - logging.debug("Checksum type is %s" % checksum_dig) + log.debug(checksum_text) + log.debug("Checksum type is %s", checksum_dig) actual = helpers.checksum_file(file_path, checksum_dig) expected = self.find_expected_checksum(checksum_text, file_name) @@ -107,20 +107,20 @@ def download_and_check(self, url, checksum_url=None, checksum_dig='sha256'): # No entry naming this file. Fall back to the old behaviour of # looking for the hash anywhere in the file, which is weaker but # keeps working with checksum files that name things differently - logging.info("No entry for %s in %s, falling back to matching the " - "hash anywhere in the file" % (file_name, checksum_url)) + log.warning("No entry for %s in %s, falling back to matching the " + "hash anywhere in the file", file_name, checksum_url) if actual in checksum_text.lower(): - logging.info("Checksum ok: %s" % actual) + log.info("Checksum ok: %s", actual) return file_path - logging.info("Checksum not found: %s" % actual) + log.error("Checksum not found: %s", actual) return None if actual == expected: - logging.info("Checksum ok: %s" % actual) + log.info("Checksum ok: %s", actual) return file_path - logging.info("Checksum mismatch for %s: expected %s, got %s" - % (file_name, expected, actual)) + log.error("Checksum mismatch for %s: expected %s, got %s", + file_name, expected, actual) return None def create_glance_image(self, image_file, name, disk_format, min_disk, @@ -141,8 +141,8 @@ def create_glance_image(self, image_file, name, disk_format, min_disk, disable_vendor_agent=False, **properties) except Exception as error: - logging.info("Failed to upload %s: %s" % (image_file, error)) + log.error("Failed to upload %s: %s", image_file, error) return None - logging.info("Successfully uploaded %s as image %s" % (image_file, name)) - logging.debug(image) + log.info("Successfully uploaded %s as image %s", image_file, name) + log.debug(image) return image.id diff --git a/image_builder/build.py b/image_builder/build.py index 83177cf..825828e 100644 --- a/image_builder/build.py +++ b/image_builder/build.py @@ -7,6 +7,8 @@ from openstack.connection import Connection from .helpers import Helpers as helpers +log = logging.getLogger(__name__) + class BuildFunctions(object): def __init__(self, session, @@ -42,17 +44,17 @@ def cleanup(self, secgroup_id, keypair_id): mask whatever actually went wrong. """ if secgroup_id: - logging.info('Removing temporary security group...') + log.info('Removing temporary security group...') try: self.conn.network.delete_security_group(secgroup_id) except Exception as error: - logging.info("Failed to remove security group %s: %s" % (secgroup_id, error)) + log.warning("Failed to remove security group %s: %s", secgroup_id, error) if keypair_id: - logging.info('Removing temporary keypair...') + log.info('Removing temporary keypair...') try: self.conn.compute.delete_keypair(keypair_id) except Exception as error: - logging.info("Failed to remove keypair %s: %s" % (keypair_id, error)) + log.warning("Failed to remove keypair %s: %s", keypair_id, error) def create_keypairs(self): """Creates a temporary keypair""" @@ -62,10 +64,10 @@ def create_keypairs(self): # No shell: there is nothing here a shell is needed for, and letting # PATH find ssh-keygen beats hardcoding where it lives cmd = ['ssh-keygen', '-b', '521', '-t', 'ecdsa', '-N', '', '-f', keypath] - logging.debug(cmd) + log.debug(cmd) out = subprocess.call(cmd) # generate temporary ssh key if out: # something went wrong with the key generation - logging.info("Failed to generate SSH key, ssh-keygen exited %s" % out) + log.error("Failed to generate SSH key, ssh-keygen exited %s", out) return None, None # read public key string and store into Openstack with open(keypath + ".pub", "r") as pubfile: @@ -80,7 +82,7 @@ def create_security_group(self): secgroup = self.conn.network.create_security_group( name=secgroup_name, description='Temporary security group for image building') - logging.info('Creating rule allowing SSH traffic...') + log.info('Creating rule allowing SSH traffic...') for ethertype in ('IPv4', 'IPv6'): self.conn.network.create_security_group_rule( security_group_id=secgroup.id, @@ -93,13 +95,13 @@ def create_security_group(self): def delete_image(self, image_id): if image_id is None: - logging.info('No image to remove') + log.warning('No image to remove') return False - logging.info('Removing image %s' % image_id) + log.info("Removing image %s", image_id) try: self.conn.image.delete_image(image_id) except Exception as error: - logging.info("Removing image %s failed: %s" % (image_id, error)) + log.error("Removing image %s failed: %s", image_id, error) return False return True @@ -113,16 +115,16 @@ def download_image(self, artifact_id): timestr = time.strftime("%Y%m%d") filename = self.image_name + '-' + timestr + '.qcow2' target = os.path.join(self.download_dir, filename) - logging.info("Downloading image to %s..." % target) + log.info("Downloading image to %s...", target) try: with open(target, 'wb') as image_file: self.conn.image.download_image(artifact_id, output=image_file) except Exception as error: - logging.info("Failed to download image %s: %s" % (artifact_id, error)) + log.error("Failed to download image %s: %s", artifact_id, error) if os.path.exists(target): os.remove(target) # don't leave a half-written image behind return 1 - logging.info('Download successful, deleting from Glance...') + log.info('Download successful, deleting from Glance...') self.delete_image(artifact_id) return 0 @@ -132,10 +134,10 @@ def find_network_id(self, name): network = next(self.conn.network.networks(name=name), None) if network: network_id = network.id - logging.info("Found network %s with id %s" % (name, network_id)) + log.info("Found network %s with id %s", name, network_id) else: network_id = False - logging.info("Cannot find network %s" % name) + log.error("Cannot find network %s", name) return network_id def find_template(self): @@ -149,13 +151,13 @@ def find_template(self): template_path = os.path.join(self.template_dir, name) if os.path.isfile(template_path): if name == 'template': - logging.info("Using legacy JSON template %s" % template_path) - logging.info("Packer no longer bundles the openstack builder. " - "Install it manually with 'packer plugins install " - "github.com/hashicorp/openstack', or migrate to " - "template.pkr.hcl") + log.warning("Using legacy JSON template %s", template_path) + log.warning("Packer no longer bundles the openstack builder. " + "Install it manually with 'packer plugins install " + "github.com/hashicorp/openstack', or migrate to " + "template.pkr.hcl") return template_path - logging.info("No Packer template found in %s" % self.template_dir) + log.error("No Packer template found in %s", self.template_dir) return None def parse_manifest(self): @@ -171,7 +173,7 @@ def parse_manifest(self): manifest = json.load(data_file) return manifest['builds'][0]['artifact_id'] except (OSError, ValueError, KeyError, IndexError) as error: - logging.info("Failed to read the image id from %s: %s" % (manifest_path, error)) + log.error("Failed to read the image id from %s: %s", manifest_path, error) return None def run_packer(self, template_path, secgroup_name, key_name, network_id): @@ -201,7 +203,7 @@ def run_packer(self, template_path, secgroup_name, key_name, network_id): '-var', provision_script_var, '-var', manifest_path_var, template_path] - logging.debug(cmd) + log.debug(cmd) process = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) with process.stdout: helpers.log_subprocess_output(process.stdout) @@ -219,7 +221,7 @@ def run_packer_init(self, template_path): if not template_path.endswith('.pkr.hcl'): return 0 cmd = ['packer', 'init', template_path] - logging.debug(cmd) + log.debug(cmd) process = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) with process.stdout: helpers.log_subprocess_output(process.stdout) diff --git a/image_builder/config.py b/image_builder/config.py index 052127a..e879d9f 100644 --- a/image_builder/config.py +++ b/image_builder/config.py @@ -14,10 +14,6 @@ def __init__(self, configfile_path=None): self.config_file_list.append(os.path.join(path, "config")) self.config = self.__read_config() - def show_config(self): - # Not implemented yet - pass - def __read_config(self): config = configparser.ConfigParser() config.read(self.config_file_list) diff --git a/image_builder/helpers.py b/image_builder/helpers.py index 98f8c2c..b87f489 100644 --- a/image_builder/helpers.py +++ b/image_builder/helpers.py @@ -3,18 +3,20 @@ import shutil import tempfile +log = logging.getLogger(__name__) + class Helpers(object): @staticmethod def clean_tmp_files(tmp_dir): # Also called from a finally block, so failing to remove a scratch # directory must not be what the user ends up seeing - logging.info('Removing temporary directory with content...') + log.info('Removing temporary directory with content...') shutil.rmtree(tmp_dir, ignore_errors=True) @staticmethod def make_tmp_dir(): - logging.info('Creating a directory for temporary files...') + log.info('Creating a directory for temporary files...') tmp_dir = tempfile.mkdtemp(prefix='imagebuilder-') return tmp_dir @@ -23,7 +25,7 @@ def log_subprocess_output(pipe): # Decoded, not repr'd: this is the build log the user watches, and # every line of it used to arrive looking like b'...\n' for line in iter(pipe.readline, b''): - logging.info(line.decode('utf-8', errors='replace').rstrip()) + log.info(line.decode('utf-8', errors='replace').rstrip()) @staticmethod def valid_digest(digest): @@ -53,5 +55,5 @@ def checksum_file(file_path, digest='sha256', chunk_size=65536): for block in iter(lambda: f.read(chunk_size), b''): hasher.update(block) checksum = hasher.hexdigest() - logging.debug("hexdigest of %s is %s" % (file_path, checksum)) + log.debug("hexdigest of %s is %s", file_path, checksum) return checksum diff --git a/image_builder/imagebuilder.py b/image_builder/imagebuilder.py index 19ff88c..d9617bf 100644 --- a/image_builder/imagebuilder.py +++ b/image_builder/imagebuilder.py @@ -1,5 +1,3 @@ -#!/usr/bin/python3 - import logging import os import sys @@ -11,6 +9,8 @@ from .config import Config from .helpers import Helpers as helpers +log = logging.getLogger(__name__) + class ImageBuilder(object): @staticmethod def auth(rc): @@ -55,8 +55,26 @@ def get_openstack_rc(cls): env_var['cacert'] = os.environ.get('OS_CACERT') return env_var +def configure_logging(args): + """Sets up logging once for whichever subcommand was chosen + + Default is WARNING rather than silence: progress stays hidden without -v, + but a failure now says so instead of the command simply exiting non-zero. + --debug wins when both flags are given, which is the way round people + expect. + """ + if args.debug: + logging.basicConfig(format="%(asctime)s %(levelname)s %(name)s %(message)s", + level=logging.DEBUG) + elif args.verbose: + logging.basicConfig(format="%(message)s", level=logging.INFO) + else: + logging.basicConfig(format="%(message)s", level=logging.WARNING) + + def main(): commands = Commands() + configure_logging(commands.build_args or commands.bootstrap_args) imagebuilder = ImageBuilder() try: @@ -100,11 +118,6 @@ def main(): provision_script = commands.build_args.provision_script network_name = commands.build_args.network_name - if commands.build_args.verbose: - logging.basicConfig(format="%(message)s", level=logging.INFO) - elif commands.build_args.debug: - logging.basicConfig(level=logging.DEBUG) - build = BuildFunctions(ib_session, region, image_name, @@ -130,13 +143,13 @@ def main(): helpers.clean_tmp_files(build.tmp_dir) sys.exit(1) - logging.info('Installing Packer plugins...') + log.info('Installing Packer plugins...') if build.run_packer_init(template_path) != 0: - logging.info('Failed to install Packer plugins') + log.error('Failed to install Packer plugins') helpers.clean_tmp_files(build.tmp_dir) sys.exit(1) - logging.info('Creating Packer security group...') + log.info('Creating Packer security group...') secgroup_name, secgroup_id = build.create_security_group() keypair_id = None @@ -147,33 +160,33 @@ def main(): # has to still remove the security group and the keypair rather than # leave them behind in the project try: - logging.info('Creating Packer keypair...') + log.info('Creating Packer keypair...') key_name, keypair_id = build.create_keypairs() if key_name is None: sys.exit(1) - logging.info('Running Packer...') + log.info('Running Packer...') exitcode = build.run_packer(template_path, secgroup_name, key_name, network_id) if exitcode == 0: artifact_id = build.parse_manifest() if artifact_id is None: exitcode = 1 else: - logging.info("Successfully created image id %s" % artifact_id) + log.info("Successfully created image id %s", artifact_id) if commands.build_args.download: exitcode = build.download_image(artifact_id) else: - logging.info('Build failed') + log.error('Build failed') exitcode = 1 if commands.build_args.purge_source: if build.delete_image(source_image): - logging.info('Successfully deleted source image') + log.info('Successfully deleted source image') else: - logging.info('Failed to delete source image') + log.error('Failed to delete source image') exitcode = 1 finally: - logging.info('Cleaning up...') + log.info('Cleaning up...') build.cleanup(secgroup_id, keypair_id) helpers.clean_tmp_files(build.tmp_dir) @@ -181,7 +194,6 @@ def main(): if commands.bootstrap_args: image_name = commands.bootstrap_args.name - avail_zone = commands.bootstrap_args.availability_zone url = commands.bootstrap_args.url checksum_url = commands.bootstrap_args.checksum_url checksum_digest = commands.bootstrap_args.checksum_digest @@ -189,11 +201,6 @@ def main(): min_disk = commands.bootstrap_args.min_disk min_ram = commands.bootstrap_args.min_ram - if commands.bootstrap_args.verbose: - logging.basicConfig(format="%(message)s", level=logging.INFO) - elif commands.bootstrap_args.debug: - logging.basicConfig(level=logging.DEBUG) - properties = {} if not commands.bootstrap_args.no_scsi_mode: @@ -206,14 +213,12 @@ def main(): properties['hw_rng_model'] = 'virtio' - bootstrap = BootstrapFunctions(ib_session, - region, - avail_zone) - logging.info('Downloading image...') + bootstrap = BootstrapFunctions(ib_session, region) + log.info('Downloading image...') image_file = bootstrap.download_and_check(url, checksum_url, checksum_digest) if image_file: - logging.info('Uploading image to Glance...') + log.info('Uploading image to Glance...') image_id = bootstrap.create_glance_image(image_file, image_name, disk_format, @@ -221,20 +226,20 @@ def main(): min_ram, properties) else: - logging.info('Downloading failed.') - logging.info('Cleaning up...') + log.error('Downloading failed.') + log.info('Cleaning up...') helpers.clean_tmp_files(bootstrap.tmp_dir) sys.exit(1) if image_id: sys.stdout.write(image_id) else: - logging.info('Uploading failed.') - logging.info('Cleaning up...') + log.error('Uploading failed.') + log.info('Cleaning up...') helpers.clean_tmp_files(bootstrap.tmp_dir) sys.exit(1) - logging.info('Cleaning up...') + log.info('Cleaning up...') helpers.clean_tmp_files(bootstrap.tmp_dir) sys.exit(0) diff --git a/imagebuilder b/imagebuilder index b7fe288..0766a13 100755 --- a/imagebuilder +++ b/imagebuilder @@ -1,4 +1,4 @@ -#!/usr/bin/python3 +#!/usr/bin/env python3 from image_builder.imagebuilder import main diff --git a/pyproject.toml b/pyproject.toml index 3732536..11dc51b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,4 +1,34 @@ +[build-system] +requires = ["setuptools>=64"] +build-backend = "setuptools.build_meta" + +[project] +name = "imagebuilder" +description = "CLI for Packer-driven image building in UH IaaS" +readme = "README.md" +requires-python = ">=3.9" +license = { file = "LICENSE" } +authors = [{ name = "Anders Vaage", email = "anders@uib.no" }] +dynamic = ["version"] +# The two libraries actually imported. Exact pins live in requirements.txt, +# which is where the Python 3.9 ceiling is worked out +dependencies = [ + "keystoneauth1", + "openstacksdk", +] + +[project.urls] +Homepage = "https://github.com/norcams/imagebuilder" + +[project.scripts] +imagebuilder = "image_builder.imagebuilder:main" + +[tool.setuptools] +packages = ["image_builder"] + +[tool.setuptools.dynamic] +version = { attr = "image_builder.__version__" } + [tool.ruff] # Never enforce `E501` (line length violations). lint.ignore = ["E501", "E722"] - diff --git a/setup.py b/setup.py deleted file mode 100644 index c9ea5ac..0000000 --- a/setup.py +++ /dev/null @@ -1,20 +0,0 @@ -#!/usr/bin/python3 - -from setuptools import setup -import image_builder - - -setup( - name=image_builder.__productname__, - description=image_builder.__description__, - version=image_builder.__version__, - author=image_builder.__author__, - author_email=image_builder.__author_email__, - url=image_builder.__url__, - packages=['image_builder'], - entry_points={ - 'console_scripts': [ - 'imagebuilder = image_builder.imagebuilder:main' - ] - } - ) diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000..7731260 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,142 @@ +"""Shared fakes. + +Nothing here touches a network or a cloud: the OpenStack calls are replaced by +recorders that assert on what the code asked for. +""" +import os +import pathlib +import sys + +import pytest + +REPO_ROOT = pathlib.Path(__file__).resolve().parent.parent +sys.path.insert(0, str(REPO_ROOT)) + + +@pytest.fixture +def repo_root(): + return REPO_ROOT + + +class FakeNetwork: + def __init__(self, log): + self.log = log + self.networks_result = [] + + def create_security_group(self, **kw): + self.log.append(('sg_created', kw)) + return type('SG', (), {'id': 'sg-uuid'})() + + def create_security_group_rule(self, **kw): + self.log.append(('rule_created', kw)) + + def delete_security_group(self, sgid): + self.log.append(('sg_deleted', sgid)) + + def networks(self, **kw): + self.log.append(('networks_listed', kw)) + return iter(self.networks_result) + + +class FakeCompute: + def __init__(self, log): + self.log = log + + def create_keypair(self, **kw): + self.log.append(('kp_created', kw)) + return type('KP', (), {'id': kw.get('name')})() + + def delete_keypair(self, keypair): + self.log.append(('kp_deleted', keypair)) + + +class FakeImage: + def __init__(self, log, fail=False): + self.log = log + self.fail = fail + + def create_image(self, **kw): + self.log.append(('image_created', kw)) + if self.fail: + raise RuntimeError('glance is unhappy') + return type('IMG', (), {'id': 'new-image-uuid'})() + + def download_image(self, image, output=None, **kw): + self.log.append(('image_downloaded', image)) + if self.fail: + raise RuntimeError('glance is unhappy') + output.write(b'qcow2 data') + + def delete_image(self, image, **kw): + self.log.append(('image_deleted', image)) + + +class FakeConn: + """Stands in for an openstacksdk Connection.""" + + def __init__(self, fail_image=False): + self.log = [] + self.network = FakeNetwork(self.log) + self.compute = FakeCompute(self.log) + self.image = FakeImage(self.log, fail=fail_image) + + def kinds(self): + return [entry[0] for entry in self.log] + + +@pytest.fixture +def conn(): + return FakeConn() + + +@pytest.fixture +def build(conn, tmp_path): + """A BuildFunctions with the constructor bypassed, so no cloud is needed.""" + from image_builder.build import BuildFunctions + + b = BuildFunctions.__new__(BuildFunctions) + b.conn = conn + b.tmp_dir = str(tmp_path / 'tmp') + os.makedirs(b.tmp_dir, exist_ok=True) + b.download_dir = str(tmp_path / 'downloads') + os.makedirs(b.download_dir, exist_ok=True) + b.template_dir = str(tmp_path / 'templates') + os.makedirs(b.template_dir, exist_ok=True) + b.image_name = 'test-image' + b.avail_zone = 'bgo-default-1' + b.flavor = 'm1.small' + b.source_image = 'src-uuid' + b.ssh_user = 'almalinux' + b.provision_script = '/bin/true' + return b + + +@pytest.fixture +def bootstrap(tmp_path): + from image_builder.bootstrap import BootstrapFunctions + + b = BootstrapFunctions.__new__(BootstrapFunctions) + b.conn = FakeConn() + b.tmp_dir = str(tmp_path / 'boot') + os.makedirs(b.tmp_dir, exist_ok=True) + return b + + +@pytest.fixture +def clean_env(monkeypatch): + """An environment with every OS_* variable removed.""" + for key in list(os.environ): + if key.startswith('OS_') or key.startswith('IB_'): + monkeypatch.delenv(key, raising=False) + return monkeypatch + + +FULL_OPENRC = { + 'OS_USERNAME': 'user', + 'OS_PROJECT_NAME': 'project', + 'OS_PASSWORD': 'secret', + 'OS_AUTH_URL': 'https://api.example.com:5000/v3', + 'OS_USER_DOMAIN_NAME': 'dom', + 'OS_PROJECT_DOMAIN_NAME': 'dom', + 'OS_REGION_NAME': 'bgo', +} diff --git a/tests/test_bootstrap.py b/tests/test_bootstrap.py new file mode 100644 index 0000000..25edd78 --- /dev/null +++ b/tests/test_bootstrap.py @@ -0,0 +1,187 @@ +"""Downloading, verifying and uploading an upstream cloud image.""" +import hashlib +import io +import os +import urllib.request + +import pytest + +from image_builder import bootstrap as bootstrap_module +from image_builder.bootstrap import BootstrapFunctions +from image_builder.helpers import Helpers as helpers + +HASH = 'a' * 64 +IMAGE = b'x' * 5000 + + +# --------------------------------------------------------------- digests + +@pytest.mark.parametrize('name', ['sha256', 'sha512', 'md5', 'sha1', 'SHA256', + 'sha384', 'blake2b']) +def test_every_hashlib_digest_works(tmp_path, name): + """The old if/elif ladder died with AttributeError on anything else.""" + sample = tmp_path / 'sample' + sample.write_bytes(b'hello') + assert len(helpers.checksum_file(str(sample), name)) > 16 + + +def test_unknown_digest_raises_a_clear_error(tmp_path): + sample = tmp_path / 'sample' + sample.write_bytes(b'hello') + with pytest.raises(ValueError): + helpers.checksum_file(str(sample), 'notahash') + + +def test_valid_digest(): + assert helpers.valid_digest('sha1') + assert not helpers.valid_digest('notahash') + + +# ------------------------------------------------- checksum file parsing + +find = BootstrapFunctions.find_expected_checksum + + +@pytest.mark.parametrize('text,name,expected', [ + pytest.param('%s img.qcow2\n' % HASH, 'img.qcow2', HASH, id='coreutils'), + pytest.param('%s *img.img\n' % HASH, 'img.img', HASH, id='binary-star'), + pytest.param('SHA256 (img.qcow2) = %s\n' % HASH, 'img.qcow2', HASH, id='bsd'), + pytest.param('%s img.qcow2\n' % HASH.upper(), 'img.qcow2', HASH, id='uppercase'), + pytest.param('# c\n\n%s img.qcow2\n' % HASH, 'img.qcow2', HASH, id='comments'), + pytest.param('%s ./images/img.qcow2\n' % HASH, 'img.qcow2', HASH, id='path'), + pytest.param('%s other.qcow2\n' % HASH, 'img.qcow2', None, id='name-absent'), +]) +def test_checksum_layouts(text, name, expected): + assert find(text, name) == expected + + +def test_the_right_line_is_picked_among_several(): + text = '%s a.qcow2\n%s b.qcow2\n' % ('a' * 64, 'b' * 64) + assert find(text, 'b.qcow2') == 'b' * 64 + + +# --------------------------------------------------- download and verify + +class FakeResponse: + def __init__(self, payload): + self.stream = io.BytesIO(payload) + self.headers = {} # deliberately no content-length + + def read(self, n=None): + return self.stream.read(n) if n else self.stream.read() + + def close(self): + pass + + +@pytest.fixture +def fake_http(monkeypatch): + """Serves image bytes for the image URL and text for the checksum URL.""" + state = {'image': IMAGE, 'checksum': b'', 'timeouts': []} + + def fake_urlopen(req, timeout=None): + state['timeouts'].append(timeout) + url = req.full_url if hasattr(req, 'full_url') else str(req) + payload = state['checksum'] if 'sum' in url.lower() else state['image'] + return FakeResponse(payload) + + monkeypatch.setattr(urllib.request, 'urlopen', fake_urlopen) + return state + + +IMAGE_URL = 'https://example.com/img.qcow2' +SUM_URL = 'https://example.com/sha256sum.txt' + + +def test_a_real_image_is_accepted_without_a_content_length_header(bootstrap, fake_http): + assert bootstrap.download_and_check(IMAGE_URL) is not None + + +def test_an_error_page_is_rejected_on_its_actual_size(bootstrap, fake_http): + fake_http['image'] = b'404 not found' + assert bootstrap.download_and_check(IMAGE_URL) is None + + +def test_requests_carry_a_timeout(bootstrap, fake_http): + bootstrap.download_and_check(IMAGE_URL) + assert fake_http['timeouts'] + assert all(t == bootstrap_module.SOCKET_TIMEOUT for t in fake_http['timeouts']) + + +def test_matching_checksum_is_accepted(bootstrap, fake_http): + digest = hashlib.sha256(IMAGE).hexdigest() + fake_http['checksum'] = ('%s img.qcow2\n' % digest).encode() + assert bootstrap.download_and_check(IMAGE_URL, SUM_URL) is not None + + +def test_uppercase_checksum_file_is_accepted(bootstrap, fake_http): + digest = hashlib.sha256(IMAGE).hexdigest() + fake_http['checksum'] = ('%s img.qcow2\n' % digest.upper()).encode() + assert bootstrap.download_and_check(IMAGE_URL, SUM_URL) is not None + + +def test_mismatched_checksum_is_rejected(bootstrap, fake_http): + fake_http['checksum'] = ('%s img.qcow2\n' % ('f' * 64)).encode() + assert bootstrap.download_and_check(IMAGE_URL, SUM_URL) is None + + +def test_hash_listed_under_another_name_still_passes_via_fallback(bootstrap, fake_http): + """Kept so vendors whose checksum files name things differently still work.""" + digest = hashlib.sha256(IMAGE).hexdigest() + fake_http['checksum'] = ('%s somethingelse.qcow2\n' % digest).encode() + assert bootstrap.download_and_check(IMAGE_URL, SUM_URL) is not None + + +def test_checksum_file_with_nothing_usable_is_rejected(bootstrap, fake_http): + fake_http['checksum'] = b'garbage\n' + assert bootstrap.download_and_check(IMAGE_URL, SUM_URL) is None + + +def test_bad_digest_is_rejected_before_downloading(bootstrap, fake_http): + assert bootstrap.download_and_check(IMAGE_URL, SUM_URL, 'notahash') is None + assert fake_http['timeouts'] == [] # nothing was fetched + + +# ------------------------------------------------------------- uploading + +def test_upload_creates_a_new_image_and_returns_its_id(bootstrap, tmp_path): + image_file = tmp_path / 'img.qcow2' + image_file.write_bytes(IMAGE) + image_id = bootstrap.create_glance_image( + str(image_file), 'GOLD Test', 'qcow2', 8, 768, + {'hw_disk_bus': 'scsi', 'hw_rng_model': 'virtio'}) + assert image_id == 'new-image-uuid' + + created = [e[1] for e in bootstrap.conn.log if e[0] == 'image_created'][0] + assert created['name'] == 'GOLD Test' + assert created['disk_format'] == 'qcow2' + assert created['container_format'] == 'bare' + assert created['visibility'] == 'private' + assert created['min_disk'] == 8 and created['min_ram'] == 768 + assert created['hw_disk_bus'] == 'scsi' + + +def test_upload_forces_a_create_and_adds_no_metadata_of_its_own(bootstrap, tmp_path): + """Defaults would return an existing image and inject vendor properties.""" + image_file = tmp_path / 'img.qcow2' + image_file.write_bytes(IMAGE) + bootstrap.create_glance_image(str(image_file), 'n', 'qcow2', 8, 768, {}) + created = [e[1] for e in bootstrap.conn.log if e[0] == 'image_created'][0] + assert created['allow_duplicates'] is True + assert created['disable_vendor_agent'] is False + + +def test_failed_upload_returns_none(bootstrap, tmp_path): + bootstrap.conn.image.fail = True + image_file = tmp_path / 'img.qcow2' + image_file.write_bytes(IMAGE) + assert bootstrap.create_glance_image( + str(image_file), 'n', 'qcow2', 8, 768, {}) is None + + +def test_bootstrap_no_longer_carries_an_unused_availability_zone(): + assert not hasattr(BootstrapFunctions, 'avail_zone') + import inspect + params = inspect.signature(BootstrapFunctions.__init__).parameters + assert 'avail_zone' not in params + assert os.environ is not None # keep flake quiet about the import diff --git a/tests/test_build.py b/tests/test_build.py new file mode 100644 index 0000000..c7946cb --- /dev/null +++ b/tests/test_build.py @@ -0,0 +1,214 @@ +"""The build path: resource lifecycle, manifest handling, image download.""" +import json +import os +import subprocess + +import pytest + +from image_builder.helpers import Helpers as helpers + + +# --------------------------------------------------------------- keypairs + +def test_keygen_failure_reports_rather_than_returning_a_usable_key(build, monkeypatch): + monkeypatch.setattr(subprocess, 'call', lambda *a, **k: 1) + name, keypair_id = build.create_keypairs() + assert name is None and keypair_id is None + + +def test_keygen_runs_without_a_shell_and_off_PATH(build, monkeypatch): + seen = {} + + def fake_call(cmd, **kwargs): + seen['cmd'] = cmd + seen['kwargs'] = kwargs + return 1 # stop before the key is read back + + monkeypatch.setattr(subprocess, 'call', fake_call) + build.create_keypairs() + assert isinstance(seen['cmd'], list) + assert seen['cmd'][0] == 'ssh-keygen' + assert 'shell' not in seen['kwargs'] + + +def test_successful_keygen_registers_the_public_key(build): + name, keypair_id = build.create_keypairs() + assert name.startswith('imagebuilder-') + assert keypair_id == name + created = [e for e in build.conn.log if e[0] == 'kp_created'] + assert len(created) == 1 + assert created[0][1]['public_key'].startswith('ecdsa-') + + +# -------------------------------------------------------- security groups + +def test_security_group_opens_ssh_on_both_ethertypes(build): + name, sgid = build.create_security_group() + assert name.startswith('imagebuilder-') + rules = [e[1] for e in build.conn.log if e[0] == 'rule_created'] + assert sorted(r['ethertype'] for r in rules) == ['IPv4', 'IPv6'] + for rule in rules: + assert rule['security_group_id'] == sgid + assert rule['direction'] == 'ingress' + assert rule['protocol'] == 'tcp' + assert rule['port_range_min'] == rule['port_range_max'] == 22 + + +# ---------------------------------------------------------------- cleanup + +def test_cleanup_removes_both_resources(build): + build.cleanup('sg-1', 'kp-1') + assert ('sg_deleted', 'sg-1') in build.conn.log + assert ('kp_deleted', 'kp-1') in build.conn.log + + +def test_cleanup_skips_resources_that_were_never_created(build): + """Runs from a finally, so it sees half-built states.""" + build.cleanup('sg-1', None) + assert ('sg_deleted', 'sg-1') in build.conn.log + assert 'kp_deleted' not in build.conn.kinds() + + +def test_cleanup_with_nothing_to_do_is_a_noop(build): + build.cleanup(None, None) + assert build.conn.log == [] + + +def test_cleanup_never_raises_so_it_cannot_mask_the_real_error(build): + def explode(_): + raise RuntimeError('neutron is down') + + build.conn.network.delete_security_group = explode + build.cleanup('sg-1', 'kp-1') # must not raise + assert ('kp_deleted', 'kp-1') in build.conn.log # and carries on + + +def test_clean_tmp_files_tolerates_a_missing_directory(): + helpers.clean_tmp_files('/nonexistent/path/at/all') + + +# --------------------------------------------------------------- manifest + +def write_manifest(build, payload): + path = os.path.join(build.tmp_dir, 'packer-manifest.json') + with open(path, 'w') as fh: + if isinstance(payload, str): + fh.write(payload) + else: + json.dump(payload, fh) + + +def test_good_manifest_yields_the_artifact_id(build): + write_manifest(build, {'builds': [{'artifact_id': 'bgo:image-uuid'}]}) + assert build.parse_manifest() == 'bgo:image-uuid' + + +@pytest.mark.parametrize('payload', [ + pytest.param('{ not json at all', id='malformed'), + pytest.param({}, id='no-builds-key'), + pytest.param({'builds': []}, id='empty-builds'), + pytest.param({'builds': [{}]}, id='no-artifact-id'), +]) +def test_unusable_manifest_returns_none_instead_of_raising(build, payload): + write_manifest(build, payload) + assert build.parse_manifest() is None + + +def test_missing_manifest_returns_none(build): + assert build.parse_manifest() is None + + +# --------------------------------------------------------------- networks + +def test_find_network_returns_the_first_match(build): + build.conn.network.networks_result = [ + type('Net', (), {'id': 'net-1'})(), + type('Net', (), {'id': 'net-2'})(), + ] + assert build.find_network_id('Dualstack') == 'net-1' + + +def test_find_network_returns_false_when_absent(build): + build.conn.network.networks_result = [] + assert build.find_network_id('Nope') is False + + +# ----------------------------------------------------------------- images + +def test_download_writes_the_file_and_then_deletes_from_glance(build): + assert build.download_image('img-uuid') == 0 + written = os.listdir(build.download_dir) + assert len(written) == 1 + assert written[0].startswith('test-image-') and written[0].endswith('.qcow2') + assert ('image_deleted', 'img-uuid') in build.conn.log + + +def test_failed_download_keeps_the_image_and_removes_the_partial_file(build): + build.conn.image.fail = True + assert build.download_image('img-uuid') == 1 + assert os.listdir(build.download_dir) == [] + assert 'image_deleted' not in build.conn.kinds() + + +def test_delete_image_of_nothing_does_not_call_the_api(build): + assert build.delete_image(None) is False + assert build.conn.log == [] + + +def test_delete_image_reports_failure(build): + def explode(image, **kw): + raise RuntimeError('nope') + + build.conn.image.delete_image = explode + assert build.delete_image('img-uuid') is False + + +# --------------------------------------------------------------- template + +def test_hcl_template_is_preferred_over_the_legacy_one(build): + open(os.path.join(build.template_dir, 'template.pkr.hcl'), 'w').close() + open(os.path.join(build.template_dir, 'template'), 'w').close() + assert build.find_template().endswith('template.pkr.hcl') + + +def test_legacy_template_is_still_accepted(build): + open(os.path.join(build.template_dir, 'template'), 'w').close() + assert build.find_template().endswith('template') + + +def test_missing_template_returns_none(build): + assert build.find_template() is None + + +def test_packer_init_is_skipped_for_a_legacy_template(build): + """required_plugins is HCL2 only, so there is nothing for init to do.""" + assert build.run_packer_init('/some/path/template') == 0 + + +# ------------------------------------------------------------ packer argv + +def test_packer_argv_supplies_every_declared_variable(build, monkeypatch, repo_root): + captured = {} + + class FakePopen: + def __init__(self, cmd, **kwargs): + captured['cmd'] = cmd + self.stdout = open(os.devnull, 'rb') + + def wait(self): + return 0 + + monkeypatch.setattr(subprocess, 'Popen', FakePopen) + build.run_packer('/tmp/template.pkr.hcl', 'sg-name', 'kp-name', 'net-uuid') + + cmd = captured['cmd'] + assert cmd[:2] == ['packer', 'build'] + assert '--var' not in cmd # the old long form + supplied = {cmd[i + 1].split('=')[0] for i, a in enumerate(cmd) if a == '-var'} + + declared = set() + with open(repo_root / 'template.pkr.hcl') as fh: + for line in fh: + if line.startswith('variable "'): + declared.add(line.split('"')[1]) + assert supplied == declared diff --git a/tests/test_cli.py b/tests/test_cli.py new file mode 100644 index 0000000..c81ab72 --- /dev/null +++ b/tests/test_cli.py @@ -0,0 +1,106 @@ +"""The command line itself, driven as a subprocess.""" +import os +import subprocess +import sys + +import pytest + +from image_builder.parsecommands import Commands + + +def run(repo_root, *args): + """Runs the checked-in entry script with no OpenStack credentials.""" + env = {k: v for k, v in os.environ.items() + if not k.startswith('OS_') and not k.startswith('IB_')} + env['PATH'] = os.environ['PATH'] + return subprocess.run([sys.executable, 'imagebuilder', *args], + cwd=str(repo_root), env=env, + capture_output=True, text=True) + + +# ------------------------------------------------------------ dispatching + +def test_help_lists_both_subcommands(repo_root): + result = run(repo_root, '--help') + assert 'build' in result.stdout + assert 'bootstrap' in result.stdout + + +def test_no_subcommand_is_an_error(repo_root): + assert run(repo_root).returncode != 0 + + +@pytest.mark.parametrize('name', ['__init__', 'cleanup', 'nonsense']) +def test_arbitrary_attributes_are_not_dispatchable(repo_root, name): + """Dispatch used to be getattr(self, argv[1]).""" + result = run(repo_root, name) + assert result.returncode != 0 + assert 'invalid choice' in (result.stderr + result.stdout) + + +# ------------------------------------------------------------- parsing + +def test_build_args_parse(): + commands = Commands(['build', '-n', 'img', '-a', 'bgo-default-1', + '-s', 'src', '-u', 'almalinux']) + assert commands.build_args.name == 'img' + assert commands.build_args.flavor == 'm1.small' + assert commands.build_args.network_name == 'Dualstack' + assert commands.bootstrap_args is False + + +def test_bootstrap_args_parse_numbers_as_ints(): + commands = Commands(['bootstrap', '-a', 'bgo-default-1', + '-u', 'https://example.com/i.qcow2', '-n', 'n', + '-r', '768', '-d', '8', '-f', 'qcow2']) + assert commands.bootstrap_args.min_ram == 768 + assert commands.bootstrap_args.min_disk == 8 + assert commands.build_args is False + + +def test_non_numeric_min_ram_is_rejected_at_parse_time(repo_root): + result = run(repo_root, 'bootstrap', '-a', 'z', '-u', 'https://x/i.qcow2', + '-n', 'n', '-r', 'notanumber', '-d', '8', '-f', 'qcow2') + assert result.returncode != 0 + assert 'invalid int' in result.stderr + + +# ------------------------------------------------------------- streams + +def test_diagnostics_go_to_stderr_leaving_stdout_clean(repo_root): + """bootstrap pipes its image id out of stdout, so errors must not go there.""" + result = run(repo_root, 'build', '-n', 'x', '-a', 'z', '-s', 'i', '-u', 'u') + assert result.returncode != 0 + assert result.stdout == '' + assert 'Missing environment variable' in result.stderr + + +def test_the_missing_variables_are_named(repo_root): + result = run(repo_root, 'build', '-n', 'x', '-a', 'z', '-s', 'i', '-u', 'u') + assert 'OS_USERNAME' in result.stderr + + +# ------------------------------------------------------------- logging + +@pytest.mark.parametrize('flags,expected', [ + pytest.param([], 'WARNING', id='default'), + pytest.param(['-v'], 'INFO', id='verbose'), + pytest.param(['--debug'], 'DEBUG', id='debug'), + pytest.param(['-v', '--debug'], 'DEBUG', id='debug-wins-over-verbose'), +]) +def test_log_level_selection(flags, expected): + import logging + + from image_builder.imagebuilder import configure_logging + + commands = Commands(['build', '-n', 'i', '-a', 'z', '-s', 's', '-u', 'u', + *flags]) + root = logging.getLogger() + saved_level, saved_handlers = root.level, root.handlers[:] + root.handlers = [] # basicConfig is a no-op otherwise + try: + configure_logging(commands.build_args) + assert logging.getLevelName(root.level) == expected + finally: + root.handlers = saved_handlers + root.setLevel(saved_level) diff --git a/tests/test_credentials.py b/tests/test_credentials.py new file mode 100644 index 0000000..3b5e979 --- /dev/null +++ b/tests/test_credentials.py @@ -0,0 +1,52 @@ +"""Reading credentials from the environment.""" +import pytest + +from image_builder.imagebuilder import ImageBuilder + +from conftest import FULL_OPENRC + + +def test_openrc_without_the_legacy_variables_is_accepted(clean_env): + """OS_IDENTITY_API_VERSION and OS_NO_CACHE were required but never used.""" + for key, value in FULL_OPENRC.items(): + clean_env.setenv(key, value) + rc = ImageBuilder.get_openstack_rc() + assert rc['username'] == 'user' + assert rc['region_name'] == 'bgo' + + +def test_unused_keys_are_not_carried_around(clean_env): + for key, value in FULL_OPENRC.items(): + clean_env.setenv(key, value) + rc = ImageBuilder.get_openstack_rc() + assert 'api_version' not in rc + assert 'no_cache' not in rc + + +def test_cacert_is_optional(clean_env): + for key, value in FULL_OPENRC.items(): + clean_env.setenv(key, value) + assert ImageBuilder.get_openstack_rc()['cacert'] is None + clean_env.setenv('OS_CACERT', '/etc/ssl/ca.pem') + assert ImageBuilder.get_openstack_rc()['cacert'] == '/etc/ssl/ca.pem' + + +@pytest.mark.parametrize('missing', sorted(FULL_OPENRC)) +def test_each_required_variable_is_reported_by_name(clean_env, missing): + for key, value in FULL_OPENRC.items(): + if key != missing: + clean_env.setenv(key, value) + with pytest.raises(KeyError) as excinfo: + ImageBuilder.get_openstack_rc() + assert missing in excinfo.value.args[0] + + +def test_all_missing_variables_are_reported_not_just_the_first(clean_env): + for key, value in FULL_OPENRC.items(): + if key not in ('OS_PASSWORD', 'OS_REGION_NAME'): + clean_env.setenv(key, value) + with pytest.raises(KeyError) as excinfo: + ImageBuilder.get_openstack_rc() + message = excinfo.value.args[0] + assert 'OS_PASSWORD' in message + assert 'OS_REGION_NAME' in message diff --git a/tests/test_main_flow.py b/tests/test_main_flow.py new file mode 100644 index 0000000..faaccc1 --- /dev/null +++ b/tests/test_main_flow.py @@ -0,0 +1,128 @@ +"""main() end to end: every exit path must still clean up. + +These drive main() with a stand-in BuildFunctions, so they cover the wiring +between the pieces rather than the pieces themselves. +""" +import tempfile + +import pytest + +import image_builder.imagebuilder as ib + +from conftest import FULL_OPENRC + + +@pytest.fixture +def run_build(clean_env, monkeypatch): + """Runs main() for a build, returning (exit code, propagated exception, calls).""" + + def run(packer=lambda: 0, keygen_ok=True, manifest='img-uuid', argv=None): + calls = [] + + class FakeBuild: + def __init__(self, *a, **k): + self.tmp_dir = tempfile.mkdtemp(prefix='mainflow-') + self.image_name = 'img' + + def find_template(self): + return '/tmp/template.pkr.hcl' + + def find_network_id(self, name): + return 'net-1' + + def run_packer_init(self, template): + return 0 + + def create_security_group(self): + calls.append('sg_created') + return 'sg-name', 'sg-1' + + def create_keypairs(self): + if not keygen_ok: + return None, None + calls.append('kp_created') + return 'kp-name', 'kp-1' + + def run_packer(self, *a): + return packer() + + def parse_manifest(self): + return manifest + + def download_image(self, artifact_id): + return 0 + + def delete_image(self, image_id): + return True + + def cleanup(self, secgroup_id, keypair_id): + calls.append('cleanup(sg=%s kp=%s)' % (secgroup_id, keypair_id)) + + for key, value in FULL_OPENRC.items(): + clean_env.setenv(key, value) + clean_env.setenv('IB_TEMPLATE_DIR', '/tmp') + clean_env.setenv('IB_DOWNLOAD_DIR', '/tmp') + monkeypatch.setattr(ib, 'BuildFunctions', FakeBuild) + monkeypatch.setattr(ib.ImageBuilder, 'auth', staticmethod(lambda rc: object())) + monkeypatch.setattr('sys.argv', argv or + ['imagebuilder', 'build', '-n', 'img', + '-a', 'bgo-default-1', '-s', 'src', '-u', 'almalinux']) + + code, raised = None, None + try: + ib.main() + except SystemExit as exc: + code = exc.code + except BaseException as exc: # noqa: BLE001 + raised = exc + return code, raised, calls + + return run + + +def cleaned(calls): + return [c for c in calls if c.startswith('cleanup')] + + +def test_successful_build_exits_zero_and_cleans_up(run_build): + code, raised, calls = run_build() + assert code == 0 and raised is None + assert cleaned(calls) == ['cleanup(sg=sg-1 kp=kp-1)'] + + +def test_failed_packer_run_exits_one_and_cleans_up(run_build): + code, raised, calls = run_build(packer=lambda: 1) + assert code == 1 + assert cleaned(calls) + + +def test_interrupt_during_build_still_cleans_up(run_build): + """The case that used to orphan a security group and a keypair.""" + def interrupt(): + raise KeyboardInterrupt() + + code, raised, calls = run_build(packer=interrupt) + assert cleaned(calls) == ['cleanup(sg=sg-1 kp=kp-1)'] + assert isinstance(raised, KeyboardInterrupt) # and is not swallowed + + +def test_unexpected_exception_still_cleans_up(run_build): + def boom(): + raise RuntimeError('packer blew up') + + code, raised, calls = run_build(packer=boom) + assert cleaned(calls) + assert isinstance(raised, RuntimeError) + + +def test_keygen_failure_exits_before_packer_and_cleans_up(run_build): + code, raised, calls = run_build(keygen_ok=False) + assert code == 1 + assert 'kp_created' not in calls + assert cleaned(calls) == ['cleanup(sg=sg-1 kp=None)'] + + +def test_unusable_manifest_fails_the_build(run_build): + code, raised, calls = run_build(manifest=None) + assert code == 1 + assert cleaned(calls) From 2e89e1d35dcdaaa2e5a6dd1c261a1b2a6d057b4e Mon Sep 17 00:00:00 2001 From: Trond Hasle Amundsen Date: Sun, 13 Sep 2026 22:01:57 +0200 Subject: [PATCH 13/22] refactor: modernise to current Python idiom One latent bug among the cleanup: three text-mode opens had no encoding, so the public key, the Packer manifest and the config file were all decoded using the locale. The same file would read differently on a build node running under LC_ALL=C than on a UTF-8 workstation. They are explicit now. Two bare excepts around config.get() would also have swallowed KeyboardInterrupt; they catch the two configparser errors they meant to. The argument check in checksum_file() was an assert, which disappears entirely under python -O, and is now a ValueError. BuildFunctions took ten positional parameters and main() passed ten positional arguments, so transposing two of them would have been silent and legal. The image-related ones are keyword-only now. Helpers was five static methods with no state and ImageBuilder was two: both were classes standing in for a module, which Python already provides. They are module-level functions, and the call sites read the same because the import became `from . import helpers`. The subcommand attributes use None rather than False for "not this one", classes no longer inherit from object, string building uses f-strings where it was concatenation or percent formatting, and subprocess.run replaces subprocess.call with Popen in a with block. Lazy percent arguments inside logging calls stay as they are, which is correct. ruff now selects its rules explicitly in pyproject.toml instead of inheriting whatever the current default happens to be, which is what broke CI when 0.16.0 widened it. That makes the pin unnecessary, so ruff is unpinned again; verified clean under both 0.15.22 and 0.16.7. pytest stays pinned because 9 requires Python 3.10. No behaviour changes. The 87 tests pass unmodified except where they reached for the things that moved: two caught the subprocess change by failing, which is what they are for. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A4Y4JQ39uQMSZcBYoTE84y --- .github/workflows/python-package.yml | 13 ++-- image_builder/bootstrap.py | 6 +- image_builder/build.py | 54 ++++++------- image_builder/config.py | 7 +- image_builder/helpers.py | 107 +++++++++++++------------- image_builder/imagebuilder.py | 110 +++++++++++++-------------- image_builder/parsecommands.py | 8 +- pyproject.toml | 29 ++++++- tests/conftest.py | 2 +- tests/test_bootstrap.py | 26 +++---- tests/test_build.py | 26 +++++-- tests/test_cli.py | 4 +- tests/test_credentials.py | 17 ++--- tests/test_main_flow.py | 15 ++-- 14 files changed, 234 insertions(+), 190 deletions(-) diff --git a/.github/workflows/python-package.yml b/.github/workflows/python-package.yml index 15b6dd9..d9226ee 100644 --- a/.github/workflows/python-package.yml +++ b/.github/workflows/python-package.yml @@ -34,16 +34,17 @@ jobs: - name: Install dependencies run: | python -m pip install --upgrade pip - # Both pinned: ruff 0.16.0 widened the default rule set, and pytest - # 9 requires Python 3.10. Bump deliberately. - pip install 'ruff==0.15.22' 'pytest==8.4.2' + # ruff is unpinned now that pyproject.toml selects rules explicitly, + # so a new release cannot widen the set under us. pytest stays pinned + # because 9 requires Python 3.10. + pip install ruff 'pytest==8.4.2' if [ -f requirements.txt ]; then pip install -r requirements.txt; fi pip install -e . - name: Lint with ruff run: | # stop the build if there are Python syntax errors or undefined names - ruff check --output-format=github --select=E9,F63,F7,F82 --target-version=py39 . - # default set of ruff rules with GitHub Annotations - ruff check --output-format=github --target-version=py39 . + ruff check --output-format=github --select=E9,F63,F7,F82 . + # the rule set from pyproject.toml, with GitHub Annotations + ruff check --output-format=github . - name: Run tests run: pytest -q diff --git a/image_builder/bootstrap.py b/image_builder/bootstrap.py index b7aab59..fb2e02d 100644 --- a/image_builder/bootstrap.py +++ b/image_builder/bootstrap.py @@ -1,8 +1,10 @@ import logging import os import urllib.request + from openstack.connection import Connection -from .helpers import Helpers as helpers + +from . import helpers log = logging.getLogger(__name__) @@ -14,7 +16,7 @@ # Anything smaller than this is an error page, not a cloud image MIN_IMAGE_BYTES = 1000 -class BootstrapFunctions(object): +class BootstrapFunctions: def __init__(self, session, region): diff --git a/image_builder/build.py b/image_builder/build.py index 825828e..59426c6 100644 --- a/image_builder/build.py +++ b/image_builder/build.py @@ -4,15 +4,18 @@ import subprocess import time import uuid + from openstack.connection import Connection -from .helpers import Helpers as helpers + +from . import helpers log = logging.getLogger(__name__) -class BuildFunctions(object): +class BuildFunctions: def __init__(self, session, region, + *, image_name, avail_zone, flavor, @@ -59,18 +62,18 @@ def cleanup(self, secgroup_id, keypair_id): def create_keypairs(self): """Creates a temporary keypair""" - keyname = "imagebuilder-" + str(uuid.uuid4().hex) + keyname = f"imagebuilder-{uuid.uuid4().hex}" keypath = os.path.join(self.tmp_dir, keyname) # No shell: there is nothing here a shell is needed for, and letting # PATH find ssh-keygen beats hardcoding where it lives cmd = ['ssh-keygen', '-b', '521', '-t', 'ecdsa', '-N', '', '-f', keypath] log.debug(cmd) - out = subprocess.call(cmd) # generate temporary ssh key + out = subprocess.run(cmd, check=False).returncode # generate temporary ssh key if out: # something went wrong with the key generation log.error("Failed to generate SSH key, ssh-keygen exited %s", out) return None, None # read public key string and store into Openstack - with open(keypath + ".pub", "r") as pubfile: + with open(keypath + ".pub", encoding="utf-8") as pubfile: pubkeystring = pubfile.read().replace('\n', '') keypair = self.conn.compute.create_keypair(name=keyname, public_key=pubkeystring) @@ -78,7 +81,7 @@ def create_keypairs(self): def create_security_group(self): """Creates a temporary security group""" - secgroup_name = "imagebuilder-" + str(uuid.uuid4().hex) + secgroup_name = f"imagebuilder-{uuid.uuid4().hex}" secgroup = self.conn.network.create_security_group( name=secgroup_name, description='Temporary security group for image building') @@ -113,7 +116,7 @@ def download_image(self, artifact_id): OS_* environment, and that CLI is deprecated upstream. """ timestr = time.strftime("%Y%m%d") - filename = self.image_name + '-' + timestr + '.qcow2' + filename = f'{self.image_name}-{timestr}.qcow2' target = os.path.join(self.download_dir, filename) log.info("Downloading image to %s...", target) try: @@ -169,7 +172,7 @@ def parse_manifest(self): """ manifest_path = os.path.join(self.tmp_dir, 'packer-manifest.json') try: - with open(manifest_path) as data_file: + with open(manifest_path, encoding="utf-8") as data_file: manifest = json.load(data_file) return manifest['builds'][0]['artifact_id'] except (OSError, ValueError, KeyError, IndexError) as error: @@ -178,17 +181,17 @@ def parse_manifest(self): def run_packer(self, template_path, secgroup_name, key_name, network_id): """Executes Packer command""" - image_name_var = 'image_name=' + self.image_name - avail_zone_var = 'availability_zone=' + self.avail_zone - secgroup_var = 'security_group=' + secgroup_name - sshuser_var = 'ssh_username=' + self.ssh_user - keyname_var = 'ssh_keypair_name=' + key_name # key as it is named in Openstack - keypath_var = 'ssh_key_path=' + os.path.join(self.tmp_dir, key_name) # the "real" private key - flavor_var = 'flavor=' + self.flavor - network_var = 'network=' + network_id - source_image_var = 'source_image=' + self.source_image - provision_script_var = 'provision_script=' + self.provision_script - manifest_path_var = 'manifest_path=' + os.path.join(self.tmp_dir, 'packer-manifest.json') + image_name_var = f'image_name={self.image_name}' + avail_zone_var = f'availability_zone={self.avail_zone}' + secgroup_var = f'security_group={secgroup_name}' + sshuser_var = f'ssh_username={self.ssh_user}' + keyname_var = f'ssh_keypair_name={key_name}' # key as it is named in Openstack + keypath_var = f'ssh_key_path={os.path.join(self.tmp_dir, key_name)}' # the "real" private key + flavor_var = f'flavor={self.flavor}' + network_var = f'network={network_id}' + source_image_var = f'source_image={self.source_image}' + provision_script_var = f'provision_script={self.provision_script}' + manifest_path_var = f"manifest_path={os.path.join(self.tmp_dir, 'packer-manifest.json')}" cmd = ['packer', 'build', '-color=false', '-var', image_name_var, @@ -204,11 +207,10 @@ def run_packer(self, template_path, secgroup_name, key_name, network_id): '-var', manifest_path_var, template_path] log.debug(cmd) - process = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) - with process.stdout: + with subprocess.Popen(cmd, stdout=subprocess.PIPE, + stderr=subprocess.STDOUT) as process: helpers.log_subprocess_output(process.stdout) - exitcode = process.wait() - return exitcode + return process.wait() def run_packer_init(self, template_path): """Installs the Packer plugins required by the template @@ -222,7 +224,7 @@ def run_packer_init(self, template_path): return 0 cmd = ['packer', 'init', template_path] log.debug(cmd) - process = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) - with process.stdout: + with subprocess.Popen(cmd, stdout=subprocess.PIPE, + stderr=subprocess.STDOUT) as process: helpers.log_subprocess_output(process.stdout) - return process.wait() + return process.wait() diff --git a/image_builder/config.py b/image_builder/config.py index e879d9f..54523f2 100644 --- a/image_builder/config.py +++ b/image_builder/config.py @@ -1,7 +1,8 @@ -import os import configparser +import os + -class Config(object): +class Config: def __init__(self, configfile_path=None): if configfile_path is None: self.configfile_path = [os.curdir, @@ -16,5 +17,5 @@ def __init__(self, configfile_path=None): def __read_config(self): config = configparser.ConfigParser() - config.read(self.config_file_list) + config.read(self.config_file_list, encoding='utf-8') return config diff --git a/image_builder/helpers.py b/image_builder/helpers.py index b87f489..8e22d34 100644 --- a/image_builder/helpers.py +++ b/image_builder/helpers.py @@ -1,3 +1,8 @@ +"""Small shared helpers. + +Module-level functions rather than static methods on a class: a module is +already the namespace a class was being used for here. +""" import hashlib import logging import shutil @@ -5,55 +10,53 @@ log = logging.getLogger(__name__) -class Helpers(object): - - @staticmethod - def clean_tmp_files(tmp_dir): - # Also called from a finally block, so failing to remove a scratch - # directory must not be what the user ends up seeing - log.info('Removing temporary directory with content...') - shutil.rmtree(tmp_dir, ignore_errors=True) - - @staticmethod - def make_tmp_dir(): - log.info('Creating a directory for temporary files...') - tmp_dir = tempfile.mkdtemp(prefix='imagebuilder-') - return tmp_dir - - @staticmethod - def log_subprocess_output(pipe): - # Decoded, not repr'd: this is the build log the user watches, and - # every line of it used to arrive looking like b'...\n' - for line in iter(pipe.readline, b''): - log.info(line.decode('utf-8', errors='replace').rstrip()) - - @staticmethod - def valid_digest(digest): - """Whether hashlib knows this digest name - - Used to reject a bad -t before spending a download on it. - """ - try: - hashlib.new(digest.lower()) - except (ValueError, TypeError): - return False - return True - - @staticmethod - def checksum_file(file_path, digest='sha256', chunk_size=65536): - """Read the file in small pieces, so as to prevent failures to read - particularly large files. Also ensures memory usage is kept to a - minimum. Testing shows default is a pretty good size.""" - assert isinstance(chunk_size, int) and chunk_size > 0 - # hashlib.new() takes any algorithm it supports and raises a clear - # ValueError naming a bad one. The hand-written if/elif ladder this - # replaces silently left unknown names as a string and then died with - # AttributeError on the first update() - hasher = hashlib.new(digest.lower()) - #pylint: disable=invalid-name - with open(file_path, 'rb') as f: - for block in iter(lambda: f.read(chunk_size), b''): - hasher.update(block) - checksum = hasher.hexdigest() - log.debug("hexdigest of %s is %s", file_path, checksum) - return checksum + +def clean_tmp_files(tmp_dir): + # Also called from a finally block, so failing to remove a scratch + # directory must not be what the user ends up seeing + log.info('Removing temporary directory with content...') + shutil.rmtree(tmp_dir, ignore_errors=True) + + +def make_tmp_dir(): + log.info('Creating a directory for temporary files...') + return tempfile.mkdtemp(prefix='imagebuilder-') + + +def log_subprocess_output(pipe): + # Decoded, not repr'd: this is the build log the user watches, and every + # line of it used to arrive looking like b'...\n' + for line in iter(pipe.readline, b''): + log.info(line.decode('utf-8', errors='replace').rstrip()) + + +def valid_digest(digest): + """Whether hashlib knows this digest name + + Used to reject a bad -t before spending a download on it. + """ + try: + hashlib.new(digest.lower()) + except (ValueError, TypeError): + return False + return True + + +def checksum_file(file_path, digest='sha256', chunk_size=65536): + """Read the file in small pieces, so as to prevent failures to read + particularly large files. Also ensures memory usage is kept to a + minimum. Testing shows default is a pretty good size.""" + if not isinstance(chunk_size, int) or chunk_size <= 0: + # Raised rather than asserted: assertions disappear under python -O + raise ValueError(f"chunk_size must be a positive integer, got {chunk_size!r}") + # hashlib.new() takes any algorithm it supports and raises a clear + # ValueError naming a bad one. The hand-written if/elif ladder this + # replaces silently left unknown names as a string and then died with + # AttributeError on the first update() + hasher = hashlib.new(digest.lower()) + with open(file_path, 'rb') as handle: + for block in iter(lambda: handle.read(chunk_size), b''): + hasher.update(block) + checksum = hasher.hexdigest() + log.debug("hexdigest of %s is %s", file_path, checksum) + return checksum diff --git a/image_builder/imagebuilder.py b/image_builder/imagebuilder.py index d9617bf..c61f465 100644 --- a/image_builder/imagebuilder.py +++ b/image_builder/imagebuilder.py @@ -1,59 +1,56 @@ +import configparser import logging import os import sys -from keystoneauth1.identity import v3 + from keystoneauth1 import session -from .parsecommands import Commands -from .build import BuildFunctions +from keystoneauth1.identity import v3 + +from . import helpers from .bootstrap import BootstrapFunctions +from .build import BuildFunctions from .config import Config -from .helpers import Helpers as helpers +from .parsecommands import Commands log = logging.getLogger(__name__) -class ImageBuilder(object): - @staticmethod - def auth(rc): - auth = v3.Password(auth_url=rc['auth_url'], +# Only what auth() and the region lookup actually consume. Asking for more than +# that turns a perfectly good openrc file into a failed login +REQUIRED_ENV = { + 'username': 'OS_USERNAME', + 'project_name': 'OS_PROJECT_NAME', + 'password': 'OS_PASSWORD', + 'auth_url': 'OS_AUTH_URL', + 'user_domain_name': 'OS_USER_DOMAIN_NAME', + 'project_domain_name': 'OS_PROJECT_DOMAIN_NAME', + 'region_name': 'OS_REGION_NAME', +} + + +def auth(rc): + """Builds an authenticated keystone session from the settings dict""" + password = v3.Password(auth_url=rc['auth_url'], project_name=rc['project_name'], username=rc['username'], password=rc['password'], user_domain_name=rc['user_domain_name'], project_domain_name=rc['project_domain_name']) - if rc['cacert'] is not None: - sess = session.Session(auth, - verify=rc['cacert']) - else: - sess = session.Session(auth) - return sess - - # Only what auth() and the region lookup actually consume. Asking for more - # than that turns a perfectly good openrc file into a failed login - REQUIRED_ENV = { - 'username': 'OS_USERNAME', - 'project_name': 'OS_PROJECT_NAME', - 'password': 'OS_PASSWORD', - 'auth_url': 'OS_AUTH_URL', - 'user_domain_name': 'OS_USER_DOMAIN_NAME', - 'project_domain_name': 'OS_PROJECT_DOMAIN_NAME', - 'region_name': 'OS_REGION_NAME', - } - - @classmethod - def get_openstack_rc(cls): - """Reads the credentials from the environment - - OS_CACERT is optional. Raises KeyError naming every variable that is - missing rather than just the first one found. - """ - missing = [name for name in cls.REQUIRED_ENV.values() - if name not in os.environ] - if missing: - raise KeyError(', '.join(missing)) - env_var = {key: os.environ[name] - for key, name in cls.REQUIRED_ENV.items()} - env_var['cacert'] = os.environ.get('OS_CACERT') - return env_var + if rc['cacert'] is not None: + return session.Session(password, verify=rc['cacert']) + return session.Session(password) + +def get_openstack_rc(): + """Reads the credentials from the environment + + OS_CACERT is optional. Raises KeyError naming every variable that is + missing rather than just the first one found. + """ + missing = [name for name in REQUIRED_ENV.values() if name not in os.environ] + if missing: + raise KeyError(', '.join(missing)) + env_var = {key: os.environ[name] for key, name in REQUIRED_ENV.items()} + env_var['cacert'] = os.environ.get('OS_CACERT') + return env_var def configure_logging(args): """Sets up logging once for whichever subcommand was chosen @@ -75,18 +72,17 @@ def configure_logging(args): def main(): commands = Commands() configure_logging(commands.build_args or commands.bootstrap_args) - imagebuilder = ImageBuilder() try: - rc = imagebuilder.get_openstack_rc() + rc = get_openstack_rc() except KeyError as missing: - print("""Missing environment variable(s): %s + print(f"""Missing environment variable(s): {missing.args[0]} Please run: source -and try again.""" % missing.args[0], file=sys.stderr) +and try again.""", file=sys.stderr) sys.exit(1) - ib_session = imagebuilder.auth(rc) + ib_session = auth(rc) region = rc['region_name'] config = Config().config @@ -96,7 +92,7 @@ def main(): else: try: template_dir = config.get('main', 'template_dir') - except: + except (configparser.NoSectionError, configparser.NoOptionError): print("Failed to read template_dir from config", file=sys.stderr) sys.exit(1) @@ -105,7 +101,7 @@ def main(): else: try: download_dir = config.get('main', 'download_dir') - except: + except (configparser.NoSectionError, configparser.NoOptionError): print("Failed to read download_dir from config", file=sys.stderr) sys.exit(1) @@ -120,14 +116,14 @@ def main(): build = BuildFunctions(ib_session, region, - image_name, - avail_zone, - flavor, - source_image, - sshuser, - provision_script, - template_dir, - download_dir) + image_name=image_name, + avail_zone=avail_zone, + flavor=flavor, + source_image=source_image, + ssh_user=sshuser, + provision_script=provision_script, + template_dir=template_dir, + download_dir=download_dir) # Everything that can fail without creating anything in OpenStack is # done first, so a bad template or network name doesn't leave a diff --git a/image_builder/parsecommands.py b/image_builder/parsecommands.py index bde0029..33c5aa0 100644 --- a/image_builder/parsecommands.py +++ b/image_builder/parsecommands.py @@ -1,6 +1,7 @@ import argparse -class Commands(object): + +class Commands: def __init__(self, argv=None): parser = argparse.ArgumentParser( @@ -103,5 +104,6 @@ def __init__(self, argv=None): default=False) args = parser.parse_args(argv) - self.build_args = args if args.command == 'build' else False - self.bootstrap_args = args if args.command == 'bootstrap' else False + self.command = args.command + self.build_args = args if args.command == 'build' else None + self.bootstrap_args = args if args.command == 'bootstrap' else None diff --git a/pyproject.toml b/pyproject.toml index 11dc51b..a6ae6b1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -30,5 +30,32 @@ packages = ["image_builder"] version = { attr = "image_builder.__version__" } [tool.ruff] +target-version = "py39" + +# Selected explicitly so the rule set is this repository's decision rather than +# whatever ruff currently defaults to. Relying on the default is what broke CI +# when 0.16.0 widened it. +lint.select = [ + "E", # pycodestyle errors + "F", # pyflakes + "W", # pycodestyle warnings + "I", # import sorting + "UP", # pyupgrade + "B", # bugbear + "SIM", # simplify + "RET", # return consistency + "C4", # comprehensions + "PIE", # misc lints + "LOG", # logging misuse + "S", # bandit +] + # Never enforce `E501` (line length violations). -lint.ignore = ["E501", "E722"] +lint.ignore = [ + "E501", + "S310", # urlopen: the URLs come from the operator running the command + "S603", # subprocess without shell: the argv is built here, not user input +] + +[tool.ruff.lint.per-file-ignores] +"tests/*" = ["S101", "S603", "S607"] # asserts and subprocess are the point diff --git a/tests/conftest.py b/tests/conftest.py index 7731260..3d4c4ea 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -126,7 +126,7 @@ def bootstrap(tmp_path): def clean_env(monkeypatch): """An environment with every OS_* variable removed.""" for key in list(os.environ): - if key.startswith('OS_') or key.startswith('IB_'): + if key.startswith(('OS_', 'IB_')): monkeypatch.delenv(key, raising=False) return monkeypatch diff --git a/tests/test_bootstrap.py b/tests/test_bootstrap.py index 25edd78..d537ad2 100644 --- a/tests/test_bootstrap.py +++ b/tests/test_bootstrap.py @@ -7,8 +7,8 @@ import pytest from image_builder import bootstrap as bootstrap_module +from image_builder import helpers from image_builder.bootstrap import BootstrapFunctions -from image_builder.helpers import Helpers as helpers HASH = 'a' * 64 IMAGE = b'x' * 5000 @@ -43,20 +43,20 @@ def test_valid_digest(): @pytest.mark.parametrize('text,name,expected', [ - pytest.param('%s img.qcow2\n' % HASH, 'img.qcow2', HASH, id='coreutils'), - pytest.param('%s *img.img\n' % HASH, 'img.img', HASH, id='binary-star'), - pytest.param('SHA256 (img.qcow2) = %s\n' % HASH, 'img.qcow2', HASH, id='bsd'), - pytest.param('%s img.qcow2\n' % HASH.upper(), 'img.qcow2', HASH, id='uppercase'), - pytest.param('# c\n\n%s img.qcow2\n' % HASH, 'img.qcow2', HASH, id='comments'), - pytest.param('%s ./images/img.qcow2\n' % HASH, 'img.qcow2', HASH, id='path'), - pytest.param('%s other.qcow2\n' % HASH, 'img.qcow2', None, id='name-absent'), + pytest.param(f'{HASH} img.qcow2\n', 'img.qcow2', HASH, id='coreutils'), + pytest.param(f'{HASH} *img.img\n', 'img.img', HASH, id='binary-star'), + pytest.param(f'SHA256 (img.qcow2) = {HASH}\n', 'img.qcow2', HASH, id='bsd'), + pytest.param(f'{HASH.upper()} img.qcow2\n', 'img.qcow2', HASH, id='uppercase'), + pytest.param(f'# c\n\n{HASH} img.qcow2\n', 'img.qcow2', HASH, id='comments'), + pytest.param(f'{HASH} ./images/img.qcow2\n', 'img.qcow2', HASH, id='path'), + pytest.param(f'{HASH} other.qcow2\n', 'img.qcow2', None, id='name-absent'), ]) def test_checksum_layouts(text, name, expected): assert find(text, name) == expected def test_the_right_line_is_picked_among_several(): - text = '%s a.qcow2\n%s b.qcow2\n' % ('a' * 64, 'b' * 64) + text = f"{'a' * 64} a.qcow2\n{'b' * 64} b.qcow2\n" assert find(text, 'b.qcow2') == 'b' * 64 @@ -110,25 +110,25 @@ def test_requests_carry_a_timeout(bootstrap, fake_http): def test_matching_checksum_is_accepted(bootstrap, fake_http): digest = hashlib.sha256(IMAGE).hexdigest() - fake_http['checksum'] = ('%s img.qcow2\n' % digest).encode() + fake_http['checksum'] = f'{digest} img.qcow2\n'.encode() assert bootstrap.download_and_check(IMAGE_URL, SUM_URL) is not None def test_uppercase_checksum_file_is_accepted(bootstrap, fake_http): digest = hashlib.sha256(IMAGE).hexdigest() - fake_http['checksum'] = ('%s img.qcow2\n' % digest.upper()).encode() + fake_http['checksum'] = f'{digest.upper()} img.qcow2\n'.encode() assert bootstrap.download_and_check(IMAGE_URL, SUM_URL) is not None def test_mismatched_checksum_is_rejected(bootstrap, fake_http): - fake_http['checksum'] = ('%s img.qcow2\n' % ('f' * 64)).encode() + fake_http['checksum'] = f"{'f' * 64} img.qcow2\n".encode() assert bootstrap.download_and_check(IMAGE_URL, SUM_URL) is None def test_hash_listed_under_another_name_still_passes_via_fallback(bootstrap, fake_http): """Kept so vendors whose checksum files name things differently still work.""" digest = hashlib.sha256(IMAGE).hexdigest() - fake_http['checksum'] = ('%s somethingelse.qcow2\n' % digest).encode() + fake_http['checksum'] = f'{digest} somethingelse.qcow2\n'.encode() assert bootstrap.download_and_check(IMAGE_URL, SUM_URL) is not None diff --git a/tests/test_build.py b/tests/test_build.py index c7946cb..c8ca793 100644 --- a/tests/test_build.py +++ b/tests/test_build.py @@ -1,17 +1,19 @@ """The build path: resource lifecycle, manifest handling, image download.""" +import io import json import os import subprocess +import types import pytest -from image_builder.helpers import Helpers as helpers - +from image_builder import helpers # --------------------------------------------------------------- keypairs def test_keygen_failure_reports_rather_than_returning_a_usable_key(build, monkeypatch): - monkeypatch.setattr(subprocess, 'call', lambda *a, **k: 1) + monkeypatch.setattr(subprocess, 'run', + lambda *a, **k: types.SimpleNamespace(returncode=1)) name, keypair_id = build.create_keypairs() assert name is None and keypair_id is None @@ -19,12 +21,12 @@ def test_keygen_failure_reports_rather_than_returning_a_usable_key(build, monkey def test_keygen_runs_without_a_shell_and_off_PATH(build, monkeypatch): seen = {} - def fake_call(cmd, **kwargs): + def fake_run(cmd, **kwargs): seen['cmd'] = cmd seen['kwargs'] = kwargs - return 1 # stop before the key is read back + return types.SimpleNamespace(returncode=1) # stop before the key is read - monkeypatch.setattr(subprocess, 'call', fake_call) + monkeypatch.setattr(subprocess, 'run', fake_run) build.create_keypairs() assert isinstance(seen['cmd'], list) assert seen['cmd'][0] == 'ssh-keygen' @@ -193,13 +195,21 @@ def test_packer_argv_supplies_every_declared_variable(build, monkeypatch, repo_r class FakePopen: def __init__(self, cmd, **kwargs): captured['cmd'] = cmd - self.stdout = open(os.devnull, 'rb') + self.stdout = io.BytesIO(b'') + + def __enter__(self): + return self + + def __exit__(self, *exc): + self.stdout.close() + return False def wait(self): return 0 monkeypatch.setattr(subprocess, 'Popen', FakePopen) - build.run_packer('/tmp/template.pkr.hcl', 'sg-name', 'kp-name', 'net-uuid') + build.run_packer(os.path.join(build.template_dir, 'template.pkr.hcl'), + 'sg-name', 'kp-name', 'net-uuid') cmd = captured['cmd'] assert cmd[:2] == ['packer', 'build'] diff --git a/tests/test_cli.py b/tests/test_cli.py index c81ab72..3b2585c 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -46,7 +46,7 @@ def test_build_args_parse(): assert commands.build_args.name == 'img' assert commands.build_args.flavor == 'm1.small' assert commands.build_args.network_name == 'Dualstack' - assert commands.bootstrap_args is False + assert commands.bootstrap_args is None def test_bootstrap_args_parse_numbers_as_ints(): @@ -55,7 +55,7 @@ def test_bootstrap_args_parse_numbers_as_ints(): '-r', '768', '-d', '8', '-f', 'qcow2']) assert commands.bootstrap_args.min_ram == 768 assert commands.bootstrap_args.min_disk == 8 - assert commands.build_args is False + assert commands.build_args is None def test_non_numeric_min_ram_is_rejected_at_parse_time(repo_root): diff --git a/tests/test_credentials.py b/tests/test_credentials.py index 3b5e979..519b6f7 100644 --- a/tests/test_credentials.py +++ b/tests/test_credentials.py @@ -1,16 +1,15 @@ """Reading credentials from the environment.""" import pytest - -from image_builder.imagebuilder import ImageBuilder - from conftest import FULL_OPENRC +from image_builder import imagebuilder + def test_openrc_without_the_legacy_variables_is_accepted(clean_env): """OS_IDENTITY_API_VERSION and OS_NO_CACHE were required but never used.""" for key, value in FULL_OPENRC.items(): clean_env.setenv(key, value) - rc = ImageBuilder.get_openstack_rc() + rc = imagebuilder.get_openstack_rc() assert rc['username'] == 'user' assert rc['region_name'] == 'bgo' @@ -18,7 +17,7 @@ def test_openrc_without_the_legacy_variables_is_accepted(clean_env): def test_unused_keys_are_not_carried_around(clean_env): for key, value in FULL_OPENRC.items(): clean_env.setenv(key, value) - rc = ImageBuilder.get_openstack_rc() + rc = imagebuilder.get_openstack_rc() assert 'api_version' not in rc assert 'no_cache' not in rc @@ -26,9 +25,9 @@ def test_unused_keys_are_not_carried_around(clean_env): def test_cacert_is_optional(clean_env): for key, value in FULL_OPENRC.items(): clean_env.setenv(key, value) - assert ImageBuilder.get_openstack_rc()['cacert'] is None + assert imagebuilder.get_openstack_rc()['cacert'] is None clean_env.setenv('OS_CACERT', '/etc/ssl/ca.pem') - assert ImageBuilder.get_openstack_rc()['cacert'] == '/etc/ssl/ca.pem' + assert imagebuilder.get_openstack_rc()['cacert'] == '/etc/ssl/ca.pem' @pytest.mark.parametrize('missing', sorted(FULL_OPENRC)) @@ -37,7 +36,7 @@ def test_each_required_variable_is_reported_by_name(clean_env, missing): if key != missing: clean_env.setenv(key, value) with pytest.raises(KeyError) as excinfo: - ImageBuilder.get_openstack_rc() + imagebuilder.get_openstack_rc() assert missing in excinfo.value.args[0] @@ -46,7 +45,7 @@ def test_all_missing_variables_are_reported_not_just_the_first(clean_env): if key not in ('OS_PASSWORD', 'OS_REGION_NAME'): clean_env.setenv(key, value) with pytest.raises(KeyError) as excinfo: - ImageBuilder.get_openstack_rc() + imagebuilder.get_openstack_rc() message = excinfo.value.args[0] assert 'OS_PASSWORD' in message assert 'OS_REGION_NAME' in message diff --git a/tests/test_main_flow.py b/tests/test_main_flow.py index faaccc1..5ead6c2 100644 --- a/tests/test_main_flow.py +++ b/tests/test_main_flow.py @@ -3,20 +3,21 @@ These drive main() with a stand-in BuildFunctions, so they cover the wiring between the pieces rather than the pieces themselves. """ +import os import tempfile import pytest +from conftest import FULL_OPENRC import image_builder.imagebuilder as ib -from conftest import FULL_OPENRC - @pytest.fixture def run_build(clean_env, monkeypatch): """Runs main() for a build, returning (exit code, propagated exception, calls).""" def run(packer=lambda: 0, keygen_ok=True, manifest='img-uuid', argv=None): + scratch = tempfile.mkdtemp(prefix='mainflow-dirs-') calls = [] class FakeBuild: @@ -25,7 +26,7 @@ def __init__(self, *a, **k): self.image_name = 'img' def find_template(self): - return '/tmp/template.pkr.hcl' + return os.path.join(scratch, 'template.pkr.hcl') def find_network_id(self, name): return 'net-1' @@ -56,14 +57,14 @@ def delete_image(self, image_id): return True def cleanup(self, secgroup_id, keypair_id): - calls.append('cleanup(sg=%s kp=%s)' % (secgroup_id, keypair_id)) + calls.append(f'cleanup(sg={secgroup_id} kp={keypair_id})') for key, value in FULL_OPENRC.items(): clean_env.setenv(key, value) - clean_env.setenv('IB_TEMPLATE_DIR', '/tmp') - clean_env.setenv('IB_DOWNLOAD_DIR', '/tmp') + clean_env.setenv('IB_TEMPLATE_DIR', scratch) + clean_env.setenv('IB_DOWNLOAD_DIR', scratch) monkeypatch.setattr(ib, 'BuildFunctions', FakeBuild) - monkeypatch.setattr(ib.ImageBuilder, 'auth', staticmethod(lambda rc: object())) + monkeypatch.setattr(ib, 'auth', lambda rc: object()) monkeypatch.setattr('sys.argv', argv or ['imagebuilder', 'build', '-n', 'img', '-a', 'bgo-default-1', '-s', 'src', '-u', 'almalinux']) From d35767657a7b482b2543639b75d8dacaaea9fdaf Mon Sep 17 00:00:00 2001 From: Trond Hasle Amundsen Date: Sun, 13 Sep 2026 22:04:10 +0200 Subject: [PATCH 14/22] refactor: split main() into run_build and run_bootstrap main() was 172 lines with 28 branches and a cyclomatic complexity of 21. It read arguments, configured logging, resolved directories and inlined both subcommand workflows, which is why neither workflow could be tested without standing up the whole thing. It is 28 lines now: parse, configure, authenticate, dispatch. The two workflows return an exit code rather than calling sys.exit, so they compose and can be called directly from a test. Returning from inside the try still runs the finally, so the cleanup guarantees are unchanged; the six main-flow tests covering interrupt, exception, keygen failure, unusable manifest, success and a failed packer run all pass untouched. Two deliberate behaviour changes, both narrowing when an error can happen: - template_dir and download_dir are now resolved only for a build. They were read before either subcommand ran, so a config without template_dir failed a bootstrap that never wanted it. - run_bootstrap cleans up its temporary directory from a finally. Previously an exception from the download left it behind. Six new tests for what the split exposed: the three image_properties variants, and run_bootstrap writing only the image id to stdout, exiting 1 on a download failure and on an upload failure, cleaning up its scratch directory in all three cases. 93 tests, all passing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A4Y4JQ39uQMSZcBYoTE84y --- image_builder/imagebuilder.py | 293 ++++++++++++++++------------------ tests/test_main_flow.py | 81 ++++++++++ 2 files changed, 223 insertions(+), 151 deletions(-) diff --git a/image_builder/imagebuilder.py b/image_builder/imagebuilder.py index c61f465..cbf6e41 100644 --- a/image_builder/imagebuilder.py +++ b/image_builder/imagebuilder.py @@ -69,9 +69,144 @@ def configure_logging(args): logging.basicConfig(format="%(message)s", level=logging.WARNING) +def resolve_directory(config, env_var, option): + """Finds a configured directory, with the environment variable winning""" + if env_var in os.environ: + return os.environ[env_var] + try: + return config.get('main', option) + except (configparser.NoSectionError, configparser.NoOptionError): + print(f"Failed to read {option} from config", file=sys.stderr) + sys.exit(1) + + +def image_properties(args): + """Hardware properties to set on the uploaded image""" + properties = {'hw_rng_model': 'virtio'} + if not args.no_scsi_mode: + properties['hw_disk_bus'] = 'scsi' + properties['hw_scsi_model'] = 'virtio-scsi' + if args.efi: + properties['hw_firmware_type'] = 'uefi' + properties['hw_machine_type'] = 'q35' + return properties + + +def run_build(args, ib_session, region, template_dir, download_dir): + """Builds an image with Packer, returning the exit code""" + build = BuildFunctions(ib_session, + region, + image_name=args.name, + avail_zone=args.availability_zone, + flavor=args.flavor, + source_image=args.source_image, + ssh_user=args.ssh_username, + provision_script=args.provision_script, + template_dir=template_dir, + download_dir=download_dir) + + # Everything that can fail without creating anything in OpenStack is done + # first, so a bad template or network name doesn't leave a security group + # and a keypair behind + template_path = build.find_template() + if not template_path: + helpers.clean_tmp_files(build.tmp_dir) + return 1 + + network_id = build.find_network_id(args.network_name) + if not network_id: + helpers.clean_tmp_files(build.tmp_dir) + return 1 + + log.info('Installing Packer plugins...') + if build.run_packer_init(template_path) != 0: + log.error('Failed to install Packer plugins') + helpers.clean_tmp_files(build.tmp_dir) + return 1 + + log.info('Creating Packer security group...') + secgroup_name, secgroup_id = build.create_security_group() + + keypair_id = None + exitcode = 1 + + # From here on something exists in OpenStack, so the rest runs under a + # finally. An interrupt during the build, or any exception on the way, has + # to still remove the security group and the keypair rather than leave them + # behind in the project + try: + log.info('Creating Packer keypair...') + key_name, keypair_id = build.create_keypairs() + if key_name is None: + return 1 + + log.info('Running Packer...') + exitcode = build.run_packer(template_path, secgroup_name, key_name, + network_id) + if exitcode == 0: + artifact_id = build.parse_manifest() + if artifact_id is None: + exitcode = 1 + else: + log.info("Successfully created image id %s", artifact_id) + if args.download: + exitcode = build.download_image(artifact_id) + else: + log.error('Build failed') + exitcode = 1 + + if args.purge_source: + if build.delete_image(args.source_image): + log.info('Successfully deleted source image') + else: + log.error('Failed to delete source image') + exitcode = 1 + finally: + log.info('Cleaning up...') + build.cleanup(secgroup_id, keypair_id) + helpers.clean_tmp_files(build.tmp_dir) + + return exitcode + + +def run_bootstrap(args, ib_session, region): + """Uploads an upstream cloud image to Glance, returning the exit code + + On success the new image id is the only thing on stdout, so the result can + be piped straight into a build. + """ + bootstrap = BootstrapFunctions(ib_session, region) + try: + log.info('Downloading image...') + image_file = bootstrap.download_and_check(args.url, + args.checksum_url, + args.checksum_digest) + if not image_file: + log.error('Downloading failed.') + return 1 + + log.info('Uploading image to Glance...') + image_id = bootstrap.create_glance_image(image_file, + args.name, + args.disk_format, + args.min_disk, + args.min_ram, + image_properties(args)) + if not image_id: + log.error('Uploading failed.') + return 1 + + sys.stdout.write(image_id) + return 0 + finally: + log.info('Cleaning up...') + helpers.clean_tmp_files(bootstrap.tmp_dir) + + def main(): commands = Commands() - configure_logging(commands.build_args or commands.bootstrap_args) + args = commands.build_args or commands.bootstrap_args + configure_logging(args) try: rc = get_openstack_rc() @@ -84,159 +219,15 @@ def main(): ib_session = auth(rc) region = rc['region_name'] - config = Config().config - if "IB_TEMPLATE_DIR" in os.environ: - template_dir = os.environ['IB_TEMPLATE_DIR'] - else: - try: - template_dir = config.get('main', 'template_dir') - except (configparser.NoSectionError, configparser.NoOptionError): - print("Failed to read template_dir from config", file=sys.stderr) - sys.exit(1) - - if "IB_DOWNLOAD_DIR" in os.environ: - download_dir = os.environ['IB_DOWNLOAD_DIR'] - else: - try: - download_dir = config.get('main', 'download_dir') - except (configparser.NoSectionError, configparser.NoOptionError): - print("Failed to read download_dir from config", file=sys.stderr) - sys.exit(1) - if commands.build_args: - image_name = commands.build_args.name - avail_zone = commands.build_args.availability_zone - flavor = commands.build_args.flavor - source_image = commands.build_args.source_image - sshuser = commands.build_args.ssh_username - provision_script = commands.build_args.provision_script - network_name = commands.build_args.network_name - - build = BuildFunctions(ib_session, - region, - image_name=image_name, - avail_zone=avail_zone, - flavor=flavor, - source_image=source_image, - ssh_user=sshuser, - provision_script=provision_script, - template_dir=template_dir, - download_dir=download_dir) - - # Everything that can fail without creating anything in OpenStack is - # done first, so a bad template or network name doesn't leave a - # security group and a keypair behind - template_path = build.find_template() - if not template_path: - helpers.clean_tmp_files(build.tmp_dir) - sys.exit(1) - - network_id = build.find_network_id(network_name) - - if not network_id: - helpers.clean_tmp_files(build.tmp_dir) - sys.exit(1) - - log.info('Installing Packer plugins...') - if build.run_packer_init(template_path) != 0: - log.error('Failed to install Packer plugins') - helpers.clean_tmp_files(build.tmp_dir) - sys.exit(1) - - log.info('Creating Packer security group...') - secgroup_name, secgroup_id = build.create_security_group() - - keypair_id = None - exitcode = 1 - - # From here on something exists in OpenStack, so the rest runs under a - # finally. An interrupt during the build, or any exception on the way, - # has to still remove the security group and the keypair rather than - # leave them behind in the project - try: - log.info('Creating Packer keypair...') - key_name, keypair_id = build.create_keypairs() - if key_name is None: - sys.exit(1) - - log.info('Running Packer...') - exitcode = build.run_packer(template_path, secgroup_name, key_name, network_id) - if exitcode == 0: - artifact_id = build.parse_manifest() - if artifact_id is None: - exitcode = 1 - else: - log.info("Successfully created image id %s", artifact_id) - if commands.build_args.download: - exitcode = build.download_image(artifact_id) - else: - log.error('Build failed') - exitcode = 1 - - if commands.build_args.purge_source: - if build.delete_image(source_image): - log.info('Successfully deleted source image') - else: - log.error('Failed to delete source image') - exitcode = 1 - finally: - log.info('Cleaning up...') - build.cleanup(secgroup_id, keypair_id) - helpers.clean_tmp_files(build.tmp_dir) - - sys.exit(exitcode) - - if commands.bootstrap_args: - image_name = commands.bootstrap_args.name - url = commands.bootstrap_args.url - checksum_url = commands.bootstrap_args.checksum_url - checksum_digest = commands.bootstrap_args.checksum_digest - disk_format = commands.bootstrap_args.disk_format - min_disk = commands.bootstrap_args.min_disk - min_ram = commands.bootstrap_args.min_ram - - properties = {} - - if not commands.bootstrap_args.no_scsi_mode: - properties['hw_disk_bus'] = 'scsi' - properties['hw_scsi_model'] = 'virtio-scsi' - - if commands.bootstrap_args.efi: - properties['hw_firmware_type'] = 'uefi' - properties['hw_machine_type'] = 'q35' - - properties['hw_rng_model'] = 'virtio' - - bootstrap = BootstrapFunctions(ib_session, region) - log.info('Downloading image...') - image_file = bootstrap.download_and_check(url, checksum_url, checksum_digest) - - if image_file: - log.info('Uploading image to Glance...') - image_id = bootstrap.create_glance_image(image_file, - image_name, - disk_format, - min_disk, - min_ram, - properties) - else: - log.error('Downloading failed.') - log.info('Cleaning up...') - helpers.clean_tmp_files(bootstrap.tmp_dir) - sys.exit(1) + # Only the build needs these, so only the build has to have them + # configured + template_dir = resolve_directory(config, 'IB_TEMPLATE_DIR', 'template_dir') + download_dir = resolve_directory(config, 'IB_DOWNLOAD_DIR', 'download_dir') + sys.exit(run_build(args, ib_session, region, template_dir, download_dir)) - if image_id: - sys.stdout.write(image_id) - else: - log.error('Uploading failed.') - log.info('Cleaning up...') - helpers.clean_tmp_files(bootstrap.tmp_dir) - sys.exit(1) - - log.info('Cleaning up...') - helpers.clean_tmp_files(bootstrap.tmp_dir) - sys.exit(0) + sys.exit(run_bootstrap(args, ib_session, region)) # vim: set ft=python3 diff --git a/tests/test_main_flow.py b/tests/test_main_flow.py index 5ead6c2..11c90c4 100644 --- a/tests/test_main_flow.py +++ b/tests/test_main_flow.py @@ -10,6 +10,7 @@ from conftest import FULL_OPENRC import image_builder.imagebuilder as ib +from image_builder.parsecommands import Commands @pytest.fixture @@ -127,3 +128,83 @@ def test_unusable_manifest_fails_the_build(run_build): code, raised, calls = run_build(manifest=None) assert code == 1 assert cleaned(calls) + + +# The split made these reachable without driving the whole of main() + +def test_image_properties_defaults_to_scsi_and_rng(): + args = Commands(['bootstrap', '-a', 'z', '-u', 'https://x/i.qcow2', '-n', 'n', + '-r', '768', '-d', '8', '-f', 'qcow2']).bootstrap_args + props = ib.image_properties(args) + assert props == {'hw_rng_model': 'virtio', 'hw_disk_bus': 'scsi', + 'hw_scsi_model': 'virtio-scsi'} + + +def test_image_properties_without_scsi(): + args = Commands(['bootstrap', '-a', 'z', '-u', 'https://x/i.qcow2', '-n', 'n', + '-r', '768', '-d', '8', '-f', 'qcow2', '-s']).bootstrap_args + props = ib.image_properties(args) + assert 'hw_disk_bus' not in props + assert props['hw_rng_model'] == 'virtio' + + +def test_image_properties_with_efi(): + args = Commands(['bootstrap', '-a', 'z', '-u', 'https://x/i.qcow2', '-n', 'n', + '-r', '768', '-d', '8', '-f', 'qcow2', '-e']).bootstrap_args + props = ib.image_properties(args) + assert props['hw_firmware_type'] == 'uefi' + assert props['hw_machine_type'] == 'q35' + + +class FakeBootstrap: + """Records what run_bootstrap asked of it.""" + + instances = [] + + def __init__(self, *a, **k): + self.tmp_dir = tempfile.mkdtemp(prefix='bootflow-') + self.downloaded = None + self.uploaded = None + FakeBootstrap.instances.append(self) + + def download_and_check(self, url, checksum_url, digest): + self.downloaded = (url, checksum_url, digest) + return None if url.endswith('fail') else os.path.join(self.tmp_dir, 'img.qcow2') + + def create_glance_image(self, *a): + self.uploaded = a + return None if a[1] == 'upload-fail' else 'new-image-uuid' + + +@pytest.fixture +def run_bootstrap(clean_env, monkeypatch, capsys): + def run(name='img', url='https://example.com/i.qcow2'): + FakeBootstrap.instances = [] + monkeypatch.setattr(ib, 'BootstrapFunctions', FakeBootstrap) + args = Commands(['bootstrap', '-a', 'z', '-u', url, '-n', name, + '-r', '768', '-d', '8', '-f', 'qcow2']).bootstrap_args + code = ib.run_bootstrap(args, object(), 'bgo') + return code, capsys.readouterr(), FakeBootstrap.instances[0] + + return run + + +def test_bootstrap_writes_only_the_image_id_to_stdout(run_bootstrap): + code, captured, fake = run_bootstrap() + assert code == 0 + assert captured.out == 'new-image-uuid' + assert not os.path.exists(fake.tmp_dir) # cleaned up + + +def test_bootstrap_download_failure_exits_one_and_cleans_up(run_bootstrap): + code, captured, fake = run_bootstrap(url='https://example.com/fail') + assert code == 1 + assert captured.out == '' + assert not os.path.exists(fake.tmp_dir) + + +def test_bootstrap_upload_failure_exits_one_and_cleans_up(run_bootstrap): + code, captured, fake = run_bootstrap(name='upload-fail') + assert code == 1 + assert captured.out == '' + assert not os.path.exists(fake.tmp_dir) From b8b0eb4cbe83088a957c53059018eac0c2d0cf27 Mon Sep 17 00:00:00 2001 From: Trond Hasle Amundsen Date: Fri, 18 Sep 2026 09:28:59 +0200 Subject: [PATCH 15/22] fix: fail early when a legacy template has no openstack plugin A build against a legacy JSON template failed deep inside packer with "the builder openstack is unknown by Packer", and by then the temporary security group and keypair had already been created. run_packer_init() skipped legacy templates entirely, on the grounds that only HCL2 can declare required_plugins and so there is nothing to install from. That is true, but returning 0 told the caller everything was ready. It now checks whether the plugin is installed at all, via 'packer plugins installed', and fails with the instruction to either install it by hand or migrate the template. 'packer plugins installed' exits 0 whether or not anything is there, so the answer comes from its output. Reproduced both halves against Packer 1.16.0 to be sure which case this is: with no plugin installed, a legacy JSON template gives "Failed to initialize build openstack ... unknown by Packer", while the HCL2 template gives "Missing plugins ... Did you run packer init for this project?". Only the first matches the reported failure. S607 joins the ignore list: resolving packer and ssh-keygen through PATH is deliberate, so that a chosen packer build or an activated virtualenv is the one that runs. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A4Y4JQ39uQMSZcBYoTE84y --- image_builder/build.py | 32 +++++++++++++++++++++++++++++--- pyproject.toml | 4 +++- tests/test_build.py | 38 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 68 insertions(+), 6 deletions(-) diff --git a/image_builder/build.py b/image_builder/build.py index 59426c6..9e80a64 100644 --- a/image_builder/build.py +++ b/image_builder/build.py @@ -212,16 +212,42 @@ def run_packer(self, template_path, secgroup_name, key_name, network_id): helpers.log_subprocess_output(process.stdout) return process.wait() + def openstack_plugin_installed(self): + """Whether Packer can already see the openstack builder + + 'packer plugins installed' exits 0 whether or not anything is there, so + the answer is in the output rather than the status. + """ + try: + installed = subprocess.run(['packer', 'plugins', 'installed'], + capture_output=True, text=True, + check=False) + except OSError as error: + log.error("Could not run packer: %s", error) + return False + log.debug(installed.stdout) + return 'openstack' in installed.stdout + def run_packer_init(self, template_path): """Installs the Packer plugins required by the template Packer does not bundle builder plugins anymore, so the openstack builder has to be installed before the build. Only HCL2 templates can - declare required_plugins, so this is a no-op for a legacy JSON - template. + declare required_plugins, so for a legacy JSON template there is + nothing to install from: all this can do is check that someone already + installed it by hand, and say so now rather than let packer fail with + 'the builder openstack is unknown' once the temporary security group + and keypair already exist. """ if not template_path.endswith('.pkr.hcl'): - return 0 + if self.openstack_plugin_installed(): + return 0 + log.error("The openstack builder is not installed, and %s is a " + "legacy JSON template so it cannot declare it. Either " + "run 'packer plugins install " + "github.com/hashicorp/openstack', or migrate to " + "template.pkr.hcl", template_path) + return 1 cmd = ['packer', 'init', template_path] log.debug(cmd) with subprocess.Popen(cmd, stdout=subprocess.PIPE, diff --git a/pyproject.toml b/pyproject.toml index a6ae6b1..277f63d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -55,7 +55,9 @@ lint.ignore = [ "E501", "S310", # urlopen: the URLs come from the operator running the command "S603", # subprocess without shell: the argv is built here, not user input + "S607", # packer and ssh-keygen are deliberately resolved through PATH, + # so a virtualenv or a chosen packer build can be picked up ] [tool.ruff.lint.per-file-ignores] -"tests/*" = ["S101", "S603", "S607"] # asserts and subprocess are the point +"tests/*" = ["S101"] # asserts are the point of a test diff --git a/tests/test_build.py b/tests/test_build.py index c8ca793..2eedd4a 100644 --- a/tests/test_build.py +++ b/tests/test_build.py @@ -182,11 +182,45 @@ def test_missing_template_returns_none(build): assert build.find_template() is None -def test_packer_init_is_skipped_for_a_legacy_template(build): - """required_plugins is HCL2 only, so there is nothing for init to do.""" +def test_legacy_template_passes_when_the_plugin_is_already_installed(build, monkeypatch): + """required_plugins is HCL2 only, so init has nothing to install from.""" + monkeypatch.setattr(type(build), 'openstack_plugin_installed', + lambda self: True) assert build.run_packer_init('/some/path/template') == 0 +def test_legacy_template_fails_early_when_the_plugin_is_missing(build, monkeypatch): + """Better here than after the security group and keypair exist.""" + monkeypatch.setattr(type(build), 'openstack_plugin_installed', + lambda self: False) + assert build.run_packer_init('/some/path/template') == 1 + + +@pytest.mark.parametrize('stdout,expected', [ + pytest.param('', False, id='nothing-installed'), + pytest.param('/home/u/.config/packer/plugins/github.com/hashicorp/openstack/' + 'packer-plugin-openstack_v1.1.4_x5.0_linux_amd64\n', True, + id='openstack-present'), + pytest.param('/home/u/.config/packer/plugins/github.com/hashicorp/qemu/' + 'packer-plugin-qemu_v1.1.0_x5.0_linux_amd64\n', False, + id='other-plugin-only'), +]) +def test_plugin_detection_reads_the_output_not_the_exit_code(build, monkeypatch, + stdout, expected): + monkeypatch.setattr(subprocess, 'run', + lambda *a, **k: types.SimpleNamespace(stdout=stdout, + returncode=0)) + assert build.openstack_plugin_installed() is expected + + +def test_missing_packer_binary_is_reported_not_raised(build, monkeypatch): + def no_packer(*a, **k): + raise FileNotFoundError('packer') + + monkeypatch.setattr(subprocess, 'run', no_packer) + assert build.openstack_plugin_installed() is False + + # ------------------------------------------------------------ packer argv def test_packer_argv_supplies_every_declared_variable(build, monkeypatch, repo_root): From b4a98c2f233b45aec4b7d033330d873a0578f94a Mon Sep 17 00:00:00 2001 From: Trond Hasle Amundsen Date: Sun, 20 Sep 2026 18:20:32 +0200 Subject: [PATCH 16/22] feat: add build-images.sh for running builds sequentially One cron job per image is hard to keep track of: each has its own randomised schedule, and nothing ties a run together or notices that one of them failed. This runs the generated build scripts one after another instead. Categories are data rather than code. The build order, the pattern that matches a build script name, the template directory imagebuilder should use and the option that selects the category all live in one place, so adding a kind of image does not mean another array, another flag and another loop. Selection is a single map from image name to category, shared by discovery and -i, so the two cannot disagree about what a name means. Built for unattended use: it exits non-zero if any build failed, naming them, and an unknown option or an unusable -t is an error rather than a quiet success. A typo in -i is rejected before anything is built, instead of running a path that does not exist and leaving a report file behind for an image nobody has. Output drops its colours when stdout is not a terminal, so a cron log does not fill with escape sequences. A flock means two runs cannot compete for the same temporary security groups and keypairs, taken after -l and -h so that looking at the list never blocks. Windows images are listed and selected but not built yet, as before. The paths are overridable so the script can be exercised away from a builder node. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A4Y4JQ39uQMSZcBYoTE84y --- build-images.sh | 397 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 397 insertions(+) create mode 100755 build-images.sh diff --git a/build-images.sh b/build-images.sh new file mode 100755 index 0000000..821adb2 --- /dev/null +++ b/build-images.sh @@ -0,0 +1,397 @@ +#!/usr/bin/bash +# +# Build NREC images sequentially, one after another, in place of a cron job +# per image. +# +# The colour palette and the XY/line/cnt helpers below are kept whole for +# future use, so unused variables and uncalled functions are expected here +# shellcheck disable=SC2034,SC2329 + +# Continue past a failed build, but treat unset variables and broken pipes as +# errors. Not -e: one image failing must not stop the rest of the run +set -uo pipefail + +#--------------------------------------------------------------------+ +#Color picker, usage: printf ${BLD}${CUR}${RED}${BBLU}"Hello!)"${DEF}| +#-------------------------+--------------------------------+---------+ +# Text color | Background color | | +#-----------+-------------+--------------+-----------------+ | +# Base color|Lighter shade| Base color | Lighter shade | | +#-----------+-------------+--------------+-----------------+ | +BLK='\e[30m'; blk='\e[90m'; BBLK='\e[40m'; bblk='\e[100m' #| Black | +RED='\e[31m'; red='\e[91m'; BRED='\e[41m'; bred='\e[101m' #| Red | +GRN='\e[32m'; grn='\e[92m'; BGRN='\e[42m'; bgrn='\e[102m' #| Green | +YLW='\e[33m'; ylw='\e[93m'; BYLW='\e[43m'; bylw='\e[103m' #| Yellow | +BLU='\e[34m'; blu='\e[94m'; BBLU='\e[44m'; bblu='\e[104m' #| Blue | +MGN='\e[35m'; mgn='\e[95m'; BMGN='\e[45m'; bmgn='\e[105m' #| Magenta | +CYN='\e[36m'; cyn='\e[96m'; BCYN='\e[46m'; bcyn='\e[106m' #| Cyan | +WHT='\e[37m'; wht='\e[97m'; BWHT='\e[47m'; bwht='\e[107m' #| White | +#----------------------------------------------------------+---------+ +# Effects | +#--------------------------------------------------------------------+ +DEF='\e[0m' #Default color and effects | +BLD='\e[1m' #Bold\brighter | +DIM='\e[2m' #Dim\darker | +CUR='\e[3m' #Italic font | +UND='\e[4m' #Underline | +INV='\e[7m' #Inverted | +COF='\e[?25l' #Cursor Off | +CON='\e[?25h' #Cursor On | +#--------------------------------------------------------------------+ +# Text positioning, usage: XY 10 10 "Hello World!" | +# shellcheck disable=SC2059 +XY () { printf "\e[${2};${1}H${3}"; } # | +#--------------------------------------------------------------------+ +# Print line, usage: line - 10 | line -= 20 | line "Hello World!" 20 | +# shellcheck disable=SC2059 +line () { printf -v LINE "%$2s"; printf -- "${LINE// /$1}"; } # | +# Create sequence like {0..X} | +# shellcheck disable=SC2059,SC2086,SC2206 +cnt () { printf -v _N %$1s; _N=(${_N// / 1}); printf "${!_N[*]}"; } #| +#--------------------------------------------------------------------+ + +# Under cron stdout is a pipe, and the escape sequences above would end up +# verbatim in the log and in the mail. Blank the palette unless we are on a +# terminal +if [ ! -t 1 ]; then + for _colour in BLK blk BBLK bblk RED red BRED bred GRN grn BGRN bgrn \ + YLW ylw BYLW bylw BLU blu BBLU bblu MGN mgn BMGN bmgn \ + CYN cyn BCYN bcyn WHT wht BWHT bwht \ + DEF BLD DIM CUR UND INV COF CON; do + printf -v "$_colour" '%s' '' + done + unset _colour +fi + +# Set path +PATH=/usr/bin:/usr/sbin +export PATH + +# Variables. Overridable so the script can be exercised outside a builder node +BUILD_PATH=${BUILD_PATH:-/home/imagebuilder/build_scripts} +LOG_PATH=${LOG_PATH:-/var/log/imagebuilder} +TEMPLATE_ROOT=${TEMPLATE_ROOT:-/etc/imagebuilder} +LOCK_FILE=${LOCK_FILE:-${LOG_PATH}/build-images.lock} + +#--------------------------------------------------------------------- +# Image categories +# +# Everything that differs between kinds of image lives here. Adding a +# category means adding a line to each map below and a letter to the getopts +# string, rather than another array, another flag and another build loop. +#--------------------------------------------------------------------- + +# Build order +CATEGORIES=(std uib gpu win) + +# How a build script name maps to a category. std has no pattern: it is the +# fallback for anything the others do not claim +declare -A CATEGORY_PATTERN=( + [uib]='_uib_puppet$' + [gpu]='_nv_vgpu$' + [win]='^winsrv_.*_wrapper$' +) + +# The template directory imagebuilder should use +declare -A CATEGORY_TEMPLATE=( + [std]="${TEMPLATE_ROOT}/default" + [uib]="${TEMPLATE_ROOT}/uib_puppet" + [gpu]="${TEMPLATE_ROOT}/nv_vgpu" + [win]="${TEMPLATE_ROOT}/default" +) + +# What -l and the summary call them +declare -A CATEGORY_LABEL=( + [std]='Standard images' + [uib]='UiB images' + [gpu]='vGPU images' + [win]='Windows images' +) + +# The option that selects a whole category +declare -A OPTION_CATEGORY=( + [S]=std + [B]=uib + [G]=gpu + [W]=win +) + +# Windows builds are not wired up yet. Listing and selecting them works, so +# that turning this off is the only change needed later +WIN_PLACEHOLDER=1 + +# Every build script found, and which category it belongs to. One map rather +# than an array per category: bash has no arrays of arrays, and the nameref +# tricks that fake them are not worth the reading +declare -A IMAGE_CATEGORY=() +declare -a ALL_IMAGES=() + +# Images to build, and the categories selected wholesale with -S/-B/-G/-W +declare -a SELECTED=() +declare -A SELECT_ALL=() + +# Option variables +LIST=0 +DRYRUN=0 +TIMEOUT=0 + +# Every -i given, kept so they can be checked before anything is built +requested_images=() + +# Outcomes, for the summary and the exit code +built_images=() +failed_images=() +skipped_images=() + +#--------------------------------------------------------------------- +# Functions +#--------------------------------------------------------------------- + +# Which category a build script name belongs to. Used for both discovery and +# -i, so the two cannot disagree about what a name means +categorise() { + local name=$1 cat + for cat in "${CATEGORIES[@]}"; do + [ -n "${CATEGORY_PATTERN[$cat]:-}" ] || continue + if [[ $name =~ ${CATEGORY_PATTERN[$cat]} ]]; then + printf '%s' "$cat" + return + fi + done + printf 'std' +} + +# Seconds as 1h02m03s, dropping the units that are zero +duration() { + local total=$1 + if [ "$total" -ge 3600 ]; then + printf '%dh%02dm%02ds' $((total / 3600)) $((total % 3600 / 60)) $((total % 60)) + elif [ "$total" -ge 60 ]; then + printf '%dm%02ds' $((total / 60)) $((total % 60)) + else + printf '%ds' "$total" + fi +} + +usage() { + echo "Usage: $0 [-h] [-S] [-B] [-G] [-W] [-t ] [-l] [-i ]..." + echo + echo 'Options:' + echo ' -S Build all standard images. Negates -i options' + echo ' -B Build all UiB images. Negates -i options' + echo ' -G Build all vgpu images. Negates -i options' + echo ' -W Build all Windows images. Negates -i options' + echo ' -i Specify image to build. Can be used multiple times' + echo ' -t Terminate build job if still running after this duration. See timeout(1)' + echo ' -l List all images and exit' + echo ' -n Dry-run: Only show what would be done' + echo ' -h Display help' + echo +} + +function build_image() { + local image=$1 + local exitval start elapsed + printf '%b>>>>>>>>>>>>>>>> BUILDING IMAGE >>%b %b%s %b[timeout: %s]%b\n' \ + "${BMGN}${BLD}${wht}" "$DEF" "${cyn}${BLD}" "$image" "${DEF}${DIM}" "$TIMEOUT" "$DEF" + if [ $DRYRUN -eq 0 ]; then + start=$SECONDS + timeout "$TIMEOUT" "${BUILD_PATH}/${image}" + exitval=$? + elapsed=$(duration $((SECONDS - start))) + if [ $exitval -eq 0 ]; then + printf '[%bSUCCESS%b] Build completed in %s: %b%s%b\n' \ + "${grn}${BLD}" "$DEF" "$elapsed" "$BLD" "$image" "$DEF" + built_images+=("$image") + elif [ $exitval -eq 124 ]; then + printf '[%bERROR%b] TIMEOUT: Build timed out after %s: %b%s%b\n' \ + "${RED}${BLD}" "$DEF" "$TIMEOUT" "$BLD" "$image" "$DEF" >&2 + jq -nc '{"result": "failed"}' >> "${LOG_PATH}/${image}-report.jsonl" + failed_images+=("$image") + else + printf '[%bERROR%b] Build failed after %s (exit %s): %b%s%b\n' \ + "${RED}${BLD}" "$DEF" "$elapsed" "$exitval" "$BLD" "$image" "$DEF" >&2 + jq -nc '{"result": "failed"}' >> "${LOG_PATH}/${image}-report.jsonl" + failed_images+=("$image") + fi + else + printf '%b(DRY-RUN)%b %btimeout %s %s/%s%b\n' \ + "$GRN" "$DEF" "$DIM" "$TIMEOUT" "$BUILD_PATH" "$image" "$DEF" + built_images+=("$image") + fi + echo +} + +#--------------------------------------------------------------------- +# Find images +#--------------------------------------------------------------------- + +if [ ! -d "$BUILD_PATH" ]; then + printf 'No build scripts directory: %s\n' "$BUILD_PATH" >&2 + exit 1 +fi + +# A glob rather than $(ls): a build script whose name contains a space used to +# be split into two images that do not exist +for script_path in "$BUILD_PATH"/*; do + [ -f "$script_path" ] || continue + script=${script_path##*/} + IMAGE_CATEGORY[$script]=$(categorise "$script") + ALL_IMAGES+=("$script") +done + +#--------------------------------------------------------------------- +# Options +#--------------------------------------------------------------------- + +# Display usage and exit with error if no options given +if [ $# -eq 0 ]; then + usage >&2 + exit 1 +fi + +while getopts "SBGWlnhi:t:" arg; do + case $arg in + S|B|G|W) + SELECT_ALL[${OPTION_CATEGORY[$arg]}]=1 + ;; + i) + requested_images+=("$OPTARG") + SELECTED+=("$OPTARG") + ;; + t) + # timeout(1) takes a number with an optional s/m/h/d suffix. Catch + # a bad one now rather than once per build + if ! [[ $OPTARG =~ ^[0-9]+([.][0-9]+)?[smhd]?$ ]]; then + printf 'Invalid timeout: %s. See timeout(1)\n' "$OPTARG" >&2 + exit 1 + fi + TIMEOUT=$OPTARG + ;; + l) + LIST=1 + ;; + n) + DRYRUN=1 + ;; + h) + usage + exit 0 + ;; + *) + # An unknown option is an error, so that a typo in a cron entry or + # a unit file does not look like a successful run + usage >&2 + exit 1 + ;; + esac +done + +# Display all images and exit +if [ $LIST -eq 1 ]; then + for cat in "${CATEGORIES[@]}"; do + printf '%s:\n' "${CATEGORY_LABEL[$cat]}" + for image in "${ALL_IMAGES[@]}"; do + if [ "${IMAGE_CATEGORY[$image]}" = "$cat" ]; then + printf ' %s\n' "$image" + fi + done + done + exit 0 +fi + +# Check every -i before building anything. A typo used to run a path that does +# not exist, report it as a failed build, and leave a report file behind for an +# image nobody has +unknown_images=() +for image in "${requested_images[@]}"; do + if [ ! -f "${BUILD_PATH}/${image}" ]; then + unknown_images+=("$image") + fi +done +if [ ${#unknown_images[@]} -gt 0 ]; then + printf 'No such image: %s\n' "${unknown_images[@]}" >&2 + printf "Run '%s -l' for the images that exist\n" "$0" >&2 + exit 1 +fi + +# -S, -B, -G and -W replace any -i for that category +for cat in "${CATEGORIES[@]}"; do + [ -n "${SELECT_ALL[$cat]:-}" ] || continue + keep=() + for image in "${SELECTED[@]}"; do + if [ "${IMAGE_CATEGORY[$image]}" != "$cat" ]; then + keep+=("$image") + fi + done + for image in "${ALL_IMAGES[@]}"; do + if [ "${IMAGE_CATEGORY[$image]}" = "$cat" ]; then + keep+=("$image") + fi + done + SELECTED=("${keep[@]}") +done + +#--------------------------------------------------------------------- +# MAIN +#--------------------------------------------------------------------- + +# One run at a time. These builds share a project, and two runs would compete +# for the temporary security groups and keypairs. Taken after -l and -h so +# that looking at the list never blocks +if ! exec 9>"$LOCK_FILE"; then + printf 'Cannot open lock file: %s\n' "$LOCK_FILE" >&2 + exit 1 +fi +if ! flock -n 9; then + printf 'Another run is already in progress (%s)\n' "$LOCK_FILE" >&2 + exit 1 +fi + +export IMAGEBUILDER_REPORT=true + +run_start=$SECONDS + +for cat in "${CATEGORIES[@]}"; do + batch=() + for image in "${SELECTED[@]}"; do + if [ "${IMAGE_CATEGORY[$image]}" = "$cat" ]; then + batch+=("$image") + fi + done + [ ${#batch[@]} -gt 0 ] || continue + + export IB_TEMPLATE_DIR="${CATEGORY_TEMPLATE[$cat]}" + for img in "${batch[@]}"; do + if [ "$cat" = win ] && [ $WIN_PLACEHOLDER -eq 1 ]; then + # Do NOT build these if cron failed. + export PLACEHOLDER=true + echo "Do NOT build $img! Poke Tor if cron job failed" + skipped_images+=("$img") + else + build_image "$img" + fi + done +done + +#--------------------------------------------------------------------- +# Summary +#--------------------------------------------------------------------- + +printf '%b%s%b %d built, %d failed, %d skipped in %s\n' \ + "$BLD" 'Summary:' "$DEF" \ + "${#built_images[@]}" "${#failed_images[@]}" "${#skipped_images[@]}" \ + "$(duration $((SECONDS - run_start)))" + +if [ ${#skipped_images[@]} -gt 0 ]; then + printf ' %bskipped:%b %s\n' "$YLW" "$DEF" "${skipped_images[*]}" +fi + +# Report what failed and exit non-zero, so that cron, a systemd unit or a +# monitoring check can tell a bad run from a good one +if [ ${#failed_images[@]} -gt 0 ]; then + printf ' %bfailed:%b %s\n' "${RED}${BLD}" "$DEF" "${failed_images[*]}" >&2 + exit 1 +fi + +exit 0 From 1f8e10eff11384e70737da893537566ecdc05d8c Mon Sep 17 00:00:00 2001 From: Trond Hasle Amundsen Date: Sun, 20 Sep 2026 18:56:44 +0200 Subject: [PATCH 17/22] fix: stream image downloads instead of holding them in memory Downloads died on a builder node with "terminated by signal 9" just after Glance returned the image. It was the OOM killer. openstacksdk's download_image defaults to stream=False, which asks requests for the whole response body up front. The chunked writing that follows is then only copying from memory to disk: the entire image is already resident. A 10 GiB image needs upwards of 20 GiB of RAM that way. Measured on a 400 MiB body, peak RSS is 837 MiB with stream=False against 34 MiB with stream=True. This came in with the move off the glance CLI. That CLI streamed to disk, and the API call that replaced it took the default and did not. The upload path in bootstrap is unaffected: openstacksdk hands create_image an open file object, which requests streams. The test for this asserts the argument rather than the behaviour, since a fake cannot run out of memory, and it was checked to fail without the fix. The earlier fake accepted any keyword arguments and so could never have caught this. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A4Y4JQ39uQMSZcBYoTE84y --- image_builder/build.py | 9 ++++++++- tests/conftest.py | 2 ++ tests/test_build.py | 10 ++++++++++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/image_builder/build.py b/image_builder/build.py index 9e80a64..f8cb10e 100644 --- a/image_builder/build.py +++ b/image_builder/build.py @@ -114,6 +114,11 @@ def download_image(self, artifact_id): Straight through the API on the session we already hold. Shelling out to the glance CLI meant a second, independent authentication from the OS_* environment, and that CLI is deprecated upstream. + + stream=True is load bearing. Without it openstacksdk asks requests for + the whole response body up front, so the entire image is held in + memory before a byte of it is written, and a large image takes the + process out via the OOM killer. """ timestr = time.strftime("%Y%m%d") filename = f'{self.image_name}-{timestr}.qcow2' @@ -121,7 +126,9 @@ def download_image(self, artifact_id): log.info("Downloading image to %s...", target) try: with open(target, 'wb') as image_file: - self.conn.image.download_image(artifact_id, output=image_file) + self.conn.image.download_image(artifact_id, + output=image_file, + stream=True) except Exception as error: log.error("Failed to download image %s: %s", artifact_id, error) if os.path.exists(target): diff --git a/tests/conftest.py b/tests/conftest.py index 3d4c4ea..933db88 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -54,6 +54,7 @@ class FakeImage: def __init__(self, log, fail=False): self.log = log self.fail = fail + self.download_kwargs = None def create_image(self, **kw): self.log.append(('image_created', kw)) @@ -63,6 +64,7 @@ def create_image(self, **kw): def download_image(self, image, output=None, **kw): self.log.append(('image_downloaded', image)) + self.download_kwargs = kw if self.fail: raise RuntimeError('glance is unhappy') output.write(b'qcow2 data') diff --git a/tests/test_build.py b/tests/test_build.py index 2eedd4a..70570fe 100644 --- a/tests/test_build.py +++ b/tests/test_build.py @@ -256,3 +256,13 @@ def wait(self): if line.startswith('variable "'): declared.add(line.split('"')[1]) assert supplied == declared + + +def test_download_streams_rather_than_buffering_the_whole_image(build): + """Without stream=True openstacksdk reads the entire image into memory. + + A 10 GiB image then needs more than 20 GiB of RAM and the process is + killed, which is exactly what happened on a builder node. + """ + build.download_image('img-uuid') + assert build.conn.image.download_kwargs.get('stream') is True From 227998ff52bc8cea73b0bbbddfea133ff0d53ce4 Mon Sep 17 00:00:00 2001 From: Trond Hasle Amundsen Date: Sun, 20 Sep 2026 19:19:26 +0200 Subject: [PATCH 18/22] feat: sweep temporary resources left behind by runs that were killed A build killed outright cannot run its own cleanup. The OOM kills during image download left an imagebuilder- security group and keypair in the project every time, and nothing was ever going to remove them: SIGKILL cannot be caught, and SIGTERM does not run a finally block either. Every build now starts by clearing out what earlier runs could not. Two things keep this from deleting something in use. Only names shaped exactly like imagebuilder- followed by 32 hex characters are considered, so a security group called default or somebody's keypair is never a candidate. And nothing younger than a day is touched, because the builder nodes share one project and a sweep here would otherwise delete resources out from under a build running on another node. Images are deliberately left alone. They are named after the image being built, so a leftover cannot be told apart from a good one, and deleting the wrong one would destroy a published image. The download directory is left alone too, since something else already tidies that. Failures during the sweep are logged and the build carries on: not being able to list keypairs is no reason to refuse to build. Removals are logged at warning level so they show up without -v. --no-sweep turns it off, for when something needs to be preserved for inspection. Nova does not always report created_at when listing keypairs, so the age is fetched for the individual keypair if the listing did not carry it. Anything whose age still cannot be read is left alone rather than guessed at. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A4Y4JQ39uQMSZcBYoTE84y --- CLAUDE.md | 6 +- TODO.md | 4 +- image_builder/build.py | 113 +++++++++++++++++++++++++++++++++ image_builder/imagebuilder.py | 5 ++ image_builder/parsecommands.py | 5 ++ tests/conftest.py | 14 ++++ tests/test_build.py | 112 ++++++++++++++++++++++++++++++++ tests/test_main_flow.py | 19 ++++++ 8 files changed, 276 insertions(+), 2 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index cff7244..565c126 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -84,7 +84,11 @@ Everything after the first OpenStack resource is created runs under a `try/final reached on exceptions and on Ctrl-C (SIGINT becomes `KeyboardInterrupt`, which unwinds). `cleanup()` is written for that position: it skips ids that are `None` and swallows API errors, because raising there would mask the original failure. SIGTERM and SIGKILL still bypass it — verified — so those remain the -one case that orphans `imagebuilder-*` resources. +one case that orphans `imagebuilder-*` resources. `sweep_orphans()` runs at the start of every +build to clear those up: only names matching `imagebuilder-<32 hex>`, only once older than +`SWEEP_MAX_AGE_HOURS` (builder nodes share a project, so the age is what stops it deleting a +resource a build elsewhere is using), and never images, whose names are indistinguishable from +legitimate ones. **`bootstrap`** (`bootstrap.py:BootstrapFunctions`) — downloads a cloud image from a URL, optionally verifies it against a checksum file (substring match of the computed hexdigest in the fetched file), and diff --git a/TODO.md b/TODO.md index 8ce3ac2..a06d0e6 100644 --- a/TODO.md +++ b/TODO.md @@ -2,7 +2,9 @@ * Isn't very good at the moment * Handle SIGTERM (clean up before dying). Exceptions and Ctrl-C already unwind through the try/finally in main(), but a SIGTERM kills the process - without running it, so those builds still leak a security group and keypair + without running it, so those builds still leak a security group and + keypair. sweep_orphans() clears them up on a later run, but only once they + are a day old * Post-process images * Convert image from raw to qcow2 after download diff --git a/image_builder/build.py b/image_builder/build.py index f8cb10e..f55f2c6 100644 --- a/image_builder/build.py +++ b/image_builder/build.py @@ -1,6 +1,8 @@ +import datetime import json import logging import os +import re import subprocess import time import uuid @@ -11,6 +13,15 @@ log = logging.getLogger(__name__) +# The temporary security group and keypair are named imagebuilder-. +# Only names of exactly this shape are ever removed by the sweep +TEMP_NAME = re.compile(r'^imagebuilder-[0-9a-f]{32}$') + +# Nothing younger than this is swept. Builder nodes share one project, so the +# age is what stops a sweep here from deleting resources out from under a +# build running somewhere else +SWEEP_MAX_AGE_HOURS = 24 + class BuildFunctions: def __init__(self, session, @@ -59,6 +70,108 @@ def cleanup(self, secgroup_id, keypair_id): except Exception as error: log.warning("Failed to remove keypair %s: %s", keypair_id, error) + @staticmethod + def resource_age_hours(created_at): + """Hours since an OpenStack timestamp, or None if it cannot be read + + Neutron and nova do not agree on the format, and python 3.9 cannot + parse a trailing Z, so this is more forgiving than fromisoformat. + """ + if not created_at: + return None + text = str(created_at).strip() + if text.endswith('Z'): + text = text[:-1] + '+00:00' + try: + stamp = datetime.datetime.fromisoformat(text) + except (TypeError, ValueError): + return None + if stamp.tzinfo is None: + stamp = stamp.replace(tzinfo=datetime.timezone.utc) + now = datetime.datetime.now(datetime.timezone.utc) + return (now - stamp).total_seconds() / 3600 + + def sweep_orphans(self, max_age_hours=SWEEP_MAX_AGE_HOURS): + """Removes temporary resources left behind by runs that were killed + + A build that dies outright, from the OOM killer or a SIGKILL, cannot + run its own cleanup, so its security group and keypair stay in the + project forever. Only names shaped exactly like imagebuilder- + are considered, and only once they are older than max_age_hours. + + Images are deliberately not swept. They are named after the image + being built, so a leftover cannot be told apart from a good one, and + deleting the wrong one would destroy a published image. The download + directory is left alone too: something else tidies that. + """ + removed = self._sweep_security_groups(max_age_hours) + removed += self._sweep_keypairs(max_age_hours) + if removed: + log.warning("Removed %d orphaned resource(s) from earlier runs", removed) + return removed + + def _sweep_security_groups(self, max_age_hours): + try: + groups = list(self.conn.network.security_groups()) + except Exception as error: + log.warning("Could not list security groups to sweep: %s", error) + return 0 + removed = 0 + for group in groups: + if not TEMP_NAME.match(group.name or ''): + continue + age = self.resource_age_hours(group.created_at) + if age is None: + log.info("Leaving security group %s alone: no usable timestamp", + group.name) + continue + if age < max_age_hours: + continue + log.warning("Removing orphaned security group %s, %.0f hours old", + group.name, age) + try: + self.conn.network.delete_security_group(group.id) + removed += 1 + except Exception as error: + log.warning("Could not remove security group %s: %s", + group.name, error) + return removed + + def _sweep_keypairs(self, max_age_hours): + try: + keypairs = list(self.conn.compute.keypairs()) + except Exception as error: + log.warning("Could not list keypairs to sweep: %s", error) + return 0 + removed = 0 + for keypair in keypairs: + if not TEMP_NAME.match(keypair.name or ''): + continue + created = keypair.created_at + if not created: + # Nova does not always report created_at when listing, so ask + # for the one keypair rather than guess at its age + try: + created = self.conn.compute.get_keypair(keypair.name).created_at + except Exception as error: + log.info("Could not read the age of keypair %s: %s", + keypair.name, error) + age = self.resource_age_hours(created) + if age is None: + log.info("Leaving keypair %s alone: no usable timestamp", + keypair.name) + continue + if age < max_age_hours: + continue + log.warning("Removing orphaned keypair %s, %.0f hours old", + keypair.name, age) + try: + self.conn.compute.delete_keypair(keypair.name) + removed += 1 + except Exception as error: + log.warning("Could not remove keypair %s: %s", keypair.name, error) + return removed + def create_keypairs(self): """Creates a temporary keypair""" diff --git a/image_builder/imagebuilder.py b/image_builder/imagebuilder.py index cbf6e41..e8c753c 100644 --- a/image_builder/imagebuilder.py +++ b/image_builder/imagebuilder.py @@ -124,6 +124,11 @@ def run_build(args, ib_session, region, template_dir, download_dir): helpers.clean_tmp_files(build.tmp_dir) return 1 + # Before creating anything, clear out what earlier runs could not. A build + # killed by the OOM killer never reaches its own cleanup + if not args.no_sweep: + build.sweep_orphans() + log.info('Creating Packer security group...') secgroup_name, secgroup_id = build.create_security_group() diff --git a/image_builder/parsecommands.py b/image_builder/parsecommands.py index 33c5aa0..0346995 100644 --- a/image_builder/parsecommands.py +++ b/image_builder/parsecommands.py @@ -47,6 +47,11 @@ def __init__(self, argv=None): help='Be verbose (default is no output)', action='store_true', default=False) + build.add_argument('--no-sweep', + help='Do not remove temporary security groups and ' + 'keypairs left behind by runs that were killed', + action='store_true', + default=False) build.add_argument('-x', '--purge-source', help='Purge source image you are building from', action='store_true', diff --git a/tests/conftest.py b/tests/conftest.py index 933db88..f107f0f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -22,6 +22,7 @@ class FakeNetwork: def __init__(self, log): self.log = log self.networks_result = [] + self.groups = [] def create_security_group(self, **kw): self.log.append(('sg_created', kw)) @@ -37,10 +38,14 @@ def networks(self, **kw): self.log.append(('networks_listed', kw)) return iter(self.networks_result) + def security_groups(self, **kw): + return iter(self.groups) + class FakeCompute: def __init__(self, log): self.log = log + self.pairs = [] def create_keypair(self, **kw): self.log.append(('kp_created', kw)) @@ -49,6 +54,15 @@ def create_keypair(self, **kw): def delete_keypair(self, keypair): self.log.append(('kp_deleted', keypair)) + def keypairs(self, **kw): + return iter(self.pairs) + + def get_keypair(self, name): + for pair in self.pairs: + if pair.name == name: + return pair + raise RuntimeError(f'no such keypair: {name}') + class FakeImage: def __init__(self, log, fail=False): diff --git a/tests/test_build.py b/tests/test_build.py index 70570fe..f276410 100644 --- a/tests/test_build.py +++ b/tests/test_build.py @@ -1,4 +1,5 @@ """The build path: resource lifecycle, manifest handling, image download.""" +import datetime import io import json import os @@ -266,3 +267,114 @@ def test_download_streams_rather_than_buffering_the_whole_image(build): """ build.download_image('img-uuid') assert build.conn.image.download_kwargs.get('stream') is True + + +# --------------------------------------------------------------- sweeping +# +# Resources left behind by a run that was killed outright. The name shape and +# the age are the only things standing between this and deleting something a +# build on another node is using. + +def stamp(hours_ago): + when = datetime.datetime.now(datetime.timezone.utc) - datetime.timedelta(hours=hours_ago) + return when.strftime('%Y-%m-%dT%H:%M:%SZ') + + +def fake_group(name, hours_ago): + return type('SG', (), {'id': 'id-' + name, 'name': name, + 'created_at': stamp(hours_ago)})() + + +def fake_keypair(name, hours_ago): + created = stamp(hours_ago) if hours_ago is not None else None + return type('KP', (), {'name': name, 'created_at': created})() + + +@pytest.mark.parametrize('name,swept', [ + pytest.param('imagebuilder-' + 'a' * 32, True, id='ours'), + pytest.param('imagebuilder-' + 'A' * 32, False, id='uppercase-not-uuid'), + pytest.param('imagebuilder-short', False, id='wrong-shape'), + pytest.param('imagebuilder-' + 'a' * 33, False, id='too-long'), + pytest.param('default', False, id='someone-elses'), + pytest.param('my-imagebuilder-' + 'a' * 32, False, id='suffix-match'), + pytest.param('web-servers', False, id='unrelated'), +]) +def test_only_our_own_resource_names_are_swept(build, name, swept): + build.conn.network.groups = [fake_group(name, hours_ago=48)] + build.sweep_orphans() + deleted = [e for e in build.conn.log if e[0] == 'sg_deleted'] + assert bool(deleted) is swept + + +@pytest.mark.parametrize('hours,swept', [ + pytest.param(48, True, id='two-days-old'), + pytest.param(25, True, id='just-over'), + pytest.param(23, False, id='just-under'), + pytest.param(0, False, id='brand-new'), +]) +def test_only_resources_older_than_the_threshold_are_swept(build, hours, swept): + """A build on another node shares this project and may be using it.""" + build.conn.network.groups = [fake_group('imagebuilder-' + 'b' * 32, hours)] + build.sweep_orphans() + deleted = [e for e in build.conn.log if e[0] == 'sg_deleted'] + assert bool(deleted) is swept + + +def test_a_resource_with_no_usable_timestamp_is_left_alone(build): + group = fake_group('imagebuilder-' + 'c' * 32, 48) + group.created_at = None + build.conn.network.groups = [group] + assert build.sweep_orphans() == 0 + + +def test_keypairs_are_swept_too(build): + build.conn.compute.pairs = [fake_keypair('imagebuilder-' + 'd' * 32, 48)] + assert build.sweep_orphans() == 1 + assert ('kp_deleted', 'imagebuilder-' + 'd' * 32) in build.conn.log + + +def test_a_keypair_whose_age_is_unknown_is_left_alone(build): + """Nova does not always report created_at when listing keypairs.""" + build.conn.compute.pairs = [fake_keypair('imagebuilder-' + 'e' * 32, None)] + assert build.sweep_orphans() == 0 + + +def test_images_are_never_swept(build): + """A leftover image is named like a good one, so it cannot be told apart.""" + build.conn.network.groups = [fake_group('imagebuilder-' + 'f' * 32, 48)] + build.sweep_orphans() + assert 'image_deleted' not in build.conn.kinds() + + +def test_a_listing_failure_does_not_stop_the_build(build): + def explode(**kw): + raise RuntimeError('neutron is down') + + build.conn.network.security_groups = explode + assert build.sweep_orphans() == 0 # must not raise + + +def test_a_delete_failure_does_not_stop_the_build(build): + def explode(sgid): + raise RuntimeError('in use') + + build.conn.network.groups = [fake_group('imagebuilder-' + 'a' * 32, 48)] + build.conn.network.delete_security_group = explode + assert build.sweep_orphans() == 0 # must not raise + + +@pytest.mark.parametrize('value', [ + '2026-09-20T16:43:06Z', + '2026-09-20T16:43:06.123456Z', + '2026-09-20T16:43:06+00:00', + '2026-09-20T16:43:06.000000', +]) +def test_timestamp_formats_openstack_actually_returns(build, value): + from image_builder.build import BuildFunctions + assert BuildFunctions.resource_age_hours(value) is not None + + +@pytest.mark.parametrize('value', [None, '', 'not a date', 12345]) +def test_unreadable_timestamps_give_no_age(build, value): + from image_builder.build import BuildFunctions + assert BuildFunctions.resource_age_hours(value) is None diff --git a/tests/test_main_flow.py b/tests/test_main_flow.py index 11c90c4..0ecafb6 100644 --- a/tests/test_main_flow.py +++ b/tests/test_main_flow.py @@ -35,6 +35,10 @@ def find_network_id(self, name): def run_packer_init(self, template): return 0 + def sweep_orphans(self, *a, **k): + calls.append('swept') + return 0 + def create_security_group(self): calls.append('sg_created') return 'sg-name', 'sg-1' @@ -208,3 +212,18 @@ def test_bootstrap_upload_failure_exits_one_and_cleans_up(run_bootstrap): assert code == 1 assert captured.out == '' assert not os.path.exists(fake.tmp_dir) + + +def test_orphans_are_swept_before_anything_new_is_created(run_build): + """A run killed by the OOM killer leaves its security group and keypair.""" + code, raised, calls = run_build() + assert 'swept' in calls + assert calls.index('swept') < calls.index('sg_created') + + +def test_no_sweep_skips_it(run_build): + code, raised, calls = run_build(argv=[ + 'imagebuilder', 'build', '-n', 'img', '-a', 'bgo-default-1', + '-s', 'src', '-u', 'almalinux', '--no-sweep']) + assert 'swept' not in calls + assert code == 0 From 5d06e9c2ac87106821cbb181cdd8aa042044fe05 Mon Sep 17 00:00:00 2001 From: Trond Hasle Amundsen Date: Sun, 20 Sep 2026 19:20:13 +0200 Subject: [PATCH 19/22] docs: correct CLAUDE.md where it describes code that has since changed Three claims had drifted out of date over the course of this branch. The environment section still listed OS_IDENTITY_API_VERSION and OS_NO_CACHE as required, which is the bug that was fixed, and still said the glance CLI must be on PATH after it stopped being used. The architecture section described main() as a straight-line driver dispatching through getattr, which the split into run_build and run_bootstrap replaced. And bootstrap was described as matching a checksum by substring, which the filename-aware parsing replaced. --- CLAUDE.md | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 565c126..f54009f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -39,19 +39,18 @@ with a fake `BuildFunctions` — that is where the cleanup-on-failure guarantees Packaging metadata and the console script live in `pyproject.toml`; there is no `setup.py`. Running any command requires OpenStack credentials in the environment (`source keystone_rc.sh`) — -`get_openstack_rc()` reads `OS_USERNAME`, `OS_PROJECT_NAME`, `OS_PASSWORD`, `OS_AUTH_URL`, -`OS_IDENTITY_API_VERSION`, `OS_USER_DOMAIN_NAME`, `OS_PROJECT_DOMAIN_NAME`, `OS_REGION_NAME`, -`OS_NO_CACHE` and exits if any are missing (`OS_CACERT` is optional). `packer`, `ssh-keygen`, -`openssl`, and the `glance` CLI must be on `$PATH` — on RHEL/CentOS beware the unrelated -`/usr/sbin/packer`; `/usr/bin/packer` is the right one. +`REQUIRED_ENV` in `imagebuilder.py` lists exactly what is read, and a missing one is reported by +name. `OS_CACERT` is optional. Deliberately *not* required: `OS_IDENTITY_API_VERSION` and +`OS_NO_CACHE` were once mandatory but never used, which rejected valid openrc files. `packer` and +`ssh-keygen` must be on `$PATH` — on RHEL/CentOS beware the unrelated `/usr/sbin/packer`; +`/usr/bin/packer` is the right one. ## Architecture -`image_builder/imagebuilder.py:main()` is a straight-line procedural driver — all orchestration lives -there, and the other modules are stateless-ish helper classes. Two subcommands, dispatched by -`parsecommands.Commands` (which uses the argparse "git-style subcommand" pattern: `sys.argv[1:2]` -selects the method, `sys.argv[2:]` is parsed by that method's own parser, and `build_args` / -`bootstrap_args` are `False` when unused). +`image_builder/imagebuilder.py:main()` parses, configures logging, authenticates and dispatches to +`run_build()` or `run_bootstrap()`, which return an exit code rather than calling `sys.exit` so they +can be driven from a test. `parsecommands.Commands` uses argparse subparsers and exposes +`build_args` / `bootstrap_args`, one of which is `None`. **`build`** (`build.py:BuildFunctions`) — the core flow, in order: 1. `helpers.make_tmp_dir()` — a `tempfile.mkdtemp(prefix='imagebuilder-')` holding the SSH keypair and @@ -91,8 +90,9 @@ resource a build elsewhere is using), and never images, whose names are indistin legitimate ones. **`bootstrap`** (`bootstrap.py:BootstrapFunctions`) — downloads a cloud image from a URL, optionally -verifies it against a checksum file (substring match of the computed hexdigest in the fetched file), and -uploads it to Glance with hardware properties set from the flags (`hw_disk_bus=scsi` + +verifies it against a checksum file (`find_expected_checksum()` parses both the coreutils and BSD +layouts and matches on the filename, falling back to "the hash appears anywhere" with a warning when +no line names the download), and uploads it to Glance with hardware properties set from the flags (`hw_disk_bus=scsi` + `hw_scsi_model=virtio-scsi` unless `-s`, `hw_firmware_type=uefi` + `hw_machine_type=q35` with `-e`, always `hw_rng_model=virtio`). On success it writes the bare image id to **stdout** and nothing else — all other output goes to `logging` — so `bootstrap ... | xargs -I % imagebuilder build -s % ...` works. From dd7c077c55b4cad25ff07441f86a6e89ec858bde Mon Sep 17 00:00:00 2001 From: Trond Hasle Amundsen Date: Tue, 22 Sep 2026 06:29:18 +0200 Subject: [PATCH 20/22] fix: do not treat the windows inner build script as an image -S built the windows images along with the standard ones. A windows image installs two build scripts: winsrv__wrapper, which cron calls, and winsrv_, which that wrapper calls. Only the first matched the pattern for the windows category, so the second fell through to the default and was listed and built as a standard image. That is worse than an unwanted build. The inner script sshes to the windows build host and starts packer there, so -S was kicking off windows builds outside their schedule, twice over, while the wrapper that does the rest of the work around them never ran. Discovery now skips a winsrv_ script that is not a _wrapper. It is not an image in its own right and nothing here should pick it up. -i is checked against what discovery found rather than against the filesystem, so the inner script cannot be reached that way either. With the real set of build scripts, -S now builds 17 images rather than 19, -B builds 2, -G builds 5, and -W still finds both wrappers. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A4Y4JQ39uQMSZcBYoTE84y --- build-images.sh | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/build-images.sh b/build-images.sh index 821adb2..2c67ed6 100755 --- a/build-images.sh +++ b/build-images.sh @@ -236,6 +236,14 @@ fi for script_path in "$BUILD_PATH"/*; do [ -f "$script_path" ] || continue script=${script_path##*/} + # A windows image installs two scripts: winsrv__wrapper, which is + # the entry point, and winsrv_, which the wrapper calls. The + # second is not an image of its own, and running it directly starts a + # build on the windows build host outside its schedule, so it is not + # something anything here should ever pick up + if [[ $script =~ ^winsrv_ ]] && [[ ! $script =~ _wrapper$ ]]; then + continue + fi IMAGE_CATEGORY[$script]=$(categorise "$script") ALL_IMAGES+=("$script") done @@ -305,7 +313,7 @@ fi # image nobody has unknown_images=() for image in "${requested_images[@]}"; do - if [ ! -f "${BUILD_PATH}/${image}" ]; then + if [ -z "${IMAGE_CATEGORY[$image]:-}" ]; then unknown_images+=("$image") fi done From 3c19cb539018e22dcd4976da8f3f467a869fd983 Mon Sep 17 00:00:00 2001 From: Trond Hasle Amundsen Date: Tue, 22 Sep 2026 06:34:36 +0200 Subject: [PATCH 21/22] ci: lint the shell scripts too build-images.sh had no automated checking at all. The bug where -S built the windows images was caught by hand, and while shellcheck would not have found that one either, it is the obvious floor to put under a script that is about to become the only entry point for every build. Its own job rather than a step in the matrix, since the shell has nothing to do with the Python version and there is no point running it three times. Only build-images.sh is linted. The provisioning scripts under scripts/ run on the guest rather than here, and produce around a hundred findings between them, so including them needs a cleanup first rather than a blanket ignore. The runner has shellcheck 0.9.0, which reports the deliberately unused palette helpers as SC2317 where 0.11 calls the same thing SC2329. The file level directive now lists both codes, so it is quiet on either version; checked against both rather than assumed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A4Y4JQ39uQMSZcBYoTE84y --- .github/workflows/python-package.yml | 19 ++++++++++++++++++- build-images.sh | 6 ++++-- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/.github/workflows/python-package.yml b/.github/workflows/python-package.yml index d9226ee..95f0880 100644 --- a/.github/workflows/python-package.yml +++ b/.github/workflows/python-package.yml @@ -1,4 +1,5 @@ -# This workflow will install Python dependencies, lint and run tests with a variety of Python versions +# This workflow will install Python dependencies, lint and run tests with a +# variety of Python versions, and lint the shell scripts # For more information see: https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-python name: Python package @@ -48,3 +49,19 @@ jobs: ruff check --output-format=github . - name: Run tests run: pytest -q + + shellcheck: + # Its own job rather than a step in the matrix: the shell scripts have + # nothing to do with the Python version, so there is no point linting them + # three times + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v7 + - name: Lint shell scripts + run: | + shellcheck --version + # Only build-images.sh for now. The provisioning scripts under + # scripts/ run on the guest rather than here and currently produce + # around a hundred findings, so adding them needs a cleanup first + shellcheck build-images.sh diff --git a/build-images.sh b/build-images.sh index 2c67ed6..2ce62bd 100755 --- a/build-images.sh +++ b/build-images.sh @@ -4,8 +4,10 @@ # per image. # # The colour palette and the XY/line/cnt helpers below are kept whole for -# future use, so unused variables and uncalled functions are expected here -# shellcheck disable=SC2034,SC2329 +# future use, so unused variables and uncalled functions are expected here. +# SC2317 and SC2329 are the same complaint: shellcheck renamed it, and both +# codes are listed so this stays quiet on either version +# shellcheck disable=SC2034,SC2317,SC2329 # Continue past a failed build, but treat unset variables and broken pipes as # errors. Not -e: one image failing must not stop the rest of the run From 5588ed6e11d7bdf82f5bae33bb075e8fb6c85745 Mon Sep 17 00:00:00 2001 From: Trond Hasle Amundsen Date: Tue, 22 Sep 2026 06:41:16 +0200 Subject: [PATCH 22/22] feat: make the sweep opt-in, and let build-images.sh ask for it --no-sweep becomes --sweep. Removing resources that the current run did not create is not something to do by default on every build: it should happen because something asked for it. build-images.sh gets -s for the same purpose. It cannot pass the flag along, because it runs the generated build scripts and those take no arguments of their own, so it asks through the environment instead: IB_SWEEP, alongside the IB_TEMPLATE_DIR and IB_DOWNLOAD_DIR that are already read from there. IB_SWEEP is exported either way, so a stray value in whoever's environment cannot turn a sweep on from outside. IB_SWEEP=false does not enable it, which is why the value is parsed rather than merely tested for being set. Note the case: -S selects the standard images and -s asks for the sweep. The help says so. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A4Y4JQ39uQMSZcBYoTE84y --- CLAUDE.md | 18 ++++++++------ build-images.sh | 18 +++++++++++++- image_builder/imagebuilder.py | 15 +++++++++--- image_builder/parsecommands.py | 8 ++++--- tests/test_main_flow.py | 43 +++++++++++++++++++++++++++------- 5 files changed, 80 insertions(+), 22 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index f54009f..e638c6f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -14,11 +14,13 @@ This fork's `origin` is `trondham/imagebuilder`; `upstream` is `norcams/imagebui ## Commands ```bash -# Lint — this is what CI runs (.github/workflows/python-package.yml), matrix py3.9/3.10/3.11 -ruff check --select=E9,F63,F7,F82 --target-version=py39 . -ruff check --target-version=py39 . +# Lint — this is what CI runs (.github/workflows/python-package.yml), matrix py3.9/3.10/3.11. +# The rule set and target-version live in pyproject.toml, so ruff needs no flags +ruff check --select=E9,F63,F7,F82 . +ruff check . +shellcheck build-images.sh # its own CI job; scripts/ is not linted yet -# Tests — CI runs these too; ruff is pinned to 0.15.22 and pytest to 8.4.2 +# Tests — CI runs these too; pytest is pinned to 8.4.2 because 9 requires 3.10 pytest -q pytest tests/test_main_flow.py -q # a single file pytest -k "cleanup" -q # a single case @@ -83,9 +85,11 @@ Everything after the first OpenStack resource is created runs under a `try/final reached on exceptions and on Ctrl-C (SIGINT becomes `KeyboardInterrupt`, which unwinds). `cleanup()` is written for that position: it skips ids that are `None` and swallows API errors, because raising there would mask the original failure. SIGTERM and SIGKILL still bypass it — verified — so those remain the -one case that orphans `imagebuilder-*` resources. `sweep_orphans()` runs at the start of every -build to clear those up: only names matching `imagebuilder-<32 hex>`, only once older than -`SWEEP_MAX_AGE_HOURS` (builder nodes share a project, so the age is what stops it deleting a +one case that orphans `imagebuilder-*` resources. `sweep_orphans()` clears those up, but only when asked +— `--sweep`, or `IB_SWEEP=true` in the environment, which is how `build-images.sh -s` reaches it, +since the generated build scripts take no arguments of their own. It is opt-in because it deletes +resources the current run did not create. Only names matching `imagebuilder-<32 hex>`, only once +older than `SWEEP_MAX_AGE_HOURS` (builder nodes share a project, so the age is what stops it deleting a resource a build elsewhere is using), and never images, whose names are indistinguishable from legitimate ones. diff --git a/build-images.sh b/build-images.sh index 2ce62bd..3ba4f1d 100755 --- a/build-images.sh +++ b/build-images.sh @@ -136,6 +136,7 @@ declare -A SELECT_ALL=() LIST=0 DRYRUN=0 TIMEOUT=0 +SWEEP=0 # Every -i given, kept so they can be checked before anything is built requested_images=() @@ -184,6 +185,9 @@ usage() { echo ' -G Build all vgpu images. Negates -i options' echo ' -W Build all Windows images. Negates -i options' echo ' -i Specify image to build. Can be used multiple times' + echo ' -s Sweep: before each build, remove temporary security groups and' + echo ' keypairs left behind by runs that were killed. Note the lower case:' + echo ' -S selects the standard images, -s asks for the sweep' echo ' -t Terminate build job if still running after this duration. See timeout(1)' echo ' -l List all images and exit' echo ' -n Dry-run: Only show what would be done' @@ -260,7 +264,7 @@ if [ $# -eq 0 ]; then exit 1 fi -while getopts "SBGWlnhi:t:" arg; do +while getopts "SBGWlnshi:t:" arg; do case $arg in S|B|G|W) SELECT_ALL[${OPTION_CATEGORY[$arg]}]=1 @@ -278,6 +282,9 @@ while getopts "SBGWlnhi:t:" arg; do fi TIMEOUT=$OPTARG ;; + s) + SWEEP=1 + ;; l) LIST=1 ;; @@ -360,6 +367,15 @@ fi export IMAGEBUILDER_REPORT=true +# The generated build scripts take no arguments of their own, so a sweep is +# asked for through the environment. Exported either way, so that a stray +# IB_SWEEP in whoever's environment cannot turn it on behind our back +if [ $SWEEP -eq 1 ]; then + export IB_SWEEP=true +else + export IB_SWEEP=false +fi + run_start=$SECONDS for cat in "${CATEGORIES[@]}"; do diff --git a/image_builder/imagebuilder.py b/image_builder/imagebuilder.py index e8c753c..ff67ff5 100644 --- a/image_builder/imagebuilder.py +++ b/image_builder/imagebuilder.py @@ -69,6 +69,15 @@ def configure_logging(args): logging.basicConfig(format="%(message)s", level=logging.WARNING) +def sweep_requested_by_environment(): + """Whether IB_SWEEP asks for a sweep + + build-images.sh runs the generated build scripts, which take no arguments + of their own, so the environment is how a sweep is asked for from there. + """ + return os.environ.get('IB_SWEEP', '').strip().lower() in ('1', 'true', 'yes', 'on') + + def resolve_directory(config, env_var, option): """Finds a configured directory, with the environment variable winning""" if env_var in os.environ: @@ -124,9 +133,9 @@ def run_build(args, ib_session, region, template_dir, download_dir): helpers.clean_tmp_files(build.tmp_dir) return 1 - # Before creating anything, clear out what earlier runs could not. A build - # killed by the OOM killer never reaches its own cleanup - if not args.no_sweep: + # Asked for rather than assumed: a sweep deletes resources this run did not + # create, so it happens when something wants it to, not on every build + if args.sweep or sweep_requested_by_environment(): build.sweep_orphans() log.info('Creating Packer security group...') diff --git a/image_builder/parsecommands.py b/image_builder/parsecommands.py index 0346995..8e4731c 100644 --- a/image_builder/parsecommands.py +++ b/image_builder/parsecommands.py @@ -47,9 +47,11 @@ def __init__(self, argv=None): help='Be verbose (default is no output)', action='store_true', default=False) - build.add_argument('--no-sweep', - help='Do not remove temporary security groups and ' - 'keypairs left behind by runs that were killed', + build.add_argument('--sweep', + help='Before building, remove temporary security ' + 'groups and keypairs left behind by runs that ' + 'were killed. Can also be asked for with ' + 'IB_SWEEP=true in the environment', action='store_true', default=False) build.add_argument('-x', '--purge-source', diff --git a/tests/test_main_flow.py b/tests/test_main_flow.py index 0ecafb6..f91b9a3 100644 --- a/tests/test_main_flow.py +++ b/tests/test_main_flow.py @@ -214,16 +214,43 @@ def test_bootstrap_upload_failure_exits_one_and_cleans_up(run_bootstrap): assert not os.path.exists(fake.tmp_dir) -def test_orphans_are_swept_before_anything_new_is_created(run_build): - """A run killed by the OOM killer leaves its security group and keypair.""" +BUILD_ARGV = ['imagebuilder', 'build', '-n', 'img', '-a', 'bgo-default-1', + '-s', 'src', '-u', 'almalinux'] + + +def test_no_sweep_unless_it_is_asked_for(run_build): + """It deletes resources this run did not create, so it is opt-in.""" code, raised, calls = run_build() + assert 'swept' not in calls + assert code == 0 + + +def test_sweep_happens_before_anything_new_is_created(run_build): + code, raised, calls = run_build(argv=BUILD_ARGV + ['--sweep']) assert 'swept' in calls assert calls.index('swept') < calls.index('sg_created') -def test_no_sweep_skips_it(run_build): - code, raised, calls = run_build(argv=[ - 'imagebuilder', 'build', '-n', 'img', '-a', 'bgo-default-1', - '-s', 'src', '-u', 'almalinux', '--no-sweep']) - assert 'swept' not in calls - assert code == 0 +def test_ib_sweep_in_the_environment_asks_for_one_too(run_build, monkeypatch): + """build-images.sh runs build scripts that take no arguments of their own.""" + monkeypatch.setenv('IB_SWEEP', 'true') + code, raised, calls = run_build() + assert 'swept' in calls + + +@pytest.mark.parametrize('value,swept', [ + pytest.param('true', True, id='true'), + pytest.param('TRUE', True, id='uppercase'), + pytest.param('1', True, id='one'), + pytest.param('yes', True, id='yes'), + pytest.param('on', True, id='on'), + pytest.param('false', False, id='false'), + pytest.param('0', False, id='zero'), + pytest.param('no', False, id='no'), + pytest.param('', False, id='empty'), +]) +def test_ib_sweep_values(run_build, monkeypatch, value, swept): + """IB_SWEEP=false must not turn it on.""" + monkeypatch.setenv('IB_SWEEP', value) + code, raised, calls = run_build() + assert ('swept' in calls) is swept