If user set limit to the same value as request keep them in sync

kubernetes · May 7, 2019 · c87f0da · c87f0da
1 parent 696dca5
commit c87f0da
Show file tree

Hide file tree

Showing 7 changed files with 303 additions and 66 deletions.
diff --git a/vertical-pod-autoscaler/pkg/admission-controller/logic/recommendation_provider.go b/vertical-pod-autoscaler/pkg/admission-controller/logic/recommendation_provider.go
@@ -17,7 +17,7 @@ limitations under the License.
 package logic
 
 import (
-	v1 "k8s.io/api/core/v1"
+	"k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/labels"
 
 	vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1beta2"
@@ -29,11 +29,15 @@ import (
 
 // ContainerResources holds resources request for container
 type ContainerResources struct {
+	Limits   v1.ResourceList
 	Requests v1.ResourceList
 }
 
 func newContainerResources() ContainerResources {
-	return ContainerResources{Requests: v1.ResourceList{}}
+	return ContainerResources{
+		Requests: v1.ResourceList{},
+		Limits:   v1.ResourceList{},
+	}
 }
 
 // RecommendationProvider gets current recommendation, annotations and vpaName for the given pod.
@@ -68,6 +72,17 @@ func getContainersResources(pod *v1.Pod, podRecommendation vpa_types.Recommended
 			continue
 		}
 		resources[i].Requests = recommendation.Target
+
+		// If user set containers limit and request for a resource to the same value then keep the limit in sync.
+		if container.Resources.Limits.Cpu() != nil && container.Resources.Limits.Cpu().Value() != 0 &&
+			container.Resources.Requests.Cpu() != nil && *container.Resources.Limits.Cpu() == *container.Resources.Requests.Cpu() {
+			resources[i].Limits[v1.ResourceCPU] = *resources[i].Requests.Cpu()
+		}
+
+		if container.Resources.Limits.Memory() != nil && container.Resources.Limits.Memory().Value() != 0 &&
+			container.Resources.Requests.Memory() != nil && *container.Resources.Limits.Memory() == *container.Resources.Requests.Memory() {
+			resources[i].Limits[v1.ResourceMemory] = *resources[i].Requests.Memory()
+		}
 	}
 	return resources
 }

diff --git a/vertical-pod-autoscaler/pkg/admission-controller/logic/recommendation_provider_test.go b/vertical-pod-autoscaler/pkg/admission-controller/logic/recommendation_provider_test.go
@@ -39,6 +39,11 @@ func parseLabelSelector(selector string) labels.Selector {
 	return parsedSelector
 }
 
