[GitHub] [incubator-yunikorn-core] yangwwei commented on a change in pull request #250: [YUNIKORN-519, YUNIKORN-460] Placeholder cleanup

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org