wilfred-s commented on code in PR #1122:
URL: https://github.com/apache/yunikorn-core/pull/1122#discussion_r3781529165
##########
pkg/scheduler/objects/application.go:
##########
@@ -877,11 +933,19 @@ func (sa *Application) deallocateAsk(ask *Allocation)
(*resources.Resource, erro
return nil, fmt.Errorf("unable to deallocate pending ask %s on
app %s", ask.GetAllocationKey(), sa.ApplicationID)
}
- askPriority := ask.GetPriority()
- if askPriority > sa.askMaxPriority {
- // increase app priority
- sa.askMaxPriority = askPriority
- sa.queue.UpdateApplicationPriority(sa.ApplicationID,
askPriority)
+ // The ask returns to the pending set, but only if it still IS this
application's ask: an ask that
+ // has already been dropped from sa.requests must not be counted again.
That is reachable today:
+ // removeAsksInternal("") wipes sa.requests while leaving
sa.allocations intact until the shim
+ // confirms the releases, and a release arriving in that window reaches
RollbackAllocation, which
+ // finds the entry in sa.allocations and deallocates it. The identity
comparison (not just a
+ // presence check) also covers a stale ask object that has since been
replaced by a new ask under
+ // the same key.
+ // This matches the converged behaviour of the full rescan this change
replaced:
+ // updateAskMaxPriority derived the max by scanning sa.requests, so an
ask absent from sa.requests
+ // never influenced it. Without the guard that pre-existing accounting
drift would turn into a
+ // permanent leak in the incremental histogram instead.
+ if sa.requests[ask.GetAllocationKey()] == ask {
+ sa.incPendingPriority(ask.GetPriority())
}
Review Comment:
That case cannot happen for multiple reasons.
removeAsksInternal without an ID is only called on removal of the
application, triggered from the k8shim on completion of the last application
pod. It is a cleanup, not a simple pod exit.
There cannot be a running allocation in a gang setup if none of the
placeholders ever got allocated. Does not matter if that is soft or hard gang
scheduling. Timing out of the placeholders does not start until the first
placeholder is allocated. At least one placeholder must be running for the
timeout to start counting down. While the placeholder timeout runs and not all
placeholders are allocated we cannot have non placeholders allocations.
We also keep the application in an accepted state until all placeholders are
allocated.
If all placeholders are allocated the first non placeholder will be released
by the k8shim. That might be a gang scheduled allocation but it does not have
to be. Allocating that ask moves the state of the app to running.
Further comments will be added in #1127
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