+func mustParseResourcePointer(val string) *resource.Quantity {
+	q := resource.MustParse(val)
+	return &q
+}
+
 func TestUpdateResourceRequests(t *testing.T) {
 	containerName := "container1"
 	vpaName := "vpa1"
@@ -51,11 +56,20 @@ func TestUpdateResourceRequests(t *testing.T) {
 		WithMaxAllowed("3", "1Gi")
 	vpa := vpaBuilder.Get()
 
-	uninitialized := test.Pod().WithName("test_uninitialized").AddContainer(test.BuildTestContainer(containerName, "", "")).Get()
-	uninitialized.ObjectMeta.Labels = labels
+	uninitialized := test.Pod().WithName("test_uninitialized").
+		AddContainer(test.Container().WithName(containerName).Get()).
+		WithLabels(labels).Get()
+
+	initializedContainer := test.Container().WithName(containerName).
+		WithCPURequest(resource.MustParse("1")).WithMemRequest(resource.MustParse("100Mi")).Get()
+	initialized := test.Pod().WithName("test_initialized").
+		AddContainer(initializedContainer).WithLabels(labels).Get()
 
-	initialized := test.Pod().WithName("test_initialized").AddContainer(test.BuildTestContainer(containerName, "1", "100Mi")).Get()
-	initialized.ObjectMeta.Labels = labels
+	guaranteedCPUContainer := test.Container().WithName(containerName).
+		WithCPURequest(resource.MustParse("2")).WithCPULimit(resource.MustParse("2")).
+		WithMemLimit(resource.MustParse("200Mi")).WithMemRequest(resource.MustParse("200Mi")).Get()
+	guaranteedCPUPod := test.Pod().WithName("test_initialized").
+		AddContainer(guaranteedCPUContainer).WithLabels(labels).Get()
 
 	offVPA := vpaBuilder.WithUpdateMode(vpa_types.UpdateModeOff).Get()
 
@@ -70,23 +84,26 @@ func TestUpdateResourceRequests(t *testing.T) {
 	vpaWithNilRecommendation.Status.Recommendation = nil
 
 	testCases := []struct {
-		name           string
-		pod            *apiv1.Pod
-		vpas           []*vpa_types.VerticalPodAutoscaler
-		expectedAction bool
-		expectedMem    resource.Quantity
-		expectedCPU    resource.Quantity
-		annotations    vpa_api_util.ContainerToAnnotationsMap
-		labelSelector  string
-	}{{
-		name:           "uninitialized pod",
-		pod:            uninitialized,
-		vpas:           []*vpa_types.VerticalPodAutoscaler{vpa},
-		expectedAction: true,
-		expectedMem:    resource.MustParse("200Mi"),
-		expectedCPU:    resource.MustParse("2"),
-		labelSelector:  "app = testingApp",
-	},
+		name             string
+		pod              *apiv1.Pod
+		vpas             []*vpa_types.VerticalPodAutoscaler
+		expectedAction   bool
+		expectedMem      resource.Quantity
+		expectedCPU      resource.Quantity
+		expectedCPULimit *resource.Quantity
+		expectedMemLimit *resource.Quantity
+		annotations      vpa_api_util.ContainerToAnnotationsMap
+		labelSelector    string
+	}{
+		{
+			name:           "uninitialized pod",
+			pod:            uninitialized,
+			vpas:           []*vpa_types.VerticalPodAutoscaler{vpa},
+			expectedAction: true,
+			expectedMem:    resource.MustParse("200Mi"),
+			expectedCPU:    resource.MustParse("2"),
+			labelSelector:  "app = testingApp",
+		},
 		{
 			name:           "target below min",
 			pod:            uninitialized,
@@ -169,6 +186,17 @@ func TestUpdateResourceRequests(t *testing.T) {
 			expectedCPU:    resource.MustParse("0"),
 			labelSelector:  "app = testingApp",
 		},
+		{
+			name:             "guaranteed resources",
+			pod:              guaranteedCPUPod,
+			vpas:             []*vpa_types.VerticalPodAutoscaler{vpa},
+			expectedAction:   true,
+			expectedMem:      resource.MustParse("200Mi"),
+			expectedCPU:      resource.MustParse("2"),
+			expectedCPULimit: mustParseResourcePointer("2"),
+			expectedMemLimit: mustParseResourcePointer("200Mi"),
+			labelSelector:    "app = testingApp",
+		},
 	}
 	for _, tc := range testCases {
 
@@ -197,13 +225,34 @@ func TestUpdateResourceRequests(t *testing.T) {
 			if tc.expectedAction {
 				assert.Equal(t, vpaName, name)
 				assert.Nil(t, err)
-				assert.Equal(t, len(resources), 1)
+				if !assert.Equal(t, len(resources), 1) {
+					return
+				}
 
 				cpuRequest := resources[0].Requests[apiv1.ResourceCPU]
 				assert.Equal(t, tc.expectedCPU.Value(), cpuRequest.Value(), "cpu request doesn't match")
 
 				memoryRequest := resources[0].Requests[apiv1.ResourceMemory]
 				assert.Equal(t, tc.expectedMem.Value(), memoryRequest.Value(), "memory request doesn't match")
+
+				cpuLimit, cpuLimitPresent := resources[0].Limits[apiv1.ResourceCPU]
+				if tc.expectedCPULimit == nil {
+					assert.False(t, cpuLimitPresent, "expected no cpu limit, got %s", cpuLimit.String())
+				} else {
+					if assert.True(t, cpuLimitPresent, "expected cpu limit, but it's missing") {
+						assert.Equal(t, *tc.expectedCPULimit, cpuLimit, "cpu limit doesn't match")
+					}
+				}
+
+				memLimit, memLimitPresent := resources[0].Limits[apiv1.ResourceMemory]
+				if tc.expectedMemLimit == nil {
+					assert.False(t, memLimitPresent, "expected no memory limit, got %s", memLimit.String())
+				} else {
+					if assert.True(t, memLimitPresent, "expected cpu limit, but it's missing") {
+						assert.Equal(t, *tc.expectedMemLimit, memLimit, "memory limit doesn't match")
+					}
+				}
+
 				assert.Len(t, annotations, len(tc.annotations))
 				if len(tc.annotations) > 0 {
 					for annotationKey, annotationValues := range tc.annotations {

diff --git a/vertical-pod-autoscaler/pkg/admission-controller/logic/server.go b/vertical-pod-autoscaler/pkg/admission-controller/logic/server.go
@@ -127,23 +127,31 @@ func (s *AdmissionServer) getContainerPatch(pod v1.Pod, i int, patchKind string,
 		pod.Spec.Containers[i].Resources.Requests == nil {
 		patches = append(patches, getPatchInitializingEmptyResources(i))
 	}
-	// Add empty resources requests object if missing
-	if pod.Spec.Containers[i].Resources.Requests == nil {
-		patches = append(patches, getPatchInitializingEmptyResourcesSubfield(i, "requests"))
-	}
+
 	annotations, found := annotationsPerContainer[pod.Spec.Containers[i].Name]
 	if !found {
 		annotations = make([]string, 0)
 	}
-	for resource, request := range containerResources.Requests {
-		// Set request
-		patches = append(patches, getAddResourceRequirementValuePatch(i, "requests", resource, request))
-		annotations = append(annotations, fmt.Sprintf("%s request", resource))
-	}
+
+	patches, annotations = appendPatchesAndAnnotations(patches, annotations, pod.Spec.Containers[i].Resources.Requests, i, containerResources.Requests, "requests", "request")
+	patches, annotations = appendPatchesAndAnnotations(patches, annotations, pod.Spec.Containers[i].Resources.Limits, i, containerResources.Limits, "limits", "limit")
+
 	updatesAnnotation := fmt.Sprintf("container %d: ", i) + strings.Join(annotations, ", ")
 	return patches, updatesAnnotation
 }
 
+func appendPatchesAndAnnotations(patches []patchRecord, annotations []string, current v1.ResourceList, containerIndex int, resources v1.ResourceList, fieldName, resourceName string) ([]patchRecord, []string) {
+	// Add empty requests object if missing
+	if current == nil && len(resources) > 0 {
+		patches = append(patches, getPatchInitializingEmptyResourcesSubfield(containerIndex, fieldName))
+	}
+	for resource, request := range resources {
+		patches = append(patches, getAddResourceRequirementValuePatch(containerIndex, fieldName, resource, request))
+		annotations = append(annotations, fmt.Sprintf("%s %s", resource, resourceName))
+	}
+	return patches, annotations
+}
+
 func parseVPA(raw []byte) (*vpa_types.VerticalPodAutoscaler, error) {
 	vpa := vpa_types.VerticalPodAutoscaler{}
 	if err := json.Unmarshal(raw, &vpa); err != nil {