Skip to content

Commit e87e840

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#130880 from tallclair/ippr-allocatable
[FG:InPlacePodVerticalScaling] Add back `AllocatedResources` and use it for scheduling
2 parents b4c6895 + c292772 commit e87e840

File tree

10 files changed

+144
-112
lines changed

10 files changed

+144
-112
lines changed

‎pkg/api/pod/util.go‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -800,9 +800,7 @@ func dropDisabledPodStatusFields(podStatus, oldPodStatus *api.PodStatus, podSpec
800800
dropResourcesField(podStatus.ContainerStatuses)
801801
dropResourcesField(podStatus.InitContainerStatuses)
802802
dropResourcesField(podStatus.EphemeralContainerStatuses)
803-
}
804-
if !utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) ||
805-
!utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScalingAllocatedStatus) {
803+
806804
// Drop AllocatedResources field
807805
dropAllocatedResourcesField := func(csl []api.ContainerStatus) {
808806
for i := range csl {

‎pkg/api/pod/util_test.go‎

Lines changed: 45 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -2743,64 +2743,55 @@ func TestDropInPlacePodVerticalScaling(t *testing.T) {
27432743
t.Run(fmt.Sprintf("InPlacePodVerticalScaling=%t", ippvsEnabled), func(t *testing.T) {
27442744
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.InPlacePodVerticalScaling, ippvsEnabled)
27452745

2746-
for _, allocatedStatusEnabled := range []bool{true, false} {
2747-
t.Run(fmt.Sprintf("AllocatedStatus=%t", allocatedStatusEnabled), func(t *testing.T) {
2748-
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.InPlacePodVerticalScalingAllocatedStatus, allocatedStatusEnabled)
2749-
2750-
for _, oldPodInfo := range podInfo {
2751-
for _, newPodInfo := range podInfo {
2752-
oldPodHasInPlaceVerticalScaling, oldPod := oldPodInfo.hasInPlaceVerticalScaling, oldPodInfo.pod()
2753-
newPodHasInPlaceVerticalScaling, newPod := newPodInfo.hasInPlaceVerticalScaling, newPodInfo.pod()
2754-
if newPod == nil {
2755-
continue
2756-
}
2746+
for _, oldPodInfo := range podInfo {
2747+
for _, newPodInfo := range podInfo {
2748+
oldPodHasInPlaceVerticalScaling, oldPod := oldPodInfo.hasInPlaceVerticalScaling, oldPodInfo.pod()
2749+
newPodHasInPlaceVerticalScaling, newPod := newPodInfo.hasInPlaceVerticalScaling, newPodInfo.pod()
2750+
if newPod == nil {
2751+
continue
2752+
}
27572753

2758-
t.Run(fmt.Sprintf("old pod %v, new pod %v", oldPodInfo.description, newPodInfo.description), func(t *testing.T) {
2759-
var oldPodSpec *api.PodSpec
2760-
var oldPodStatus *api.PodStatus
2761-
if oldPod != nil {
2762-
oldPodSpec = &oldPod.Spec
2763-
oldPodStatus = &oldPod.Status
2764-
}
2765-
dropDisabledFields(&newPod.Spec, nil, oldPodSpec, nil)
2766-
dropDisabledPodStatusFields(&newPod.Status, oldPodStatus, &newPod.Spec, oldPodSpec)
2767-
2768-
// old pod should never be changed
2769-
if !reflect.DeepEqual(oldPod, oldPodInfo.pod()) {
2770-
t.Errorf("old pod changed: %v", cmp.Diff(oldPod, oldPodInfo.pod()))
2771-
}
2772-
2773-
switch {
2774-
case ippvsEnabled || oldPodHasInPlaceVerticalScaling:
2775-
// new pod shouldn't change if feature enabled or if old pod has ResizePolicy set
2776-
expected := newPodInfo.pod()
2777-
if !ippvsEnabled || !allocatedStatusEnabled {
2778-
expected.Status.ContainerStatuses[0].AllocatedResources = nil
2779-
}
2780-
if !reflect.DeepEqual(newPod, expected) {
2781-
t.Errorf("new pod changed: %v", cmp.Diff(newPod, expected))
2782-
}
2783-
case newPodHasInPlaceVerticalScaling:
2784-
// new pod should be changed
2785-
if reflect.DeepEqual(newPod, newPodInfo.pod()) {
2786-
t.Errorf("new pod was not changed")
2787-
}
2788-
// new pod should not have ResizePolicy
2789-
if !reflect.DeepEqual(newPod, podWithoutInPlaceVerticalScaling()) {
2790-
t.Errorf("new pod has ResizePolicy: %v", cmp.Diff(newPod, podWithoutInPlaceVerticalScaling()))
2791-
}
2792-
default:
2793-
// new pod should not need to be changed
2794-
if !reflect.DeepEqual(newPod, newPodInfo.pod()) {
2795-
t.Errorf("new pod changed: %v", cmp.Diff(newPod, newPodInfo.pod()))
2796-
}
2797-
}
2798-
})
2754+
t.Run(fmt.Sprintf("old pod %v, new pod %v", oldPodInfo.description, newPodInfo.description), func(t *testing.T) {
2755+
var oldPodSpec *api.PodSpec
2756+
var oldPodStatus *api.PodStatus
2757+
if oldPod != nil {
2758+
oldPodSpec = &oldPod.Spec
2759+
oldPodStatus = &oldPod.Status
27992760
}
2800-
}
2761+
dropDisabledFields(&newPod.Spec, nil, oldPodSpec, nil)
2762+
dropDisabledPodStatusFields(&newPod.Status, oldPodStatus, &newPod.Spec, oldPodSpec)
28012763

2802-
})
2764+
// old pod should never be changed
2765+
if !reflect.DeepEqual(oldPod, oldPodInfo.pod()) {
2766+
t.Errorf("old pod changed: %v", cmp.Diff(oldPod, oldPodInfo.pod()))
2767+
}
2768+
2769+
switch {
2770+
case ippvsEnabled || oldPodHasInPlaceVerticalScaling:
2771+
// new pod shouldn't change if feature enabled or if old pod has ResizePolicy set
2772+
expected := newPodInfo.pod()
2773+
if !reflect.DeepEqual(newPod, expected) {
2774+
t.Errorf("new pod changed: %v", cmp.Diff(newPod, expected))
2775+
}
2776+
case newPodHasInPlaceVerticalScaling:
2777+
// new pod should be changed
2778+
if reflect.DeepEqual(newPod, newPodInfo.pod()) {
2779+
t.Errorf("new pod was not changed")
2780+
}
2781+
// new pod should not have ResizePolicy
2782+
if !reflect.DeepEqual(newPod, podWithoutInPlaceVerticalScaling()) {
2783+
t.Errorf("new pod has ResizePolicy: %v", cmp.Diff(newPod, podWithoutInPlaceVerticalScaling()))
2784+
}
2785+
default:
2786+
// new pod should not need to be changed
2787+
if !reflect.DeepEqual(newPod, newPodInfo.pod()) {
2788+
t.Errorf("new pod changed: %v", cmp.Diff(newPod, newPodInfo.pod()))
2789+
}
2790+
}
2791+
})
2792+
}
28032793
}
2794+
28042795
})
28052796
}
28062797
}

‎pkg/features/kube_features.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -311,7 +311,8 @@ const (
311311
// owner: @tallclair
312312
// kep: http://kep.k8s.io/1287
313313
//
314-
// Enables the AllocatedResources field in container status. This feature requires
314+
// Deprecated: This feature gate is no longer used.
315+
// Was: Enables the AllocatedResources field in container status. This feature requires
315316
// InPlacePodVerticalScaling also be enabled.
316317
InPlacePodVerticalScalingAllocatedStatus featuregate.Feature = "InPlacePodVerticalScalingAllocatedStatus"
317318

@@ -1344,6 +1345,7 @@ var defaultVersionedKubernetesFeatureGates = map[featuregate.Feature]featuregate
13441345

13451346
InPlacePodVerticalScalingAllocatedStatus: {
13461347
{Version: version.MustParse("1.32"), Default: false, PreRelease: featuregate.Alpha},
1348+
{Version: version.MustParse("1.33"), Default: false, PreRelease: featuregate.Deprecated}, // remove in 1.36
13471349
},
13481350

13491351
InPlacePodVerticalScalingExclusiveCPUs: {

‎pkg/kubelet/kubelet_pods.go‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2285,9 +2285,7 @@ func (kl *Kubelet) convertToAPIContainerStatuses(pod *v1.Pod, podStatus *kubecon
22852285
allocatedContainer := kubecontainer.GetContainerSpec(pod, cName)
22862286
if allocatedContainer != nil {
22872287
status.Resources = convertContainerStatusResources(allocatedContainer, status, cStatus, oldStatuses)
2288-
if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScalingAllocatedStatus) {
2289-
status.AllocatedResources = allocatedContainer.Resources.Requests
2290-
}
2288+
status.AllocatedResources = allocatedContainer.Resources.Requests
22912289
}
22922290
}
22932291

‎pkg/kubelet/kubelet_pods_test.go‎

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5104,20 +5104,8 @@ func TestConvertToAPIContainerStatusesForResources(t *testing.T) {
51045104
}
51055105
podStatus := testPodStatus(state, resources)
51065106

5107-
for _, enableAllocatedStatus := range []bool{true, false} {
5108-
t.Run(fmt.Sprintf("AllocatedStatus=%t", enableAllocatedStatus), func(t *testing.T) {
5109-
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.InPlacePodVerticalScalingAllocatedStatus, enableAllocatedStatus)
5110-
5111-
expected := tc.Expected
5112-
if !enableAllocatedStatus {
5113-
expected = *expected.DeepCopy()
5114-
expected.AllocatedResources = nil
5115-
}
5116-
5117-
cStatuses := kubelet.convertToAPIContainerStatuses(tPod, podStatus, []v1.ContainerStatus{tc.OldStatus}, tPod.Spec.Containers, false, false)
5118-
assert.Equal(t, expected, cStatuses[0])
5119-
})
5120-
}
5107+
cStatuses := kubelet.convertToAPIContainerStatuses(tPod, podStatus, []v1.ContainerStatus{tc.OldStatus}, tPod.Spec.Containers, false, false)
5108+
assert.Equal(t, tc.Expected, cStatuses[0])
51215109
})
51225110
}
51235111
}

