Resolved latest review comments

Author: vivzbansal

pkg/apis/core/validation/validation.go

@@ -5649,22 +5649,29 @@ func ValidatePodResize(newPod, oldPod *core.Pod, opts PodValidationOptions) fiel
 	originalCPUMemPodSpec.Containers = newContainers
 
 	// Ensure that only CPU and memory resources are mutable for restartable init containers.
+	// Also ensure that resources are immutable for non-restartable init containers.
 	var newInitContainers []core.Container
 	if utilfeature.DefaultFeatureGate.Enabled(features.SidecarContainers) {
 		for ix, container := range originalCPUMemPodSpec.InitContainers {
 			if isRestartableInitContainer(&container) { // restartable init container
 				dropCPUMemoryResourcesFromContainer(&container, &oldPod.Spec.InitContainers[ix])
+				if !apiequality.Semantic.DeepEqual(container, oldPod.Spec.InitContainers[ix]) {
+					// This likely means that the user has made changes to resources other than CPU and memory for sidecar container.
+					specDiff := cmp.Diff(oldPod.Spec.InitContainers[ix], container)
+					errs := field.Forbidden(specPath, fmt.Sprintf("only cpu and memory resources for sidecar containers are mutable\n%v", specDiff))
+					allErrs = append(allErrs, errs)
+					return allErrs
+				}
+			} else if !apiequality.Semantic.DeepEqual(container, oldPod.Spec.InitContainers[ix]) { // non-restartable init container
+				// This likely means that the user has modified resources of non-sidecar init container.
+				specDiff := cmp.Diff(oldPod.Spec.InitContainers[ix], container)
+				errs := field.Forbidden(specPath, fmt.Sprintf("resources for non-sidecar init containers are immutable\n%v", specDiff))
+				allErrs = append(allErrs, errs)
+				return allErrs
 			}
 			newInitContainers = append(newInitContainers, container)
 		}
 		originalCPUMemPodSpec.InitContainers = newInitContainers
-		if !apiequality.Semantic.DeepEqual(originalCPUMemPodSpec.InitContainers, oldPod.Spec.InitContainers) {
-			// This likely means that the user has modified non-sidecar container resources.
-			specDiff := cmp.Diff(oldPod.Spec.InitContainers, originalCPUMemPodSpec.InitContainers)
-			errs := field.Forbidden(specPath, fmt.Sprintf("cpu and memory resources for only sidecar containers are mutable\n%v", specDiff))
-			allErrs = append(allErrs, errs)
-			return allErrs
-		}
 	}
 
 	if !apiequality.Semantic.DeepEqual(originalCPUMemPodSpec, oldPod.Spec) {

pkg/apis/core/validation/validation_test.go

@@ -25911,7 +25911,7 @@ func TestValidatePodResize(t *testing.T) {
 			test: "Pod QoS unchanged, burstable -> burstable, remove cpu and memory requests",
 			old:  mkPodWithInitContainers(getResources("100m", "100Mi", "", ""), getResources("100m", "", "", ""), ""),
 			new:  mkPodWithInitContainers(core.ResourceList{}, getResources("100m", "", "", ""), ""),
-			err:  "spec: Forbidden: cpu and memory resources for only sidecar containers are mutable",
+			err:  "spec: Forbidden: resources for non-sidecar init containers are immutable",
 		}, {
 			test: "Pod with nil Resource field in Status",
 			old: mkPod(core.ResourceList{}, getResources("100m", "0", "1Gi", ""), podtest.SetStatus(core.PodStatus{
@@ -25993,7 +25993,7 @@ func TestValidatePodResize(t *testing.T) {
 			test: "storage limit change for sidecar containers",
 			old:  mkPodWithInitContainers(core.ResourceList{}, getResources("100m", "100Mi", "2Gi", ""), core.ContainerRestartPolicyAlways),
 			new:  mkPodWithInitContainers(core.ResourceList{}, getResources("100m", "100Mi", "1Gi", ""), core.ContainerRestartPolicyAlways),
-			err:  "spec: Forbidden: cpu and memory resources for only sidecar containers are mutable",
+			err:  "spec: Forbidden: only cpu and memory resources for sidecar containers are mutable",
 		}, {
 			test: "cpu request change for sidecar containers",
 			old:  mkPodWithInitContainers(getResources("200m", "0", "", ""), core.ResourceList{}, core.ContainerRestartPolicyAlways),
@@ -26008,7 +26008,7 @@ func TestValidatePodResize(t *testing.T) {
 			test: "storage request change for sidecar containers",
 			old:  mkPodWithInitContainers(getResources("100m", "0", "1Gi", ""), core.ResourceList{}, core.ContainerRestartPolicyAlways),
 			new:  mkPodWithInitContainers(getResources("100m", "0", "2Gi", ""), core.ResourceList{}, core.ContainerRestartPolicyAlways),
-			err:  "spec: Forbidden: cpu and memory resources for only sidecar containers are mutable",
+			err:  "spec: Forbidden: only cpu and memory resources for sidecar containers are mutable",
 		},
 	}
 

pkg/kubelet/kubelet_pods_test.go

@@ -6880,44 +6880,59 @@ func TestAllocatedResourcesMatchStatus(t *testing.T) {
 	}}
 
 	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			allocatedPod := v1.Pod{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "test",
-				},
-				Spec: v1.PodSpec{
-					Containers: []v1.Container{{
-						Name:      "c",
-						Resources: test.allocatedResources,
-					}},
-					InitContainers: []v1.Container{{
-						Name:          "c1-init",
-						Resources:     test.allocatedResources,
-						RestartPolicy: &containerRestartPolicyAlways,
-					}},
-				},
-			}
-			state := kubecontainer.ContainerStateRunning
-			if test.statusTerminated {
-				state = kubecontainer.ContainerStateExited
-			}
-			podStatus := &kubecontainer.PodStatus{
-				Name: "test",
-				ContainerStatuses: []*kubecontainer.Status{
-					{
-						Name:      "c",
-						State:     state,
-						Resources: test.statusResources,
-					},
-					{
-						Name:      "c1-init",
-						State:     state,
-						Resources: test.statusResources,
+		for _, isSidecarContainer := range []bool{false, true} {
+			t.Run(test.name, func(t *testing.T) {
+				var podStatus *kubecontainer.PodStatus
+				state := kubecontainer.ContainerStateRunning
+				if test.statusTerminated {
+					state = kubecontainer.ContainerStateExited
+				}
+
+				allocatedPod := v1.Pod{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "test",
 					},
-				},
-			}
-			match := allocatedResourcesMatchStatus(&allocatedPod, podStatus)
-			assert.Equal(t, test.expectMatch, match)
-		})
+				}
+
+				if isSidecarContainer {
+					allocatedPod.Spec = v1.PodSpec{
+						InitContainers: []v1.Container{{
+							Name:          "c1-init",
+							Resources:     test.allocatedResources,
+							RestartPolicy: &containerRestartPolicyAlways,
+						}},
+					}
+					podStatus = &kubecontainer.PodStatus{
+						Name: "test",
+						ContainerStatuses: []*kubecontainer.Status{
+							{
+								Name:      "c1-init",
+								State:     state,
+								Resources: test.statusResources,
+							},
+						},
+					}
+				} else {
+					allocatedPod.Spec = v1.PodSpec{
+						Containers: []v1.Container{{
+							Name:      "c",
+							Resources: test.allocatedResources,
+						}},
+					}
+					podStatus = &kubecontainer.PodStatus{
+						Name: "test",
+						ContainerStatuses: []*kubecontainer.Status{
+							{
+								Name:      "c",
+								State:     state,
+								Resources: test.statusResources,
+							},
+						},
+					}
+				}
+				match := allocatedResourcesMatchStatus(&allocatedPod, podStatus)
+				assert.Equal(t, test.expectMatch, match)
+			})
+		}
 	}
 }

