Repository navigation
Expose cache sync timeout and API client QPS/burst as settings - #289
Conversation
f9d107d to
d742b16
Compare
|
No need for a LBCacheSyncTimeout config on Controller Manager layer, it just complicates things without any real gain. LB Deployment Template based configuration is the standard approach. |
91876e3 to
46727de
Compare
There was a problem hiding this comment.
I don't think at the moment I can add to how this change affects the internals, so I focused on the user facing docs. If you agree with the option rename, that should be done before merge. The rest is just suggestions about docs, so they can be delayed as seen fit.
| | `--kube-api-qps` | `MERIDIO_KUBE_API_QPS` | `-1` (disabled) | Sustained client-side request rate towards the API server. Values `<= 0` disable client-side rate limiting. | | ||
| | `--kube-api-burst` | `MERIDIO_KUBE_API_BURST` | `30` | Client-side burst allowance. Only effective when `--kube-api-qps > 0`. | |
There was a problem hiding this comment.
May make sense to state that we just expose the go k8s client's config options, so the meaning of these options is actually defined there. Could even link to it. This is mentioned in the next section, but it would be useful fact to know here already. This applies to docs/controllers/*.md as well.
I think MERIDIO_KUBE_CLIENT_QPS and MERIDIO_KUBE_CLIENT_BURST (that is: s/API/CLIENT/) would be slightly better names, because 1) they make it clear that throttling happens on the client side, and 2) make the above link to go k8s client clearer from the option names already.
There was a problem hiding this comment.
Good call on the client-go link. Done: both options are now documented as straight pass-throughs to rest.Config.QPS / rest.Config.Burst, stating that client-go defines their meaning and that Meridio-2 adds nothing of its own beyond the zero-normalization. Applied in docs/operations/scale-tuning.md and in docs/controllers/gateway.md plus docs/controllers/loadbalancer.md.
On the rename I'd like to push back, though I'm happy to be outvoted. --kube-api-qps / --kube-api-burst is the established upstream Kubernetes name for exactly this client-go setting:
| Component | Flag | Source |
|---|---|---|
| kubelet | --kube-api-qps, --kube-api-burst |
cmd/kubelet/app/options/options.go |
| kube-controller-manager | --kube-api-qps, --kube-api-burst |
cmd/kube-controller-manager/app/options/ |
| kube-scheduler | --kube-api-qps |
cmd/kube-scheduler/app/options/deprecated.go |
The kubelet's own config field is kubeAPIQPS (k8s.io/kubelet/config/v1beta1/types.go).
So the current names are what operators already recognise from every other Kubernetes component, and they're what #215 asked for. I think your goal 1 (making the client side explicit) is better served by the help text and docs, which already say "client-side", than by diverging from that convention, and goal 2 is now covered by the godoc link.
There was a problem hiding this comment.
I did not realize the option names are already present in other places. Thanks for letting me know! Knowing this, I fully agree to keep the 'api' variation.
|
|
||
| ### A note on QPS and burst | ||
|
|
||
| Client-side rate limiting is **disabled by default and was never active** in Meridio-2. |
There was a problem hiding this comment.
I'm confused by this part. If we never had client side throttling, then how did we get to see the issue in the first place? If we started to see it because somebody, somehow hacked client side throttling in there (without Meridio-2 properly supporting it) then this section should be an email to them, because nobody else will ever see this problem. It feels like you started this change believing we had client side throttling (I guess that's what the issue suggested), later realized we did not, so you changed the default to -1 (that's good), but the documentation was not fully updated to reflect this realization.
| [BGP Authentication (TCP-AO)](../controllers/router.md#bgp-authentication-tcp-ao). | ||
| In a namespace that also holds unrelated Secrets (Helm release data, image pull | ||
| credentials, service account tokens) this can dominate the router's cache and its sync | ||
| time. The router currently exposes no tuning flags; if its sync becomes a problem, the |
There was a problem hiding this comment.
I don't really get the mitigation claim.about dedicated namespace. But our controllers' resolution chain is namespace locked. Basically, everything (Pods, DGs, Secret) must exist in the same namespace as the Controller Manager.
At scale (~4000 L34Routes, ~300 DistributionGroups) the controller-manager
and stateless-load-balancer can fail to start because the initial informer
cache sync does not finish within controller-runtime's 2 minute deadline:
ERROR Could not wait for Cache to sync
{"controller": "endpointnetworkconfiguration",
"source": "kind source: *v1alpha1.L34Route",
"error": "timed out waiting for cache to be synced"}
Add a shared KubeAPIConfig, embedded into ManagerConfig and
LoadBalancerConfig, exposing --cache-sync-timeout (MERIDIO_CACHE_SYNC_TIMEOUT),
--kube-api-qps (MERIDIO_KUBE_API_QPS) and --kube-api-burst
(MERIDIO_KUBE_API_BURST). The timeout is applied manager-wide via
ctrl.Options.Controller.CacheSyncTimeout, which controller-runtime propagates
to every controller that does not set its own value, so no controller code
changes are needed. RESTConfig() replaces the bare ctrl.GetConfigOrDie() calls
and applies the rate limiting settings; both binaries log the effective values
at startup.
QPS defaults to -1 (disabled) rather than the 20 suggested in the issue.
Client-side rate limiting was never active here: controller-runtime's
GetConfig() sets QPS to -1 and client-go only builds a token bucket limiter
when QPS is positive. Defaulting to 20 would introduce a throttle that does
not exist today, and raising QPS cannot speed up a slow cache sync because
there is no client-side throttle to lift. The flags therefore exist to
deliberately throttle this client. A literal 0 is normalized to -1, since
client-go reads a zero QPS as DefaultQPS (5) and would silently throttle hard.
Router and network-sidecar are left unchanged: neither watches L34Routes and
both hold small caches.
Also document the failure mode, the CPU-throttling versus slow-API-server
diagnosis, and per-scale-tier resource and timeout starting points in
docs/operations/scale-tuning.md, with commented examples in the
controller-manager and LB deployment manifests.
Refs #215
Signed-off-by: Botond Szirtes <botond.szirtes@est.tech>
Follow-up to the review of the cache sync tuning work on this branch. The manager flag alone only fixed half the problem. LB Pods are not launched by the operator but rendered by the gateway controller from the LB deployment template, so an operator who set --cache-sync-timeout left every LB Pod at the 2m default with nothing to indicate it. Add --lb-cache-sync-timeout (MERIDIO_LB_CACHE_SYNC_TIMEOUT) to the controller-manager. When set, the gateway controller injects MERIDIO_CACHE_SYNC_TIMEOUT into the loadbalancer container of every LB Deployment it manages, reusing the env var the LB binary already reads. The default of 0 leaves the rendered Deployment byte-identical, and a value already present in the template always wins, so a manual template edit remains a valid override. Setting it rolls the LB Pods, since it changes the Pod template. Documentation and manifest corrections: - The stateless-load-balancer flag table was missing the three tuning flags added by the previous commit. - The scale-tuning component table understated the router: it watches Gateways, GatewayRouters and, unfiltered, every Secret in the namespace, which can dominate its cache. The cached-kind lists now match the actual watch sets. - The shipped controller-manager memory limit of 128Mi contradicted the 512Mi baseline the tuning guide recommends for the smallest tier. Raise it to 512Mi, matching the LB template. - Template edits do not take effect on the next reconcile as previously documented. Nothing watches the template ConfigMap, so a change lands only after the kubelet refreshes the mount and the next Gateway event, which with the default 10h resync may not be imminent. Two small consistency fixes: CheckGatewayAPI now takes the REST config from the caller instead of calling GetConfigOrDie, so the discovery call uses the same tuned client as the manager and the config is loaded once; and the kube API validation error is wrapped like its siblings in validateConfig. Signed-off-by: Botond Szirtes <botond.szirtes@est.tech>
Per review feedback the controller-manager does not need its own knob for the LB's cache sync timeout. Revert the propagation added in the previous commit: drop --lb-cache-sync-timeout (MERIDIO_LB_CACHE_SYNC_TIMEOUT), the LBCacheSyncTimeout reconciler field and the injectLBCacheSyncTimeout rendering step, together with their tests and the now-unused container name and env var constants. LB Pods are instead tuned by setting MERIDIO_CACHE_SYNC_TIMEOUT on the loadbalancer container in the LB deployment template, which the LB binary already reads; the shipped template carries a commented example. The value survives both the create and the update render path, since reconcileDeploymentSpec overwrites containers from the freshly loaded template on every reconcile. The manager's --cache-sync-timeout does not reach the LB Pods, so the two have to be raised together. scale-tuning.md now states that explicitly instead of describing the retired flag. Documentation fixes carried in this commit: - gateway.md documented --lb-cache-sync-timeout but none of the manager's own tuning flags. Document --cache-sync-timeout, --kube-api-qps and --kube-api-burst, and drop the retired entry. - The tuning table recommended 10m for the largest tier, which meets the Deployment default progressDeadlineSeconds of 600s and can make GitOps tooling report a legitimately syncing rollout as failed. Lower the suggestion to 9m and explain the interaction. - The startup log example showed an lbCacheSyncTimeout key that no binary emits any more. Signed-off-by: Botond Szirtes <botond.szirtes@est.tech>
Two review follow-ups on the cache sync tuning work. The manager built its REST config, and through the Gateway API discovery call made its first API server contact, before validating the configuration those client settings came from. Nothing broke in practice, but only because applyRateLimits ignores a non-positive burst, so the ordering was backwards rather than harmless by design. Split validate into the checks that need no cluster access (config values, pod cache label, and the LB deployment template on disk) and a new checkClusterPrerequisites holding the Gateway API CRD discovery, then reorder run to validate, build the REST config, check prerequisites. An invalid configuration is now rejected before any API server traffic. A missing CRD also no longer reports under the misleading config validation prefix, since it is not a config problem. The stateless-load-balancer returned its ValidateKubeAPI error bare while the manager wrapped it, leaving the operator with a message that did not say which configuration area had failed. Wrap it the same way. Cover the kube API validation path with a test, as it now runs ahead of any cluster contact. Signed-off-by: Botond Szirtes <botond.szirtes@est.tech>
d4cae35 to
558d410
Compare
Review feedback on the scale tuning guide. The QPS and burst options were documented alongside the cache sync timeout, inside a guide about fixing a cache sync failure, implying a connection that does not exist. Split the configuration section into the cache sync timeout, which is the actual remedy, and client-side rate limiting, which now opens by stating that it is not one. Drop the claim that rate limiting was never active in Meridio-2. That sentence answered a review question rather than an operator's, and it was the main source of confusion in the section. The two facts worth keeping remain: raising QPS cannot speed up a slow sync, and a zero QPS is normalized to -1 because client-go reads zero as DefaultQPS of 5. For the record, the 20 QPS / 30 burst defaults cited in the issue are kube-controller-manager's, set in its own defaults.go, not controller-runtime's. controller-runtime deliberately sets QPS to -1, which is why the shipped default preserves that behaviour. Document both options as straight pass-throughs to the Kubernetes Go client, setting rest.Config QPS and Burst, with a link to the godoc, in the scale tuning guide and in the gateway and loadbalancer controller docs. client-go is the authoritative definition of their meaning and that should be discoverable from the option names. Also drop the suggestion to mitigate the router's unfiltered Secret watch by deploying Gateways into a dedicated namespace. The controllers' resolution chain is namespace locked, so Pods, DistributionGroups and Secrets all have to live in the controller-manager's namespace, which makes that advice unworkable. Signed-off-by: Botond Szirtes <botond.szirtes@est.tech>
zolug
left a comment
zolug
left a comment
There was a problem hiding this comment.
looks good to me
(Probably a follow-up issue to extending KubeAPIConfig to the router and sidecar worth to be added, and the sync timeout for the router would make sense as well)
3 participants
Background
At scale (~4000 L34Routes, ~300 DistributionGroups) the controller-manager and
stateless-load-balancer can fail to start, because the initial informer cache sync does not
finish within controller-runtime's 2 minute per-controller deadline:
ERROR Could not wait for Cache to sync
{"controller": "endpointnetworkconfiguration", "source": "kind source: v1alpha1.L34Route",
"error": "timed out waiting for cache to be synced"}
The sync must List and deserialize every watched object before any controller starts
reconciling. Both the object count and CPU throttling stretch the wall-clock duration while
the deadline stays fixed.
What this adds
A shared
KubeAPIConfig, embedded intoManagerConfigandLoadBalancerConfig:--cache-sync-timeoutMERIDIO_CACHE_SYNC_TIMEOUT2m--kube-api-qpsMERIDIO_KUBE_API_QPS-1(disabled)--kube-api-burstMERIDIO_KUBE_API_BURST30--cache-sync-timeoutis applied manager-wide viactrl.Options.Controller.CacheSyncTimeout, whichcontroller-runtime propagates to every controller that does not set its own value, so no
controller code changes were needed.
RESTConfig()replaces the barectrl.GetConfigOrDie()calls and applies the rate limiting settings; both binaries log the effective values at startup.
Please confirm: the QPS default deviates from the issue
The issue proposes
--kube-api-qpsdefault20. This PR defaults to-1(disabled)instead, because client-side rate limiting was never active here: controller-runtime's
GetConfig()setsQPS = -1, and client-go only builds a token bucket limiter when QPS ispositive. The
20/30figures in the issue are kube-controller-manager's own defaults(
pkg/controller/apis/config/v1alpha1/defaults.go), not controller-runtime's; this PR followscontroller-runtime instead, since that is what Meridio-2 is built on. Defaulting to
20wouldintroduce a throttle that does not exist today, and raising QPS cannot speed up a slow
cache sync because there is no client-side throttle to
lift. The flags therefore exist to deliberately throttle this client, e.g. on a shared API
server. A literal
0is normalized to-1, since client-go reads a zero QPS asDefaultQPS(5) and would silently throttle hard.
This contradicts the written proposal, so it needs an explicit ack rather than a silent merge.
LB tuning is template-based
LB Pods are not launched by the operator — the gateway controller renders them from the LB
deployment template. An earlier revision of this PR added a
--lb-cache-sync-timeoutmanagerflag to propagate the setting; per review feedback that flag has been dropped, since the LB
deployment template is already the standard way to configure LB Pods and a second manager-level
flag for this one setting added complexity without real gain.
To tune the LB's cache sync timeout, set
MERIDIO_CACHE_SYNC_TIMEOUTdirectly on theloadbalancercontainer'senvin the template (a commented example ships in the template).The LB Pods roll when the value changes, since it alters the Pod template. The manager's
--cache-sync-timeoutdoes not propagate to the LB Pods; the two have to be raisedtogether, or the LB Pods silently stay at the 2m default.
Binaries intentionally unchanged
Router and network-sidecar: neither watches L34Routes. Note that the router's cache is not as
small as the issue assumed — its Secret watch is unfiltered, so it caches every Secret in the
Gateway's namespace. That is pre-existing and now documented. There is no workable mitigation:
the controllers' resolution chain is namespace-locked, so isolating Secrets via a dedicated
Gateway namespace is not an option.
Documentation and manifest corrections
docs/operations/scale-tuning.md: the failure mode, CPU-throttling vs slow-API-serverdiagnosis, per-tier timeout and resource starting points, and how to apply the settings.
docs/controllers/loadbalancer.mdflag table was missing the new flags;gateway.mdnowdocuments
--cache-sync-timeout,--kube-api-qpsand--kube-api-burstas well.GatewayRouters and all Secrets); cached-kind lists now match the actual watch sets.
config/controller-manager/manager.yamlshipped a 128Mi memory limit, contradicting the512Mi baseline the guide recommends for the smallest tier. Raised to 512Mi, matching the
LB template. Requests are unchanged at 64Mi, so there is no scheduling or QoS change.
template ConfigMap, so a change lands only after the kubelet refreshes the mount and the
next Gateway event, which with the default 10h resync may not be imminent.
--cache-sync-timeout=10m. Neither the LBtemplate nor the controller-manager manifest sets
progressDeadlineSeconds, so theKubernetes default of 600s applies;
10mrisks GitOps tooling reporting a legitimatelysyncing rollout as a failed one. Lowered to
9m, with the interaction explained in the guide.Two small consistency fixes:
CheckGatewayAPInow takes the REST config from the callerinstead of calling
GetConfigOrDie, so the discovery call uses the same tuned client and theconfig is loaded once; and the kube API validation error is now wrapped the same way on both
binaries (
fmt.Errorf("kube API tuning: %w", err)).Out of scope
achievable without either mandating labels on user-owned L34Routes or depending on CRD
selectableFields(K8s 1.31+). Documented as a known limitation; worth a separate issue.bind*helpers swallow parse errors, soMERIDIO_CACHE_SYNC_TIMEOUT=300(no unit) keeps the 2m default. Pre-existing behaviour forall config env vars; hardening it is a behaviour change for all four binaries and belongs in
its own PR. The startup log line is the current mitigation.
GatewayConfigurationalongsideverticalScaling, so sizing andtimeout live in one user-facing object.
This change was created with AI assistance.
Fixes issue: #215