remove HashWithoutResources field

HirazawaUi · HirazawaUi · commit 3ec13c5e3752 · 2024-05-22T10:01:31.000+08:00
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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) {

Original file line number	Diff line number	Diff line change
`@@ -2436,7 +2436,6 @@ func TestComputePodActionsForPodResize(t *testing.T) {`
`2436`	`2436`	`// compute hash`
`2437`	`2437`	`if kcs := kps.FindContainerStatusByName(pod.Spec.Containers[idx].Name); kcs != nil {`
`2438`	`2438`	`kcs.Hash = kubecontainer.HashContainer(&pod.Spec.Containers[idx])`
`2439`		`- kcs.HashWithoutResources = kubecontainer.HashContainer(&pod.Spec.Containers[idx])`
`2440`	`2439`	`}`
`2441`	`2440`	`}`
`2442`	`2441`	`makeAndSetFakePod(t, m, fakeRuntime, pod)`
`@@ -2452,7 +2451,6 @@ func TestComputePodActionsForPodResize(t *testing.T) {`
`2452`	`2451`	`for idx := range pod.Spec.Containers {`
`2453`	`2452`	`if kcs := kps.FindContainerStatusByName(pod.Spec.Containers[idx].Name); kcs != nil {`
`2454`	`2453`	`kcs.Hash = kubecontainer.HashContainer(&pod.Spec.Containers[idx])`
`2455`		`- kcs.HashWithoutResources = kubecontainer.HashContainer(&pod.Spec.Containers[idx])`
`2456`	`2454`	`}`
`2457`	`2455`	`}`
`2458`	`2456`	`if test.mutatePodFn != nil {`