Skip to content

Commit 6c5cf68

Browse files
vivzbansalvivzbansal
committed
Resolved latest review comments
1 parent 6cf5b80 commit 6c5cf68

File tree

15 files changed

+276
-685
lines changed

15 files changed

+276
-685
lines changed

‎pkg/api/pod/util.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1092,7 +1092,8 @@ func inPlacePodVerticalScalingInUse(podSpec *api.PodSpec) bool {
10921092
return false
10931093
}
10941094
var inUse bool
1095-
VisitContainers(podSpec, Containers, func(c *api.Container, containerType ContainerType) bool {
1095+
containersMask := Containers | InitContainers
1096+
VisitContainers(podSpec, containersMask, func(c *api.Container, containerType ContainerType) bool {
10961097
if len(c.ResizePolicy) > 0 {
10971098
inUse = true
10981099
return false
@@ -1297,7 +1298,7 @@ func MarkPodProposedForResize(oldPod, newPod *api.Pod) {
12971298
if c.Name != oldPod.Spec.Containers[i].Name {
12981299
return // Update is invalid (container mismatch): let validation handle it.
12991300
}
1300-
if c.Resources.Requests == nil || cmp.Equal(oldPod.Spec.Containers[i].Resources, c.Resources) {
1301+
if apiequality.Semantic.DeepEqual(oldPod.Spec.Containers[i].Resources, c.Resources) {
13011302
continue
13021303
}
13031304
newPod.Status.Resize = api.PodResizeStatusProposed
@@ -1310,7 +1311,7 @@ func MarkPodProposedForResize(oldPod, newPod *api.Pod) {
13101311
if c.Name != oldPod.Spec.InitContainers[i].Name {
13111312
return // Update is invalid (container mismatch): let validation handle it.
13121313
}
1313-
if c.Resources.Requests == nil || cmp.Equal(oldPod.Spec.InitContainers[i].Resources, c.Resources) {
1314+
if apiequality.Semantic.DeepEqual(oldPod.Spec.InitContainers[i].Resources, c.Resources) {
13141315
continue
13151316
}
13161317
newPod.Status.Resize = api.PodResizeStatusProposed

‎pkg/api/pod/util_test.go‎

Lines changed: 85 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -2950,13 +2950,13 @@ func TestDropSidecarContainers(t *testing.T) {
29502950
}
29512951

29522952
func TestMarkPodProposedForResize(t *testing.T) {
2953+
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SidecarContainers, true)
29532954
containerRestartPolicyAlways := api.ContainerRestartPolicyAlways
29542955
testCases := []struct {
29552956
desc string
29562957
newPodSpec api.PodSpec
29572958
oldPodSpec api.PodSpec
29582959
expectProposedResize bool
2959-
hasSidecarContainer bool
29602960
}{
29612961
{
29622962
desc: "nil requests",
@@ -3211,8 +3211,7 @@ func TestMarkPodProposedForResize(t *testing.T) {
32113211
expectProposedResize: false,
32123212
},
32133213
{
3214-
desc: "resources unchanged with sidecar containers",
3215-
hasSidecarContainer: true,
3214+
desc: "resources unchanged with sidecar containers",
32163215
newPodSpec: api.PodSpec{
32173216
Containers: []api.Container{
32183217
{
@@ -3262,8 +3261,7 @@ func TestMarkPodProposedForResize(t *testing.T) {
32623261
expectProposedResize: false,
32633262
},
32643263
{
3265-
desc: "requests resized with sidecar containers",
3266-
hasSidecarContainer: true,
3264+
desc: "requests resized with sidecar containers",
32673265
newPodSpec: api.PodSpec{
32683266
Containers: []api.Container{
32693267
{
@@ -3274,28 +3272,47 @@ func TestMarkPodProposedForResize(t *testing.T) {
32743272
Limits: api.ResourceList{api.ResourceCPU: resource.MustParse("200m")},
32753273
},
32763274
},
3275+
},
3276+
InitContainers: []api.Container{
32773277
{
3278-
Name: "c2",
3278+
Name: "i1",
32793279
Image: "image",
32803280
Resources: api.ResourceRequirements{
32813281
Requests: api.ResourceList{api.ResourceCPU: resource.MustParse("300m")},
32823282
Limits: api.ResourceList{api.ResourceCPU: resource.MustParse("400m")},
32833283
},
3284+
RestartPolicy: &containerRestartPolicyAlways,
3285+
},
3286+
},
3287+
},
3288+
oldPodSpec: api.PodSpec{
3289+
Containers: []api.Container{
3290+
{
3291+
Name: "c1",
3292+
Image: "image",
3293+
Resources: api.ResourceRequirements{
3294+
Requests: api.ResourceList{api.ResourceCPU: resource.MustParse("100m")},
3295+
Limits: api.ResourceList{api.ResourceCPU: resource.MustParse("200m")},
3296+
},
32843297
},
32853298
},
32863299
InitContainers: []api.Container{
32873300
{
32883301
Name: "i1",
32893302
Image: "image",
32903303
Resources: api.ResourceRequirements{
3291-
Requests: api.ResourceList{api.ResourceCPU: resource.MustParse("300m")},
3292-
Limits: api.ResourceList{api.ResourceCPU: resource.MustParse("400m")},
3304+
Requests: api.ResourceList{api.ResourceCPU: resource.MustParse("200m")},
3305+
Limits: api.ResourceList{api.ResourceCPU: resource.MustParse("300m")},
32933306
},
32943307
RestartPolicy: &containerRestartPolicyAlways,
32953308
},
32963309
},
32973310
},
3298-
oldPodSpec: api.PodSpec{
3311+
expectProposedResize: true,
3312+
},
3313+
{
3314+
desc: "limits resized with sidecar containers",
3315+
newPodSpec: api.PodSpec{
32993316
Containers: []api.Container{
33003317
{
33013318
Name: "c1",
@@ -3305,22 +3322,37 @@ func TestMarkPodProposedForResize(t *testing.T) {
33053322
Limits: api.ResourceList{api.ResourceCPU: resource.MustParse("200m")},
33063323
},
33073324
},
3325+
},
3326+
InitContainers: []api.Container{
33083327
{
3309-
Name: "c2",
3328+
Name: "i1",
33103329
Image: "image",
33113330
Resources: api.ResourceRequirements{
3312-
Requests: api.ResourceList{api.ResourceCPU: resource.MustParse("200m")},
3331+
Requests: api.ResourceList{api.ResourceCPU: resource.MustParse("300m")},
33133332
Limits: api.ResourceList{api.ResourceCPU: resource.MustParse("400m")},
33143333
},
3334+
RestartPolicy: &containerRestartPolicyAlways,
3335+
},
3336+
},
3337+
},
3338+
oldPodSpec: api.PodSpec{
3339+
Containers: []api.Container{
3340+
{
3341+
Name: "c1",
3342+
Image: "image",
3343+
Resources: api.ResourceRequirements{
3344+
Requests: api.ResourceList{api.ResourceCPU: resource.MustParse("100m")},
3345+
Limits: api.ResourceList{api.ResourceCPU: resource.MustParse("200m")},
3346+
},
33153347
},
33163348
},
33173349
InitContainers: []api.Container{
33183350
{
33193351
Name: "i1",
33203352
Image: "image",
33213353
Resources: api.ResourceRequirements{
3322-
Requests: api.ResourceList{api.ResourceCPU: resource.MustParse("200m")},
3323-
Limits: api.ResourceList{api.ResourceCPU: resource.MustParse("300m")},
3354+
Requests: api.ResourceList{api.ResourceCPU: resource.MustParse("300m")},
3355+
Limits: api.ResourceList{api.ResourceCPU: resource.MustParse("500m")},
33243356
},
33253357
RestartPolicy: &containerRestartPolicyAlways,
33263358
},
@@ -3329,8 +3361,7 @@ func TestMarkPodProposedForResize(t *testing.T) {
33293361
expectProposedResize: true,
33303362
},
33313363
{
3332-
desc: "limits resized with sidecar containers",
3333-
hasSidecarContainer: true,
3364+
desc: "requests resized should fail with non-sidecar init container",
33343365
newPodSpec: api.PodSpec{
33353366
Containers: []api.Container{
33363367
{
@@ -3341,28 +3372,45 @@ func TestMarkPodProposedForResize(t *testing.T) {
33413372
Limits: api.ResourceList{api.ResourceCPU: resource.MustParse("200m")},
33423373
},
33433374
},
3375+
},
3376+
InitContainers: []api.Container{
33443377
{
3345-
Name: "c2",
3378+
Name: "i1",
33463379
Image: "image",
33473380
Resources: api.ResourceRequirements{
33483381
Requests: api.ResourceList{api.ResourceCPU: resource.MustParse("300m")},
33493382
Limits: api.ResourceList{api.ResourceCPU: resource.MustParse("400m")},
33503383
},
33513384
},
33523385
},
3386+
},
3387+
oldPodSpec: api.PodSpec{
3388+
Containers: []api.Container{
3389+
{
3390+
Name: "c1",
3391+
Image: "image",
3392+
Resources: api.ResourceRequirements{
3393+
Requests: api.ResourceList{api.ResourceCPU: resource.MustParse("100m")},
3394+
Limits: api.ResourceList{api.ResourceCPU: resource.MustParse("200m")},
3395+
},
3396+
},
3397+
},
33533398
InitContainers: []api.Container{
33543399
{
33553400
Name: "i1",
33563401
Image: "image",
33573402
Resources: api.ResourceRequirements{
3358-
Requests: api.ResourceList{api.ResourceCPU: resource.MustParse("300m")},
3359-
Limits: api.ResourceList{api.ResourceCPU: resource.MustParse("400m")},
3403+
Requests: api.ResourceList{api.ResourceCPU: resource.MustParse("200m")},
3404+
Limits: api.ResourceList{api.ResourceCPU: resource.MustParse("300m")},
33603405
},
3361-
RestartPolicy: &containerRestartPolicyAlways,
33623406
},
33633407
},
33643408
},
3365-
oldPodSpec: api.PodSpec{
3409+
expectProposedResize: false,
3410+
},
3411+
{
3412+
desc: "limits resized should fail with non-sidecar init containers",
3413+
newPodSpec: api.PodSpec{
33663414
Containers: []api.Container{
33673415
{
33683416
Name: "c1",
@@ -3372,12 +3420,26 @@ func TestMarkPodProposedForResize(t *testing.T) {
33723420
Limits: api.ResourceList{api.ResourceCPU: resource.MustParse("200m")},
33733421
},
33743422
},
3423+
},
3424+
InitContainers: []api.Container{
33753425
{
3376-
Name: "c2",
3426+
Name: "i1",
33773427
Image: "image",
33783428
Resources: api.ResourceRequirements{
33793429
Requests: api.ResourceList{api.ResourceCPU: resource.MustParse("300m")},
3380-
Limits: api.ResourceList{api.ResourceCPU: resource.MustParse("500m")},
3430+
Limits: api.ResourceList{api.ResourceCPU: resource.MustParse("400m")},
3431+
},
3432+
},
3433+
},
3434+
},
3435+
oldPodSpec: api.PodSpec{
3436+
Containers: []api.Container{
3437+
{
3438+
Name: "c1",
3439+
Image: "image",
3440+
Resources: api.ResourceRequirements{
3441+
Requests: api.ResourceList{api.ResourceCPU: resource.MustParse("100m")},
3442+
Limits: api.ResourceList{api.ResourceCPU: resource.MustParse("200m")},
33813443
},
33823444
},
33833445
},
@@ -3389,11 +3451,10 @@ func TestMarkPodProposedForResize(t *testing.T) {
33893451
Requests: api.ResourceList{api.ResourceCPU: resource.MustParse("300m")},
33903452
Limits: api.ResourceList{api.ResourceCPU: resource.MustParse("500m")},
33913453
},
3392-
RestartPolicy: &containerRestartPolicyAlways,
33933454
},
33943455
},
33953456
},
3396-
expectProposedResize: true,
3457+
expectProposedResize: false,
33973458
},
33983459
{
33993460
desc: "the number of sidecar containers in the pod has increased; no action should be taken.",
@@ -3524,7 +3585,6 @@ func TestMarkPodProposedForResize(t *testing.T) {
35243585
}
35253586
for _, tc := range testCases {
35263587
t.Run(tc.desc, func(t *testing.T) {
3527-
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SidecarContainers, tc.hasSidecarContainer)
35283588
newPod := &api.Pod{Spec: tc.newPodSpec}
35293589
newPodUnchanged := newPod.DeepCopy()
35303590
oldPod := &api.Pod{Spec: tc.oldPodSpec}

‎pkg/apis/core/v1/defaults.go‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -183,8 +183,7 @@ func SetDefaults_Pod(obj *v1.Pod) {
183183
}
184184
}
185185
}
186-
if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) &&
187-
(obj.Spec.Containers[i].Resources.Requests != nil || obj.Spec.Containers[i].Resources.Limits != nil) {
186+
if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) {
188187
// For normal containers, set resize restart policy to default value (NotRequired), if not specified..
189188
setDefaultResizePolicy(&obj.Spec.Containers[i])
190189
}
@@ -201,12 +200,7 @@ func SetDefaults_Pod(obj *v1.Pod) {
201200
}
202201
}
203202
if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) && utilfeature.DefaultFeatureGate.Enabled(features.SidecarContainers) {
204-
isRestartableInitContainer := false
205-
c := obj.Spec.InitContainers[i]
206-
if c.RestartPolicy != nil && *c.RestartPolicy == v1.ContainerRestartPolicyAlways {
207-
isRestartableInitContainer = true
208-
}
209-
if isRestartableInitContainer && (c.Resources.Requests != nil || c.Resources.Limits != nil) {
203+
if obj.Spec.InitContainers[i].RestartPolicy != nil && *obj.Spec.InitContainers[i].RestartPolicy == v1.ContainerRestartPolicyAlways {
210204
// For restartable init containers, set resize restart policy to default value (NotRequired), if not specified.
211205
setDefaultResizePolicy(&obj.Spec.InitContainers[i])
212206
}
@@ -232,6 +226,9 @@ func SetDefaults_Pod(obj *v1.Pod) {
232226

233227
// setDefaultResizePolicy set resize restart policy to default value (NotRequired), if not specified.
234228
func setDefaultResizePolicy(obj *v1.Container) {
229+
if obj.Resources.Requests == nil && obj.Resources.Limits == nil {
230+
return
231+
}
235232
resizePolicySpecified := make(map[v1.ResourceName]bool)
236233
for _, p := range obj.ResizePolicy {
237234
resizePolicySpecified[p.ResourceName] = true

‎pkg/apis/core/v1/defaults_test.go‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2985,6 +2985,7 @@ func TestSetDefaultServiceInternalTrafficPolicy(t *testing.T) {
29852985
func TestSetDefaultResizePolicy(t *testing.T) {
29862986
// verify we default to NotRequired restart policy for resize when resources are specified
29872987
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.InPlacePodVerticalScaling, true)
2988+
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SidecarContainers, true)
29882989
restartAlways := v1.ContainerRestartPolicyAlways
29892990
for desc, tc := range map[string]struct {
29902991
testContainer v1.Container
@@ -3184,14 +3185,12 @@ func TestSetDefaultResizePolicy(t *testing.T) {
31843185
} {
31853186
for _, isSidecarContainer := range []bool{true, false} {
31863187
t.Run(desc, func(t *testing.T) {
3187-
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SidecarContainers, isSidecarContainer)
3188-
if isSidecarContainer {
3189-
tc.testContainer.RestartPolicy = &restartAlways
3190-
}
31913188
testPod := v1.Pod{}
3192-
testPod.Spec.Containers = append(testPod.Spec.Containers, tc.testContainer)
31933189
if isSidecarContainer {
3190+
tc.testContainer.RestartPolicy = &restartAlways
31943191
testPod.Spec.InitContainers = append(testPod.Spec.InitContainers, tc.testContainer)
3192+
} else {
3193+
testPod.Spec.Containers = append(testPod.Spec.Containers, tc.testContainer)
31953194
}
31963195
output := roundTrip(t, runtime.Object(&testPod))
31973196
pod2 := output.(*v1.Pod)

‎pkg/kubelet/kubelet.go‎

Lines changed: 8 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -2812,47 +2812,6 @@ func (kl *Kubelet) HandlePodSyncs(pods []*v1.Pod) {
28122812
}
28132813
}
28142814

2815-
func isPodResizeInProgress(pod *v1.Pod, podStatus *kubecontainer.PodStatus) bool {
2816-
for i := range pod.Spec.Containers {
2817-
if containerResourcesChanged(&pod.Spec.Containers[i], podStatus) {
2818-
return true
2819-
}
2820-
}
2821-
2822-
if utilfeature.DefaultFeatureGate.Enabled(features.SidecarContainers) {
2823-
for i, c := range pod.Spec.InitContainers {
2824-
if podutil.IsRestartableInitContainer(&c) {
2825-
if containerResourcesChanged(&pod.Spec.InitContainers[i], podStatus) {
2826-
return true
2827-
}
2828-
}
2829-
}
2830-
}
2831-
return false
2832-
}
2833-
2834-
func containerResourcesChanged(c *v1.Container, podStatus *kubecontainer.PodStatus) bool {
2835-
if cs := podStatus.FindContainerStatusByName(c.Name); cs != nil {
2836-
if cs.State != kubecontainer.ContainerStateRunning || cs.Resources == nil {
2837-
return false
2838-
}
2839-
if c.Resources.Requests != nil {
2840-
if cs.Resources.CPURequest != nil && !cs.Resources.CPURequest.Equal(*c.Resources.Requests.Cpu()) {
2841-
return true
2842-
}
2843-
}
2844-
if c.Resources.Limits != nil {
2845-
if cs.Resources.CPULimit != nil && !cs.Resources.CPULimit.Equal(*c.Resources.Limits.Cpu()) {
2846-
return true
2847-
}
2848-
if cs.Resources.MemoryLimit != nil && !cs.Resources.MemoryLimit.Equal(*c.Resources.Limits.Memory()) {
2849-
return true
2850-
}
2851-
}
2852-
}
2853-
return false
2854-
}
2855-
28562815
// canResizePod determines if the requested resize is currently feasible.
28572816
// pod should hold the desired (pre-allocated) spec.
28582817
// Returns true if the resize can proceed.
@@ -2947,6 +2906,14 @@ func (kl *Kubelet) handlePodResourcesResize(pod *v1.Pod, podStatus *kubecontaine
29472906
kl.backOff.Reset(key)
29482907
}
29492908
}
2909+
for i, container := range pod.Spec.InitContainers {
2910+
if podutil.IsRestartableInitContainer(&container) {
2911+
if !apiequality.Semantic.DeepEqual(container.Resources, allocatedPod.Spec.InitContainers[i].Resources) {
2912+
key := kuberuntime.GetStableKey(pod, &container)
2913+
kl.backOff.Reset(key)
2914+
}
2915+
}
2916+
}
29502917
allocatedPod = pod
29512918

29522919
// Special case when the updated allocation matches the actual resources. This can occur

0 commit comments

Comments
 (0)