Skip to content

Commit abaff59

Browse files
weicaoweicao
committed
fix: cap preserved request to limit during limit-only scale-down
When the desired template omits CPU/memory requests and the limit is scaled down, mergeInPlaceFields preserved the live request (defaulted from the old higher limit). This produced request > limit in the resize patch, which Kubernetes rejects. Cap the preserved request to not exceed the new limit when the desired template omits it. Addresses Leon P2 review comment on #10362.
1 parent d5af53f commit abaff59

4 files changed

Lines changed: 202 additions & 0 deletions

File tree

‎pkg/controller/instance/in_place_update_utils.go‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,13 +142,38 @@ func mergeInPlaceFields(src, dst *corev1.Pod) {
142142
requests, limits := copyRequestsNLimitsFields(&container)
143143
mergeResources(&requests, &dst.Spec.Containers[i].Resources.Requests)
144144
mergeResources(&limits, &dst.Spec.Containers[i].Resources.Limits)
145+
capOmittedRequestsToLimits(&container, &dst.Spec.Containers[i])
145146
}
146147
break
147148
}
148149
}
149150
}
150151
}
151152

153+
// capOmittedRequestsToLimits ensures that when the desired template omits a
154+
// CPU or memory request, the preserved live request does not exceed the new
155+
// limit. Kubernetes defaults omitted requests to limits, so after a
156+
// limit-only scale-down the live request (defaulted from the old higher
157+
// limit) would violate the request ≤ limit invariant and be rejected.
158+
func capOmittedRequestsToLimits(desired, merged *corev1.Container) {
159+
for _, rn := range []corev1.ResourceName{corev1.ResourceCPU, corev1.ResourceMemory} {
160+
if _, specified := desired.Resources.Requests[rn]; specified {
161+
continue
162+
}
163+
limit, hasLimit := merged.Resources.Limits[rn]
164+
if !hasLimit {
165+
continue
166+
}
167+
req, hasReq := merged.Resources.Requests[rn]
168+
if !hasReq {
169+
continue
170+
}
171+
if req.Cmp(limit) > 0 {
172+
merged.Resources.Requests[rn] = limit.DeepCopy()
173+
}
174+
}
175+
}
176+
152177
func equalField(old, new any) bool {
153178
oType := reflect.TypeOf(old)
154179
nType := reflect.TypeOf(new)

‎pkg/controller/instance/utils_test.go‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -719,6 +719,92 @@ func TestUpdateReconcilerConvergesResizeAfterConfigMetadataWithOmittedRequest(t
719719
}
720720
}
721721

722+
func TestLimitOnlyScaleDownConvergesWithOmittedRequest(t *testing.T) {
723+
oldFeatureGate := viper.GetBool(constant.FeatureGateInPlacePodVerticalScaling)
724+
defer viper.Set(constant.FeatureGateInPlacePodVerticalScaling, oldFeatureGate)
725+
viper.Set(constant.FeatureGateInPlacePodVerticalScaling, true)
726+
727+
origSupportResize := intctrlutil.SupportResizeSubResource
728+
intctrlutil.SupportResizeSubResource = func() (bool, error) { return true, nil }
729+
defer func() { intctrlutil.SupportResizeSubResource = origSupportResize }()
730+
731+
spy := &instanceLifecycleCallSpy{}
732+
origNewLifecycleAction := newLifecycleAction
733+
newLifecycleAction = func(_ *workloads.Instance, _ []*corev1.Pod, _ *corev1.Pod) (lifecycle.Lifecycle, error) {
734+
return spy, nil
735+
}
736+
defer func() { newLifecycleAction = origNewLifecycleAction }()
737+
738+
// Start with limits=1, requests omitted (K8s defaults requests=1)
739+
inst := builder.NewInstanceBuilder("default", "valkey-0").
740+
SetPodUpdatePolicy(kbappsv1.PreferInPlacePodUpdatePolicyType).
741+
SetLifecycleActions(&workloads.LifecycleActions{
742+
Switchover: &kbappsv1.Action{
743+
Exec: &kbappsv1.ExecAction{Command: []string{"true"}},
744+
},
745+
}).
746+
AddContainer(corev1.Container{
747+
Name: "valkey",
748+
Image: "valkey:8",
749+
Resources: corev1.ResourceRequirements{
750+
Limits: corev1.ResourceList{corev1.ResourceCPU: resource.MustParse("1")},
751+
},
752+
}).
753+
GetObject()
754+
755+
revision, err := buildInstancePodRevision(&inst.Spec.Template, inst)
756+
if err != nil {
757+
t.Fatalf("buildInstancePodRevision() error = %v", err)
758+
}
759+
inst.Status.UpdateRevision = revision
760+
761+
pod, err := buildInstancePod(inst, revision)
762+
if err != nil {
763+
t.Fatalf("buildInstancePod() error = %v", err)
764+
}
765+
pod.Status.Phase = corev1.PodRunning
766+
pod.Status.Conditions = append(pod.Status.Conditions, corev1.PodCondition{
767+
Type: corev1.PodReady,
768+
Status: corev1.ConditionTrue,
769+
LastTransitionTime: metav1.NewTime(time.Now().Add(-10 * time.Second)),
770+
})
771+
// Simulate K8s defaulting: live pod has requests = old limits
772+
pod.Spec.Containers[0].Resources = corev1.ResourceRequirements{
773+
Limits: corev1.ResourceList{corev1.ResourceCPU: resource.MustParse("1")},
774+
Requests: corev1.ResourceList{corev1.ResourceCPU: resource.MustParse("1")},
775+
}
776+
777+
// Scale down: desired limits=500m, requests omitted
778+
inst.Spec.Template.Spec.Containers[0].Resources = corev1.ResourceRequirements{
779+
Limits: corev1.ResourceList{corev1.ResourceCPU: resource.MustParse("500m")},
780+
}
781+
782+
tree := kubebuilderx.NewObjectTree()
783+
tree.SetRoot(inst)
784+
if err = tree.Add(pod); err != nil {
785+
t.Fatalf("tree.Add() error = %v", err)
786+
}
787+
788+
res, err := NewUpdateReconciler().Reconcile(tree)
789+
if err != nil {
790+
t.Fatalf("Reconcile() error = %v", err)
791+
}
792+
if res != kubebuilderx.Continue {
793+
t.Fatalf("expected Continue, got %v", res)
794+
}
795+
796+
pods := tree.List(&corev1.Pod{})
797+
if len(pods) != 1 {
798+
t.Fatalf("expected one pod, got %d", len(pods))
799+
}
800+
updatedPod := pods[0].(*corev1.Pod)
801+
limit := updatedPod.Spec.Containers[0].Resources.Limits[corev1.ResourceCPU]
802+
req := updatedPod.Spec.Containers[0].Resources.Requests[corev1.ResourceCPU]
803+
if req.Cmp(limit) > 0 {
804+
t.Fatalf("preserved request must not exceed new limit after scale-down; got request=%s limit=%s", req.String(), limit.String())
805+
}
806+
}
807+
722808
type instanceLifecycleCallSpy struct {
723809
switchoverCalls int
724810
reconfigureCalls int

‎pkg/controller/instanceset/in_place_update_util.go‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,13 +151,38 @@ func mergeInPlaceFields(src, dst *corev1.Pod) {
151151
requests, limits := copyRequestsNLimitsFields(&container)
152152
mergeResources(&requests, &dst.Spec.Containers[i].Resources.Requests)
153153
mergeResources(&limits, &dst.Spec.Containers[i].Resources.Limits)
154+
capOmittedRequestsToLimits(&container, &dst.Spec.Containers[i])
154155
}
155156
break
156157
}
157158
}
158159
}
159160
}
160161

162+
// capOmittedRequestsToLimits ensures that when the desired template omits a
163+
// CPU or memory request, the preserved live request does not exceed the new
164+
// limit. Kubernetes defaults omitted requests to limits, so after a
165+
// limit-only scale-down the live request (defaulted from the old higher
166+
// limit) would violate the request ≤ limit invariant and be rejected.
167+
func capOmittedRequestsToLimits(desired, merged *corev1.Container) {
168+
for _, rn := range []corev1.ResourceName{corev1.ResourceCPU, corev1.ResourceMemory} {
169+
if _, specified := desired.Resources.Requests[rn]; specified {
170+
continue
171+
}
172+
limit, hasLimit := merged.Resources.Limits[rn]
173+
if !hasLimit {
174+
continue
175+
}
176+
req, hasReq := merged.Resources.Requests[rn]
177+
if !hasReq {
178+
continue
179+
}
180+
if req.Cmp(limit) > 0 {
181+
merged.Resources.Requests[rn] = limit.DeepCopy()
182+
}
183+
}
184+
}
185+
161186
func equalField(old, new any) bool {
162187
oType := reflect.TypeOf(old)
163188
nType := reflect.TypeOf(new)

‎pkg/controller/instanceset/reconciler_update_test.go‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -533,6 +533,72 @@ var _ = Describe("update reconciler test", func() {
533533
"after resize updates the desired limit, the omitted desired request must not keep resourceUpdate true")
534534
})
535535

536+
It("converges limit-only scale-down when desired request is omitted", func() {
537+
oldFeatureGate := viper.GetBool(constant.FeatureGateInPlacePodVerticalScaling)
538+
defer viper.Set(constant.FeatureGateInPlacePodVerticalScaling, oldFeatureGate)
539+
viper.Set(constant.FeatureGateInPlacePodVerticalScaling, true)
540+
541+
origSupportResize := intctrlutil.SupportResizeSubResource
542+
intctrlutil.SupportResizeSubResource = func() (bool, error) { return true, nil }
543+
defer func() { intctrlutil.SupportResizeSubResource = origSupportResize }()
544+
545+
spy := &lifecycleCallSpy{}
546+
origNewLifecycleAction := newLifecycleAction
547+
newLifecycleAction = func(_ *workloads.InstanceSet, _ *kubebuilderx.ObjectTree, _ *corev1.Pod) (lifecycle.Lifecycle, error) {
548+
return spy, nil
549+
}
550+
defer func() { newLifecycleAction = origNewLifecycleAction }()
551+
552+
tree := kubebuilderx.NewObjectTree()
553+
its.Spec.PodManagementPolicy = appsv1.ParallelPodManagement
554+
its.Spec.Replicas = ptr.To[int32](1)
555+
its.Spec.PodUpdatePolicy = kbappsv1.PreferInPlacePodUpdatePolicyType
556+
its.Spec.LifecycleActions = &workloads.LifecycleActions{
557+
Switchover: &kbappsv1.Action{
558+
Exec: &kbappsv1.ExecAction{Command: []string{"true"}},
559+
},
560+
}
561+
// Live pod: limits=1, requests=1 (K8s defaulted requests to limits)
562+
its.Spec.Template.Spec.Containers[0].Resources = corev1.ResourceRequirements{
563+
Limits: corev1.ResourceList{corev1.ResourceCPU: resource.MustParse("1")},
564+
}
565+
tree.SetRoot(its)
566+
prepareForUpdate(tree)
567+
568+
pods := tree.List(&corev1.Pod{})
569+
Expect(pods).Should(HaveLen(1))
570+
pod := pods[0].(*corev1.Pod)
571+
pod.Status.Phase = corev1.PodRunning
572+
pod.Status.Conditions = append(pod.Status.Conditions, corev1.PodCondition{
573+
Type: corev1.PodReady,
574+
Status: corev1.ConditionTrue,
575+
LastTransitionTime: metav1.NewTime(time.Now().Add(-1 * minReadySeconds * time.Second)),
576+
})
577+
// Simulate K8s defaulting: live pod has requests = old limits
578+
pod.Spec.Containers[0].Resources = corev1.ResourceRequirements{
579+
Limits: corev1.ResourceList{corev1.ResourceCPU: resource.MustParse("1")},
580+
Requests: corev1.ResourceList{corev1.ResourceCPU: resource.MustParse("1")},
581+
}
582+
583+
// Scale down: new desired limits=500m, requests omitted
584+
its.Spec.Template.Spec.Containers[0].Resources = corev1.ResourceRequirements{
585+
Limits: corev1.ResourceList{corev1.ResourceCPU: resource.MustParse("500m")},
586+
}
587+
588+
reconciler = NewUpdateReconciler()
589+
res, err := reconciler.Reconcile(tree)
590+
Expect(err).Should(BeNil())
591+
Expect(res).Should(Equal(kubebuilderx.Continue))
592+
593+
postPods := tree.List(&corev1.Pod{})
594+
Expect(postPods).Should(HaveLen(1))
595+
updatedPod := postPods[0].(*corev1.Pod)
596+
Expect(updatedPod.Spec.Containers[0].Resources.Limits[corev1.ResourceCPU]).Should(Equal(resource.MustParse("500m")))
597+
req := updatedPod.Spec.Containers[0].Resources.Requests[corev1.ResourceCPU]
598+
Expect(req.Cmp(updatedPod.Spec.Containers[0].Resources.Limits[corev1.ResourceCPU])).Should(BeNumerically("<=", 0),
599+
"preserved request must not exceed new limit after scale-down; got request=%s limit=%s", req.String(), "500m")
600+
})
601+
536602
It("patches pod without calling switchover for metadata-only in-place updates", func() {
537603
// This test exercises the Reconcile call site (not just the
538604
// safeMetadataOnlyInPlaceUpdate helper) to assert the contract

0 commit comments

Comments
 (0)