[
https://issues.apache.org/jira/browse/YUNIKORN-3356?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
ASF GitHub Bot updated YUNIKORN-3356:
-------------------------------------
Labels: pull-request-available (was: )
> 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
> Assignee: Dale Richardson
> Priority: Major
> Labels: pull-request-available
>
> 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
> Server-side, every request from every client below is governed by APF
> unconditionally (on by default since K8s 1.20, GA 1.29) — mutating requests
> as seat-costed writes, relists as seat-costed LISTs, established watches
> streaming seat-free with their cost charged to writers via the fan-out
> surcharge. The table therefore describes only the *client-side* mechanism,
> and the design question per row is solely: does a client-side limiter add
> anything APF cannot provide?
>
> ||Client||Serves||Client-side limiter||
> |*writes*|Bind, Create/Delete, status updates, volume binder|none by default;
> {{kubernetes.qps}} / {{kubernetes.burst}} retained as opt-in cap|
> |*informers*|all SharedInformerFactory watch/list traffic|none; no knob
> offered|
> |*events*|events/v1 broadcaster sink|token bucket: new
> {{kubernetes.eventQPS}} / {{{}kubernetes.eventBurst{}}}, default 200/400|
> |*bootstrap*|two startup ConfigMap GETs|n/a — folded into the writes client|
> Per-client rationale:
> * *writes* — values <= 0 (the new default) install an explicit no-op
> limiter; implementation 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 for default-off: 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* — steady-state request rate is intrinsically tiny: watches are
> long-lived and client-go exempts WATCH from the client limiter anyway
> ({{{}rest/request.go:763{}}}); the only limitable calls are relists, which
> occur precisely when client-side delay damages recovery (see YUNIKORN-3355).
> There is no legitimate operator policy that wants relists delayed, hence no
> knob.
> * *events* — 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, while 200/400 is lossless in steady state
> and bounds storms. Bounded loss under storm is acceptable and now {*}possible
> without capping binds{*}.
> * *bootstrap* — the dedicated 5/10-QPS clientset is vestigial; its two
> ConfigMap GETs move to the writes client.
> 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 — three rules govern the table above:
> # *APF arbitrates everything that is sent.* On every supported Kubernetes
> version, all shim traffic — binds, status updates, relists, events — contends
> under APF, and that is desirable: only the server can allocate its capacity
> fairly against other clients' load, which no amount of client self-restraint
> can influence.
> # *Client-side limiting exists only to avoid sending discardable traffic.*
> Events are the sole discardable class: their terminal state under overload is
> "dropped" no matter who decides. The server's cheapest possible answer still
> costs both ends a round trip plus APF classification and queue bookkeeping,
> after which the events/v1 broadcaster abandons the event anyway (server
> rejections — any {{StatusError}}, including 429 — are never retried: "Server
> rejected event (will not retry!)"; its 12-attempt/10s retry loop covers only
> network errors, i.e. outages, where APF cannot act at all). Source-shedding
> reaches the same outcome for free and keeps working when the server is
> unreachable. The companion APF proposal may still route events to a cheap
> priority level (resource-matching FlowSchema rule, {{resources: events}},
> same ServiceAccount) as defence in depth for whatever the bucket admits — the
> two are complements, not alternatives: APF protects the scheduler from
> everyone else's load; the bucket keeps the scheduler's own discardable work
> from costing anything.
> # *Must-complete traffic is never self-throttled by default.* The write-path
> {{kubernetes.qps}} knob survives only as an opt-in policy cap — the measured
> cost of engaging it is 52 vs ~1,170 binds/s — 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. Caveat:
> default-unlimited slightly enlarges the blast radius of a hypothetical write
> hot-loop bug; containment is APF plus client-go's backoff, as it effectively
> is today — a limiter permitting 1,000+ binds/s never meaningfully contained
> it.
> 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]
