fix(monitoring-agents): give kube-state-metrics a sized memory limit - #3846
fix(monitoring-agents): give kube-state-metrics a sized memory limit#3846myasnikovdaniil wants to merge 2 commits into
Conversation
It ran BestEffort with no resources at all, so it set no memory.max on its pod cgroup and the Talos userspace OOM handler kept it permanently in the victim set: any tenant workload driving node memory pressure could evict it, and it is the source of every kube-state metric we alert on. Numbers are measured rather than guessed. A 14d peak on a 3-node cluster with 322 pods and ~25.6k series is 307Mi working set, 280Mi RSS, 176Mi mean. The request sits above the mean and the limit carries wide headroom, because this component scales with object count and --metric-labels-allowlist widens every pod and deployment series further, so a larger cluster lands well above what was measured. No CPU limit, following the node DaemonSet sizing in c9ba7a3: CPU is irrelevant to the OOM ranking, and a binding ceiling on a scraper turns into scrape timeouts rather than backpressure. Supersedes #2632, whose 256Mi limit sits below the measured peak. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe monitoring-agents chart adds kube-state-metrics resource requests and a memory limit. A Helm test verifies these values and confirms that no CPU limit is configured. ChangesKube-state-metrics resources
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change adds measured memory requests and a limit for kube-state-metrics while preserving the intended absence of a CPU limit; no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Rendered it to check the claim chain: the limit lands on the only container in the pod, so pod-level memory.max is real. Sizing sits inside the series envelope — limit/peak 6.7x vs 6.1x/3.2x for node-exporter/fluent-bit, request/peak 0.63, cpu request 50m matching fluent-bit. The "no CPU limit, following c9ba7a3" claim checks out: that commit touches 19 files and none of them sets limits.cpu. helm unittest green.
Two questions, neither blocking:
- vmagent in this same package still has
resources: {}on both containers, and vpa-vmagent inupdateMode: Initialcannot synthesize a limit without a request-to-limit ratio — so it stays in exactly the perpetual-victim state this PR fixes for kube-state-metrics. Follow-up of the series, or deliberately left to the LimitRange? - Why 2Gi rather than the 4Gi LimitRange default from #3494? For the one component whose memory scales with object count, an explicit chart limit below the blanket default is the direction the docs call risky. A short "2Gi covers Nx the measured object count" note would close this.
Review asked why the chart pins 2Gi when the default LimitRange limit proposed in #3494 is 4Gi, and the sizing comment named the measured peak without saying how much room 2Gi leaves above it. 2Gi is 6.7x the measured 307Mi peak working set. Read as linear in object count, that is roughly 2100 pods and 170k series against the 322 pods and ~25.6k series the peak was measured on, so the limit stays a ceiling rather than a bound on a cluster several times larger than the one behind the numbers. Comment only. The requests, the limit and the deliberate absence of a CPU limit are all unchanged. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
Both non-blocking but the first one needed a correction to its premise. Conclusion right, mechanic half wrong. Victim state is real anyway. Pod cgroup on the node reads Follow-up of the series, not the LimitRange. #3494 is not merged so there is no backstop today, and it carries the side effect its own body names, wide defaulted ceiling against a small request hands vpa a ratio it then preserves. Note is in On the risky direction, I think that sentence is about a different layer. It sits in the Tuning section under the table defining No measurement forbids |
kube-state-metricsran BestEffort with no resources at all: the subchart shipsresources: {}and our wrapper never set any. So it put nomemory.maxon its pod cgroup, and the Talos userspace OOM handler only ever selects cgroups that lack one, which left it permanently in the victim set. Any tenant workload driving node memory pressure could evict the source of every kube-state metric we alert on. Background is indocs/operations/system-memory-limits.md.The numbers are measured
On a three node cluster with 322 pods and roughly 25.6k series, which is the smallest we run: 14 day peak working set 307Mi, peak RSS 280Mi, mean 176Mi.
So the request sits above the mean at 192Mi rather than at the peak, and the limit carries wide headroom at 2Gi. The headroom is deliberate: this component's memory scales with the number of objects in the cluster, and the
--metric-labels-allowlist=pods=[*],deployments=[*]already in this file widens every pod and deployment series further, so a cluster larger than the one measured lands well above what was seen here.2Giis 6.7x that peak, which read as linear in object count is roughly 2100 pods and 170k series, so the limit stays a ceiling rather than a bound well past the largest cluster this sizing was drawn from.No CPU limit, following the node DaemonSet sizing in
c9ba7a3cc, which set one zero times across nineteen files. CPU is irrelevant to the OOM ranking, and a ceiling that binds on a scraper turns into scrape timeouts rather than backpressure.This supersedes #2632
#2632 proposes 256Mi, which is below the 307Mi peak measured on the smallest cluster we have, so it would convert an eviction risk into a reliable OOM kill. It also adds a
cpu: 200mlimit, which is the half that buys nothing. The diagnosis there was right and it sat since May; the numbers are what needed replacing.Checks
packages/system/monitoring-agents/tests/kube_state_metrics_resources_test.yamlpins the limit, both requests, and the absence of a CPU limit. Mutation checked: dropping the limit to 256Mi turns it red. The chart's suites are 3 files and 6 tests, all green.Release note
Summary by CodeRabbit