From f6b650430a757a0afb6d70f5d335223ced3e9982 Mon Sep 17 00:00:00 2001 From: HirazawaUi <695097494plus@gmail.com> Date: Sat, 6 Apr 2024 23:37:20 +0800 Subject: [PATCH 1/2] fixed container restart due to field changes --- pkg/kubelet/container/container_hash_test.go | 11 ++------ pkg/kubelet/container/helpers.go | 28 +++++++------------ pkg/kubelet/container/helpers_test.go | 12 ++++---- .../kuberuntime/kuberuntime_manager.go | 2 +- .../kuberuntime/kuberuntime_manager_test.go | 4 +-- pkg/kubelet/kuberuntime/labels.go | 2 +- pkg/kubelet/kuberuntime/labels_test.go | 4 +-- 7 files changed, 25 insertions(+), 38 deletions(-) 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) { From 3ec13c5e37525147c0fab05eb9416945906e0cf2 Mon Sep 17 00:00:00 2001 From: HirazawaUi <695097494plus@gmail.com> Date: Wed, 10 Apr 2024 22:09:14 +0800 Subject: [PATCH 2/2] remove HashWithoutResources field --- pkg/kubelet/container/helpers.go | 17 +++++++-------- pkg/kubelet/container/runtime.go | 7 ------- pkg/kubelet/kuberuntime/helpers.go | 17 +++++++-------- .../kuberuntime/kuberuntime_container.go | 21 +++++++++---------- .../kuberuntime/kuberuntime_manager.go | 9 ++++---- .../kuberuntime/kuberuntime_manager_test.go | 2 -- pkg/kubelet/kuberuntime/labels.go | 12 ----------- pkg/kubelet/kuberuntime/labels_test.go | 2 -- 8 files changed, 31 insertions(+), 56 deletions(-) diff --git a/pkg/kubelet/container/helpers.go b/pkg/kubelet/container/helpers.go index bcd20643271..7f8963ab6a1 100644 --- a/pkg/kubelet/container/helpers.go +++ b/pkg/kubelet/container/helpers.go @@ -261,15 +261,14 @@ func ConvertPodStatusToRunningPod(runtimeName string, podStatus *PodStatus) Pod continue } container := &Container{ - ID: containerStatus.ID, - Name: containerStatus.Name, - Image: containerStatus.Image, - ImageID: containerStatus.ImageID, - ImageRef: containerStatus.ImageRef, - ImageRuntimeHandler: containerStatus.ImageRuntimeHandler, - Hash: containerStatus.Hash, - HashWithoutResources: containerStatus.HashWithoutResources, - State: containerStatus.State, + ID: containerStatus.ID, + Name: containerStatus.Name, + Image: containerStatus.Image, + ImageID: containerStatus.ImageID, + ImageRef: containerStatus.ImageRef, + ImageRuntimeHandler: containerStatus.ImageRuntimeHandler, + Hash: containerStatus.Hash, + State: containerStatus.State, } runningPod.Containers = append(runningPod.Containers, container) } diff --git a/pkg/kubelet/container/runtime.go b/pkg/kubelet/container/runtime.go index b53214ca017..539c97bb0e4 100644 --- a/pkg/kubelet/container/runtime.go +++ b/pkg/kubelet/container/runtime.go @@ -295,11 +295,6 @@ type Container struct { // Hash of the container, used for comparison. Optional for containers // not managed by kubelet. Hash uint64 - // Hash of the container over fields with Resources field zero'd out. - // NOTE: This is needed during alpha and beta so that containers using Resources are - // not unexpectedly restarted when InPlacePodVerticalScaling feature-gate is toggled. - //TODO(vinaykul,InPlacePodVerticalScaling): Remove this in GA+1 and make HashWithoutResources to become Hash. - HashWithoutResources uint64 // State is the state of the container. State State } @@ -365,8 +360,6 @@ type Status struct { ImageRuntimeHandler string // Hash of the container, used for comparison. Hash uint64 - // Hash of the container over fields with Resources field zero'd out. - HashWithoutResources uint64 // Number of times that the container has been restarted. RestartCount int // A string explains why container is in such a status. diff --git a/pkg/kubelet/kuberuntime/helpers.go b/pkg/kubelet/kuberuntime/helpers.go index a257e52d75f..85961cb0280 100644 --- a/pkg/kubelet/kuberuntime/helpers.go +++ b/pkg/kubelet/kuberuntime/helpers.go @@ -102,15 +102,14 @@ func (m *kubeGenericRuntimeManager) toKubeContainer(c *runtimeapi.Container) (*k annotatedInfo := getContainerInfoFromAnnotations(c.Annotations) return &kubecontainer.Container{ - ID: kubecontainer.ContainerID{Type: m.runtimeName, ID: c.Id}, - Name: c.GetMetadata().GetName(), - ImageID: imageID, - ImageRef: c.ImageRef, - ImageRuntimeHandler: c.Image.RuntimeHandler, - Image: c.Image.Image, - Hash: annotatedInfo.Hash, - HashWithoutResources: annotatedInfo.HashWithoutResources, - State: toKubeContainerState(c.State), + ID: kubecontainer.ContainerID{Type: m.runtimeName, ID: c.Id}, + Name: c.GetMetadata().GetName(), + ImageID: imageID, + ImageRef: c.ImageRef, + ImageRuntimeHandler: c.Image.RuntimeHandler, + Image: c.Image.Image, + Hash: annotatedInfo.Hash, + State: toKubeContainerState(c.State), }, nil } diff --git a/pkg/kubelet/kuberuntime/kuberuntime_container.go b/pkg/kubelet/kuberuntime/kuberuntime_container.go index 3b534f1536f..4d84643dfae 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_container.go +++ b/pkg/kubelet/kuberuntime/kuberuntime_container.go @@ -622,17 +622,16 @@ func toKubeContainerStatus(status *runtimeapi.ContainerStatus, runtimeName strin Type: runtimeName, ID: status.Id, }, - Name: labeledInfo.ContainerName, - Image: status.Image.Image, - ImageID: imageID, - ImageRef: status.ImageRef, - ImageRuntimeHandler: status.Image.RuntimeHandler, - Hash: annotatedInfo.Hash, - HashWithoutResources: annotatedInfo.HashWithoutResources, - RestartCount: annotatedInfo.RestartCount, - State: toKubeContainerState(status.State), - CreatedAt: time.Unix(0, status.CreatedAt), - Resources: cStatusResources, + Name: labeledInfo.ContainerName, + Image: status.Image.Image, + ImageID: imageID, + ImageRef: status.ImageRef, + ImageRuntimeHandler: status.Image.RuntimeHandler, + Hash: annotatedInfo.Hash, + RestartCount: annotatedInfo.RestartCount, + State: toKubeContainerState(status.State), + CreatedAt: time.Unix(0, status.CreatedAt), + Resources: cStatusResources, } if status.State != runtimeapi.ContainerState_CONTAINER_CREATED { diff --git a/pkg/kubelet/kuberuntime/kuberuntime_manager.go b/pkg/kubelet/kuberuntime/kuberuntime_manager.go index b81cffee064..716ff486385 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_manager.go +++ b/pkg/kubelet/kuberuntime/kuberuntime_manager.go @@ -518,6 +518,10 @@ func (p podActions) String() string { p.KillPod, p.CreateSandbox, p.UpdatePodResources, p.Attempt, p.InitContainersToStart, p.ContainersToStart, p.EphemeralContainersToStart, p.ContainersToUpdate, p.ContainersToKill) } +// containerChanged will determine whether the container has changed based on the fields that will affect the running of the container. +// Currently, there are only `image` and `name` fields. +// we don't need to consider the pod UID here, because we find the containerStatus through the pod UID. +// If the pod UID changes, we will not be able to find the containerStatus to compare against. func containerChanged(container *v1.Container, containerStatus *kubecontainer.Status) (uint64, uint64, bool) { expectedHash := kubecontainer.HashContainer(container) return expectedHash, containerStatus.Hash, containerStatus.Hash != expectedHash @@ -981,10 +985,7 @@ func (m *kubeGenericRuntimeManager) computePodActions(ctx context.Context, pod * var message string var reason containerKillReason restart := shouldRestartOnFailure(pod) - // Do not restart if only the Resources field has changed with InPlacePodVerticalScaling enabled - if _, _, changed := containerChanged(&container, containerStatus); changed && - (!isInPlacePodVerticalScalingAllowed(pod) || - kubecontainer.HashContainer(&container) != containerStatus.HashWithoutResources) { + if _, _, changed := containerChanged(&container, containerStatus); changed { 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 3fc2519f6aa..7b3519635ad 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go +++ b/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go @@ -2436,7 +2436,6 @@ 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.HashContainer(&pod.Spec.Containers[idx]) } } makeAndSetFakePod(t, m, fakeRuntime, pod) @@ -2452,7 +2451,6 @@ 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.HashContainer(&pod.Spec.Containers[idx]) } } if test.mutatePodFn != nil { diff --git a/pkg/kubelet/kuberuntime/labels.go b/pkg/kubelet/kuberuntime/labels.go index edec7785a28..3611bebff0f 100644 --- a/pkg/kubelet/kuberuntime/labels.go +++ b/pkg/kubelet/kuberuntime/labels.go @@ -22,10 +22,8 @@ import ( v1 "k8s.io/api/core/v1" kubetypes "k8s.io/apimachinery/pkg/types" - utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/klog/v2" "k8s.io/kubelet/pkg/types" - "k8s.io/kubernetes/pkg/features" kubecontainer "k8s.io/kubernetes/pkg/kubelet/container" ) @@ -35,7 +33,6 @@ const ( podTerminationGracePeriodLabel = "io.kubernetes.pod.terminationGracePeriod" containerHashLabel = "io.kubernetes.container.hash" - containerHashWithoutResourcesLabel = "io.kubernetes.container.hashWithoutResources" containerRestartCountLabel = "io.kubernetes.container.restartCount" containerTerminationMessagePathLabel = "io.kubernetes.container.terminationMessagePath" containerTerminationMessagePolicyLabel = "io.kubernetes.container.terminationMessagePolicy" @@ -65,7 +62,6 @@ type labeledContainerInfo struct { type annotatedContainerInfo struct { Hash uint64 - HashWithoutResources uint64 RestartCount int PodDeletionGracePeriod *int64 PodTerminationGracePeriod *int64 @@ -117,9 +113,6 @@ 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.HashContainer(container), 16) - } annotations[containerRestartCountLabel] = strconv.Itoa(restartCount) annotations[containerTerminationMessagePathLabel] = container.TerminationMessagePath annotations[containerTerminationMessagePolicyLabel] = string(container.TerminationMessagePolicy) @@ -200,11 +193,6 @@ func getContainerInfoFromAnnotations(annotations map[string]string) *annotatedCo if containerInfo.Hash, err = getUint64ValueFromLabel(annotations, containerHashLabel); err != nil { klog.ErrorS(err, "Unable to get label value from annotations", "label", containerHashLabel, "annotations", annotations) } - if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) { - if containerInfo.HashWithoutResources, err = getUint64ValueFromLabel(annotations, containerHashWithoutResourcesLabel); err != nil { - klog.ErrorS(err, "Unable to get label value from annotations", "label", containerHashWithoutResourcesLabel, "annotations", annotations) - } - } if containerInfo.RestartCount, err = getIntValueFromLabel(annotations, containerRestartCountLabel); err != nil { klog.ErrorS(err, "Unable to get label value from annotations", "label", containerRestartCountLabel, "annotations", annotations) } diff --git a/pkg/kubelet/kuberuntime/labels_test.go b/pkg/kubelet/kuberuntime/labels_test.go index 7a7a0fef706..439a52a7ef4 100644 --- a/pkg/kubelet/kuberuntime/labels_test.go +++ b/pkg/kubelet/kuberuntime/labels_test.go @@ -155,7 +155,6 @@ func TestContainerAnnotations(t *testing.T) { PodDeletionGracePeriod: pod.DeletionGracePeriodSeconds, PodTerminationGracePeriod: pod.Spec.TerminationGracePeriodSeconds, Hash: kubecontainer.HashContainer(container), - HashWithoutResources: kubecontainer.HashContainer(container), RestartCount: restartCount, TerminationMessagePath: container.TerminationMessagePath, PreStopHandler: container.Lifecycle.PreStop, @@ -182,7 +181,6 @@ 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.HashContainer(container) annotations = newContainerAnnotations(container, pod, restartCount, opts) containerInfo = getContainerInfoFromAnnotations(annotations) if !reflect.DeepEqual(containerInfo, expected) {