wilfred-s commented on code in PR #1122:
URL: https://github.com/apache/yunikorn-core/pull/1122#discussion_r3756658197
##########
pkg/scheduler/objects/application.go:
##########
@@ -664,6 +666,9 @@ func (sa *Application) AddAllocationAsk(ask *Allocation)
error {
var oldAskResource *resources.Resource = nil
if oldAsk := sa.requests[ask.GetAllocationKey()]; oldAsk != nil &&
!oldAsk.IsAllocated() {
oldAskResource = oldAsk.GetAllocatedResource().Clone()
+ // the old ask was pending and is being replaced: remove it
from the pending histogram so the
+ // new ask's addAllocationAskInternal (via incPendingPriority)
nets correctly.
+ sa.decPendingPriority(oldAsk.GetPriority())
}
Review Comment:
I am just walking through the callers of `AddAllocationAsk()`. There is only
one caller: `PartitionContext.UpdateAllocation()`. The call to add the ask is
only triggered if the allocationKey is not found in the requests for the app.
That means `oldAsk` will always be nil.
There could be a small exception of a race condition. This is however
negated by the fact that the processing loop that triggers this update via
`PartitionContext.UpdateAllocation()` is single threaded. We process changes in
order from the informers as one update could require the previous one.
We can thus not have an add and remove at the same time processing.
Test code however relies on this check. We need a new jira to clean this up.
Two further points which need new jiras:
1) only in place pod vertical scaling can change the resources of a
container or pod. For that the pod must be running. That means asks, pending
allocations, can not change their resources only allocations can.
2) priority class is an immutable object, pods must reference a priority on
creation and that value is also immutable. Priority can thus not change after
creation.
##########
pkg/scheduler/objects/application.go:
##########
@@ -778,15 +783,68 @@ func (sa *Application) RecoverAllocationAsk(alloc
*Allocation) {
}
}
+// incPendingPriority records that an ask at priority p just became pending
(added, or
+// deallocated back to pending). Call with sa.Lock() held.
+func (sa *Application) incPendingPriority(p int32) {
Review Comment:
NIT: rename to `addToPriorities()`
##########
pkg/scheduler/objects/application.go:
##########
@@ -778,15 +783,68 @@ func (sa *Application) RecoverAllocationAsk(alloc
*Allocation) {
}
}
+// incPendingPriority records that an ask at priority p just became pending
(added, or
+// deallocated back to pending). Call with sa.Lock() held.
+func (sa *Application) incPendingPriority(p int32) {
+ sa.pendingPriorities[p]++
+ if p > sa.askMaxPriority {
+ sa.setAskMaxPriority(p)
+ }
+}
+
+// decPendingPriority records that a pending ask at priority p just left the
pending set
+// (allocated, or removed). Call with sa.Lock() held.
+func (sa *Application) decPendingPriority(p int32) {
Review Comment:
NIT: rename to `removeFromPriorities()`
##########
pkg/scheduler/objects/application.go:
##########
@@ -2278,6 +2330,10 @@ func (sa *Application) GetAskMaxPriority() int32 {
func (sa *Application) cleanupAsks() {
sa.requests = make(map[string]*Allocation)
sa.sortedRequests = nil
+ // a Failed app can still hold pending asks: reset the histogram or the
consistency check would
+ // fire on terminal apps.
Review Comment:
This is called after the application is removed from the queue. This cleans
up the app state internally. It is not relevant anymore as the queue and the
partition are no longer linked to the app. Nothing can reference these fields
or trigger updates. The only reason to do this is to release memory. The
comment is not correct.
##########
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:
This is is not needed, `deallocateAsk()` is the unlocked version of
`DeallocateAsk()` which already does this check. All other callers must have
taken the application write lock before calling this and retrieved the ask
reference from the application inside that lock. That means the ask has to
exist in the requests map. This should be a straight call without the check.
--
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]
