tigerquoll opened a new pull request, #1060: URL: https://github.com/apache/yunikorn-k8shim/pull/1060
### What is this PR for? Implements [YUNIKORN-3356](https://issues.apache.org/jira/browse/YUNIKORN-3356): replaces the shim's two general-purpose clientsets — whose groupings followed code structure rather than traffic type — with purpose-built clients, each with its own rate-limit policy and a distinct, versioned User-Agent. Previously, `kubernetes.qps` applied to each clientset independently (effective ceiling 2× the configured value), an event burst shared a token bucket with informer relists, events could not be limited without limiting binds, and no client was identifiable server-side. This PR has a follow up that provides a suggested APF configuration for the Yunikorn helm chart. | Client | Serves | Client-side limiter | |---|---|---| | writes | Bind, Create/Delete, status updates, volume binder, predicate handle | none by default; `kubernetes.qps`/`kubernetes.burst` as opt-in cap (`<= 0` = unlimited) | | informers | cluster-wide + namespaced informer factories (one shared clientset) | none; no knob | | events | events/v1 broadcaster sink | shed above `kubernetes.eventQPS`/`kubernetes.eventBurst` (new keys, default 200/400), plus server-informed muting on 429 | | bootstrap | two startup ConfigMap GETs (both binaries) | unlimited; `yunikorn-bootstrap` user agent | ### Design rationale Server-side API Priority & Fairness (APF) governs every request on every supported Kubernetes version (default-on since 1.20, GA 1.29) and, unlike a client rate limit/token bucket, is fair across tenants and work-aware. We can take advantage of that by: 1. **Having server side APF arbitrate everything that is sent.** Client-side limiting is retained only where it does something APF cannot. 2. **Disable existing client-side throttling of critical traffic generation (binds, status), and receiving (Informers)** We should not try and guess the state of the server — we should simply take heed of the 429 metadata dynamically suggesting a slowdown period. Informers will simply not receive as much traffic if APF is slowing down traffic, as the entities actually generating traffic will be limited by APF, meaning there is less traffic for the informer to receive. 3. **We only use client-side limiting to avoid sending discardable traffic.** Events are the sole discardable class: ~1 per scheduled pod (about half the shim's write volume), spiking exactly during incidents. Dropping events is the one action Yunikorn can do to lower K8S API Server load that does not compromise on core scheduling functionality. ### How events are shed A standard token bucket "delay on limit" implementation for the events clientset would be wrong: the events/v1 broadcaster spawns a goroutine per event, so delaying traffic converts traffic backlogs into unbounded goroutine growth with nothing ever dropped. Instead we configure a qps/burst bucket to manage event traffic, but we drop events rather than delay them if the event client is over budget. Once APF is configured separately for events, rather than relying on a statically configured rate limit, we can rely on the server itself to tell us to back off (by sending 429 status codes with a suggested slow down period). In this case we would drop events for the nominated cool down period rather than delay them. The event dropping is implemented inside the event sink object. The 429 status detection and removal is done via an http round-tripper added to the clientset. Just to re-emphasise, only the event generation path gets this treatment. For must-complete traffic, the k8s client-go's Retry-After default retransmission behaviour is left in place and remains untouched. ### Behaviour changes - `kubernetes.qps`/`kubernetes.burst` now apply **only to the write path**; defaults change **1000/1000 → unlimited** (rationale above). Deployments relying on the implicit 1000 cap should set it explicitly. - Upgrade corner cases — what happens to a config already deployed under the old semantics: | Existing config | Old behaviour | New behaviour | |---|---|---| | `burst` set, `qps` unset | qps silently defaulted to 1000, so the burst value applied | no limiter; burst ignored with a startup warning¹ — set `kubernetes.qps` to restore one (burst then defaults to the same value) | | `qps: "0"` explicitly | client-go's hidden 5/10 fallback limiter (surely never intended) | unlimited | | `qps` set, `burst` unset | effective qps/1000 | qps/qps — same sustained rate, less bursty | | `qps` set, `burst: 0` | crashed client construction (startup crash-loop) | burst defaults to qps, with a warning | ¹ burst is a bucket *size*, qps its *refill rate* — without a positive refill rate there is no limiter for the burst value to shape. For the new event keys the same convention applies to the rate: `eventQPS <= 0` disables shedding, while an unset `eventBurst` falls back to its own default (400) rather than to `eventQPS`. - **Informer traffic is no longer client-side limited** and has no knob (rationale above); operators who capped `kubernetes.qps` to protect the apiserver should use APF placement instead (companion proposal). - **Admission controller**: clients run unlimited — its traffic is a trickle (small, low-churn informers plus rare webhook/secret writes; the old 1000/1000 limiter never engaged), the scheduler ConfigMap keys never actually applied to it (it loads its own config), and it sits in the pod-admission critical path where client-side throttling only delays webhook readiness. Its webhook client previously ran on client-go's hidden 5/10 fallback — now explicitly unlimited — and all its traffic carries `yunikorn-admission-controller`. - **All clients send distinct User-Agents** (`yunikorn-scheduler/writes`, `/informers`, `/events`, `yunikorn-bootstrap`, `yunikorn-admission-controller`; build version appended) for apiserver-side attribution. ### Type of change - Improvement ### Jira issue Jira ID : https://issues.apache.org/jira/browse/YUNIKORN-3356 - [x] I have created a Jira issue for this pull request. - [x] The Jira ID is part of the title of this pull request. ### How was this patch tested? - `make test` (18 packages) and `make lint` clean; both binaries cross-compile. - Unit: every `rateLimitPolicy` branch; `newRestConfig` plumbing; both `UserAgent()` version branches; sink shed/pass-through/mute paths (fake clock); transport strip/record (httptest, asserting a single request in ~10ms vs 11 in 10s); integration-level crash-loop regression through `kubernetes.NewForConfig`. - Live on kind (K8s v1.36.1) + KWOK (500 nodes): - defaults: 3,000-pod bind burst at ~1,900 binds/s; four clients logged once each with correct policies; - `qps: 100` without burst: boots with warning and a working 100/100 limiter (crash-looped before the fix); cap enforces ~134 binds/s; - event storm: 9,805 events shed with one throttled warning; steady-state delivery lossless; - APF squeeze (2,394 rejections served): server escalated Retry-After to 31–32s, mute engaged (`delaySource: transport`), 869 events shed in one window, no goroutine accumulation (vs ~360 parked pre-transport). ### Questions: - [x] The change needs documentation, a pull request for apache/yunikorn-site repository will be created. - [x] There is breaking changes for older versions: jira is tagged with `release-notes` label. - [ ] The licenses files needs to be updated. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
