NO-ISSUE: restore OSAC-1992 JIT storage extra_vars in 5 AAP playbooks - #192
Merged
openshift-merge-bot[bot] merged 2 commits intoAug 6, 2026
Conversation
PR osac-project#97 (OSAC-3547, ansible_eda -> osac_job_vars rename) branched before PR osac-project#99 (OSAC-1992, STORAGE_TIERS -> storage_tier_definitions/storage_backend_connections) merged, and was itself merged ~8 hours later without rebasing onto the new main. Its mechanical rename carried the pre-PR99 STORAGE_TIERS-based logic underneath, silently discarding PR osac-project#99's functional changes on the 5 files they both touched while correctly renaming everything else. Restores PR osac-project#99's logic via 3-way merge (base: pre-PR99 commit, theirs: PR osac-project#99's merged content) against current main, then renames the restored ansible_eda.event.* references to osac_job_vars.* to match OSAC-3547. osac-operator already emits osac_job_vars.storage_tier_definitions and osac_job_vars.storage_backend_connections on current main; only the AAP-side consumers were out of sync. Also drops storage_provider_snapshots_enabled from the ComputeInstance, tenant-cluster-storage, and tenant-storage-backend create playbooks' include_role calls, matching PR osac-project#99's original reviewed decision to rely on the role's own default (true) rather than re-threading a static env var lookup through the dynamic tier-driven path. Verified: ansible-lint clean, ansible-playbook --syntax-check clean on all 5 files, diff scoped to exactly the 5 regressed files (confirmed via full audit of all 32 files PR osac-project#99 touched -- no other collateral damage). Generated-By: Claude Code (Anthropic) Signed-off-by: Will Gordon <wgordon@redhat.com>
|
@wgordon17: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughFive Ansible playbooks now read storage configuration from ChangesStorage event migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@osac-aap/playbook_osac_delete_tenant_cluster_storage.yml`:
- Around line 73-78: Update the “Clean up cluster-side tenant storage resources”
include_role task to pass osac_job_vars.storage_backend_connections alongside
storage_provider_tiers when invoking osac.service.storage_provider with
storage_provider_action set to teardown_cluster_storage, matching the other
storage-provider calls.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8fac7664-025e-475b-b3a4-477430a83593
📒 Files selected for processing (5)
osac-aap/playbook_osac_create_compute_instance.ymlosac-aap/playbook_osac_create_tenant_cluster_storage.ymlosac-aap/playbook_osac_create_tenant_storage_backend.ymlosac-aap/playbook_osac_delete_tenant_cluster_storage.ymlosac-aap/playbook_osac_delete_tenant_storage_backend.yml
Comment on lines
73
to
+78
| - name: Clean up cluster-side tenant storage resources | ||
| when: _storage_tiers_raw | length > 0 | ||
| ansible.builtin.include_role: | ||
| name: osac.service.storage_provider | ||
| vars: | ||
| storage_provider_action: teardown_cluster_storage | ||
| storage_provider_tiers: "{{ _storage_tiers }}" | ||
| storage_provider_tiers: "{{ osac_job_vars.storage_tier_definitions }}" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Pass backend connections to cluster teardown.
Line 78 forwards tier definitions but not osac_job_vars.storage_backend_connections. The other storage-provider calls forward this event field. Cluster teardown can run without its required backend configuration.
Proposed fix
storage_provider_action: teardown_cluster_storage
storage_provider_tiers: "{{ osac_job_vars.storage_tier_definitions }}"
+ storage_provider_backend_connections: "{{ osac_job_vars.storage_backend_connections | default({}) }}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Clean up cluster-side tenant storage resources | |
| when: _storage_tiers_raw | length > 0 | |
| ansible.builtin.include_role: | |
| name: osac.service.storage_provider | |
| vars: | |
| storage_provider_action: teardown_cluster_storage | |
| storage_provider_tiers: "{{ _storage_tiers }}" | |
| storage_provider_tiers: "{{ osac_job_vars.storage_tier_definitions }}" | |
| - name: Clean up cluster-side tenant storage resources | |
| ansible.builtin.include_role: | |
| name: osac.service.storage_provider | |
| vars: | |
| storage_provider_action: teardown_cluster_storage | |
| storage_provider_tiers: "{{ osac_job_vars.storage_tier_definitions }}" | |
| storage_provider_backend_connections: "{{ osac_job_vars.storage_backend_connections | default({}) }}" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@osac-aap/playbook_osac_delete_tenant_cluster_storage.yml` around lines 73 -
78, Update the “Clean up cluster-side tenant storage resources” include_role
task to pass osac_job_vars.storage_backend_connections alongside
storage_provider_tiers when invoking osac.service.storage_provider with
storage_provider_action set to teardown_cluster_storage, matching the other
storage-provider calls.
Every current environment that registers a storage tier via the Tier API (register-local-storage.yaml, gated on lvms.enabled) or the now-removed static STORAGE_TIERS config names it 'local', never 'default'. No environment defines a tier literally named 'default' -- that name only ever appeared in storage_provider's own VAST-oriented unit test fixtures and the sample payload, not in any deployed config. Changes the two independent hardcoded tier-name fallbacks to 'local' to match: playbook_osac_create_compute_instance.yml's _requested_storage_tier (used when STORAGE_REQUESTED_TIER is unset, which it always is today), and ocp_virt_vm/create_resources.yaml's tenant_storage_class_storage_tier fallback (used when _requested_storage_tier is entirely undefined, i.e. when the role is reached via osac.workflows.compute_instance.create instead of the real top-level playbook). Also finishes OSAC-1992's stated goal of eliminating STORAGE_TIERS: removes it from osac-installer's chart default (values.yaml), schema (values.schema.json), and the vmaas-ci CI override -- the only environment that runs the ComputeInstance controller (caas-ci and bmaas-ci both have computeInstance: false). Confirmed via helm template rendering: the storage-operations-ig ConfigMap previously rendered STORAGE_TIERS with the 'local' LVMS tier; after this change that key is gone entirely and the Tier-API-backed register-local-storage.yaml hook (already registering the same 'local' tier/backend) is the only source of truth. Generated-By: Claude Code (Anthropic) Signed-off-by: Will Gordon <wgordon@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akshaynadkarni, wgordon17 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 tasks
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
3 participants
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.
Summary
ansible_eda->osac_job_varsrename — restoresstorage_tier_definitions/storage_backend_connectionsextra_vars handling in all 5, confirmed via full audit of all 32 files PR OSAC-1992: read tier definitions and backend credentials from AAP extra_vars #99 originally touchedregister-local-storage.yamlor the now-removed staticSTORAGE_TIERS) names itlocal, neverdefault— changes both hardcoded tier-name fallbacks (playbook_osac_create_compute_instance.ymlandocp_virt_vm/create_resources.yaml) to matchSTORAGE_TIERSentirely — removed from osac-installer's chart default, schema, and thevmaas-ciCI override (the only environment running the ComputeInstance controller); verified viahelm templatethat the rendered ConfigMap no longer carries it and the Tier-API hook remains the sole source of truthSummary by CodeRabbit
localtier.