Add drift check for retired release references - #2643
Draft
ideaship wants to merge 1 commit into
Draft
Conversation
Retirement removes a release from the supported range, but osism/defaults keeps two kinds of reference to it that nothing fails on afterwards: - a version gate -- openstack_version in ['A', 'B'] -- selecting a value for particular releases. Once a listed release is gone that branch can never be taken again. - a per-release file -- all/010-<release>.yml -- a backward-compat layer extending the 001 mirror for an older supported release. Its own header says to delete it when that release leaves the range, and nothing checks that it was. Both are dead rather than broken, so nothing prompts the cleanup. A gate on victoria has sat in the image catalogue since roughly 2021 unnoticed, which is the argument for a check rather than a habit. Nothing covered this: every other defaults-reading plugin asks whether a var name exists upstream (kolla_image_orphan, kolla_enablement_orphan), whether an orphaned enable flag anchors it (kolla_orphan_config), or whether a value matches upstream and only in all/001 (kolla_mirror_verbatim). The staleness here is in neither the name nor the value but in the gate condition, which nothing read. This is the mirror image of kolla_source_ref_phase: that plugin catches a reference that should have moved forward when a release changed phase, this one a reference that should have been dropped when a release was retired. Both failure modes are silent, and both went years unnoticed. Findings are advisory. A gate on a release that can no longer be deployed may still be load-bearing for an upgrade *from* it -- the deploy scripts pick the key-value store by reading the release from the running kolla-ansible image label, precisely to handle the mid-upgrade state -- so "outside the supported range" means "confirm this is still needed", not "this is a defect". The nightly job stays green and deliberate keeps go in the allowlist. Scope is osism/defaults only, and that is a deliberate limit. The same redis/valkey cutover is gated in shell case statements in testbed, metalbox and container-image-kolla-ansible, so covering them would find three more today -- but a case parser has to handle ;;& and ;&, nested case, arms sharing a line with code, quoting and heredocs, and a subtle mis-parse yields a silent false negative, which is the failure this check exists to prevent. It would also be the only shell-parsing code in the tool. kolla_source_ref_phase made the same call for the hardcoded requirements ref in container-images-kolla scripts/002-generate.sh. Those copies are covered by the retirement checklist in the guide, which names each file (osism/osism.github.io#1047). The defaults side needs no such judgement: the repository uses exactly one idiom, `in` and its `not in` form with a bracketed list of quoted literals, and every all/*.yml file is valid YAML whose jinja lives only in values. parse_version_gates therefore matches one stable construct inside safe_load'ed strings and ignores the rest of the expression rather than trying to understand jinja. Values are walked recursively, since a gate can sit inside a map or a list rather than a scalar. The all/010-<release>.yml convention now has one definition, enablement.per_release_file, used both by groupvars_home -- which routes dropped keys into these files -- and by the check that retires them, so the two cannot disagree about the name. Ordered next to kolla_orphan_config: both report dead content in osism/defaults, one for services upstream removed and one for releases we no longer support. Verified against the live repositories: reports mariadb_image in all/002-images-kolla.yml as the single advisory finding, and stays silent on the three all/010-*.yml files, whose releases are all still supported. 422 tests pass; flake8 and black clean. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
ideaship
force-pushed
the
feature/drift-version-gate-orphan
branch
from
August 24, 2026 10:26
b3b1528 to
4fd0ede
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Retirement removes a release from the supported range, but
osism/defaultskeepstwo kinds of reference to it that nothing fails on afterwards:
openstack_version in ['A', 'B']— selecting a value forparticular releases. Once a listed release is gone, that branch can never be
taken again.
all/010-<release>.yml— a backward-compat layerextending the 001 mirror for an older supported release. Its own header says to
delete it when that release leaves the range, and nothing checks that it was.
Both are dead rather than broken, so nothing prompts the cleanup. A gate on
victoriahas sat in the image catalogue since roughly 2021 unnoticed, which isthe argument for a check rather than a habit.
Stacked on:
Both touch the plugin registry,
drift-config.ymland the registry-order test,so this is based on that branch to avoid conflicting edits in those three files.
Merge #2642 first.
Why nothing caught this
Every other
defaults-reading plugin asks a different question:kolla_image_orphankolla_enablement_orphankolla_orphan_configkolla_mirror_verbatimall/001-*mariadb_imagepasses all four:mariadb-serverexists upstream, mariadb isenabled, and the var lives in
002-*, outsidekolla_mirror_verbatim's scope.The staleness is in neither the name nor the value but in the gate condition,
which nothing read.
This is the mirror image of
kolla_source_ref_phasein #2642: that check catchesa reference that should have moved forward when a release changed phase, this
one a reference that should have been dropped when a release was retired. Both
failure modes are silent, and both went years unnoticed.
Advisory, not actionable
A gate on a release that can no longer be deployed may still be load-bearing for
an upgrade from it — the deploy scripts pick the key-value store by reading the
release from the running kolla-ansible image label, precisely to handle the
mid-upgrade state. So "outside the supported range" means confirm this is still
needed, not this is a defect. The nightly job stays green and deliberate keeps
go in the allowlist. Precedent:
role_shadowsuses advisory for its live/dormantdistinction.
Scope is
osism/defaultsonly, deliberatelyThe same redis/valkey cutover is gated in shell
casestatements intestbed,metalboxandcontainer-image-kolla-ansible, so covering them would find threemore findings today (
2023.*arms in all three). I left them out: acaseparserhas to handle
;;∧&, nestedcase, arms sharing a line with code,quoting and heredocs — and a subtle mis-parse yields a silent false negative,
which is exactly the failure this check exists to prevent. It would also be the
only shell-parsing code in the tool. #2642 made the same call for the hardcoded
requirements ref in
container-images-kollascripts/002-generate.sh.Those copies are covered by the retirement checklist instead, which now names each
file:
The
defaultsside needs no such judgement. The repository uses exactly oneidiom —
inand itsnot inform, with a bracketed list of quoted literals, 7occurrences, no
==or>=— and everyall/*.ymlfile is valid YAML whosejinja lives only in values.
parse_version_gatestherefore matches one stableconstruct inside
safe_loaded strings and ignores the rest of the expressionrather than trying to understand jinja. Values are walked recursively, since a
gate can sit inside a map or a list rather than a scalar.
Note on the
010-conventionThe
all/010-<release>.ymlname now has a single definition,enablement.per_release_file, used both bygroupvars_home— which routesdropped keys into those files — and by the check that retires them, so the two
cannot disagree about the name.
Verification
Live run against the real repositories reports exactly one thing:
Silent on all three existing
all/010-*.ymlfiles, whose releases are stillsupported — at the next retirement,
010-<version>.ymland that version's gateliterals fire together. 422 tests pass (+24); flake8, black and yamllint clean;
--demorenders. The committed tree was re-verified in a clean worktree.The
victoriagate is deliberately not fixed here — it is the check's liveself-test, and there is no upgrade path from victoria to a supported release, so
nothing depends on it.
🤖 Generated with Claude Code