[YUNIKORN-657] Expose application failure reasons #258
Conversation
ghost
mentioned this pull request
Codecov Report
@@ Coverage Diff @@
## master #258 +/- ##
==========================================
+ Coverage 59.75% 60.65% +0.90%
==========================================
Files 35 36 +1
Lines 3133 3342 +209
==========================================
+ Hits 1872 2027 +155
- Misses 1180 1232 +52
- Partials 81 83 +2
Continue to review full report at Codecov.
|
pkg/client/kubeclient.go
Outdated
| func (nc SchedulerKubeClient) Update(pod *v1.Pod) (*v1.Pod, error) { | ||
| var updatedPod *v1.Pod | ||
| var err error | ||
| if updatedPod, err = nc.clientSet.CoreV1().Pods(pod.Namespace).UpdateStatus(pod); err != nil { | ||
| log.Logger().Warn("failed to update pod", | ||
| zap.String("namespace", pod.Namespace), | ||
| zap.String("podName", pod.Name), | ||
| zap.Error(err)) | ||
| return pod, err | ||
| } | ||
| return updatedPod, nil | ||
| } |
There was a problem hiding this comment.
The function is called "Update" but the actual impl is "UpdateStatus".
I think we should either re-implement this with "nc.clientSet.CoreV1().Pods(pod.Namespace).Update()" or change this function name to "UpdateStatus" to avoid mis-usage.
There was a problem hiding this comment.
Good catch. Changed to UpdateStatus
pkg/common/utils/utils.go
Outdated
| if pod.Status.Phase == v1.PodFailed && pod.Status.Reason == constants.ApplicationRejectedFailure || pod.Status.Reason == constants.ApplicationInsufficientResourcesFailure { | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
Can we simply return false here when pod phase is Failed?
Do we have to check the reason?
There was a problem hiding this comment.
You are right. Removed reason check
pkg/shim/scheduler.go
Outdated
| var err error | ||
| if len(se.GetArgs()) >= 1 { | ||
| eventArgs := make([]string, 2) | ||
| if err = events.GetEventArgsAsStrings(eventArgs, se.GetArgs()); err != nil { | ||
| log.Logger().Error("fail to parse event arg", zap.Error(err)) | ||
| } | ||
| message := eventArgs[1] | ||
| err = ss.stateMachine.Event(string(se.GetEvent()), message) | ||
| } else { | ||
| err = ss.stateMachine.Event(string(se.GetEvent())) | ||
| } | ||
| if err != nil && err.Error() == "no transition" { |
There was a problem hiding this comment.
Can we use a more general form? The interface looks like
type SchedulerEvent interface {
GetEvent() SchedulerEventType
GetArgs() []interface{}
}
Can we leverage the args here:
err = ss.stateMachine.Event(string(se.GetEvent()), se.GetArgs())
There was a problem hiding this comment.
Good point. Updated accordingly. Now it's more generic and cleaner 👍
|
Overall looks good, thanks for improving this! Please see some minor comments, thank you @yuchaoran-apple . |
yangwwei
changed the title
ghost
deleted the
When the application transits to failed state, publish pod events to all pods of this application to indicate why the application was failed.
What is this PR for?
When an application fails or is rejected by the scheduler, the pods currently stay in pending forever without visibility on what happened to them. This PR propagates the scheduling error or failure to the pods so that the user will be able to see the reason of failure. If third-party operators (e.g. Spark K8s Operator) are used, they can also react according to the pod status change and update the status of the corresponding CRD and possibly retry. Core side changes are in apache/yunikorn-core#271
What type of PR is it?
Todos
What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-657
How should this be tested?
Added unit tests to cover this change. When running in a cluster, tested that scheduling failures will be reflected on the pods:
For example, if an application is rejected, its pods will be marked as failed and its
statusfield will be populated with the following information:Another example is that if a gang-scheduled application cannot successfully run all placeholders before they expire, the app pods'
statusfield will show:Screenshots (if appropriate)
Questions: