fix(chart): pin CronJob memory requests==limits so a node OOM stops culling them (backend#2935)

[aptracebloc]

Closes tracebloc/backend#2935

What was happening

ap-workspace-auto-upgrade-* was exit-137 (SIGKILL) killed on every hourly tick on a live ap-workspace edge, and image-refresh failed in lock-step — six paired failures. image-refresh runs every 15m, so failing only when it coincides with the hourly upgrade tick is itself the tell: one shared cause, not two independent container-limit overruns.

Diagnosis (measured)

1. Both declare limits — auto-upgrade 256Mi, image-refresh 128Mi.
2. Neither outgrew its limit; no leak/unbounded read. Measured peaks (/usr/bin/time -l): helm ~50 MiB (repo update + search + template render), kubectl ~40–80 MiB, jq ~2 MiB — each a fraction of its limit. Both scripts read only small bounded JSON and stream through awk.
3. Shared cause = a node-level OOM. Both were Burstable, they die together, and the telemetry reason is Error, exit code 137 (not OOMKilled) — the signature of the kernel OOM-killer under node memory pressure, not either container hitting its own cgroup limit.

The fix — make both CronJobs Guaranteed QoS

Two independent levers, each doing an honest, distinct thing (this is the shape after @saadqbal's review — the first revision pinned memory only and mis-credited it):

1. Memory request raised to meet the limit = an honest scheduler reservation. This attacks the cause: the node is no longer overcommitted by the gap between what these pods reserved and what they use, so it's less likely to enter the pressure that triggered the kill.
2. CPU pinned too ⇒ Guaranteed QoS ⇒ oom_score_adj = -997. This is what takes the pod off the kernel OOM-killer's shortlist under pressure. A memory pin alone does not — a Burstable pod's oom_score_adj = 1000 − 1000·memRequest/nodeCapacity is fixed at start and independent of usage, so 128Mi→256Mi moves auto-upgrade only ~992→984 (16Gi node) and image-refresh 999→998 (large node), i.e. nothing.

These two CronJobs are the only chart pods one line from Guaranteed — zero initContainers, memory already equal, cpu the sole blocker. (#642 pinned jobs-manager's memory but couldn't reach Guaranteed there — it renders an unresourced init container; here we can.)

auto-upgrade:  requests cpu 50m→500m, mem 128Mi→256Mi   (limits unchanged 500m/256Mi)  → Guaranteed
image-refresh: requests cpu 20m→200m, mem  64Mi→128Mi   (limits unchanged 200m/128Mi)  → Guaranteed

Limits never moved — only requests rose to meet them, so no too-low cap was introduced. Tradeoff (stated in values.yaml): Guaranteed reserves the full envelope for the pod's brief run and caps it there; helm/kubectl are I/O-bound so the cpu cap doesn't slow real work, and Forbid + startingDeadlineSeconds bound the Pending case if a small edge can't spare the reservation.

Guard / test

auto_upgrade_test.yaml and image_refresh_test.yaml now assert cpu AND memory request==limit on each CronJob pod — the in-manifest proxy for Guaranteed given no initContainers (client#922's pod-qos-class checker will assert the class directly once it lands). Reverting either request reddens the matching assert. Also corrected the #554 comment. Chart version+appVersion bumped to 1.9.93 (develop advanced to 1.9.92 under this branch; merged and re-bumped one patch above).

Verification

• helm unittest ./client — 646 passed (incl. the strengthened guards).
• helm lint ./client — 0 charts failed.
• CI on the prior push was green incl. Bugbot, Fleet auto-upgrade E2E, closing-ref gate, version-bump gate.

Notes for the reviewer

• Ceiling adequacy for the heaviest op is still an untested assumption. Measured client-side helm ops (~50 MiB), not a live helm upgrade --atomic --wait. All evidence says node-pressure, so the limit was deliberately not bumped; if a live edge ever shows a >256Mi peak (it'd surface as OOMKilled), the ceiling is the thing to raise — orthogonal to the QoS fix.
• Fix-the-class: other Burstable-by-memory pods exist (requests-proxy, resource-monitor, the egress-*/storage-assertions check hooks); they were not observed OOMing and are out of scope here. Worth a look only if fleet-wide pressure is confirmed.

Decision gate (recorded, not folded in)

• "Do" item 1 (why the node was under pressure) — the pressure source isn't identifiable from the failure telemetry (we know it wasn't these two containers; the prime suspect is a spawned training pod whose envelope dwarfs the control plane). Landed as a line on backend#2935 with what to capture next time; the QoS fix makes these two survivors of pressure, it doesn't remove the cause.
• "Do" item 2 (visibility of a stalled upgrade path regardless of environment) — flagged on backend#2920, where the dev-exclusion policy lives.
• "Do" item 3 (fleet not swept) — only ap-workspace inspected; recommended a fleet check on backend#2935.

---

Note

Medium Risk
Changes scheduler reservations and OOM victim preference for the security-fix auto-upgrade path on memory-tight edges; behavior is intentional but can leave CronJobs Pending if the node cannot honor Guaranteed envelopes.

Overview
Addresses backend#2935, where auto-upgrade and image-refresh CronJob pods were exit-137 killed together on a live edge—consistent with node-level OOM, not cgroup limit breaches (measured peaks stayed well under existing limits).

Resource requests for both CronJobs are raised so CPU and memory requests equal their existing limits, moving the pods to Guaranteed QoS (oom_score_adj -997) and reserving memory honestly on the node. Limits are unchanged (auto-upgrade stays 500m/256Mi; image-refresh 200m/128Mi). Tradeoff: tighter scheduler reservation on small edges; pods may go Pending rather than OOM-loop.

Helm unittest cases in auto_upgrade_test.yaml and image_refresh_test.yaml lock request==limit for CPU and memory so the QoS fix cannot regress. The #554 pending-upgrade recovery comment in auto-upgrade-cronjob.yaml is updated to mention node OOM and Guaranteed QoS. Chart version/appVersion bump to 1.9.93.

^{Reviewed by Cursor Bugbot for commit 638b977. Bugbot is set up for automated code reviews on this repo. Configure here.}

[saadqbal]

Good diagnosis — the paired-failure timing argument (6 image-refresh failures out of ~24 runs,
exactly the ones coinciding with the hourly tick) and the Error vs OOMKilled distinction are
both right, and I agree this is a kernel node-OOM rather than either container overrunning its
own cgroup. The peak-RSS measurements are the right instinct too.

But the remedy doesn't reach that mechanism, and the comment says it does. oom_score_adj for a
Burstable pod is 1000 - 1000*memRequest/nodeCapacity — a function of the request against node
capacity, set once at container start. It does not depend on whether the pod is running above
its request, which is what the values.yaml note asserts. So 128Mi→256Mi moves auto-upgrade
993→985 on a 16Gi node, in a 3..999 band; image-refresh 64Mi→128Mi on a 64Gi node moves 999→999,
i.e. nothing. Guaranteed is −997. "A node under pressure no longer prefers it" isn't what this
buys.

And these two are the only pods in the chart one line from Guaranteed — I checked: zero
initContainers on either CronJob, memory now equal, cpu the sole blocker
(helm:cpu(req=50m,lim=500m)). The #642 precedent doesn't transfer, because jobs-manager
renders an unresourced init container and can't reach Guaranteed however you set cpu; these can.
"Share weight, throttled not killed" is true in isolation, but cpu equality is also the QoS
precondition, and that's the part that matters here.

So: either pin cpu and take Guaranteed, stating the reservation-vs-throttle tradeoff, or keep the
memory pin and rewrite the claim as what it actually delivers — an honest scheduler reservation
that reduces node overcommit, which is a real structural fix for #2935's cause and currently
framed as a side-note. I'd take the first for auto-upgrade; it's the security-fix delivery path.

Also nothing establishes why the node was under pressure. #2935's "Do" item 1 is half-answered
— we know it wasn't these two containers, we don't know what it was — and I'd rather that line
landed on the issue before it closes.

Rest is clean and the safety work is solid. No new top-level values key, so no nil-guard needed.
Limits never moved and requests rose to meet them, so no too-low cap was introduced. Guards are
mutation-proof both ways — reverting the request, then separately raising only the limit, each
reddens the right assert. And on the coupling worry: #922's full 19-test suite and all five
goldens pass verbatim against this branch, both CronJobs still Burstable, so no golden moves and
the two can land in either order. Forbid plus startingDeadlineSeconds already bound the
Pending case from your operator note.

Nit: the tests assert two literal values rather than the relation or the class. With #922's
checker landing in the same window the class is derivable in-repo — and "asserted the values
believed to imply the class" is the exact phrase in its docstring.

[aptracebloc]

Thanks @saadqbal — this was exactly right, and I took the first option. Pushed 036ecf1 (+ a develop merge, 638b977).

oom_score_adj mechanism / the claim. You're correct and I verified the arithmetic: 1000 − 1000·memRequest/nodeCapacity, fixed at start, usage-independent → auto-upgrade 992→984 on 16Gi, image-refresh 999→998 on a large node. The memory pin does not buy "the node no longer prefers it." Fixed both the mechanism and the framing:

• Went Guaranteed on both (pinned cpu req==limit: auto-upgrade 50m→500m, image-refresh 20m→200m; memory was already equal). oom_score_adj = -997 is now what actually moves them off the shortlist.
• Rewrote the values.yaml notes to credit each lever with what it delivers: memory request = honest reservation that reduces node overcommit (the structural half — #2935's cause); Guaranteed's −997 = the victim-preference change (the mechanism). The old "running above its request draws a high oom_score_adj" line is gone, with a pointer to this review.

#642 doesn't transfer. Agreed — noted in the comment that jobs-manager renders an unresourced init container and can't reach Guaranteed however cpu is set, whereas these two have zero initContainers and can, which is why they get the stronger fix.

Reservation-vs-throttle tradeoff is now stated in values.yaml: Guaranteed reserves the full envelope for the brief run and caps it there; helm/kubectl are I/O-bound so the cap doesn't slow real work; limits never moved (only requests rose to meet them); Forbid + startingDeadlineSeconds bound the Pending case.

"Do" item 1 (why the node was under pressure). Landed on backend#2935: we know it wasn't these two containers, we can't identify the source from the failure telemetry (no node memory in hand), prime suspect is a spawned training pod whose envelope dwarfs the control plane — with the describe node / per-pod-RSS to capture next time. Called out that the QoS fix makes these two survivors of pressure but doesn't remove the cause. Offered to split the source hunt into a follow-up.

Tests nit. Guards now assert cpu AND memory request==limit on both pods (the in-manifest proxy for Guaranteed given no initContainers), not just memory literals. Left a note that client#922's pod-qos-class checker asserts the class directly once it lands — happy to switch these to call it in a follow-up so we're not asserting the values-that-imply-the-class.

Re-requesting your review.

[aptracebloc]

bugbot run

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 638b977. Configure here.}

[LukasWodka]

Read this at 638b977d. The diagnosis is right and the correction Asad asked for landed properly — not approving only because his CHANGES_REQUESTED is still standing (recorded at 563037df; GitHub dismisses stale approvals on push, not change-requests, so it needs him to clear it). Everything below is so the re-read is short.

The oom_score_adj arithmetic now in the comment is correct, and it was worth correcting. Burstable is 1000 - 1000 * memRequest / nodeCapacity, fixed at pod start and independent of actual usage, so on a 16Gi node 128Mi → 256Mi moves ~992 → ~984 — noise against a Guaranteed pod's -997. The earlier framing credited the memory pin with the victim-preference change; it doesn't do that, the CPU pin does, by moving the pod into Guaranteed. Two levers, two different mechanisms, and the current comment separates them cleanly: the memory request is an honest reservation that reduces node overcommit (attacking the cause), the CPU pin is what moves it off the kernel's shortlist (attacking the symptom). Worth having both, and worth having said which does which.

I checked the tradeoff you flag rather than taking the reassurance. Reserving 500m/256Mi for a brief hourly run can make the pod unschedulable on a small edge, and with concurrencyPolicy: Forbid a single stuck-Pending Job would block every later tick — which for the security-fix delivery path would be a quieter failure than the OOM loop it replaces. It's genuinely bounded: activeDeadlineSeconds is set on both CronJobs (max(900, timeout+300) on auto-upgrade, default 3600 on image-refresh), and a Job's deadline runs from startTime regardless of whether a pod ever scheduled, so a Pending Job self-terminates and releases Forbid. The chart already says as much a few lines up. So the tradeoff is real, acknowledged, and has a working brake — no objection from me.

One gap in the guard, smaller than the one you already named. The comments say the assertion is a proxy for Guaranteed because there are no initContainers — true, I checked both templates. But the QoS class depends on every container in the pod, and the tests assert containers[0] only. Today each CronJob renders exactly one container (helm, refresh), so the proxy holds; add a sidecar tomorrow and the assertions stay green while the class silently drops to Burstable — the same shape of hole as the initContainer one, via a different door. Not worth a commit here given client#922 lands the direct pod-qos-class checker, but the comment naming only initContainers slightly overstates what the proxy covers.

Two things I verified and am happy with: Helm unit tests ran (16s, pass) so the new assertions actually executed rather than being skipped, and both new tests fail if either request is reverted — each resource has its own equal, so the mutation is per-lever rather than one line covering both.

Gate otherwise: 24 pass, 0 failing, 0 pending, MERGEABLE, zero unresolved threads. Ready from my side the moment Asad clears his review.

[saadqbal]

This is the right shape now — thanks for taking the cpu pin rather than just softening the prose.
I checked the class rather than the values: #922's pod-qos-class.py, which implements
ComputePodQOS including init containers, derives Guaranteed for both t-auto-upgrade and
t-image-refresh off this branch's render on all five profiles. Single container, zero
initContainers each, so the class is real, and the comment now credits the right lever to the
right mechanism. Clearing my review.

Two things before you merge, neither of them code.

Closes tracebloc/backend#2935 no longer matches the ticket. Lukas retitled it at 09:35 — after
your last push — and the headline is now 431x exit 1 against 46x exit 137. This PR fixes the
46. I checked whether the exit-1s might be downstream of the OOM kills and they aren't: the
pending-upgrade wedge path deliberately exit 0s (#2877), so the 431 are the LATEST/CURRENT
resolution failures under set -eu, and nothing here touches them. Make it Part of and give the
exit-1 half its own ticket. Cross-repo keywords don't auto-close here — every prior one was closed
by hand — which is exactly how someone reads "Closes" and closes it anyway, and that edge is still
missing security fixes.

Merge order against #922, and this reverses what I told you last pass. Then, the memory-only pin
moved no golden. The cpu pin does: all five pod-qos-expect.*.txt files move two rows each —
class t-auto-upgrade and class t-image-refresh, Burstable -> Guaranteed, ten rows total. On the
merged tree #922's suite goes 19/19 to 15/19 (tests 1, 2, 3, 18). The init rows don't move, and
qos-reachability-expect.txt stays 20/20 both ways because it already recorded both as reachable.
Land #922 first and let this PR carry the ten rows, or land this first and expect #922 to. Chart.yaml
collides either way — #922 is at 1.9.94 and you're at 1.9.93, so the second one re-bumps.

Nit, and it argues for #922 landing first: the comment says the proxy holds because there are no
initContainers, but the tests read containers[0] only. That's Lukas's point and it's the same hole
through a different door — add a sidecar and these stay green while the class drops. #922's checker
closes it properly.

Nit: the arithmetic is off by one, in the direction that understates your own argument. kubelet
integer-divides on bytes, so 16Gi is 993->985 rather than 992->984, and image-refresh on a 64Gi node
is 999->999 — the request rounds to zero and the 1000 is clamped. Literally no movement, which is
the stronger version of the point you're making.

Good call landing the "Do item 1" comment on #2935 bounding the node-pressure cause as not
identified rather than guessing — what we know, what we don't, the unconfirmed suspect, and what to
capture next time. That's the part that stops this being re-diagnosed from scratch.