MGMT-23976: Add multi-tier storage types to Tenant CRD (additive) - #204
Conversation
|
@zszabo-rh: This pull request references MGMT-23976 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. |
📝 WalkthroughWalkthroughAdds tier-aware storage resolution: introduces Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as "Controller"
participant K8sAPI as "Kubernetes API"
participant StorageClass as "StorageClass objects"
Controller->>K8sAPI: List/Watch StorageClasses
K8sAPI->>StorageClass: Return matching StorageClass(es)
StorageClass-->>Controller: StorageClass (with labels)
Controller->>Controller: Extract label osac.../storage-tier or "default"
Controller->>K8sAPI: Update Tenant.status (storageClass, storageClasses[{name,tier}], phase)
K8sAPI-->>Controller: Status update acknowledged
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/controller/tenant_controller_test.go (1)
109-115: Tier expectation hardcoded to"default"— fine for now, but plan to extend when tier becomes label-driven.The assertion mirrors today's controller behavior. Once
Tieris populated fromosac.openshift.io/storage-tier(per the doc comment onResolvedStorageClass.Tier), please add steps that label SCs with non-default tiers (e.g.,gold,silver) and assertStorageClassescontains the corresponding(name, tier)entries — including the+listMapKey=tieruniqueness invariant.🤖 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 109 - 115, The test currently asserts tenant.Status.StorageClasses[0].Tier == "default", which is too rigid; update the test in tenant_controller_test.go (around the block asserting tenant.Status.StorageClasses) to support non-default tiers by labeling created StorageClass objects with the osac.openshift.io/storage-tier annotation (e.g., "gold", "silver") and then assert that tenant.Status.StorageClasses contains entries with the expected (Name, Tier) pairs (using ResolvedStorageClass.Tier), not always "default"; also add an assertion that the list-map uniqueness invariant (+listMapKey=tier) holds for StorageClasses so no duplicate tiers exist for a given name. Ensure the test branches use the expected tier value for the given test case when expectedSC != "".
🤖 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 171-173: The Status currently sets ResolvedStorageClass.Tier to
the hardcoded "default" which violates the Tier contract; modify
getTenantStorageClass to return the resolved storagev1.StorageClass (or a struct
that includes it, e.g., storageClassResult carrying the StorageClass) instead of
only the name, then in the code that sets instance.Status.StorageClasses (where
ResolvedStorageClass is constructed) read the tier from the storage class labels
using the osacStorageTierLabel key and fallback to "default" if the label is
absent; update construction of ResolvedStorageClass (and any callers of
getTenantStorageClass) to propagate the label-derived Tier rather than the fixed
string.
In `@internal/controller/tenant_names.go`:
- Around line 50-51: The package-level variable osacStorageTierLabel is
currently unused and should either be removed or used to resolve storage tier;
update getTenantStorageClass (and the caller handleUpdate) to read
sc.Labels[osacStorageTierLabel] and set ResolvedStorageClass.Tier accordingly,
falling back to "default" when the label is absent or empty, or remove the
osacStorageTierLabel declaration if you prefer to defer label-based logic;
ensure the controller (where Tier is currently hardcoded to "default") uses the
value returned by getTenantStorageClass so golangci-lint unused is no longer
triggered.
---
Nitpick comments:
In `@internal/controller/tenant_controller_test.go`:
- Around line 109-115: The test currently asserts
tenant.Status.StorageClasses[0].Tier == "default", which is too rigid; update
the test in tenant_controller_test.go (around the block asserting
tenant.Status.StorageClasses) to support non-default tiers by labeling created
StorageClass objects with the osac.openshift.io/storage-tier annotation (e.g.,
"gold", "silver") and then assert that tenant.Status.StorageClasses contains
entries with the expected (Name, Tier) pairs (using ResolvedStorageClass.Tier),
not always "default"; also add an assertion that the list-map uniqueness
invariant (+listMapKey=tier) holds for StorageClasses so no duplicate tiers
exist for a given name. Ensure the test branches use the expected tier value for
the given test case when expectedSC != "".
🪄 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: d53e85d9-f3c5-41c9-9639-5fe6b6b92991
📒 Files selected for processing (6)
api/v1alpha1/tenant_types.goapi/v1alpha1/zz_generated.deepcopy.goconfig/crd/bases/osac.openshift.io_tenants.yamlinternal/controller/tenant_controller.gointernal/controller/tenant_controller_test.gointernal/controller/tenant_names.go
| // storageTierFromLabel reads the osac.openshift.io/storage-tier label from a | ||
| // StorageClass. Returns "default" if the label is absent or empty. | ||
| func storageTierFromLabel(sc *storagev1.StorageClass) string { | ||
| if tier := sc.GetLabels()[osacStorageTierLabel]; tier != "" { | ||
| return tier | ||
| } | ||
| return "default" | ||
| } |
There was a problem hiding this comment.
When the storage-tier label is absent, this falls back to "default". Two questions:
- What's the motivation for the fallback vs. treating a missing label as an error?
- When should this fallback be removed?
There was a problem hiding this comment.
The fallback exists for backward compatibility during the transition, so that existing tenants on the dev env wouldn't break. The fallback removal is tracked in the cleanup ticket. Once the full multi-tier resolution lands, SCs without the tier label will be treated as errors.
| // storageTierFromLabel reads the osac.openshift.io/storage-tier label from a | ||
| // StorageClass. Returns "default" if the label is absent or empty. | ||
| func storageTierFromLabel(sc *storagev1.StorageClass) string { | ||
| if tier := sc.GetLabels()[osacStorageTierLabel]; tier != "" { | ||
| return tier | ||
| } | ||
| return "default" | ||
| } |
There was a problem hiding this comment.
Kubernetes label values allow uppercase, but the Tier field validation only allows lowercase. If someone labels a StorageClass with storage-tier: FAST, the controller reads it as-is, finds the StorageClass, and tries to write the status. The API server rejects the update because FAST does not match the Tier validation pattern. Since the entire status update fails, the conditions do not get written either. The tenant is stuck in Progressing with no indication of what went wrong.
A couple of options:
- Lowercase the value in
storageTierFromLabelto handle this silently - Validate the value and set a
StorageClassReady=Falsecondition with a descriptive message before attempting the status update
What do you think?
There was a problem hiding this comment.
You're right, this can be a problem..
Applied strings.ToLower() to prevent the scenario you described!
Add ResolvedStorageClass struct with name and tier fields. Add status.storageClasses list alongside the existing status.storageClass string for backward compatibility. The controller populates both fields. The old field will be removed by MGMT-24139 once all consumers migrate. Add osac.openshift.io/storage-tier label constant. Add Storage Tiers printer column. Use listType=map with listMapKey=tier to enforce one entry per tier in the CRD schema. Verified on hypershift1: both fields populated, Tenant reaches Ready, existing ComputeInstance flow unaffected. Signed-off-by: Zoltan Szabo <zszabo@redhat.com> Generated-By: Claude Code (Anthropic)
Read the osac.openshift.io/storage-tier label from the resolved StorageClass and fall back to "default" when absent. This makes osacStorageTierLabel used and correctly populates the tier field from the actual SC label value. Signed-off-by: Zoltan Szabo <zszabo@redhat.com> Generated-By: Claude Code (Anthropic)
Lowercase the storage-tier label value in storageTierFromLabel to prevent uppercase values from failing CRD validation silently. The Tier field pattern only allows lowercase; without normalization, an uppercase label like "FAST" would cause the entire status update to be rejected by the API server, leaving the tenant stuck. Signed-off-by: Zoltan Szabo <zszabo@redhat.com> Generated-By: Claude Code (Anthropic)
1d91dca to
ee94ca5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 182-185: storageClassResult currently models only a single
{name,tier} causing the resolver in tenant_controller.go (functions around
storageClassResult usage and the success paths at the switch blocks) to treat
multi-tier results as "MultipleFound"; change the result shape to collect
StorageClass entries keyed by tier (e.g., map[string]storageClassEntry or a
slice of ResolvedStorageClass keyed by Tier) and update the resolution logic
(the code that builds storageClassResult, the switch len(...) branches, and the
return paths around lines 223-230 and 253-263) to: gather one StorageClass per
tier, detect and fail only on duplicate StorageClasses for the same tier, and
return a collection of ResolvedStorageClass (one per tier) instead of a single
entry so the API's multi-tier contract is satisfied.
🪄 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: 0b86e966-ac17-4144-a99e-b65129bd7093
📒 Files selected for processing (6)
api/v1alpha1/tenant_types.goapi/v1alpha1/zz_generated.deepcopy.goconfig/crd/bases/osac.openshift.io_tenants.yamlinternal/controller/tenant_controller.gointernal/controller/tenant_controller_test.gointernal/controller/tenant_names.go
✅ Files skipped from review due to trivial changes (2)
- internal/controller/tenant_names.go
- internal/controller/tenant_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- api/v1alpha1/zz_generated.deepcopy.go
|
[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
ResolvedStorageClassstruct withnameandtierfields to support multiple storage tiers per tenant (EP #32)status.storageClasseslist alongside the existingstatus.storageClassstring — controller populates bothosac.openshift.io/storage-tierlabel constantStorage Tiersprinter column (existingStorage Classcolumn retained)listType=mapwithlistMapKey=tierto enforce one entry per tierThis is a purely additive change — no fields removed, no existing behavior changed. The deprecated
status.storageClassfield will be removed by MGMT-24139 once all downstream consumers migrate tostatus.storageClasses.Test plan
make testpasses — existing tests retained, new assertions verifystorageClassesmirrorsstorageClassmake manifestsregenerates CRD with correct schemago build ./...compiles cleanlyJira: MGMT-23976
Summary by CodeRabbit
New Features
Documentation