MGMT-23977: Implement per-tier StorageClass resolution in Tenant controller - #199
Conversation
|
@zszabo-rh: This pull request references MGMT-23977 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 sub-task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughController storage-class resolution changed from selecting a single tenant/shared Default to resolving StorageClasses across all distinct Changes
Sequence Diagram(s)sequenceDiagram
participant Controller
participant K8sAPI as "Kubernetes API\n(StorageClass list)"
participant Events
participant Status as "Tenant Status"
Controller->>K8sAPI: List StorageClasses
K8sAPI-->>Controller: StorageClass objects
Controller->>Controller: groupByTier -> sort tiers
alt For each tier
Controller->>Controller: resolve tenant-specific SCs
alt tenant-specific found (1)
Controller->>Status: add ResolvedStorageClass(tier, tenantSC)
else multiple tenant-specific
Controller->>Events: emit duplicate warning (per-tier)
Controller->>Status: set condition MultipleFound for tier
else none
Controller->>Controller: fallback -> find shared Default SCs
alt single shared Default
Controller->>Status: add ResolvedStorageClass(tier, defaultSC)
else multiple defaults
Controller->>Events: emit duplicate default warning
Controller->>Status: set condition MultipleFound for tier
else none
Controller->>Events: emit StorageClassNotReady (tier aggregated)
Controller->>Status: set StorageClassReady = False (NotFound)
end
end
end
Controller->>Status: set Status.StorageClasses (all resolved)
Controller->>Status: set Status.StorageClass = first resolved tier (if any)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 0/1 reviews remaining, refill in 42 minutes and 43 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/controller/tenant_controller.go (2)
181-194: Nit:conditionMessage()can use a value receiver.
conditionMessagedoesn't mutate state, andtierResolutionResultis small (four slice headers). A value receiver is more idiomatic for read-only methods on small structs. Optional.♻️ Proposed change
-func (r *tierResolutionResult) conditionMessage() string { +func (r tierResolutionResult) conditionMessage() string { var parts []string parts = append(parts, r.resolvedMessages...) parts = append(parts, r.errorMessages...) return strings.Join(parts, "; ") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/tenant_controller.go` around lines 181 - 194, Change the method receiver of conditionMessage from a pointer to a value receiver: replace func (r *tierResolutionResult) conditionMessage() string with a value receiver version (func (r tierResolutionResult) conditionMessage() string) leaving the body unchanged; this targets the tierResolutionResult type and the conditionMessage method only—no other call sites need modification since value and pointer receivers are interchangeable for method calls here.
240-252: Optional: collapse tier-set collection usingmaps.Keys/slices.Sort.The union-then-sort can be expressed more compactly with stdlib
maps/sliceshelpers. The project uses Go 1.25.0, which fully supports these functions (available since Go 1.21).♻️ Proposed change
- allTiers := make(map[string]struct{}) - for t := range tenantByTier { - allTiers[t] = struct{}{} - } - for t := range defaultByTier { - allTiers[t] = struct{}{} - } - - sortedTiers := make([]string, 0, len(allTiers)) - for t := range allTiers { - sortedTiers = append(sortedTiers, t) - } - sort.Strings(sortedTiers) + tierSet := make(map[string]struct{}, len(tenantByTier)+len(defaultByTier)) + for t := range tenantByTier { + tierSet[t] = struct{}{} + } + for t := range defaultByTier { + tierSet[t] = struct{}{} + } + sortedTiers := slices.Sorted(maps.Keys(tierSet))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/tenant_controller.go` around lines 240 - 252, Replace the manual key-collection and sort in tenant_controller.go by using Go stdlib helpers: after merging tenantByTier and defaultByTier into allTiers (map[string]struct{}), obtain the slice of tier names with maps.Keys(allTiers) and then sort in-place with slices.Sort; update references to sortedTiers to use that result. This touches the variables allTiers, tenantByTier, defaultByTier and sortedTiers.internal/controller/tenant_controller_test.go (1)
360-388: LGTM —groupByTierunit tests.Covers the three meaningful cases: label-missing is ignored, multi-SC grouping across distinct tiers, and nil input. One optional addition worth considering: an SC with an empty-string tier label (
osacStorageTierLabel="") to lock in thetier == ""branch ofgroupByTier. Not blocking.➕ Optional additional case
It("ignores StorageClasses without storage-tier label", func() { scs := []storagev1.StorageClass{ *makeSC("sc-no-tier", "tenant-a", ""), + // Explicit empty-string tier value (label present but empty) should also be ignored. + func() storagev1.StorageClass { + sc := makeSC("sc-empty-tier", "tenant-a", "") + sc.Labels[osacStorageTierLabel] = "" + return *sc + }(), *makeSC("sc-with-tier", "tenant-a", "fast"), } groups := groupByTier(scs) Expect(groups).To(HaveLen(1)) Expect(groups).To(HaveKey("fast")) Expect(groups["fast"]).To(HaveLen(1)) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/tenant_controller_test.go` around lines 360 - 388, Add a unit test that covers the case where a StorageClass has the storage-tier label present but set to the empty string to exercise the tier == "" branch in groupByTier; create an SC via makeSC with osacStorageTierLabel set to "" (distinct from nil/no-label case), call groupByTier with that SC and assert the resulting map contains a key "" with the expected slice length (or that empty-string keys are handled as your implementation expects) to lock in behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/tenant_controller.go`:
- Around line 206-218: The groupByTier function currently accepts any non-empty
osac.openshift.io/storage-tier label; update groupByTier to validate each tier
value against the CRD regex used by ResolvedStorageClass.Tier
(^[a-z0-9]([a-z0-9._-]*[a-z0-9])?$) and skip any label values that do not match
so invalid (e.g. uppercase) tiers are not propagated to
instance.Status.StorageClasses; include an optional processLogger/EventRecorder
call (or similar) to record skipped invalid tier values for observability and
reference groupByTier and ResolvedStorageClass.Tier when locating the code to
change.
---
Nitpick comments:
In `@internal/controller/tenant_controller_test.go`:
- Around line 360-388: Add a unit test that covers the case where a StorageClass
has the storage-tier label present but set to the empty string to exercise the
tier == "" branch in groupByTier; create an SC via makeSC with
osacStorageTierLabel set to "" (distinct from nil/no-label case), call
groupByTier with that SC and assert the resulting map contains a key "" with the
expected slice length (or that empty-string keys are handled as your
implementation expects) to lock in behavior.
In `@internal/controller/tenant_controller.go`:
- Around line 181-194: Change the method receiver of conditionMessage from a
pointer to a value receiver: replace func (r *tierResolutionResult)
conditionMessage() string with a value receiver version (func (r
tierResolutionResult) conditionMessage() string) leaving the body unchanged;
this targets the tierResolutionResult type and the conditionMessage method
only—no other call sites need modification since value and pointer receivers are
interchangeable for method calls here.
- Around line 240-252: Replace the manual key-collection and sort in
tenant_controller.go by using Go stdlib helpers: after merging tenantByTier and
defaultByTier into allTiers (map[string]struct{}), obtain the slice of tier
names with maps.Keys(allTiers) and then sort in-place with slices.Sort; update
references to sortedTiers to use that result. This touches the variables
allTiers, tenantByTier, defaultByTier and sortedTiers.
🪄 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 UI
Review profile: CHILL
Plan: Pro
Run ID: 3eae8515-ecc7-4d3c-80a2-e0adef310740
📒 Files selected for processing (2)
internal/controller/tenant_controller.gointernal/controller/tenant_controller_test.go
32ca009 to
34f43b8
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 (1)
internal/controller/tenant_controller_test.go (1)
116-118:⚠️ Potential issue | 🟠 MajorAssert reconcile success instead of discarding errors
On Line 117,
_ = doReconcile()can mask reconciliation failures and let assertions pass against stale status. Please assert success in the polling loop.Proposed fix
Eventually(func(g Gomega) { - _ = doReconcile() + g.Expect(doReconcile()).To(Succeed()) g.Expect(k8sClient.Get(ctx, typeNamespacedName, tenant)).To(Succeed()) g.Expect(tenant.Status.Phase).To(Equal(expectedPhase))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/tenant_controller_test.go` around lines 116 - 118, The test currently discards errors from doReconcile() which can hide failures; inside the Eventually(func(g Gomega) { ... }) replace `_ = doReconcile()` with an assertion that it succeeds (e.g. `g.Expect(doReconcile()).To(Succeed())`) so reconcile errors fail the poll immediately, then continue with `g.Expect(k8sClient.Get(ctx, typeNamespacedName, tenant)).To(Succeed())` to assert the fetched resource is present.
🧹 Nitpick comments (1)
internal/controller/tenant_controller_test.go (1)
40-45: Use the tenant label constant inmakeSCfor clarity
makeSCpopulatesObjectMeta.Labels, but Line 41 usesosacTenantAnnotation. Even if values currently match, this couples tests to cross-constant equivalence and is easy to regress. Prefer the label constant here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/tenant_controller_test.go` around lines 40 - 45, In makeSC, change the key used when populating ObjectMeta.Labels from osacTenantAnnotation to the tenant label constant (the constant used for tenant labels, e.g., osacTenantLabel) so the test uses the label constant rather than the annotation constant; locate makeSC and replace the labels[osacTenantAnnotation] assignment with labels[<tenant-label-constant>] = tenant (keeping the tier logic using osacStorageTierLabel unchanged) to avoid coupling the test to annotation vs label constant equality.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/controller/tenant_controller_test.go`:
- Around line 116-118: The test currently discards errors from doReconcile()
which can hide failures; inside the Eventually(func(g Gomega) { ... }) replace
`_ = doReconcile()` with an assertion that it succeeds (e.g.
`g.Expect(doReconcile()).To(Succeed())`) so reconcile errors fail the poll
immediately, then continue with `g.Expect(k8sClient.Get(ctx, typeNamespacedName,
tenant)).To(Succeed())` to assert the fetched resource is present.
---
Nitpick comments:
In `@internal/controller/tenant_controller_test.go`:
- Around line 40-45: In makeSC, change the key used when populating
ObjectMeta.Labels from osacTenantAnnotation to the tenant label constant (the
constant used for tenant labels, e.g., osacTenantLabel) so the test uses the
label constant rather than the annotation constant; locate makeSC and replace
the labels[osacTenantAnnotation] assignment with labels[<tenant-label-constant>]
= tenant (keeping the tier logic using osacStorageTierLabel unchanged) to avoid
coupling the test to annotation vs label constant equality.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d25785ed-56b6-47f8-ba7a-f27670190c37
📒 Files selected for processing (3)
internal/controller/tenant_controller.gointernal/controller/tenant_controller_test.gointernal/controller/tenant_names.go
✅ Files skipped from review due to trivial changes (1)
- internal/controller/tenant_names.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/tenant_controller.go
There was a problem hiding this comment.
I guess you should remove this for the CI to pass.
| g.Expect(tenant.Status.Phase).To(Equal(expectedPhase)) | ||
| g.Expect(tenant.Status.StorageClass).To(Equal(expectedSC)) | ||
| if expectedSC == "" { | ||
| if expectedSCs == nil { |
There was a problem hiding this comment.
Should we check status.StorageClass here too? It's populated for backward compatibility but not asserted anywhere. Something like:
if expectedSCs == nil {
g.Expect(tenant.Status.StorageClass).To(BeEmpty())
g.Expect(tenant.Status.StorageClasses).To(BeNil())
} else {
g.Expect(tenant.Status.StorageClasses).To(ConsistOf(expectedSCs))
g.Expect(tenant.Status.StorageClass).To(Equal(expectedSCs[0].Name))
}
|
|
| } | ||
| reason, | ||
| condMsg) | ||
| r.Recorder.Eventf(instance, nil, corev1.EventTypeWarning, eventReasonStorageClassNotReady, "StorageClassResolution", "%s", condMsg) |
There was a problem hiding this comment.
Question: When should a Tenant be considered not ready due to StorageClass issues?
Currently StorageClassNotReady is emitted only when zero tiers resolve. If some tiers resolve and others have duplicates, the Tenant is Ready and the only signal is a DuplicateStorageClass event.
This raises a question: should a Tenant be Ready when a tier that was explicitly configured for it doesn't resolve? For example, if an admin creates a tenant-specific "fast" StorageClass, then accidentally introduces a duplicate, the Tenant stays Ready. The misconfiguration is only visible in events, not in the phase.
Before deciding on the event model (StorageClassNotReady vs TenantNotReady vs TenantTierNotReady), it's worth clarifying the readiness semantics:
Option 1: At least one tier resolves = Ready (current, per EP #32)
StorageClassNotReadyonly fires when all tiers fail- Pro: lenient, unrelated tier misconfigurations don't block the tenant
- Con: hides misconfigurations in tiers explicitly set up for the tenant
Option 2: All discovered tiers must resolve = Ready
- Any tier failure blocks readiness
- Pro: catches all misconfigurations
- Con: a duplicate in a shared Default tier the tenant doesn't need would block it
Option 3: Spec-driven required tiers
- Add
requiredTiersto TenantSpec, controller checks exactly those - Pro: explicit contract, cleanest model
- Con: CRD change, broader scope
This PR implements Option 1 per the EP. Raising it here since the answer shapes what events we emit and when. Happy to discuss offline.
There was a problem hiding this comment.
I think option 1 is the right starting point for v1, a misconfiguration in one tier shouldn't block the entire tenant when other tiers are fine.
The DuplicateStorageClass event already signals per-tier misconfigurations. If we find that operators need stronger guarantees, option 3 would be the cleanest future evolution, it makes the contract explicit without overloading the resolution logic.
There was a problem hiding this comment.
Thinking about the event hierarchy a bit more. Even with Option 1, we could separate cause from effect in the event naming:
Tenant-level (all tiers failed, tenant is Progressing):
TenantNotReady: emitted when no tier resolves at all
Tier-level (specific tier failed, tenant may still be Ready):
TenantStorageTierNotReady: emitted per failing tier, with reason in the message:DuplicateStorageClass: multiple SCs matched for the tierStorageClassMissing: no SC found for the tier (no tenant-specific, no Default)
This separates what happened (cause) from the impact (effect), and also future-proofs for new failure reasons without changing the event structure. Today DuplicateStorageClass serves as both the cause and the event name, which works but conflates the two.
One thing to consider: for TenantStorageTierNotReady to be meaningful, we'd need to know which tiers are expected for a tenant. Otherwise we'd fire events for tiers that aren't relevant to the tenant (e.g., a shared Default "archive" tier that the tenant doesn't use). This might circle back to tracking enabled tiers per tenant, either in the spec or derived from discovered SCs. This can be tackled in post v1.0, if we identify the events are noisy or just as part of event hardening.
WDYT?
…roller Replace single-tier getTenantStorageClass() with getTenantStorageClasses() that resolves all storage tiers independently. For each tier, applies tenant-specific first, then shared Default fallback. StorageClasses without the storage-tier label are ignored. Tier values are normalized to lowercase. Tenant is Ready when at least one tier resolves. Per-tier duplicate detection emits DuplicateStorageClass warning events without blocking other tiers. Emit StorageClassNotReady warning event when no tier resolves. The deprecated status.storageClass field is populated from the first resolved tier for backward compatibility (MGMT-24139). Signed-off-by: Zoltan Szabo <zszabo@redhat.com> Generated-By: Claude Code (Anthropic)
- Remove unused TenantReasonSharedDefault and TenantReasonMultipleDefaultsFound constants (dead code after per-tier refactor) - Assert deprecated status.storageClass field in multi-tier tests to verify backward compatibility Signed-off-by: Zoltan Szabo <zszabo@redhat.com> Generated-By: Claude Code (Anthropic)
34f43b8 to
bd6d450
Compare
Remove unused storageTierFromLabel function. Add tier label validation in groupByTier: values that don't match the CRD pattern after lowercase normalization are silently skipped, preventing status update rejections and reconciliation loops from malformed labels. Signed-off-by: Zoltan Szabo <zszabo@redhat.com> Generated-By: Claude Code (Anthropic)
akshaynadkarni
left a comment
akshaynadkarni
left a comment
There was a problem hiding this comment.
@zszabo-rh Approving this PR. Let's carry forward this discussion offline.
It will good to get an agreement on the events to generate and add a ticket to the story to address it properly.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akshaynadkarni, zszabo-rh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
Summary
getTenantStorageClass()withgetTenantStorageClasses()that resolves all storage tiers independentlystorage-tierlabel value, apply two-step fallback: tenant-specific SC first, then shared Default SCosac.openshift.io/storage-tierlabel are ignoredReadywhen at least one tier resolves;Progressingwhen none doDuplicateStorageClasswarning events without blocking other tiersStorageClassNotReadywarning event when no tier resolvesgroupByTier()helper that classifies StorageClasses by tier labelstatus.storageClassfield populated from the first resolved tier for backward compatibility (MGMT-24139)Implements the resolution algorithm from EP #32.
Test plan
make testpasses — 12-step integration test covering: no namespace, no SCs, SC without tier label ignored, shared Default fallback, tenant-specific priority, multi-tier resolution, per-tier duplicate isolation, mixed resolution, full degradation with NotReady event, all-tiers-duplicategroupByTier()(including lowercase normalization) andjoinStorageClassNames()go build ./...compiles cleanlyJira: MGMT-23977
Summary by CodeRabbit
New Features
Chores
Tests