[
https://issues.apache.org/jira/browse/YUNIKORN-3426?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Dale Richardson updated YUNIKORN-3426:
--------------------------------------
Description:
Since YUNIKORN-2884 the bind goroutine in {{Task.postTaskAllocated}} takes
{{task.lock.Lock()}} and then runs both the volume bind and the pod bind
through {{retry.OnError(retryBackoff, ...)}}, releasing the lock only
afterwards. {{retryBackoff}} is eight steps starting at one second, doubling,
capped at 30 s; the file's own comment puts the total at 121 seconds.
So while the API server is slow or failing, the task write lock is held for up
to two minutes of sleeping plus the round trips, and everything that takes that
lock waits behind it. Correction to the first version of this text:
{{GetTaskState}} reads the state machine directly and is not affected. What is
affected is every {{task.lock}} user: {{GetTaskPod}} (read lock, which a held
write lock excludes) from the dispatcher and the scheduling loop,
{{task.handle}} for any event on the task, the application-level walks that
take task locks, and {{SetTaskPod}} from the pod informer. The last one is the
broad effect: {{Context.UpdatePod}} runs on the informer goroutine, so while
one task's bind is retrying, the informer handler is parked in {{SetTaskPod}}
and no other pod update is processed until it returns. With many tasks binding
at once during an API-server outage the shim's pod-event processing stalls for
the length of the backoff.
Not reproduced; the mechanism is unconditional. It is understood that the bind
goroutine wants to keep the task from being updated while the bind is in
flight; the question is whether a lock held across sleeps is the right way to
express that, versus a bind-in-progress state or flag that {{SetTaskPod}}
respects, which keeps the informer goroutine free.
This landed after the lock annotations were written and was reported by the
analyser when the branch was rebased; it is the kind of change the check is
meant to catch at review.
Fix: snapshot what the bind needs under the lock, release it, run the retry
loop unlocked, and re-take the lock only to apply the result and drive the
state transition, the same shape the code already uses for the pod/alias/node
snapshot it passes into the goroutine; if pod updates must be held off during
the bind, do it with state rather than by holding the lock.
Marker: the two {{retry.OnError}} calls in {{task.go}} carry
{{+lockblockingignore}} for this JIRA; the fix removes them.
was:
Since YUNIKORN-2884 the bind goroutine in {{Task.postTaskAllocated}} takes
{{task.lock.Lock()}} and then runs both the volume bind and the pod bind
through {{retry.OnError(retryBackoff, ...)}}, releasing the lock only
afterwards. {{retryBackoff}} is eight steps starting at one second, doubling,
capped at 30 s; the file's own comment puts the total at 121 seconds.
So while the API server is slow or failing, the task write lock is held for up
to two minutes of sleeping plus the round trips, and everything else that
touches that task waits behind it: the informer's pod updates ({{SetTaskPod}}),
{{GetTaskState}} / {{GetTaskPod}} from the dispatcher and the scheduling loop,
task events through {{task.handle}}, and the application-level walks that take
task locks. With many tasks binding at once during an API-server outage the
shim's per-task paths stall broadly. Not reproduced; the mechanism is
unconditional.
This landed after the lock annotations were written and was reported by the
analyser when the branch was rebased; it is the kind of change the check is
meant to catch at review.
Fix: snapshot what the bind needs under the lock, release it, run the retry
loop unlocked, and re-take the lock only to apply the result and drive the
state transition, the same shape the code already uses for the pod/alias/node
snapshot it passes into the goroutine.
Marker: the two {{retry.OnError}} calls in {{task.go}} carry
{{+lockblockingignore}} for this JIRA; the fix removes them.
> Pod bind retries with backoff run under the task write lock
> -----------------------------------------------------------
>
> Key: YUNIKORN-3426
> URL: https://issues.apache.org/jira/browse/YUNIKORN-3426
> Project: Apache YuniKorn
> Issue Type: Sub-task
> Components: shim - kubernetes
> Reporter: Dale Richardson
> Assignee: Aditya Maheshwari
> Priority: Major
>
> Since YUNIKORN-2884 the bind goroutine in {{Task.postTaskAllocated}} takes
> {{task.lock.Lock()}} and then runs both the volume bind and the pod bind
> through {{retry.OnError(retryBackoff, ...)}}, releasing the lock only
> afterwards. {{retryBackoff}} is eight steps starting at one second, doubling,
> capped at 30 s; the file's own comment puts the total at 121 seconds.
> So while the API server is slow or failing, the task write lock is held for
> up to two minutes of sleeping plus the round trips, and everything that takes
> that lock waits behind it. Correction to the first version of this text:
> {{GetTaskState}} reads the state machine directly and is not affected. What
> is affected is every {{task.lock}} user: {{GetTaskPod}} (read lock, which a
> held write lock excludes) from the dispatcher and the scheduling loop,
> {{task.handle}} for any event on the task, the application-level walks that
> take task locks, and {{SetTaskPod}} from the pod informer. The last one is
> the broad effect: {{Context.UpdatePod}} runs on the informer goroutine, so
> while one task's bind is retrying, the informer handler is parked in
> {{SetTaskPod}} and no other pod update is processed until it returns. With
> many tasks binding at once during an API-server outage the shim's pod-event
> processing stalls for the length of the backoff.
> Not reproduced; the mechanism is unconditional. It is understood that the
> bind goroutine wants to keep the task from being updated while the bind is in
> flight; the question is whether a lock held across sleeps is the right way to
> express that, versus a bind-in-progress state or flag that {{SetTaskPod}}
> respects, which keeps the informer goroutine free.
> This landed after the lock annotations were written and was reported by the
> analyser when the branch was rebased; it is the kind of change the check is
> meant to catch at review.
> Fix: snapshot what the bind needs under the lock, release it, run the retry
> loop unlocked, and re-take the lock only to apply the result and drive the
> state transition, the same shape the code already uses for the pod/alias/node
> snapshot it passes into the goroutine; if pod updates must be held off during
> the bind, do it with state rather than by holding the lock.
> Marker: the two {{retry.OnError}} calls in {{task.go}} carry
> {{+lockblockingignore}} for this JIRA; the fix removes them.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
