wilfred-s commented on code in PR #1127:
URL: https://github.com/apache/yunikorn-core/pull/1127#discussion_r3860843461
##########
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:
This same structure should be used in tryAllocate
##########
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:
This cannot happen. The application is write locked when the tryAllocate
starts. Any remove or add of an ask MUST have a write lock also and will thus
wait. Updating or accessing sortedRequest outside of locking is a race.
To keep the code simple we should range over a clone of the slice, we can
then cleanup the real sortedRequests without an issue.
##########
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:
`allocationKey` is a UID that is generated by K8s for the pod. There cannot
be two identical allocation keys for the same app ever.
Comment is incorrect
Pointer comparison is faster in this case so we can keep the code change
just not the comment.
##########
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:
This is a behavioural change I do not think we should make. It short
circuits the allocation cycle and moves on to the next app/queue etc.
Using a clone of the slice to iterate over would allow cleanup here without
changing behaviour.
--
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]
