Skip to content

Commit 2f16cbe

Browse files
authored
fix(operations): respect explicit zero requests in vertical scaling (#10234)
1 parent 0ed0f2e commit 2f16cbe

2 files changed

Lines changed: 111 additions & 2 deletions

File tree

pkg/operations/vertical_scaling.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -200,8 +200,11 @@ func (vs verticalScalingHandler) podApplyCompOps(
200200
vsResources.Requests = corev1.ResourceList{}
201201
}
202202
for resName, resValue := range vsResources.Limits {
203-
requestResource := vsResources.Requests[resName]
204-
if requestResource.IsZero() {
203+
// Only default a request value to the matching limit when the caller
204+
// omitted the request key entirely. An explicit zero value is a valid
205+
// Pod spec and must be compared as a literal zero, not silently
206+
// promoted to the limit value.
207+
if _, ok := vsResources.Requests[resName]; !ok {
205208
vsResources.Requests[resName] = resValue
206209
}
207210
if !resValue.Equal(podResources.Limits[resName]) {

pkg/operations/vertical_scaling_test.go

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -366,3 +366,109 @@ var _ = Describe("VerticalScaling OpsRequest", func() {
366366
})
367367
})
368368
})
369+
370+
var _ = Describe("verticalScalingHandler resource match contract", func() {
371+
// These tests pin the contract that podApplyCompOps must use when comparing
372+
// a Pod's actual container resources against the target requirements declared
373+
// in a VerticalScaling spec. They distinguish two distinct intents that the
374+
// previous implementation collapsed into a single state:
375+
// 1) the caller omitted a request key entirely (defaulting it to the limit
376+
// value matches the apiserver behaviour for an absent request);
377+
// 2) the caller explicitly set the request value to zero (which is a valid
378+
// Pod spec and must be compared as a literal zero, not silently
379+
// promoted to the limit value).
380+
const (
381+
clusterCompName = "c1"
382+
podName = "pod-0"
383+
containerName = "main"
384+
)
385+
386+
vs := verticalScalingHandler{}
387+
388+
makePod := func(limits, requests corev1.ResourceList) *corev1.Pod {
389+
return &corev1.Pod{
390+
ObjectMeta: metav1.ObjectMeta{Name: podName},
391+
Spec: corev1.PodSpec{
392+
Containers: []corev1.Container{
393+
{
394+
Name: containerName,
395+
Resources: corev1.ResourceRequirements{
396+
Limits: limits,
397+
Requests: requests,
398+
},
399+
},
400+
},
401+
},
402+
}
403+
}
404+
405+
makeInstance := func(pod *corev1.Pod) Instance {
406+
return &defaultInstance{name: podName, componentName: clusterCompName, pod: pod}
407+
}
408+
409+
makePgRes := func(target corev1.ResourceRequirements) *progressResource {
410+
return &progressResource{
411+
updatedPodSet: map[string]string{podName: constant.EmptyInsTemplateName},
412+
clusterComponent: &appsv1.ClusterComponentSpec{Name: clusterCompName},
413+
compOps: opsv1alpha1.VerticalScaling{
414+
ComponentOps: opsv1alpha1.ComponentOps{ComponentName: clusterCompName},
415+
ResourceRequirements: target,
416+
},
417+
}
418+
}
419+
420+
ops := &opsv1alpha1.OpsRequest{}
421+
422+
It("treats an explicit requests=0 with limits>0 as a match when the Pod actual has the same explicit zero request", func() {
423+
target := corev1.ResourceRequirements{
424+
Limits: corev1.ResourceList{
425+
corev1.ResourceCPU: resource.MustParse("1"),
426+
corev1.ResourceMemory: resource.MustParse("2Gi"),
427+
},
428+
Requests: corev1.ResourceList{
429+
corev1.ResourceCPU: resource.MustParse("0"),
430+
corev1.ResourceMemory: resource.MustParse("2Gi"),
431+
},
432+
}
433+
pod := makePod(target.Limits.DeepCopy(), target.Requests.DeepCopy())
434+
Expect(vs.podApplyCompOps(ops, makeInstance(pod), makePgRes(target))).Should(BeTrue())
435+
})
436+
437+
It("defaults an absent request key to its limit value when comparing", func() {
438+
target := corev1.ResourceRequirements{
439+
Limits: corev1.ResourceList{
440+
corev1.ResourceCPU: resource.MustParse("1"),
441+
corev1.ResourceMemory: resource.MustParse("2Gi"),
442+
},
443+
Requests: corev1.ResourceList{
444+
// cpu key is intentionally absent here.
445+
corev1.ResourceMemory: resource.MustParse("2Gi"),
446+
},
447+
}
448+
podRequests := corev1.ResourceList{
449+
corev1.ResourceCPU: resource.MustParse("1"),
450+
corev1.ResourceMemory: resource.MustParse("2Gi"),
451+
}
452+
pod := makePod(target.Limits.DeepCopy(), podRequests)
453+
Expect(vs.podApplyCompOps(ops, makeInstance(pod), makePgRes(target))).Should(BeTrue())
454+
})
455+
456+
It("returns false when the Pod's actual requests differ from the target", func() {
457+
target := corev1.ResourceRequirements{
458+
Limits: corev1.ResourceList{
459+
corev1.ResourceCPU: resource.MustParse("1"),
460+
corev1.ResourceMemory: resource.MustParse("2Gi"),
461+
},
462+
Requests: corev1.ResourceList{
463+
corev1.ResourceCPU: resource.MustParse("500m"),
464+
corev1.ResourceMemory: resource.MustParse("2Gi"),
465+
},
466+
}
467+
podRequests := corev1.ResourceList{
468+
corev1.ResourceCPU: resource.MustParse("300m"),
469+
corev1.ResourceMemory: resource.MustParse("2Gi"),
470+
}
471+
pod := makePod(target.Limits.DeepCopy(), podRequests)
472+
Expect(vs.podApplyCompOps(ops, makeInstance(pod), makePgRes(target))).Should(BeFalse())
473+
})
474+
})

0 commit comments

Comments
 (0)