‎pkg/registry/core/pod/strategy_test.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3092,7 +3092,6 @@ func TestPodResizePrepareForUpdate(t *testing.T) {
30923092
for _, tc := range tests {
30933093
t.Run(tc.name, func(t *testing.T) {
30943094
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.InPlacePodVerticalScaling, true)
3095-
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.InPlacePodVerticalScalingAllocatedStatus, true)
30963095
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SidecarContainers, true)
30973096
ctx := context.Background()
30983097
ResizeStrategy.PrepareForUpdate(ctx, tc.newPod, tc.oldPod)

‎staging/src/k8s.io/component-helpers/resource/helpers.go‎

Lines changed: 7 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -224,9 +224,9 @@ func AggregateContainerRequests(pod *v1.Pod, opts PodResourcesOptions) v1.Resour
224224
// determineContainerReqs will return a copy of the container requests based on if resizing is feasible or not.
225225
func determineContainerReqs(pod *v1.Pod, container *v1.Container, cs *v1.ContainerStatus) v1.ResourceList {
226226
if IsPodResizeInfeasible(pod) {
227-
return cs.Resources.Requests.DeepCopy()
227+
return max(cs.Resources.Requests, cs.AllocatedResources)
228228
}
229-
return max(container.Resources.Requests, cs.Resources.Requests)
229+
return max(container.Resources.Requests, cs.Resources.Requests, cs.AllocatedResources)
230230
}
231231

232232
// determineContainerLimits will return a copy of the container limits based on if resizing is feasible or not.
@@ -399,23 +399,12 @@ func maxResourceList(list, newList v1.ResourceList) {
399399
}
400400
}
401401

