OSAC-23: Rename storage playbooks to match two-stage model - #338
openshift-merge-bot[bot] merged 7 commits into
Conversation
|
@zszabo-rh: This pull request references OSAC-1145 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 task 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. |
|
Skipping CI for Draft Pull Request. |
WalkthroughReplaces the monolithic ChangesTenant Storage Teardown Split and New Create/Delete Playbooks
Sequence Diagram(s)sequenceDiagram
participant AAP as AAP Job Template
participant PB_Backend as create_tenant_storage_backend
participant PB_Cluster as create_tenant_cluster_storage
participant SP as storage_provider
participant VAST as vast_storage role
participant VMS as VAST VMS API
participant K8s as Kubernetes API
rect rgba(100, 149, 237, 0.5)
note over AAP,K8s: Create Phase
AAP->>PB_Backend: create-tenant-storage-backend (Stage 1)
PB_Backend->>PB_Backend: extract tenant metadata
PB_Backend->>PB_Backend: parse STORAGE_TIERS
PB_Backend->>SP: storage_provider_action=setup
SP->>VAST: dispatch to VAST provider
VAST->>VMS: create VAST VMs + hub Secret
AAP->>PB_Cluster: create-tenant-cluster-storage (Stage 2)
PB_Cluster->>SP: storage_provider_action=ensure_storage_class (with rescue/retry)
SP->>VAST: dispatch to VAST provider
VAST->>K8s: create StorageClasses + CSI Secret
end
rect rgba(205, 92, 92, 0.5)
note over AAP,K8s: Delete Phase
AAP->>PB_Cluster: delete-tenant-cluster-storage
PB_Cluster->>PB_Cluster: extract tenant metadata
PB_Cluster->>PB_Cluster: parse STORAGE_TIERS
PB_Cluster->>SP: storage_provider_action=teardown_cluster_storage
SP->>VAST: dispatch to VAST provider
VAST->>K8s: delete StorageClasses, VolumeSnapshotClasses, CSI Secret
AAP->>PB_Backend: delete-tenant-storage-backend
PB_Backend->>SP: storage_provider_action=teardown_backend
SP->>VAST: dispatch to VAST provider
VAST->>VMS: delete VAST VMs + hub Secret
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (10 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 |
d4cb6a4 to
1ba5e60
Compare
zszabo-rh
changed the title
|
@zszabo-rh: This pull request references OSAC-23 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 epic 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. |
1ba5e60 to
b6873c0
Compare
|
Hey @zszabo-rh , noting a naming mismatch between this PR and the PRD/design for when we finalize the coordinated merge. The backend names are consistent everywhere, but the Stage 2 (cluster-side) names diverge:
The dispatcher action also differs: Just leaving this here so we have a reference point to align on before merging. |
51f514b to
ab06298
Compare
|
@akshaynadkarni |
|
Nit: the comment on _dispatch_provider.yaml line 8 still lists three actions ( |
|
The old |
Fixed both! |
|
@coderabbitai resolve |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
✅ Action performedComments resolved and changes approved. |
Split the combined configure_tenant_storage playbook into four independent playbooks aligned with the two-stage storage model: - osac-create-tenant-storage-backend (Stage 1: backend setup on VAST) - osac-create-tenant-storage-class (Stage 2: StorageClasses + CSI on target) - osac-delete-tenant-storage-class (cluster-side resource removal) - osac-delete-tenant-storage-backend (backend teardown on VAST) Add cleanup action to storage_provider role dispatcher and vast_storage role for cluster-side resource removal without touching backend. Update config-as-code with four job template definitions. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Zoltan Szabo <zszabo@redhat.com>
Add teardown_backend action that removes VAST backend resources (tenant, views, quotas, manager, local provider) and hub Secret without touching cluster-side K8s resources (StorageClasses, VolumeSnapshotClasses, CSI Secret). The delete-tenant-storage-backend playbook now uses teardown_backend instead of teardown, ensuring that cluster-side resources are only removed by the delete-tenant-storage-class playbook (cleanup action). This separation is required for CaaS where a ClusterOrder deletion must clean up cluster-side resources without destroying the backend that other clusters depend on. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Zoltan Szabo <zszabo@redhat.com>
Rename Stage 2 playbooks from *-storage-class to *-cluster-storage and the dispatcher action from cleanup to teardown_cluster_storage, per the approved design spec (PR osac-project#58). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Zoltan Szabo <zszabo@redhat.com>
- Remove hcp_data_plane from teardown_cluster_storage (not implemented) - Update _dispatch_provider.yaml comment to list all 5 actions - Remove old monolithic teardown action — the operator uses teardown_backend + teardown_cluster_storage independently Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Zoltan Szabo <zszabo@redhat.com>
@zszabo-rh I still see teardown in the argument_specs and main.yaml validation list at HEAD (ab06298). Also, I'm not seeing any changes to |
ab06298 to
4c5ec15
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
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_tenant_cluster_storage.yml (1)
50-52: 🎯 Functional Correctness | 🔴 CriticalAdd
osac.service.commonkubeconfig bootstrap to storage_provider role before dispatching to provider tasks.The
vast_storagerole'skubernetes.core.k8s_infoandkubernetes.core.k8stasks (lines 43, 149, 216, 242, 271, 281 inensure_storage_class.yaml, and similar inteardown_cluster_storage.yamlandsetup.yaml) operate without akubeconfig:parameter. Without bootstrappingremote_cluster_kubeconfigfirst, these tasks will execute against the default kubeconfig context (hub cluster), not the target cluster where the StorageClasses should be created.Follow the established pattern: include
osac.service.commonwithtasks_from: get_remote_cluster_kubeconfiginstorage_provider/tasks/main.yamlbefore any dispatch to provider roles, similar to howocp_virt_vmandnetwork_policyroles implement it.🤖 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_tenant_cluster_storage.yml` around lines 50 - 52, The storage_provider role is being invoked without first bootstrapping the remote_cluster_kubeconfig, causing the kubernetes.core.k8s_info and kubernetes.core.k8s tasks in the vast_storage role to execute against the hub cluster's default kubeconfig context instead of the target cluster. In the storage_provider/tasks/main.yaml file, add an include_role task for osac.service.common with tasks_from: get_remote_cluster_kubeconfig before dispatching to any provider-specific roles, following the established pattern used in ocp_virt_vm and network_policy roles to ensure the target cluster context is properly set.Source: Coding guidelines
🤖 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/storage_provider/tasks/_dispatch_provider.yaml`:
- Line 7: Update the comment on the _dispatch_action line in
_dispatch_provider.yaml by removing the obsolete teardown action from the list
of available actions. The comment currently includes teardown as one of the
valid task file options, but since teardown has been replaced with
teardown_cluster_storage and teardown_backend, remove teardown from the
comma-separated list to keep the documentation consistent with the actual
implementation in argument_specs.yaml and main.yaml.
In
`@collections/ansible_collections/osac/service/roles/storage_provider/tasks/teardown_cluster_storage.yaml`:
- Around line 7-14: The task name "Dispatch cleanup to each provider" is too
generic and does not accurately reflect the specific action being performed
which is teardown_cluster_storage. Rename the task from "Dispatch cleanup to
each provider" to "Dispatch teardown_cluster_storage to each provider" to
improve clarity and consistency with the action name, and to distinguish it from
the similar teardown_backend action.
In
`@collections/ansible_collections/osac/templates/roles/vast_storage/tasks/teardown_cluster_storage.yaml`:
- Around line 82-89: The "Report cleanup summary" task is running
unconditionally after the block/rescue structure, causing it to report
"complete" with potentially zero counts even when the cleanup failed due to an
unreachable cluster (triggering the rescue block). To fix this, either move the
"Report cleanup summary" task inside the main block before the rescue clause so
it only runs on successful cleanup, or add a conditional statement (when clause)
to the task that checks if cleanup was successful before reporting the summary.
This will prevent misleading success messages when the cluster is unreachable
and the rescue block executes instead.
In `@playbook_osac_create_tenant_storage_backend.yml`:
- Around line 12-15: The debug task named "Show EDA Event" currently logs the
full ansible_eda.event.payload without redacting sensitive information such as
the passphrase field. Modify this debug task to redact or mask sensitive payload
keys before logging to prevent exposing secrets in AAP logs. Apply a redaction
filter or transformation to the payload variable that removes or masks fields
like passphrase while maintaining the existing traceability pattern and overall
log structure.
- Around line 33-38: Add validation immediately after parsing the STORAGE_TIERS
JSON in the "Parse tier list from env var" task to ensure the parsed result is a
JSON array rather than any other valid JSON type. After the from_json filter
assigns to _storage_tiers, add a condition or assertion that checks if
_storage_tiers is of type list, and if not, fail the playbook with a clear error
message indicating that STORAGE_TIERS must be a JSON array. This prevents
downstream roles from encountering unclear errors when the wrong JSON type is
passed.
In `@playbook_osac_delete_tenant_cluster_storage.yml`:
- Around line 41-45: The task "Skip cleanup when STORAGE_TIERS is not
configured" currently uses ansible.builtin.debug to only log a message when
_storage_tiers_raw has no length, allowing the playbook to continue and silently
skip cleanup. Replace the ansible.builtin.debug module with ansible.builtin.fail
to halt playbook execution when STORAGE_TIERS is not configured, ensuring the
job fails instead of reporting success while leaving tenant artifacts orphaned.
- Around line 26-31: The "Parse tier list from env var" task parses
STORAGE_TIERS JSON into the _storage_tiers variable but does not validate that
the parsed result is actually a list, allowing non-list JSON values to pass
through and cause failures later. After the from_json filter in the set_fact
task that sets _storage_tiers, add an assertion or conditional check to validate
that _storage_tiers is a list type. If the parsed value is not a list, fail the
task with a clear error message indicating that STORAGE_TIERS must be a JSON
array.
---
Outside diff comments:
In `@playbook_osac_create_tenant_cluster_storage.yml`:
- Around line 50-52: The storage_provider role is being invoked without first
bootstrapping the remote_cluster_kubeconfig, causing the
kubernetes.core.k8s_info and kubernetes.core.k8s tasks in the vast_storage role
to execute against the hub cluster's default kubeconfig context instead of the
target cluster. In the storage_provider/tasks/main.yaml file, add an
include_role task for osac.service.common with tasks_from:
get_remote_cluster_kubeconfig before dispatching to any provider-specific roles,
following the established pattern used in ocp_virt_vm and network_policy roles
to ensure the target cluster context is properly set.
🪄 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: Enterprise
Run ID: 98469350-59e5-4747-8f0e-73229cc2774b
📒 Files selected for processing (13)
collections/ansible_collections/osac/config_as_code/roles/aap/vars/controller.ymlcollections/ansible_collections/osac/service/roles/storage_provider/meta/argument_specs.yamlcollections/ansible_collections/osac/service/roles/storage_provider/tasks/_dispatch_provider.yamlcollections/ansible_collections/osac/service/roles/storage_provider/tasks/main.yamlcollections/ansible_collections/osac/service/roles/storage_provider/tasks/teardown.yamlcollections/ansible_collections/osac/service/roles/storage_provider/tasks/teardown_backend.yamlcollections/ansible_collections/osac/service/roles/storage_provider/tasks/teardown_cluster_storage.yamlcollections/ansible_collections/osac/templates/roles/vast_storage/tasks/teardown_backend.yamlcollections/ansible_collections/osac/templates/roles/vast_storage/tasks/teardown_cluster_storage.yamlplaybook_osac_create_tenant_cluster_storage.ymlplaybook_osac_create_tenant_storage_backend.ymlplaybook_osac_delete_tenant_cluster_storage.ymlplaybook_osac_delete_tenant_storage_backend.yml
💤 Files with no reviewable changes (1)
- collections/ansible_collections/osac/service/roles/storage_provider/tasks/teardown.yaml
|
Hey @zszabo-rh, two things remaining:
|
- Fix integration test: split teardown into teardown_cluster_storage + teardown_backend (old teardown action was removed) - Remove stale 'teardown' from _dispatch_provider.yaml action list - Rename 'cleanup' to 'teardown_cluster_storage' in dispatcher and vast_storage task names for consistency - Move summary task inside block so it only reports on success, not after rescue fires on unreachable cluster - Add STORAGE_TIERS JSON array type validation in create-backend and delete-cluster-storage playbooks Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Zoltan Szabo <zszabo@redhat.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
playbook_osac_delete_tenant_cluster_storage.yml (1)
7-8: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThis playbook has the same pre-validation dereference failure path as the create playbook.
Play vars dereference
ansible_eda.event.payload.metadata.*beforepre_tasksexecute, and thewhenguard omits intermediate checks (event,payload,metadata). A malformed event can crash before the intended fail message.Suggested fix
vars: - tenant_name: "{{ ansible_eda.event.payload.metadata.name }}" - tenant_namespace: "{{ ansible_eda.event.payload.metadata.namespace }}" _storage_tiers_raw: "{{ lookup('env', 'STORAGE_TIERS') }}" @@ - name: Validate tenant payload is present ansible.builtin.fail: msg: "tenant must be provided via ansible_eda.event.payload with metadata.name and metadata.namespace set" when: >- ansible_eda is not defined or + ansible_eda.event is not defined or + ansible_eda.event.payload is not defined or + ansible_eda.event.payload.metadata is not defined or ansible_eda.event.payload.metadata.name is not defined or ansible_eda.event.payload.metadata.name | length == 0 or ansible_eda.event.payload.metadata.namespace is not defined or ansible_eda.event.payload.metadata.namespace | length == 0 + + - name: Set tenant identifiers from validated payload + ansible.builtin.set_fact: + tenant_name: "{{ ansible_eda.event.payload.metadata.name }}" + tenant_namespace: "{{ ansible_eda.event.payload.metadata.namespace }}"Also applies to: 16-24
🤖 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 7 - 8, The playbook is dereferencing nested properties ansible_eda.event.payload.metadata.* in the vars section (tenant_name and tenant_namespace variables) before the pre_tasks validation executes. If a malformed event is passed without the intermediate properties (event, payload, metadata), the dereference will fail before reaching the intended validation logic. Fix this by adding comprehensive existence checks in the when condition to verify all intermediate properties exist before allowing the playbook to proceed, checking for ansible_eda.event, ansible_eda.event.payload, and ansible_eda.event.payload.metadata in the guard clause. Apply the same fix to the other variable dereferences mentioned in lines 16-24 that have the same vulnerability.playbook_osac_create_tenant_storage_backend.yml (1)
7-8: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPlay-level payload dereference can fail before your validation task runs.
tenant_name/tenant_namespaceare resolved at play-var evaluation time, so missingansible_eda.event.payload.metadatacan abort before Line 16’s guarded failure message. Thewhenclause also needs intermediate-key checks (event,payload,metadata) to avoid undefined-attribute errors.Suggested fix
vars: - tenant_name: "{{ ansible_eda.event.payload.metadata.name }}" - tenant_namespace: "{{ ansible_eda.event.payload.metadata.namespace }}" _storage_tiers_raw: "{{ lookup('env', 'STORAGE_TIERS') }}" @@ - name: Validate tenant payload is present ansible.builtin.fail: msg: "tenant must be provided via ansible_eda.event.payload with metadata.name and metadata.namespace set" when: >- ansible_eda is not defined or + ansible_eda.event is not defined or + ansible_eda.event.payload is not defined or + ansible_eda.event.payload.metadata is not defined or ansible_eda.event.payload.metadata.name is not defined or ansible_eda.event.payload.metadata.name | length == 0 or ansible_eda.event.payload.metadata.namespace is not defined or ansible_eda.event.payload.metadata.namespace | length == 0 + + - name: Set tenant identifiers from validated payload + ansible.builtin.set_fact: + tenant_name: "{{ ansible_eda.event.payload.metadata.name }}" + tenant_namespace: "{{ ansible_eda.event.payload.metadata.namespace }}"Also applies to: 16-24
🤖 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_tenant_storage_backend.yml` around lines 7 - 8, The play-level variables tenant_name and tenant_namespace are being dereferenced at play evaluation time without guarding against missing intermediate keys in ansible_eda.event.payload.metadata, which causes the playbook to fail before the validation task at line 16. Add a when clause that checks for the existence of each intermediate key (event, payload, and metadata) using the defined test before dereferencing tenant_name and tenant_namespace. Apply the same defensive checks to the when conditions at lines 16-24 to verify ansible_eda.event.payload.metadata exists before attempting to access any nested attributes within it.
🤖 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.
Outside diff comments:
In `@playbook_osac_create_tenant_storage_backend.yml`:
- Around line 7-8: The play-level variables tenant_name and tenant_namespace are
being dereferenced at play evaluation time without guarding against missing
intermediate keys in ansible_eda.event.payload.metadata, which causes the
playbook to fail before the validation task at line 16. Add a when clause that
checks for the existence of each intermediate key (event, payload, and metadata)
using the defined test before dereferencing tenant_name and tenant_namespace.
Apply the same defensive checks to the when conditions at lines 16-24 to verify
ansible_eda.event.payload.metadata exists before attempting to access any nested
attributes within it.
In `@playbook_osac_delete_tenant_cluster_storage.yml`:
- Around line 7-8: The playbook is dereferencing nested properties
ansible_eda.event.payload.metadata.* in the vars section (tenant_name and
tenant_namespace variables) before the pre_tasks validation executes. If a
malformed event is passed without the intermediate properties (event, payload,
metadata), the dereference will fail before reaching the intended validation
logic. Fix this by adding comprehensive existence checks in the when condition
to verify all intermediate properties exist before allowing the playbook to
proceed, checking for ansible_eda.event, ansible_eda.event.payload, and
ansible_eda.event.payload.metadata in the guard clause. Apply the same fix to
the other variable dereferences mentioned in lines 16-24 that have the same
vulnerability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 11b43bcb-7fd2-4d80-9549-118a3574c626
📒 Files selected for processing (5)
collections/ansible_collections/osac/service/roles/storage_provider/tasks/teardown_cluster_storage.yamlcollections/ansible_collections/osac/templates/roles/vast_storage/tasks/teardown_cluster_storage.yamlplaybook_osac_create_tenant_storage_backend.ymlplaybook_osac_delete_tenant_cluster_storage.ymltests/integration/targets/storage_provider_teardown/tasks/main.yml
|
/retest |
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
|
/retest |
akshaynadkarni
left a comment
akshaynadkarni
left a comment
There was a problem hiding this comment.
Changes LGTM
Thanks for addressing the comments.
I am trying to run the changes on my beaker, but I am running into issues due to a db migration issue. In the meantime, I have kicked off the E2E suite on the PR. Hoping it goes through 🤞 while I re-attempt to run the suite on my machine.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akshaynadkarni, zszabo-rh 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 |
3 participants
Summary
Splits the monolithic storage playbooks into four lifecycle actions aligned with the OSAC-23 design (merged) two-stage storage model:
Stage 1 — Backend:
osac-create-tenant-storage-backend(wasosac-create-org): Provisions VAST backend resources + hub Secretosac-delete-tenant-storage-backend(wasosac-delete-org): Removes backend resources + hub SecretStage 2 — Cluster Storage:
osac-create-tenant-cluster-storage(new): Installs StorageClasses and CSI on target clusterosac-delete-tenant-cluster-storage(new): Removes cluster-side K8s resources onlyKey changes:
storage_providerdispatcher: 4 actions (setup,ensure_storage_class,teardown_cluster_storage,teardown_backend)vast_storagerole: newteardown_backend.yamlandteardown_cluster_storage.yamltask filesargument_specs.yamlupdated with new action choices and descriptionsDependencies:
Test plan
Assisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit