Repository navigation
feat(observability): warn before a node reaches the memory ceiling - #1196
Conversation
NodeHighMemory fires at >90% held for 15m. Measured on 2026-09-12, worker-hnn peaked at 89.8% over 7 days and nothing fired: the peak was under the threshold, and as a short spike it would not have survived the 15m hold either. The source was concurrent mctl-agents-implement pods -- one sat at its 8Gi container limit -- and it was real anonymous memory, not reclaimable page cache (MemTotal minus MemFree/Cached/Buffers reached 14100Mi of 15612Mi). No OOMKill, so the only trace was in the metric. NodeMemoryHeadroomLow adds the tier below: >85% held for 10m, warning. NodeHighMemory keeps its 0.9/15m unchanged -- this sits under it rather than replacing it. The alertname is added to the alertmanager mctl-agent route in the same commit, otherwise the rule would fire into nothing. mctl-agent's classifyAlert does not know the name yet, so it classifies as generic until that repo follows; the alert still routes and still notifies. Unit tests assert both directions and are mutation-proven: raising the threshold to 0.95 fails the firing case, and shortening the hold to 1m fails the spike case (which is why the spike test asserts at 5m, inside the spike -- the 12m assertion alone passes at any hold length). Claude-Session: https://claude.ai/code/session_01Dix3wU7BjCgs85hYe6z85U
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude finished @mashkovd's task in 2m 38s —— View job Review complete ✅
SummaryReviewed the diff against the pinned commits (base
Checked the expression math, series values ( Findings: 1 P3 (non-blocking) — posted inline on No P1/P2 issues found. Approved (commit |
Antigravity (agy) review — informationalModel: gemini-3.1-pro-high · commit No significant issues found. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # 0.9 and, as a short spike, would not have survived its for: 15m | ||
| # either. A ceiling that is only visible once it is hit is not a | ||
| # warning, so this rule makes the approach to it visible instead. | ||
| - alert: NodeMemoryHeadroomLow |
There was a problem hiding this comment.
P3: The file header (lines 1-5) states alert names must stay in sync with mctl-agent/internal/monitor/alerthandler.go::classifyAlert. NodeMemoryHeadroomLow isn't added there, so it falls through to default/TypeGeneric/SeverityWarning until a follow-up PR lands. The PR description discloses this as a deliberate gitops-first sequencing choice, which is reasonable, but consider adding a one-line reference to the follow-up (issue/PR link) near this rule or in the header so the gap doesn't get silently forgotten.
There was a problem hiding this comment.
Addressed in 2b1b55dc: the follow-up now exists as mctlhq/mctl-agent#131 and the comment above the rule names it instead of promising it.
Worth recording why this is more than a naming gap, which the review's framing understates: an unlisted node alert classifies as TypeGeneric, and alerthandler.go's fallback then sets service = alertName (it is skipped only for TypeResourceLimit). With a non-empty Service the alert no longer satisfies isInfraAlert()'s TypeResourceLimit + empty Service gate in pipeline.go — the gate that routes node alerts manual-only rather than into cpu_throttle's auto-fix path. So the miss changes routing, not just the ticket label.
mctl-agent#131 carries the switch entry, the sibling enumeration in the comment that documents the exclusion, and a mutation-proven test row (dropping the name back out fails with type = "generic", want "resource_limit").
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
b9c4992 to
67a9982
Compare
|
Claude finished @mashkovd's task in 2m 24s —— View job Review complete ✅
SummaryReviewed
Findings:
No issues found in the memory-alert work itself ( VerdictREQUEST_CHANGES submitted against commit |
|
Skipping — this commit (67a9982) already has a Claude review verdict. Push a new commit, or check the run log if you expected a fresh pass and the previous run actually failed. |
| # the flag and docs/ingress-header-measurement.md are all deleted in the same | ||
| # pass; leaving it on costs log volume on an unauthenticated path for no | ||
| # measurement. | ||
| MCP_HEADER_PROBE: "true" |
There was a problem hiding this comment.
P1: MCP_HEADER_PROBE: "true" is turned on here, but image.tag on line 15 of this same file is still "0.64.2". The comment directly above this key (line 142) says "Needs 0.65.0 or later; earlier images ignore the key." As written, this env var is a silent no-op — the Phase 3 correlation measurement this commit exists to enable will not actually run until a follow-up bumps the image tag to >=0.65.0. Given the PR's own theme this cycle (a measurement that produces no signal and nothing flags it), worth bumping the tag in the same commit, or at minimum calling out the required follow-up bump explicitly so it isn't forgotten.
| # Telegram OIDC/session login rather than this password bypass, and must | ||
| # carry per-account send_enabled=false so probes stay read-only even though | ||
| # ALLOW_SEND is true globally. | ||
| # TEMPORARY, mctl-telegram#617 Slice 1 (Phase 3 correlation, .github#35). |
There was a problem hiding this comment.
P2: This commit (b9c4992b, "enable the temporary inbound-header probe for #617 Slice 1") is unrelated to this PR's stated purpose (node memory headroom alerting) and to 67a9982e. It touches a different tenant service (labs/mctl-telegram) for a separate initiative (mctlhq/mctl-telegram#617), bundled onto the feat/node-memory-headroom-alert branch via a later push. It also adds an unrelated 7-line comment to vm-rules/mctl-alerts.yaml (referencing mctlhq/mctl-agent#131) that has nothing to do with either topic. Recommend splitting this onto its own branch/PR — it enables new request logging on an unauthenticated ingress path for a different service, which deserves its own focused review and its own audit trail rather than riding in under the memory-alert title.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 1 P1, 1 P2. The head commit (b9c4992) is an unrelated 'enable inbound-header probe for mctl-telegram#617' change bundled onto this memory-alert branch — it sets MCP_HEADER_PROBE: "true" in labs/mctl-telegram/values.yaml while that file's own image.tag is still 0.64.2, and its own added comment says the flag needs >=0.65.0 or is silently ignored (P1). It also mixes an unrelated service/initiative into this PR's diff and history (P2, scope hygiene). The original NodeMemoryHeadroomLow alert work in 67a9982 is unaffected and was already reviewed/approved separately.
… follow-up claude[bot]'s P3 on this PR: the file header requires alert names to stay in sync with mctl-agent's classifyAlert, and the review asked for a pointer so the follow-up is not forgotten. It now exists -- mctlhq/mctl-agent#131 -- so the comment names it rather than promising it. Also records why the gap matters beyond naming: an unlisted node alert classifies as TypeGeneric, takes the service=alertName fallback, and therefore misses isInfraAlert's "TypeResourceLimit + empty Service" manual-only gate every other node alert depends on. Claude-Session: https://claude.ai/code/session_01Dix3wU7BjCgs85hYe6z85U
|
Correction on this PR's history, since it is visible in the timeline and would mislead anyone reading it. An earlier push put commit The intended change is now on the branch as Any |
Antigravity (agy) review — informationalModel: gemini-3.1-pro-high · commit No significant issues found. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude finished @mashkovd's task in 2m 38s —— View job Automated review — commit
|
Antigravity (agy) review — informationalModel: gemini-3.1-pro-high · commit No significant issues found. |
There was a problem hiding this comment.
No P1/P2 findings (0 P3). This commit only adds explanatory comments referencing the classifyAlert follow-up (mctlhq/mctl-agent#131); the underlying NodeMemoryHeadroomLow rule, route, and tests were already reviewed and are unchanged since. Good to merge.
Why
A node-metrics sweep of
mctl-preprodon 2026-09-12 found one anomaly: worker-hnn peaked at 89.8% memory over 7 days and nothing alerted.NodeHighMemoryfires at>0.9heldfor: 15m. The peak passed under the threshold, and as a short spike it would not have survived the 15m hold either. There was no OOMKill anywhere in the cluster over the window, so the metric was the only trace.What was actually happening
Per-pod
max_over_time(...[7d:5m])on hnn, namespaceargo-workflows:mctl-agents-implement-43dfb607-run-implementer-……-69b99015-……-d72e6387-…One implementer sat at 8134 Mi against its 8Gi container limit. And this is not reclaimable page cache: on the node itself,
MemTotal − MemFree − Cached − Buffersreached 14100 Mi of 15612 Mi (90.3%).The mechanism is the request/limit gap in
cwft-mctl-agents-implement.yaml— the scheduler reservesmemory: 1Giwhile the container may grow to8Gi, and the implementer step carries no mutex (the two in that template cover the claim step and the gitops write). That part is not in this PR; see "Follow-up" below.What changes
vm-rules/mctl-alerts.yaml— newNodeMemoryHeadroomLow:>0.85,for: 10m,severity: warning.NodeHighMemorykeeps0.9/for: 15muntouched; this sits below it rather than replacing it.bootstrap/templates/observability/monitoring.yaml— the alertname is added to the alertmanagermctl-agentroute in the same commit, otherwise the rule would fire into nothing.vm-rules/tests/mctl-alerts_test.yaml— three cases.Tests are mutation-proven
Both directions were verified to actually fail when the rule is wrong:
0.85→0.95FAILED: a node holding 87% memory for 10m firesfor: 10m→for: 1mFAILED: an 87% spike shorter than the 10m hold does not fire, time: 5mThe second is why the spike case asserts at 5m, inside the spike — a 12m-only assertion passes at any hold length, because the series has already recovered by then. A test that can only pass is not a test.
scripts/check-vm-rules.shand its--selftestboth pass;helm templateconfirms the new alertname renders into the route.Known gap
mctl-agent'sclassifyAlertdoes not knowNodeMemoryHeadroomLow, so it falls todefault→TypeGeneric/SeverityWarning. The alert routes and notifies correctly; only the ticket type is less specific until a follow-up PR inmctl-agentadds the name next toNodeHighCPU/NodeHighMemory. Gitops-first is deliberate here — the rule is inert without the route, and harmless without the classifier.Follow-up (not this PR)
Raising the implementer's memory request
1Gi → 3Gicloses the scheduler-side gap. It touches the exactresources:block that open PR #1194 rewrites, so it lands after #1194 merges rather than conflicting with it.🤖 Generated with Claude Code
https://claude.ai/code/session_01Dix3wU7BjCgs85hYe6z85U