[
https://issues.apache.org/jira/browse/YUNIKORN-3356?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Dale Richardson updated YUNIKORN-3356:
--------------------------------------
Description:
h2. Summary
The shim builds two general-purpose clientsets whose groupings follow code
structure rather than traffic type: one serves both the informers and the
events sink, the other serves writes, the volume binder, and predicate lookups.
Because each clientset gets its own rate limiter from the same
{{kubernetes.qps}} setting, (a) unrelated traffic classes share failure domains
— an event burst can delay informer relists during recovery — and (b) the
effective request ceiling is 2× the configured value, which is unlikely to be
what operators expect.
Proposal: restructure into purpose-built clients — {*}writes/binds{*},
{*}informers{*}, *events* - each with its own rate limiter, sensible
per-concern defaults, and a distinct User-Agent.
Scope note: this ticket is entirely client-side (shim code + config). It ships
no FlowSchema/PriorityLevelConfiguration objects and requires none - all
server-side APF work is proposed separately in a later proposal. APF is
referenced below only as rationale for where client-side limits are and are not
appropriate.
h2. Current state (master, {{pkg/client}} / {{{}pkg/shim{}}})
* Bootstrap client ({{{}pkg/client/kubeclient.go:43{}}},
{{{}interfaces.go:58{}}}, called from {{{}bootstrap.go:30{}}}): two ConfigMap
GETs at startup. Sets no QPS, so no clientset-level limiter is created and it
runs on client-go's rest-layer defaults (5 QPS / 10 burst).
* Clientset 2 ({{{}pkg/shim/scheduler.go:67{}}}): serves *both* the
cluster-wide SharedInformerFactory ({{{}scheduler.go:70{}}}) *and* the
events/v1 broadcaster sink ({{{}scheduler.go:84-85{}}}).
* Clientset 3 ({{{}pkg/client/apifactory.go:93{}}}): serves *all writes*
(Bind/Create/Delete/UpdateStatus), the namespaced ConfigMap informer factory,
the volume binder ({{{}apifactory.go:122{}}}), and the predicate framework
handle.
Consequences:
# *{{kubernetes.qps}} is a per-clientset limit, so the effective ceiling is 2×
the configured value.* Each {{NewKubeClient}} call builds a fresh
{{rest.Config}} ({{{}kubeclient.go:55-69{}}}) and client-go creates a new token
bucket per clientset ({{{}kubernetes/clientset.go{}}},
{{{}NewForConfigAndClient{}}}). An operator setting {{kubernetes.qps: 500}}
expecting to cap the scheduler at 500 req/s actually allows ~1,000. Any further
client splitting silently multiplies this again unless the config surface is
redesigned — so this restructuring and the config semantics must land together.
# *An event flood competes with informer relists on the same token bucket*
(clientset 2). Watch calls themselves bypass the client rate limiter entirely
({{{}rest/request.go:763-764{}}} — watches are deliberately not throttled), but
the *relist / initial LIST* after a watch break does pay a token — and the
events/v1 broadcaster spawns a goroutine per event
({{{}tools/events/event_broadcaster.go:170-173{}}}), so a mass-failure burst
can park thousands of concurrent token waits ahead of a recovery relist on the
shared FIFO bucket. Order-of-magnitude impact (assuming a burst of a few
thousand events with the bucket drained): seconds of relist delay at the
1000/1000 default, minutes at operator-lowered values (e.g. 50). Recovery
correctness is sensitive to relist timeliness (see YUNIKORN-3355). Splitting
removes the coupling at every setting.
# {*}Events compete with nothing they should{*}: events are droppable by
design, but today the only way to limit them is to limit everything else too.
Note the broadcaster's bounded 1000-entry drop-on-full queue bounds only the
broadcast side; in-flight sink recordings are per-event goroutines with *no*
bound — a client-side event bucket introduces a real bound that does not exist
today.
# No {{UserAgent}} is set on any client (zero occurrences in non-test code),
so apiserver-side attribution (audit logs, per-client debugging) cannot
distinguish the shim's write path from its watch path.
h2. Proposed design
||Client||Serves||Limiter||Config||
|writes|Bind, Create/Delete, status updates, volume binder|*none by default* —
APF governs this path on every supported Kubernetes version (on by default
since 1.20, GA 1.29); {{{}kubernetes.qps{}}}/{{{}kubernetes.burst{}}} retained
as an *opt-in* cap|Values <= 0 (the new default) install an explicit no-op
limiter — note this must be {{{}flowcontrol.NewFakeAlwaysRateLimiter(){}}},
because leaving {{rest.Config.QPS}} at 0 silently applies client-go's 5/10
defaults. Positive values install a token bucket for operators with a
deliberate policy reason to cap the scheduler (fleet limits, fragile downstream
admission/audit infrastructure). Rationale: the limiter's only measured
production effect is harm — 52 binds/s at qps 50, and even the 1000 default
clips sustained peak (~1,170 binds/s measured only as a burst-window average) —
while APF provides the actual protection. Method: 3,000–5,000-pod bind bursts
on a kind+KWOK rig, 500 nodes; harness offered below.|
|informers|all SharedInformerFactory watch/list traffic|*none* (unlimited)|—
steady-state request rate is intrinsically tiny (watches are long-lived and
bypass the limiter; the limiter charges per request, not per event); relist
bursts are better governed server-side by APF than by a client limiter that
would slow recovery|
|events|events/v1 broadcaster sink|token bucket|new {{kubernetes.eventQPS}} /
{{kubernetes.eventBurst}} (suggested default 200/400 — higher than kubelet's
{{eventRecordQPS}} 50/100 because a batch scheduler's event volume tracks
pods/s, roughly one event per scheduled pod; 50 would drop events in normal
operation at moderate throughput, 200/400 is lossless in steady state while
bounding storms). Bounded loss under storm is acceptable and now *possible
without capping binds*|
|bootstrap|2 startup ConfigMap GETs|n/a|fold into the writes client; the
dedicated 5/10-QPS clientset is vestigial|
Each client sets a distinct {{UserAgent}} ({{{}yunikorn-scheduler/writes{}}},
{{{}/informers{}}}, {{{}/events{}}}) for apiserver-side attribution (audit
logs, APF debug endpoints). ({{{}rest_client_*{}}} metrics are not
User-Agent-labelled; the client split itself is what would let per-client
metrics registries be distinguished, if metrics are ever registered.)
Design principle: server-side APF arbitrates all must-complete traffic (binds,
status updates). Client-side limiting is used only for *discardable* traffic
(events), where the terminal state under overload is "dropped" regardless of
who decides. Shedding at the source is free; sending an event to be rejected
costs a full round trip and APF seat-time on an already-stressed server, after
which the events/v1 broadcaster abandons it anyway (server rejections —
{{StatusError}} — are not retried: "Server rejected event (will not retry!)";
the 12-attempt retry loop applies only to network errors). APF *can* route the
shim's events to a separate cheap priority level via a resource-matching
FlowSchema rule (same ServiceAccount, {{{}resources: events{}}}), and the
companion APF proposal may ship one as defence in depth — but it complements
source-shedding rather than replacing it. The write-path knob is retained as an
opt-in policy cap, not a default: APF is present on every supported Kubernetes
version, and the rare deliberately-APF-disabled cluster
({{{}--enable-priority-and-fairness=false{}}}) is exactly the operator who can
be expected to set an explicit value.
h2. Risks / compatibility
* Connection overhead: ~nil — client-go caches transports keyed on connection
config, so additional clientsets with identical TLS/dial settings share the
underlying connection pool.
* Event delivery under sustained storm becomes explicitly and predictably
bounded (today the bounds are accidental: the broadcaster's 1000-entry
broadcast queue plus unbounded in-flight sink goroutines).
h2. Validation
* Unit: config plumbing (defaults, back-compat of {{{}kubernetes.qps{}}}).
* e2e: mass-failure event storm concurrent with a bind burst — assert bind
throughput is unaffected by event-path saturation (today they share a bucket);
assert informer relist latency is unaffected by an event backlog. A kind+KWOK
harness that produces both conditions exists from the API-server load
characterisation work and can be contributed.
was:
h2. Summary
The shim builds two general-purpose clientsets whose groupings follow code
structure rather than traffic type: one serves both the informers and the
events sink, the other serves writes, the volume binder, and predicate lookups.
Because each clientset gets its own rate limiter from the same
{{kubernetes.qps}} setting, (a) unrelated traffic classes share failure domains
— an event burst can starve informer watch re-establishment during recovery —
and (b) the effective request ceiling is 2× the configured value, which is
unlikely to be what operators expect.
Proposal: restructure into purpose-built clients — {*}writes/binds{*},
{*}informers{*}, *events* (and later *leader election* if it is ever added) —
each with its own rate limiter, sensible per-concern defaults, and a distinct
User-Agent.
h2. Current state (master, {{pkg/client}} / {{{}pkg/shim{}}})
* Bootstrap client ({{{}pkg/client/kubeclient.go:43{}}},
{{{}interfaces.go:58{}}}, called from {{{}bootstrap.go:30{}}}): two ConfigMap
GETs at startup. Sets no QPS, so no clientset-level limiter is created and it
runs on client-go's rest-layer defaults (5 QPS / 10 burst).
* Clientset 2 ({{{}pkg/shim/scheduler.go:67{}}}): serves *both* the
cluster-wide SharedInformerFactory ({{{}scheduler.go:70{}}}) *and* the
events/v1 broadcaster sink ({{{}scheduler.go:84-85{}}}).
* Clientset 3 ({{{}pkg/client/apifactory.go:93{}}}): serves *all writes*
(Bind/Create/Delete/UpdateStatus), the namespaced ConfigMap informer factory,
the volume binder ({{{}apifactory.go:122{}}}), and the predicate framework
handle.
Consequences:
# *{{kubernetes.qps}} is a per-clientset limit, so the effective ceiling is 2×
the configured value.* Each {{NewKubeClient}} call builds a fresh
{{rest.Config}} ({{{}kubeclient.go:55-69{}}}) and client-go creates a new token
bucket per clientset ({{{}client-go kubernetes/clientset.go{}}},
{{{}NewForConfigAndClient{}}}). An operator setting {{kubernetes.qps: 500}}
expecting to cap the scheduler at 500 req/s actually allows ~1,000. Any further
client splitting silently multiplies this again unless the config surface is
redesigned — so this restructuring and the config semantics must land together.
# *An event flood competes with watch re-establishment on the same token
bucket* (clientset 2). Mass-failure scenarios emit events in bursts precisely
when informers need tokens to reconnect and relist — the worst possible
coupling (see YUNIKORN-3355 for how much recovery correctness depends on timely
informer delivery). Severity scales inversely with the configured QPS: at the
1000/1000 default a mass-failure event burst (thousands of events) delays
informer re-establishment by seconds; at operator-lowered values (e.g. 50,
kube-scheduler parity) the same burst starves the watch path for minutes.
Splitting removes the coupling at every setting.
# {*}Events compete with nothing they should{*}: events are droppable by
design (the events/v1 broadcaster already has a bounded 1000-entry queue with
drop-on-full), yet today they can only be limited by limiting everything else
too.
# No {{UserAgent}} is set on any client (zero occurrences in non-test code),
so apiserver-side attribution (audit logs, per-client debugging) cannot
distinguish the shim's write path from its watch path.
h2. Proposed design
||Client||Serves||Limiter||Config||
|writes|Bind, Create/Delete, status updates, volume binder|token
bucket|{{kubernetes.qps}} / {{kubernetes.burst}} — {*}semantics preserved{*}:
this is the knob that caps how fast YuniKorn acts on the cluster (binds are
~1:1 with QPS tokens; measured 52 vs 1,170 binds/s at 50 vs 1000)|
|informers|all SharedInformerFactory watch/list traffic|*none* (unlimited)|—
steady-state request rate is intrinsically tiny (watches are long-lived; the
limiter charges per request, not per event); relist bursts are better governed
server-side by APF than by a client limiter that would slow recovery|
|events|events/v1 broadcaster sink|token bucket|new {{kubernetes.eventQPS}} /
{{kubernetes.eventBurst}} (suggested default 200/400; kubelet precedent
{{{}eventRecordQPS{}}}). Bounded loss under storm is acceptable and now
*possible without capping binds*|
|bootstrap|2 startup ConfigMap GETs|n/a|fold into the writes client; the
dedicated 5/10-QPS clientset is vestigial|
Each client sets a distinct {{UserAgent}} ({{{}yunikorn-scheduler/writes{}}},
{{{}/informers{}}}, {{{}/events{}}}) for apiserver-side attribution and cleaner
{{rest_client_*}} metrics if/when client-go metrics are registered.
h2. Interactions
* {*}Config semantics{*}: this issue narrows {{kubernetes.qps}} from "per
clientset, effectively ×2" to "write path only". Release notes must state both
the old effective behaviour and the new meaning. The documentation work for QPS
guidance should land as part of this change (one table: client / purpose / knob
/ default) rather than separately.
* {*}Hot-reload{*}: with per-concern defaults there is little reason left to
make QPS hot-reloadable; recommend dropping that idea rather than building
config-rebuild machinery for multiple clients. Server-side APF is the correct
dynamic control.
* {*}APF (future, optional phase 2){*}: APF distinguishes flows by {*}user{*},
so split clients on one ServiceAccount still share one queue position. Running
the events client under a separate identity would allow events to be placed in
a cheap priority level while the write path keeps a protected one. Not required
for this issue, but the client boundary drawn here is what makes it possible
later — worth keeping in mind during review.
* {*}Write dispatcher (KEP-5229-style, future){*}: a bounded write-dispatch
worker pool would sit naturally on the writes client; clean separation here
simplifies that later work.
h2. Risks / compatibility
* Connection count rises by ~2 HTTP/2 connections — negligible.
* Operators who relied (knowingly or not) on the 2× effective ceiling will see
the informer path become unlimited and the write path unchanged; net behaviour
change in normal operation is nil, but it should be called out.
* Event delivery under sustained storm becomes explicitly bounded (today it is
implicitly bounded by the broadcaster's 1000-entry drop-on-full queue anyway).
h2. Validation
* Unit: config plumbing (defaults, back-compat of {{{}kubernetes.qps{}}}).
* e2e: mass-failure event storm concurrent with a bind burst — assert bind
throughput is unaffected by event-path saturation (today they share a bucket);
assert informer watch re-establishment is unaffected by an event backlog. A
kind+KWOK harness that produces both conditions exists from the API-server load
characterisation work and can be contributed.
> Split the shim's Kubernetes clients by concern (writes / informers / events)
> ----------------------------------------------------------------------------
>
> Key: YUNIKORN-3356
> URL: https://issues.apache.org/jira/browse/YUNIKORN-3356
> Project: Apache YuniKorn
> Issue Type: Improvement
> Components: shim - kubernetes
> Reporter: Dale Richardson
> Priority: Major
>
> h2. Summary
> The shim builds two general-purpose clientsets whose groupings follow code
> structure rather than traffic type: one serves both the informers and the
> events sink, the other serves writes, the volume binder, and predicate
> lookups. Because each clientset gets its own rate limiter from the same
> {{kubernetes.qps}} setting, (a) unrelated traffic classes share failure
> domains — an event burst can delay informer relists during recovery — and (b)
> the effective request ceiling is 2× the configured value, which is unlikely
> to be what operators expect.
> Proposal: restructure into purpose-built clients — {*}writes/binds{*},
> {*}informers{*}, *events* - each with its own rate limiter, sensible
> per-concern defaults, and a distinct User-Agent.
> Scope note: this ticket is entirely client-side (shim code + config). It
> ships no FlowSchema/PriorityLevelConfiguration objects and requires none -
> all server-side APF work is proposed separately in a later proposal. APF is
> referenced below only as rationale for where client-side limits are and are
> not appropriate.
> h2. Current state (master, {{pkg/client}} / {{{}pkg/shim{}}})
> * Bootstrap client ({{{}pkg/client/kubeclient.go:43{}}},
> {{{}interfaces.go:58{}}}, called from {{{}bootstrap.go:30{}}}): two ConfigMap
> GETs at startup. Sets no QPS, so no clientset-level limiter is created and it
> runs on client-go's rest-layer defaults (5 QPS / 10 burst).
> * Clientset 2 ({{{}pkg/shim/scheduler.go:67{}}}): serves *both* the
> cluster-wide SharedInformerFactory ({{{}scheduler.go:70{}}}) *and* the
> events/v1 broadcaster sink ({{{}scheduler.go:84-85{}}}).
> * Clientset 3 ({{{}pkg/client/apifactory.go:93{}}}): serves *all writes*
> (Bind/Create/Delete/UpdateStatus), the namespaced ConfigMap informer factory,
> the volume binder ({{{}apifactory.go:122{}}}), and the predicate framework
> handle.
> Consequences:
> # *{{kubernetes.qps}} is a per-clientset limit, so the effective ceiling is
> 2× the configured value.* Each {{NewKubeClient}} call builds a fresh
> {{rest.Config}} ({{{}kubeclient.go:55-69{}}}) and client-go creates a new
> token bucket per clientset ({{{}kubernetes/clientset.go{}}},
> {{{}NewForConfigAndClient{}}}). An operator setting {{kubernetes.qps: 500}}
> expecting to cap the scheduler at 500 req/s actually allows ~1,000. Any
> further client splitting silently multiplies this again unless the config
> surface is redesigned — so this restructuring and the config semantics must
> land together.
> # *An event flood competes with informer relists on the same token bucket*
> (clientset 2). Watch calls themselves bypass the client rate limiter entirely
> ({{{}rest/request.go:763-764{}}} — watches are deliberately not throttled),
> but the *relist / initial LIST* after a watch break does pay a token — and
> the events/v1 broadcaster spawns a goroutine per event
> ({{{}tools/events/event_broadcaster.go:170-173{}}}), so a mass-failure burst
> can park thousands of concurrent token waits ahead of a recovery relist on
> the shared FIFO bucket. Order-of-magnitude impact (assuming a burst of a few
> thousand events with the bucket drained): seconds of relist delay at the
> 1000/1000 default, minutes at operator-lowered values (e.g. 50). Recovery
> correctness is sensitive to relist timeliness (see YUNIKORN-3355). Splitting
> removes the coupling at every setting.
> # {*}Events compete with nothing they should{*}: events are droppable by
> design, but today the only way to limit them is to limit everything else too.
> Note the broadcaster's bounded 1000-entry drop-on-full queue bounds only the
> broadcast side; in-flight sink recordings are per-event goroutines with *no*
> bound — a client-side event bucket introduces a real bound that does not
> exist today.
> # No {{UserAgent}} is set on any client (zero occurrences in non-test code),
> so apiserver-side attribution (audit logs, per-client debugging) cannot
> distinguish the shim's write path from its watch path.
> h2. Proposed design
> ||Client||Serves||Limiter||Config||
> |writes|Bind, Create/Delete, status updates, volume binder|*none by default*
> — APF governs this path on every supported Kubernetes version (on by default
> since 1.20, GA 1.29); {{{}kubernetes.qps{}}}/{{{}kubernetes.burst{}}}
> retained as an *opt-in* cap|Values <= 0 (the new default) install an explicit
> no-op limiter — note this must be
> {{{}flowcontrol.NewFakeAlwaysRateLimiter(){}}}, because leaving
> {{rest.Config.QPS}} at 0 silently applies client-go's 5/10 defaults. Positive
> values install a token bucket for operators with a deliberate policy reason
> to cap the scheduler (fleet limits, fragile downstream admission/audit
> infrastructure). Rationale: the limiter's only measured production effect is
> harm — 52 binds/s at qps 50, and even the 1000 default clips sustained peak
> (~1,170 binds/s measured only as a burst-window average) — while APF provides
> the actual protection. Method: 3,000–5,000-pod bind bursts on a kind+KWOK
> rig, 500 nodes; harness offered below.|
> |informers|all SharedInformerFactory watch/list traffic|*none* (unlimited)|—
> steady-state request rate is intrinsically tiny (watches are long-lived and
> bypass the limiter; the limiter charges per request, not per event); relist
> bursts are better governed server-side by APF than by a client limiter that
> would slow recovery|
> |events|events/v1 broadcaster sink|token bucket|new {{kubernetes.eventQPS}} /
> {{kubernetes.eventBurst}} (suggested default 200/400 — higher than kubelet's
> {{eventRecordQPS}} 50/100 because a batch scheduler's event volume tracks
> pods/s, roughly one event per scheduled pod; 50 would drop events in normal
> operation at moderate throughput, 200/400 is lossless in steady state while
> bounding storms). Bounded loss under storm is acceptable and now *possible
> without capping binds*|
> |bootstrap|2 startup ConfigMap GETs|n/a|fold into the writes client; the
> dedicated 5/10-QPS clientset is vestigial|
> Each client sets a distinct {{UserAgent}} ({{{}yunikorn-scheduler/writes{}}},
> {{{}/informers{}}}, {{{}/events{}}}) for apiserver-side attribution (audit
> logs, APF debug endpoints). ({{{}rest_client_*{}}} metrics are not
> User-Agent-labelled; the client split itself is what would let per-client
> metrics registries be distinguished, if metrics are ever registered.)
> Design principle: server-side APF arbitrates all must-complete traffic
> (binds, status updates). Client-side limiting is used only for *discardable*
> traffic (events), where the terminal state under overload is "dropped"
> regardless of who decides. Shedding at the source is free; sending an event
> to be rejected costs a full round trip and APF seat-time on an
> already-stressed server, after which the events/v1 broadcaster abandons it
> anyway (server rejections — {{StatusError}} — are not retried: "Server
> rejected event (will not retry!)"; the 12-attempt retry loop applies only to
> network errors). APF *can* route the shim's events to a separate cheap
> priority level via a resource-matching FlowSchema rule (same ServiceAccount,
> {{{}resources: events{}}}), and the companion APF proposal may ship one as
> defence in depth — but it complements source-shedding rather than replacing
> it. The write-path knob is retained as an opt-in policy cap, not a default:
> APF is present on every supported Kubernetes version, and the rare
> deliberately-APF-disabled cluster
> ({{{}--enable-priority-and-fairness=false{}}}) is exactly the operator who
> can be expected to set an explicit value.
> h2. Risks / compatibility
> * Connection overhead: ~nil — client-go caches transports keyed on
> connection config, so additional clientsets with identical TLS/dial settings
> share the underlying connection pool.
> * Event delivery under sustained storm becomes explicitly and predictably
> bounded (today the bounds are accidental: the broadcaster's 1000-entry
> broadcast queue plus unbounded in-flight sink goroutines).
> h2. Validation
> * Unit: config plumbing (defaults, back-compat of {{{}kubernetes.qps{}}}).
> * e2e: mass-failure event storm concurrent with a bind burst — assert bind
> throughput is unaffected by event-path saturation (today they share a
> bucket); assert informer relist latency is unaffected by an event backlog. A
> kind+KWOK harness that produces both conditions exists from the API-server
> load characterisation work and can be contributed.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
