OSAC-3234: CaaS local storage — LVMS on guest cluster workers - #199
Conversation
|
@zszabo-rh: This pull request references OSAC-3234 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. |
|
Warning Review limit reached
Next review available in: 50 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughThe LVMS role now supports CaaS guest-cluster installation through OLM and remote StorageClass management. Agent VM setup can attach an optional data disk. The operator ClusterRole can read secrets and ChangesLVMS storage provisioning
Agent VM data disk
Operator RBAC
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Ansible
participant GuestCluster
participant LVMSOperator
Ansible->>GuestCluster: Create LVMS namespace, OperatorGroup, and Subscription
GuestCluster->>LVMSOperator: Install LVMS through OLM
Ansible->>GuestCluster: Create LVMCluster with device settings
LVMSOperator->>GuestCluster: Generate StorageClass
Ansible->>GuestCluster: Discover or create tenant StorageClass
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
osac-aap/collections/ansible_collections/osac/templates/roles/lvms_storage/tasks/ensure_storage_class.yaml (1)
55-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the CatalogSource configurable.
redhat-operatorsandopenshift-marketplaceare hardcoded. A disconnected guest cluster uses a mirrored CatalogSource with a different name. Move both values intodefaults/main.yamlnext tolvms_storage_operator_channel.♻️ Proposed change
name: lvms-operator - source: redhat-operators - sourceNamespace: openshift-marketplace + source: "{{ lvms_storage_operator_catalog_source }}" + sourceNamespace: "{{ lvms_storage_operator_catalog_namespace }}"Add to
defaults/main.yaml:lvms_storage_operator_catalog_source: "redhat-operators" lvms_storage_operator_catalog_namespace: "openshift-marketplace"🤖 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/collections/ansible_collections/osac/templates/roles/lvms_storage/tasks/ensure_storage_class.yaml` around lines 55 - 56, Make the CatalogSource configurable by defining lvms_storage_operator_catalog_source and lvms_storage_operator_catalog_namespace in defaults/main.yaml with the current values, then update the CatalogSource resource in the lvms_storage role to reference these variables instead of hardcoded strings.
🤖 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/collections/ansible_collections/osac/templates/roles/lvms_storage/tasks/ensure_storage_class.yaml`:
- Around line 2-12: Update the LVMS provider metadata in osac.yaml to describe
both hub and CaaS guest-cluster support, and add CaaS to
capabilities.provisioning_targets alongside vmaas. Keep the metadata consistent
with ensure_storage_class.yaml’s _remote_kubeconfig path and existing provider
structure.
- Around line 85-92: The LVMCluster specification under thinPoolConfig lacks
selectors tying storage to the required agent data disk. Update the LVMCluster
resource to include deviceSelector.paths for the expected data-device path, and
ensure provisioning fails when that disk is absent if the role requires it; use
the existing lvms_storage configuration symbols and setup-caas-agents disk path
conventions.
In `@osac-installer/scripts/setup-caas-agents.sh`:
- Around line 189-210: Update the generated VM creation script around
_virt_install_args to use an if block for AGENT_VM_DATA_DISK_SIZE, expanding and
quoting AGENT_VM_STORAGE_DIR, AGENT_VM_NAME, and AGENT_VM_DATA_DISK_SIZE when
removing, creating, and appending the optional data disk. Ensure the final
virt-install invocation expands the _virt_install_args array rather than passing
the literal ${_virt_install_args[@]} text.
- Line 43: Replace the character-only validation for AGENT_VM_DATA_DISK_SIZE
with validation that matches the installer’s qemu-img size grammar: a positive
numeric value with an optional k, M, G, T, or b suffix, rejecting zero,
nonnumeric values, and leading hyphens. Confirm the accepted syntax against the
qemu-img version used by the installer before applying the check.
---
Nitpick comments:
In
`@osac-aap/collections/ansible_collections/osac/templates/roles/lvms_storage/tasks/ensure_storage_class.yaml`:
- Around line 55-56: Make the CatalogSource configurable by defining
lvms_storage_operator_catalog_source and lvms_storage_operator_catalog_namespace
in defaults/main.yaml with the current values, then update the CatalogSource
resource in the lvms_storage role to reference these variables instead of
hardcoded strings.
🪄 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: Pro Plus
Run ID: 2ee4c358-5d6d-4f4c-b67a-bfe72614d6a7
📒 Files selected for processing (4)
osac-aap/collections/ansible_collections/osac/templates/roles/lvms_storage/defaults/main.yamlosac-aap/collections/ansible_collections/osac/templates/roles/lvms_storage/tasks/ensure_storage_class.yamlosac-installer/scripts/setup-caas-agents.shosac-operator/charts/operator/templates/clusterrole.yaml
9fd9bb7 to
f217c6b
Compare
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/collections/ansible_collections/osac/templates/README.md`:
- Line 194: Update the generic my_provider documentation example and the
associated provider-capability text to remove the blanket caas claim, or
explicitly state that providers must list only supported targets. Keep caas
documented only where the implementation supports it through lvms_storage and
_remote_kubeconfig.
🪄 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: Pro Plus
Run ID: 0e841474-a9cb-4458-9f07-5cb3e11a8619
📒 Files selected for processing (3)
osac-aap/collections/ansible_collections/osac/templates/README.mdosac-aap/collections/ansible_collections/osac/templates/roles/lvms_storage/tasks/teardown_cluster_storage.yamlosac-operator/charts/operator/templates/clusterrole.yaml
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Zoltan Szabo <zszabo@redhat.com>
- README: remove caas from generic provider example — providers declare only targets they implement; update text to reflect that caas is lvms_storage-specific, not a universal provider target - ensure_storage_class: use lvms_storage_operator_catalog_namespace for PackageManifest lookup instead of hardcoded openshift-marketplace so disconnected clusters with a mirrored catalog query the correct source Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Zoltan Szabo <zszabo@redhat.com>
|
🤖 Review · Commit: |
|
🤖 Finished Review · ✅ Success · Started 2:08 PM UTC · Completed 2:26 PM UTC Commit: |
Superseded by updated review
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the review comment for full details.
Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:
osac-aap/collections/ansible_collections/osac/templates/roles/lvms_storage/tasks/teardown_cluster_storage.yaml(file-level): Line 13 · [medium] logic-error
teardown_cluster_storage.yaml does not pass kubeconfig/validate_certs to its k8s_info and k8s calls, relying on the dispatcher's module_defaults. The updated ensure_storage_class.yaml adds explicit parameters as defense-in-depth. This creates an inconsistency between the two task files for CaaS path operation.
Suggested fix: Add explicit kubeconfig and validate_certs params to match ensure_storage_class.yaml.
osac-aap/collections/ansible_collections/osac/templates/roles/lvms_storage/tasks/ensure_storage_class.yaml:150: [low] edge-case
Wait for LVMS StorageClass uses until/retries without failed_when. The CSV and LVMCluster wait tasks include failed_when for actionable error messages; this wait would benefit from the same pattern for consistency.
osac-aap/collections/ansible_collections/osac/templates/roles/lvms_storage/tasks/ensure_storage_class.yaml:18: [low] pattern-violation
validate_certs uses not(_remote_kubeconfig_insecure) (always explicit boolean) rather than the dispatcher's omit-based pattern. Functionally identical but diverges from established convention.
osac-installer/scripts/setup-caas-agents.sh:193: [low] naming-convention
_virt_install_args uses leading-underscore naming (Ansible convention) within a shell heredoc where all other variables use SCREAMING_SNAKE_CASE.
osac-installer/scripts/setup-caas-agents.sh:42: [low] naming-convention
AGENT_VM_DATA_DISK_SIZE validation uses a purpose-built regex while AGENT_VM_DISK_SIZE uses validate_safe. Different validation styles for similar variables.
osac-aap/collections/ansible_collections/osac/templates/roles/lvms_storage/defaults/main.yaml:28: [low] edge-case
lvms_storage_data_device defaults to empty (LVMS auto-discovers all eligible block devices). Documented behavior, but operators should set this explicitly in CaaS deployments to avoid accidental disk consumption.
osac-aap/collections/ansible_collections/osac/templates/README.md:197: [low] stale-reference
_provisioning_target variable referenced in README step 3 does not exist in the dispatcher code. Pre-existing issue surfaced by this PR's documentation updates.
osac-operator/charts/operator/templates/clusterrole.yaml:51: [low] comment-style
hostedcontrolplanes rule has an inline comment; no other rule in the file uses comments. The comment itself is useful but creates a style inconsistency.
osac-aap/collections/ansible_collections/osac/templates/roles/lvms_storage/meta/osac.yaml:14: [low] naming-convention
caas provisioning target uses a higher-level abstraction than the existing hcp_* taxonomy. Appears intentional for the LVMS use case.
fullsend-ai-review
Bot
added
the
requires-manual-review
|
/retest |
|
Re-triggered failed runs:
|
|
[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 |
Auto-dismissed: only Prow labels gate merging
72aefaa
4 participants
Summary
Enable developers without VAST to provision CaaS clusters and create PVCs
using LVMS — no external storage backend required.
This is a temporary approach using LVMS directly on guest cluster workers.
A CSI-based approach is planned for a later phase (OSAC-3702).
Design: LVMS directly on guest cluster workers
The dispatcher already passes
_remote_kubeconfig(fromadmin_kubeconfigin the storage event for ClusterOrder resources). The
ensure_storage_classrole is extended to use it when present, keeping the hub path unchanged.
Changes
osac-aap —
lvms_storage/tasks/ensure_storage_class.yamlNew CaaS block (gated on
_remote_kubeconfig is defined):PackageManifest.status.defaultChannel(OLM sets this per cluster version).Explicit
lvms_storage_operator_channeloverride still works for pinning.overprovisionRatio(required field in LVMS 4.16+)and optional
deviceSelector.pathsfor explicit disk bindingReadystate — fails immediately ifFailed(e.g. no eligible block device found on the worker node)topolvm.ioprovisionerHub path is unchanged.
osac-aap —
lvms_storage/defaults/main.yamlNew variables:
lvms_storage_operator_channel— defaults to""(auto-detect fromPackageManifest); set explicitly to pin a channel (e.g.
stable-4.18)lvms_storage_operator_catalog_source/namespace— configurable fordisconnected clusters
lvms_storage_data_device— bind LVMCluster to a specific device path;empty (default) = auto-discover
osac-installer —
scripts/setup-caas-agents.shAGENT_VM_DATA_DISK_SIZE(opt-in): when set, creates a second qcow2 image andpasses it to
virt-install. Required for LVMS in thesetup-caas-agents.shpath (bare metal nodes have physical data disks;
cluster-toolalready addsa second disk via its own workflow).
osac-operator —
charts/operator/templates/clusterrole.yamlAdds
hostedcontrolplanesto the existing hypershift RBAC rule.The storage controller's
getClusterKubeconfig()reads the HostedControlPlaneresource to locate the admin kubeconfig secret for a CaaS guest cluster
(pre-existing code, not added by this PR). The kubebuilder marker at
storage_controller.go:111already declared this permission, soconfig/rbac/role.yamlwas correct — but the Helm chart was out of sync.This PR exercises the CaaS kubeconfig path for the first time and surfaces
the gap.
Test Plan
AGENT_VM_DATA_DISK_SIZE=50GReadystate on guest clustertopolvm.ioStorageClass available on guest cluster/dev/vdc) — verify job failsimmediately on first retry (
attempts: 1) withstate: Failedand a clearlstat /dev/vdc: no such file or directoryreason; no 30-retry silent timeoutCloses OSAC-3234
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation