feat: require tokenRateLimits and remove tokenRateLimitRef for 3.4#644
Conversation
BREAKING CHANGE: MaaSSubscription CRD now requires inline tokenRateLimits on each model reference. The unused tokenRateLimitRef field has been removed. Changes: - Remove tokenRateLimitRef field from ModelSubscriptionRef API type - Make tokenRateLimits required with MinItems=1 validation - Update CRD manifests and generated code (make generate manifests) - Fix 4 unit tests to add missing field indexes - Update documentation to reflect required tokenRateLimits - Add migration guide for 3.4 breaking change - Add release notes for v3.4.0 Live cluster testing verified: - Subscriptions without tokenRateLimits are rejected (Required value) - Subscriptions with tokenRateLimits are accepted - Old tokenRateLimitRef field is rejected (unknown field) - Controller reconciliation works correctly Migration: Users must add inline tokenRateLimits to all MaaSSubscription resources before upgrading to 3.4. See docs/content/migration/tokenratelimits-required-3.4.md for detailed migration instructions. Resolves: User story for MaaSSubscription API tokenRateLimits requirement Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe changes remove the optional Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
somya-bhatnagar
changed the title
![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)
maas-controller/pkg/controller/maas/maassubscription_controller_test.go (1)
400-400: Replace duplicated"spec.modelRef"literals with the shared index-key symbol.Hardcoding the field index key in multiple tests is drift-prone. Use the same key symbol used by the reconciler setup.
Proposed refactor
- WithIndex(&maasv1alpha1.MaaSSubscription{}, "spec.modelRef", subscriptionModelRefIndexer). + WithIndex(&maasv1alpha1.MaaSSubscription{}, modelRefIndexKey, subscriptionModelRefIndexer).Apply the same replacement at Line 496, Line 722, and Line 816.
Also applies to: 496-496, 722-722, 816-816
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@maas-controller/pkg/controller/maas/maassubscription_controller_test.go` at line 400, Replace the hardcoded "spec.modelRef" string in the test WithIndex calls with the reconciler's shared index-key symbol (use the same constant used in reconciler setup, e.g. subscriptionModelRefIndexKey) instead of the literal; update the calls that currently pass "spec.modelRef" (they also pass subscriptionModelRefIndexer) so they read WithIndex(&maasv1alpha1.MaaSSubscription{}, subscriptionModelRefIndexKey, subscriptionModelRefIndexer), and make the same replacement at the other test sites where "spec.modelRef" is duplicated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/content/migration/tokenratelimits-required-3.4.md`:
- Around line 95-98: Change the code fence language from bash to yaml for the
example showing the YAML key tokenRateLimitRef so the snippet is valid YAML and
won't be mis-copied; locate the block containing the fenced code that currently
begins with ```bash and replace the fence language with ```yaml (or
alternatively provide an actual shell command if you intend a bash example) so
the shown content (tokenRateLimitRef: "some-ref") is correctly labeled.
In `@maas-controller/api/maas/v1alpha1/maassubscription_types.go`:
- Around line 67-68: The TokenRateLimit.Limit field currently allows
zero/negative values; add a Kubebuilder validation marker to enforce a minimum
of 1 on the Limit field. In the maassubscription_types.go TokenRateLimit struct,
add a comment tag like +kubebuilder:validation:Minimum=1 immediately above the
Limit field declaration and regenerate CRD YAMLs if needed so the API rejects
values <1. Ensure the marker is applied to the Limit field name (Limit) so the
controller and admission validation enforce the constraint.
---
Nitpick comments:
In `@maas-controller/pkg/controller/maas/maassubscription_controller_test.go`:
- Line 400: Replace the hardcoded "spec.modelRef" string in the test WithIndex
calls with the reconciler's shared index-key symbol (use the same constant used
in reconciler setup, e.g. subscriptionModelRefIndexKey) instead of the literal;
update the calls that currently pass "spec.modelRef" (they also pass
subscriptionModelRefIndexer) so they read
WithIndex(&maasv1alpha1.MaaSSubscription{}, subscriptionModelRefIndexKey,
subscriptionModelRefIndexer), and make the same replacement at the other test
sites where "spec.modelRef" is duplicated.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 583f99f8-4c9d-415a-933e-b1695bad7699
📒 Files selected for processing (8)
deployment/base/maas-controller/crd/bases/maas.opendatahub.io_maassubscriptions.yamldocs/content/configuration-and-management/maas-controller-overview.mddocs/content/migration/tokenratelimits-required-3.4.mddocs/content/reference/crds/maas-subscription.mddocs/content/release-notes/index.mdmaas-controller/api/maas/v1alpha1/maassubscription_types.gomaas-controller/api/maas/v1alpha1/zz_generated.deepcopy.gomaas-controller/pkg/controller/maas/maassubscription_controller_test.go
💤 Files with no reviewable changes (1)
- maas-controller/api/maas/v1alpha1/zz_generated.deepcopy.go
The tokenRateLimitRef removal example was showing YAML content but was marked as bash. Changed to yaml for correct syntax highlighting. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
maas-controller/api/maas/v1alpha1/maassubscription_types.go (1)
67-68: Validation correctly enforces non-empty tokenRateLimits.The
MinItems=1marker and removal ofomitemptyproperly enforce that each model reference must have at least one rate limit defined at admission time.However, this change creates dead code in
maas-controller/pkg/controller/maas/maassubscription_controller.golines 212-217. The guardif len(mRef.TokenRateLimits) > 0and its else branch (default 100-token fallback) can never be reached since the API now guarantees at least one item.,
Consider removing the dead code path in a follow-up:
for _, mRef := range sub.Spec.ModelRefs { if mRef.Namespace != modelNamespace || mRef.Name != modelName { continue } var rates []interface{} - if len(mRef.TokenRateLimits) > 0 { - for _, trl := range mRef.TokenRateLimits { - rates = append(rates, map[string]interface{}{"limit": trl.Limit, "window": trl.Window}) - } - } else { - rates = append(rates, map[string]interface{}{"limit": int64(100), "window": "1m"}) + for _, trl := range mRef.TokenRateLimits { + rates = append(rates, map[string]interface{}{"limit": trl.Limit, "window": trl.Window}) } subs = append(subs, subInfo{sub: sub, mRef: mRef, rates: rates}) break }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@maas-controller/api/maas/v1alpha1/maassubscription_types.go` around lines 67 - 68, Remove the dead branch that checks len(mRef.TokenRateLimits) > 0 in maassubscription_controller.go and its else fallback to the 100-token default (the guard and fallback are no longer reachable because the CRD enforces MinItems=1 on TokenRateLimits); update the logic that reads rate limits (references to mRef.TokenRateLimits and the code handling the empty-case/default) to directly consume the guaranteed entries (e.g., use the first TokenRateLimit or iterate over TokenRateLimits as intended) and delete the unreachable else-path and related comments so the controller assumes at least one rate limit per model reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/content/migration/tokenratelimits-required-3.4.md`:
- Around line 141-143: The markdown fenced code blocks showing error messages
(e.g., the blocks containing "Error: spec.modelRefs[0].tokenRateLimits: Required
value", "Error: unknown field \"spec.modelRefs[0].tokenRateLimitRef\"", and
"Error: spec.modelRefs[0].tokenRateLimits: Invalid value: \"array\": must have
at least 1 items") need a language specifier to satisfy MD040; edit the three
affected code fences in docs/content/migration/tokenratelimits-required-3.4.md
(the blocks around those exact error strings) and add `text` after the opening
``` so each becomes ```text.
---
Nitpick comments:
In `@maas-controller/api/maas/v1alpha1/maassubscription_types.go`:
- Around line 67-68: Remove the dead branch that checks
len(mRef.TokenRateLimits) > 0 in maassubscription_controller.go and its else
fallback to the 100-token default (the guard and fallback are no longer
reachable because the CRD enforces MinItems=1 on TokenRateLimits); update the
logic that reads rate limits (references to mRef.TokenRateLimits and the code
handling the empty-case/default) to directly consume the guaranteed entries
(e.g., use the first TokenRateLimit or iterate over TokenRateLimits as intended)
and delete the unreachable else-path and related comments so the controller
assumes at least one rate limit per model reference.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 54f32997-624b-4c68-a937-fb3846c63fbf
📒 Files selected for processing (3)
deployment/base/maas-controller/crd/bases/maas.opendatahub.io_maassubscriptions.yamldocs/content/migration/tokenratelimits-required-3.4.mdmaas-controller/api/maas/v1alpha1/maassubscription_types.go
| ``` | ||
| Error: spec.modelRefs[0].tokenRateLimits: Required value | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to error message code blocks.
Per markdownlint MD040, fenced code blocks should have a language specified. Use text for error messages.
Proposed fix
### Missing tokenRateLimits
-```
+```text
Error: spec.modelRefs[0].tokenRateLimits: Required valueSolution: Add at least one tokenRateLimits entry to each model reference.
Using tokenRateLimitRef
- +text
Error: unknown field "spec.modelRefs[0].tokenRateLimitRef"
**Solution:** Remove `tokenRateLimitRef` field and use inline `tokenRateLimits` instead.
### Empty tokenRateLimits array
-```
+```text
Error: spec.modelRefs[0].tokenRateLimits: Invalid value: "array": must have at least 1 items
</details>
Also applies to: 149-151, 157-159
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.0)</summary>
[warning] 141-141: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @docs/content/migration/tokenratelimits-required-3.4.md around lines 141 -
143, The markdown fenced code blocks showing error messages (e.g., the blocks
containing "Error: spec.modelRefs[0].tokenRateLimits: Required value", "Error:
unknown field "spec.modelRefs[0].tokenRateLimitRef"", and "Error:
spec.modelRefs[0].tokenRateLimits: Invalid value: "array": must have at least
1 items") need a language specifier to satisfy MD040; edit the three affected
code fences in docs/content/migration/tokenratelimits-required-3.4.md (the
blocks around those exact error strings) and add text after the opening so each becomestext.
</details>
<!-- fingerprinting:phantom:medusa:ocelot:fe928791-81e3-4f86-8631-846ee7d81f1d -->
<!-- This is an auto-generated comment by CodeRabbit -->
jland-redhat
left a comment
jland-redhat
left a comment
There was a problem hiding this comment.
Looks good but I think we can just remove the breaking change piece.
Once that is out I will LGTM
| @@ -0,0 +1,173 @@ | |||
| # MaaSSubscription Breaking Change - tokenRateLimits Required (3.4) | |||
There was a problem hiding this comment.
Mentioned this in the comments but we do not need a migration plan for this one.
It will be covered by our overall migration plan
There was a problem hiding this comment.
Understood, will get rid of this.
docs/content/release-notes/index.md
Outdated
|
|
||
| #### MaaSSubscription: tokenRateLimits Now Required | ||
|
|
||
| **⚠️ BREAKING CHANGE** |
There was a problem hiding this comment.
As noted above all of these objects are new so everything is breaking :)
We do need to add that to the release notes in a general sense but we don't need this change noted.
There was a problem hiding this comment.
Agree, updated.
…-service into feat/tokenratelimits-required-3.4
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 `@docs/content/release-notes/index.md`:
- Around line 7-9: The release note entry for v3.4.0 is too generic about
breaking CRD/API changes; update the v3.4.0 section in
docs/content/release-notes/index.md to list exact schema deltas for the new MaaS
CRDs (MaaSModelRef, MaaSAuthPolicy, MaaSSubscription): enumerate added required
fields, removed/renamed fields, changed field types, default value changes, and
any new validation/webhook behaviors, and include concrete migration steps
(e.g., field mappings or example kubectl patch commands) so operators can
perform upgrades safely.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: a74084ed-d08d-4f46-90a1-764249e1f3d6
📒 Files selected for processing (1)
docs/content/release-notes/index.md
| Version 3.4.0 introduces new CRDs and API resources that are not compatible with previous versions. All MaaS custom resources (`MaaSModelRef`, `MaaSAuthPolicy`, `MaaSSubscription`) are new in this release. | ||
|
|
||
| **Migration:** See the overall migration plan for detailed upgrade instructions from previous versions. |
There was a problem hiding this comment.
Document the exact breaking schema changes in this release note entry.
This text is too generic for a breaking CRD/API change and can lead to upgrade failures because it does not name the required/removed fields. Add the concrete migration-critical deltas directly in v3.4.0 notes.
Suggested edit
-Version 3.4.0 introduces new CRDs and API resources that are not compatible with previous versions. All MaaS custom resources (`MaaSModelRef`, `MaaSAuthPolicy`, `MaaSSubscription`) are new in this release.
+Version 3.4.0 introduces breaking MaaSSubscription schema changes:
+- `spec.modelRefs[].tokenRateLimits` is now required and must contain at least one item (`minItems: 1`).
+- `spec.modelRefs[].tokenRateLimitRef` has been removed.
+- `spec.modelRefs[].tokenRateLimits[].limit` now enforces `minimum: 1`.
+Resources missing `tokenRateLimits` will be rejected by validation after upgrade.
-**Migration:** See the overall migration plan for detailed upgrade instructions from previous versions.
+**Migration:** Update all `MaaSSubscription` resources to inline `tokenRateLimits` before upgrading. See `docs/content/migration/tokenratelimits-required-3.4.md`.As per coding guidelines, "REVIEW PRIORITIES: ... 3. Bug-prone patterns and error handling gaps".
🧰 Tools
🪛 LanguageTool
[style] ~7-~7: Consider using “incompatible” to avoid wordiness.
Context: ...ces new CRDs and API resources that are not compatible with previous versions. All MaaS custom...
(NOT_ABLE_PREMIUM)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/content/release-notes/index.md` around lines 7 - 9, The release note
entry for v3.4.0 is too generic about breaking CRD/API changes; update the
v3.4.0 section in docs/content/release-notes/index.md to list exact schema
deltas for the new MaaS CRDs (MaaSModelRef, MaaSAuthPolicy, MaaSSubscription):
enumerate added required fields, removed/renamed fields, changed field types,
default value changes, and any new validation/webhook behaviors, and include
concrete migration steps (e.g., field mappings or example kubectl patch
commands) so operators can perform upgrades safely.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jland-redhat, somya-bhatnagar 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 |
openshift-merge-bot
bot
merged commit d4a15d8
into
opendatahub-io:main
…pendatahub-io#644) ## Summary Related to - https://redhat.atlassian.net/browse/RHOAIENG-55976 This PR makes `tokenRateLimits` required on each `MaaSSubscription` model reference and removes the unused `tokenRateLimitRef` field from the CRD. ## Changes ### API Changes - ✅ Removed `tokenRateLimitRef` field from `ModelSubscriptionRef` - ✅ Made `tokenRateLimits` required with `MinItems=1` validation - ✅ Regenerated CRD manifests and deepcopy code ### Tests - ✅ Fixed 4 unit tests (added missing field indexes) - ✅ All tests passing - ✅ Live cluster validation complete ### Documentation - ✅ Updated CRD reference docs - ✅ Updated architecture overview - ✅ Created migration guide: `docs/content/migration/tokenratelimits-required-3.4.md` - ✅ Added v3.4.0 release notes with breaking change notice ## Live Cluster Testing Results (https://console-openshift-console.apps.ci-ln-hdzds3t-76ef8.aws-4.ci.openshift.org/) Deployed custom controller and verified: - ✅ Subscriptions **without** `tokenRateLimits` → **REJECTED** with "Required value" error - ✅ Subscriptions **with** `tokenRateLimits` → **ACCEPTED** - ✅ Subscriptions with old `tokenRateLimitRef` → **REJECTED** with "unknown field" error - ✅ Controller reconciliation working correctly - ✅ No RBAC errors ## Migration Impact **Before (3.3 and earlier):** ```yaml modelRefs: - name: my-model namespace: llm tokenRateLimits: # Optional - limit: 1000 window: 1m tokenRateLimitRef: "some-ref" # Unused by controller ``` **After (3.4+):** ```yaml modelRefs: - name: my-model namespace: llm tokenRateLimits: # REQUIRED - at least one - limit: 1000 window: 1m # tokenRateLimitRef removed ``` ## Migration Guide See detailed migration instructions in `docs/content/migration/tokenratelimits-required-3.4.md` Users must add inline `tokenRateLimits` to all `MaaSSubscription` resources before upgrading to 3.4. ## Acceptance Criteria All criteria from the user story met: - [x] `tokenRateLimitRef` not present in API - [x] Each `modelRefs` entry must include non-empty `tokenRateLimits` - [x] Validation rejects non-compliant resources with clear messages - [x] Tests cover required limits, no `tokenRateLimitRef` references - [x] Generated CRD YAML matches types - [x] Documentation updated - [x] Migration guide provided - [x] Breaking change documented in release notes ## Files Changed - `maas-controller/api/maas/v1alpha1/maassubscription_types.go` - API types - `deployment/base/maas-controller/crd/bases/maas.opendatahub.io_maassubscriptions.yaml` - CRD manifest - `maas-controller/api/maas/v1alpha1/zz_generated.deepcopy.go` - Generated code - `maas-controller/pkg/controller/maas/maassubscription_controller_test.go` - Test fixes - `docs/content/configuration-and-management/maas-controller-overview.md` - Architecture docs - `docs/content/reference/crds/maas-subscription.md` - CRD reference - `docs/content/migration/tokenratelimits-required-3.4.md` - Migration guide (new) - `docs/content/release-notes/index.md` - Release notes 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Breaking Changes** * Rate limit configuration for each model subscription is now mandatory and must be specified inline rather than referenced externally * At least one rate limit entry is required per model * Rate limit values must be greater than zero * **Documentation** * Updated configuration and CRD documentation to reflect new rate limit requirements <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
2 participants
Summary
Related to - https://redhat.atlassian.net/browse/RHOAIENG-55976
This PR makes
tokenRateLimitsrequired on eachMaaSSubscriptionmodel reference and removes the unusedtokenRateLimitReffield from the CRD.Changes
API Changes
tokenRateLimitReffield fromModelSubscriptionReftokenRateLimitsrequired withMinItems=1validationTests
Documentation
docs/content/migration/tokenratelimits-required-3.4.mdLive Cluster Testing Results (https://console-openshift-console.apps.ci-ln-hdzds3t-76ef8.aws-4.ci.openshift.org/)
Deployed custom controller and verified:
tokenRateLimits→ REJECTED with "Required value" errortokenRateLimits→ ACCEPTEDtokenRateLimitRef→ REJECTED with "unknown field" errorMigration Impact
Before (3.3 and earlier):
After (3.4+):
Migration Guide
See detailed migration instructions in
docs/content/migration/tokenratelimits-required-3.4.mdUsers must add inline
tokenRateLimitsto allMaaSSubscriptionresources before upgrading to 3.4.Acceptance Criteria
All criteria from the user story met:
tokenRateLimitRefnot present in APImodelRefsentry must include non-emptytokenRateLimitstokenRateLimitRefreferencesFiles Changed
maas-controller/api/maas/v1alpha1/maassubscription_types.go- API typesdeployment/base/maas-controller/crd/bases/maas.opendatahub.io_maassubscriptions.yaml- CRD manifestmaas-controller/api/maas/v1alpha1/zz_generated.deepcopy.go- Generated codemaas-controller/pkg/controller/maas/maassubscription_controller_test.go- Test fixesdocs/content/configuration-and-management/maas-controller-overview.md- Architecture docsdocs/content/reference/crds/maas-subscription.md- CRD referencedocs/content/migration/tokenratelimits-required-3.4.md- Migration guide (new)docs/content/release-notes/index.md- Release notes🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Breaking Changes
Documentation