[jira] [Updated] (YUNIKORN-3356) Split the shim's Kubernetes clients by concern (writes / informers / events)

["Dale Richardson (Jira)" <ji...@apache.org>]

     [ 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: issues-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: issues-help@yunikorn.apache.org