tigerquoll commented on code in PR #1127:
URL: https://github.com/apache/yunikorn-core/pull/1127#discussion_r3888382428
##########
pkg/scheduler/objects/application.go:
##########
@@ -1162,7 +1187,26 @@ func (sa *Application) tryAllocate(headRoom
*resources.Resource, allowPreemption
// because the len check above guarantees at least one iteration would
occur.
backoffThreshold := sa.queue.GetMaxAppUnschedAskBackoff()
// get all the requests from the app sorted in order
- for _, request := range sa.sortedRequests {
+ // LOAD-BEARING INVARIANT: sa.sortedRequests now holds only pending
asks and
+ // allocateAsk/deallocateAsk mutate it in place. An index based loop is
used deliberately: it
+ // re-reads len() and re-indexes the current slice on every iteration,
so a mid-loop remove can
+ // never make it read the nil'd tail slot, and it stays correct if an
insert reallocates the
+ // backing array. A "range" loop captures the slice header once and
would panic in that case.
+ // On top of that the length is asserted below: every
successful-allocation path returns
+ // immediately today, so a length change mid-loop means a future edit
mutated and continued.
+ // Worst case without the assert is skipping a single ask for this
cycle (self-correcting on the
+ // next cycle) rather than a crash. If a mutate-then-continue is ever
really needed, restructure
+ // to a snapshot copy (see tryPlaceholderAllocate).
+ startLen := len(sa.sortedRequests)
+ for i := 0; i < len(sa.sortedRequests); i++ {
+ if len(sa.sortedRequests) != startLen {
+ log.Log(log.SchedApplication).DPanic("sortedRequests
mutated during tryAllocate iteration",
+ zap.String("application ID", sa.ApplicationID),
+ zap.Int("length at loop start", startLen),
+ zap.Int("current length",
len(sa.sortedRequests)))
+ return nil
+ }
+ request := sa.sortedRequests[i]
Review Comment:
Agreed the length assert cannot fire — dropped it together with the comment
block. One clarification for the record: the hazard the loop guarded was
same-goroutine, not concurrent access — `allocateAsk` is called from inside the
loop and now removes from the slice being iterated. Every allocating path
returns, so that hazard is equally dead today.
On the clone I measured rather than assumed.
`BenchmarkScheduling/1000Nodes/10000Pods`, 3 alternating samples, medians:
| loop | pods/s | B/op |
|---|---:|---:|
| index loop | 38,604 | 203 MB |
| `slices.Clone` | 34,461 | 407 MB |
The clone is O(pending) per call for a loop that normally exits at index 0,
so it costs 11% of throughput and doubles allocation on this workload. I kept
the plain index loop (it re-reads `len()`, so the ghost repair below can remove
and continue) with a three-line comment and no assert. If you would still
rather have the clone for uniformity it is a two-line change — but I would
prefer not to hand back 11% of what this PR buys.
##########
pkg/scheduler/objects/application.go:
##########
@@ -1358,7 +1412,11 @@ func (sa *Application)
tryPlaceholderAllocate(nodeIterator func() NodeIterator,
var phFit *Allocation
var reqFit *Allocation
// get all the requests from the app sorted in order
- for _, request := range sa.sortedRequests {
+ // NOTE: iterate over a snapshot, not sa.sortedRequests directly. The
revert path below calls
+ // sa.deallocateAsk(request), which re-inserts the ask into
sa.sortedRequests mid-range - mutating
+ // the live slice while ranging over it would skip/revisit entries.
allocateAsk/deallocateAsk keep
+ // only pending asks in sortedRequests.
+ for _, request := range slices.Clone(sa.sortedRequests) {
Review Comment:
`tryPlaceholderAllocate` keeps its clone: its revert path re-inserts into
the slice mid-loop, which an index loop cannot tolerate, and it is cold. For
`tryAllocate` see the thread above — the clone measured at −11%, so it uses the
index loop.
##########
pkg/scheduler/objects/application.go:
##########
@@ -1172,7 +1216,17 @@ func (sa *Application) tryAllocate(headRoom
*resources.Resource, allowPreemption
return nil
}
if request.IsAllocated() {
- continue
+ // sortedRequests holds pending asks only, so reaching
an allocated one means the
+ // remove-on-allocate pairing has been broken and this
entry is a ghost that nothing
+ // else will clean up: every other removal path skips
allocated asks precisely because
+ // they cannot be in the slice. Scream, then repair -
drop the ghost and end the cycle,
+ // so a broken pairing is a hard failure in tests and a
single logged self-heal in
+ // production, rather than the same entry being
re-detected every cycle forever.
+ log.Log(log.SchedApplication).DPanic("allocated ask
found in pending-only sortedRequests",
+ zap.String("appID", sa.ApplicationID),
+ zap.String("allocationKey",
request.GetAllocationKey()))
+ sa.sortedRequests.remove(request)
+ return nil
Review Comment:
Agreed — it now drops the ghost and continues with the next ask; the cycle
is no longer cut short.
##########
pkg/scheduler/objects/sorted_asks.go:
##########
@@ -49,9 +59,13 @@ func (s *sortedRequests) insertAt(index int, ask
*Allocation) {
(*s)[index] = ask
}
+// remove drops the entry that IS the passed ask, matching on pointer identity
and not on
+// allocationKey. The slice tracks the pending asks the application holds in
sa.requests, and every
+// caller passes the tracked object. Matching on key would remove the first
entry that shares the
+// key, which is a different ask than the caller asked to remove as soon as
two ever coexist.
Review Comment:
Fair — the comment is trimmed to the identity/cost rationale; code kept.
--
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]
