yangwwei commented on a change in pull request #250:
URL:
https://github.com/apache/incubator-yunikorn-core/pull/250#discussion_r585277655
##########
File path: pkg/scheduler/objects/application.go
##########
@@ -225,6 +234,56 @@ func (sa *Application) clearStateTimer() {
zap.String("state", sa.stateMachine.Current()))
}
+func (sa *Application) isWaitingStateTimedOut() bool {
+ return sa.IsWaiting() && sa.stateTimer == nil
+}
+func (sa *Application) initPlaceholderTimer() {
+ if sa.placeholderTimer != nil || !sa.IsAccepted() || sa.execTimeout <=
0 {
+ return
+ }
+ log.Logger().Debug("Application placeholder timer initiated",
+ zap.String("AppID", sa.ApplicationID),
+ zap.Duration("Timeout", sa.execTimeout))
+ sa.placeholderTimer = time.AfterFunc(sa.execTimeout,
sa.timeoutPlaceholderProcessing)
+}
+
+func (sa *Application) clearPlaceholderTimer() {
+ if sa == nil || sa.placeholderTimer == nil {
+ return
+ }
+ sa.placeholderTimer.Stop()
+ sa.placeholderTimer = nil
+}
+
+func (sa *Application) timeoutPlaceholderProcessing() {
Review comment:
Why do we need to handle different cases?
In my opinion, at the app level, we simply do start the placeholderTimer
(when we get the first allocation) and stop it when the condition reaches: 1)
all placeholders are replaced; 2) app transited to completed;
when the timer times out, the handling is very simple, we just clean up all
the existing placeholder allocations and pending placeholder asks.
##########
File path: pkg/scheduler/objects/application.go
##########
@@ -1015,8 +1072,11 @@ func (sa *Application) GetAllAllocations() []*Allocation
{
// get a copy of all placeholder allocations of the application
// No locking must be called while holding the lock
-func (sa *Application) getPlaceholderAllocations() []*Allocation {
+func (sa *Application) GetPlaceholderAllocations() []*Allocation {
Review comment:
why do we need to expose this function?
note this function doesn't hold any locks, which implies it is supposed to
be only used internally
##########
File path: pkg/scheduler/objects/application.go
##########
@@ -1025,6 +1085,17 @@ func (sa *Application) getPlaceholderAllocations()
[]*Allocation {
return allocations
}
+func (sa *Application) GetAllRequests() []*AllocationAsk {
Review comment:
No need to expose this function
##########
File path: pkg/scheduler/objects/application.go
##########
@@ -1103,11 +1177,19 @@ func (sa *Application) removeAllocationInternal(uuid
string) *Allocation {
sa.allocatedResource = resources.Sub(sa.allocatedResource,
alloc.AllocatedResource)
}
// When the resource trackers are zero we should not expect anything to
come in later.
- if resources.IsZero(sa.pending) &&
resources.IsZero(sa.allocatedResource) &&
resources.IsZero(sa.allocatedPlaceholder) {
- if err := sa.HandleApplicationEvent(WaitApplication); err !=
nil {
- log.Logger().Warn("Application state not changed to
Waiting while removing some allocation(s)",
- zap.String("currentState", sa.CurrentState()),
- zap.Error(err))
+ if resources.IsZero(sa.pending) &&
resources.IsZero(sa.allocatedResource) {
+ if sa.isWaitingStateTimedOut() &&
resources.IsZero(sa.allocatedPlaceholder) {
+ if err :=
sa.HandleApplicationEvent(CompleteApplication); err != nil {
+ log.Logger().Warn("Application state not
changed to Completed while removing some allocation(s)",
+ zap.String("currentState",
sa.CurrentState()),
+ zap.Error(err))
+ }
+ } else {
+ if err := sa.HandleApplicationEvent(WaitApplication);
err != nil {
+ log.Logger().Warn("Application state not
changed to Waiting while removing some allocation(s)",
+ zap.String("currentState",
sa.CurrentState()),
+ zap.Error(err))
+ }
Review comment:
This part is quite confusing to me.
Why do we need to handle `CompleteApplication` here in a special case?
According to the doc:
http://yunikorn.apache.org/docs/next/design/scheduler_object_states. We claim
an app is in `Completed` state only when: _An application is considered
completed when it has been in the waiting state for a defined time period._
If an app has gang members, and there are placeholders (either ask or
allocation), we do not transit the app to `Waiting` state. Instead, it is in
`Running` state with a `placeholderTimer`. The placeholder timer determines
"_the max duration an app can run from the start of applying placeholders to
all placeholders is replaced_". When it times out, we trigger a clean for all
existing placeholders (no matter they are allocated or pending). Then once all
cleanup are done, the app will enter `Waiting` state (use the old logic).
Would this be easier?
##########
File path: pkg/scheduler/objects/application.go
##########
@@ -1037,6 +1108,9 @@ func (sa *Application) AddAllocation(info *Allocation) {
func (sa *Application) addAllocationInternal(info *Allocation) {
// placeholder allocations do not progress the state of the app and are
tracked in a separate total
if info.placeholder {
+ if resources.IsZero(sa.allocatedPlaceholder) {
+ sa.initPlaceholderTimer()
+ }
Review comment:
please add some comment, why we `initPlaceholderTimer()` here?
I think we have discussed, but could not remember the answer to the
following question:
what if we cannot get even one placeholder allocated for an app? does that
mean the app will be stuck forever? It could happen if the job's pods were
created with some incorrect resource sizes (e.g bigger than node capability),
wrong node selector, tolerations, etc.
##########
File path: pkg/scheduler/objects/application.go
##########
@@ -225,6 +234,56 @@ func (sa *Application) clearStateTimer() {
zap.String("state", sa.stateMachine.Current()))
}
+func (sa *Application) isWaitingStateTimedOut() bool {
+ return sa.IsWaiting() && sa.stateTimer == nil
+}
+func (sa *Application) initPlaceholderTimer() {
+ if sa.placeholderTimer != nil || !sa.IsAccepted() || sa.execTimeout <=
0 {
+ return
+ }
+ log.Logger().Debug("Application placeholder timer initiated",
+ zap.String("AppID", sa.ApplicationID),
+ zap.Duration("Timeout", sa.execTimeout))
+ sa.placeholderTimer = time.AfterFunc(sa.execTimeout,
sa.timeoutPlaceholderProcessing)
+}
+
+func (sa *Application) clearPlaceholderTimer() {
+ if sa == nil || sa.placeholderTimer == nil {
+ return
+ }
+ sa.placeholderTimer.Stop()
+ sa.placeholderTimer = nil
+}
+
+func (sa *Application) timeoutPlaceholderProcessing() {
+ sa.Lock()
+ defer sa.Unlock()
+ var allocationsToRelease []*Allocation
+ // Case 1: if all app's placeholders are allocated, only part of them
gets replaced, just delete the remaining placeholders
+ switch {
+ case sa.allPlaceholdersAllocated() && !sa.allPlaceholdersReplaced():
Review comment:
the first check seems wrong. the `allocatedPlaceholder` will be
decreased if there is any placeholder replacement happened. so even all
placeholder were allocated, after there is any replacement, this is always
false.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
