Skip to content
This repository was archived by the owner on Sep 9, 2026. It is now read-only.

OSAC-1327: adds hcp_data_plane provisioning target for CaaS StorageClasses - #377

Merged
openshift-merge-bot[bot] merged 26 commits into
osac-project:mainfrom
wgordon17:feat/OSAC-1327-hcp-data-plane-storage
Jul 11, 2026
Merged

openshift-merge-bot[bot] merged 26 commits into
osac-project:mainfrom
wgordon17:feat/OSAC-1327-hcp-data-plane-storage

Conversation

@wgordon17

@wgordon17 wgordon17 commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Removes vmaas-only guards from all vast_storage task files, enabling hcp_data_plane (and other provisioning targets) to pass through
  • Adds cluster-aware volume naming (volumeNameFormat, ephemeralVolumeNameFormat, volumeGroup) to all StorageClasses unconditionally, using the cluster name discovered from the target cluster's infrastructure API
  • Configures truncateVolumeName: null on VastCSIDriver CRs and parameterizes all 4 storage playbooks to accept STORAGE_PROVISIONING_TARGET env var and optional admin_kubeconfig from the EDA event

Summary by CodeRabbit

  • New Features
    • Storage class/CSI naming now incorporates the tenant cluster name for both NFS and block configurations (including volume name format and volume group).
    • Provisioning and teardown behavior can be driven by STORAGE_PROVISIONING_TARGET with a default when unset.
  • Bug Fixes
    • Removed early aborts for unsupported provisioning contexts so validation/provisioning and cleanup flows proceed normally.
    • Improved remote kubeconfig and tenant credential handling; tenant selection now uses the tenant annotation with correct namespace mapping.
  • Tests
    • Updated integration tests to include the expected cluster name in assertions.

@openshift-ci

openshift-ci Bot commented Jun 25, 2026

Copy link
Copy Markdown

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci-robot

openshift-ci-robot commented Jun 25, 2026

Copy link
Copy Markdown

@wgordon17: This pull request references OSAC-1327 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.

Details

In response to this:

Summary

  • Removes vmaas-only guards from all vast_storage task files, enabling hcp_data_plane (and other provisioning targets) to pass through
  • Adds cluster-aware volume naming (volumeNameFormat, ephemeralVolumeNameFormat, volumeGroup) to all StorageClasses unconditionally, using the cluster name discovered from the target cluster's infrastructure API
  • Configures truncateVolumeName: null on VastCSIDriver CRs and parameterizes all 4 storage playbooks to accept STORAGE_PROVISIONING_TARGET env var and optional admin_kubeconfig from the EDA event

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.

@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@wgordon17, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 44 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 471a8f1d-01de-4d52-bfbc-91afe79e8a68

📥 Commits

Reviewing files that changed from the base of the PR and between a1afcde and 8a6e661.

📒 Files selected for processing (2)
  • playbook_osac_create_tenant_cluster_storage.yml
  • playbook_osac_delete_tenant_cluster_storage.yml

Walkthrough

Removes vmaas-only provisioning guards, derives tenant and provisioning inputs, forwards remote kubeconfig settings, discovers cluster names for CSI naming, and updates VAST CSI resources and tests.

Changes

Multi-target storage with cluster name propagation

Layer / File(s) Summary
Remove provisioning target validation
collections/.../storage_provider/meta/argument_specs.yaml, collections/.../storage_provider/tasks/main.yaml, collections/.../storage_provider/tests/test.yml
Removes provisioning-target declaration and validation, along with the obsolete invalid-target test.
Derive workflow inputs
playbook_osac_create_tenant_*.yml, playbook_osac_delete_tenant_*.yml
Derives tenant scope and provisioning targets, writes optional admin kubeconfig files, and removes hardcoded target values where appropriate.
Forward kubeconfig through dispatch
collections/.../storage_provider/tasks/_dispatch_provider.yaml
Applies kubeconfig and conditional certificate-validation defaults to Kubernetes modules during provider dispatch.
Update VAST provisioning and CSI resources
collections/.../vast_storage/tasks/*, tests/integration/targets/storage_provider_*/tasks/main.yml
Removes vmaas-only guards, discovers and sanitizes cluster names, creates tenant namespaces, adds cluster-specific CSI parameters, updates the CSI driver spec, and extends integration assertions.

Implementation artifact exclusion

Layer / File(s) Summary
Ignore implementation artifacts
.gitignore
Adds .artifacts/ to the ignored paths.

Estimated code review effort: 4 (Complex) | ~50 minutes

Sequence Diagram(s)

sequenceDiagram
  participant EDA as ansible_eda.event
  participant Playbook as storage playbooks
  participant TempFile as osac.service.to_temp_file
  participant InfraAPI as OpenShift Infrastructure
  participant Dispatcher as _dispatch_provider.yaml
  participant Role as osac.service.storage_provider

  Playbook->>EDA: read admin_kubeconfig and tenant annotation
  Playbook->>TempFile: write admin kubeconfig when present
  TempFile-->>Playbook: return _remote_kubeconfig
  Playbook->>InfraAPI: query target cluster infrastructure
  InfraAPI-->>Playbook: return cluster name
  Playbook->>Role: invoke storage action with derived inputs
  Role->>Dispatcher: dispatch provider action
  Dispatcher->>Role: include provider role with Kubernetes module defaults
Loading

Possibly related PRs

Suggested reviewers: rgolangh, ygalblum, trewest

🚥 Pre-merge checks | ✅ 11
✅ Passed checks (11 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title matches the main change: adding hcp_data_plane support for CaaS StorageClasses.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
No-Hardcoded-Secrets ✅ Passed Cumulative diff adds no new API keys/tokens/password literals or long base64 strings; only cluster-name, kubeconfig, and allowed admin/admin test defaults changed.
No-Weak-Crypto ✅ Passed No MD5/SHA1/DES/RC4/3DES/Blowfish/ECB or custom secret comparisons appear in the touched files; only AES-xts-plain64/SHA-256 block-encryption settings are present.
No-Injection-Vectors ✅ Passed Reviewed all changed files; none contain SQL concat, shell=True, eval/exec, pickle.loads, yaml.load, os.system, or dangerouslySetInnerHTML.
Container-Privileges ✅ Passed No changed YAML adds privileged/host* flags, SYS_ADMIN, or allowPrivilegeEscalation; touched manifests are role/playbook tasks only.
No-Sensitive-Data-In-Logs ✅ Passed No new secret-bearing logging was added; the diff only adds non-sensitive error text/task names, and existing payload debug output was unchanged.
Ai-Attribution ✅ Passed AI-assisted commits use proper Generated-By/Assisted-by trailers for Claude Code; no Co-Authored-By AI attribution found.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 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/templates/roles/vast_storage/tasks/ensure_csi_operator.yaml`:
- Line 129: The VastCSIDriver reconciliation currently only happens in the
install-only path guarded by _vast_any_driver_missing, so clusters that already
have CSI drivers never get truncateVolumeName updated. Move the VastCSIDriver
patch/update logic out of that conditional in ensure_csi_operator.yaml, keeping
only the CRD existence check before it, so the truncateVolumeName field is
reconciled for both fresh installs and existing deployments.

In
`@collections/ansible_collections/osac/templates/roles/vast_storage/tasks/ensure_storage_class.yaml`:
- Around line 233-237: The StorageClass creation flow in ensure_storage_class is
short-circuiting on existing names, so upgraded tenants can keep stale
parameters and miss volumeNameFormat, ephemeralVolumeNameFormat, and
volumeGroup. Update the existing-StorageClass path to detect parameter drift in
ensure_storage_class and the related VAST parameter merge blocks, then either
fail with clear remediation guidance or route through an explicit
migration/recreate path so these naming settings are applied consistently.
- Around line 15-24: Validate _cluster_name more strictly in the
ensure_storage_class assert task before it is used later by the CSI name/group
builders. In addition to checking it is defined and non-empty, add a character
restriction so values from the CLUSTER_NAME fallback cannot contain separator
characters such as : or / that are consumed by the naming logic in the storage
class templates. Keep the validation alongside the existing
ansible.builtin.assert for _cluster_name so the later CSI naming paths remain
safe.

In `@playbook_osac_create_tenant_cluster_storage.yml`:
- Around line 57-66: The cluster discovery block is allowing the query to run
without an explicit remote kubeconfig, which can fall back to the controller’s
default context. Update the workflow around the k8s_info step in the
cluster-name discovery logic to require a remote kubeconfig before querying
Infrastructure, or source remote_cluster_kubeconfig via osac.service.common
first, and apply the same guard to the later remote-cluster resource creation
block as well.

In `@playbook_osac_delete_tenant_cluster_storage.yml`:
- Around line 56-59: The fallback in the playbook task that sets
_remote_kubeconfig from OSAC_REMOTE_CLUSTER_KUBECONFIG should not silently allow
an empty value, because later cluster teardown can proceed with the wrong or
default kubeconfig. Update the logic around the _remote_kubeconfig set_fact task
to fail immediately when no remote kubeconfig is available, and ensure the play
includes osac.service.common so remote_cluster_kubeconfig is populated before
any remote-cluster cleanup or resource operations.
🪄 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: 2fed3422-72e8-4ba4-a542-75b67f67ee27

📥 Commits

Reviewing files that changed from the base of the PR and between e58e0c3 and 06e48c2.

📒 Files selected for processing (12)
  • .gitignore
  • collections/ansible_collections/osac/service/roles/storage_provider/tasks/_dispatch_provider.yaml
  • collections/ansible_collections/osac/templates/roles/vast_storage/tasks/ensure_csi_operator.yaml
  • collections/ansible_collections/osac/templates/roles/vast_storage/tasks/ensure_storage_class.yaml
  • collections/ansible_collections/osac/templates/roles/vast_storage/tasks/read_tenant_credentials.yaml
  • collections/ansible_collections/osac/templates/roles/vast_storage/tasks/setup.yaml
  • collections/ansible_collections/osac/templates/roles/vast_storage/tasks/teardown_backend.yaml
  • collections/ansible_collections/osac/templates/roles/vast_storage/tasks/teardown_cluster_storage.yaml
  • playbook_osac_create_tenant_cluster_storage.yml
  • playbook_osac_create_tenant_storage_backend.yml
  • playbook_osac_delete_tenant_cluster_storage.yml
  • playbook_osac_delete_tenant_storage_backend.yml
💤 Files with no reviewable changes (3)
  • collections/ansible_collections/osac/templates/roles/vast_storage/tasks/teardown_backend.yaml
  • collections/ansible_collections/osac/templates/roles/vast_storage/tasks/setup.yaml
  • collections/ansible_collections/osac/templates/roles/vast_storage/tasks/teardown_cluster_storage.yaml

Comment thread playbook_osac_create_tenant_cluster_storage.yml Outdated
Comment thread playbook_osac_delete_tenant_cluster_storage.yml Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_tenant_cluster_storage.yml (1)

47-52: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Remove the temp kubeconfig after use. mkstemp already creates an owner-only file, but _remote_kubeconfig is never cleaned up, so the admin kubeconfig can linger on the execution environment after the job finishes. Add an always cleanup step once the file is no longer needed.

🤖 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 47 - 52, The
temporary admin kubeconfig created by the “Write admin_kubeconfig to temporary
file when provided” task is never removed, so add an always cleanup step in this
playbook to delete _remote_kubeconfig after it is no longer needed. Update the
same play flow that sets _remote_kubeconfig via ansible.builtin.set_fact to
include a cleanup task in an always block, guarding it so it only runs when
_remote_kubeconfig is defined.
🤖 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_tenant_cluster_storage.yml`:
- Line 7: The tenant annotation lookup in the tenant cluster storage playbook is
too direct and can fail before the role’s validation runs. Update the logic
around the tenant_name derivation from
ansible_eda.event.payload.metadata.annotations['osac.openshift.io/tenant'] to
first verify the annotation exists (alongside the existing metadata.name and
metadata.namespace checks) and only then set tenant_name; use the same task or a
preceding guard near the tenant_name assignment to fail clearly if it is
missing.

---

Outside diff comments:
In `@playbook_osac_create_tenant_cluster_storage.yml`:
- Around line 47-52: The temporary admin kubeconfig created by the “Write
admin_kubeconfig to temporary file when provided” task is never removed, so add
an always cleanup step in this playbook to delete _remote_kubeconfig after it is
no longer needed. Update the same play flow that sets _remote_kubeconfig via
ansible.builtin.set_fact to include a cleanup task in an always block, guarding
it so it only runs when _remote_kubeconfig is defined.
🪄 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: bf8fdf1a-5aa8-4fe0-89da-8d4930661cf7

📥 Commits

Reviewing files that changed from the base of the PR and between 703452c and d4776ab.

📒 Files selected for processing (1)
  • playbook_osac_create_tenant_cluster_storage.yml

Comment thread playbook_osac_create_tenant_cluster_storage.yml

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

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)

224-227: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Avoid disabling TLS verification for the infrastructure lookup. playbook_osac_create_tenant_cluster_storage.yml:224-227 hardcodes validate_certs: false, so this call skips certificate checks even when the kubeconfig already carries CA data. Keep verification on by default and gate any opt-out behind an explicit flag for broken kubeconfigs.

🤖 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 224 - 227, The
infrastructure lookup in the k8s_info task should not hardcode TLS verification
off. Update the Query OpenShift infrastructure object for cluster name step to
keep validate_certs enabled by default, and only disable it behind an explicit
opt-out flag for broken kubeconfigs. Use the existing k8s_info task and its
kubeconfig handling to preserve certificate checks when CA data is already
available.
♻️ Duplicate comments (1)
playbook_osac_create_tenant_cluster_storage.yml (1)

224-227: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Require an explicit remote kubeconfig before querying the target cluster.

default(omit) lets k8s_info fall back to the controller’s default kube context, so cluster-name discovery can run against the wrong cluster. This was already flagged previously and still applies here. As per coding guidelines, “Always include osac.service.common to get remote_cluster_kubeconfig before creating K8s resources on remote clusters.”

🤖 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 224 - 227,
Require an explicit remote kubeconfig for the cluster-name lookup in the
`kubernetes.core.k8s_info` task so it cannot fall back to the controller
context. Update the `Query OpenShift infrastructure object for cluster name`
step to use `osac.service.common` to obtain `remote_cluster_kubeconfig` and pass
that value directly instead of `default(omit)`. Keep the lookup tied to the
remote target cluster by referencing the same
`_remote_kubeconfig`/`remote_cluster_kubeconfig` flow used elsewhere in the
playbook.

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`:
- Around line 34-37: The k8s_info task in the storage_provider dispatch flow is
disabling TLS verification whenever _remote_kubeconfig is present, which should
not be the default. Update the kubernetes.core.k8s_info invocation so
validate_certs is not forced to false by _remote_kubeconfig; instead, let the
caller explicitly opt out when needed, and keep the existing kubeconfig handling
in _dispatch_provider.yaml aligned with that behavior.

In `@playbook_osac_create_tenant_cluster_storage.yml`:
- Around line 54-220: Remove the entire temporary debug section from the
playbook before merging, including the tasks marked with "[DEBUG]" and the
surrounding temporary block comments. The block in the main task sequence leaks
sensitive kubeconfig-auth material through the kubeconfig structure dump and
uses ad hoc command invocations that trigger lint failures; delete these debug
tasks entirely rather than keeping partial redaction. Locate the removal around
the debug helpers that reference _remote_kubeconfig, _debug_kc_structure,
_debug_server_url, _debug_curl_noauth, _debug_curl_withauth, and
_debug_k8s_python.

---

Outside diff comments:
In `@playbook_osac_create_tenant_cluster_storage.yml`:
- Around line 224-227: The infrastructure lookup in the k8s_info task should not
hardcode TLS verification off. Update the Query OpenShift infrastructure object
for cluster name step to keep validate_certs enabled by default, and only
disable it behind an explicit opt-out flag for broken kubeconfigs. Use the
existing k8s_info task and its kubeconfig handling to preserve certificate
checks when CA data is already available.

---

Duplicate comments:
In `@playbook_osac_create_tenant_cluster_storage.yml`:
- Around line 224-227: Require an explicit remote kubeconfig for the
cluster-name lookup in the `kubernetes.core.k8s_info` task so it cannot fall
back to the controller context. Update the `Query OpenShift infrastructure
object for cluster name` step to use `osac.service.common` to obtain
`remote_cluster_kubeconfig` and pass that value directly instead of
`default(omit)`. Keep the lookup tied to the remote target cluster by
referencing the same `_remote_kubeconfig`/`remote_cluster_kubeconfig` flow used
elsewhere in the playbook.
🪄 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: f5b0fd0a-b799-432f-a5b2-6c27ec07d33f

📥 Commits

Reviewing files that changed from the base of the PR and between 7887222 and 2c48a37.

📒 Files selected for processing (2)
  • collections/ansible_collections/osac/service/roles/storage_provider/tasks/_dispatch_provider.yaml
  • playbook_osac_create_tenant_cluster_storage.yml

Comment thread playbook_osac_create_tenant_cluster_storage.yml Outdated
wgordon17 added 15 commits July 10, 2026 12:52
…h chain

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>
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>
Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Will Gordon <wgordon@redhat.com>
The CaaS storage playbook receives a ClusterOrder payload, not a Tenant.
The tenant name must be read from the osac.openshift.io/tenant annotation
rather than metadata.name (which is the ClusterOrder name).

Generated-By: Claude Code (Anthropic)
Signed-off-by: Will Gordon <wgordon@redhat.com>
The HCP kubeconfig uses a self-signed CA. The infrastructure
discovery task needs validate_certs: false to connect.

Generated-By: Claude Code (Anthropic)
Signed-off-by: Will Gordon <wgordon@redhat.com>
HCP kubeconfigs use self-signed CAs. When _remote_kubeconfig is set,
validate_certs is disabled for kubernetes.core.k8s and k8s_info calls
routed to the CaaS cluster. Hub cluster calls are unaffected.

Generated-By: Claude Code (Anthropic)
Signed-off-by: Will Gordon <wgordon@redhat.com>
Generated-By: Claude Code (Anthropic)
Signed-off-by: Will Gordon <wgordon@redhat.com>
Root cause: HOSTED_CLUSTER_BASE_DOMAIN=apps.osac-local.gordoncode.dev
puts the CaaS API hostname under *.apps wildcard, causing CoreDNS
to resolve to the SNO node IP instead of the MetalLB VIP.

Generated-By: Claude Code (Anthropic)
Signed-off-by: Will Gordon <wgordon@redhat.com>
tenant_namespace was derived from ansible_eda.event.payload.metadata.namespace,
which is correct when this template is triggered from the Tenant-based hub
storage path (osac-tenants) but resolves to the ClusterOrder's own hub
namespace (osac-devel) when triggered from the CaaS guest-cluster path,
where it has no relation to the target cluster. Branches on payload.kind and
uses a fixed openshift-osac-storage system namespace for the ClusterOrder
case, and ensures that namespace exists on the target cluster before the
CSI Secret is created there.

Generated-By: Claude Code (Anthropic)
Signed-off-by: Will Gordon <wgordon@redhat.com>
Moves cluster-name discovery from playbook_osac_create_tenant_cluster_storage.yml
into the vast_storage role itself, running only when provisioning actually
happens (after the short-circuit check) and only when the caller hasn't
already supplied _cluster_name. The dispatcher's existing kubeconfig routing
means the role queries the correct cluster (hub or remote) automatically.

This also fixes a latent gap: playbook_osac_create_compute_instance.yml (the
VMaaS path) calls ensure_storage_class but never discovered a cluster name,
so it would have failed once the _cluster_name assertion became unconditional.
Updates the two integration test fixtures that never supplied _cluster_name.

Generated-By: Claude Code (Anthropic)
Signed-off-by: Will Gordon <wgordon@redhat.com>
…te kubeconfigs

Generated-By: Claude Code (Anthropic)
Signed-off-by: Will Gordon <wgordon@redhat.com>
Generated-By: Claude Code (Anthropic)
Signed-off-by: Will Gordon <wgordon@redhat.com>
…ng_target var

Generated-By: Claude Code (Anthropic)
Signed-off-by: Will Gordon <wgordon@redhat.com>
@wgordon17
wgordon17 force-pushed the feat/OSAC-1327-hcp-data-plane-storage branch from 549758c to a1afcde Compare July 10, 2026 16:52

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
collections/ansible_collections/osac/service/roles/storage_provider/tests/test.yml (1)

2-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Test count/numbering drifted after removing the provisioning-target test.

Header still says "All 12 scenarios run in a single invocation" but one scenario was removed, leaving 11. Also, the renumbered duplicate-tier-names test jumps straight from "Test 7" to "Test 9" (Test 8 is now missing), which will confuse anyone debugging by test number.

✏️ Proposed fix
-# All 12 scenarios run in a single invocation -- no flags needed.
+# All 11 scenarios run in a single invocation -- no flags needed.

And renumber "Test 9" → "Test 8", "Test 10" → "Test 9", etc. through the rest of the file for consistency.

Also applies to: 241-244, 279-281

🤖 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
`@collections/ansible_collections/osac/service/roles/storage_provider/tests/test.yml`
around lines 2 - 4, Update the test.yml header to state that 11 scenarios run in
one invocation, then renumber the duplicate-tier-names test and every subsequent
test sequentially, changing the current “Test 9” onward labels through the end
of the file so there is no missing Test 8.
collections/ansible_collections/osac/templates/roles/vast_storage/tasks/read_tenant_credentials.yaml (1)

30-38: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Add kubeconfig: ~ to the hub Secret tasks in setup.yaml and teardown_backend.yaml. They inherit _remote_kubeconfig from the storage-provider dispatch block, so the Secret reads/writes can hit the guest cluster instead of the hub Secret.

🤖 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
`@collections/ansible_collections/osac/templates/roles/vast_storage/tasks/read_tenant_credentials.yaml`
around lines 30 - 38, Ensure the hub Secret tasks in setup.yaml and
teardown_backend.yaml explicitly set kubeconfig to ~, overriding inherited
_remote_kubeconfig. Update the relevant kubernetes.core.k8s_info and Secret
read/write tasks so they always target the hub cluster.
🤖 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_tenant_cluster_storage.yml`:
- Around line 65-74: Add an early fail-fast validation for ClusterOrder events
requiring a non-empty ansible_eda.event.admin_kubeconfig before any
target-cluster Kubernetes operations, including Infrastructure discovery and
Namespace/StorageClass creation. Place the guard near the existing
_remote_kubeconfig setup, preserve the current temporary-file and insecure
settings when provided, and ensure missing credentials cannot fall back to the
controller’s default kube context.

