stantheman0128 opened a new pull request, #1120:
URL: https://github.com/apache/yunikorn-core/pull/1120
### Description
`TryPreemption()` called `initWorkingState()` and threw away its return
value. That
value is the number of reservations cancelled while the working state is
built: as the
node iterator walks the nodes it releases stale, lower priority reservations
that block
the ask.
Those cancellations stand whether or not a preemption follows. Five of the
six return
paths below that point leave without an allocation result, and the sixth
returns a
reserved result that carries no release count, so nothing ever reached the
partition.
`PartitionContext.reservations` therefore only drifted upward, and the drift
was
permanent for the lifetime of the partition.
The fix reports the count through the `reservationReleasedCallback` that
YUNIKORN-3321
added to the application, immediately after `initWorkingState()` returns.
One call site
covers every return path.
Generated by Claude Code (Claude Opus 5).
Two notes on the approach:
- The Jira description suggests returning an "empty" `AllocationResult` that
only carries
the release count. That does not work: `Queue.TryAllocate` treats any
non-nil result as
a real allocation or reservation and returns it up the stack (`queue.go`),
so an
accounting-only result would end the scheduling pass for that queue. The
callback keeps
the accounting off the scheduling path.
- The callback fires from inside `TryPreemption()` rather than through a new
return value,
which avoids changing a signature that 30 tests call directly. The release
happens while
the working state is built, regardless of what the preemption decides
afterwards, so the
accounting belongs at that point rather than on the result. As for
locking, `NewPreemptor`
documents that the preemptor "assumes the application lock is held", and
`tryAllocate`
does hold the write lock across this call, so the unlocked read of the
callback field is
safe. `executeReservationReleasedCallback` then hands the body to a
separate go routine,
the same arrangement the placeholder timeout path already uses, so nothing
blocks while
that lock is held.
- There is a second place where a cancelled reservation does not reach the
partition
counter, which I left alone because it is outside what this Jira
describes. In
`tryReservedAllocate` (`application.go`, the wait time expiry branch under
the headroom
check) the reservation is dropped with `unReserveInternal` plus
`queue.UnReserve`, and the
loop then continues without returning a result, so the count reaches the
node, the
application and the queue but never the partition. Happy to fix it here
instead if you
would rather have both in one change, or to open a separate Jira for it.
### Type of change
- [x] Bug Fix
### Jira issue
Jira ID : https://issues.apache.org/jira/browse/YUNIKORN-3319
- [ ] I have created a Jira issue for this pull request.
- [x] The Jira ID is part of the title of this pull request.
### AI Tooling
- [x] The PR includes the phrase "Generated by Claude Code (Claude Opus 5)",
where Claude Code is the name of the AI tool used.
- [x] My use of AI contributions follows the ASF legal policy.
Check https://www.apache.org/legal/generative-tooling.html for details.
### How has this been tested?
- [x] New unit tests were added to cover new or changed code paths.
- [x] `make test_all` was run, and no failures reported.
- [ ] A pull request will be opened for new e2e tests
(apache/yunikorn-k8shim repository).
Both sides of the branch are covered, because the point of the ticket is
that the release
has to be reported even when no preemption happens:
- `Test_PreemptForAppOnReservedNode` (extended) drives the no-preemption
path through
`Application.tryAllocate`, so the real lock context is exercised. The
stale reservation
is cancelled, `tryNodes()` finds no candidate, and `TryPreemption()`
returns without an
allocation result. The test asserts the high priority ask was never marked
as having
triggered preemption, and that the release is still reported.
- `Test_PreemptReleasesReservationsOnSuccess` (new) covers the path that
does preempt: a
stale low priority reservation is cancelled on the node the preemption
then picks. It
asserts the release is reported exactly once, and that
`CancelledReservations` on the
result stays zero, since the partition applies that separately and would
otherwise
decrement twice for the same cancellation.
Both tests collect the reported value over a channel, since the callback
body runs
asynchronously.
Reverting only the `preemption.go` hunk fails both tests, so they are not
vacuous:
--- FAIL: Test_PreemptForAppOnReservedNode (5.00s)
preemption_test.go:2312: timed out waiting for the released
reservation to be reported
--- FAIL: Test_PreemptReleasesReservationsOnSuccess (5.00s)
preemption_test.go:2387: timed out waiting for the released
reservation to be reported
`make test_all`:
running shellcheck
checking license headers:
all OK
running golangci-lint
0 issues.
running unit tests
"go" test ./... -race -tags deadlock -coverprofile="build/coverage.txt"
-covermode=atomic
ok github.com/apache/yunikorn-core/pkg/scheduler 21.794s
coverage: 75.6% of statements
ok github.com/apache/yunikorn-core/pkg/scheduler/objects 10.881s
coverage: 89.5% of statements
ok github.com/apache/yunikorn-core/pkg/scheduler/tests 49.821s
("go" vet passed, every other package ok, output trimmed)
Repeated runs of the two tests, to check for flakiness and races:
go test ./pkg/scheduler/objects/ -run
'Test_PreemptForAppOnReservedNode|Test_PreemptReleasesReservationsOnSuccess'
-count=1000
ok github.com/apache/yunikorn-core/pkg/scheduler/objects 111.534s
go test -race ./pkg/scheduler/objects/ -run
'Test_PreemptForAppOnReservedNode|Test_PreemptReleasesReservationsOnSuccess'
-count=1000
ok github.com/apache/yunikorn-core/pkg/scheduler/objects 124.910s
The existing reservation counting tests still pass:
`TestReservationTracking` and
`TestRemoveAppWithReservations` in `partition_test.go`, and the
`CancelledReservations`
assertions in `TestTryRequiredNode`, `TestTryRequiredNodeReserved`,
`TestTryRequiredNodeCancel` and `TestTryRequiredNodeAdd`.
### Questions:
- The change does not need documentation.
- There are no breaking changes.
- The license files do not need to be updated.
--
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]
