I disagree. We will always need the annotation because Pods can be recreated, and we want to count failures across recreations.
But definitely the Job controller can benefit from this feature by setting different maxRestarts every time a pod is recreated.

I think a better "pro" is the fact that a container can be restarted in the kubelet much faster than a pod recreation.

OK, the implementation details of BackoffLimitPerIndex is something beyond the KEP.

I actually agree with Aldo.
It's not an implementation detail - it's a fundamental thing of "pod recreations". If we want to track something across pod recreations (which is the case for jobs), maxRestarts won't solve it on its own - but actually may help with it.

OK, the implementation details of BackoffLimitPerIndex is something beyond the KEP.

It's not currently stated in the Non-Goals. But I think doing so would be a mistake. Even if we don't have the full semantics for how the pod level restart limit would work with backoffLimit or backoffLimitPerIndex, we should have some idea of how they could work together. Otherwise we could end up in another scenario where features work independently, but when paired together are mostly useless or have very strange semantics that are hard for users to understand, and hard to maintain.

Otherwise we could end up in another scenario where features work independently, but when paired together are mostly useless or have very strange semantics that are hard for users to understand, and hard to maintain.

I like this analysis.

Why is this a con? We can just validate that restartPolicy!=Never.

Here's talking about the cons when supporting restartPolicy=Never.

I guess you are talking about the case when parallelism=1.

I'm not liking these semantics, even if they are new. The backoffLimit in Job is already not accurate when restartPolicy=OnFailure https://kubernetes.io/docs/concepts/workloads/controllers/job/#pod-backoff-failure-policy and kubernetes/kubernetes#109870

Note that when Pod is marked as Failed (as it will be when the Pod reaches maxRestartTimes), the logic would count the number of failures as 1, regardless of the number of container restarts. I know, it's ugly, but it's the legacy behavior. This might not give the desired outcome for users of Job.

What if when if the job controller observes the failure reason/condition for "reached max restarts", it would add this to the number of failures and count towards backoffLimit. You need to consider finalizers, but maybe it's doable?

We could also consider not allowing users of Job to set maxRestartTimes and let the job controller add it, in behalf of the user, when creating a Pod. But how does a user tell the Job API whether to use the old counting method or use maxRestartTimes?

So, overall, I think this section needs to consider some alternatives to deal with Job backoffLimit, for different levels of parallelism.

cc @mimowo @soltysh

I have a different proposal - when a user specifies maxRestartTimes, then backoffLimit should only count pod when they are Failed, instead of checking individual containers.

This puts a clear separation of concerns between kubelet and Job controller, eliminating the issue kubernetes/kubernetes#109870.

In this proposal we could support counting across pod recreations by a dedicated Pod annotation which specifies how many pod restarts happened to this point. Job controller could set it based on the previous pod, and Kubelet subtract from maxRestartTimes.

The issue I see with setting maxRestartTimes based on backoffLimit is that it is per-pod, while backoffLimit is global.

So, another proposal would be that Job controller sets maxRestartTimes but based on the backoffLimitPerIndex (and the annotation counting how many failures happened before recreation).

I have a different proposal - when a user specifies maxRestartTimes, then backoffLimit should only count pod when they are Failed, instead of checking individual containers.

I think this is the easiest solution. But would that be useful to users of Job? That I'm not sure about.

So, another proposal would be that Job controller sets maxRestartTimes but based on the backoffLimitPerIndex (and the annotation counting how many failures happened before recreation).

In the case of backoffLimitPerIndex it's much easier. The job controller would create the first Pod with full maxPodRestarts. If the Pod is preempted/evicted/etc, the replacement pod has a lower maxPodRestarts.

I'm not too sure what do do with regular backoffLimit... Maybe going with the easiest solution above is the best we can do?

Actually, with the approach both backoffLimit and backoffLimitPerIndex can be implemented very similarly to as currently when restartPolicy=Never. We only count failed pods (not container restarts), we don't even need the annotation. Simply, user sets maxRestartTimes once the pod reaches the Failed phase it is counted towards the backoff limits.

EDIT: with the setting of maxRestartTimes we know that the pod will eventually be terminal, which isn't the case currently.

I like the idea that the only difference between backoffLimit and backoffLimitPerIndex is that one counts failures globally, and the other per index.

