[YUNIKORN-3481] Fix orphan allocation when a removal races with scheduling by rjgoyln · Pull Request #1175 · apache/yunikorn-core

rjgoyln · 2026-09-30T16:53:12Z

Summary

Application.tryNode() creates the allocation under the application lock, but node, bind time, and instance type were set later in PartitionContext.allocate(). A removal in between could:

leave the allocation on the node with its resources still on the queue
track usage under an empty instance type with a zero bind time

The first leaves an orphan allocation until restart. This regressed from YUNIKORN-2459 (1.6.0), which stopped setting the node when the allocation was created.

Change

Set node, bind time, and instance type together in tryNode()
Clear these fields when deallocating an ask
Skip deallocated allocations during usage tracking
In allocate(), only unwind an allocation on the removed node

The three fields move together because the removal path uses all of them. A deallocated allocation was never handed to the shim, so it should not contribute to usage tracking.

An Unreserved result points at the reserved node, but the shim may have allocated its ask on any node. An allocation on another node is still running and must not be unwound.

Tests

TestRemoveAllocationRaceWithAllocate: covers release and application removal between tryNode() and allocate()
TestRemoveNodeRaceWithAllocate: covers both orderings of node removal and allocate()
TestUnreserveRaceWithRemoveNode: covers a stale reservation for an ask placed on the removed node and on another node

The tests fail on master where the corresponding bugs exist and pass with the fix. make test_all and race/deadlock tests pass.

Follow-up

allocate() can still hand a result to the shim after the node has been removed. Handling that by returning nil, as it already does for a removed node, is a separate change.

Type of change

Bug Fix

Jira issue

Jira ID : https://issues.apache.org/jira/browse/YUNIKORN-3481

I have created a Jira issue for this pull request.
The Jira ID is part of the title of this pull request.

AI Tooling

Generated by Claude Code (Opus 5)

The PR includes the phrase "Generated by <tool>", where <tool> is the name of the AI tool used.
My use of AI contributions follows the ASF legal policy.

How has this been tested?

New unit tests were added to cover new or changed code paths.
make test_all was run, and no failures reported.

Questions:

The change needs documentation, a pull request for apache/yunikorn-site repository will be created.
There is breaking changes for older versions: jira is tagged with release-notes label.
The licenses files needs to be updated.

codecov · 2026-09-30T17:44:56Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.46%. Comparing base (0ef9fd7) to head (3a5b29d).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1175      +/-   ##
==========================================
+ Coverage   83.31%   83.46%   +0.14%     
==========================================
  Files         104      104              
  Lines       14652    14656       +4     
==========================================
+ Hits        12208    12232      +24     
+ Misses       2146     2128      -18     
+ Partials      298      296       -2

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

manirajv06 · 2026-10-01T07:58:05Z

Can we generalise this specific change to "lets do partition related changes only in partition.allocate() ? So that even other two calls to set the bind time and node type can be moved over there. Also bind time should be set immediately after the node is set.

rjgoyln · 2026-10-01T13:00:20Z

Can we generalise this specific change to "lets do partition related changes only in partition.allocate() ? So that even other two calls to set the bind time and node type can be moved over there. Also bind time should be set immediately after the node is set.

Thanks for the suggestion — I generalized it to all three fields: node ID, bind time, and instance type, with bind time set immediately after the node as you suggested.

For now, I kept them in tryNode() rather than partition.allocate(), because a release or an application removal can be processed between the two. The removal path reads all three: the node for node and queue cleanup, and bind time and instance type in trackCompletedResource() → AggregateTrackedResource(). If these are set only in allocate(), a removal in that window can leave the allocation on the node and aggregate the application's usage under an empty instance type, with the duration derived from a zero bind time.

allocate() no longer sets any of the three, so the scheduling path now writes them together in one place.

I've pushed the update, so please feel free to take a look at the new diff.

HuangTing-Yao

LGTM

+1 to binding in tryNode() rather than allocate(), since a removal can run in between.

chenyulin0719

+1

wilfred-s

This introduces a different issue.
The ask has a node set and the node could be removed while the allocation is getting processed. That triggers a deallocate call. With this change the node details are set on the allocation before that removal is checked. We now also need to make sure that the deallocate removes the node details that are set in the application.

Please check that, besides this one point change looks good

rjgoyln · 2026-10-05T13:14:32Z

Please check that, besides this one point change looks good

Thanks for pointing this out. I updated deallocate() to clear the node details and added a guard in trackCompletedResource() for already deallocated allocations. Also added regression tests for the node removal race.

rjgoyln · 2026-10-05T13:41:27Z

I also found a stale reservation case where an already-allocated ask can be returned as Unreserved. I updated allocate() to only deallocate allocations created by the current attempt and added a regression test in e2eff88.

HuangTing-Yao · 2026-10-08T10:23:51Z

@rjgoyln , please help to resolve the conflict.

…ation Scheduling places an allocation on the node and the application in one step and records the node it was placed on in a second step. A release handled in between finds no node on the allocation, so it leaves the allocation on the node and the resources on the queue. Both leak until the scheduler restarts, with the health check reporting an orphan allocation on the node.

…ndition The review of the first commit pointed out that the issue names two readers of the node on the allocation, a release and an application removal, while the test only exercised the release. It also noted that the file states lock requirements on the function comment rather than at the statement that relies on them.

Review feedback asked to generalise the fix instead of moving one field. The removal path aggregates the application's tracked resource usage from the instance type and the bind time, so both are read in the same window as the node: a removal that lands before the allocation is finalised keys the usage under an empty instance type and derives its duration from a zero bind time. Keeping these in PartitionContext.allocate() cannot close that window, so the allocation is bound to the node in one step and allocate() no longer repeats it.

An ask unwound in allocate() because its node was removed went back to pending still bound to that node. Clearing the binding on deallocate exposed the node removal to a zero bind time when allocate() unwinds the ask before the removal reaches it, so a deallocated allocation no longer has its usage tracked: it was never handed to the shim.

An unreserve result can carry an ask that the shim already placed on another node. When the reserved node was removed, allocate() turned that live allocation back into a pending ask, and with the binding now cleared on deallocate its release left it on the node.

The skip was only exercised through the partition tests, and coverage is collected per package, so the objects package reported it as uncovered.

AddAllocation returns an error since YUNIKORN-3218.

rjgoyln · 2026-10-08T11:02:28Z

@rjgoyln , please help to resolve the conflict.

Thanks, rebased onto the latest master and resolved the conflict.

An unreserve only carries an allocated ask when its reservation went stale. When that ask is allocated on the removed node, the node removal drops its allocation and the ask must return to pending, while an ask allocated on another node is still running there.

An unreserve result carries the reserved node, which is not always the node the ask is allocated on. Comparing against that node missed an ask whose own node was removed as well, leaving it allocated without an allocation.

HuangTing-Yao assigned rjgoyln Sep 30, 2026

rjgoyln marked this pull request as ready for review September 30, 2026 17:47

HuangTing-Yao self-requested a review September 30, 2026 23:24

manirajv06 requested review from 0yukali0, chenyulin0719, mitdesai, pbacsko and wilfred-s October 1, 2026 07:58

HuangTing-Yao approved these changes Oct 1, 2026

View reviewed changes

HuangTing-Yao requested a review from manirajv06 October 1, 2026 14:35

chenyulin0719 approved these changes Oct 1, 2026

View reviewed changes

wilfred-s requested changes Oct 5, 2026

View reviewed changes

rjgoyln mentioned this pull request Oct 5, 2026

Add rjgoyln's September contribution report for apache-yunikorn opensource4you/readme#23

Merged

HuangTing-Yao requested a review from wilfred-s October 5, 2026 14:37

rjgoyln force-pushed the YUNIKORN-3481 branch from e2eff88 to ab80b5e Compare October 6, 2026 11:23

rjgoyln added 7 commits October 8, 2026 18:57

[YUNIKORN-3481] Cover the usage tracking skip in the objects package

0b048c6

The skip was only exercised through the partition tests, and coverage is collected per package, so the objects package reported it as uncovered.

[YUNIKORN-3481] Check the AddAllocation error in the objects test

0350796

AddAllocation returns an error since YUNIKORN-3218.

rjgoyln force-pushed the YUNIKORN-3481 branch from a4c166a to 0350796 Compare October 8, 2026 11:00

wilfred-s requested changes Oct 8, 2026

View reviewed changes

Comment thread pkg/scheduler/partition.go Outdated

HuangTing-Yao requested a review from wilfred-s October 9, 2026 12:12

wilfred-s requested changes Oct 12, 2026

View reviewed changes

[YUNIKORN-3481] Unwind an allocation whose own node is gone

52c6cb6

An unreserve result carries the reserved node, which is not always the node the ask is allocated on. Comparing against that node missed an ask whose own node was removed as well, leaving it allocated without an allocation.

Navigation Menu

Repository navigation

[YUNIKORN-3481] Fix orphan allocation when a removal races with scheduling - #1175

[YUNIKORN-3481] Fix orphan allocation when a removal races with scheduling#1175
rjgoyln wants to merge 9 commits into
apache:masterfrom
rjgoyln:YUNIKORN-3481

rjgoyln commented Sep 30, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Sep 30, 2026 •

edited

Loading

Uh oh!

manirajv06 commented Oct 1, 2026

Uh oh!

rjgoyln commented Oct 1, 2026

Uh oh!

HuangTing-Yao left a comment

Uh oh!

chenyulin0719 left a comment

Uh oh!

wilfred-s left a comment •

edited

Loading

Uh oh!

rjgoyln commented Oct 5, 2026

Uh oh!

rjgoyln commented Oct 5, 2026

Uh oh!

HuangTing-Yao commented Oct 8, 2026

Uh oh!

rjgoyln commented Oct 8, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

rjgoyln commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Change

Tests

Follow-up

Type of change

Jira issue

AI Tooling

How has this been tested?

Questions:

Uh oh!

codecov Bot commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

manirajv06 commented Oct 1, 2026

Uh oh!

rjgoyln commented Oct 1, 2026

Uh oh!

HuangTing-Yao left a comment

Choose a reason for hiding this comment

Uh oh!

chenyulin0719 left a comment

Choose a reason for hiding this comment

Uh oh!

wilfred-s left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjgoyln commented Oct 5, 2026

Uh oh!

rjgoyln commented Oct 5, 2026

Uh oh!

HuangTing-Yao commented Oct 8, 2026

Uh oh!

rjgoyln commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rjgoyln commented Sep 30, 2026 •

edited

Loading

codecov Bot commented Sep 30, 2026 •

edited

Loading

wilfred-s left a comment •

edited

Loading

rjgoyln commented Oct 8, 2026 •

edited

Loading