Updated some unit tests and resolved some review comments

Author: vivzbansal

pkg/apis/core/v1/defaults.go

@@ -166,7 +166,6 @@ func SetDefaults_Service(obj *v1.Service) {
 	}
 
 }
-
 func SetDefaults_Pod(obj *v1.Pod) {
 	// If limits are specified, but requests are not, default requests to limits
 	// This is done here rather than a more specific defaulting pass on v1.ResourceRequirements
@@ -183,10 +182,6 @@ func SetDefaults_Pod(obj *v1.Pod) {
 				}
 			}
 		}
-		if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) {
-			// For normal containers, set resize restart policy to default value (NotRequired), if not specified..
-			setDefaultResizePolicy(&obj.Spec.Containers[i])
-		}
 	}
 	for i := range obj.Spec.InitContainers {
 		if obj.Spec.InitContainers[i].Resources.Limits != nil {
@@ -199,12 +194,6 @@ func SetDefaults_Pod(obj *v1.Pod) {
 				}
 			}
 		}
-		if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) && utilfeature.DefaultFeatureGate.Enabled(features.SidecarContainers) {
-			if obj.Spec.InitContainers[i].RestartPolicy != nil && *obj.Spec.InitContainers[i].RestartPolicy == v1.ContainerRestartPolicyAlways {
-				// For restartable init containers, set resize restart policy to default value (NotRequired), if not specified.
-				setDefaultResizePolicy(&obj.Spec.InitContainers[i])
-			}
-		}
 	}
 
 	// Pod Requests default values must be applied after container-level default values
@@ -223,42 +212,6 @@ func SetDefaults_Pod(obj *v1.Pod) {
 		defaultHostNetworkPorts(&obj.Spec.InitContainers)
 	}
 }
-
-// setDefaultResizePolicy set resize restart policy to default value (NotRequired), if not specified.
-func setDefaultResizePolicy(obj *v1.Container) {
-	if obj.Resources.Requests == nil && obj.Resources.Limits == nil {
-		return
-	}
-	resizePolicySpecified := make(map[v1.ResourceName]bool)
-	for _, p := range obj.ResizePolicy {
-		resizePolicySpecified[p.ResourceName] = true
-	}
-	defaultResizePolicy := func(resourceName v1.ResourceName) {
-		if _, found := resizePolicySpecified[resourceName]; !found {
-			obj.ResizePolicy = append(obj.ResizePolicy,
-				v1.ContainerResizePolicy{
-					ResourceName:  resourceName,
-					RestartPolicy: v1.NotRequired,
-				})
-		}
-	}
-	if !resizePolicySpecified[v1.ResourceCPU] {
-		if _, exists := obj.Resources.Requests[v1.ResourceCPU]; exists {
-			defaultResizePolicy(v1.ResourceCPU)
-		} else if _, exists := obj.Resources.Limits[v1.ResourceCPU]; exists {
-			defaultResizePolicy(v1.ResourceCPU)
-		}
-	}
-
-	if !resizePolicySpecified[v1.ResourceMemory] {
-		if _, exists := obj.Resources.Requests[v1.ResourceMemory]; exists {
-			defaultResizePolicy(v1.ResourceMemory)
-		} else if _, exists := obj.Resources.Limits[v1.ResourceMemory]; exists {
-			defaultResizePolicy(v1.ResourceMemory)
-		}
-	}
-}
-
 func SetDefaults_PodSpec(obj *v1.PodSpec) {
 	// New fields added here will break upgrade tests:
 	// https://github.com/kubernetes/kubernetes/issues/69445

pkg/apis/core/v1/defaults_test.go

@@ -2982,230 +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)
-	featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SidecarContainers, true)
-	restartAlways := v1.ContainerRestartPolicyAlways
-	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,
-				},
-			},
-		},
-	} {
-		for _, isSidecarContainer := range []bool{true, false} {
-			t.Run(desc, func(t *testing.T) {
-				testPod := v1.Pod{}
-				if isSidecarContainer {
-					tc.testContainer.RestartPolicy = &restartAlways
-					testPod.Spec.InitContainers = append(testPod.Spec.InitContainers, tc.testContainer)
-				} else {
-					testPod.Spec.Containers = append(testPod.Spec.Containers, tc.testContainer)
-				}
-				output := roundTrip(t, runtime.Object(&testPod))
-				pod2 := output.(*v1.Pod)
-				if isSidecarContainer {
-					if !cmp.Equal(pod2.Spec.InitContainers[0].ResizePolicy, tc.expectedResizePolicy) {
-						t.Errorf("expected resize policy %+v, but got %+v for restartable init containers", tc.expectedResizePolicy, pod2.Spec.InitContainers[0].ResizePolicy)
-					}
-				} else if !cmp.Equal(pod2.Spec.Containers[0].ResizePolicy, tc.expectedResizePolicy) {
-					t.Errorf("expected resize policy %+v, but got %+v for normal containers", 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/kubelet_pods.go

@@ -1805,37 +1805,36 @@ func allocatedContainerResourcesMatchStatus(allocatedPod *v1.Pod, c *v1.Containe
 			}
 		}
 
-			// Only compare resizeable resources, and only compare resources that are explicitly configured.
-			if hasCPUReq {
-				if cs.Resources.CPURequest == nil {
-					if !cpuReq.IsZero() {
-						return false
-					}
-				} else if !cpuReq.Equal(*cs.Resources.CPURequest) &&
-					(cpuReq.MilliValue() > cm.MinShares || cs.Resources.CPURequest.MilliValue() > cm.MinShares) {
-					// If both allocated & status CPU requests are at or below MinShares then they are considered equal.
+		// Only compare resizeable resources, and only compare resources that are explicitly configured.
+		if hasCPUReq {
+			if cs.Resources.CPURequest == nil {
+				if !cpuReq.IsZero() {
 					return false
 				}
+			} else if !cpuReq.Equal(*cs.Resources.CPURequest) &&
+				(cpuReq.MilliValue() > cm.MinShares || cs.Resources.CPURequest.MilliValue() > cm.MinShares) {
+				// If both allocated & status CPU requests are at or below MinShares then they are considered equal.
+				return false
 			}
-			if hasCPULim {
-				if cs.Resources.CPULimit == nil {
-					if !cpuLim.IsZero() {
-						return false
-					}
-				} else if !cpuLim.Equal(*cs.Resources.CPULimit) &&
-					(cpuLim.MilliValue() > cm.MinMilliCPULimit || cs.Resources.CPULimit.MilliValue() > cm.MinMilliCPULimit) {
-					// If both allocated & status CPU limits are at or below the minimum limit, then they are considered equal.
+		}
+		if hasCPULim {
+			if cs.Resources.CPULimit == nil {
+				if !cpuLim.IsZero() {
 					return false
 				}
+			} else if !cpuLim.Equal(*cs.Resources.CPULimit) &&
+				(cpuLim.MilliValue() > cm.MinMilliCPULimit || cs.Resources.CPULimit.MilliValue() > cm.MinMilliCPULimit) {
+				// If both allocated & status CPU limits are at or below the minimum limit, then they are considered equal.
+				return false
 			}
-			if hasMemLim {
-				if cs.Resources.MemoryLimit == nil {
-					if !memLim.IsZero() {
-						return false
-					}
-				} else if !memLim.Equal(*cs.Resources.MemoryLimit) {
+		}
+		if hasMemLim {
+			if cs.Resources.MemoryLimit == nil {
+				if !memLim.IsZero() {
 					return false
 				}
+			} else if !memLim.Equal(*cs.Resources.MemoryLimit) {
+				return false
 			}
 		}
 	}