In `@playbook_osac_delete_tenant_cluster_storage.yml`:
- Around line 60-69: Ensure the ClusterOrder path fails fast when
admin_kubeconfig is absent or empty, rather than proceeding with an unset
_remote_kubeconfig and potentially using the default context. Add the guard
alongside the _remote_kubeconfig setup and apply the same protection to the
referenced later task block, while preserving the existing temporary-file and
insecure-connection handling.

---

Outside diff comments:
In
`@collections/ansible_collections/osac/service/roles/storage_provider/tests/test.yml`:
- Around line 2-4: Update the test.yml header to state that 11 scenarios run in
one invocation, then renumber the duplicate-tier-names test and every subsequent
test sequentially, changing the current “Test 9” onward labels through the end
of the file so there is no missing Test 8.

In
`@collections/ansible_collections/osac/templates/roles/vast_storage/tasks/read_tenant_credentials.yaml`:
- Around line 30-38: Ensure the hub Secret tasks in setup.yaml and
teardown_backend.yaml explicitly set kubeconfig to ~, overriding inherited
_remote_kubeconfig. Update the relevant kubernetes.core.k8s_info and Secret
read/write tasks so they always target the hub cluster.
🪄 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: 8038aa18-f134-4994-89e4-3cd630327871

📥 Commits