staging/src/k8s.io/kubectl/pkg/util/resource/resource.go

@@ -45,16 +45,15 @@ func podRequests(pod *corev1.Pod) corev1.ResourceList {
 	for i := range pod.Status.ContainerStatuses {
 		containerStatuses[pod.Status.ContainerStatuses[i].Name] = &pod.Status.ContainerStatuses[i]
 	}
+	for i := range pod.Status.InitContainerStatuses {
+		containerStatuses[pod.Status.InitContainerStatuses[i].Name] = &pod.Status.InitContainerStatuses[i]
+	}
 
 	for _, container := range pod.Spec.Containers {
 		containerReqs := container.Resources.Requests
 		cs, found := containerStatuses[container.Name]
 		if found && cs.Resources != nil {
-			if pod.Status.Resize == corev1.PodResizeStatusInfeasible {
-				containerReqs = cs.Resources.Requests.DeepCopy()
-			} else {
-				containerReqs = max(container.Resources.Requests, cs.Resources.Requests)
-			}
+			containerReqs = determineContainerReqs(pod, &container, cs)
 		}
 		addResourceList(reqs, containerReqs)
 	}
@@ -66,6 +65,10 @@ func podRequests(pod *corev1.Pod) corev1.ResourceList {
 		containerReqs := container.Resources.Requests
 
 		if container.RestartPolicy != nil && *container.RestartPolicy == corev1.ContainerRestartPolicyAlways {
+			cs, found := containerStatuses[container.Name]
+			if found && cs.Resources != nil {
+				containerReqs = determineContainerReqs(pod, &container, cs)
+			}
 			// and add them to the resulting cumulative container requests
 			addResourceList(reqs, containerReqs)
 
@@ -137,6 +140,14 @@ func podLimits(pod *corev1.Pod) corev1.ResourceList {
 	return limits
 }
 
+// determineContainerReqs will return a copy of the container requests based on if resizing is feasible or not.
+func determineContainerReqs(pod *corev1.Pod, container *corev1.Container, cs *corev1.ContainerStatus) corev1.ResourceList {
+	if pod.Status.Resize == corev1.PodResizeStatusInfeasible {
+		return cs.Resources.Requests.DeepCopy()
+	}
+	return max(container.Resources.Requests, cs.Resources.Requests)
+}
+
 // max returns the result of max(a, b) for each named resource and is only used if we can't
 // accumulate into an existing resource list
 func max(a corev1.ResourceList, b corev1.ResourceList) corev1.ResourceList {