Conversation
…aybooks Signed-off-by: Will Gordon <wgordon@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
…playbooks Signed-off-by: Will Gordon <wgordon@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Will Gordon <wgordon@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
…instance tier lookup Signed-off-by: Will Gordon <wgordon@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Will Gordon <wgordon@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
…rbing OSAC-1957 Signed-off-by: Will Gordon <wgordon@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Will Gordon <wgordon@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
…key mismatch Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Will Gordon <wgordon@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Will Gordon <wgordon@redhat.com>
… infra Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Will Gordon <wgordon@redhat.com>
…ig template Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Will Gordon <wgordon@redhat.com>
storage_provider_max_tiers has no consumer anywhere in tasks/main.yaml -- the validation it once guarded was deleted in 77cdf99 without updating this test, leaving it asserting on nonexistent behavior since 2026-05-17. No layer above AAP (fulfillment-service API, osac-operator) enforces a tier-count ceiling either, so reintroducing an AAP-only check would be an arbitrary, unbacked limit rather than a real product constraint. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Will Gordon <wgordon@redhat.com>
qos_limits README description didn't mention that a tier with qos_limits.static_limits present but both bandwidth fields zero (or unset) also opts out of qos_policy defaulting -- same as omitting qos_limits entirely. Clarified to match the actual condition in storage_provider/tasks/main.yaml. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Will Gordon <wgordon@redhat.com>
…backend misconfigurations Wraps four fail-hard checks -- unsupported protocol in vast_storage/setup.yaml, and three backend-connection-resolution checks in read_credentials.yaml (ambiguous backend association, unresolved backend connection, incomplete backend connection) -- in block/rescue so each also computes a structured osac_storage_provider_failures list (reason, tiers, backend_ids, message) via set_fact and emits it via ansible.builtin.set_stats before re-raising the original human-readable message unchanged. Nothing reads these artifacts yet -- additive scaffolding for a future osac-operator consumer to parse AAP Job API artifacts and attribute a job failure to a specific StorageTier/StorageBackend. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Will Gordon <wgordon@redhat.com>
|
@wgordon17: This pull request references OSAC-1992 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wgordon17 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughStorage provisioning now consumes event-supplied tier definitions and backend connections, derives QoS policies, resolves VAST credentials by backend ID, validates provider inputs, redacts EDA event logging, detects snapshot CRDs, and expands unit and integration coverage. ChangesStorage provider event-driven provisioning
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant EDAEvent
participant StoragePlaybook
participant StorageProviderRole
participant DispatchedVASTRole
participant BackendConnections
EDAEvent->>StoragePlaybook: tier definitions and backend connections
StoragePlaybook->>StorageProviderRole: provider tiers and connections
StorageProviderRole->>DispatchedVASTRole: normalized tiers and backend mappings
DispatchedVASTRole->>BackendConnections: resolve endpoint and credentials
DispatchedVASTRole->>DispatchedVASTRole: validate protocols, quotas, and snapshots
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Add the missing closed-set protocol check to ensure_storage_class (the dominant cluster-storage action), factored into a shared _validate_tier_protocols.yaml so setup and ensure_storage_class can't drift out of sync again - Replace the STORAGE_SNAPSHOTS_ENABLED env var with a set_fact computed unconditionally from the existing live VolumeSnapshot CRD check, removing the now-dead toggle from argument_specs, 3 playbooks, the example ConfigMap, and 5 test fixtures - Make qos_policy fully system-derived: any caller-supplied value is discarded and ignored (never validated or preserved), consistent with how every other field outside this role's schema is already treated - Wrap the missing-backend_id check in the same block/rescue + set_stats structured-failure pattern as its three sibling backend-connection checks - Delete the dead _requested_tiers filtering branch in _dispatch_provider.yaml -- no caller ever wired it up, and playbook_osac_create_compute_instance.yml already solves single-tier filtering its own way - Remove OSAC_STORAGE_CONFIG_NAMESPACE's redundant, non-functional env var injection in favor of reading the standard Kubernetes service-account namespace file directly; the documented ConfigMap-based override can now actually take effect - Add message field assertions to 3 structured-failure tests that were only checking reason/tiers/backend_ids, plus new tests for the missing-backend_id artifact, protocol validation via ensure_storage_class, and the real reachable empty-string/ UNSPECIFIED protocol case - Wire the storage_provider dispatcher unit tests into CI via run_tests.sh (previously only run manually) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Will Gordon <wgordon@redhat.com>
Full-branch self-review (independent subagent, commit 51db654) found no CRITICAL/HIGH findings. Fixes three LOW-severity items: removes now-inert qos_policy example values from two integration fixtures (the role always derives qos_policy from the tier name and discards any caller-supplied value, so these were misleading), and disambiguates the duplicate "Validate tier protocols are supported by VAST provider" task name shared between the two include_tasks call sites and the shared _validate_tier_protocols.yaml file itself. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Will Gordon <wgordon@redhat.com>
Addresses the No-Sensitive-Data-In-Logs pre-merge check failure. storage_provider_block_encryption_passphrase is read from ansible_eda.event.payload.spec.blockEncryptionPassphrase in playbook_osac_create_tenant_cluster_storage.yml, playbook_osac_create_tenant_storage_backend.yml, and playbook_osac_create_compute_instance.yml -- a real secret nested inside .payload itself, unlike storage_tier_definitions/storage_backend_connections which arrive as siblings of .payload. All three playbooks' "Show EDA Event" pre-task unconditionally debugged the whole payload object, which prints that passphrase in cleartext to the job log. Replaced the blanket payload debug with an explicit metadata allowlist (kind, name, namespace, uid, plus the tenant annotation or templateID each playbook already relies on) in those three playbooks, and in the two delete-side sibling playbooks for consistency, since they share the same convention even though they don't currently read blockEncryptionPassphrase. Verified: ansible-lint clean, and confirmed live (with a real secret string injected via -e) that the new debug task prints only the allowlisted fields in all three affected create playbooks -- the injected secret does not appear anywhere in the log output, including in the ClusterOrder-payload and ComputeInstance-payload variants which have different metadata shapes. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Will Gordon <wgordon@redhat.com>
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
playbook_osac_create_compute_instance.yml (1)
48-63: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject malformed optional tier definitions before filtering.
A defined string or mapping reaches
selectattrbeforestorage_providercan enforce its required-list contract, producing a misleading “tier not found” path or skipped provisioning. Validate that a defined value is a non-string, non-mapping sequence before Line 49.Proposed fix
+ - name: Validate optional storage_tier_definitions type + ansible.builtin.fail: + msg: ansible_eda.event.storage_tier_definitions must be an array when provided. + when: >- + ansible_eda.event.storage_tier_definitions is defined and + (ansible_eda.event.storage_tier_definitions is not sequence or + ansible_eda.event.storage_tier_definitions is string or + ansible_eda.event.storage_tier_definitions is mapping) + - name: Filter storage tier definitions to the requested tier for JIT storage🤖 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 `@playbook_osac_create_compute_instance.yml` around lines 48 - 63, The JIT storage tier filtering block must reject malformed storage_tier_definitions before selectattr runs. Add validation before the block’s filtering task so any defined value is a non-string, non-mapping sequence, preserving the existing empty/default behavior and preventing invalid input from reaching _jit_storage_tiers filtering.
🤖 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 `@playbook_osac_create_compute_instance.yml`:
- Around line 15-25: Centralize the EDA payload-redaction policy by introducing
or reusing a shared redacted event logger, then replace the local “Show EDA
Event” allowlists in playbook_osac_create_compute_instance.yml (lines 15-25),
playbook_osac_create_tenant_cluster_storage.yml (lines 27-37),
playbook_osac_create_tenant_storage_backend.yml (lines 12-21),
playbook_osac_delete_tenant_cluster_storage.yml (lines 25-35), and
playbook_osac_delete_tenant_storage_backend.yml (lines 11-20). Ensure the shared
sanitizer preserves required event metadata while excluding sensitive fields
such as storage_provider_block_encryption_passphrase, and apply the same logging
convention repo-wide rather than maintaining storage-specific policies.
---
Outside diff comments:
In `@playbook_osac_create_compute_instance.yml`:
- Around line 48-63: The JIT storage tier filtering block must reject malformed
storage_tier_definitions before selectattr runs. Add validation before the
block’s filtering task so any defined value is a non-string, non-mapping
sequence, preserving the existing empty/default behavior and preventing invalid
input from reaching _jit_storage_tiers filtering.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: 6effbe6d-ed0b-4a99-baf4-86ca0c3cd26f
📒 Files selected for processing (9)
playbook_osac_create_compute_instance.ymlplaybook_osac_create_tenant_cluster_storage.ymlplaybook_osac_create_tenant_storage_backend.ymlplaybook_osac_delete_tenant_cluster_storage.ymlplaybook_osac_delete_tenant_storage_backend.ymlsamples/storage_tier_definitions_payload.jsontests/integration/integration_config.yml.templatetests/integration/targets/storage_provider_ensure_sc/tasks/main.ymltests/integration/targets/storage_provider_teardown/tasks/main.yml
Addresses the 2 remaining findings from CodeRabbit's round-4 review. - Centralize the EDA payload-redaction policy: rather than 5 separate copy-pasted metadata allowlists, added osac.service.common/tasks/show_eda_event_metadata.yaml as the single shared implementation. It always logs kind/name/namespace/uid and accepts an optional eda_event_extra_fields dict for the one extra non-sensitive field each playbook needs (tenant_annotation or template_id). All 5 storage playbooks now call this shared task via include_role instead of inlining their own debug block. Documented the convention in .claude/rules/playbook-patterns.md so future playbooks reading a sensitive payload.spec field use the shared task from the start instead of the bare "debug: var: payload" example. A full repo-wide migration of every other playbook's existing debug task is a separate, larger initiative than this PR's scope -- this centralizes the actual redaction logic in one place and gives new playbooks the correct pattern by default. - playbook_osac_create_compute_instance.yml treats storage_tier_definitions as optional (JIT-provisioning path only) and wasn't type-checked before selectattr, unlike the 4 other playbooks that require it and already validate. Added the same non-sequence/string/mapping rejection, gated on "is defined" since omitting the field entirely is valid here. Verified: ansible-lint clean repo-wide, --syntax-check on all 5 playbooks, and live -e runs (with injected fake secrets) confirming: the shared task renders the same redacted metadata as before with no secret leakage in any of the Tenant/ClusterOrder/ComputeInstance payload shapes; the new type-check rejects a malformed string value with a clear message; and omitting storage_tier_definitions entirely still skips cleanly with no regression to existing behavior. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Will Gordon <wgordon@redhat.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
playbook_osac_delete_tenant_cluster_storage.yml (1)
81-86: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPass backend connections into cluster teardown.
This call passes
storage_provider_tiersbut omitsstorage_provider_backend_connections, leaving the provider role without the event-supplied credentials needed to resolve each tier’sbackend_id. VAST cluster cleanup can therefore fail when it attempts credential resolution.Proposed fix
storage_provider_action: teardown_cluster_storage storage_provider_tiers: "{{ ansible_eda.event.storage_tier_definitions }}" + storage_provider_backend_connections: "{{ ansible_eda.event.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 `@playbook_osac_delete_tenant_cluster_storage.yml` around lines 81 - 86, Update the “Clean up cluster-side tenant storage resources” include_role task to pass ansible_eda.event.storage_provider_backend_connections through the storage_provider_backend_connections variable alongside storage_provider_tiers, so teardown_cluster_storage can resolve each tier’s backend_id using the event-supplied credentials.
🤖 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
`@collections/ansible_collections/osac/service/roles/common/tasks/show_eda_event_metadata.yaml`:
- Around line 15-25: Update the “Show EDA Event metadata” task to enforce an
explicit allowlist for eda_event_extra_fields, permitting only approved safe
fields and excluding arbitrary nested payload or credential data. Ensure the
fixed kind, name, namespace, and uid metadata is merged last so callers cannot
overwrite it; do not log the unfiltered dictionary.
---
Outside diff comments:
In `@playbook_osac_delete_tenant_cluster_storage.yml`:
- Around line 81-86: Update the “Clean up cluster-side tenant storage resources”
include_role task to pass ansible_eda.event.storage_provider_backend_connections
through the storage_provider_backend_connections variable alongside
storage_provider_tiers, so teardown_cluster_storage can resolve each tier’s
backend_id using the event-supplied credentials.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: 6259dd45-7773-4c5b-80d0-34ae98741bdc
📒 Files selected for processing (7)
.claude/rules/playbook-patterns.mdcollections/ansible_collections/osac/service/roles/common/tasks/show_eda_event_metadata.yamlplaybook_osac_create_compute_instance.ymlplaybook_osac_create_tenant_cluster_storage.ymlplaybook_osac_create_tenant_storage_backend.ymlplaybook_osac_delete_tenant_cluster_storage.ymlplaybook_osac_delete_tenant_storage_backend.yml
…elds
Addresses the round-5 CodeRabbit finding on the shared redaction task itself.
combine(eda_event_extra_fields | default({})) merged the caller-supplied
dict on top of the fixed metadata with no restriction on its keys: a
careless future caller could overwrite kind/name/namespace/uid, or pass
something like the whole payload.spec as an "extra field" and defeat the
entire point of centralizing redaction in this task.
Added an explicit allowlist (tenant_annotation, template_id -- the only
two keys any current caller uses) that fails loudly if
eda_event_extra_fields contains anything else, and reversed the combine
order so the fixed metadata is merged last and always wins regardless.
Adding a new extra field now requires deliberately updating the allowlist
in this one file, keeping it the single actual enforcement point rather
than a convention callers could accidentally bypass.
Verified: ansible-lint clean, confirmed the normal cases still render
correctly (no extra fields, and the tenant_annotation/template_id cases
used by the 5 real playbooks), and confirmed both attack scenarios from
the review are now rejected with a clear error: a disallowed key
("leaked_secret") and an attempt to override a fixed key ("name") both
fail at the new validation task before anything is logged.
Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Will Gordon <wgordon@redhat.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
zszabo-rh
left a comment
zszabo-rh
left a comment
There was a problem hiding this comment.
One minor concern noted inline, otherwise the changes LGTM! (👍🏻ᴗ _ᴗ)👍🏻
| STORAGE_TIERS env var contains invalid JSON. | ||
| Value: '{{ _storage_tiers_raw }}'. | ||
| Expected a JSON array of tier objects with name, protocol, and provider fields. | ||
| - name: Validate storage_tier_definitions is a non-empty array in the EDA event |
There was a problem hiding this comment.
I'm not sure about this is really necessary here.. missing tier definitions on creation is definitely a real misconfiguration but for teardown this may be too strict.
Not asking for a full change, just consider whether this should be warn + skip rather than fail when storage_tier_definitions is absent on the delete path.
| storage_provider_action: ensure_storage_class | ||
| storage_provider_tiers: "{{ _jit_storage_tiers }}" | ||
| storage_provider_backend_connections: "{{ ansible_eda.event.storage_backend_connections | default({}) }}" | ||
| storage_provider_provisioning_target: vmaas |
There was a problem hiding this comment.
nit:
Is this being used? Are there plans to use this in the future for caas, bmaas etc.?
| success_msg: "Role correctly rejected invalid input" | ||
|
|
||
| # ────────────────────────────────────────────────────────────── | ||
| # Test 9: Invalid DNS label tier name -- error path |
There was a problem hiding this comment.
nit: Test comments jump from 7 to 9 to 11 (missing 8, 10). Likely from removed scenarios during iteration. Perhaps we can skip numbering.
| # store. Tests that assert on the call log or pre-seed VMS resources cannot | ||
| # run in parallel without cross-contamination. Run all sequentially — each | ||
| # test takes ~7s so the total overhead is negligible. | ||
| STORAGE_TESTS=( |
There was a problem hiding this comment.
nit: The STORAGE_TESTS targets (storage_provider_setup, storage_provider_ensure_sc, etc.) are VAST-specific end-to-end: they hardcode provider: vast, assert vast-* resource names, and require the mock VMS server. When a second provider lands (e.g. PURE, LVMS), these names will be ambiguous.
Should we add a prefix or a vast/ sub-directory?
WDYT?
| block: | ||
| - name: "Include provider storage role" | ||
| ansible.builtin.include_role: | ||
| name: "osac.templates.{{ _current_provider }}_storage" |
There was a problem hiding this comment.
nit: Today this dispatch loop is only exercised with a single provider (vast) in tests. Once LVMS lands (cc @zszabo-rh ), can we add a test with tiers spanning two providers? WDYT?
| - name: Create VolumeSnapshotClass per tier | ||
| when: >- | ||
| (storage_provider_snapshots_enabled | default(true)) and | ||
| (_vast_vsc_crd_check.resources | default([]) | length > 0) | ||
| when: | ||
| - _vast_snapshot_crd_available | bool | ||
| - storage_provider_snapshots_enabled | default(true) | bool |
There was a problem hiding this comment.
question: storage_provider_snapshots_enabled defaults to true, meaning every ensure_storage_class call creates a VolumeSnapshotClass per tier alongside the StorageClass. Two questions:
-
Are there side effects to creating
VolumeSnapshotClassesby default?
e.g., does the VAST CSI driver need additional RBAC or a specific version to handle snapshot operations, and what happens if a user tries to create aVolumeSnapshoton a cluster where the driver doesn't support it? -
This parameter isn't declared in
storage_provider/meta/argument_specs.yaml, so consumers of the role won't know it exists unless they read the task file. Perhaps you can add it for completeness.
| - secretRef: | ||
| name: storage-operations-ig |
There was a problem hiding this comment.
question: After this PR the Secret has no credential fields left, but the secretRef is still required (no optional: true). The compute-instance IG already marks it optional. Should this match the compute-instance IG?
I am not seeing a reason to keep it required, for now. WDYT?
|
This PR has been migrated to the new osac mono-repo as osac-project/osac#99, following Elior Erez's guidance in #wg-osac-eng (2026-07-31) that open PR authors should port their work to This is a migration, not an abandonment — the full 22-commit history and review thread remain here for reference. Review findings addressed in the new PR: @akshaynadkarni @zszabo-rh — here is the status of each substantive item from round 6:
The 4 forward-looking nits (CaaS/BMaaS field usage, multi-provider test layout, multi-provider dispatch test, test-comment numbering) are carried into the new PR's description for continuity. |
4 participants
Summary
STORAGE_TIERSenv var andstorage-operations-igVAST admin credentials withansible_eda.event.storage_tier_definitions/storage_backend_connectionsextra_vars populated by osac-operator's new Tier API integration, absorbing OSAC-1957's outstanding AAP-side ACqos_policyderivation in thestorage_providerrole so every caller gets consistent naming without duplicating the logicset_statsfailure artifacts, additive scaffolding for a future osac-operator consumer to attribute job failures to a specific StorageTier/StorageBackendSummary by CodeRabbit
storage_tier_definitionsand per-backend_idstorage_backend_connections.quota_bytes; QoS policy is always derived from the tier name.