fixed container restart due to field changes

pkg/kubelet/container/container_hash_test.go

@@ -68,8 +68,7 @@ var (
 }
 `
 
-	sampleV115HashValue = uint64(0x311670a)
+	sampleV131HashValue = uint64(0x8e45cbd0)
-	sampleV116HashValue = sampleV115HashValue
 )
 
 func TestConsistentHashContainer(t *testing.T) {
@@ -79,11 +78,7 @@ func TestConsistentHashContainer(t *testing.T) {
 	}
 
 	currentHash := HashContainer(container)
-	if currentHash != sampleV116HashValue {
+	if currentHash != sampleV131HashValue {
-		t.Errorf("mismatched hash value with v1.16")
+		t.Errorf("mismatched hash value with v1.31")
-	}
-
-	if currentHash != sampleV115HashValue {
-		t.Errorf("mismatched hash value with v1.15")
 	}
 }

pkg/kubelet/container/helpers.go

@@ -110,28 +110,20 @@ func ShouldContainerBeRestarted(container *v1.Container, pod *v1.Pod, podStatus
 // Note: remember to update hashValues in container_hash_test.go as well.
 func HashContainer(container *v1.Container) uint64 {
 	hash := fnv.New32a()
-	// Omit nil or empty field when calculating hash value
+	containerJSON, _ := json.Marshal(pickFieldsToHash(container))
-	// Please see https://github.com/kubernetes/kubernetes/issues/53644
-	containerJSON, _ := json.Marshal(container)
 	hashutil.DeepHashObject(hash, containerJSON)
 	return uint64(hash.Sum32())
 }
 
-// HashContainerWithoutResources returns the hash of the container with Resources field zero'd out.
+// pickFieldsToHash pick fields that will affect the running status of the container for hash,
-func HashContainerWithoutResources(container *v1.Container) uint64 {
+// currently this field range only contains `image` and `name`.
-	// InPlacePodVerticalScaling enables mutable Resources field.
+// Note: this list must be updated if ever kubelet wants to allow mutations to other fields.
-	// Changes to this field may not require container restart depending on policy.
+func pickFieldsToHash(container *v1.Container) map[string]string {
-	// Compute hash over fields besides the Resources field
+	retval := map[string]string{
-	// NOTE: This is needed during alpha and beta so that containers using Resources but
+		"name":  container.Name,
-	//       not subject to In-place resize are not unexpectedly restarted when
+		"image": container.Image,
-	//       InPlacePodVerticalScaling feature-gate is toggled.
+	}
-	//TODO(vinaykul,InPlacePodVerticalScaling): Remove this in GA+1 and make HashContainerWithoutResources to become Hash.
+	return retval
-	hashWithoutResources := fnv.New32a()
-	containerCopy := container.DeepCopy()
-	containerCopy.Resources = v1.ResourceRequirements{}
-	containerJSON, _ := json.Marshal(containerCopy)
-	hashutil.DeepHashObject(hashWithoutResources, containerJSON)
-	return uint64(hashWithoutResources.Sum32())
 }
 
 // envVarsToMap constructs a map of environment name to value from a slice

pkg/kubelet/container/helpers_test.go

@@ -657,7 +657,7 @@ func TestHashContainer(t *testing.T) {
 				"echo abc",
 			},
 			containerPort: int32(8001),
-			expectedHash:  uint64(0x3c42280f),
+			expectedHash:  uint64(0x8e45cbd0),
 		},
 	}
 
@@ -938,7 +938,7 @@ func TestHashContainerWithoutResources(t *testing.T) {
 				},
 				ResizePolicy: []v1.ContainerResizePolicy{cpuPolicyRestartRequired, memPolicyRestartNotRequired},
 			},
-			0x5f62cb4c,
+			0x11a6d6d6,
 		},
 		{
 			"Burstable pod with memory policy restart required",
@@ -951,7 +951,7 @@ func TestHashContainerWithoutResources(t *testing.T) {
 				},
 				ResizePolicy: []v1.ContainerResizePolicy{cpuPolicyRestartNotRequired, memPolicyRestartRequired},
 			},
-			0xcdab9e00,
+			0x11a6d6d6,
 		},
 		{
 			"Guaranteed pod with CPU policy restart required",
@@ -964,7 +964,7 @@ func TestHashContainerWithoutResources(t *testing.T) {
 				},
 				ResizePolicy: []v1.ContainerResizePolicy{cpuPolicyRestartRequired, memPolicyRestartNotRequired},
 			},
-			0x5f62cb4c,
+			0x11a6d6d6,
 		},
 		{
 			"Guaranteed pod with memory policy restart required",
@@ -977,13 +977,13 @@ func TestHashContainerWithoutResources(t *testing.T) {
 				},
 				ResizePolicy: []v1.ContainerResizePolicy{cpuPolicyRestartNotRequired, memPolicyRestartRequired},
 			},
-			0xcdab9e00,
+			0x11a6d6d6,
 		},
 	}
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
 			containerCopy := tc.container.DeepCopy()
-			hash := HashContainerWithoutResources(tc.container)
+			hash := HashContainer(tc.container)
 			assert.Equal(t, tc.expectedHash, hash, "[%s]", tc.name)
 			assert.Equal(t, containerCopy, tc.container, "[%s]", tc.name)
 		})

pkg/kubelet/kuberuntime/kuberuntime_manager.go

@@ -984,7 +984,7 @@ func (m *kubeGenericRuntimeManager) computePodActions(ctx context.Context, pod *
 		// Do not restart if only the Resources field has changed with InPlacePodVerticalScaling enabled
 		if _, _, changed := containerChanged(&container, containerStatus); changed &&
 			(!isInPlacePodVerticalScalingAllowed(pod) ||
-				kubecontainer.HashContainerWithoutResources(&container) != containerStatus.HashWithoutResources) {
+				kubecontainer.HashContainer(&container) != containerStatus.HashWithoutResources) {
 			message = fmt.Sprintf("Container %s definition changed", container.Name)
 			// Restart regardless of the restart policy because the container
 			// spec changed.

pkg/kubelet/kuberuntime/kuberuntime_manager_test.go

@@ -2436,7 +2436,7 @@ func TestComputePodActionsForPodResize(t *testing.T) {
 			// compute hash
 			if kcs := kps.FindContainerStatusByName(pod.Spec.Containers[idx].Name); kcs != nil {
 				kcs.Hash = kubecontainer.HashContainer(&pod.Spec.Containers[idx])
-				kcs.HashWithoutResources = kubecontainer.HashContainerWithoutResources(&pod.Spec.Containers[idx])
+				kcs.HashWithoutResources = kubecontainer.HashContainer(&pod.Spec.Containers[idx])
 			}
 		}
 		makeAndSetFakePod(t, m, fakeRuntime, pod)
@@ -2452,7 +2452,7 @@ func TestComputePodActionsForPodResize(t *testing.T) {
 		for idx := range pod.Spec.Containers {
 			if kcs := kps.FindContainerStatusByName(pod.Spec.Containers[idx].Name); kcs != nil {
 				kcs.Hash = kubecontainer.HashContainer(&pod.Spec.Containers[idx])
-				kcs.HashWithoutResources = kubecontainer.HashContainerWithoutResources(&pod.Spec.Containers[idx])
+				kcs.HashWithoutResources = kubecontainer.HashContainer(&pod.Spec.Containers[idx])
 			}
 		}
 		if test.mutatePodFn != nil {

pkg/kubelet/kuberuntime/labels.go

@@ -118,7 +118,7 @@ func newContainerAnnotations(container *v1.Container, pod *v1.Pod, restartCount
 
 	annotations[containerHashLabel] = strconv.FormatUint(kubecontainer.HashContainer(container), 16)
 	if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) {
-		annotations[containerHashWithoutResourcesLabel] = strconv.FormatUint(kubecontainer.HashContainerWithoutResources(container), 16)
+		annotations[containerHashWithoutResourcesLabel] = strconv.FormatUint(kubecontainer.HashContainer(container), 16)
 	}
 	annotations[containerRestartCountLabel] = strconv.Itoa(restartCount)
 	annotations[containerTerminationMessagePathLabel] = container.TerminationMessagePath

pkg/kubelet/kuberuntime/labels_test.go

@@ -155,7 +155,7 @@ func TestContainerAnnotations(t *testing.T) {
 		PodDeletionGracePeriod:    pod.DeletionGracePeriodSeconds,
 		PodTerminationGracePeriod: pod.Spec.TerminationGracePeriodSeconds,
 		Hash:                      kubecontainer.HashContainer(container),
-		HashWithoutResources:      kubecontainer.HashContainerWithoutResources(container),
+		HashWithoutResources:      kubecontainer.HashContainer(container),
 		RestartCount:              restartCount,
 		TerminationMessagePath:    container.TerminationMessagePath,
 		PreStopHandler:            container.Lifecycle.PreStop,
@@ -182,7 +182,7 @@ func TestContainerAnnotations(t *testing.T) {
 	expected.PreStopHandler = nil
 	// Because container is changed, the Hash should be updated
 	expected.Hash = kubecontainer.HashContainer(container)
-	expected.HashWithoutResources = kubecontainer.HashContainerWithoutResources(container)
+	expected.HashWithoutResources = kubecontainer.HashContainer(container)
 	annotations = newContainerAnnotations(container, pod, restartCount, opts)
 	containerInfo = getContainerInfoFromAnnotations(annotations)
 	if !reflect.DeepEqual(containerInfo, expected) {