[
https://issues.apache.org/jira/browse/YUNIKORN-3356?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Dale Richardson updated YUNIKORN-3356:
--------------------------------------
Summary: Split the shim's Kubernetes clients by concern (writes / informers
/ events) (was: Split the shim's Kubernetes clients by concern (writes /
informers / events) with per-concern rate limits and User-Agents)
> 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 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.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
