diff --git a/pkg/kubelet/container/container_hash_test.go b/pkg/kubelet/container/container_hash_test.go index 53404c1924a..6585ad25ea5 100644 --- a/pkg/kubelet/container/container_hash_test.go +++ b/pkg/kubelet/container/container_hash_test.go @@ -68,8 +68,7 @@ var ( } ` - sampleV115HashValue = uint64(0x311670a) - sampleV116HashValue = sampleV115HashValue + sampleV131HashValue = uint64(0x8e45cbd0) ) func TestConsistentHashContainer(t *testing.T) { @@ -79,11 +78,7 @@ func TestConsistentHashContainer(t *testing.T) { } currentHash := HashContainer(container) - if currentHash != sampleV116HashValue { - t.Errorf("mismatched hash value with v1.16") - } - - if currentHash != sampleV115HashValue { - t.Errorf("mismatched hash value with v1.15") + if currentHash != sampleV131HashValue { + t.Errorf("mismatched hash value with v1.31") } } diff --git a/pkg/kubelet/container/helpers.go b/pkg/kubelet/container/helpers.go index 41427ad8f0f..bcd20643271 100644 --- a/pkg/kubelet/container/helpers.go +++ b/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 - // Please see https://github.com/kubernetes/kubernetes/issues/53644 - containerJSON, _ := json.Marshal(container) + containerJSON, _ := json.Marshal(pickFieldsToHash(container)) hashutil.DeepHashObject(hash, containerJSON) return uint64(hash.Sum32()) } -// HashContainerWithoutResources returns the hash of the container with Resources field zero'd out. -func HashContainerWithoutResources(container *v1.Container) uint64 { - // InPlacePodVerticalScaling enables mutable Resources field. - // Changes to this field may not require container restart depending on policy. - // Compute hash over fields besides the Resources field - // NOTE: This is needed during alpha and beta so that containers using Resources but - // not subject to In-place resize are not unexpectedly restarted when - // InPlacePodVerticalScaling feature-gate is toggled. - //TODO(vinaykul,InPlacePodVerticalScaling): Remove this in GA+1 and make HashContainerWithoutResources to become Hash. - hashWithoutResources := fnv.New32a() - containerCopy := container.DeepCopy() - containerCopy.Resources = v1.ResourceRequirements{} - containerJSON, _ := json.Marshal(containerCopy) - hashutil.DeepHashObject(hashWithoutResources, containerJSON) - return uint64(hashWithoutResources.Sum32()) +// pickFieldsToHash pick fields that will affect the running status of the container for hash, +// currently this field range only contains `image` and `name`. +// Note: this list must be updated if ever kubelet wants to allow mutations to other fields. +func pickFieldsToHash(container *v1.Container) map[string]string { + retval := map[string]string{ + "name": container.Name, + "image": container.Image, + } + return retval } // envVarsToMap constructs a map of environment name to value from a slice diff --git a/pkg/kubelet/container/helpers_test.go b/pkg/kubelet/container/helpers_test.go index 4964510dd2a..d44c386164d 100644 --- a/pkg/kubelet/container/helpers_test.go +++ b/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) }) diff --git a/pkg/kubelet/kuberuntime/kuberuntime_manager.go b/pkg/kubelet/kuberuntime/kuberuntime_manager.go index 86976ac2b54..b81cffee064 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_manager.go +++ b/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. diff --git a/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go b/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go index b994ba30c8c..3fc2519f6aa 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go +++ b/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 { diff --git a/pkg/kubelet/kuberuntime/labels.go b/pkg/kubelet/kuberuntime/labels.go index ef319eed868..edec7785a28 100644 --- a/pkg/kubelet/kuberuntime/labels.go +++ b/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 diff --git a/pkg/kubelet/kuberuntime/labels_test.go b/pkg/kubelet/kuberuntime/labels_test.go index b0b3b410bb7..7a7a0fef706 100644 --- a/pkg/kubelet/kuberuntime/labels_test.go +++ b/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) {