Merge pull request #128920 from tallclair/ippr-defaults

[FG:InPlacePodVerticalScaling] Remove ResizePolicy defaulting

pkg/apis/core/v1/defaults.go

@@ -182,29 +182,6 @@ func SetDefaults_Pod(obj *v1.Pod) {
 				}
 			}
 		}
-		if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) &&
-			obj.Spec.Containers[i].Resources.Requests != nil {
-			// For normal containers, set resize restart policy to default value (NotRequired), if not specified.
-			resizePolicySpecified := make(map[v1.ResourceName]bool)
-			for _, p := range obj.Spec.Containers[i].ResizePolicy {
-				resizePolicySpecified[p.ResourceName] = true
-			}
-			setDefaultResizePolicy := func(resourceName v1.ResourceName) {
-				if _, found := resizePolicySpecified[resourceName]; !found {
-					obj.Spec.Containers[i].ResizePolicy = append(obj.Spec.Containers[i].ResizePolicy,
-						v1.ContainerResizePolicy{
-							ResourceName:  resourceName,
-							RestartPolicy: v1.NotRequired,
-						})
-				}
-			}
-			if _, exists := obj.Spec.Containers[i].Resources.Requests[v1.ResourceCPU]; exists {
-				setDefaultResizePolicy(v1.ResourceCPU)
-			}
-			if _, exists := obj.Spec.Containers[i].Resources.Requests[v1.ResourceMemory]; exists {
-				setDefaultResizePolicy(v1.ResourceMemory)
-			}
-		}
 	}
 	for i := range obj.Spec.InitContainers {
 		if obj.Spec.InitContainers[i].Resources.Limits != nil {

pkg/apis/core/v1/defaults_test.go

@@ -2982,218 +2982,6 @@ func TestSetDefaultServiceInternalTrafficPolicy(t *testing.T) {
 	}
 }
 
-func TestSetDefaultResizePolicy(t *testing.T) {
-	// verify we default to NotRequired restart policy for resize when resources are specified
-	featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.InPlacePodVerticalScaling, true)
-
-	for desc, tc := range map[string]struct {
-		testContainer        v1.Container
-		expectedResizePolicy []v1.ContainerResizePolicy
-	}{
-		"CPU and memory limits are specified": {
-			testContainer: v1.Container{
-				Resources: v1.ResourceRequirements{
-					Limits: v1.ResourceList{
-						v1.ResourceCPU:    resource.MustParse("100m"),
-						v1.ResourceMemory: resource.MustParse("200Mi"),
-					},
-				},
-			},
-			expectedResizePolicy: []v1.ContainerResizePolicy{
-				{
-					ResourceName:  v1.ResourceCPU,
-					RestartPolicy: v1.NotRequired,
-				},
-				{
-					ResourceName:  v1.ResourceMemory,
-					RestartPolicy: v1.NotRequired,
-				},
-			},
-		},
-		"CPU requests are specified": {
-			testContainer: v1.Container{
-				Resources: v1.ResourceRequirements{
-					Requests: v1.ResourceList{
-						v1.ResourceCPU: resource.MustParse("100m"),
-					},
-				},
-			},
-			expectedResizePolicy: []v1.ContainerResizePolicy{
-				{
-					ResourceName:  v1.ResourceCPU,
-					RestartPolicy: v1.NotRequired,
-				},
-			},
-		},
-		"Memory limits are specified": {
-			testContainer: v1.Container{
-				Resources: v1.ResourceRequirements{
-					Limits: v1.ResourceList{
-						v1.ResourceMemory: resource.MustParse("200Mi"),
-					},
-				},
-			},
-			expectedResizePolicy: []v1.ContainerResizePolicy{
-				{
-					ResourceName:  v1.ResourceMemory,
-					RestartPolicy: v1.NotRequired,
-				},
-			},
-		},
-		"No resources are specified": {
-			testContainer:        v1.Container{Name: "besteffort"},
-			expectedResizePolicy: nil,
-		},
-		"CPU and memory limits are specified with restartContainer resize policy for memory": {
-			testContainer: v1.Container{
-				Resources: v1.ResourceRequirements{
-					Limits: v1.ResourceList{
-						v1.ResourceCPU:    resource.MustParse("100m"),
-						v1.ResourceMemory: resource.MustParse("200Mi"),
-					},
-				},
-				ResizePolicy: []v1.ContainerResizePolicy{
-					{
-						ResourceName:  v1.ResourceMemory,
-						RestartPolicy: v1.RestartContainer,
-					},
-				},
-			},
-			expectedResizePolicy: []v1.ContainerResizePolicy{
-				{
-					ResourceName:  v1.ResourceMemory,
-					RestartPolicy: v1.RestartContainer,
-				},
-				{
-					ResourceName:  v1.ResourceCPU,
-					RestartPolicy: v1.NotRequired,
-				},
-			},
-		},
-		"CPU requests and memory limits are specified with restartContainer resize policy for CPU": {
-			testContainer: v1.Container{
-				Resources: v1.ResourceRequirements{
-					Limits: v1.ResourceList{
-						v1.ResourceMemory: resource.MustParse("200Mi"),
-					},
-					Requests: v1.ResourceList{
-						v1.ResourceCPU: resource.MustParse("100m"),
-					},
-				},
-				ResizePolicy: []v1.ContainerResizePolicy{
-					{
-						ResourceName:  v1.ResourceCPU,
-						RestartPolicy: v1.RestartContainer,
-					},
-				},
-			},
-			expectedResizePolicy: []v1.ContainerResizePolicy{
-				{
-					ResourceName:  v1.ResourceCPU,
-					RestartPolicy: v1.RestartContainer,
-				},
-				{
-					ResourceName:  v1.ResourceMemory,
-					RestartPolicy: v1.NotRequired,
-				},
-			},
-		},
-		"CPU and memory requests are specified with restartContainer resize policy for both": {
-			testContainer: v1.Container{
-				Resources: v1.ResourceRequirements{
-					Requests: v1.ResourceList{
-						v1.ResourceCPU:    resource.MustParse("100m"),
-						v1.ResourceMemory: resource.MustParse("200Mi"),
-					},
-				},
-				ResizePolicy: []v1.ContainerResizePolicy{
-					{
-						ResourceName:  v1.ResourceCPU,
-						RestartPolicy: v1.RestartContainer,
-					},
-					{
-						ResourceName:  v1.ResourceMemory,
-						RestartPolicy: v1.RestartContainer,
-					},
-				},
-			},
-			expectedResizePolicy: []v1.ContainerResizePolicy{
-				{
-					ResourceName:  v1.ResourceCPU,
-					RestartPolicy: v1.RestartContainer,
-				},
-				{
-					ResourceName:  v1.ResourceMemory,
-					RestartPolicy: v1.RestartContainer,
-				},
-			},
-		},
-		"Ephemeral storage limits are specified": {
-			testContainer: v1.Container{
-				Resources: v1.ResourceRequirements{
-					Limits: v1.ResourceList{
-						v1.ResourceEphemeralStorage: resource.MustParse("500Mi"),
-					},
-				},
-			},
-			expectedResizePolicy: nil,
-		},
-		"Ephemeral storage requests and CPU limits are specified": {
-			testContainer: v1.Container{
-				Resources: v1.ResourceRequirements{
-					Limits: v1.ResourceList{
-						v1.ResourceCPU: resource.MustParse("100m"),
-					},
-					Requests: v1.ResourceList{
-						v1.ResourceEphemeralStorage: resource.MustParse("500Mi"),
-					},
-				},
-			},
-			expectedResizePolicy: []v1.ContainerResizePolicy{
-				{
-					ResourceName:  v1.ResourceCPU,
-					RestartPolicy: v1.NotRequired,
-				},
-			},
-		},
-		"Ephemeral storage requests and limits, memory requests with restartContainer policy are specified": {
-			testContainer: v1.Container{
-				Resources: v1.ResourceRequirements{
-					Limits: v1.ResourceList{
-						v1.ResourceEphemeralStorage: resource.MustParse("500Mi"),
-					},
-					Requests: v1.ResourceList{
-						v1.ResourceEphemeralStorage: resource.MustParse("500Mi"),
-						v1.ResourceMemory:           resource.MustParse("200Mi"),
-					},
-				},
-				ResizePolicy: []v1.ContainerResizePolicy{
-					{
-						ResourceName:  v1.ResourceMemory,
-						RestartPolicy: v1.RestartContainer,
-					},
-				},
-			},
-			expectedResizePolicy: []v1.ContainerResizePolicy{
-				{
-					ResourceName:  v1.ResourceMemory,
-					RestartPolicy: v1.RestartContainer,
-				},
-			},
-		},
-	} {
-		t.Run(desc, func(t *testing.T) {
-			testPod := v1.Pod{}
-			testPod.Spec.Containers = append(testPod.Spec.Containers, tc.testContainer)
-			output := roundTrip(t, runtime.Object(&testPod))
-			pod2 := output.(*v1.Pod)
-			if !cmp.Equal(pod2.Spec.Containers[0].ResizePolicy, tc.expectedResizePolicy) {
-				t.Errorf("expected resize policy %+v, but got %+v", tc.expectedResizePolicy, pod2.Spec.Containers[0].ResizePolicy)
-			}
-		})
-	}
-}
-
 func TestSetDefaults_Volume(t *testing.T) {
 	featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ImageVolume, true)
 	for desc, tc := range map[string]struct {

pkg/kubelet/kuberuntime/kuberuntime_manager.go

@@ -617,17 +617,16 @@ func (m *kubeGenericRuntimeManager) computePodResizeAction(pod *v1.Pod, containe
 		return true
 	}
 
-	resizePolicy := make(map[v1.ResourceName]v1.ResourceResizeRestartPolicy)
-	for _, pol := range container.ResizePolicy {
-		resizePolicy[pol.ResourceName] = pol.RestartPolicy
-	}
 	determineContainerResize := func(rName v1.ResourceName, specValue, statusValue int64) (resize, restart bool) {
 		if specValue == statusValue {
 			return false, false
 		}
-		if resizePolicy[rName] == v1.RestartContainer {
+		for _, policy := range container.ResizePolicy {
-			return true, true
+			if policy.ResourceName == rName {
+				return true, policy.RestartPolicy == v1.RestartContainer
+			}
 		}
+		// If a resource policy isn't set, the implicit default is NotRequired.
 		return true, false
 	}
 	markContainerForUpdate := func(rName v1.ResourceName, specValue, statusValue int64) {

test/e2e/common/node/pod_resize.go

@@ -980,8 +980,6 @@ func doPodResizeTests() {
 			var pErr error
 
 			tStamp := strconv.Itoa(time.Now().Nanosecond())
-			e2epod.InitDefaultResizePolicy(tc.containers)
-			e2epod.InitDefaultResizePolicy(tc.expected)
 			testPod = e2epod.MakePodWithResizableContainers(f.Namespace.Name, "", tStamp, tc.containers)
 			testPod.GenerateName = "resize-test-"
 			testPod = e2epod.MustMixinRestrictedPodSecurity(testPod)
@@ -1168,8 +1166,6 @@ func doPodResizeErrorTests() {
 			var pErr error
 
 			tStamp := strconv.Itoa(time.Now().Nanosecond())
-			e2epod.InitDefaultResizePolicy(tc.containers)
-			e2epod.InitDefaultResizePolicy(tc.expected)
 			testPod = e2epod.MakePodWithResizableContainers(f.Namespace.Name, "testpod", tStamp, tc.containers)
 			testPod = e2epod.MustMixinRestrictedPodSecurity(testPod)
 

test/e2e/framework/pod/resize.go

@@ -142,21 +142,6 @@ func getTestResourceInfo(tcInfo ResizableContainerInfo) (res v1.ResourceRequirem
 	return res, resizePol
 }
 
-func InitDefaultResizePolicy(containers []ResizableContainerInfo) {
-	noRestart := v1.NotRequired
-	setDefaultPolicy := func(ci *ResizableContainerInfo) {
-		if ci.CPUPolicy == nil {
-			ci.CPUPolicy = &noRestart
-		}
-		if ci.MemPolicy == nil {
-			ci.MemPolicy = &noRestart
-		}
-	}
-	for i := range containers {
-		setDefaultPolicy(&containers[i])
-	}
-}
-
 func makeResizableContainer(tcInfo ResizableContainerInfo) v1.Container {
 	cmd := "grep Cpus_allowed_list /proc/self/status | cut -f2 && sleep 1d"
 	res, resizePol := getTestResourceInfo(tcInfo)

test/e2e/node/pod_resize.go

@@ -138,8 +138,6 @@ func doPodResizeAdmissionPluginsTests() {
 			tc.enableAdmissionPlugin(ctx, f)
 
 			tStamp := strconv.Itoa(time.Now().Nanosecond())
-			e2epod.InitDefaultResizePolicy(containers)
-			e2epod.InitDefaultResizePolicy(expected)
 			testPod1 := e2epod.MakePodWithResizableContainers(f.Namespace.Name, "testpod1", tStamp, containers)
 			testPod1 = e2epod.MustMixinRestrictedPodSecurity(testPod1)
 			testPod2 := e2epod.MakePodWithResizableContainers(f.Namespace.Name, "testpod2", tStamp, containers)
@@ -267,8 +265,6 @@ func doPodResizeSchedulerTests(f *framework.Framework) {
 			}`, testPod2CPUQuantityResized.MilliValue(), testPod2CPUQuantityResized.MilliValue())
 
 		tStamp := strconv.Itoa(time.Now().Nanosecond())
-		e2epod.InitDefaultResizePolicy(c1)
-		e2epod.InitDefaultResizePolicy(c2)
 		testPod1 := e2epod.MakePodWithResizableContainers(f.Namespace.Name, "testpod1", tStamp, c1)
 		testPod1 = e2epod.MustMixinRestrictedPodSecurity(testPod1)
 		testPod2 := e2epod.MakePodWithResizableContainers(f.Namespace.Name, "testpod2", tStamp, c2)
@@ -324,7 +320,6 @@ func doPodResizeSchedulerTests(f *framework.Framework) {
 			}`, testPod1CPUQuantityResized.MilliValue(), testPod1CPUQuantityResized.MilliValue())
 
 		tStamp = strconv.Itoa(time.Now().Nanosecond())
-		e2epod.InitDefaultResizePolicy(c3)
 		testPod3 := e2epod.MakePodWithResizableContainers(f.Namespace.Name, "testpod3", tStamp, c3)
 		testPod3 = e2epod.MustMixinRestrictedPodSecurity(testPod3)
 		e2epod.SetNodeAffinity(&testPod3.Spec, node.Name)