Skip to content

Commit 91da9c4

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#3463 from jbartosik/fix-limit-rounding-master
Fix limit rounding
2 parents 8d76bb3 + 825ec2c commit 91da9c4

File tree

3 files changed

+158
-14
lines changed

3 files changed

+158
-14
lines changed

‎vertical-pod-autoscaler/pkg/utils/vpa/capping.go‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -335,11 +335,11 @@ func getBoundaryRecommendation(recommendation apiv1.ResourceList, container apiv
335335
if boundaryLimit == nil {
336336
return apiv1.ResourceList{}
337337
}
338-
cpuMaxRequest := GetBoundaryRequest(container.Resources.Requests.Cpu(), container.Resources.Limits.Cpu(), boundaryLimit.Cpu(), defaultLimit.Cpu())
339-
memMaxRequest := GetBoundaryRequest(container.Resources.Requests.Memory(), container.Resources.Limits.Memory(), boundaryLimit.Memory(), defaultLimit.Memory())
338+
boundaryCpu := GetBoundaryRequest(container.Resources.Requests.Cpu(), container.Resources.Limits.Cpu(), boundaryLimit.Cpu(), defaultLimit.Cpu())
339+
boundaryMem := GetBoundaryRequest(container.Resources.Requests.Memory(), container.Resources.Limits.Memory(), boundaryLimit.Memory(), defaultLimit.Memory())
340340
return apiv1.ResourceList{
341-
apiv1.ResourceCPU: *cpuMaxRequest,
342-
apiv1.ResourceMemory: *memMaxRequest,
341+
apiv1.ResourceCPU: *boundaryCpu,
342+
apiv1.ResourceMemory: *boundaryMem,
343343
}
344344
}
345345

@@ -371,7 +371,12 @@ func applyPodLimitRange(resources []vpa_types.RecommendedContainerResources,
371371
if minLimit.Cmp(sumRecommendation) > 0 && !sumLimit.IsZero() {
372372
for i := range pod.Spec.Containers {
373373
request := (*fieldGetter(resources[i]))[resourceName]
374-
cappedContainerRequest, _ := scaleQuantityProportionally(&request, &sumRecommendation, &minLimit)
374+
var cappedContainerRequest *resource.Quantity
375+
if resourceName == apiv1.ResourceMemory {
376+
cappedContainerRequest, _ = scaleQuantityProportionally(&request, &sumRecommendation, &minLimit, roundUpToFullUnit)
377+
} else {
378+
cappedContainerRequest, _ = scaleQuantityProportionally(&request, &sumRecommendation, &minLimit, noRounding)
379+
}
375380
(*fieldGetter(resources[i]))[resourceName] = *cappedContainerRequest
376381
}
377382
return resources
@@ -390,7 +395,12 @@ func applyPodLimitRange(resources []vpa_types.RecommendedContainerResources,
390395
}
391396
for i := range pod.Spec.Containers {
392397
limit := (*fieldGetter(resources[i]))[resourceName]
393-
cappedContainerRequest, _ := scaleQuantityProportionally(&limit, &sumLimit, &targetTotalLimit)
398+
var cappedContainerRequest *resource.Quantity
399+
if resourceName == apiv1.ResourceMemory {
400+
cappedContainerRequest, _ = scaleQuantityProportionally(&limit, &sumLimit, &targetTotalLimit, roundDownToFullUnit)
401+
} else {
402+
cappedContainerRequest, _ = scaleQuantityProportionally(&limit, &sumLimit, &targetTotalLimit, noRounding)
403+
}
394404
(*fieldGetter(resources[i]))[resourceName] = *cappedContainerRequest
395405
}
396406
return resources

‎vertical-pod-autoscaler/pkg/utils/vpa/capping_test.go‎

Lines changed: 122 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -577,13 +577,13 @@ func TestApplyPodLimitRange(t *testing.T) {
577577
{
578578
ContainerName: "container1",
579579
Target: apiv1.ResourceList{
580-
apiv1.ResourceMemory: resource.MustParse("2000000000000m"),
580+
apiv1.ResourceMemory: resource.MustParse("2000000000"),
581581
},
582582
},
583583
{
584584
ContainerName: "container2",
585585
Target: apiv1.ResourceList{
586-
apiv1.ResourceMemory: resource.MustParse("2000000000000m"),
586+
apiv1.ResourceMemory: resource.MustParse("2000000000"),
587587
},
588588
},
589589
},
@@ -644,13 +644,13 @@ func TestApplyPodLimitRange(t *testing.T) {
644644
{
645645
ContainerName: "container1",
646646
Target: apiv1.ResourceList{
647-
apiv1.ResourceMemory: resource.MustParse("2000000000000m"),
647+
apiv1.ResourceMemory: resource.MustParse("2000000000"),
648648
},
649649
},
650650
{
651651
ContainerName: "container2",
652652
Target: apiv1.ResourceList{
653-
apiv1.ResourceMemory: resource.MustParse("2000000000000m"),
653+
apiv1.ResourceMemory: resource.MustParse("2000000000"),
654654
},
655655
},
656656
},
@@ -801,3 +801,121 @@ func TestApplyLimitRangeMinToRequest(t *testing.T) {
801801
})
802802
}
803803
}
804+
805+
func TestCapPodMemoryWithUnderByteSplit(t *testing.T) {
806+
// It's important that recommendations are different so straightforwardly
807+
// splitting Max between containers results in fractional bytes going to them.
808+
// We want to ensure we'll round millibytes the right way (rounding down when
809+
// caping to max, to stay below max, rounding up when caping to min).
810+
recommendation := vpa_types.RecommendedPodResources{
811+
ContainerRecommendations: []vpa_types.RecommendedContainerResources{
812+
{
813+
ContainerName: "container-1",
814+
Target: apiv1.ResourceList{
815+
apiv1.ResourceMemory: resource.MustParse("1Gi"),
816+
},
817+
},
818+
{
819+
ContainerName: "container-2",
820+
Target: apiv1.ResourceList{
821+
apiv1.ResourceMemory: resource.MustParse("2Gi"),
822+
},
823+
},
824+
},
825+
}
826+
pod := apiv1.Pod{
827+
Spec: apiv1.PodSpec{
828+
Containers: []apiv1.Container{
829+
{
830+
Name: "container-1",
831+
Resources: apiv1.ResourceRequirements{
832+
Requests: apiv1.ResourceList{
833+
apiv1.ResourceMemory: resource.MustParse("50M"),
834+
},
835+
Limits: apiv1.ResourceList{
836+
apiv1.ResourceMemory: resource.MustParse("50M"),
837+
},
838+
},
839+
},
840+
{
841+
Name: "container-2",
842+
Resources: apiv1.ResourceRequirements{
843+
Requests: apiv1.ResourceList{
844+
apiv1.ResourceMemory: resource.MustParse("50M"),
845+
},
846+
Limits: apiv1.ResourceList{
847+
apiv1.ResourceMemory: resource.MustParse("50M"),
848+
},
849+
},
850+
},
851+
},
852+
},
853+
}
854+
855+
tests := []struct {
856+
name string
857+
limitRange apiv1.LimitRangeItem
858+
expectedRecommendation vpa_types.RecommendedPodResources
859+
}{
860+
{
861+
name: "cap to max",
862+
limitRange: apiv1.LimitRangeItem{
863+
Type: apiv1.LimitTypePod,
864+
Max: apiv1.ResourceList{
865+
apiv1.ResourceMemory: resource.MustParse("1Gi"),
866+
},
867+
},
868+
expectedRecommendation: vpa_types.RecommendedPodResources{
869+
ContainerRecommendations: []vpa_types.RecommendedContainerResources{
870+
{
871+
ContainerName: "container-1",
872+
Target: apiv1.ResourceList{
873+
apiv1.ResourceMemory: *resource.NewQuantity(357913941, resource.BinarySI),
874+
},
875+
},
876+
{
877+
ContainerName: "container-2",
878+
Target: apiv1.ResourceList{
879+
apiv1.ResourceMemory: *resource.NewQuantity(715827882, resource.BinarySI),
880+
},
881+
},
882+
},
883+
},
884+
},
885+
{
886+
name: "cap to min",
887+
limitRange: apiv1.LimitRangeItem{
888+
Type: apiv1.LimitTypePod,
889+
Min: apiv1.ResourceList{
890+
apiv1.ResourceMemory: resource.MustParse("4Gi"),
891+
},
892+
},
893+
expectedRecommendation: vpa_types.RecommendedPodResources{
894+
ContainerRecommendations: []vpa_types.RecommendedContainerResources{
895+
{
896+
ContainerName: "container-1",
897+
Target: apiv1.ResourceList{
898+
apiv1.ResourceMemory: *resource.NewQuantity(1431655766, resource.BinarySI),
899+
},
900+
},
901+
{
902+
ContainerName: "container-2",
903+
Target: apiv1.ResourceList{
904+
apiv1.ResourceMemory: *resource.NewQuantity(2863311531, resource.BinarySI),
905+
},
906+
},
907+
},
908+
},
909+
},
910+
}
911+
912+
for _, tc := range tests {
913+
t.Run(tc.name, func(t *testing.T) {
914+
calculator := fakeLimitRangeCalculator{podLimitRange: tc.limitRange}
915+
processor := NewCappingRecommendationProcessor(&calculator)
916+
processedRecommendation, _, err := processor.Apply(&recommendation, nil, nil, &pod)
917+
assert.NoError(t, err)
918+
assert.Equal(t, tc.expectedRecommendation, *processedRecommendation)
919+
})
920+
}
921+
}