Reviewing files that changed from the base of the PR and between d34e9ad and a1afcde.

📒 Files selected for processing (17)
  • .gitignore
  • collections/ansible_collections/osac/service/roles/storage_provider/meta/argument_specs.yaml
  • collections/ansible_collections/osac/service/roles/storage_provider/tasks/_dispatch_provider.yaml
  • collections/ansible_collections/osac/service/roles/storage_provider/tasks/main.yaml
  • collections/ansible_collections/osac/service/roles/storage_provider/tests/test.yml
  • collections/ansible_collections/osac/templates/roles/vast_storage/tasks/ensure_csi_operator.yaml
  • collections/ansible_collections/osac/templates/roles/vast_storage/tasks/ensure_storage_class.yaml
  • collections/ansible_collections/osac/templates/roles/vast_storage/tasks/read_tenant_credentials.yaml
  • collections/ansible_collections/osac/templates/roles/vast_storage/tasks/setup.yaml
  • collections/ansible_collections/osac/templates/roles/vast_storage/tasks/teardown_backend.yaml
  • collections/ansible_collections/osac/templates/roles/vast_storage/tasks/teardown_cluster_storage.yaml
  • playbook_osac_create_tenant_cluster_storage.yml
  • playbook_osac_create_tenant_storage_backend.yml
  • playbook_osac_delete_tenant_cluster_storage.yml
  • playbook_osac_delete_tenant_storage_backend.yml
  • tests/integration/targets/storage_provider_ensure_sc/tasks/main.yml
  • tests/integration/targets/storage_provider_onboarding/tasks/main.yml
