From f05e4c232195d5e19547de63a1388876e7c51b21 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Thu, 10 Sep 2026 12:11:51 +0200 Subject: [PATCH 1/5] data: resolve the OpenStack release Collections deploy a few roles only on some releases, so expanding one needs to know which OpenStack release is deployed. Nothing in the package could answer that. Resolve it from --openstack-version, then OPENSTACK_VERSION, then openstack_version in /interface/versions/kolla-ansible.yml. The kolla-ansible container copies that file from its own group_vars/all/versions.yml when it starts, so the value describes the containers that are actually deployed; reading /interface/versions has precedent in tasks/__init__.py, which takes container versions from the same directory. The manager configuration is not a source, even though it has the key. environments/manager/configuration.yml carries openstack_version only on a latest deployment: for a stable release the OpenStack and Ceph versions follow manager_version and the key is stripped from the configuration, so it is absent on exactly the deployments that pin a release. Releases parse to a (year, minor) tuple so they compare in release order, and format_release renders one back for log and error text. Parsing happens at call time, never at import: settings.py and utils/__init__.py show the failure mode where an unparseable environment variable raises during import and takes down the whole CLI rather than the one command that needed the value. Failures are distinguished because callers render them differently. ReleaseUndetermined carries a cause fragment to append to a sentence naming what needed the release; ReleaseUnparseable carries a complete sentence. Both a missing key and YAML that parses to something other than a mapping raise the former -- a list or a bare scalar is well formed, and would otherwise reach the lookup and raise AttributeError, escaping the diagnostic entirely. The versions file is opened without testing for its existence first. A stat saying the file is there is no promise that the open succeeds, and an unreadable file, a directory in its place, or one removed in between raises OSError past callers that catch ReleaseError only -- a traceback where the diagnostic belongs. Of the non-mappings just one means "nothing configured" rather than "malformed": an empty file, which parses to None. The falsy rest ([], false, 0, "") are reported as the malformed file they are, not as an unset key. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi --- osism/data/releases.py | 129 +++++++++++++ tests/unit/data/test_releases.py | 322 +++++++++++++++++++++++++++++++ 2 files changed, 451 insertions(+) create mode 100644 osism/data/releases.py create mode 100644 tests/unit/data/test_releases.py diff --git a/osism/data/releases.py b/osism/data/releases.py new file mode 100644 index 000000000..94eb2f3a0 --- /dev/null +++ b/osism/data/releases.py @@ -0,0 +1,129 @@ +# SPDX-License-Identifier: Apache-2.0 + +"""Resolution of the OpenStack release a deployment runs. + +Collections deploy a few roles only on some releases -- see the ``since`` and +``until`` bounds on ``osism.data.enums.Role`` -- so expanding one needs to know +which release is deployed. + +The value is read at call time, never at import. ``osism/settings.py`` and +``osism/utils/__init__.py`` parse environment variables during module import, +which turns a single typo into a CLI that cannot start at all; this module does +not repeat that. +""" + +import os +import re + +import yaml + +# The kolla-ansible container copies its own group_vars/all/versions.yml here +# when it starts, so the file describes the containers that are actually +# deployed and is present for a stable release as well as for latest. Reading +# /interface/versions has precedent in tasks/__init__.py, which takes container +# versions from the same directory. +# +# The manager configuration is deliberately not a source. It carries +# openstack_version only on a latest deployment: for a stable release the +# OpenStack and Ceph versions follow manager_version, and the key is stripped +# from environments/manager/configuration.yml -- so it is missing on exactly +# the deployments that pin a release. +VERSIONS_FILE = "/interface/versions/kolla-ansible.yml" + +# OpenStack releases are YYYY.N. Note YAML parses an unquoted 2024.2 as a float, +# so parse_release stringifies before matching. +_RELEASE_RE = re.compile(r"^(\d{4})\.(\d+)$") + + +class ReleaseError(Exception): + """Base class for failures to establish the OpenStack release.""" + + +class ReleaseUndetermined(ReleaseError): + """No source supplied a release. + + The message is a *cause fragment* ("... not found."), meant to be appended + to a sentence naming what needed the release. + """ + + +class ReleaseUnparseable(ReleaseError): + """A source supplied something that is not a release. + + The message is a complete sentence and is printed as-is. + """ + + +def parse_release(value): + """Return ``value`` as a ``(year, minor)`` tuple, e.g. ``(2025, 1)``. + + Tuples compare in release order, so callers can use ``<`` and ``>``. + """ + match = _RELEASE_RE.match(str(value).strip()) + if not match: + raise ReleaseUnparseable( + f"Could not parse OpenStack release {value!r} " + f"(expected a release like 2025.1)." + ) + + return int(match.group(1)), int(match.group(2)) + + +def format_release(release): + """Return ``release`` (a ``(year, minor)`` tuple) as ``"year.minor"``. + + Inverse of ``parse_release``. + """ + return f"{release[0]}.{release[1]}" + + +def openstack_release(override=None): + """Return the deployed OpenStack release as a ``(year, minor)`` tuple. + + Sources, in order: ``override`` (the ``--openstack-version`` argument), the + ``OPENSTACK_VERSION`` environment variable, and ``openstack_version`` in + ``VERSIONS_FILE``, which the kolla-ansible container publishes for every + deployment. + + Raises ``ReleaseUnparseable`` if a source supplied a value that is not a + release, and ``ReleaseUndetermined`` if no source supplied one at all. + """ + if override: + return parse_release(override) + + from_environment = os.environ.get("OPENSTACK_VERSION") + if from_environment: + return parse_release(from_environment) + + # Opened without testing for existence first: a stat that says the file is + # there is no promise that the open succeeds, and every way it can fail -- + # gone since, unreadable, a directory, /interface not mounted -- has to + # reach the caller as a ReleaseError anyway. + try: + with open(VERSIONS_FILE) as fp: + versions = yaml.safe_load(fp) + except FileNotFoundError: + raise ReleaseUndetermined(f"{VERSIONS_FILE} not found.") + except OSError as exc: + raise ReleaseUndetermined(f"{VERSIONS_FILE} could not be read: {exc.strerror}.") + except yaml.YAMLError: + raise ReleaseUndetermined(f"{VERSIONS_FILE} is not valid YAML.") + + # An empty file parses to None, which is the one non-mapping that means + # nothing is published rather than that the file is malformed. + if versions is None: + versions = {} + + # Well-formed YAML is not necessarily a mapping: a list or a bare scalar + # parses fine and would turn the lookup below into an AttributeError, + # escaping the diagnostic this function exists to produce. Falsy ones + # ([], false, 0, "") are caught here too, so they are reported as the + # malformed file they are and not as an unset key. + if not isinstance(versions, dict): + raise ReleaseUndetermined(f"{VERSIONS_FILE} does not contain a YAML mapping.") + + value = versions.get("openstack_version") + if not value: + raise ReleaseUndetermined(f"openstack_version not set in {VERSIONS_FILE}.") + + return parse_release(value) diff --git a/tests/unit/data/test_releases.py b/tests/unit/data/test_releases.py new file mode 100644 index 000000000..6e7cda1d9 --- /dev/null +++ b/tests/unit/data/test_releases.py @@ -0,0 +1,322 @@ +# SPDX-License-Identifier: Apache-2.0 + +"""Tests for OpenStack release resolution. + +``osism.data.releases`` answers one question -- which OpenStack release is +deployed -- from three sources in a fixed order, the last of them the versions +file the kolla-ansible container publishes under /interface. These tests +characterize the parser, the resolution order, and the distinct failures the +caller has to render differently (missing file, unreadable file, missing key, +malformed file, unparseable value). +""" + +import os + +import pytest + +from osism.data import releases + +# parse_release + + +@pytest.mark.parametrize( + "value,expected", + [ + ("2025.1", (2025, 1)), + ("2026.1", (2026, 1)), + ("2024.2", (2024, 2)), + (" 2025.1 ", (2025, 1)), + ], +) +def test_parse_release_accepts_releases(value, expected): + assert releases.parse_release(value) == expected + + +def test_parse_release_accepts_yaml_float(): + """YAML parses an unquoted ``openstack_version: 2024.2`` as a float.""" + assert releases.parse_release(2024.2) == (2024, 2) + + +@pytest.mark.parametrize("value", ["master", "2025", "", "stable/2025.1", "v2025.1"]) +def test_parse_release_rejects_non_releases(value): + with pytest.raises(releases.ReleaseUnparseable) as excinfo: + releases.parse_release(value) + + assert "expected a release like 2025.1" in str(excinfo.value) + assert repr(value) in str(excinfo.value) + + +def test_parse_release_orders_as_tuples(): + assert releases.parse_release("2025.2") > releases.parse_release("2025.1") + assert releases.parse_release("2026.1") > releases.parse_release("2025.2") + + +# format_release + + +@pytest.mark.parametrize( + "release,expected", + [ + ((2025, 1), "2025.1"), + ((2026, 1), "2026.1"), + ((2024, 2), "2024.2"), + ], +) +def test_format_release_renders_year_dot_minor(release, expected): + assert releases.format_release(release) == expected + + +@pytest.mark.parametrize( + "value", + ["2025.1", "2026.1", "2024.2", 2024.2], +) +def test_format_release_round_trips_with_parse_release(value): + release = releases.parse_release(value) + + assert releases.parse_release(releases.format_release(release)) == release + + +# openstack_release resolution order + + +def test_openstack_release_override_wins(monkeypatch, tmp_path): + monkeypatch.setenv("OPENSTACK_VERSION", "2024.1") + monkeypatch.setattr(releases, "VERSIONS_FILE", str(tmp_path / "absent.yml")) + + assert releases.openstack_release("2026.1") == (2026, 1) + + +def test_openstack_release_env_beats_file(monkeypatch, tmp_path): + versions = tmp_path / "kolla-ansible.yml" + versions.write_text("openstack_version: 2024.1\n") + monkeypatch.setenv("OPENSTACK_VERSION", "2026.1") + monkeypatch.setattr(releases, "VERSIONS_FILE", str(versions)) + + assert releases.openstack_release() == (2026, 1) + + +def test_openstack_release_reads_versions_file(monkeypatch, tmp_path): + versions = tmp_path / "kolla-ansible.yml" + versions.write_text("openstack_version: 2026.1\nother_key: value\n") + monkeypatch.delenv("OPENSTACK_VERSION", raising=False) + monkeypatch.setattr(releases, "VERSIONS_FILE", str(versions)) + + assert releases.openstack_release() == (2026, 1) + + +def test_openstack_release_reads_versions_file_as_published(monkeypatch, tmp_path): + """The file as the kolla-ansible container publishes it. + + Two shapes in it would break a naive reader: openstack_version is quoted, + so it arrives as a string and not as the float an unquoted 2024.1 would + become, and the keys beside it hold un-rendered Jinja that must not be + mistaken for anything but opaque text. + """ + versions = tmp_path / "kolla-ansible.yml" + versions.write_text( + 'openstack_version: "2024.1"\n' + 'kolla_ansible_version: "{{ openstack_version }}"\n' + 'kolla_image_version: "{{ openstack_version }}"\n' + 'openstack_release: "{{ openstack_version }}"\n' + ) + monkeypatch.delenv("OPENSTACK_VERSION", raising=False) + monkeypatch.setattr(releases, "VERSIONS_FILE", str(versions)) + + assert releases.openstack_release() == (2024, 1) + + +def test_openstack_release_empty_env_falls_through(monkeypatch, tmp_path): + versions = tmp_path / "kolla-ansible.yml" + versions.write_text("openstack_version: 2026.1\n") + monkeypatch.setenv("OPENSTACK_VERSION", "") + monkeypatch.setattr(releases, "VERSIONS_FILE", str(versions)) + + assert releases.openstack_release() == (2026, 1) + + +# openstack_release failures + + +def test_openstack_release_missing_file(monkeypatch, tmp_path): + missing = tmp_path / "absent.yml" + monkeypatch.delenv("OPENSTACK_VERSION", raising=False) + monkeypatch.setattr(releases, "VERSIONS_FILE", str(missing)) + + with pytest.raises(releases.ReleaseUndetermined) as excinfo: + releases.openstack_release() + + assert str(missing) in str(excinfo.value) + assert "not found" in str(excinfo.value) + + +def test_openstack_release_unreadable_file(monkeypatch, tmp_path): + """A file that exists but cannot be opened is still a ReleaseError. + + Testing for existence and then opening leaves a window -- the file can be + unreadable, a directory, or gone by the time the open runs -- and an + OSError escaping here would reach the operator as a traceback instead of + the release-resolution diagnostic the caller renders. + """ + versions = tmp_path / "kolla-ansible.yml" + versions.write_text("openstack_version: 2025.1\n") + versions.chmod(0o000) + monkeypatch.delenv("OPENSTACK_VERSION", raising=False) + monkeypatch.setattr(releases, "VERSIONS_FILE", str(versions)) + + try: + if os.access(str(versions), os.R_OK): + pytest.skip("running with privileges that ignore file permissions") + + with pytest.raises(releases.ReleaseUndetermined) as excinfo: + releases.openstack_release() + finally: + versions.chmod(0o644) + + assert str(versions) in str(excinfo.value) + assert "could not be read" in str(excinfo.value) + + +def test_openstack_release_versions_path_is_a_directory(monkeypatch, tmp_path): + """The same window, in the form that does not depend on the test's uid.""" + versions = tmp_path / "kolla-ansible.yml" + versions.mkdir() + monkeypatch.delenv("OPENSTACK_VERSION", raising=False) + monkeypatch.setattr(releases, "VERSIONS_FILE", str(versions)) + + with pytest.raises(releases.ReleaseUndetermined) as excinfo: + releases.openstack_release() + + assert str(versions) in str(excinfo.value) + assert "could not be read" in str(excinfo.value) + + +def test_openstack_release_missing_key(monkeypatch, tmp_path): + versions = tmp_path / "kolla-ansible.yml" + versions.write_text("other_key: value\n") + monkeypatch.delenv("OPENSTACK_VERSION", raising=False) + monkeypatch.setattr(releases, "VERSIONS_FILE", str(versions)) + + with pytest.raises(releases.ReleaseUndetermined) as excinfo: + releases.openstack_release() + + assert "openstack_version not set" in str(excinfo.value) + assert str(versions) in str(excinfo.value) + + +def test_openstack_release_empty_file(monkeypatch, tmp_path): + versions = tmp_path / "kolla-ansible.yml" + versions.write_text("") + monkeypatch.delenv("OPENSTACK_VERSION", raising=False) + monkeypatch.setattr(releases, "VERSIONS_FILE", str(versions)) + + with pytest.raises(releases.ReleaseUndetermined): + releases.openstack_release() + + +def test_openstack_release_invalid_yaml(monkeypatch, tmp_path): + versions = tmp_path / "kolla-ansible.yml" + versions.write_text("openstack_version: [unclosed\n") + monkeypatch.delenv("OPENSTACK_VERSION", raising=False) + monkeypatch.setattr(releases, "VERSIONS_FILE", str(versions)) + + with pytest.raises(releases.ReleaseUndetermined) as excinfo: + releases.openstack_release() + + assert "not valid YAML" in str(excinfo.value) + + +@pytest.mark.parametrize( + "content,label", + [ + ("- a\n- b\n", "a list"), + ("just a scalar\n", "a bare scalar"), + # Falsy non-mappings. An "or {}" on the load turns each of these into + # an empty mapping, which passes the isinstance check below and is + # then reported as an unset key rather than as a malformed file. + ("[]\n", "an empty list"), + ("false\n", "a false scalar"), + ("0\n", "a zero scalar"), + ("''\n", "an empty string"), + ], +) +def test_openstack_release_yaml_that_is_not_a_mapping( + monkeypatch, tmp_path, content, label +): + """Well-formed YAML need not be a mapping. + + A list or a bare scalar parses without error, and looking a key up on it + would raise AttributeError -- escaping the ReleaseError diagnostic that + callers render. + """ + versions = tmp_path / "kolla-ansible.yml" + versions.write_text(content) + monkeypatch.delenv("OPENSTACK_VERSION", raising=False) + monkeypatch.setattr(releases, "VERSIONS_FILE", str(versions)) + + with pytest.raises(releases.ReleaseUndetermined) as excinfo: + releases.openstack_release() + + assert "does not contain a YAML mapping" in str(excinfo.value) + + +def test_openstack_release_unparseable_value(monkeypatch, tmp_path): + versions = tmp_path / "kolla-ansible.yml" + versions.write_text("openstack_version: master\n") + monkeypatch.delenv("OPENSTACK_VERSION", raising=False) + monkeypatch.setattr(releases, "VERSIONS_FILE", str(versions)) + + with pytest.raises(releases.ReleaseUnparseable): + releases.openstack_release() + + +def test_undetermined_and_unparseable_are_both_release_errors(): + assert issubclass(releases.ReleaseUndetermined, releases.ReleaseError) + assert issubclass(releases.ReleaseUnparseable, releases.ReleaseError) + + +# import-time safety + + +def test_import_does_not_read_environment(): + """A bad OPENSTACK_VERSION must break one command, not every import. + + ``osism/settings.py`` and ``osism/utils/__init__.py`` parse env vars at + import, so a typo there takes down the whole CLI. This module must not. + + This characterizes the environment half only: a bad ``OPENSTACK_VERSION`` + must not raise on import. It says nothing about filesystem access at + import time -- there is no environment variable to poison the way + ``OPENSTACK_VERSION`` poisons the environment half, so that half is not + exercised here. + + The subprocess also prints ``__file__`` so the assertion below pins down + which copy of the module was actually imported: run from a different + working directory, ``python -c`` can silently resolve an installed + package instead of this worktree's source, and a stale copy would make + this test pass without proving anything about the code under review. + """ + import os + import subprocess + import sys + + environment = dict(os.environ, OPENSTACK_VERSION="not-a-release") + result = subprocess.run( + [ + sys.executable, + "-c", + "import osism.data.releases as r; print(r.__file__)", + ], + env=environment, + capture_output=True, + text=True, + ) + + assert result.returncode == 0, result.stderr + + expected_file = os.path.abspath( + os.path.join( + os.path.dirname(__file__), "..", "..", "..", "osism", "data", "releases.py" + ) + ) + imported_file = os.path.abspath(result.stdout.strip()) + assert imported_file == expected_file From ec784fb6d72f583c264c485f36a8144e4dd872cd Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Thu, 10 Sep 2026 12:12:17 +0200 Subject: [PATCH 2/5] enums: add release bounds to Role MAP_ROLE2ROLE is static: it has no notion of an OpenStack release, so a collection cannot express that one of its roles belongs only to some of them. That is the gap behind redis surviving in three collections after kolla stopped shipping its play. Give Role optional inclusive since/until bounds, a deployed_in predicate and a bound_description renderer for log and error text, plus a bounded_roles walk so a caller can ask whether a collection needs the release at all. A role with neither bound never consults the release and behaves exactly as before, which is every role in the catalog for now. A bound governs collection membership only: whether a collection should deploy this role on this release. It is NOT a statement that the role's playbook exists, or that the role may be applied. valkey is the illustration -- its play ships from OpenStack 2025.1, but collections should not deploy it there because osism/defaults leaves enable_valkey off until 2025.2. Reading the bound as availability would wrongly reject "osism apply valkey" on 2025.1, so the name deployed_in and the docstring both say membership rather than existence, and only collection expansion will consult them. Bounds are parsed eagerly, unlike the deployed release: these are literals in this file, so a typo is a bug here that should surface at once rather than on the one deployment whose release happens to test it. bound_description raises rather than falling through for a role with no bounds at all, which is unreachable today but one call site away. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi --- osism/data/enums.py | 74 ++++++++++++++++++++++- tests/unit/data/test_enums.py | 107 ++++++++++++++++++++++++++++++++++ 2 files changed, 179 insertions(+), 2 deletions(-) diff --git a/osism/data/enums.py b/osism/data/enums.py index b9562c4d7..b3e69fb23 100644 --- a/osism/data/enums.py +++ b/osism/data/enums.py @@ -1,5 +1,7 @@ # SPDX-License-Identifier: Apache-2.0 +from osism.data.releases import format_release, parse_release + class Role: """ @@ -8,6 +10,20 @@ class Role: Args: name: The name of the role (string) dependencies: Optional list of dependent Role objects + since: Earliest OpenStack release, inclusive, on which a collection + should deploy this role (string, e.g. "2025.2") + until: Latest such release, inclusive (string, e.g. "2025.1") + + ``since`` and ``until`` govern **collection membership only**: whether a + collection should deploy this role on a given release. They are NOT a + statement that the role's playbook exists, or that the role may be applied, + on those releases. ``valkey`` is the illustration -- its play ships from + OpenStack 2025.1, but collections deploy it only from 2025.2, because + ``osism/defaults`` leaves ``enable_valkey`` off until then. Reading the + bound as availability would wrongly reject ``osism apply valkey`` on 2025.1. + + Only collection expansion consults these bounds. ``osism apply `` + never does. Example: >>> role = Role("keystone", dependencies=[Role("glance"), Role("cinder")]) @@ -17,10 +33,64 @@ class Role: 2 """ - def __init__(self, name, dependencies=None): - """Initialize a Role with a name and optional dependencies.""" + def __init__(self, name, dependencies=None, since=None, until=None): + """Initialize a Role with a name, optional dependencies and bounds.""" self.name = name self.dependencies = dependencies or [] + # Parsed eagerly, unlike the deployed release: these are literals in + # this file, so a bad value is a bug here that should surface at once + # rather than on the one deployment whose release happens to test it. + self.since = parse_release(since) if since else None + self.until = parse_release(until) if until else None + + @property + def release_bounded(self): + """Whether collection membership depends on the OpenStack release.""" + return self.since is not None or self.until is not None + + def deployed_in(self, release): + """Whether a collection should deploy this role on ``release``. + + ``release`` is a ``(year, minor)`` tuple from ``osism.data.releases``. + """ + if self.since is not None and release < self.since: + return False + + if self.until is not None and release > self.until: + return False + + return True + + def bound_description(self): + """Render the bounds for log and error text, e.g. "from 2025.2". + + Only meaningful for a release-bounded role; raises ``ValueError`` on + one with neither bound, rather than silently rendering nonsense. + """ + if self.since is not None and self.until is not None: + return f"from {format_release(self.since)} to {format_release(self.until)}" + + if self.since is not None: + return f"from {format_release(self.since)}" + + if self.until is not None: + return f"up to {format_release(self.until)}" + + raise ValueError(f"{self.name!r} has no release bounds to describe") + + +def bounded_roles(roles): + """Yield every release-bounded Role reachable from ``roles``. + + Used to decide whether expanding a collection needs the release at all: a + collection with no bounded roles never triggers the lookup, and so can never + fail on it. + """ + for role in roles: + if role.release_bounded: + yield role + + yield from bounded_roles(role.dependencies) VALIDATE_PLAYBOOKS = { diff --git a/tests/unit/data/test_enums.py b/tests/unit/data/test_enums.py index f1706248d..008c8af59 100644 --- a/tests/unit/data/test_enums.py +++ b/tests/unit/data/test_enums.py @@ -1,9 +1,12 @@ # SPDX-License-Identifier: Apache-2.0 +import pytest + from osism.data.enums import ( MAP_ROLE2ROLE, VALIDATE_PLAYBOOKS, Role, + bounded_roles, ) @@ -284,3 +287,107 @@ def test_map_role2role_walk_handles_cycles(): assert {role.name for role in visited} == {"a", "b"} assert len(visited) == 2 + + +# --------------------------------------------------------------------------- +# Role release bounds +# --------------------------------------------------------------------------- + + +def test_role_bounds_default_to_none(): + role = Role("keystone") + + assert role.since is None + assert role.until is None + assert role.release_bounded is False + + +def test_role_bounds_parsed_to_tuples(): + role = Role("valkey", since="2025.2", until="2026.1") + + assert role.since == (2025, 2) + assert role.until == (2026, 1) + assert role.release_bounded is True + + +def test_role_rejects_unparseable_bound(): + """Catalog literals are parsed eagerly so a typo fails loudly, at once.""" + from osism.data.releases import ReleaseUnparseable + + with pytest.raises(ReleaseUnparseable): + Role("valkey", since="master") + + +def test_role_without_bounds_is_in_every_release(): + role = Role("keystone") + + assert role.deployed_in((2024, 1)) is True + assert role.deployed_in((2026, 1)) is True + + +def test_role_since_is_inclusive(): + role = Role("valkey", since="2025.2") + + assert role.deployed_in((2025, 1)) is False + assert role.deployed_in((2025, 2)) is True + assert role.deployed_in((2026, 1)) is True + + +def test_role_until_is_inclusive(): + role = Role("redis", until="2025.1") + + assert role.deployed_in((2024, 2)) is True + assert role.deployed_in((2025, 1)) is True + assert role.deployed_in((2025, 2)) is False + + +def test_role_both_bounds_form_a_closed_range(): + role = Role("interim", since="2025.2", until="2026.1") + + assert role.deployed_in((2025, 1)) is False + assert role.deployed_in((2025, 2)) is True + assert role.deployed_in((2026, 1)) is True + assert role.deployed_in((2026, 2)) is False + + +@pytest.mark.parametrize( + "kwargs,expected", + [ + ({"since": "2025.2"}, "from 2025.2"), + ({"until": "2025.1"}, "up to 2025.1"), + ({"since": "2025.2", "until": "2026.1"}, "from 2025.2 to 2026.1"), + ], +) +def test_role_bound_description(kwargs, expected): + assert Role("role", **kwargs).bound_description() == expected + + +def test_role_bound_description_raises_on_unbounded_role(): + """Nothing to describe on an unbounded Role; fail loud, not with nonsense.""" + with pytest.raises(ValueError): + Role("keystone").bound_description() + + +# --------------------------------------------------------------------------- +# bounded_roles +# --------------------------------------------------------------------------- + + +def test_bounded_roles_finds_nothing_when_unbounded(): + roles = [Role("a", dependencies=[Role("b")]), Role("c")] + + assert list(bounded_roles(roles)) == [] + + +def test_bounded_roles_finds_nested_bounds(): + valkey = Role("valkey", since="2025.2") + roles = [Role("a", dependencies=[Role("b", dependencies=[valkey])]), Role("c")] + + assert list(bounded_roles(roles)) == [valkey] + + +def test_bounded_roles_includes_a_bounded_parent_and_its_bounded_child(): + child = Role("child", since="2026.1") + parent = Role("parent", until="2025.1", dependencies=[child]) + + assert list(bounded_roles([parent])) == [parent, child] From b045b6800a7ca54ed246e9f0282741cf7ae2cf56 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Thu, 10 Sep 2026 12:13:14 +0200 Subject: [PATCH 3/5] apply: skip roles outside their release Teach collection expansion to drop a role whose bounds exclude the running release. A release of None means no bounded roles are involved, so nothing is filtered and every existing caller behaves as before. An excluded role must not take its subtree with it, so its dependencies are promoted into the surrounding group, keeping the ordering the retained ancestors impose. That makes both halves of a node optional, and neither can be appended unconditionally any more: the role's own task is absent when it is excluded, and the expansion of its dependencies is absent when that expansion came back empty. Appending regardless would build chain(pt, None) in the first case and group([None]) in the second. The empty case is reached recursively -- an excluded parent's subtree is empty only if each of its children resolved to nothing -- so the absence propagates outward through however many excluded levels there are. Promotion cannot preserve what the excluded role supplied to that subtree: chain(pt, st) runs a role BEFORE its dependencies, so they are its dependents and it is their prerequisite, and nothing at expansion time can know which retained or replacement role belongs in that place. Every bounded role in the catalog is a leaf, and a test added with the catalog change keeps it that way, so the question is forced on whoever first needs otherwise. show_tree also leaves the role's task absent, for an unrelated reason, so it returns before the append block rather than joining that logic; the skip is logged before it, which makes --show-tree a release preview. The skip is logged at info, not warning: on a release where the role does not belong this is correct output on every run, and a warning that always fires teaches operators to ignore warnings. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi --- osism/commands/apply.py | 91 ++++++++--- tests/unit/commands/test_apply.py | 242 ++++++++++++++++++++++++++++++ 2 files changed, 310 insertions(+), 23 deletions(-) diff --git a/osism/commands/apply.py b/osism/commands/apply.py index 12d946d62..7e7a2af05 100644 --- a/osism/commands/apply.py +++ b/osism/commands/apply.py @@ -11,6 +11,7 @@ from osism import utils from osism.data import enums from osism.data.enums import Role +from osism.data.releases import format_release def _collect_result(result): @@ -165,6 +166,7 @@ def _handle_collection( retry, dry_run, show_tree, + release=None, ): from celery import chain, group from osism.tasks import ansible @@ -182,27 +184,47 @@ def _handle_collection( role_name = item.name dependencies = item.dependencies - logger.info(f"A [{counter}] {'-' * (counter + 1)} {role_name}") - - if show_tree: - # Only show the tree, don't create tasks + if release is not None and not item.deployed_in(release): + # Not a warning: on a release where the role does not belong, + # this is correct output on every run, and a warning that always + # fires teaches operators to ignore warnings. + logger.info( + f"Skipping {role_name}: not deployed by this collection " + f"on OpenStack {format_release(release)} " + f"(deployed {item.bound_description()})" + ) pt = None - elif dry_run: - pt = ansible.noop.si() else: - pt = self._prepare_task( - arguments, - environment, - overwrite, - sub, - role_name, - action, - wait, - format, - timeout, - task_timeout, - ) + if release is None and item.release_bounded: + logger.warning( + f"{role_name} has release bounds " + f"({item.bound_description()}) but no OpenStack " + f"release was resolved; its bounds are being ignored " + f"and it will be scheduled unconditionally." + ) + + logger.info(f"A [{counter}] {'-' * (counter + 1)} {role_name}") + if show_tree: + # Only show the tree, don't create tasks + pt = None + elif dry_run: + pt = ansible.noop.si() + else: + pt = self._prepare_task( + arguments, + environment, + overwrite, + sub, + role_name, + action, + wait, + format, + timeout, + task_timeout, + ) + + st = None if dependencies: logger.debug(f"X [{counter + 1}] --> {dependencies}") st = self._handle_collection( @@ -221,12 +243,33 @@ def _handle_collection( retry, dry_run, show_tree, + release, ) - if not show_tree: - g.append(chain(pt, st)) - else: - if not show_tree: - g.append(pt) + + if show_tree: + continue + + # Both halves can be absent: pt when the role is excluded, st when + # its dependencies all were. Appending either unconditionally would + # build chain(pt, None) or group([None]). + if pt is not None and st is not None: + g.append(chain(pt, st)) + elif pt is not None: + g.append(pt) + elif st is not None: + # Promotion keeps the subtree and its position among retained + # siblings, but not what the excluded role supplied to it: + # chain(pt, st) ran the role BEFORE its dependencies, so they + # are its dependents and it is their prerequisite. Dropping it + # leaves them with no predecessor, and nothing here can know + # which retained or replacement role belongs in that place. + # + # Nothing checks that at runtime, deliberately. The catalog + # cannot grow a bounded role with dependencies without turning + # test_no_bounded_role_has_dependencies red first, so by the + # time this branch runs the ordering has already been settled + # by whoever made that test pass. + g.append(st) if g: return group(g) @@ -246,6 +289,7 @@ def handle_collection( retry, dry_run, show_tree, + release=None, ): if dry_run: logger.info(f"Dry run for collection {collection}. No tasks are scheduled.") @@ -270,6 +314,7 @@ def handle_collection( retry, dry_run, show_tree, + release, ) # Only apply tasks if not in show_tree mode diff --git a/tests/unit/commands/test_apply.py b/tests/unit/commands/test_apply.py index 1245079b9..66faffd29 100644 --- a/tests/unit/commands/test_apply.py +++ b/tests/unit/commands/test_apply.py @@ -111,6 +111,7 @@ def _public_collection_kwargs(**overrides): retry=0, dry_run=False, show_tree=False, + release=None, ) params.update(overrides) return params @@ -397,6 +398,247 @@ def test_handle_collection_show_tree_only_logs(task_mocks, loguru_logs): assert "A [1] -- child" in messages +# _handle_collection release bounds + + +def test_handle_collection_keeps_role_inside_its_bounds(mocker): + group_mock = mocker.patch("celery.group") + cmd = make_command(apply.Run) + pt = MagicMock(name="pt-redis") + cmd._prepare_task = MagicMock(return_value=pt) + + result = cmd._handle_collection( + [Role("redis", until="2025.1")], **_collection_kwargs(release=(2025, 1)) + ) + + group_mock.assert_called_once_with([pt]) + assert result is group_mock.return_value + + +def test_handle_collection_drops_role_outside_its_bounds(mocker, loguru_logs): + group_mock = mocker.patch("celery.group") + cmd = make_command(apply.Run) + cmd._prepare_task = MagicMock() + + result = cmd._handle_collection( + [Role("redis", until="2025.1")], **_collection_kwargs(release=(2026, 1)) + ) + + cmd._prepare_task.assert_not_called() + group_mock.assert_not_called() + assert result is None + assert any( + r["level"] == "INFO" + and r["message"] + == "Skipping redis: not deployed by this collection on OpenStack 2026.1 (deployed up to 2025.1)" + for r in loguru_logs + ) + + +def test_handle_collection_selects_between_two_bounded_roles(mocker): + group_mock = mocker.patch("celery.group") + cmd = make_command(apply.Run) + valkey_pt = MagicMock(name="pt-valkey") + cmd._prepare_task = MagicMock(return_value=valkey_pt) + roles = [Role("redis", until="2025.1"), Role("valkey", since="2025.2")] + + cmd._handle_collection(roles, **_collection_kwargs(release=(2026, 1))) + + assert [c.args[4] for c in cmd._prepare_task.call_args_list] == ["valkey"] + group_mock.assert_called_once_with([valkey_pt]) + + +def test_handle_collection_without_release_ignores_bounds(mocker): + """``release=None`` means no bounded roles were requested; filter nothing.""" + group_mock = mocker.patch("celery.group") + cmd = make_command(apply.Run) + prepared = [MagicMock(name="pt-redis"), MagicMock(name="pt-valkey")] + cmd._prepare_task = MagicMock(side_effect=prepared) + roles = [Role("redis", until="2025.1"), Role("valkey", since="2025.2")] + + cmd._handle_collection(roles, **_collection_kwargs(release=None)) + + group_mock.assert_called_once_with(prepared) + + +def test_handle_collection_without_release_warns_for_bounded_roles(mocker, loguru_logs): + """Diagnostic only: today the preflight guarantees this never happens, but + + if a future caller ever expands a collection with ``release=None`` while + it still contains bounded roles, that silent fail-open must be visible. + """ + mocker.patch("celery.group") + cmd = make_command(apply.Run) + cmd._prepare_task = MagicMock() + roles = [Role("redis", until="2025.1"), Role("valkey", since="2025.2")] + + cmd._handle_collection(roles, **_collection_kwargs(release=None)) + + warnings = [r["message"] for r in loguru_logs if r["level"] == "WARNING"] + assert any( + "redis" in m and "no OpenStack release was resolved" in m for m in warnings + ) + assert any( + "valkey" in m and "no OpenStack release was resolved" in m for m in warnings + ) + + +def test_handle_collection_with_release_does_not_warn_for_bounded_roles( + mocker, loguru_logs +): + """The warning is only for the ``release is None`` fail-open case.""" + mocker.patch("celery.group") + cmd = make_command(apply.Run) + cmd._prepare_task = MagicMock() + roles = [Role("valkey", since="2025.2")] + + cmd._handle_collection(roles, **_collection_kwargs(release=(2026, 1))) + + warnings = [r["message"] for r in loguru_logs if r["level"] == "WARNING"] + assert warnings == [] + + +# _handle_collection tree structure under exclusion + + +def test_handle_collection_excluded_parent_promotes_subtree(mocker): + """pt None, st a task: the subtree replaces the parent, unchained.""" + group_mock = mocker.patch("celery.group") + chain_mock = mocker.patch("celery.chain") + cmd = make_command(apply.Run) + child_pt = MagicMock(name="pt-child") + cmd._prepare_task = MagicMock(return_value=child_pt) + + result = cmd._handle_collection( + [Role("parent", until="2025.1", dependencies=[Role("child")])], + **_collection_kwargs(release=(2026, 1)), + ) + + assert [c.args[4] for c in cmd._prepare_task.call_args_list] == ["child"] + chain_mock.assert_not_called() + assert group_mock.call_args_list == [ + call([child_pt]), + call([group_mock.return_value]), + ] + assert result is group_mock.return_value + + +def test_handle_collection_excluded_parent_preserves_sibling_order(mocker): + """Promotion must not reorder the surrounding group.""" + group_mock = mocker.patch("celery.group") + mocker.patch("celery.chain") + cmd = make_command(apply.Run) + cmd._prepare_task = MagicMock(side_effect=lambda *args, **kwargs: args[4]) + roles = [ + Role("first"), + Role("gone", until="2025.1", dependencies=[Role("promoted")]), + Role("last"), + ] + + cmd._handle_collection(roles, **_collection_kwargs(release=(2026, 1))) + + outer = group_mock.call_args_list[-1].args[0] + assert outer == ["first", group_mock.return_value, "last"] + + +def test_handle_collection_retained_parent_all_children_excluded(mocker): + """pt a task, st None: schedule the parent alone, never chain(pt, None).""" + group_mock = mocker.patch("celery.group") + chain_mock = mocker.patch("celery.chain") + cmd = make_command(apply.Run) + parent_pt = MagicMock(name="pt-parent") + cmd._prepare_task = MagicMock(return_value=parent_pt) + + result = cmd._handle_collection( + [Role("parent", dependencies=[Role("child", until="2025.1")])], + **_collection_kwargs(release=(2026, 1)), + ) + + assert [c.args[4] for c in cmd._prepare_task.call_args_list] == ["parent"] + chain_mock.assert_not_called() + group_mock.assert_called_once_with([parent_pt]) + assert result is group_mock.return_value + + +def test_handle_collection_nested_fully_excluded_subtree(mocker): + """pt None, st None, nested: append nothing, never group([None]). + + The emptiness has to propagate all the way out as ``None`` rather than as an + empty group, through however many excluded levels there are. + """ + group_mock = mocker.patch("celery.group") + chain_mock = mocker.patch("celery.chain") + cmd = make_command(apply.Run) + cmd._prepare_task = MagicMock() + + result = cmd._handle_collection( + [ + Role( + "outer", + until="2025.1", + dependencies=[ + Role( + "middle", + until="2025.1", + dependencies=[Role("inner", until="2025.1")], + ) + ], + ) + ], + **_collection_kwargs(release=(2026, 1)), + ) + + cmd._prepare_task.assert_not_called() + chain_mock.assert_not_called() + group_mock.assert_not_called() + assert result is None + + +def test_handle_collection_all_roles_excluded_yields_nothing(mocker): + group_mock = mocker.patch("celery.group") + cmd = make_command(apply.Run) + cmd._prepare_task = MagicMock() + + result = cmd._handle_collection( + [Role("redis", until="2025.1"), Role("other", until="2024.2")], + **_collection_kwargs(release=(2026, 1)), + ) + + group_mock.assert_not_called() + assert result is None + + +def test_handle_collection_excluded_role_not_logged_as_applied(loguru_logs, mocker): + mocker.patch("celery.group") + cmd = make_command(apply.Run) + cmd._prepare_task = MagicMock() + + cmd._handle_collection( + [Role("redis", until="2025.1")], **_collection_kwargs(release=(2026, 1)) + ) + + assert not any(r["message"].startswith("A [0]") for r in loguru_logs) + + +def test_handle_collection_show_tree_still_reports_exclusions(loguru_logs): + cmd = make_command(apply.Run) + cmd._prepare_task = MagicMock() + + result = cmd._handle_collection( + [Role("redis", until="2025.1"), Role("valkey", since="2025.2")], + **_collection_kwargs(release=(2026, 1), show_tree=True), + ) + + assert result is None + cmd._prepare_task.assert_not_called() + messages = [r["message"] for r in loguru_logs] + assert ( + "Skipping redis: not deployed by this collection on OpenStack 2026.1 (deployed up to 2025.1)" + in messages + ) + assert "A [0] - valkey" in messages + + # handle_collection From 59776abe0e5216c5da85f6ffbe8045a0d4a8aeea Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Thu, 10 Sep 2026 12:13:31 +0200 Subject: [PATCH 4/5] apply: resolve the release up front Collection expansion can now filter on the release, but nothing supplies one. Resolve it in take_action and pass it down, with --openstack-version overriding the versions file. The resolution happens before the dispatch loop, not inside it. That loop iterates the //-separated entries and each handle_collection call ends in apply_async(), so resolving per entry would let "osism apply collection-monitoring//nutshell" schedule monitoring and only then fail on nutshell, having already touched the cluster. Done up front, a failed resolution schedules nothing at all, and the release is resolved exactly once, so every entry and every level of the recursion sees the same value even if the versions file changes mid-run. The lookup is skipped entirely for collections that contain no bounded roles, which is eight of the eleven, so such a command cannot fail on any of this. When it is needed and fails, the three causes are reported differently -- an unparseable value is already a sentence and is printed as is, while a missing file or key is appended to one naming the affected collections -- followed by the roles that needed the release and how to supply it. The advice names no file to edit: the versions file belongs to the kolla-ansible container, so the two ways an operator can supply a release are the environment variable and the flag. --openstack-version deliberately does not default from the environment the way sync.py does: openstack_release already consults it, and reading it here would make the override always truthy. Nor does it default to a release, which would be the silent guess this all exists to avoid. Note that argparse.REMAINDER on the trailing arguments swallows any flag placed after the collection name, so both the help text and the error advice lead with the environment variable, which is order-independent. Bounds stay out of the single-role path: "osism apply " never consults them, because a bound is collection membership and not a claim that the play exists. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi --- osism/commands/apply.py | 72 ++++++++ tests/unit/commands/test_apply.py | 290 +++++++++++++++++++++++++++++- 2 files changed, 361 insertions(+), 1 deletion(-) diff --git a/osism/commands/apply.py b/osism/commands/apply.py index 7e7a2af05..d12ac317e 100644 --- a/osism/commands/apply.py +++ b/osism/commands/apply.py @@ -107,6 +107,19 @@ def get_parser(self, prog_name): help="Dry run, do not initiate tasks (for collections only)", action="store_true", ) + parser.add_argument( + "--openstack-version", + default=None, + type=str, + help=( + "OpenStack release the collection is applied to, e.g. 2026.1. " + "Collections deploy a few roles only on certain releases (redis " + "up to 2025.1, valkey from 2025.2). Read from openstack_version " + "in /interface/versions/kolla-ansible.yml when not given. Must " + "be given before the collection name, or it is swallowed as an " + "Ansible argument. (env: OPENSTACK_VERSION)" + ), + ) parser.add_argument( "--show-tree", dest="show_tree", @@ -329,6 +342,49 @@ def handle_collection( return 0 + def _resolve_release(self, override, bounded): + """Return the deployed release, or exit reporting why it is unknown. + + ``bounded`` maps collection name to the release-bounded roles it + contains; it is used only to name what needed the release. + """ + from osism.data.releases import ( + ReleaseUndetermined, + ReleaseUnparseable, + openstack_release, + ) + + try: + return openstack_release(override) + except ReleaseUnparseable as exc: + # Already a complete sentence. + logger.error(str(exc)) + except ReleaseUndetermined as exc: + names = ", ".join(sorted(bounded)) + noun = "Collection" if len(bounded) == 1 else "Collections" + verb = "contains" if len(bounded) == 1 else "contain" + logger.error( + f"{noun} {names} {verb} roles that depend on the OpenStack " + f"release, but the release could not be determined: {exc}" + ) + + # One role can appear in several collections; report it once. + descriptions = {} + for roles in bounded.values(): + for role in roles: + descriptions[role.name] = role.bound_description() + + affected = ", ".join( + f"{name} ({descriptions[name]})" for name in sorted(descriptions) + ) + logger.error(f"Affected roles: {affected}") + logger.error( + "Supply the release with OPENSTACK_VERSION= osism apply " + ", or with --openstack-version before the " + "collection name." + ) + exit(1) + def _prepare_task( self, arguments, @@ -502,6 +558,21 @@ def take_action(self, parsed_args): rc = 0 + # Resolve the release before the dispatch loop below, not inside it: each + # iteration ends in apply_async(), so resolving per entry would let an + # earlier collection reach the cluster before a later one failed. + release = None + if role: + bounded = {} + for entry in role.split("//"): + if entry in enums.MAP_ROLE2ROLE: + found = list(enums.bounded_roles(enums.MAP_ROLE2ROLE[entry])) + if found: + bounded[entry] = found + + if bounded: + release = self._resolve_release(parsed_args.openstack_version, bounded) + if not role: table = [] for role in MAP_ROLE2ENVIRONMENT: @@ -529,6 +600,7 @@ def take_action(self, parsed_args): retry, dry_run, show_tree, + release=release, ) if rc != 0: outer_break = True diff --git a/tests/unit/commands/test_apply.py b/tests/unit/commands/test_apply.py index 66faffd29..ffa476f59 100644 --- a/tests/unit/commands/test_apply.py +++ b/tests/unit/commands/test_apply.py @@ -35,7 +35,7 @@ from osism.data import enums, playbooks from osism.data.enums import Role -from ._helpers import assert_not_called_before_lock_check, make_command +from ._helpers import assert_not_called_before_lock_check, make_command, parse_args @pytest.fixture(autouse=True) @@ -997,3 +997,291 @@ def test_take_action_collection_chain_continues_after_success(take_action_mocks) assert rc == 0 cmd.handle_role.assert_called_once() assert cmd.handle_role.call_args.args[4] == "other" + + +# take_action release preflight + + +def _bounded_collection(): + return [Role("valkey", since="2025.2"), Role("plain")] + + +def test_apply_accepts_openstack_version_argument(): + _, parsed = parse_args(apply.Run, ["--openstack-version", "2026.1", "nutshell"]) + + assert parsed.openstack_version == "2026.1" + + +def test_apply_openstack_version_defaults_to_none(): + _, parsed = parse_args(apply.Run, ["nutshell"]) + + assert parsed.openstack_version is None + + +def test_apply_openstack_version_help_warns_about_ordering(): + """``arguments`` is ``REMAINDER``: a flag after the role is swallowed.""" + cmd = make_command(apply.Run) + parser = cmd.get_parser("test") + action = next( + a for a in parser._actions if "--openstack-version" in a.option_strings + ) + + assert "before the collection name" in action.help + + +def test_apply_openstack_version_help_names_the_versions_file(): + """The fallback source has to be the one the CLI actually reads. + + The manager configuration is not it: openstack_version is stripped from + that file for a stable release, so help text pointing an operator there + would describe a source that is absent on every pinned deployment. + """ + from osism.data import releases + + cmd = make_command(apply.Run) + parser = cmd.get_parser("test") + action = next( + a for a in parser._actions if "--openstack-version" in a.option_strings + ) + + assert releases.VERSIONS_FILE in action.help + assert "/opt/configuration" not in action.help + + +def test_take_action_passes_resolved_release_to_collection(take_action_mocks): + cmd, parsed = parse_args( + apply.Run, ["--openstack-version", "2026.1", "testcollection"] + ) + cmd.handle_collection = MagicMock(return_value=0) + + with patch.dict(enums.MAP_ROLE2ROLE, {"testcollection": _bounded_collection()}): + cmd.take_action(parsed) + + assert cmd.handle_collection.call_args.kwargs["release"] == (2026, 1) + + +def test_take_action_unbounded_collection_never_resolves_release( + take_action_mocks, mocker +): + resolve = mocker.patch.object(apply.Run, "_resolve_release") + cmd, parsed = parse_args(apply.Run, ["testcollection"]) + cmd.handle_collection = MagicMock(return_value=0) + + with patch.dict(enums.MAP_ROLE2ROLE, {"testcollection": [Role("plain")]}): + cmd.take_action(parsed) + + resolve.assert_not_called() + assert cmd.handle_collection.call_args.kwargs["release"] is None + + +def test_take_action_undeterminable_release_exits( + take_action_mocks, mocker, loguru_logs +): + from osism.data import releases + + mocker.patch.object( + releases, + "openstack_release", + side_effect=releases.ReleaseUndetermined("/some/versions.yml not found."), + ) + cmd, parsed = parse_args(apply.Run, ["testcollection"]) + cmd.handle_collection = MagicMock(return_value=0) + + with patch.dict(enums.MAP_ROLE2ROLE, {"testcollection": _bounded_collection()}): + with pytest.raises(SystemExit) as excinfo: + cmd.take_action(parsed) + + assert excinfo.value.code == 1 + cmd.handle_collection.assert_not_called() + messages = [r["message"] for r in loguru_logs if r["level"] == "ERROR"] + assert any( + "Collection testcollection contains roles that depend on the OpenStack " + "release, but the release could not be determined: " + "/some/versions.yml not found." == m + for m in messages + ) + assert any("Affected roles: valkey (from 2025.2)" == m for m in messages) + assert any( + m.startswith("Supply the release with OPENSTACK_VERSION=") + and "--openstack-version" in m + for m in messages + ) + + +def test_take_action_undeterminable_release_advice_is_order_independent( + take_action_mocks, mocker, loguru_logs +): + """The advice must not recommend a flag placement that silently fails. + + ``arguments`` is ``nargs=argparse.REMAINDER``, so a flag placed after the + collection name is swallowed as an Ansible argument rather than parsed. + The advice therefore leads with the env-var form and, if it mentions the + flag at all, says where it must go. It names no file to edit either: the + versions file it failed to read belongs to the kolla-ansible container. + """ + from osism.data import releases + + mocker.patch.object( + releases, + "openstack_release", + side_effect=releases.ReleaseUndetermined("/some/versions.yml not found."), + ) + cmd, parsed = parse_args(apply.Run, ["testcollection"]) + cmd.handle_collection = MagicMock(return_value=0) + + with patch.dict(enums.MAP_ROLE2ROLE, {"testcollection": _bounded_collection()}): + with pytest.raises(SystemExit): + cmd.take_action(parsed) + + messages = [r["message"] for r in loguru_logs if r["level"] == "ERROR"] + assert any( + m + == ( + "Supply the release with OPENSTACK_VERSION= osism apply " + ", or with --openstack-version before the " + "collection name." + ) + for m in messages + ) + + +def test_take_action_unparseable_release_prints_bare_message( + take_action_mocks, mocker, loguru_logs +): + cmd, parsed = parse_args( + apply.Run, ["--openstack-version", "master", "testcollection"] + ) + cmd.handle_collection = MagicMock(return_value=0) + + with patch.dict(enums.MAP_ROLE2ROLE, {"testcollection": _bounded_collection()}): + with pytest.raises(SystemExit): + cmd.take_action(parsed) + + messages = [r["message"] for r in loguru_logs if r["level"] == "ERROR"] + assert any( + m == "Could not parse OpenStack release 'master' " + "(expected a release like 2025.1)." + for m in messages + ) + assert not any("could not be determined" in m for m in messages) + + +def test_take_action_preflight_schedules_nothing_across_slashes( + take_action_mocks, mocker +): + """The guarantee is command-wide, not collection-wide. + + Resolving inside the dispatch loop would let the first entry reach + apply_async before the second one failed. + """ + from osism.data import releases + + mocker.patch.object( + releases, + "openstack_release", + side_effect=releases.ReleaseUndetermined("/some/versions.yml not found."), + ) + cmd, parsed = parse_args(apply.Run, ["unbounded//testcollection"]) + cmd._handle_collection = MagicMock() + + with patch.dict( + enums.MAP_ROLE2ROLE, + {"unbounded": [Role("plain")], "testcollection": _bounded_collection()}, + ): + with pytest.raises(SystemExit): + cmd.take_action(parsed) + + cmd._handle_collection.assert_not_called() + + +def test_take_action_resolves_the_release_once_per_invocation( + take_action_mocks, mocker +): + from osism.data import releases + + resolve = mocker.patch.object(releases, "openstack_release", return_value=(2026, 1)) + cmd, parsed = parse_args(apply.Run, ["testcollection//othercollection"]) + cmd.handle_collection = MagicMock(return_value=0) + + with patch.dict( + enums.MAP_ROLE2ROLE, + { + "testcollection": _bounded_collection(), + "othercollection": [Role("redis", until="2025.1")], + }, + ): + cmd.take_action(parsed) + + resolve.assert_called_once() + assert [c.kwargs["release"] for c in cmd.handle_collection.call_args_list] == [ + (2026, 1), + (2026, 1), + ] + + +def test_take_action_names_every_affected_collection( + take_action_mocks, mocker, loguru_logs +): + from osism.data import releases + + mocker.patch.object( + releases, + "openstack_release", + side_effect=releases.ReleaseUndetermined("/some/versions.yml not found."), + ) + cmd, parsed = parse_args(apply.Run, ["testcollection//othercollection"]) + + with patch.dict( + enums.MAP_ROLE2ROLE, + { + "testcollection": _bounded_collection(), + "othercollection": [Role("redis", until="2025.1")], + }, + ): + with pytest.raises(SystemExit): + cmd.take_action(parsed) + + messages = [r["message"] for r in loguru_logs if r["level"] == "ERROR"] + assert any( + m.startswith("Collections othercollection, testcollection contain roles") + for m in messages + ) + assert any( + m == "Affected roles: redis (up to 2025.1), valkey (from 2025.2)" + for m in messages + ) + + +# bounds are collection membership, never role availability + + +def test_take_action_explicit_bounded_role_ignores_bounds(take_action_mocks, mocker): + """``osism apply valkey`` on 2025.1 must dispatch. + + valkey's play exists on 2025.1; the ``since="2025.2"`` bound says only that + collections should not deploy it there. Reading the bound as availability + would reject a legitimate command. + """ + resolve = mocker.patch.object(apply.Run, "_resolve_release") + cmd, parsed = parse_args(apply.Run, ["--openstack-version", "2025.1", "valkey"]) + cmd.handle_role = MagicMock(return_value=0) + + cmd.take_action(parsed) + + resolve.assert_not_called() + # handle_role is called positionally: (arguments, environment, overwrite, + # sub, role, ...), so the role is args[4]. + assert cmd.handle_role.call_args.args[4] == "valkey" + + +def test_take_action_explicit_redis_still_dispatches_on_2026_1( + take_action_mocks, mocker +): + resolve = mocker.patch.object(apply.Run, "_resolve_release") + cmd, parsed = parse_args(apply.Run, ["--openstack-version", "2026.1", "redis"]) + cmd.handle_role = MagicMock(return_value=0) + + cmd.take_action(parsed) + + resolve.assert_not_called() + assert cmd.handle_role.call_args.args[4] == "redis" From 567060197e76aa5331e5b5ea03692b3018f0cfe0 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Thu, 10 Sep 2026 12:13:52 +0200 Subject: [PATCH 5/5] enums: deploy valkey from 2025.2 kolla replaced Redis with Valkey at OpenStack 2025.2: from that release kolla-redis.yml is not shipped and playbooks.yml has no redis entry. The three collections that listed Role("redis") -- nutshell, collection-infrastructure and cloudpod-infrastructure -- therefore carry a role that cannot resolve on 2025.2 and later, and valkey was never added, so nothing deploys the replacement either. On 2026.1 that aborts "osism apply nutshell" once common succeeds, and every coordination backend osism/defaults points at valkey has no backend at all. Give each of the three collections both backends under disjoint, adjacent bounds, so exactly one is selected on any release: never both, never neither. On 2025.1 both plays exist, and the bound rather than the playbook is what keeps valkey out, matching enable_valkey in osism/defaults. A second test pins that a bounded role has no dependencies. Excluding one promotes its dependents but not the prerequisite it was, and nothing at expansion time can supply a replacement, so the resulting graph can be well formed and still ordered wrongly. Forcing that decision in CI, in the change that introduces such a role, is the only place it is cheap to make: kolla 2026.1 also dissolved common into logs, kolla_toolbox, cron and fluentd, and common has six dependents, so that migration trips this test rather than silently reordering a deployment. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi --- osism/data/enums.py | 11 +++- tests/unit/data/test_enums.py | 106 ++++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 3 deletions(-) diff --git a/osism/data/enums.py b/osism/data/enums.py index b3e69fb23..b9e188931 100644 --- a/osism/data/enums.py +++ b/osism/data/enums.py @@ -228,7 +228,10 @@ def bounded_roles(roles): ), Role("openvswitch", dependencies=[Role("ovn")]), Role("memcached"), - Role("redis"), + # kolla replaced redis with valkey at OpenStack 2025.2; on 2025.1 + # both plays exist but osism/defaults leaves enable_valkey off. + Role("redis", until="2025.1"), + Role("valkey", since="2025.2"), Role("rabbitmq"), ], ), @@ -289,7 +292,8 @@ def bounded_roles(roles): ), Role("openvswitch", dependencies=[Role("ovn")]), Role("memcached"), - Role("redis"), + Role("redis", until="2025.1"), + Role("valkey", since="2025.2"), Role("rabbitmq"), ], ), @@ -433,7 +437,8 @@ def bounded_roles(roles): ), Role("openvswitch", dependencies=[Role("ovn")]), Role("memcached"), - Role("redis"), + Role("redis", until="2025.1"), + Role("valkey", since="2025.2"), Role("rabbitmq"), ], ), diff --git a/tests/unit/data/test_enums.py b/tests/unit/data/test_enums.py index 008c8af59..aceb2a3c4 100644 --- a/tests/unit/data/test_enums.py +++ b/tests/unit/data/test_enums.py @@ -391,3 +391,109 @@ def test_bounded_roles_includes_a_bounded_parent_and_its_bounded_child(): parent = Role("parent", until="2025.1", dependencies=[child]) assert list(bounded_roles([parent])) == [parent, child] + + +# --------------------------------------------------------------------------- +# The key-value store cut-over +# --------------------------------------------------------------------------- + + +KVS_COLLECTIONS = ["nutshell", "collection-infrastructure", "cloudpod-infrastructure"] + + +@pytest.mark.parametrize("collection", KVS_COLLECTIONS) +def test_kvs_collections_carry_both_backends(collection): + roles = MAP_ROLE2ROLE[collection] + + assert find_role(roles, "redis") is not None + assert find_role(roles, "valkey") is not None + + +@pytest.mark.parametrize("collection", KVS_COLLECTIONS) +def test_kvs_backends_are_bounded_and_disjoint(collection): + roles = MAP_ROLE2ROLE[collection] + redis = find_role(roles, "redis") + valkey = find_role(roles, "valkey") + + assert redis.until == (2025, 1) + assert redis.since is None + assert valkey.since == (2025, 2) + assert valkey.until is None + + +@pytest.mark.parametrize("collection", KVS_COLLECTIONS) +@pytest.mark.parametrize( + "release,expected", + [ + ((2024, 2), "redis"), + ((2025, 1), "redis"), + ((2025, 2), "valkey"), + ((2026, 1), "valkey"), + ], +) +def test_exactly_one_backend_per_release(collection, release, expected): + """Never both, never neither -- on any release, past or future.""" + roles = MAP_ROLE2ROLE[collection] + selected = [ + name + for name in ("redis", "valkey") + if find_role(roles, name).deployed_in(release) + ] + + assert selected == [expected] + + +def test_only_the_kvs_roles_carry_bounds(): + """A bound anywhere else is new and wants its own test above.""" + bounded = { + role.name for roles in MAP_ROLE2ROLE.values() for role in bounded_roles(roles) + } + + assert bounded == {"redis", "valkey"} + + +def test_no_bounded_role_has_dependencies(): + """A release-bounded role must be a leaf, until someone settles ordering. + + Excluding a role promotes its dependencies into the surrounding group so + the subtree survives. That keeps the subtree and its position among + retained siblings, but not what the excluded role supplied to it: + ``chain(pt, st)`` runs a role BEFORE its ``dependencies``, so they are its + dependents and it is their prerequisite. Promote them past an excluded + parent and they run with no predecessor -- a well-formed task graph that + may be ordered wrongly, which is worse than an error because it looks + intentional. Nothing at expansion time can know which retained or + replacement role belongs in that place. + + So the decision is forced here, in CI, in the change that introduces such + a role -- not at deploy time, and not only for whoever reads the design + document. The case this exists for is the kolla 2026.1 split of ``common`` + into ``logs``, ``kolla_toolbox``, ``cron`` and ``fluentd``: ``common`` has + six dependents, so bounding it trips this test. + + If you are here because you added one: decide what runs in the excluded + role's place on each release, encode that, and replace this test with one + that pins the ordering you chose. + """ + offenders = { + role.name: [dependency.name for dependency in role.dependencies] + for roles in MAP_ROLE2ROLE.values() + for role in bounded_roles(roles) + if role.dependencies + } + + assert not offenders, ( + f"release-bounded roles with dependencies: {offenders}. " + "Excluding one promotes its dependents but not the prerequisite it " + "was; settle the ordering and replace this test. See the docstring." + ) + + +def test_valkey_absent_from_collections_that_never_had_redis(): + """The cut-over touches exactly the three collections that listed redis.""" + for name, roles in MAP_ROLE2ROLE.items(): + if name in KVS_COLLECTIONS: + continue + + assert find_role(roles, "valkey") is None, name + assert find_role(roles, "redis") is None, name