[set_containers] Add cifmw.general.set_containers module#3955
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Build failed (check pipeline). Post ✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 18m 45s |
arxcruz
force-pushed
the
feature/set-containers-module
branch
from
4b08e81 to
7ff95ec
Compare
|
Build failed (check pipeline). Post ✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 26m 34s |
arxcruz
force-pushed
the
feature/set-containers-module
branch
3 times, most recently
from
7863e46 to
507f3c3
Compare
Replace the update_containers role invocation in deploy_architecture with the new cifmw.general.set_containers module. The module generates and optionally applies an OpenStackVersion CR, accepting a dynamic images list where each entry can override registry, org, name_prefix, and tag individually. This removes the rigid Jinja2 template in favor of a Python module that is easier to test, extend, and call from any playbook without role-level variable coupling. Key design points: - _OPENSTACK_SUFFIXES table enables partial overrides (e.g. tag-only) for any standard OpenStack image field without repeating the suffix. - Empty name_prefix is supported so images that ship without the "openstack-" prefix (EDPM, IPA) can be built from parts rather than requiring a pre-assembled full_registry URL. - backends list handles cinderVolumeImages / manilaShareImages nested dicts. - Unit tests cover _build_url, _resolve_image, _resolve_backend, _build_cr, and the full run_module flow including apply, absent, and validation paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Arx Cruz <arxcruz@redhat.com>
arxcruz
force-pushed
the
feature/set-containers-module
branch
from
507f3c3 to
20386f6
Compare
| registry: "{{ cifmw_set_containers_registry | default(cifmw_update_containers_registry) }}" | ||
| org: "{{ cifmw_set_containers_org | default(cifmw_update_containers_org) }}" | ||
| tag: "{{ cifmw_set_containers_tag | default(cifmw_update_containers_tag) }}" |
There was a problem hiding this comment.
If neither variable in the chain is defined, Ansible raises UndefinedError. The inner default() evaluates cifmw_update_containers_registry eagerly — when it is also undefined, it fails.
Safe pattern:
registry: "{{ cifmw_set_containers_registry | default(cifmw_update_containers_registry | default(omit)) }}"
org: "{{ cifmw_set_containers_org | default(cifmw_update_containers_org | default(omit)) }}"
tag: "{{ cifmw_set_containers_tag | default(cifmw_update_containers_tag | default(omit)) }}"(include_openstack and apply already chain default(false), so they're fine.)
| # TODO: remove the default() fallbacks below once all callers in ci-framework-jobs | ||
| # are migrated from cifmw_update_containers_* to cifmw_set_containers_*. | ||
| # Scenario-specific variables that also require explicit per-scenario migration: | ||
| # cifmw_update_containers_cindervolumes -> cifmw_set_containers_images (backends) | ||
| # cifmw_update_containers_manilashares -> cifmw_set_containers_images (backends) | ||
| # cifmw_update_containers_watcher -> cifmw_set_containers_extra_images | ||
| # cifmw_update_containers_edpm_image_url -> cifmw_set_containers_images (name_prefix: "") | ||
| # cifmw_update_containers_ipa_image_url -> cifmw_set_containers_images (name_prefix: "") |
There was a problem hiding this comment.
Regression risk during transition. Cross-referencing update_containers.j2 against downstream jobs, these old variables are actively set in production CI and have no backward-compat wiring here:
| Old variable | Where it's set | What silently breaks |
|---|---|---|
cifmw_update_containers_edpm_image_url |
scenarios/uni/default-vars.yaml, scenarios/baremetal/default-vars.yaml, periodic configs |
osContainerImage missing from CR |
cifmw_update_containers_ipa_image_url |
same files | ironicPythonAgentImage missing |
cifmw_update_containers_barbican_custom_tag |
zuul.d/adoption.yaml, barbican component jobs |
barbican images get wrong tag |
cifmw_update_containers_watcher |
uni03gamma, uni05epsilon, podified watcher scenarios | watcher images missing |
cifmw_update_containers_cindervolumes_extra |
uni09iota (Pure Storage FC) | extra cinder backends missing |
Consider adding backward-compat fallbacks in the images parameter construction (at least for the full-URL ones like edpm_image_url and ipa_image_url), or gate this change behind a feature flag so callers can opt in after migrating their variables.
| (cifmw_set_containers_images | default([])) + | ||
| (cifmw_set_containers_extra_images | default([])) | ||
| }} | ||
| tags: |
There was a problem hiding this comment.
Tag rename from update_containers to set_containers — one downstream hit confirmed:
# ci-framework-jobs/playbooks/adoption_uni_jobs/operators/install_operators.yaml
--skip-tags=openstack_ca,edpm_post,update_containers
After this merges, that --skip-tags will silently stop filtering this task. Worth a coordinated update or keeping both tags during transition.
| if not os.path.exists(dest_path): | ||
| module.exit_json(**result) | ||
| if not module.check_mode: | ||
| if kubeconfig: |
There was a problem hiding this comment.
For state=present, cluster interaction requires apply=true. Here, oc delete fires whenever kubeconfig is set.
The old role used a separate boolean (cifmw_update_containers_rollback) to gate oc delete in cleanup.yml — not kubeconfig presence. A caller that sets kubeconfig for other purposes and passes state=absent expecting only local file cleanup will accidentally delete the CR from the cluster.
Suggest gating on apply for consistency:
if kubeconfig and module.params["apply"]:| """Return (backend_name, image_url) for one item in a backends list.""" | ||
| if isinstance(backend, str): | ||
| return backend, parent_url | ||
| name = backend.get("name") |
There was a problem hiding this comment.
backend.get("name") returns None when the key is missing, which silently inserts None as a dict key in the CR. Add validation:
name = backend.get("name")
if not name:
if module:
module.fail_json(msg="backends entry is a dict but has no 'name' key")
return None, parent_url| } | ||
|
|
||
|
|
||
| def _eff(per_image_val, module_val): |
There was a problem hiding this comment.
nit: _eff is cryptic for anyone reading the module cold. _effective or _fallback would be self-explanatory at no cost.
| return backend, parent_url | ||
| name = backend.get("name") | ||
| url = _resolve_image(backend, mod_registry, mod_org, mod_prefix, mod_tag, module) | ||
| return name, url if url is not None else parent_url |
There was a problem hiding this comment.
nit: Precedence is correct but explicit grouping prevents a double-take:
return name, (url if url is not None else parent_url)| env = {"KUBECONFIG": kubeconfig} if kubeconfig else {} | ||
| rc, out, err = module.run_command( | ||
| [oc_bin] + args, environ_update=env if env else None |
There was a problem hiding this comment.
nit: The double conditional can be collapsed:
environ_update = {"KUBECONFIG": kubeconfig} if kubeconfig else None
rc, out, err = module.run_command([oc_bin] + args, environ_update=environ_update)| written = [] | ||
|
|
||
| def fake_open(path, mode="r"): | ||
| m = mock_open()() | ||
| if mode == "r": |
There was a problem hiding this comment.
written and fake_open are defined but never used — the with patch(...) block below uses a different mock_open().
Can we remove them?
2 participants
Replace the update_containers role invocation in deploy_architecture with the new cifmw.general.set_containers module. The module generates and optionally applies an OpenStackVersion CR, accepting a dynamic images list where each entry can override registry, org, name_prefix, and tag individually. This removes the rigid Jinja2 template in favor of a Python module that is easier to test, extend, and call from any playbook without role-level variable coupling.
Key design points: