Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pod limit range #2080

Merged
merged 2 commits into from
May 31, 2019
Merged

Pod limit range #2080

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9ce20d6
Add method for getting pod limit range
jbartosik May 30, 2019
8f62c6b
Capping to pod limit range
jbartosik May 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions vertical-pod-autoscaler/pkg/admission-controller/logic/recommendation_provider_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,18 @@ func mustParseResourcePointer(val string) *resource.Quantity {
}

type fakeLimitRangeCalculator struct {
limitRange *apiv1.LimitRangeItem
err error
containerLimitRange *apiv1.LimitRangeItem
containerErr error
podLimitRange *apiv1.LimitRangeItem
podErr error
}

func (nlrc *fakeLimitRangeCalculator) GetContainerLimitRangeItem(namespace string) (*apiv1.LimitRangeItem, error) {
return nlrc.limitRange, nlrc.err
return nlrc.containerLimitRange, nlrc.containerErr
}

func (nlrc *fakeLimitRangeCalculator) GetPodLimitRangeItem(namespace string) (*apiv1.LimitRangeItem, error) {
return nlrc.podLimitRange, nlrc.podErr
}

func TestUpdateResourceRequests(t *testing.T) {
Expand Down Expand Up @@ -315,8 +321,8 @@ func TestUpdateResourceRequests(t *testing.T) {
recommendationProcessor: vpa_api_util.NewCappingRecommendationProcessor(limitrange.NewNoopLimitsCalculator()),
selectorFetcher: mockSelectorFetcher,
limitsRangeCalculator: &fakeLimitRangeCalculator{
tc.limitRange,
tc.limitRangeCalcErr,
containerLimitRange: tc.limitRange,
containerErr: tc.limitRangeCalcErr,
},
}

Expand Down
18 changes: 16 additions & 2 deletions vertical-pod-autoscaler/pkg/utils/limitrange/limit_range_calculator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ import (
type LimitRangeCalculator interface {
// GetContainerLimitRangeItem returns LimitRangeItem that describes limitation on container limits in the given namespace.
GetContainerLimitRangeItem(namespace string) (*core.LimitRangeItem, error)
// GetPodLimitRangeItem returns LimitRangeItem that describes limitation on pod limits in the given namespace.
GetPodLimitRangeItem(namespace string) (*core.LimitRangeItem, error)
}

type noopLimitsRangeCalculator struct{}
Expand All @@ -38,6 +40,10 @@ func (lc *noopLimitsRangeCalculator) GetContainerLimitRangeItem(namespace string
return nil, nil
}

func (lc *noopLimitsRangeCalculator) GetPodLimitRangeItem(namespace string) (*core.LimitRangeItem, error) {
return nil, nil
}

type limitsChecker struct {
limitRangeLister listers.LimitRangeLister
}
Expand Down Expand Up @@ -66,6 +72,14 @@ func NewNoopLimitsCalculator() *noopLimitsRangeCalculator {
}

func (lc *limitsChecker) GetContainerLimitRangeItem(namespace string) (*core.LimitRangeItem, error) {
return lc.getLimitRangeItem(namespace, core.LimitTypeContainer)
}

func (lc *limitsChecker) GetPodLimitRangeItem(namespace string) (*core.LimitRangeItem, error) {
return lc.getLimitRangeItem(namespace, core.LimitTypePod)
}

func (lc *limitsChecker) getLimitRangeItem(namespace string, limitType core.LimitType) (*core.LimitRangeItem, error) {
limitRanges, err := lc.limitRangeLister.LimitRanges(namespace).List(labels.Everything())
if err != nil {
return nil, fmt.Errorf("error loading limit ranges: %s", err)
Expand Down Expand Up @@ -102,10 +116,10 @@ func (lc *limitsChecker) GetContainerLimitRangeItem(namespace string) (*core.Lim
return q2
}

result := &core.LimitRangeItem{Type: core.LimitTypeContainer}
result := &core.LimitRangeItem{Type: limitType}
for _, lr := range limitRanges {
for _, lri := range lr.Spec.Limits {
if lri.Type == core.LimitTypeContainer && (lri.Max != nil || lri.Default != nil || lri.Min != nil) {
if lri.Type == limitType && (lri.Max != nil || lri.Default != nil || lri.Min != nil) {
if lri.Default != nil {
result.Default = lri.Default
}
Expand Down
57 changes: 56 additions & 1 deletion vertical-pod-autoscaler/pkg/utils/vpa/capping.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,11 @@ func (c *cappingRecommendationProcessor) Apply(
}
updatedRecommendations := []vpa_types.RecommendedContainerResources{}
containerToAnnotationsMap := ContainerToAnnotationsMap{}
for _, containerRecommendation := range podRecommendation.ContainerRecommendations {
limitAdjustedRecommendation, err := c.capProportionallyToPodLimitRange(podRecommendation.ContainerRecommendations, pod)
if err != nil {
return nil, nil, err
}
for _, containerRecommendation := range limitAdjustedRecommendation {
container := getContainer(containerRecommendation.ContainerName, pod)

if container == nil {
Expand Down Expand Up @@ -300,3 +304,54 @@ func getBoundaryRecommendation(recommendation apiv1.ResourceList, container apiv
apiv1.ResourceMemory: *memMaxRequest,
}
}

func applyPodLimitRange(resources []vpa_types.RecommendedContainerResources,
pod *apiv1.Pod, limitRange apiv1.LimitRangeItem, resourceName apiv1.ResourceName) []vpa_types.RecommendedContainerResources {
minLimit := limitRange.Min[resourceName]
maxLimit := limitRange.Max[resourceName]
defaultLimit := limitRange.Default[resourceName]

var sumLimit resource.Quantity
for i, container := range pod.Spec.Containers {
if i >= len(resources) {
continue
}
limit := container.Resources.Limits[resourceName]
request := container.Resources.Requests[resourceName]
recommendation := resources[i].Target[resourceName]
containerLimit, _ := getProportionalResourceLimit(resourceName, &limit, &request, &recommendation, &defaultLimit)
if containerLimit != nil {
sumLimit.Add(*containerLimit)
}
}
if minLimit.Cmp(sumLimit) <= 0 && (maxLimit.IsZero() || maxLimit.Cmp(sumLimit) >= 0) {
return resources
}
var targetTotalLimit resource.Quantity
if minLimit.Cmp(sumLimit) > 0 {
targetTotalLimit = minLimit
}
if !maxLimit.IsZero() && maxLimit.Cmp(sumLimit) < 0 {
targetTotalLimit = maxLimit
}
for i := range pod.Spec.Containers {
limit := resources[i].Target[resourceName]
cappedContainerRequest, _ := scaleQuantityProportionally(&limit, &sumLimit, &targetTotalLimit)
resources[i].Target[resourceName] = *cappedContainerRequest
}
return resources
}

func (c *cappingRecommendationProcessor) capProportionallyToPodLimitRange(
containerRecommendations []vpa_types.RecommendedContainerResources, pod *apiv1.Pod) ([]vpa_types.RecommendedContainerResources, error) {
podLimitRange, err := c.limitsRangeCalculator.GetPodLimitRangeItem(pod.Namespace)
if err != nil {
return nil, fmt.Errorf("error obtaining limit range: %s", err)
}
if podLimitRange == nil {
return containerRecommendations, nil
}
containerRecommendations = applyPodLimitRange(containerRecommendations, pod, *podLimitRange, apiv1.ResourceCPU)
containerRecommendations = applyPodLimitRange(containerRecommendations, pod, *podLimitRange, apiv1.ResourceMemory)
return containerRecommendations, nil
}
218 changes: 215 additions & 3 deletions vertical-pod-autoscaler/pkg/utils/vpa/capping_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,11 +217,16 @@ func TestApplyVpa(t *testing.T) {
}

type fakeLimitRangeCalculator struct {
limitRange apiv1.LimitRangeItem
containerLimitRange apiv1.LimitRangeItem
podLimitRange apiv1.LimitRangeItem
}

func (nlrc *fakeLimitRangeCalculator) GetContainerLimitRangeItem(namespace string) (*apiv1.LimitRangeItem, error) {
return &nlrc.limitRange, nil
return &nlrc.containerLimitRange, nil
}

func (nlrc *fakeLimitRangeCalculator) GetPodLimitRangeItem(namespace string) (*apiv1.LimitRangeItem, error) {
return &nlrc.podLimitRange, nil
}

func TestApplyCapsToLimitRange(t *testing.T) {
Expand Down Expand Up @@ -276,11 +281,218 @@ func TestApplyCapsToLimitRange(t *testing.T) {
},
}

calculator := fakeLimitRangeCalculator{limitRange}
calculator := fakeLimitRangeCalculator{containerLimitRange: limitRange}
processor := NewCappingRecommendationProcessor(&calculator)
processedRecommendation, annotations, err := processor.Apply(&recommendation, nil, nil, &pod)
assert.NoError(t, err)
assert.Contains(t, annotations, "container")
assert.ElementsMatch(t, []string{"cpu capped to fit Max in container LimitRange", "memory capped to fit Min in container LimitRange"}, annotations["container"])
assert.Equal(t, expectedRecommendation, *processedRecommendation)
}

func TestApplyPodLimitRange(t *testing.T) {
tests := []struct {
name string
resources []vpa_types.RecommendedContainerResources
pod apiv1.Pod
limitRange apiv1.LimitRangeItem
resourceName apiv1.ResourceName
expect []vpa_types.RecommendedContainerResources
}{
{
name: "cap target cpu to max",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also both min and max

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test is now redundant. I'll change limit ranges to have both min and max.

resources: []vpa_types.RecommendedContainerResources{
{
ContainerName: "container1",
Target: apiv1.ResourceList{
apiv1.ResourceCPU: resource.MustParse("1"),
},
},
{
ContainerName: "container2",
Target: apiv1.ResourceList{
apiv1.ResourceCPU: resource.MustParse("1"),
},
},
},
pod: apiv1.Pod{
Spec: apiv1.PodSpec{
Containers: []apiv1.Container{
{
Resources: apiv1.ResourceRequirements{
Requests: apiv1.ResourceList{
apiv1.ResourceCPU: resource.MustParse("1"),
},
Limits: apiv1.ResourceList{
apiv1.ResourceCPU: resource.MustParse("1"),
},
},
},
{
Resources: apiv1.ResourceRequirements{
Requests: apiv1.ResourceList{
apiv1.ResourceCPU: resource.MustParse("1"),
},
Limits: apiv1.ResourceList{
apiv1.ResourceCPU: resource.MustParse("1"),
},
},
},
},
},
},
limitRange: apiv1.LimitRangeItem{
Max: apiv1.ResourceList{
apiv1.ResourceCPU: resource.MustParse("1"),
},
},
resourceName: apiv1.ResourceCPU,
expect: []vpa_types.RecommendedContainerResources{
{
ContainerName: "container1",
Target: apiv1.ResourceList{
apiv1.ResourceCPU: *resource.NewMilliQuantity(500, resource.DecimalSI),
},
},
{
ContainerName: "container2",
Target: apiv1.ResourceList{
apiv1.ResourceCPU: *resource.NewMilliQuantity(500, resource.DecimalSI),
},
},
},
},
{
name: "cap cpu to max",
resources: []vpa_types.RecommendedContainerResources{
{
ContainerName: "container1",
Target: apiv1.ResourceList{
apiv1.ResourceCPU: resource.MustParse("1"),
},
},
{
ContainerName: "container2",
Target: apiv1.ResourceList{
apiv1.ResourceCPU: resource.MustParse("1"),
},
},
},
pod: apiv1.Pod{
Spec: apiv1.PodSpec{
Containers: []apiv1.Container{
{
Resources: apiv1.ResourceRequirements{
Requests: apiv1.ResourceList{
apiv1.ResourceCPU: resource.MustParse("1"),
},
Limits: apiv1.ResourceList{
apiv1.ResourceCPU: resource.MustParse("1"),
},
},
},
{
Resources: apiv1.ResourceRequirements{
Requests: apiv1.ResourceList{
apiv1.ResourceCPU: resource.MustParse("1"),
},
Limits: apiv1.ResourceList{
apiv1.ResourceCPU: resource.MustParse("1"),
},
},
},
},
},
},
limitRange: apiv1.LimitRangeItem{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is Pod type the default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it doesn't matter for this test but I see how this could be confusing. Done.

Max: apiv1.ResourceList{
apiv1.ResourceCPU: resource.MustParse("1"),
},
},
resourceName: apiv1.ResourceCPU,
expect: []vpa_types.RecommendedContainerResources{
{
ContainerName: "container1",
Target: apiv1.ResourceList{
apiv1.ResourceCPU: *resource.NewMilliQuantity(500, resource.DecimalSI),
},
},
{
ContainerName: "container2",
Target: apiv1.ResourceList{
apiv1.ResourceCPU: *resource.NewMilliQuantity(500, resource.DecimalSI),
},
},
},
},
{
name: "cap mem to min",
resources: []vpa_types.RecommendedContainerResources{
{
ContainerName: "container1",
Target: apiv1.ResourceList{
apiv1.ResourceMemory: resource.MustParse("1G"),
},
},
{
ContainerName: "container2",
Target: apiv1.ResourceList{
apiv1.ResourceMemory: resource.MustParse("1G"),
},
},
},
pod: apiv1.Pod{
Spec: apiv1.PodSpec{
Containers: []apiv1.Container{
{
Resources: apiv1.ResourceRequirements{
Requests: apiv1.ResourceList{
apiv1.ResourceMemory: resource.MustParse("1"),
},
Limits: apiv1.ResourceList{
apiv1.ResourceMemory: resource.MustParse("1"),
},
},
},
{
Resources: apiv1.ResourceRequirements{
Requests: apiv1.ResourceList{
apiv1.ResourceMemory: resource.MustParse("1"),
},
Limits: apiv1.ResourceList{
apiv1.ResourceMemory: resource.MustParse("1"),
},
},
},
},
},
},
limitRange: apiv1.LimitRangeItem{
Min: apiv1.ResourceList{
apiv1.ResourceMemory: resource.MustParse("4G"),
},
},
resourceName: apiv1.ResourceMemory,
expect: []vpa_types.RecommendedContainerResources{
{
ContainerName: "container1",
Target: apiv1.ResourceList{
apiv1.ResourceMemory: resource.MustParse("2000000000000m"),
},
},
{
ContainerName: "container2",
Target: apiv1.ResourceList{
apiv1.ResourceMemory: resource.MustParse("2000000000000m"),
},
},
},
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
got := applyPodLimitRange(tc.resources, &tc.pod, tc.limitRange, tc.resourceName)
assert.Equal(t, tc.expect, got)
})
}
}