tigerquoll commented on code in PR #1122:
URL: https://github.com/apache/yunikorn-core/pull/1122#discussion_r3776190544
##########
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:
I may have found a path:
a soft-gang app that is Running via a plain (non-task-group) allocation
while none of its placeholders ever allocated. The placeholder timeout then
takes the else branch - `ResumeApplication` is rejected from Running, so the
app stays rollback-eligible — and `removeAsksInternal("")` wipes `sa.requests`
while `sa.allocations` survives until the shim confirms the releases. A
`SCHEDULING_FAILED_ON_RM` for the in-flight bind arriving in that window
reaches RollbackAllocation, and deallocateAsk gets an ask that is no longer
tracked.
#1127 adds `TestRollbackAllocationAskNotTracked` to pin this, and also puts
the pending-resource re-add behind the same check: without it, an untracked
rollback leaves phantom pending on the app and queue, so the app stops
completing on its own and queue pending stays inflated until the app is
removed.
On the requests check in `RollbackAllocation`: #1127 effectively implements
the safe half of that, one level down - `deallocateAsk` only returns an ask to
the pending structures when it is still tracked, so an untracked rollback can
no longer produce an ask that looks pending but never schedules, and that holds
for every caller. A hard reject in `RollbackAllocation` itself would overshoot:
on error the partition drops the release, so the node and queue would keep
counting a dead allocation. What may still be worth doing under your jira is a
warning log when the key is missing from requests. If you do proceed with a
JIRA, I will add a ref to #1127 about it.
--
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]
