[
https://issues.apache.org/jira/browse/YUNIKORN-3358?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Dale Richardson updated YUNIKORN-3358:
--------------------------------------
Description:
{{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.
Priority: Minor (was: Major)
> 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
> Priority: Minor
>
> {{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]