💤 Files with no reviewable changes (8)
  • collections/ansible_collections/osac/templates/roles/vast_storage/tasks/teardown_cluster_storage.yaml
  • collections/ansible_collections/osac/templates/roles/vast_storage/tasks/ensure_csi_operator.yaml
  • collections/ansible_collections/osac/templates/roles/vast_storage/tasks/setup.yaml
  • collections/ansible_collections/osac/templates/roles/vast_storage/tasks/teardown_backend.yaml
  • playbook_osac_delete_tenant_storage_backend.yml
  • collections/ansible_collections/osac/service/roles/storage_provider/tasks/main.yaml
  • collections/ansible_collections/osac/service/roles/storage_provider/meta/argument_specs.yaml
  • playbook_osac_create_tenant_storage_backend.yml

Comment thread playbook_osac_create_tenant_cluster_storage.yml
Comment thread playbook_osac_delete_tenant_cluster_storage.yml
…kubeconfig

Generated-By: Claude Code (Anthropic)
Signed-off-by: Will Gordon <wgordon@redhat.com>

@akshaynadkarni akshaynadkarni left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@openshift-ci

openshift-ci Bot commented Jul 11, 2026

Copy link
Copy Markdown

[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

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot
openshift-merge-bot Bot merged commit 85c079e into osac-project:main Jul 11, 2026
7 checks passed
@wgordon17
wgordon17 deleted the feat/OSAC-1327-hcp-data-plane-storage branch July 17, 2026 17:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants