From aba588cd14e0d3654e02402cc9acb114190f7769 Mon Sep 17 00:00:00 2001 From: Tim Allclair Date: Mon, 17 Mar 2025 20:55:55 -0700 Subject: [PATCH] Deprecate IPPVSAllocatedStatus: always set allocatedResources with InPlacePodVerticalScaling --- pkg/api/pod/util.go | 4 +- pkg/api/pod/util_test.go | 101 ++++++++---------- pkg/features/kube_features.go | 4 +- pkg/kubelet/kubelet_pods.go | 4 +- pkg/kubelet/kubelet_pods_test.go | 16 +-- pkg/registry/core/pod/strategy_test.go | 1 - .../reference/versioned_feature_list.yaml | 4 + 7 files changed, 57 insertions(+), 77 deletions(-) diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index 67a641debc2..062272c844a 100644 --- a/pkg/api/pod/util.go +++ b/pkg/api/pod/util.go @@ -800,9 +800,7 @@ func dropDisabledPodStatusFields(podStatus, oldPodStatus *api.PodStatus, podSpec dropResourcesField(podStatus.ContainerStatuses) dropResourcesField(podStatus.InitContainerStatuses) dropResourcesField(podStatus.EphemeralContainerStatuses) - } - if !utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) || - !utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScalingAllocatedStatus) { + // Drop AllocatedResources field dropAllocatedResourcesField := func(csl []api.ContainerStatus) { for i := range csl { diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index 9abf43c5928..4ffb44b4ad0 100644 --- a/pkg/api/pod/util_test.go +++ b/pkg/api/pod/util_test.go @@ -2743,64 +2743,55 @@ func TestDropInPlacePodVerticalScaling(t *testing.T) { t.Run(fmt.Sprintf("InPlacePodVerticalScaling=%t", ippvsEnabled), func(t *testing.T) { featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.InPlacePodVerticalScaling, ippvsEnabled) - for _, allocatedStatusEnabled := range []bool{true, false} { - t.Run(fmt.Sprintf("AllocatedStatus=%t", allocatedStatusEnabled), func(t *testing.T) { - featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.InPlacePodVerticalScalingAllocatedStatus, allocatedStatusEnabled) - - for _, oldPodInfo := range podInfo { - for _, newPodInfo := range podInfo { - oldPodHasInPlaceVerticalScaling, oldPod := oldPodInfo.hasInPlaceVerticalScaling, oldPodInfo.pod() - newPodHasInPlaceVerticalScaling, newPod := newPodInfo.hasInPlaceVerticalScaling, newPodInfo.pod() - if newPod == nil { - continue - } - - t.Run(fmt.Sprintf("old pod %v, new pod %v", oldPodInfo.description, newPodInfo.description), func(t *testing.T) { - var oldPodSpec *api.PodSpec - var oldPodStatus *api.PodStatus - if oldPod != nil { - oldPodSpec = &oldPod.Spec - oldPodStatus = &oldPod.Status - } - dropDisabledFields(&newPod.Spec, nil, oldPodSpec, nil) - dropDisabledPodStatusFields(&newPod.Status, oldPodStatus, &newPod.Spec, oldPodSpec) - - // old pod should never be changed - if !reflect.DeepEqual(oldPod, oldPodInfo.pod()) { - t.Errorf("old pod changed: %v", cmp.Diff(oldPod, oldPodInfo.pod())) - } - - switch { - case ippvsEnabled || oldPodHasInPlaceVerticalScaling: - // new pod shouldn't change if feature enabled or if old pod has ResizePolicy set - expected := newPodInfo.pod() - if !ippvsEnabled || !allocatedStatusEnabled { - expected.Status.ContainerStatuses[0].AllocatedResources = nil - } - if !reflect.DeepEqual(newPod, expected) { - t.Errorf("new pod changed: %v", cmp.Diff(newPod, expected)) - } - case newPodHasInPlaceVerticalScaling: - // new pod should be changed - if reflect.DeepEqual(newPod, newPodInfo.pod()) { - t.Errorf("new pod was not changed") - } - // new pod should not have ResizePolicy - if !reflect.DeepEqual(newPod, podWithoutInPlaceVerticalScaling()) { - t.Errorf("new pod has ResizePolicy: %v", cmp.Diff(newPod, podWithoutInPlaceVerticalScaling())) - } - default: - // new pod should not need to be changed - if !reflect.DeepEqual(newPod, newPodInfo.pod()) { - t.Errorf("new pod changed: %v", cmp.Diff(newPod, newPodInfo.pod())) - } - } - }) - } + for _, oldPodInfo := range podInfo { + for _, newPodInfo := range podInfo { + oldPodHasInPlaceVerticalScaling, oldPod := oldPodInfo.hasInPlaceVerticalScaling, oldPodInfo.pod() + newPodHasInPlaceVerticalScaling, newPod := newPodInfo.hasInPlaceVerticalScaling, newPodInfo.pod() + if newPod == nil { + continue } - }) + t.Run(fmt.Sprintf("old pod %v, new pod %v", oldPodInfo.description, newPodInfo.description), func(t *testing.T) { + var oldPodSpec *api.PodSpec + var oldPodStatus *api.PodStatus + if oldPod != nil { + oldPodSpec = &oldPod.Spec + oldPodStatus = &oldPod.Status + } + dropDisabledFields(&newPod.Spec, nil, oldPodSpec, nil) + dropDisabledPodStatusFields(&newPod.Status, oldPodStatus, &newPod.Spec, oldPodSpec) + + // old pod should never be changed + if !reflect.DeepEqual(oldPod, oldPodInfo.pod()) { + t.Errorf("old pod changed: %v", cmp.Diff(oldPod, oldPodInfo.pod())) + } + + switch { + case ippvsEnabled || oldPodHasInPlaceVerticalScaling: + // new pod shouldn't change if feature enabled or if old pod has ResizePolicy set + expected := newPodInfo.pod() + if !reflect.DeepEqual(newPod, expected) { + t.Errorf("new pod changed: %v", cmp.Diff(newPod, expected)) + } + case newPodHasInPlaceVerticalScaling: + // new pod should be changed + if reflect.DeepEqual(newPod, newPodInfo.pod()) { + t.Errorf("new pod was not changed") + } + // new pod should not have ResizePolicy + if !reflect.DeepEqual(newPod, podWithoutInPlaceVerticalScaling()) { + t.Errorf("new pod has ResizePolicy: %v", cmp.Diff(newPod, podWithoutInPlaceVerticalScaling())) + } + default: + // new pod should not need to be changed + if !reflect.DeepEqual(newPod, newPodInfo.pod()) { + t.Errorf("new pod changed: %v", cmp.Diff(newPod, newPodInfo.pod())) + } + } + }) + } } + }) } } diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index d66c22ef71c..8f7d2035b5c 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -311,7 +311,8 @@ const ( // owner: @tallclair // kep: http://kep.k8s.io/1287 // - // Enables the AllocatedResources field in container status. This feature requires + // Deprecated: This feature gate is no longer used. + // Was: Enables the AllocatedResources field in container status. This feature requires // InPlacePodVerticalScaling also be enabled. InPlacePodVerticalScalingAllocatedStatus featuregate.Feature = "InPlacePodVerticalScalingAllocatedStatus" @@ -1344,6 +1345,7 @@ var defaultVersionedKubernetesFeatureGates = map[featuregate.Feature]featuregate InPlacePodVerticalScalingAllocatedStatus: { {Version: version.MustParse("1.32"), Default: false, PreRelease: featuregate.Alpha}, + {Version: version.MustParse("1.33"), Default: false, PreRelease: featuregate.Deprecated}, // remove in 1.36 }, InPlacePodVerticalScalingExclusiveCPUs: { diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go index adab3a58c34..45ff4b1bd06 100644 --- a/pkg/kubelet/kubelet_pods.go +++ b/pkg/kubelet/kubelet_pods.go @@ -2279,9 +2279,7 @@ func (kl *Kubelet) convertToAPIContainerStatuses(pod *v1.Pod, podStatus *kubecon allocatedContainer := kubecontainer.GetContainerSpec(pod, cName) if allocatedContainer != nil { status.Resources = convertContainerStatusResources(allocatedContainer, status, cStatus, oldStatuses) - if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScalingAllocatedStatus) { - status.AllocatedResources = allocatedContainer.Resources.Requests - } + status.AllocatedResources = allocatedContainer.Resources.Requests } } diff --git a/pkg/kubelet/kubelet_pods_test.go b/pkg/kubelet/kubelet_pods_test.go index 5fd49e8db05..04ee389c6c5 100644 --- a/pkg/kubelet/kubelet_pods_test.go +++ b/pkg/kubelet/kubelet_pods_test.go @@ -5104,20 +5104,8 @@ func TestConvertToAPIContainerStatusesForResources(t *testing.T) { } podStatus := testPodStatus(state, resources) - for _, enableAllocatedStatus := range []bool{true, false} { - t.Run(fmt.Sprintf("AllocatedStatus=%t", enableAllocatedStatus), func(t *testing.T) { - featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.InPlacePodVerticalScalingAllocatedStatus, enableAllocatedStatus) - - expected := tc.Expected - if !enableAllocatedStatus { - expected = *expected.DeepCopy() - expected.AllocatedResources = nil - } - - cStatuses := kubelet.convertToAPIContainerStatuses(tPod, podStatus, []v1.ContainerStatus{tc.OldStatus}, tPod.Spec.Containers, false, false) - assert.Equal(t, expected, cStatuses[0]) - }) - } + cStatuses := kubelet.convertToAPIContainerStatuses(tPod, podStatus, []v1.ContainerStatus{tc.OldStatus}, tPod.Spec.Containers, false, false) + assert.Equal(t, tc.Expected, cStatuses[0]) }) } } diff --git a/pkg/registry/core/pod/strategy_test.go b/pkg/registry/core/pod/strategy_test.go index 42dde76d425..4dd0f83de2d 100644 --- a/pkg/registry/core/pod/strategy_test.go +++ b/pkg/registry/core/pod/strategy_test.go @@ -3092,7 +3092,6 @@ func TestPodResizePrepareForUpdate(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.InPlacePodVerticalScaling, true) - featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.InPlacePodVerticalScalingAllocatedStatus, true) featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SidecarContainers, true) ctx := context.Background() ResizeStrategy.PrepareForUpdate(ctx, tc.newPod, tc.oldPod) diff --git a/test/compatibility_lifecycle/reference/versioned_feature_list.yaml b/test/compatibility_lifecycle/reference/versioned_feature_list.yaml index acc2f63f7fb..380100da3d0 100644 --- a/test/compatibility_lifecycle/reference/versioned_feature_list.yaml +++ b/test/compatibility_lifecycle/reference/versioned_feature_list.yaml @@ -581,6 +581,10 @@ lockToDefault: false preRelease: Alpha version: "1.32" + - default: false + lockToDefault: false + preRelease: Deprecated + version: "1.33" - name: InPlacePodVerticalScalingExclusiveCPUs versionedSpecs: - default: false