I'm supportive of the idea of a global backoffLimit and local maxRestarts, which allows granular control over your job. For Indexed Jobs where you set backoffLimitPerIndex it seems reasonable that we'll copy that value into that new field maxRestarts to get rid of the extra counting we currently have from job controller. On the other hand we clearly excluded OnFailure from that other KEP, but the discussions here are around having a global mechanism, so how this affects backoffLimitPerIndex are still to be decided once we have a crisp idea how this proposals finishes. #3850 is currently in alpha, I'm inclined to block GA of it until we have this feature alpha or ideally beta.

ndeed the number of restarts when backoffLimitPerIndex is specified along with maxRestartTimes would be: maxRestartTimes * (backoffLimitPerIndex+1)

The problem is that this is not always accurate. If a Pod is evicted before it completes its maxRestartTimes, then the equation no longer applies. So overall I think this makes the feature highly unpredictable.

I like the idea of making #3850 GA depend on beta for #3322. But I really see it as the job controller using the field maxRestarts, rather than letting the user set it up.

In practice, what this means for the purpose of #3322 going to alpha, is that we should have a validation rule in Job that forbids users from setting maxRestarts for Indexed Jobs (or something along those lines).

I'm not convinced about blocking graduation of #3850, which is technically scoped to Never so there is no direct interaction, unless we want to extend its scope.

I see these approaches:

1. limit 3850 to Never, and support maxRestarts with backoffLimitPerIndex and backoffLimit when OnFailure in a dedicated KEP - that KEP could also enable it for podFailurePolicy
2. support OnFailure in 3850: maxRestarts with backoffLimitPerIndex when OnFailure , but without backoffLimit or podFailurePolicy
3. support OnFailure in 3850: maxRestarts with backoffLimitPerIndex when OnFailure, along with backoffLimit (and possibly podFailurePolicy.

My preference is for (1.) to keep KEPs small and graduating fast, but I'm ok with (2.). My preference for (1.) is that when approaching (2.) we may scope creep to (3.), and possibly pod failure policy.

Thus, I feel like there is room for a dedicated KEP focused on enabling OnFailure with maxRestarts for backoffLimitPerIndex, backoffLimit and pod failure policy. The need for pod failure policy was also discussed here: #3329 (comment).

I actually agree with Aldo.
It's not an implementation detail - it's a fundamental thing of "pod recreations". If we want to track something across pod recreations (which is the case for jobs), maxRestarts won't solve it on its own - but actually may help with it.

+1 to Tim - I think that generalizing it to "Always" is natural and instead of making the API narrow, let's make it more generic.

I don't understand it.

FWUW - this section isn't necessary for Alpha so given time bounds you may want to delete your answers from rollout and monitoring sections as their answers are controversial...

What I mean here is when upgrading api servers, we'll wait until all the apiservers are ready, then upgrade the kubelet. So if feature gates are enabled on some apiservers, we'll do nothing. Is this reasonable? Or what we want here is all the possibilities not the best practices, since it said as paranoid as possible. cc @wojtek-t

Removed for alpha

I would just comment out this section.

need to find an approver. Without approver defined we unlikely can take it.

@mrunalp @derekwaynecarr @dchen1107 any of you want to take it?

two questions

1. if Pod's RestartPolicy is OnFailure and maxRestartTimesOnFailure is 0, invalid? or means Never?
2. is the maxRestartTimesOnFailure editable for the pod?

IMO:

1. if restartPolicy is "OnFailure" and maxRestarts is 0, it is effectively "Never". I don't think we need to special-case 0 to be a failure, but I don't feel strongly and could be argued either way.
2. let's start with "no" and see if there's really a need?

Yeah, it feels like never to me too.

Otherwise we could end up in another scenario where features work independently, but when paired together are mostly useless or have very strange semantics that are hard for users to understand, and hard to maintain.

I like this analysis.

But it's not the same pod. Job is a higher-level contruct.

Yes, it feels like a conflation of a pod-level maxRestarts and a job-level maxRecreates or something.

Yeah, it feels like never to me too.

Pod.spec.containers[].maxRestarts reads well to me.

But it's a little weird because Containers also have a RestartPolicy and the only allowed value is Always. It would stop being intuitive how the attribute interactions actually work because now we're intermingling pod level attributes with container level attributes.

Does this enhancement involve coordinating behavior in the control plane and in the kubelet? How does an n-2 kubelet without this feature available behave when this feature is used?

In other words, if we have a kubelet at 1.26 and a kube-apiserver at 1.29 with the feature enabled, what is the expected behavior?

Will any other components on the node change? For example, changes to CSI, CRI or CNI may require updating that component before the kubelet.

I believe the answer to this should be no.

Is this a new or existing metric?

I would just comment out this section.