402-
// max returns the result of max(a, b) for each named resource and is only used if we can't
402+
// max returns the result of max(a, b...) for each named resource and is only used if we can't
403403
// accumulate into an existing resource list
404-
func max(a v1.ResourceList, b v1.ResourceList) v1.ResourceList {
405-
result := v1.ResourceList{}
406-
for key, value := range a {
407-
if other, found := b[key]; found {
408-
if value.Cmp(other) <= 0 {
409-
result[key] = other.DeepCopy()
410-
continue
411-
}
412-
}
413-
result[key] = value.DeepCopy()
414-
}
415-
for key, value := range b {
416-
if _, found := result[key]; !found {
417-
result[key] = value.DeepCopy()
418-
}
404+
func max(a v1.ResourceList, b ...v1.ResourceList) v1.ResourceList {
405+
result := a.DeepCopy()
406+
for _, other := range b {
407+
maxResourceList(result, other)
419408
}
420409
return result
421410
}

‎staging/src/k8s.io/component-helpers/resource/helpers_test.go‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -459,6 +459,41 @@ func TestPodResourceRequests(t *testing.T) {
459459
},
460460
},
461461
},
462+
{
463+
description: "resized, infeasible & in-progress",
464+
expectedRequests: v1.ResourceList{
465+
v1.ResourceCPU: resource.MustParse("4"),
466+
},
467+
podResizeStatus: []v1.PodCondition{{
468+
Type: v1.PodResizePending,
469+
Status: v1.ConditionTrue,
470+
Reason: v1.PodReasonInfeasible,
471+
}},
472+
options: PodResourcesOptions{UseStatusResources: true},
473+
containers: []v1.Container{
474+
{
475+
Name: "container-1",
476+
Resources: v1.ResourceRequirements{
477+
Requests: v1.ResourceList{
478+
v1.ResourceCPU: resource.MustParse("6"),
479+
},
480+
},
481+
},
482+
},
483+
containerStatus: []v1.ContainerStatus{
484+
{
485+
Name: "container-1",
486+
AllocatedResources: v1.ResourceList{
487+
v1.ResourceCPU: resource.MustParse("4"),
488+
},
489+
Resources: &v1.ResourceRequirements{
490+
Requests: v1.ResourceList{
491+
v1.ResourceCPU: resource.MustParse("2"),
492+
},
493+
},
494+
},
495+
},
496+
},
462497
{
463498
description: "resized, no resize status",
464499
expectedRequests: v1.ResourceList{
@@ -486,6 +521,45 @@ func TestPodResourceRequests(t *testing.T) {
486521
},
487522
},
488523
},
524+
{
525+
description: "resized: per-resource 3-way maximum",
526+
expectedRequests: v1.ResourceList{
527+
v1.ResourceCPU: resource.MustParse("30m"),
528+
v1.ResourceMemory: resource.MustParse("30M"),
529+
// Note: EphemeralStorage is not resizable, but that doesn't matter for the purposes of this test.
530+
v1.ResourceEphemeralStorage: resource.MustParse("30G"),
531+
},
532+
options: PodResourcesOptions{UseStatusResources: true},
533+
containers: []v1.Container{
534+
{
535+
Name: "container-1",
536+
Resources: v1.ResourceRequirements{
537+
Requests: v1.ResourceList{
538+
v1.ResourceCPU: resource.MustParse("30m"),
539+
v1.ResourceMemory: resource.MustParse("20M"),
540+
v1.ResourceEphemeralStorage: resource.MustParse("10G"),
541+
},
542+
},
543+
},
544+
},
545+
containerStatus: []v1.ContainerStatus{
546+
{
547+
Name: "container-1",
548+
AllocatedResources: v1.ResourceList{
549+
v1.ResourceCPU: resource.MustParse("20m"),
550+
v1.ResourceMemory: resource.MustParse("10M"),
551+
v1.ResourceEphemeralStorage: resource.MustParse("30G"),
552+
},
553+
Resources: &v1.ResourceRequirements{
554+
Requests: v1.ResourceList{
555+
v1.ResourceCPU: resource.MustParse("10m"),
556+
v1.ResourceMemory: resource.MustParse("30M"),
557+
v1.ResourceEphemeralStorage: resource.MustParse("20G"),
558+
},
559+
},
560+
},
561+
},
562+
},
489563
{
490564
description: "resized, infeasible, but don't use status",
491565
expectedRequests: v1.ResourceList{

‎staging/src/k8s.io/kubectl/pkg/util/resource/resource.go‎

Lines changed: 7 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -144,28 +144,17 @@ func podLimits(pod *corev1.Pod) corev1.ResourceList {
144144
// determineContainerReqs will return a copy of the container requests based on if resizing is feasible or not.
145145
func determineContainerReqs(pod *corev1.Pod, container *corev1.Container, cs *corev1.ContainerStatus) corev1.ResourceList {
146146
if helpers.IsPodResizeInfeasible(pod) {
147-
return cs.Resources.Requests.DeepCopy()
147+
return max(cs.Resources.Requests, cs.AllocatedResources)
148148
}
149-
return max(container.Resources.Requests, cs.Resources.Requests)
149+
return max(container.Resources.Requests, cs.Resources.Requests, cs.AllocatedResources)
150150
}
151151

152-
// max returns the result of max(a, b) for each named resource and is only used if we can't
152+
// max returns the result of max(a, b...) for each named resource and is only used if we can't
153153
// accumulate into an existing resource list
154-
func max(a corev1.ResourceList, b corev1.ResourceList) corev1.ResourceList {
155-
result := corev1.ResourceList{}
156-
for key, value := range a {
157-
if other, found := b[key]; found {
158-
if value.Cmp(other) <= 0 {
159-
result[key] = other.DeepCopy()
160-
continue
161-
}
162-
}
163-
result[key] = value.DeepCopy()
164-
}
165-
for key, value := range b {
166-
if _, found := result[key]; !found {
167-
result[key] = value.DeepCopy()
168-
}
154+
func max(a corev1.ResourceList, b ...corev1.ResourceList) corev1.ResourceList {
155+
result := a.DeepCopy()
156+
for _, other := range b {
157+
maxResourceList(result, other)
169158
}
170159
return result
171160
}

‎test/compatibility_lifecycle/reference/versioned_feature_list.yaml‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -581,6 +581,10 @@
581581
lockToDefault: false
582582
preRelease: Alpha
583583
version: "1.32"
584+
- default: false
585+
lockToDefault: false
586+
preRelease: Deprecated
587+
version: "1.33"
584588
- name: InPlacePodVerticalScalingExclusiveCPUs
585589
versionedSpecs:
586590
- default: false

0 commit comments

Comments
 (0)