OCPBUGS-86238: set limits for aro.openshift.io/swift-nic in request overrides for ARO swift#8552
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
the
jira/valid-reference
|
@enxebre: This pull request references Jira Issue OCPBUGS-86238, which is valid. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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. |
openshift-ci-robot
added
the
jira/valid-bug
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR enforces limits == requests for the aro.openshift.io/swift-nic extended resource by adding applyNonOvercommitableResourceLimits and calling it from applyRequestsOverrides after copying request overrides into init and regular containers. Tests and an import were added to verify request override behavior and that limits are set only for the non-overcommittable Swift-NIC resource while standard resources remain unchanged. 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/control-plane-operator
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre 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-ci
Bot
added
the
approved
enxebre
force-pushed
the
swift-nic-resource-limit-override
branch
from
b5cc6f6 to
98234f8
Compare
enxebre
changed the title
enxebre
force-pushed
the
swift-nic-resource-limit-override
branch
from
98234f8 to
a418b82
Compare
enxebre
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
support/controlplane-component/defaults_test.go (1)
289-521: ⚡ Quick winEnable parallel execution in the new unit tests.
Please add
t.Parallel()inTestApplyRequestsOverridesandTestApplyNonOvercommitableResourceLimits(and inside eacht.Run) to align with test execution guidance and keep CI time down.Proposed diff
func TestApplyRequestsOverrides(t *testing.T) { + t.Parallel() tests := []struct { @@ for _, test := range tests { t.Run(test.name, func(t *testing.T) { + t.Parallel() g := NewGomegaWithT(t) @@ func TestApplyNonOvercommitableResourceLimits(t *testing.T) { + t.Parallel() tests := []struct { @@ for _, test := range tests { t.Run(test.name, func(t *testing.T) { + t.Parallel() g := NewGomegaWithT(t)As per coding guidelines, "Use race detection and parallel execution for unit tests".
🤖 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 `@support/controlplane-component/defaults_test.go` around lines 289 - 521, Tests TestApplyRequestsOverrides and TestApplyNonOvercommitableResourceLimits are not marked for parallel execution; add t.Parallel() at the start of each top-level test function (TestApplyRequestsOverrides and TestApplyNonOvercommitableResourceLimits) and inside each subtest closure (the func passed to t.Run) so each subtest runs in parallel; ensure t.Parallel() is the first statement in those functions to follow Go testing guidance and avoid shared-state races when calling workload.applyRequestsOverrides and applyNonOvercommitableResourceLimits.
🤖 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.
Nitpick comments:
In `@support/controlplane-component/defaults_test.go`:
- Around line 289-521: Tests TestApplyRequestsOverrides and
TestApplyNonOvercommitableResourceLimits are not marked for parallel execution;
add t.Parallel() at the start of each top-level test function
(TestApplyRequestsOverrides and TestApplyNonOvercommitableResourceLimits) and
inside each subtest closure (the func passed to t.Run) so each subtest runs in
parallel; ensure t.Parallel() is the first statement in those functions to
follow Go testing guidance and avoid shared-state races when calling
workload.applyRequestsOverrides and applyNonOvercommitableResourceLimits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0fb84255-e279-425e-b1db-51a82c56fa4e
📒 Files selected for processing (2)
support/controlplane-component/defaults.gosupport/controlplane-component/defaults_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
support/controlplane-component/defaults_test.go (1)
289-467: ⚡ Quick winStrong test coverage for the override logic.
The test cases comprehensively verify standard resources, extended resources, mixed scenarios, init containers, and deployment name filtering.
Consider adding an edge case test after fixing the nil
Resources.Requestsissue flagged in defaults.go: verify that overrides work correctly when a container has no pre-existing resource requirements (nilResources.Requestsmap). This would prevent regression of the nil-map panic.🤖 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 `@support/controlplane-component/defaults_test.go` around lines 289 - 467, Add a new subtest in TestApplyRequestsOverrides that covers the edge case where a container has Resources set but Resources.Requests is nil (not an empty map); call workload.applyRequestsOverrides with an annotation like "resource-request-override.hypershift.openshift.io/router.router": "cpu=250m" and assert the container ends up with Requests populated accordingly. Ensure the test targets the same workload.applyRequestsOverrides method and validates both Containers and InitContainers variants as needed so we catch the nil-map panic previously observed in defaults.go.
🤖 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.
Inline comments:
In `@support/controlplane-component/defaults.go`:
- Around line 427-439: The loop copies into container resource request maps that
may be nil causing a panic; before calling maps.Copy in both the InitContainers
and Containers loops, ensure
podTemplate.Spec.InitContainers[i].Resources.Requests and
podTemplate.Spec.Containers[i].Resources.Requests are non-nil (initialize to an
empty v1.ResourceList if nil) and then call maps.Copy and
applyNonOvercommitableResourceLimits(&podTemplate.Spec.InitContainers[i], res) /
applyNonOvercommitableResourceLimits(&podTemplate.Spec.Containers[i], res) as
currently done.
---
Nitpick comments:
In `@support/controlplane-component/defaults_test.go`:
- Around line 289-467: Add a new subtest in TestApplyRequestsOverrides that
covers the edge case where a container has Resources set but Resources.Requests
is nil (not an empty map); call workload.applyRequestsOverrides with an
annotation like
"resource-request-override.hypershift.openshift.io/router.router": "cpu=250m"
and assert the container ends up with Requests populated accordingly. Ensure the
test targets the same workload.applyRequestsOverrides method and validates both
Containers and InitContainers variants as needed so we catch the nil-map panic
previously observed in defaults.go.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0b1eb6b6-fe8d-4391-90b9-ccd1620561ab
📒 Files selected for processing (2)
support/controlplane-component/defaults.gosupport/controlplane-component/defaults_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8552 +/- ##
==========================================
+ Coverage 40.34% 40.37% +0.02%
==========================================
Files 755 755
Lines 93167 93175 +8
==========================================
+ Hits 37587 37618 +31
+ Misses 52877 52856 -21
+ Partials 2703 2701 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| } | ||
| } | ||
|
|
||
| func TestApplyNonOvercommitableResourceLimits(t *testing.T) { |
There was a problem hiding this comment.
isn't this already covered in TestApplyRequestsOverrides
There was a problem hiding this comment.
yes, I still want to cover this function in isolation as a consumable piece of code.
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Dropped some minor nits. Otherwise lgtm
| // The API server requires limits == requests for these resources. | ||
| // https://github.com/kubernetes/kubernetes/blob/621e250502ddeeab8274836e88b506c0c4f57232/pkg/apis/core/validation/validation.go#L7975-L7976 | ||
| func applyNonOvercommitableResourceLimits(container *corev1.Container, overrides corev1.ResourceList) { | ||
| for resourceName, quantity := range overrides { |
There was a problem hiding this comment.
Nit: if we keep the single-resource approach, a direct map lookup is cleaner than iterating the whole map:
if quantity, ok := overrides[aroSwiftNICResource]; ok {
if container.Resources.Limits == nil {
container.Resources.Limits = corev1.ResourceList{}
}
container.Resources.Limits[aroSwiftNICResource] = quantity
}This becomes moot if we go with a generic isExtendedResource approach (where the loop is the right pattern).
| }{ | ||
| { | ||
| name: "When overriding cpu and memory it should only update requests", | ||
| annotations: map[string]string{ |
There was a problem hiding this comment.
Minor: these annotation keys are hardcoded as string literals, but the production code uses hyperv1.ResourceRequestOverrideAnnotationPrefix. Using the constant here would prevent drift if the prefix ever changes:
hyperv1.ResourceRequestOverrideAnnotationPrefix + "/router.router": "cpu=500m,memory=1Gi",There was a problem hiding this comment.
this is intentionally using the literal, I want this tests to break if the value of the constant is changed, to ensure that is a conscious action.
…c in request overrides The resource-request-override annotation only sets requests, but extended resources like aro.openshift.io/swift-nic require limits equal to requests. The API server rejects pods where an extended resource has a request without a matching limit. When the override includes aro.openshift.io/swift-nic, automatically set the limit to the same value as the request. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
enxebre
force-pushed
the
swift-nic-resource-limit-override
branch
from
a418b82 to
69d9dd8
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
support/controlplane-component/defaults.go (1)
427-437:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard
Resources.Requestsbeforemaps.Copyto avoid nil-map panic.Line 429 and Line 435 can panic when
Resources.Requestsis nil. Initialize the destination map before copying in both loops.Proposed fix
for i, c := range podTemplate.Spec.InitContainers { if res, ok := requestsOverrides[c.Name]; ok { + if podTemplate.Spec.InitContainers[i].Resources.Requests == nil { + podTemplate.Spec.InitContainers[i].Resources.Requests = corev1.ResourceList{} + } maps.Copy(podTemplate.Spec.InitContainers[i].Resources.Requests, res) applyNonOvercommitableResourceLimits(&podTemplate.Spec.InitContainers[i], res) } } for i, c := range podTemplate.Spec.Containers { if res, ok := requestsOverrides[c.Name]; ok { + if podTemplate.Spec.Containers[i].Resources.Requests == nil { + podTemplate.Spec.Containers[i].Resources.Requests = corev1.ResourceList{} + } maps.Copy(podTemplate.Spec.Containers[i].Resources.Requests, res) applyNonOvercommitableResourceLimits(&podTemplate.Spec.Containers[i], res) } }#!/bin/bash # Verify potential nil-map writes in applyRequestsOverrides and whether guards exist. rg -n -C3 'maps\.Copy\(podTemplate\.Spec\.(InitContainers|Containers)\[i\]\.Resources\.Requests,\s*res\)' support/controlplane-component/defaults.go rg -n -C2 'Resources\.Requests\s*==\s*nil' support/controlplane-component/defaults.go🤖 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 `@support/controlplane-component/defaults.go` around lines 427 - 437, The loops over podTemplate.Spec.InitContainers and podTemplate.Spec.Containers call maps.Copy into podTemplate.Spec.*[i].Resources.Requests which can be nil; before calling maps.Copy, check if podTemplate.Spec.*[i].Resources.Requests == nil and initialize it to an empty corev1.ResourceList (or equivalent) so maps.Copy has a non-nil destination, then call maps.Copy(...) and applyNonOvercommitableResourceLimits(&podTemplate.Spec.*[i], res) as before to avoid nil-map panics.
🤖 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.
Duplicate comments:
In `@support/controlplane-component/defaults.go`:
- Around line 427-437: The loops over podTemplate.Spec.InitContainers and
podTemplate.Spec.Containers call maps.Copy into
podTemplate.Spec.*[i].Resources.Requests which can be nil; before calling
maps.Copy, check if podTemplate.Spec.*[i].Resources.Requests == nil and
initialize it to an empty corev1.ResourceList (or equivalent) so maps.Copy has a
non-nil destination, then call maps.Copy(...) and
applyNonOvercommitableResourceLimits(&podTemplate.Spec.*[i], res) as before to
avoid nil-map panics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 01e1928b-a800-4a11-97a4-b212b5035599
📒 Files selected for processing (2)
support/controlplane-component/defaults.gosupport/controlplane-component/defaults_test.go
|
/lgtm |
|
Scheduling tests matching the |
Test Resultse2e-aws
e2e-aks
|
…iversion.go so we can validate Swift NIC scheduling end-to-end Hypershift sets aro.openshift.io/swift-nic limits overrides (openshift/hypershift#8552) by ensuring it matches the request override value sets by CS in https://redhat.atlassian.net/browse/ARO-27209 for 4.23+ clusters. We want to have an e2e that guarantees that when the hypershift change is merged and the CS change bumped, it'll be functional.
…iversion.go so we can validate Swift NIC scheduling end-to-end Hypershift sets aro.openshift.io/swift-nic limits overrides (openshift/hypershift#8552) by ensuring it matches the request override value sets by CS in https://redhat.atlassian.net/browse/ARO-27209 for 4.23+ clusters. We want to have an e2e that guarantees that when the hypershift change is merged and the CS change bumped, it'll be functional.
|
@enxebre: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/verified by units @enxebre |
|
@enxebre: Jira Issue OCPBUGS-86238: Some pull requests linked via external trackers have merged: The following pull request, linked via external tracker, has not merged:
All associated pull requests must be merged or unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-86238 has not been moved to the MODIFIED state. 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. |
|
@enxebre: #8552 failed to apply on top of branch "release-4.20": 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 kubernetes-sigs/prow repository. |
|
@enxebre: new pull request created: #8564 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 kubernetes-sigs/prow repository. |
|
@enxebre: new pull request created: #8565 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 kubernetes-sigs/prow repository. |
…r NIC scheduling Pick the fix done in this PR openshift/hypershift#8552 and test it against the CS bump to validate that it works end to end
…r NIC scheduling Pick the fix done in this PR openshift/hypershift#8552 and test it against the CS bump to validate that it works end to end
|
Fix included in release 5.0.0-0.nightly-2026-05-21-222828 |
8 participants
Summary
resource-request-override.hypershift.openshift.io/to override extended resources likearo.openshift.io/swift-nic, the pod fails API server admission because Kubernetes requireslimits == requestsfor extended resourcesapplyNonOvercommitableResourceLimits()to automatically set limits equal to requests foraro.openshift.io/swift-nicduring request overridesJira
https://issues.redhat.com/browse/OCPBUGS-86238
Test plan
applyNonOvercommitableResourceLimits(3 cases: swift-nic only, standard resources, mixed)applyRequestsOverridesintegration (5 cases: cpu/memory, swift-nic, mixed, init containers, different deployment)make verifypasses (codespell, lint, build, tests)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Chores