‎vertical-pod-autoscaler/pkg/utils/vpa/limit_and_request_scaling.go‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ func getProportionalResourceLimit(resourceName core.ResourceName, originalLimit,
8181
result := *recommendedRequest
8282
return &result, ""
8383
}
84-
result, capped := scaleQuantityProportionally( /*scaledQuantity=*/ originalLimit /*scaleBase=*/, originalRequest /*scaleResult=*/, recommendedRequest)
84+
result, capped := scaleQuantityProportionally( /*scaledQuantity=*/ originalLimit /*scaleBase=*/, originalRequest /*scaleResult=*/, recommendedRequest, noRounding)
8585
if !capped {
8686
return result, ""
8787
}
@@ -103,21 +103,37 @@ func GetBoundaryRequest(originalRequest, originalLimit, boundaryLimit, defaultLi
103103
if originalRequest == nil || originalRequest.Value() == 0 {
104104
return boundaryLimit
105105
}
106-
result, _ := scaleQuantityProportionally(originalRequest /* scaledQuantity */, originalLimit /*scaleBase*/, boundaryLimit /*scaleResult*/)
106+
result, _ := scaleQuantityProportionally(originalRequest /* scaledQuantity */, originalLimit /*scaleBase*/, boundaryLimit /*scaleResult*/, noRounding)
107107
return result
108108
}
109109

110+
type roundingMode int
111+
112+
const (
113+
noRounding roundingMode = iota
114+
roundUpToFullUnit
115+
roundDownToFullUnit
116+
)
117+
110118
// scaleQuantityProportionally returns value which has the same proportion to scaledQuantity as scaleResult has to scaleBase
111119
// It also returns a bool indicating if it had to cap result to MaxInt64 milliunits.
112-
func scaleQuantityProportionally(scaledQuantity, scaleBase, scaleResult *resource.Quantity) (*resource.Quantity, bool) {
120+
func scaleQuantityProportionally(scaledQuantity, scaleBase, scaleResult *resource.Quantity, rounding roundingMode) (*resource.Quantity, bool) {
113121
originalMilli := big.NewInt(scaledQuantity.MilliValue())
114122
scaleBaseMilli := big.NewInt(scaleBase.MilliValue())
115123
scaleResultMilli := big.NewInt(scaleResult.MilliValue())
116124
var scaledOriginal big.Int
117125
scaledOriginal.Mul(originalMilli, scaleResultMilli)
118126
scaledOriginal.Div(&scaledOriginal, scaleBaseMilli)
119127
if scaledOriginal.IsInt64() {
120-
return resource.NewMilliQuantity(scaledOriginal.Int64(), scaledQuantity.Format), false
128+
result := resource.NewMilliQuantity(scaledOriginal.Int64(), scaledQuantity.Format)
129+
if rounding == roundUpToFullUnit {
130+
result.RoundUp(resource.Scale(0))
131+
}
132+
if rounding == roundDownToFullUnit {
133+
result.Sub(*resource.NewMilliQuantity(999, result.Format))
134+
result.RoundUp(resource.Scale(0))
135+
}
136+
return result, false
121137
}
122138
return resource.NewMilliQuantity(math.MaxInt64, scaledQuantity.Format), true
123139
}

0 commit comments

Comments
 (0)