[
https://issues.apache.org/jira/browse/YUNIKORN-3358?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18111604#comment-18111604
]
Dian-Xuan Yang commented on YUNIKORN-3358:
------------------------------------------
I initially considered using `DPanic`, but decided against it as it could
either crash the scheduler or allow state corruption to continue.
The final approach is to reject duplicate keys with an error before any state
mutation.
Would this approach look reasonable to you? More details are included in the PR.
> Remove the unreachable replace-existing-ask branch from
> Application.AddAllocationAsk
> ------------------------------------------------------------------------------------
>
> Key: YUNIKORN-3358
> URL: https://issues.apache.org/jira/browse/YUNIKORN-3358
> Project: Apache YuniKorn
> Issue Type: Bug
> Components: core - scheduler
> Affects Versions: 1.9.0
> Reporter: Dale Richardson
> Assignee: Dian-Xuan Yang
> Priority: Minor
> Labels: pull-request-available
>
> {{Application.AddAllocationAsk}} (pkg/scheduler/objects/application.go)
> contains a branch that handles the case where the ask key is already present
> in {{{}sa.requests{}}}: it clones the old ask's resource, nets it out of the
> pending delta, and (since YUNIKORN-3352) decrements the pending-priority
> histogram.
> That branch is unreachable in production. Raised by [~wilfreds] in review of
> [PR 1122|https://github.com/apache/yunikorn-core/pull/1122].
> h2. Why it is unreachable
> * The only insertion into the requests map happens once, in
> {{{}addAllocationAskInternal{}}}:
> {noformat}
> sa.requests[ask.GetAllocationKey()] = ask
> {noformat}
> * Its only callers are {{AddAllocationAsk}} and {{{}RecoverAllocationAsk{}}}.
> * Both are reached only from {{{}PartitionContext.UpdateAllocation{}}},
> which calls {{AddAllocationAsk}} only when {{GetAllocationAsk}} has returned
> nil.
> * {{UpdateAllocation}} runs only on the single goroutine started as {{go
> s.handleAllocEvent()}} in pkg/scheduler/scheduler.go, so the two add paths
> cannot overlap.
> There is exactly one adder and it is single threaded, so nothing can insert
> the key between the gate's check and {{AddAllocationAsk}} taking the write
> lock. A concurrent remove cannot produce a non-nil {{oldAsk}} either: a
> remove only keeps the key absent.
> h2. Note for whoever picks this up
> The branch is not simply unused code. It is the re-validation half of a
> check-then-act gate: {{GetAllocationAsk}} takes and releases the read lock in
> {{{}UpdateAllocation{}}}, and this branch re-reads the requests map under the
> write lock and compensates.
> Removing it is correct, but the reason it is safe is the single-adder
> property above, and that reason should be recorded in a comment at the
> insertion site so a future second adder does not reintroduce the hazard
> silently.
> h2. Supporting facts
> Two properties make the surrounding accounting sound and are worth capturing:
> * Only in-place pod vertical scaling can change a container's resources, and
> it requires a running pod. A pending ask's resources therefore cannot change;
> only allocations can.
> * PriorityClass is immutable and a pod must reference it at creation. An
> ask's priority therefore cannot change after creation.
> So an ask is immutable in both of the fields the pending-priority histogram
> keys on.
> h2. Scope
> * Remove the branch and its compensation: the {{oldAsk}} lookup,
> {{{}oldAskResource{}}}, and the later {{{}delta.SubFrom(oldAskResource){}}}.
> * Add a comment at {{addAllocationAskInternal}} recording the single-adder
> property.
> * Update the tests that depend on the branch.
> h2. Test impact (measured)
> Removing the branch locally and running {{./pkg/scheduler/...}} fails exactly
> three tests, all in pkg/scheduler/objects, nothing elsewhere:
> ||Test||Action||
> |{{TestAddAllocationAskReplaceExistingPendingAsk}}|Purpose-built for this
> branch: adds a key at one priority, then re-adds it at another. Delete.|
> |{{TestAddAllocAsk}}|Incidentally re-adds a key. Adjust.|
> |{{TestApplicationPropertyFuzzHistogram}}|The property fuzzer added by
> YUNIKORN-3352. Its {{case 8}} drives the replace branch deliberately and
> asserts the case stays covered. See below.|
> h2. Decision needed on the fuzzer
> Deleting {{case 8}} would remove coverage that YUNIKORN-3352 just added. The
> suggested alternative is to invert it: have the fuzzer assert that re-adding
> an existing key never occurs, so the single-adder invariant is pinned by a
> test rather than by a comment.
> h2. Acceptance criteria
> * The replace branch is gone from {{{}AddAllocationAsk{}}}.
> * The single-adder property is documented at the insertion site.
> * {{pkg/scheduler/...}} is green, including under {{{}-race{}}}.
> * The property fuzzer either still exercises the invariant or explicitly
> asserts it cannot be violated.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
