feat(operator): add LimitRange defaults for Cluster Autoscaler bin-packing#1040
Merged
Gkrumbach07 merged 1 commit intoambient-code:mainfrom Mar 26, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughAdds a LimitRange resource and registers it in kustomize; grants operator RBAC for limitranges; reconciler ensures per-namespace LimitRanges and updates Changes
Sequence DiagramsequenceDiagram
participant Reconciler as ProjectSettingsReconciler
participant Handler as ensureLimitRange
participant KubeAPI as Kubernetes API
participant CR as ProjectSettings CR
Reconciler->>Handler: ensureLimitRange(namespace, owner)
Handler->>KubeAPI: GET LimitRange "ambient-default-limits" in namespace
alt exists
KubeAPI-->>Handler: returns LimitRange
Handler->>CR: mark status.limitRangeReady = true
else not found
KubeAPI-->>Handler: NotFound
Handler->>Handler: build LimitRange manifest (defaults + ownerRef)
Handler->>KubeAPI: CREATE LimitRange
KubeAPI-->>Handler: created
Handler->>CR: mark status.limitRangeReady = true
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/operator/internal/handlers/projectsettings.go`:
- Around line 153-158: The code currently treats ensureLimitRange(namespace,
obj) failures as non-fatal by setting limitRangeReady = false and continuing;
change this to fail the reconcile immediately so missing/failed LimitRange
reconciliation is reported as an error. Locate the block using ensureLimitRange,
limitRangeReady, namespace and obj in projectsettings.go and replace the
non-fatal branch with an immediate return of the error (or wrap and return a
descriptive error) so the reconcile reports failure when ensureLimitRange
returns an error.
- Around line 222-225: The current create-only logic in the LimitRange creation
block (call to config.K8sClient.CoreV1().LimitRanges(namespace).Create with
lrName and namespace) treats AlreadyExists as success and skips reconciliation;
change it to detect errors.IsAlreadyExists(err) and then fetch the existing
LimitRange (using the same LimitRanges(namespace).Get), compare the
spec/labels/ownerReferences against the desired lr, and if they differ perform
an Update (LimitRanges(namespace).Update) to reconcile fields (or Patch as
appropriate) so pre-existing ambient-default-limits with drifted values are
corrected; ensure you preserve resourceVersion when updating and log or return
errors from Get/Update operations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b0fa6d5e-4f4b-43e8-8c03-f849cd69ef0b
📒 Files selected for processing (5)
components/manifests/base/core/kustomization.yamlcomponents/manifests/base/core/limitrange.yamlcomponents/manifests/base/rbac/operator-clusterrole.yamlcomponents/operator/internal/handlers/projectsettings.gocomponents/operator/internal/handlers/projectsettings_test.go
chambridge
force-pushed
the
fix/limitrange-default-requests
branch
from
43fb809 to
5e542eb
Compare
Contributor
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 `@components/operator/internal/handlers/projectsettings.go`:
- Around line 160-165: The ProjectSettings controller writes
statusUpdate["limitRangeReady"] (limitRangeReady) but the CRD schema for
ProjectSettings doesn't declare this status property, so the apiserver prunes
it; update the ProjectSettings CRD schema to include a boolean status property
named limitRangeReady (with an appropriate description) under status.properties
so UpdateStatus will persist this field; ensure the CRD validation type matches
the controller (boolean) and regenerate/apply the CRD manifest so the
controller's statusUpdate map can store limitRangeReady durably.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 881cbd46-7f38-4d02-b09f-006e1594f0da
📒 Files selected for processing (5)
components/manifests/base/core/kustomization.yamlcomponents/manifests/base/core/limitrange.yamlcomponents/manifests/base/rbac/operator-clusterrole.yamlcomponents/operator/internal/handlers/projectsettings.gocomponents/operator/internal/handlers/projectsettings_test.go
chambridge
force-pushed
the
fix/limitrange-default-requests
branch
2 times, most recently
from
5d63dbe to
84ec8ac
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/manifests/base/core/limitrange.yaml`:
- Around line 11-16: Remove the hard `default` limits from the LimitRange
manifest and the operator code so only `defaultRequest` remains: delete the
`default` block in the YAML snippet (the cpu: "2" and memory: 4Gi entries) and
update the operator code in
components/operator/internal/handlers/projectsettings.go to stop
creating/injecting the `Default` limits (remove or conditionally omit the fields
set around the LimitRange creation at the block that sets Default CPU/Memory—the
code around lines that set Default.Limits/DefaultRequest). Ensure the manifest
and the operator-created LimitRange only include `defaultRequest` to avoid
imposing cluster-wide hard limits.
In `@components/manifests/base/crds/projectsettings-crd.yaml`:
- Around line 80-82: The CRD lacks the status subresource so ProjectSettings
status updates (e.g., via UpdateStatus in
components/operator/internal/handlers/projectsettings.go) will be ignored;
update the CRD version spec in projectsettings-crd.yaml to enable
subresources.status for the relevant version so that .status fields like
limitRangeReady are writable via the /status endpoint, ensuring the status
subresource is declared at the version level (not just the schema).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 974ec258-2f89-4ed9-a99c-40de0278a5af
📒 Files selected for processing (6)
components/manifests/base/core/kustomization.yamlcomponents/manifests/base/core/limitrange.yamlcomponents/manifests/base/crds/projectsettings-crd.yamlcomponents/manifests/base/rbac/operator-clusterrole.yamlcomponents/operator/internal/handlers/projectsettings.gocomponents/operator/internal/handlers/projectsettings_test.go
…cking Add LimitRange objects with defaultRequest values to both the platform namespace (static YAML manifest) and project namespaces (dynamically created by the operator during ProjectSettings reconciliation). This ensures containers without explicit resource requests receive a non-zero scheduling footprint, which is critical for Cluster Autoscaler bin-packing accuracy on ROSA. Changes: - Static LimitRange manifest for the platform namespace (250m CPU, 256Mi memory defaultRequest; 2 CPU, 4Gi memory default limit) - ensureLimitRange() in ProjectSettings reconciler for dynamic per-project namespace creation with OwnerReference lifecycle - Extracted buildOwnerRef() helper to reduce duplication between ensureLimitRange() and ensureSessionTriggerRBAC() - RBAC: granted operator limitranges get/create permissions - Three unit tests: creation, idempotency, multi-namespace isolation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Chris Hambridge <chambrid@redhat.com>
chambridge
force-pushed
the
fix/limitrange-default-requests
branch
from
84ec8ac to
522f0b3
Compare
Gkrumbach07
approved these changes
Mar 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add LimitRange objects with defaultRequest values to both the platform namespace (static YAML manifest) and project namespaces (dynamically created by the operator during ProjectSettings reconciliation). This ensures containers without explicit resource requests receive a non-zero scheduling footprint, which is critical for Cluster Autoscaler bin-packing accuracy on ROSA.
Changes: