Remove terminating count from rmAtLeast

[kannon92]

What type of PR is this?

/kind bug

What this PR does / why we need it:

PodReplacementPolicy was enabled in 1.28 and it tracks the number of terminating pods. If Failed is specified, then we will halt replacement until the pod is fully terminal.

We discovered a bug where we were tracking (terminating + active) and using that index to access the active pods list. If there are more terminating pods than active, it is possible to hit an out of bounds error.

@mimowo pointed out that we don't need the terminating count as we don't need to delete the terminating pods.

This PR drops terminating so we are only counting active.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix panic in Job controller when podRecreationPolicy: Failed is used, and the number of terminating pods exceeds parallelism.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[k8s-ci-robot]

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[kannon92]

/cc @mimowo

With this PR, I am not sure we need #121009.

I'll verify that this functionally works today.

[kannon92]

/cc @mimowo

With this PR, I am not sure we need #121009.

I'll verify that this functionally works today.

I did a manual test to verify that the Failed case works. We have integration tests for this also but wanted to make sure.

[mimowo]

I think this is on the right track.

The reasoning that suggests me this is correct is as follows. The panic happens when
rmAtLeast > len(pods), that is rmAtLeast > active

Now, substituting active + terminating - wantActive for rmAtLeast we get
wantActive < terminating.

With the proposed change, we have active - wantActive > active, that is wantActive < 0 -> not possible :).

[mimowo]

Drop it.

[kannon92]

Drop the initial status? I get an error saying that StartTime is not set unless I have this here.

[kannon92]

Seems to depend on the parllelism of 4, dropping.

[mimowo]

LGTM, please update the PR description, with user-oriented release note, and some justification why this helps for the panic.

[alculquicondor]

/retest
(flaky #120080)

[alculquicondor]

/approve

[alculquicondor]

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 32b302f5924668c4eedc3e6567b040ef672ba027

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, kannon92

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/controller/job/OWNERS [alculquicondor]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[superbrothers]

We encountered the following panic. The situation occurred when some nodes on which Job pods were running became NotReady. We feel that maybe this bug will be fixed in this PR and would like you to consider cherry-picking to patch release.

kube-controller-manager version is 1.28.4 and PodRecreationPolicy feature gate is not enabled.

E1206 07:42:50.514010       1 runtime.go:79] Observed a panic: runtime.boundsError{x:5, y:0, signed:true, code:0x2} (runtime error: slice bounds out of range [:5] with capacity 0)
goroutine 3391 [running]:
k8s.io/apimachinery/pkg/util/runtime.logPanic({0x45ea720?, 0xc0f86db8f0})
     vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:75 +0x99
k8s.io/apimachinery/pkg/util/runtime.HandleCrash({0x0, 0x0, 0xc0a72ba240?})
     vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:49 +0x75
panic({0x45ea720, 0xc0f86db8f0})
     /usr/local/go/src/runtime/panic.go:884 +0x213
k8s.io/kubernetes/pkg/controller/job.activePodsForRemoval(0xc0fa589900?, {0x0?, 0x74aa080?, 0x74a1198?}, 0x5)
     pkg/controller/job/job_controller.go:1670 +0x26b
k8s.io/kubernetes/pkg/controller/job.(*Controller).manageJob(0xc0038360e0, {0x50b6fe0?, 0xc00042d130}, 0xc0fa589900, 0xc0f7fdcfc0)
     pkg/controller/job/job_controller.go:1500 +0x565
k8s.io/kubernetes/pkg/controller/job.(*Controller).syncJob(0xc0038360e0, {0x50b6fe0, 0xc00042d130}, {0xc0a6931b80, 0x3f})
     pkg/controller/job/job_controller.go:871 +0x18f8
k8s.io/kubernetes/pkg/controller/job.(*Controller).processNextWorkItem(0xc0038360e0, {0x50b6fe0, 0xc00042d130})
     pkg/controller/job/job_controller.go:589 +0x123
k8s.io/kubernetes/pkg/controller/job.(*Controller).worker(...)
     pkg/controller/job/job_controller.go:578
k8s.io/apimachinery/pkg/util/wait.JitterUntilWithContext.func1()
     vendor/k8s.io/apimachinery/pkg/util/wait/backoff.go:259 +0x25
k8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1(0x30?)
     vendor/k8s.io/apimachinery/pkg/util/wait/backoff.go:226 +0x3e
k8s.io/apimachinery/pkg/util/wait.BackoffUntil(0xc001461bc0?, {0x508ccc0, 0xc0aa5a2000}, 0x1, 0xc000de5440)
     vendor/k8s.io/apimachinery/pkg/util/wait/backoff.go:227 +0xb6
k8s.io/apimachinery/pkg/util/wait.JitterUntil(0x508ccc0?, 0x3b9aca00, 0x0, 0xa0?, 0x1000000000001f4?)
     vendor/k8s.io/apimachinery/pkg/util/wait/backoff.go:204 +0x89
k8s.io/apimachinery/pkg/util/wait.JitterUntilWithContext({0x50b6fe0, 0xc00042d130}, 0xc06957e860, 0xc0042d5fa0?, 0x170e606?, 0x20?)
     vendor/k8s.io/apimachinery/pkg/util/wait/backoff.go:259 +0x99
k8s.io/apimachinery/pkg/util/wait.UntilWithContext({0x50b6fe0?, 0xc00042d130?}, 0xc0004cc3a8?, 0xc0042d5fb8?)
     vendor/k8s.io/apimachinery/pkg/util/wait/backoff.go:170 +0x2b
created by k8s.io/kubernetes/pkg/controller/job.(*Controller).Run
     pkg/controller/job/job_controller.go:234 +0x40d
panic: runtime error: slice bounds out of range [:5] with capacity 0 [recovered]
     panic: runtime error: slice bounds out of range [:5] with capacity 0

[kannon92]

We encountered the following panic. The situation occurred when some nodes on which Job pods were running became NotReady. We feel that maybe this bug will be fixed in this PR and would like you to consider cherry-picking to patch release.
kube-controller-manager version is 1.28.4 and PodRecreationPolicy feature gate is not enabled.
panic: runtime error: slice bounds out of range [:5] with capacity 0 [recovered]
panic: runtime error: slice bounds out of range [:5] with capacity 0

Since the feature gate is not enabled, I don't know if this would actually fix this. Terminating should be 0 with this off. Ideally, we would have a test case where we can reproduce this.

[mimowo]

@superbrothers do you have a reproducible scenario for this? If so, it would be great if you can open a dedicated issue to have the discussion there. Ideally, with the yaml's and steps necessary to reproduce. For example, it can matter if this is IndexedJob, or not.

[superbrothers]

I will try to see if I can reproduce it, but it looks like I won't have time for a while.

[mimowo]

I will try to see if I can reproduce it, but it looks like I won't have time for a while.

Retrospectively describing as closely as possible, with the yamls to create the API objects might be a good start already to open the issue. In the meanwhile, was this a job with completionMode: Indexed, do you have the complete yaml? Also, was the job spec edited since created, for example to scale up or down the parallelism?

[superbrothers]

OK, I would create an issue first. The Job that I believe caused the problem was NonIndexed. But this may not be the cause (not sure, since a lot of jobs are created all the time).