Design: Rework Tenant Storage Onboarding (OSAC-1001) - #58
Conversation
|
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: Enterprise Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
WalkthroughReplaces the OSAC-23 design document with a reworked architecture that extracts all storage provisioning and teardown from the Tenant controller into a new dedicated Storage Controller. It defines a two-stage workflow, new Tenant CRD conditions and status structs, a reconciliation state machine, AAP playbook split into four lifecycle actions, controller wiring details, security/RBAC constraints, and upgrade/version-skew strategies. ChangesOSAC-23 Storage Controller Enhancement Proposal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
zszabo-rh
left a comment
zszabo-rh
left a comment
There was a problem hiding this comment.
Overall the design looks great! Just left a minor comment regarding ClusterOrder watch, however that doesn't really have much effect on what we have to implement in this scope.
![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
🤖 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 `@enhancements/tenant-storage-onboarding-OSAC-23/README.md`:
- Around line 157-176: The design documentation does not clarify the
multi-backend handling strategy for the StorageBackendStatus structure. Update
the README to explicitly document the following: whether a tenant can provision
multiple storage backends or is limited to a single backend in Stage 1, how
backend names are determined (hardcoded values like "vast-1" vs. dynamic
discovery from a StorageBackend CRD), and whether the status list will always
contain a single item or can grow to multiple items. Align this clarification
with the onboarding workflow diagram (which currently shows only a single
backend path) and explain whether the list structure is future-proofing for the
StorageBackend API (OSAC-1111) or reflects the current implementation
requirements.
- Around line 366-379: Explicitly document the hard dependency on PR `#338` (AAP
playbook split) to prevent deployment issues. Add PR `#338` to the "Depends on"
section of the design document to make clear that the new playbook templates
(OSAC_STORAGE_BACKEND_AAP_PROVISION_TEMPLATE and
OSAC_STORAGE_CLUSTER_AAP_PROVISION_TEMPLATE) referenced in the code at lines
243-251 do not exist until after PR `#338` is merged. Update the pull request
description to state that both this PR and PR `#338` must be merged in the same
release cycle, and clarify that the operator cannot function if deployed before
the AAP playbook split is complete to avoid the "template not found" error
condition described at line 564.
- Around line 404-416: The design document states it inherits the existing OSAC
security model without changes, but this claim is incomplete because it does not
clarify how the Fulfillment Service discovers and accesses per-tenant storage
credentials (vast-tenant-config-*) when provisioning workloads. Update the
Credential isolation section (around line 404-416) or the security model
inheritance statement (line 402) to explicitly address the Fulfillment Service
credential handling: either confirm that the fulfillment-service already has an
established contract to read per-tenant hub Secrets and describe the mechanism,
or document this as a prerequisite/dependency on another design effort. Ensure
the clarification acknowledges that credential access for workload provisioning
is a functional requirement, not optional, despite being mentioned as a non-goal
elsewhere.
- Line 536: The Unmanaged management state feature (referenced at lines 536 and
584-585) lacks critical implementation clarity required before coding begins.
Enhance the documentation to specify three key details: first, describe the
concrete implementation mechanism for how the storage controller will check the
osac.openshift.io/management-state annotation and enforce the skip behavior
(e.g., early return in reconciliation); second, define the deletion safety
contract—what prevents operators from accidentally deleting backends while a
tenant is Unmanaged, and whether the finalizer should validate this state;
third, clarify the re-enablement atomicity by explicitly stating whether
unsetting the Unmanaged annotation automatically enables reconciliation on the
next controller run, or if operator restart is required, and whether the
annotation is auto-cleared or must be manually removed. Update both the feature
description section (line 536) and the usage/debugging context section (lines
584-585) to include these specification details so the behavior is fully defined
before implementation begins.
🪄 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: 9b06177b-3f13-4cad-bba7-a93bfef0bfef
📒 Files selected for processing (1)
enhancements/tenant-storage-onboarding-OSAC-23/README.md
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)
enhancements/tenant-storage-onboarding-OSAC-23/README.md (2)
456-465:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope Secret RBAC to the storage-config namespace.
This grants
get/list/watchon all Secrets cluster-wide, even though the design says the controller only verifies hub Secret existence inOSAC_STORAGE_CONFIG_NAMESPACE. That is broader than needed and weakens the isolation story. Prefer a namespace-scoped Role/RoleBinding (or equivalent narrowed cache/client scope) for hub Secret reads.Also applies to: 475-475
🤖 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 `@enhancements/tenant-storage-onboarding-OSAC-23/README.md` around lines 456 - 465, The current RBAC annotation for secrets grants cluster-wide get/list/watch permissions on all Secrets, which is broader than needed since the controller only reads secrets from the OSAC_STORAGE_CONFIG_NAMESPACE. Replace the cluster-wide secret RBAC rule with a namespace-scoped permission that restricts secret access to only the storage-config namespace. This can be done by using a Role and RoleBinding (instead of ClusterRole) for secrets, or by narrowing the cache/client scope in the controller code to watch secrets only in that specific namespace. This change applies to all occurrences of the cluster-wide secret RBAC permission in the file.
135-137:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnify the retry contract.
Failure handling says Stage 1/2 do not auto-retry and wait for an external trigger, but the upgrade section says the controller “retries automatically once AAP is updated.” Those cannot both be true; this affects rollout recovery and could leave new tenants without storage. Pick one behavior and document it consistently.
Also applies to: 579-580
🤖 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 `@enhancements/tenant-storage-onboarding-OSAC-23/README.md` around lines 135 - 137, The README contains contradictory statements about the controller's retry behavior: lines 135-137 state that Stage 1 and Stage 2 failures do not auto-retry and wait for external triggers, while lines 579-580 in the upgrade section claim the controller retries automatically once AAP is updated. Determine the actual retry behavior from the codebase (particularly osac-operator/internal/controller/tenant_controller.go), then update both the failure handling section (lines 135-137) and the upgrade section (lines 579-580) to consistently document the same retry contract. Ensure the documentation clearly explains whether the controller auto-retries after failure or waits for an external trigger in all scenarios.
🤖 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 `@enhancements/tenant-storage-onboarding-OSAC-23/README.md`:
- Around line 456-465: The current RBAC annotation for secrets grants
cluster-wide get/list/watch permissions on all Secrets, which is broader than
needed since the controller only reads secrets from the
OSAC_STORAGE_CONFIG_NAMESPACE. Replace the cluster-wide secret RBAC rule with a
namespace-scoped permission that restricts secret access to only the
storage-config namespace. This can be done by using a Role and RoleBinding
(instead of ClusterRole) for secrets, or by narrowing the cache/client scope in
the controller code to watch secrets only in that specific namespace. This
change applies to all occurrences of the cluster-wide secret RBAC permission in
the file.
- Around line 135-137: The README contains contradictory statements about the
controller's retry behavior: lines 135-137 state that Stage 1 and Stage 2
failures do not auto-retry and wait for external triggers, while lines 579-580
in the upgrade section claim the controller retries automatically once AAP is
updated. Determine the actual retry behavior from the codebase (particularly
osac-operator/internal/controller/tenant_controller.go), then update both the
failure handling section (lines 135-137) and the upgrade section (lines 579-580)
to consistently document the same retry contract. Ensure the documentation
clearly explains whether the controller auto-retries after failure or waits for
an external trigger in all scenarios.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 4f2ae402-9528-45d4-b9f4-3901c47452db
📒 Files selected for processing (1)
enhancements/tenant-storage-onboarding-OSAC-23/README.md
|
@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 (5)
enhancements/tenant-storage-onboarding-OSAC-23/README.md (5)
434-436: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winCredential-safe logging constraints are documented—implementation must enforce at code review.
Lines 434–436 specify that the storage controller must not log Secret contents, AAP job parameters with credentials, or VAST API responses. Log messages should reference Secret names and job IDs only. This is the right constraint, but implementation enforcement is critical and must be verified during code review.
Risk: Medium. It is easy to accidentally log a Secret's
Datafield or an AAP response containing credentials. The design should note that the code review checklist includes a mandatory spot-check of log statements.Recommendation: Add a note to the "Credential-safe logging" section or the Test Plan stating:
- Code review must include a spot-check of all log statements for Secret data, API responses, and parameters.
- Consider using a linter rule to flag
secret.Data,job.Parameters, or similar patterns in logging calls (if available).🤖 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 `@enhancements/tenant-storage-onboarding-OSAC-23/README.md` around lines 434 - 436, The credential-safe logging constraint in the README.md is documented but lacks implementation enforcement guidance. Add a note to the "Credential-safe logging" section (or Test Plan) that explicitly states code review must include a mandatory spot-check of all log statements to ensure Secret data, API response contents, and job parameters containing credentials are never logged. Include guidance that reviewers should verify only Secret names and job IDs are referenced in log messages, and optionally suggest that a linter rule be considered to flag patterns like secret.Data or job.Parameters in logging calls to prevent accidental credential exposure.
557-562: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUnit test plan covers core scenarios—e2e test coverage for multi-controller coordination is sparse.
The unit test plan (lines 557–562) covers:
- Stage 1 lifecycle (not ready, trigger, skip existing, failure, retry).
- Stage 2 lifecycle (wait for Stage 1, trigger, SCs discovered, failure, retry).
- Tier resolution (tenant-specific, Default fallback, duplicates, missing tier).
- Teardown (ordered, partial failure, finalizer removal).
- Management state (Unmanaged skip).
- Tenant controller (Phase=Ready with NamespaceReady only).
Gaps:
- Multi-controller status conflicts: Line 571 lists "status conflict resolution when both controllers update simultaneously," but no detail on how this is tested (concurrent updates? conflict injection?).
- Finalizer coexistence: Line 570 lists "finalizer coexistence (tenant + storage finalizers)," but does not explain how the test verifies both finalizers are honored.
- e2e test coverage for operator-first deployment: Lines 589–590 justify operator-first deployment, but the e2e tests (lines 575–579) do not explicitly cover the failure scenario (operator deployed before AAP template exists, recovery).
Risk: Low-to-Medium. The core scenarios are covered, but multi-controller coordination and deployment ordering should be explicitly tested.
Recommendation: Enhance test plan:
- Unit test for concurrent status updates: Mock two controllers writing different status fields; verify conflict retry succeeds.
- Unit test for finalizer coexistence: Verify that deleting a Tenant with both finalizers triggers both controllers' delete handlers in sequence.
- e2e test for operator-first deployment: Deploy operator, verify storage controller waits for AAP template; deploy AAP playbook split, verify storage controller recovers and provisions 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 `@enhancements/tenant-storage-onboarding-OSAC-23/README.md` around lines 557 - 562, The test plan in the README lacks specific details on how three critical test scenarios will be verified. Update the documentation to explicitly explain: (1) for multi-controller status conflicts (mentioned at line 571), describe the test approach such as mocking concurrent controller writes to different status fields and verifying conflict retry succeeds; (2) for finalizer coexistence (mentioned at line 570), clarify how the test verifies that both the tenant and storage finalizers are honored by confirming both controllers' delete handlers execute in sequence; (3) for e2e operator-first deployment coverage (lines 589-590), add explicit test cases that cover the failure scenario where the operator deploys before the AAP template exists and verify recovery behavior when the AAP playbook split is deployed. Ensure each test scenario includes concrete verification steps rather than just listing the scenario name.
513-513: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winTwo-controller status updates require robust conflict handling—test coverage is unclear.
Lines 513, 453, and 571 mention status conflict resolution when both the Tenant controller and storage controller update Tenant status. The design states "Kubernetes optimistic concurrency handles conflicts" (line 513) and "Conflict retries are automatic" (line 513).
However:
- Conflict resolution mechanism is not specified beyond "DeepCopy-compare-update pattern" (line 513). The unit tests (line 571) list "status conflict resolution when both controllers update simultaneously," but the test plan does not explain how conflicts are resolved (retry loop? exponential backoff? max retry count?).
- Tail latency risk: If the Tenant controller frequently updates
Phasewhile the storage controller updates conditions, conflicts can cause serialization. The design does not quantify expected conflict frequency or latency bounds.Risk: Low-to-Medium. Kubernetes optimistic concurrency is reliable, but the test plan should explicitly validate conflict retry behavior under contention. The design should specify retry bounds (max retries, backoff strategy).
Recommendation: Enhance the unit test description (line 571) to specify the retry loop behavior, or add a note to line 513 documenting the backoff strategy.
Also applies to: 453-453, 571-571
🤖 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 `@enhancements/tenant-storage-onboarding-OSAC-23/README.md` at line 513, The documentation about conflict handling in status updates lacks specificity about the retry mechanism. At line 513 in the README.md, replace the vague statement about "Kubernetes optimistic concurrency handles conflicts" with explicit documentation of the backoff strategy, maximum retry count, and how conflicts are resolved (specify the retry loop behavior). At line 453, ensure consistent language that reinforces this conflict resolution approach. At line 571, enhance the unit test description to explicitly state the validation of retry loop behavior under contention, including specifics about maximum retries and backoff strategy rather than just listing "status conflict resolution when both controllers update simultaneously."
564-564:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffFinalizer orphan risk identified—mitigation is incomplete.
The risks table (line 511) does not list a HIGH severity risk: If the storage controller is uninstalled or disabled while Tenants have the
osac.openshift.io/storagefinalizer, those Tenants become stuck inTerminatingand cannot be deleted.The design acknowledges this indirectly:
- Line 149: "Tenant deletion is blocked if the storage finalizer is present and the controller is unavailable."
- Lines 605–607: Support procedure to manually remove the finalizer with
kubectl edit.However, the risks table (line 511) has three rows and does not include the finalizer orphan scenario. This is a critical operational risk that should be explicitly listed and mitigated.
Severity: HIGH. Orphaned finalizers block cluster cleanup and require manual intervention.
Mitigation: The design should add:
- A risk row: "Storage controller uninstalled/disabled → Finalizer orphan → Tenants stuck in Terminating."
- Mitigation: Implement a graceful disable mechanism (e.g., a validating webhook or pre-stop hook that removes the finalizer before shutdown when an operator-provided flag is set).
- Alternative: Document a one-time cleanup procedure (e.g.,
kubectl patch tenant -p '{"metadata":{"finalizers":[]}}' --type=merge) in the support section.Also applies to: 149-149
🤖 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 `@enhancements/tenant-storage-onboarding-OSAC-23/README.md` at line 564, Add a HIGH severity risk row to the risks table (around line 511) documenting the finalizer orphan scenario: when the storage controller is uninstalled or disabled while Tenants have the osac.openshift.io/storage finalizer, those Tenants become stuck in Terminating state and cannot be deleted. Include this risk entry alongside the existing three rows in the table. Then add corresponding mitigation details either by implementing a graceful disable mechanism (such as a validating webhook or pre-stop hook that removes the finalizer before shutdown when a flag is set), or by documenting a one-time cleanup procedure (e.g., using kubectl patch tenant with metadata.finalizers) in the support section. Verify that the existing acknowledgments of this scenario at line 149 (mentioning deletion is blocked if controller is unavailable) and lines 605-607 (support procedure for manual finalizer removal) are consistent with the new risk documentation.
32-32: 🧹 Nitpick | 🔵 TrivialPhase decoupling is documented but lacks explicit downstream consumer guidance.
The design already documents the Phase=Ready change in the "Operational impact" section (line 149): "If the storage controller is down, Tenant CRs continue to reach Phase=Ready." The Tenant controller's Phase logic is explicit at line 338: "Phase=Ready is set when NamespaceReady=True. Storage conditions are not consulted."
However, the document does not provide explicit guidance for downstream consumers. ComputeInstance is verified as backward-compatible (line 38: "reads status.storageClasses"), but there is no dedicated section clarifying that other consumers—if they exist—must check
StorageBackendReadyandClusterStorageReadyinstead of assuming Phase=Ready implies storage readiness.Suggested improvement: Add a brief section addressing downstream consumer expectations, explicitly stating that
Phase=Readyindicates namespace and UDN readiness only, not storage readiness. This is especially relevant given the design's foundation for future multi-cluster support (CaaS, listed in Non-Goals line 46) where consumers may have different expectations per delivery model.The code search found no actual downstream consumers making this assumption, so this is preventive documentation rather than a correction.
🤖 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 `@enhancements/tenant-storage-onboarding-OSAC-23/README.md` at line 32, Add a new section to the README that addresses downstream consumer expectations and migration guidance. This section should explicitly state that Phase=Ready now indicates only namespace and UDN readiness (not storage readiness), and that consumers must check the StorageBackendReady and ClusterStorageReady conditions to determine storage readiness. Reference the Operational impact section (line 149) and Phase logic (line 338) as context, and note that ComputeInstance's backward-compatibility (line 38) serves as an example. Include a note that this distinction becomes more critical with future multi-cluster support (CaaS, mentioned in Non-Goals at line 46) where consumer expectations may vary by deployment model.
🤖 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 `@enhancements/tenant-storage-onboarding-OSAC-23/README.md`:
- Around line 293-295: The reconciliation loop description in lines 293-295
derives stages implicitly from hub Secret state and conditions rather than using
an explicit phase field, which may be unclear to maintainers during code review.
Add clarifying text to make this implicit sequencing explicit: in the Stage 1
(Backend) description, modify the phrase about "no matching Secret exists" to
explicitly state "(indicating Stage 1 has not run yet)" to clarify that Secret
absence is the stage indicator, and in the Stage 2 (Cluster Storage)
description, add a clarification that "If `StorageBackendReady=True` (Stage 1
complete)" to make the sequencing dependency obvious. This helps code reviewers
verify the correct conditional checks are in place.
---
Outside diff comments:
In `@enhancements/tenant-storage-onboarding-OSAC-23/README.md`:
- Around line 434-436: The credential-safe logging constraint in the README.md
is documented but lacks implementation enforcement guidance. Add a note to the
"Credential-safe logging" section (or Test Plan) that explicitly states code
review must include a mandatory spot-check of all log statements to ensure
Secret data, API response contents, and job parameters containing credentials
are never logged. Include guidance that reviewers should verify only Secret
names and job IDs are referenced in log messages, and optionally suggest that a
linter rule be considered to flag patterns like secret.Data or job.Parameters in
logging calls to prevent accidental credential exposure.
- Around line 557-562: The test plan in the README lacks specific details on how
three critical test scenarios will be verified. Update the documentation to
explicitly explain: (1) for multi-controller status conflicts (mentioned at line
571), describe the test approach such as mocking concurrent controller writes to
different status fields and verifying conflict retry succeeds; (2) for finalizer
coexistence (mentioned at line 570), clarify how the test verifies that both the
tenant and storage finalizers are honored by confirming both controllers' delete
handlers execute in sequence; (3) for e2e operator-first deployment coverage
(lines 589-590), add explicit test cases that cover the failure scenario where
the operator deploys before the AAP template exists and verify recovery behavior
when the AAP playbook split is deployed. Ensure each test scenario includes
concrete verification steps rather than just listing the scenario name.
- Line 513: The documentation about conflict handling in status updates lacks
specificity about the retry mechanism. At line 513 in the README.md, replace the
vague statement about "Kubernetes optimistic concurrency handles conflicts" with
explicit documentation of the backoff strategy, maximum retry count, and how
conflicts are resolved (specify the retry loop behavior). At line 453, ensure
consistent language that reinforces this conflict resolution approach. At line
571, enhance the unit test description to explicitly state the validation of
retry loop behavior under contention, including specifics about maximum retries
and backoff strategy rather than just listing "status conflict resolution when
both controllers update simultaneously."
- Line 564: Add a HIGH severity risk row to the risks table (around line 511)
documenting the finalizer orphan scenario: when the storage controller is
uninstalled or disabled while Tenants have the osac.openshift.io/storage
finalizer, those Tenants become stuck in Terminating state and cannot be
deleted. Include this risk entry alongside the existing three rows in the table.
Then add corresponding mitigation details either by implementing a graceful
disable mechanism (such as a validating webhook or pre-stop hook that removes
the finalizer before shutdown when a flag is set), or by documenting a one-time
cleanup procedure (e.g., using kubectl patch tenant with metadata.finalizers) in
the support section. Verify that the existing acknowledgments of this scenario
at line 149 (mentioning deletion is blocked if controller is unavailable) and
lines 605-607 (support procedure for manual finalizer removal) are consistent
with the new risk documentation.
- Line 32: Add a new section to the README that addresses downstream consumer
expectations and migration guidance. This section should explicitly state that
Phase=Ready now indicates only namespace and UDN readiness (not storage
readiness), and that consumers must check the StorageBackendReady and
ClusterStorageReady conditions to determine storage readiness. Reference the
Operational impact section (line 149) and Phase logic (line 338) as context, and
note that ComputeInstance's backward-compatibility (line 38) serves as an
example. Include a note that this distinction becomes more critical with future
multi-cluster support (CaaS, mentioned in Non-Goals at line 46) where consumer
expectations may vary by deployment model.
🪄 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: f06227ce-c397-4d2b-a321-a6e562f75826
📒 Files selected for processing (1)
enhancements/tenant-storage-onboarding-OSAC-23/README.md
Introduces a dedicated OSAC Storage Controller that extracts all storage provisioning logic from the Tenant controller. The controller manages a two-stage onboarding workflow (backend setup, then cluster-side StorageClass installation) and a two-step ordered teardown, using condition ownership on the Tenant CR. Key design decisions: - Two conditions on Tenant CR: StorageBackendReady, ClusterStorageReady - Two ProvisioningProvider instances (backend + cluster-storage) - Tenant Phase=Ready decoupled from storage readiness - Four AAP playbook lifecycle actions - Per-backend and per-cluster status detail structs Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
ClusterOrder and Tenant live in different namespaces, so ownerReferences cannot be used. Replace EnqueueRequestForOwner with EnqueueRequestsFromMapFunc that reads the osac.openshift.io/tenant annotation from ClusterOrder to enqueue the correct Tenant. This is consistent with how ComputeInstance and all other OSAC resources associate with Tenants via the same annotation, set by the fulfillment-service at CR creation time. Also incorporates review feedback: management state scoping (FR-4 steady-state only), label-based hub Secret lookup, no-auto-retry on failure, operator-first deployment order, priority=1 print columns, per-tenant management-state in support procedures, and tier addition workflows as non-goal. Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
Frame the reconciliation loop around two pluggable sources (backend list and tier source) so the controller logic can evolve from single VAST backend to multiple backends (OSAC-1111) and from STORAGE_TIERS env var to StorageTier CRs (OSAC-1110) without rewriting the core loop or changing the status schema. Clarify that v0.1 storageBackends and clusterStorage lists each have a single entry, with the list structures anticipating multi-backend and multi-cluster support. Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
4347f79 to
ba99134
Compare
Add parenthetical notes to make the stage sequencing explicit for code reviewers: Secret absence indicates Stage 1 has not run, StorageBackendReady=True indicates Stage 1 is complete. Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
Remove source markers, cut redundant sentences, shorten failure table, simplify alternatives and boilerplate sections, remove inline Go comments from self-documenting structs. Preserves all technical content in a more concise form. Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
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 `@enhancements/tenant-storage-onboarding-OSAC-23/README.md`:
- Around line 442-447: The Risks and Mitigations table (lines 442–447) is
missing a HIGH-severity risk identified in the PR objectives: orphaned
finalizers when the storage controller is disabled or uninstalled. Add a new row
to the table documenting this risk with the following elements: the risk
scenario (Storage controller disabled/uninstalled with Tenants carrying the
osac.openshift.io/storage finalizer), the impact (High: Tenants stuck in
Terminating state), and the mitigations (graceful disable logic to auto-remove
finalizers if implemented as a feature, per-tenant use of
osac.openshift.io/management-state: Unmanaged before disabling the controller,
and manual finalizer removal documented in support procedures). Additionally,
clarify in the README whether graceful disable logic is a planned automatic
feature or a procedural recommendation only.
🪄 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: 6ccfe731-9752-4563-a103-7e023b0a86b6
📒 Files selected for processing (1)
enhancements/tenant-storage-onboarding-OSAC-23/README.md
| } | ||
| ``` | ||
|
|
||
| Gated by `OSAC_ENABLE_STORAGE_CONTROLLER` / `--enable-storage-controller`. |
There was a problem hiding this comment.
This follows the existing operator convention. All OSAC controllers have per-controller enable flags (cmd/main.go#L107-L136):
OSAC_ENABLE_TENANT_CONTROLLEROSAC_ENABLE_COMPUTE_INSTANCE_CONTROLLEROSAC_ENABLE_CLUSTER_CONTROLLEROSAC_ENABLE_NETWORKING_CONTROLLER
Default behavior: if no enable flags are set (neither env vars nor CLI flags), all controllers run (enableAllIfNoneSet()). If any flag is set, only the flagged controllers run. This lets operators run a subset for debugging, scaled deployments, or gradual rollouts.
The storage controller gets the same flag for consistency.
| **Credentials** are handled in three tiers, none of which the storage controller accesses directly: | ||
| - Admin credentials (VAST endpoint, username, password) live in the `storage-operations-ig` Secret, mounted as env vars in the AAP pod, cleared from playbook memory after use. | ||
| - Per-tenant credentials live in hub Secrets (`vast-tenant-config-{tenant_name}`) in `OSAC_STORAGE_CONFIG_NAMESPACE`. The storage controller checks their existence (Stage 1 gate) but never reads their contents. | ||
| - CSI credentials (`vast-csi-{tenant_name}`) are created by AAP in the tenant namespace with per-tenant manager credentials (not admin credentials). |
There was a problem hiding this comment.
so this stage creates the vast-tenant-config-{tenant_name} ?
There was a problem hiding this comment.
Yes. The osac-create-tenant-storage-backend AAP playbook (Stage 1) creates the hub Secret named vast-tenant-config-{tenant_name} in the OSAC_STORAGE_CONFIG_NAMESPACE namespace. The name prefix is defined in the VAST role defaults (vast_storage/defaults/main.yaml#L12: vast_storage_tenant_config_secret_prefix: "vast-tenant-config-"). The Secret stores per-tenant credentials (manager username/password, VAST tenant ID, endpoint) created during backend provisioning.
The storage controller only checks whether this Secret exists (Stage 1 completion gate). It never reads the contents.
|
/lgtm |
|
|
||
| The storage controller watches Tenant CRs for `Phase=Ready`. On detecting a ready Tenant, it begins Stage 1 by querying Secrets in `OSAC_STORAGE_CONFIG_NAMESPACE` with label `osac.openshift.io/tenant=<tenantName>`. If no matching Secret exists, the controller triggers the `osac-create-tenant-storage-backend` AAP playbook and polls until the job completes. On success, the controller sets `StorageBackendReady=True`. | ||
|
|
||
| After Stage 1 completes, Stage 2 begins. The controller triggers `osac-create-tenant-cluster-storage` on the VMaaS target cluster, polls until completion, then discovers installed StorageClasses using the `osac.openshift.io/tenant` label. When all expected StorageClasses are present, the controller populates `status.storageClasses` and sets `ClusterStorageReady=True`. |
There was a problem hiding this comment.
triggers
osac-create-tenant-cluster-storageon the VMaaS target cluster
Is there only one VMaaS target cluster? What happens if there are more than one?
ClusterStorageReady=True
Is Cluster StorageReady the right naming convention here? My initial thought based on name-alone was that this was CaaS related, and not for VMaas. What about other storage targets, like BMaaS or possibly even MaaS? Does it make sense to keep these statuses at the tenant level vs somewhere else?
There was a problem hiding this comment.
Is there only one VMaaS target cluster?
For this scope, yes. The VMaaS target is a single cluster configured via OSAC_REMOTE_CLUSTER_KUBECONFIG (or the hub cluster itself). The storage controller tracks its readiness in status.clusterStorage even though there's only one cluster today, so that CaaS clusters can be added to the same list without schema changes. See the Extension Points section.
Is Cluster StorageReady the right naming convention here?
ClusterStorageReady is intentionally delivery-model-agnostic. It answers "are StorageClasses installed on the cluster?" regardless of whether the cluster is VMaaS or CaaS. The name parallels StorageBackendReady (backend provisioned?) and avoids tying the condition to a specific delivery model.
Does it make sense to keep these statuses at the tenant level vs somewhere else?
For VMaaS, there's no per-cluster CR to put the condition on (the VMaaS target is just a kubeconfig, not a ClusterOrder). For CaaS, the CaaS PRD (OSAC-1123) is expected to add a ClusterStorageReady condition on each ClusterOrder CR for a per-cluster view. The Tenant's status.clusterStorage array would aggregate across all clusters so admins can see the full picture in one place. The status structure supports both without changes.
BMaaS provisions bare metal hosts, not Kubernetes clusters, so it doesn't have the same storage model. When BMaaS needs storage support, it will be a separate effort.
Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com> Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
eranco74
changed the title
4 participants
Design: Rework Tenant Storage Onboarding
Jira: OSAC-1001
PRD: PR #52
Summary
This design extracts all storage provisioning logic from the Tenant controller into a dedicated OSAC Storage Controller that owns
StorageBackendReadyandClusterStorageReadyconditions on the Tenant CR using the condition ownership pattern. The controller manages a two-stage onboarding workflow (backend setup via AAP, then cluster-side StorageClass installation) and a two-step ordered teardown, with four independent AAP playbooks replacing the current combined playbooks.Requesting Review On
ProvisioningProviderinstances (backend + cluster-storage) rather than extending the interface. Is this the right trade-off vs a single provider with template switching?StorageBackendStatusandClusterStorageStatusare added to Tenant status for observability. Are these fields structured correctly for the multi-backend future?Phase=Readynow means "namespace exists and UDN is reconciled" only. Storage readiness is tracked via independent conditions. Does this cause issues for any downstream consumers?Unmanagedskips steady-state reconciliation but finalizer/deletion handling still runs. Is this the right behavior?Dependencies
This design depends on:
How to Review
design/OSAC-23onzszabo-rh/enhancement-proposalsfor alignmentSigned-off-by: akshaynadkarni 25892229+akshaynadkarni@users.noreply.github.com
Assisted-by: Cursor/Claude
Summary by CodeRabbit