OSAC-3011: remove defaultStorageClassSentinel fallback - #397
Conversation
|
@zszabo-rh: This pull request references OSAC-3011 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zszabo-rh The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughStorageClass resolution now uses tenant-labeled classes exclusively. Shared Default fallback is removed, and reconciliation and tests now cover not-found, duplicate-class, and unavailable-backend outcomes. ChangesTenant storage resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TenantReconcile
participant resolveTenantSpecificStorageClasses
participant TenantStatus
TenantReconcile->>resolveTenantSpecificStorageClasses: list StorageClasses for tenant label
resolveTenantSpecificStorageClasses->>TenantStatus: return resolved, duplicate, or empty tier results
TenantReconcile->>TenantStatus: set ClusterStorageReady=False when no usable classes exist
Possibly related PRs
Suggested labels: 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
internal/controller/storage_controller_test.go (2)
405-433: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the "without triggering provisioning" claim.
The test title promises duplicate tenant SCs won't trigger provisioning, but the body never checks
tenant.Status.ClusterStorageJobs. Add the assertion so the test actually verifies its own claim (the production code does correctly skiphandleClusterStorageProvisioningon the duplicate-message branch instorage_controller.go, lines 272-283, but this test doesn't confirm it).✅ Proposed fix
Expect(clusterCond.Reason).To(Equal(v1alpha1.TenantReasonMultipleFound)) Expect(tenant.Status.StorageClasses).To(BeNil()) + Expect(tenant.Status.ClusterStorageJobs).To(BeEmpty()) })🤖 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 `@internal/controller/storage_controller_test.go` around lines 405 - 433, Extend the duplicate tenant storage class test to assert that tenant.Status.ClusterStorageJobs remains empty or nil after reconciliation. Add this check alongside the existing tenant status assertions in the test beginning “should detect duplicate tenant SCs,” preserving the current condition and StorageClasses validations.
377-403: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate tests: "no tenant SCs" and "no SCs at all" are now the same scenario.
Before the Default-SC fallback was removed, these two tests covered distinct states (tenant-specific SC missing but a shared Default SC present, vs. no SCs whatsoever). Now that the fallback path is gone, both tests create zero StorageClasses, use the same cluster-provider setup, and assert identical outcomes (
ClusterStorageReady=False/NotFound,StorageClassesnil,ClusterStorageJobslen 1). One of the two is now redundant test maintenance overhead.
internal/controller/storage_controller_test.go#L377-L403: keep this test (or the one at L435-L461, not both) and rename it to reflect the single remaining "no SCs, provider configured" scenario.internal/controller/storage_controller_test.go#L435-L461: remove this test since it duplicates the scenario and assertions now covered by L377-L403.🤖 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 `@internal/controller/storage_controller_test.go` around lines 377 - 403, Remove the duplicate “no SCs at all” test at internal/controller/storage_controller_test.go#L435-L461. Keep internal/controller/storage_controller_test.go#L377-L403, rename its test description to reflect the single “no storage classes, provider configured” scenario, and preserve its existing assertions and setup.internal/controller/storage_controller.go (1)
256-262: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale "fall back to Default SC" comments.
Both comment blocks still describe falling back to shared Default StorageClasses, but this PR removes that exact behavior (the code now immediately sets
ClusterStorageReady=False/NotFoundinstead). Update both to describe the tenant-only behavior to avoid misleading future readers.📝 Proposed comment fixes
if r.ClusterStorageProvider != nil { - // When AAP is configured, prefer tenant-specific StorageClasses - // (labeled osac.openshift.io/tenant=<tenantName>). If none exist, - // fall back to shared Default SCs so VMs can provision immediately, - // and trigger the AAP cluster storage job to create a proper - // tenant-specific SC. Once the tenant-specific SC appears (via the - // StorageClass watch), the next reconcile picks it up and replaces - // the Default. + // When AAP is configured, resolve tenant-specific StorageClasses + // (labeled osac.openshift.io/tenant=<tenantName>). If none exist, + // mark ClusterStorageReady=False and trigger the AAP cluster + // storage job to create one; the StorageClass watch will pick it + // up on the next reconcile.} else { - // When no provisioning provider is configured, resolve StorageClasses - // using the full tier resolution logic: tenant-specific SCs take - // priority, with shared default SCs (labeled tenant=Default) as - // fallback. This serves environments running OSAC without AAP/VAST - // where an admin or prepare-tenant.sh has labeled existing - // StorageClasses manually. + // When no provisioning provider is configured, resolve tenant- + // specific StorageClasses only. This serves environments running + // OSAC without AAP/VAST where an admin has labeled existing + // StorageClasses manually.Also applies to: 310-315
🤖 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 `@internal/controller/storage_controller.go` around lines 256 - 262, Update both StorageClass selection comment blocks near the reconciliation logic to remove references to falling back to shared Default StorageClasses or immediate VM provisioning. Describe that AAP requires a tenant-specific StorageClass and that reconciliation reports ClusterStorageReady=False/NotFound until it appears through the StorageClass watch.internal/controller/storage_tier_resolution.go (1)
73-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo now-identical tenant-SC resolution implementations.
With the default-fallback path removed,
getTenantStorageClasseshere andresolveTenantSpecificStorageClassesinstorage_controller.go(lines 894-927) both perform the same List-by-tenant-label →groupByTier→ per-tier single/duplicate resolution. Previously they diverged because this function also merged in shared Default SCs; that difference is gone now. Consider havingresolveTenantSpecificStorageClassessimply callgetTenantStorageClasses(or vice versa) and adapt the return shape, instead of maintaining two parallel implementations of the same algorithm.♻️ Sketch: delegate resolveTenantSpecificStorageClasses to getTenantStorageClasses
func (r *StorageReconciler) resolveTenantSpecificStorageClasses( ctx context.Context, targetClient client.Client, tenantName string, ) ([]v1alpha1.ResolvedStorageClass, []string, error) { - log := ctrllog.FromContext(ctx) - scList := &storagev1.StorageClassList{} - if err := targetClient.List(ctx, scList, client.MatchingLabels{osacTenantKey: tenantName}); err != nil { - return nil, nil, err - } - byTier := groupByTier(scList.Items) - sortedTiers := make([]string, 0, len(byTier)) - for tier := range byTier { - sortedTiers = append(sortedTiers, tier) - } - sort.Strings(sortedTiers) - - var resolved []v1alpha1.ResolvedStorageClass - var duplicateMessages []string - for _, tier := range sortedTiers { - scs := byTier[tier] - switch len(scs) { - case 1: - resolved = append(resolved, v1alpha1.ResolvedStorageClass{ - Name: scs[0].GetName(), - Tier: tier, - }) - default: - joined, names := joinStorageClassNames(scs) - msg := fmt.Sprintf("tier %q: multiple tenant StorageClasses [%s]", tier, joined) - log.Info(msg, "tenant", tenantName, "tier", tier, "storageClasses", names) - duplicateMessages = append(duplicateMessages, msg) - } - } - return resolved, duplicateMessages, nil + result, err := getTenantStorageClasses(ctx, targetClient, tenantName) + if err != nil { + return nil, nil, err + } + return result.resolved, result.duplicateMessages, nil }🤖 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 `@internal/controller/storage_tier_resolution.go` around lines 73 - 118, Consolidate the duplicate tenant StorageClass resolution logic by making resolveTenantSpecificStorageClasses delegate to getTenantStorageClasses and adapt tierResolutionResult to its existing return shape. Remove the parallel List-by-label, groupByTier, sorting, and per-tier resolution implementation while preserving duplicate and no-StorageClass messages.
🤖 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 `@internal/controller/storage_controller_test.go`:
- Around line 405-433: Extend the duplicate tenant storage class test to assert
that tenant.Status.ClusterStorageJobs remains empty or nil after reconciliation.
Add this check alongside the existing tenant status assertions in the test
beginning “should detect duplicate tenant SCs,” preserving the current condition
and StorageClasses validations.
- Around line 377-403: Remove the duplicate “no SCs at all” test at
internal/controller/storage_controller_test.go#L435-L461. Keep
internal/controller/storage_controller_test.go#L377-L403, rename its test
description to reflect the single “no storage classes, provider configured”
scenario, and preserve its existing assertions and setup.
In `@internal/controller/storage_controller.go`:
- Around line 256-262: Update both StorageClass selection comment blocks near
the reconciliation logic to remove references to falling back to shared Default
StorageClasses or immediate VM provisioning. Describe that AAP requires a
tenant-specific StorageClass and that reconciliation reports
ClusterStorageReady=False/NotFound until it appears through the StorageClass
watch.
In `@internal/controller/storage_tier_resolution.go`:
- Around line 73-118: Consolidate the duplicate tenant StorageClass resolution
logic by making resolveTenantSpecificStorageClasses delegate to
getTenantStorageClasses and adapt tierResolutionResult to its existing return
shape. Remove the parallel List-by-label, groupByTier, sorting, and per-tier
resolution implementation while preserving duplicate and no-StorageClass
messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c4f4bf20-c74d-46f8-a468-af281633ef54
📒 Files selected for processing (4)
internal/controller/storage_controller.gointernal/controller/storage_controller_test.gointernal/controller/storage_tier_resolution.gointernal/controller/tenant_names.go
💤 Files with no reviewable changes (1)
- internal/controller/tenant_names.go
Remove the tenant=Default StorageClass fallback mechanism. Environments that need LVMS-backed storage will now get it via the AAP dispatcher routing to local_lvms_storage, rather than relying on shared Default- labeled SCs as a stop-gap. Changes: - getTenantStorageClasses: drop defaultSCList query and all Default fallback logic; function now returns only tenant-specific SCs - storage_controller: remove defaultFallback call site and the tenant=Default sentinel branch in mapStorageClassToTenant (Default- labeled SC changes no longer trigger global tenant reconciliation) - tenant_names.go: remove defaultStorageClassSentinel constant - tests: update 6 test cases — remove sentinel SC setup and correct expected outcomes to reflect NotFound/False instead of fallback-True Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Zoltan Szabo <zszabo@redhat.com>
The function was left as dead code after removing the defaultStorageClassSentinel fallback (the sentinel was the only caller). golangci-lint flags it as unused. Generated-By: Claude Code (Anthropic) Signed-off-by: Zoltan Szabo <zszabo@redhat.com>
Two test cases in the Backend API routing context still created a sentinel SC and expected StorageClasses to be non-empty, reflecting the old fallback behavior. With the sentinel removed, both expectations should be nil. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Zoltan Szabo <zszabo@redhat.com>
2945042 to
22bbaf5
Compare
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 (7)
internal/controller/storage_controller_test.go (2)
472-498: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test is now a duplicate of "no SCs at all and provider is configured" (lines 530-556).
Both create a ready tenant + hub Secret + cluster provider with zero StorageClasses and assert
False/NotFound,StorageClassesnil, oneClusterStorageJobsentry. Removing the Default SC setup erased the only thing that distinguished them. The tenant namestorage-test-default-only-with-provideris also stale — there is no "default only" scenario anymore.Drop one, or give this one a distinct scenario worth covering.
Same collapse happened to lines 307-335 vs 337-364 in Stage 1: "no provider configured" and "no provider and no labeled SCs" are now the identical setup.
🤖 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 `@internal/controller/storage_controller_test.go` around lines 472 - 498, Remove the now-duplicate test using the stale “storage-test-default-only-with-provider” scenario name, and retain the existing “no SCs at all and provider is configured” coverage. Also remove the duplicate Stage 1 test where “no provider configured” and “no provider and no labeled SCs” have identical setup and assertions, preserving only one representative case.
500-528: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that duplicates do not trigger provisioning — that's the untested half of the new contract.
The title says "without triggering provisioning", but nothing checks it.
handleUpdatereturns early at the duplicate branch specifically to avoid firing an AAP job that can't resolve an operator-created duplicate; a future refactor that folds this branch into the not-found path would silently start launching jobs and every assertion here would still pass.🧪 Proposed addition
Expect(tenant.Status.StorageClasses).To(BeNil()) + Expect(tenant.Status.ClusterStorageJobs).To(BeEmpty()) })🤖 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 `@internal/controller/storage_controller_test.go` around lines 500 - 528, Extend the duplicate-tenant storage class test around NewStorageReconciler and mockProvisioningProvider to assert that provisioning is not invoked when duplicate classes are detected. Verify the provider records no provisioning call or job request after Reconcile, while preserving the existing ClusterStorageReady=False, MultipleFound, and nil StorageClasses assertions.internal/controller/storage_controller.go (4)
991-1027: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
resolveTenantSpecificStorageClassesis now a near-clone ofgetTenantStorageClasses.With the Default fallback deleted, both functions do the same thing: list by
osacTenantKey,groupByTier, sort, resolve single matches, and recordtier %q: multiple tenant StorageClasses [%s]for the rest. The only remaining delta isresolvedMessages/errorMessagesbookkeeping and the "no StorageClasses with … label found" sentinel message. The two resolvers existed because one had fallback and one didn't — that distinction is exactly what this PR removes.Keeping both means every future tier-resolution change has to be made twice, and the duplicate-message string is already copy-pasted verbatim in two places. Suggest collapsing onto
getTenantStorageClassesand having the AAP branch readresult.resolved/result.duplicateMessages/result.ambiguousTiersdirectly, deriving the condition message from the reason instead of a second result type.Also note the doc comment here still says "ignoring shared defaults (labeled tenant=Default)" — after this PR there is no such concept to ignore.
🤖 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 `@internal/controller/storage_controller.go` around lines 991 - 1027, Remove the redundant resolveTenantSpecificStorageClasses method and reuse getTenantStorageClasses for the AAP path, reading its resolved, duplicateMessages, and ambiguousTiers fields directly. Derive any condition message from the existing reason data instead of maintaining a second result type or duplicated tier-resolution logic, and update the related documentation so it no longer describes ignoring tenant=Default shared fallbacks.
332-341: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDoc comments across both files still describe the deleted
tenant=Defaultfallback. The code paths were removed but their surrounding prose wasn't, so three sites now document behavior that no longer exists — the fastest route to someone "restoring" the fallback later.
internal/controller/storage_controller.go#L332-L341: rewrite the preceding AAP-branch comment (Lines 325-331) to say resolution is tenant-only and reportsNotFound, and the no-provider-branch comment (Lines 381-386) to drop "with shared default SCs (labeled tenant=Default) as fallback".internal/controller/storage_controller.go#L991-L1027: drop "ignoring shared defaults (labeled tenant=Default)" from theresolveTenantSpecificStorageClassesdoc comment — there is no shared default left to ignore.internal/controller/storage_tier_resolution.go#L38-L41: change "(tenant-specific or Default)" to "multiple tenant-specific StorageClasses" on theambiguousTiersfield.🤖 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 `@internal/controller/storage_controller.go` around lines 332 - 341, Update the stale documentation about the removed Default-tenant fallback: in internal/controller/storage_controller.go lines 325-331, describe tenant-only resolution reporting NotFound; in lines 381-386, remove the shared-default fallback wording; in the resolveTenantSpecificStorageClasses documentation at lines 991-1027, remove the reference to ignoring shared defaults; and in internal/controller/storage_tier_resolution.go lines 38-41, describe ambiguousTiers as multiple tenant-specific StorageClasses.
93-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comment is missing a predicate.
"When nil (no gRPC connection configured), both backward compatible with environments…" doesn't parse. State what actually happens (backend registration is treated as unregistered and Get is skipped).
✏️ Proposed wording
- // BackendsClient queries the fulfillment service Backend API to determine whether - // a storage backend is registered (List, via backendRegistered()) and to resolve - // a backend's connection details by ID (Get, via resolveTierDefinitions). When - // nil (no gRPC connection configured), both backward compatible with - // environments that run without a fulfillment service connection (e.g. - // prepare-tenant.sh). + // BackendsClient queries the fulfillment service Backend API to determine whether + // a storage backend is registered (List, via backendRegistered()) and to resolve + // a backend's connection details by ID (Get, via resolveTierDefinitions). When nil + // (no gRPC connection configured), backend registration is treated as absent and + // backend resolution is skipped — backward compatible with environments that run + // without a fulfillment service connection (e.g. prepare-tenant.sh).🤖 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 `@internal/controller/storage_controller.go` around lines 93 - 99, The BackendsClient doc comment is grammatically incomplete and does not describe nil behavior. Update the comment for BackendsClient to state that when it is nil, backend registration is treated as unregistered and Get/connection-detail resolution is skipped, preserving compatibility with environments without a fulfillment-service connection.
986-991: 📐 Maintainability & Code Quality | 🔴 Critical | ⚡ Quick wingofmt failure — missing blank line before the doc comment.
CI lint is red on this exact spot. A doc comment must be separated from the preceding declaration.
🔧 Proposed fix
type tenantSpecificStorageClasses struct { resolved []v1alpha1.ResolvedStorageClass duplicateMessages []string ambiguousTiers []string } + // resolveTenantSpecificStorageClasses lists only StorageClasses labeled with the🤖 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 `@internal/controller/storage_controller.go` around lines 986 - 991, Add a blank line between the tenantSpecificStorageClasses struct declaration and the resolveTenantSpecificStorageClasses documentation comment, preserving the existing declarations and comment text.Source: Linters/SAST tools
internal/controller/storage_tier_resolution.go (1)
38-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale "or Default" in the
ambiguousTierscomment.Default-labeled classes are no longer resolved anywhere in this file, so a tier can only be ambiguous via multiple tenant-specific StorageClasses.
✏️ Proposed wording
// ambiguousTiers holds the name of every tier excluded from resolved because - // multiple StorageClasses matched it (tenant-specific or Default) — a distinct, - // separately-reported problem from a tier having no StorageClass at all. + // multiple tenant-specific StorageClasses matched it — a distinct, + // separately-reported problem from a tier having no StorageClass at all.🤖 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 `@internal/controller/storage_tier_resolution.go` around lines 38 - 41, Update the comment for ambiguousTiers to remove the stale “or Default” wording and state that ambiguity occurs only when multiple tenant-specific StorageClasses match a tier; leave the field and resolution logic unchanged.
🤖 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 `@internal/controller/storage_controller_test.go`:
- Around line 472-498: Remove the now-duplicate test using the stale
“storage-test-default-only-with-provider” scenario name, and retain the existing
“no SCs at all and provider is configured” coverage. Also remove the duplicate
Stage 1 test where “no provider configured” and “no provider and no labeled SCs”
have identical setup and assertions, preserving only one representative case.
- Around line 500-528: Extend the duplicate-tenant storage class test around
NewStorageReconciler and mockProvisioningProvider to assert that provisioning is
not invoked when duplicate classes are detected. Verify the provider records no
provisioning call or job request after Reconcile, while preserving the existing
ClusterStorageReady=False, MultipleFound, and nil StorageClasses assertions.
In `@internal/controller/storage_controller.go`:
- Around line 991-1027: Remove the redundant resolveTenantSpecificStorageClasses
method and reuse getTenantStorageClasses for the AAP path, reading its resolved,
duplicateMessages, and ambiguousTiers fields directly. Derive any condition
message from the existing reason data instead of maintaining a second result
type or duplicated tier-resolution logic, and update the related documentation
so it no longer describes ignoring tenant=Default shared fallbacks.
- Around line 332-341: Update the stale documentation about the removed
Default-tenant fallback: in internal/controller/storage_controller.go lines
325-331, describe tenant-only resolution reporting NotFound; in lines 381-386,
remove the shared-default fallback wording; in the
resolveTenantSpecificStorageClasses documentation at lines 991-1027, remove the
reference to ignoring shared defaults; and in
internal/controller/storage_tier_resolution.go lines 38-41, describe
ambiguousTiers as multiple tenant-specific StorageClasses.
- Around line 93-99: The BackendsClient doc comment is grammatically incomplete
and does not describe nil behavior. Update the comment for BackendsClient to
state that when it is nil, backend registration is treated as unregistered and
Get/connection-detail resolution is skipped, preserving compatibility with
environments without a fulfillment-service connection.
- Around line 986-991: Add a blank line between the tenantSpecificStorageClasses
struct declaration and the resolveTenantSpecificStorageClasses documentation
comment, preserving the existing declarations and comment text.
In `@internal/controller/storage_tier_resolution.go`:
- Around line 38-41: Update the comment for ambiguousTiers to remove the stale
“or Default” wording and state that ambiguity occurs only when multiple
tenant-specific StorageClasses match a tier; leave the field and resolution
logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 97dbf4e7-6ba8-4930-ba5a-9d85151c5297
📒 Files selected for processing (4)
internal/controller/storage_controller.gointernal/controller/storage_controller_test.gointernal/controller/storage_tier_resolution.gointernal/controller/tenant_names.go
The hook labeled the cluster default StorageClass with tenant=Default to support the defaultStorageClassSentinel fallback. With the sentinel removed, the hook is no longer needed and should not run. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Zoltan Szabo <zszabo@redhat.com>
2 participants
Summary
Removes the `tenant=Default` StorageClass fallback that was explicitly marked as
a temporary stop-gap in the code, pending the Backend API detection work in
OSAC-1957 (osac-operator#354).
What the fallback did: When no tenant-specific SC existed, the controller
looked for any SC labeled `osac.openshift.io/tenant=Default` and reported
`ClusterStorageReady=True` using that shared SC, with a note that tenant-specific
provisioning was pending. If no Default SC existed either, it reported `False`.
Why it was a stop-gap: Without the Backend API (PR #354), the controller had
no way to know whether storage was actually configured or simply hadn't provisioned
yet. The fallback papered over that gap. The in-code TODO explicitly named OSAC-1957
as the precondition for removal.
Now that PR #354 is in place: The controller knows whether a StorageBackend is
registered. If one is registered, it triggers AAP provisioning and waits for a
real per-tenant SC to appear. No tenant-specific SC means provisioning is
incomplete or storage is not configured for this environment — which should surface
as `ClusterStorageReady=False`, not be silently masked by a shared fallback.
Secondary benefit: Removing the sentinel also eliminates a mass-reconciliation
side effect — any change to a `tenant=Default`-labeled SC previously triggered
reconciliation of all tenants simultaneously.
Code changes:
logic from `getTenantStorageClasses`
remove the `defaultStorageClassSentinel` watch branch from `mapStorageClassToTenant`
Test plan
Dependencies
Generated-By: Claude Code (Anthropic)
Summary by CodeRabbit