Add a way for JobSet's failure policy rules to match `pod.status.reason`

[GiuseppeTT]

What would you like to be added:

A way for JobSet's failure policy rules to match pod.status.reason.

Why is this needed:

Currently, JobSet's failure policy rules (jobset.spec.failurePolicy.rules) only act on Job failure reasons (job.status.conditions[].reason). While a Job's Pod failure policy (job.spec.podFailurePolicy) can propagate Pod failures to the Job level, it is limited to matching on:

• pod.status.conditions[].type
• pod.status.conditions[].status
• pod.status.containerStatuses[].state.terminated.name
• pod.status.containerStatuses[].state.terminated.exitCode
• pod.status.initContainerStatuses[].state.terminated.name
• pod.status.initContainerStatuses[].state.terminated.exitCode

This becomes a problem when Pods fail with a minimal status like the following (observed in production)

status:
  phase: "Failed"
  reason: "UnexpectedAdmissionError"
  message: ...
  qosClass: ...
  startTime: ...

Since there are no fields pod.status.conditions[] and pod.status.containerStatuses[], there is no way for the Job's Pod failure policy to bubble up the failure reason to JobSet's failure policy to allow for things such as restarting the JobSet without counting towards maxRestarts.

Possible solutions:

• (1) Make kubelet add a Pod condition for UnexpectedAdmissionError
  • This is the probably the ideal solution, but it is also the most complex and the slowest to deliver
• (2) Make the Job API support matching pod.status.reason
  • This is simpler than changing kubelet and would be faster to deliver, but it is probably still too slow (besides implementation, it would require a KEP and 2~3 Kubernetes release cycles)
• (3) Make the JobSet API support matching pod.status.reason
  • This is probably not the ideal soltuion, but pragramatically it would be the fastest to deliver (besides implementation, it would require updating the failure policy KEP and one JobSet release cycle)

This enhancement requires the following artifacts:

• Design doc (technically only update the failure policy KEP)
• API change
• Docs update

[kannon92]

Rather than debating on what is the easiest solution, what is the ideal solution you'd like here? We should work backwards on that and implement that solution. I wouldn't focus on what is the easiest to deliver.

For 1, we could get the input from @haircommander, @SergeyKanzhelev or @harche. Adding Kubelet conditions has been done recently (https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/3085-pod-conditions-for-starting-completition-of-sandbox-creation). I'm not sure if these issues are due to device plugin failures in admission. Maybe @ffromani has some thoughts on ways to improve the UX for pods if there are admission failures. I'm not sure if there is a condition that exposes failures here but I think this may be useful.

I don't quite follow how pods get into this state from your issue but adding error information into conditions make sense to me here.

For 2, we could get the inputs from @mimowo or @tenzen-y.

Ideally JobSet should leverage PodFailurePolicy as much as possible. That will make the ecosystem more stable and provide good UX for workload controllers (rely on conditions).

If we can implement 3 with also leading efforts for 1 and 2, I am happy to see a KEP update here. But I don't want to get into a half measure that we have to support because we focus on getting a substandard solution into JobSet.

But maybe with 2 there is a consideration to expand PodFailurePolicy to look at reason in the pod API. If so I think 3 would not be a bad solution.

[mimowo]

+1 for trying to go with 1+2 (long term) . Short term I would suggest JobSet supporting matching of PodFailurePolicy messages (with regexes) rather than reasons. wdyt?

[kannon92]

Short term I would suggest JobSet supporting matching of PodFailurePolicy messages (with regexes) rather than reasons. wdyt?

I believe that was implemented in 0.10 (#1033)

[ahg-g]

I am not sure how would option #3 work, jobset in principle doesn't track individual pods failures, it monitors job failures. Option #2 seems the most aligned with jobset design principles

[GiuseppeTT]

Short term I would suggest JobSet supporting matching of PodFailurePolicy messages (with regexes) rather than reasons. wdyt?

As Kevin mentioned, this has already been implemented. Unfourtunately, this solution would not work because the Job failure condition is also very generic. Something like

conditions:
- status: "True"
  type: "FailureTarget"
  reason: "MaxFailedIndexesExceeded"
  message: "Job has exceeded the specified maximal number of failed indexes"
  lastProbeTime: ...
  lastTransitionTime: ...

[GiuseppeTT]

Rather than debating on what is the easiest solution, what is the ideal solution you'd like here? We should work backwards on that and implement that solution. I wouldn't focus on what is the easiest to deliver.

IMO the ideal solution here is (1). That is, make Kubelet more transparent about the state of the Pod by adding an appropriate condition to the Pod manifest when the Pod is failed with reason MaxFailedIndexesExceeded UnexpectedAdmissionError. This solution solves the problem and is basically an observability improvement with 0 changes to the Job and JobSet API.

@SergeyKanzhelev do you have any opinions on this solution and the feasibility of it?

If we can implement 3 with also leading efforts for 1 and 2, I am happy to see a KEP update here. But I don't want to get into a half measure that we have to support because we focus on getting a substandard solution into JobSet.

I agree. Let's focus on (1). If we really need a faster solution, then we can consider (3) (but without doing (3))

[kannon92]

I don't think there is anything in kubelet called MaxFailedIndexesExceeded.

You would want to look at what failures you see on the pods and create a condition from that. I like the idea of AdmissionErrors.

Do you have any data on what pod failure your users experience? That may help figure out what kind of errors you need to detect.

[GiuseppeTT]

I don't think there is anything in kubelet called MaxFailedIndexesExceeded.

My bad, I meant UnexpectedAdmissionError. I updated the comment.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale