Skip to content

[YUNIKORN-3512] Fix placeholder accounting on placeholder resize - #1192

Closed
rjgoyln wants to merge 6 commits into
apache:masterfrom
rjgoyln:YUNIKORN-3512
Closed

rjgoyln wants to merge 6 commits into
apache:masterfrom
rjgoyln:YUNIKORN-3512

Conversation

@rjgoyln

@rjgoyln rjgoyln commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Summary

When a placeholder pod is resized in place, the core books the resize delta into the application's allocatedResource instead of allocatedPlaceholder. Queue and node usage stay correct, but the application's split drifts. Growing one of two allocated 2-vcore placeholders to 3 vcore gives:

Variable Before resize Current (incorrect) Expected
allocatedResource 0 1 0
allocatedPlaceholder 4 4 5

Once the placeholders are replaced or released, this can:

  • leave the application in Running or Completing with no allocations left;
  • clear the placeholder timer while a placeholder is still allocated, so that placeholder never times out.

Change

  • Book an allocated placeholder's resize delta into allocatedPlaceholder.
  • Track placeholder resize deltas, pending or allocated, separately from placeholderAsk for the gang completion check.

The gang moves to Running once the allocated placeholders add up to the gang's total, and a resize changes that total. placeholderAsk keeps the value the shim declared. This also fixes a resize of a still-pending placeholder, which on master leaves the application in Accepted with every placeholder allocated.

No delta is tracked without a placeholder ask, since a delta alone could match a partial gang.

The delta does not survive a scheduler restart, because the shim declares the original ask again; master has the same gap.

Tests

TestUpdateAllocationResourcePlaceholder and TestUpdateAllocationPlaceholderResize fail on master. TestUpdateAllocationResourcePlaceholderWithoutAsk fails without the zero-ask check.
TestFailReplacePlaceholder now unregisters its predicate plugin: its cleanup re-registered the same failing plugin, so later tests took a different replacement path.

Type of change

  • Bug Fix

Jira issue

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

  • 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.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.

An in-place resize of an allocated placeholder pod added the delta to
the application's allocated resources instead of its placeholder
resources. The two totals drifted apart, which left applications stuck
in Running or Completing and could clear the placeholder timer while a
placeholder was still allocated.

The gang moves to Running when the allocated placeholders equal the
placeholder ask. The ask now follows every placeholder resize, so the
gang still completes when a placeholder is resized before the last one
is allocated, including a resize while it is still pending.
An application without a placeholder ask has no gang to complete. Adding
a resize delta to its empty ask produced a small or negative ask that a
single placeholder allocation could match, moving the application to
Running while other placeholders were still pending.

The partition test now checks what each release returns, in line with
the neighbouring placeholder tests.
The partition test only grew a single resource after the gang was
complete. Shrinking, mixed-sign and multi-resource deltas and repeated
resizes before the gang completes exercise placeholder ask tracking and
pruning differently, so each step is now checked against the app, queue,
node and user usage.

Checking the node exposed that TestFailReplacePlaceholder left its
failing predicate plugin registered: its cleanup registered a plugin
with the same failing arguments. Later tests silently took a different
replacement path, so the plugin is now unregistered.
The partition test derived its expected usage from the placeholder
sizes the scheduler stored, so it still passed when the partition
ignored placeholder resizes altogether. The test now keeps its own
record of every placeholder's size and checks the stored sizes, the
pending usage and the partition placeholder count at each step.

The zero placeholder ask check now covers growing, shrinking and
mixed-sign deltas, since a shrinking or mixed resize would otherwise
create a negative ask rather than a matching one.
The expectation model only followed placeholder sizes, so the
placeholder ask and pending usage were checked only before replacement
and the application state only at a few points. It now also follows the
real asks waiting to replace placeholders, the ask and the state, so
every step checks the full accounting.
@rjgoyln
rjgoyln marked this pull request as ready for review October 10, 2026 13:34
@HuangTing-Yao
HuangTing-Yao self-requested a review October 10, 2026 14:01
@codecov

codecov Bot commented Oct 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.29%. Comparing base (0ef9fd7) to head (7073c63).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1192      +/-   ##
==========================================
- Coverage   83.31%   83.29%   -0.03%     
==========================================
  Files         104      104              
  Lines       14652    14659       +7     
==========================================
+ Hits        12208    12210       +2     
- Misses       2146     2151       +5     
  Partials      298      298              

☔ 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.

@HuangTing-Yao HuangTing-Yao left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree the code defect is real, and the PR's tests do fail on master. I have three concerns about how this ticket is framed, though.

1. Is the trigger realistic?

A placeholder is an intermediate artifact of gang scheduling, and I can't think of a reason for a user or a tool to resize one.

  • Has this been reported by a user or reproduced on a real cluster?
  • If it only happens with a manual kubectl patch, I'd suggest lowering the priority from Major to Minor.

2. The fix does not survive a scheduler restart

The adjusted placeholderAsk only lives in the core's memory. After a restart the shim sends the original ask again, while the recovered placeholders carry their resized sizes. The two don't match, so the application stays in Accepted.

This is already the case on master and is not caused by this PR, but it means the problem is not fully solved.

3. The meaning of placeholderAsk changes

I have no objection to booking the delta into allocatedPlaceholder; that matches how a normal allocation behaves.

But placeholderAsk is a total the shim declares from the task groups, not a sum of the live allocations. With this PR it no longer matches the shim's own copy, which is where point 2 comes from.

Any thought from @wilfred-s and @manirajv06 ?

placeholderAsk is the total the shim declares from the task groups.
Adjusting it on a resize made the core's copy differ from the shim's.
The resize deltas are now kept in a separate total that is added to the
ask only in the gang completion check, so the behaviour is unchanged.
@rjgoyln

rjgoyln commented Oct 13, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review!

  1. This hasn't been reported by a user. I found it while working on YUNIKORN-2133 and reproduced it through shim unit tests, not on a real cluster. A manual in-place resize is the only trigger I know of, so I've lowered the JIRA priority to Minor.

  2. Agreed. After a restart, the shim declares the original ask while the recovered placeholders retain their resized sizes, leaving an app with only placeholders in Accepted until the placeholder timeout. Master behaves the same. I'll track the recovery fix in a follow-up JIRA.

  3. Good point. placeholderAsk now retains the value declared by the shim, while resize deltas are tracked separately in placeholderResize. The gate compares allocatedPlaceholder against the ask plus the resize delta, preserving the existing behavior. Updated in ad619e0.

@wilfred-s

Copy link
Copy Markdown
Contributor

See my comments in the jira: I think we need to outright reject any resize for placeholder pods. As @HuangTing-Yao mentioned it is not a realistic use case. A placeholder pod should be lightweight. We use the pause pod by default as it is as small and light as possible.

If we can change the jira into reject placeholder resize I would be all for it. The K8s quota enforcement does that also. I think we need to do the same. That might need to be in our admission controller. We should really look at that.

@rjgoyln

rjgoyln commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, that makes sense. I'll re-scope YUNIKORN-3512 to rejecting placeholder resizes and close this PR.

@rjgoyln rjgoyln closed this Oct 14, 2026
@rjgoyln
rjgoyln deleted the YUNIKORN-3512 branch October 14, 2026 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants