[jira] [Comment Edited] (YUNIKORN-3360) RollbackAllocation should safety check request existence

["Dale Richardson (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/YUNIKORN-3360?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18106476#comment-18106476 ] 

Dale Richardson edited comment on YUNIKORN-3360 at 8/21/26 9:09 AM:
--------------------------------------------------------------------

[~wilfreds] 

The new guard surfaced a semantic conflict with tests merged two days later in YUNIKORN-3352 (tracked in YUNIKORN-3398, test-only fix). While looking at that I think two aspects of this change deserve a follow-up look.

1. The state where {{sa.allocations}} holds the key but {{sa.requests}} does not is reachable, because ask removal and allocation removal are deliberately decoupled: {{removeAsksInternal}} drops the asks immediately, while {{sa.allocations}} entries survive until the shim confirms the releases. Any {{TerminationType_SCHEDULING_FAILED_ON_RM}} release arriving in that window reaches {{RollbackAllocation}} with the ask gone. One concrete path: a soft-gang app in Running with zero allocated placeholders falls into {{timeoutPlaceholderProcessing}} case 2, fires {{{}ResumeApplication{}}}, which is only valid from New/Accepted - the transition fails, the app stays Running (still rollback-eligible), and {{removeAsksInternal("")}} has wiped the asks. {{RemoveAllocationAsk}} on an allocated key opens the same window.

2. {{SCHEDULING_FAILED_ON_RM}} bypasses the normal remove-and-destroy path entirely ({{{}partition.removeAllocation{}}} routes it straight to {{{}rollbackAllocation{}}}), and on error the caller only logs a warning. So the allocation stays in {{sa.allocations}} and {{{}sa.allocatedResource{}}}, the node allocation is not removed, the queue's allocated resource is not decremented, and the partition allocation count is not adjusted - while the shim considers the release processed (the rollback path intentionally sends no confirmation back). Cleanup then depends on some later release arriving for the same key, e.g. from a pod delete.

To be clear, I don't think reverting the current fix is the answer as before the current guard was added a rollback in that window had its own accounting error: {{deallocateAsk}} unconditionally adds the ask's resource back to app and queue pending, so an ask nothing tracks any more permanently inflated pending (terminal cleanup walks {{sa.requests}} and never finds it). Neither before nor after is the ghost case handled correctly.

The correct handling for a rollback whose ask is no longer tracked is probably to proceed with the allocated-side cleanup (remove from {{{}sa.allocations{}}}, decrement allocated resources, node, queue, partition count) but skip returning the ask to pending - there is nothing left to re-schedule. That could live in {{RollbackAllocation}} itself, or the caller could fall through to the normal remove path when the ask is untracked. Happy to put up a patch if there's agreement on the direction.

One judgement call to flag: the last sentence offers a patch — leave it in only if you actually want to pick that work up; otherwise end at "handled correctly" and let the 3360 participants propose the direction. Also, if you'd rather split concerns, paragraph 1–2 (reachability + dropped release) stand alone as the bug report and paragraphs 3–4 could go in a second comment proposing the fix — but as one comment it reads fine and avoids a fragmented thread.


was (Author: tigerquoll):
[~wilfreds] 

The new guard surfaced a semantic conflict with tests merged two days later in YUNIKORN-3352 (tracked in YUNIKORN-3398, test-only fix). While looking at that I think two aspects of this change deserve a follow-up look.

1. ** The state where {{sa.allocations}} holds the key but {{sa.requests}} does not is reachable, because ask removal and allocation removal are deliberately decoupled: {{removeAsksInternal}} drops the asks immediately, while {{sa.allocations}} entries survive until the shim confirms the releases. Any {{TerminationType_SCHEDULING_FAILED_ON_RM}} release arriving in that window reaches {{RollbackAllocation}} with the ask gone. One concrete path: a soft-gang app in Running with zero allocated placeholders falls into {{timeoutPlaceholderProcessing}} case 2, fires {{{}ResumeApplication{}}}, which is only valid from New/Accepted - the transition fails, the app stays Running (still rollback-eligible), and {{removeAsksInternal("")}} has wiped the asks. {{RemoveAllocationAsk}} on an allocated key opens the same window.

2. ** {{SCHEDULING_FAILED_ON_RM}} bypasses the normal remove-and-destroy path entirely ({{{}partition.removeAllocation{}}} routes it straight to {{{}rollbackAllocation{}}}), and on error the caller only logs a warning. So the allocation stays in {{sa.allocations}} and {{{}sa.allocatedResource{}}}, the node allocation is not removed, the queue's allocated resource is not decremented, and the partition allocation count is not adjusted - while the shim considers the release processed (the rollback path intentionally sends no confirmation back). Cleanup then depends on some later release arriving for the same key, e.g. from a pod delete.

To be clear, I don't think reverting the current fix is the answer as before the current guard was added a rollback in that window had its own accounting error: {{deallocateAsk}} unconditionally adds the ask's resource back to app and queue pending, so an ask nothing tracks any more permanently inflated pending (terminal cleanup walks {{sa.requests}} and never finds it). Neither before nor after is the ghost case handled correctly.

The correct handling for a rollback whose ask is no longer tracked is probably to proceed with the allocated-side cleanup (remove from {{{}sa.allocations{}}}, decrement allocated resources, node, queue, partition count) but skip returning the ask to pending - there is nothing left to re-schedule. That could live in {{RollbackAllocation}} itself, or the caller could fall through to the normal remove path when the ask is untracked. Happy to put up a patch if there's agreement on the direction.

One judgement call to flag: the last sentence offers a patch — leave it in only if you actually want to pick that work up; otherwise end at "handled correctly" and let the 3360 participants propose the direction. Also, if you'd rather split concerns, paragraph 1–2 (reachability + dropped release) stand alone as the bug report and paragraphs 3–4 could go in a second comment proposing the fix — but as one comment it reads fine and avoids a fragmented thread.

> RollbackAllocation should safety check request existence 
> ---------------------------------------------------------
>
>                 Key: YUNIKORN-3360
>                 URL: https://issues.apache.org/jira/browse/YUNIKORN-3360
>             Project: Apache YuniKorn
>          Issue Type: Task
>          Components: core - scheduler
>            Reporter: Wilfred Spiegelenburg
>            Assignee: Aditya Maheshwari
>            Priority: Major
>              Labels: pull-request-available
>             Fix For: 1.10.0
>
>
> When RollbackAllocation is called the only check that is made is for the existence in the allocations map. There should be a check against the requests map also to make sure the allocation is looked at.
> The ask should always be there but we need to be consistent with other code paths which do this check.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: issues-help@yunikorn.apache.org