feat(chart): expose foreman-agent rollout strategy and grace period per pool

[Defilan]

What

Expose the foreman-agent rollout strategy and termination grace period as
per-pool chart values.

Why

Refs #1438

agent-deployment.yaml hardcoded strategy: Recreate and
terminationGracePeriodSeconds: 30, so a routine image bump tore down every
replica of a pool at once, and there was no per-pool way to opt into a gentler
rollout.

Scope: the chart half only

No preStop hook, no operator change, deliberately. A preStop hook runs before
SIGTERM and its runtime counts against the grace period, and the agent's executor
dies with the SIGTERM (fleetnode.go patches phase=Draining on context
cancellation; it does not finish in-flight work). A preStop sleep would look like
a drain without being one. Making the agent outlive SIGTERM is an agent-side
design decision.

The race the old comment asserted

The comment justifying Recreate said it "stops a rolling update from briefly
running two agents that race for the same Scheduled tasks." That race is
currently unreachable, and only because of a bug.

FLEET_NODE_NAME is set from spec.nodeName but read by no Go code, so the
agent falls back to os.Hostname() — the pod name — and each replica registers
its own FleetNode. pollOnce then claims only tasks whose status.assignedNode
matches its own node, so two pods are never offered the same task. Under the
intended node-scoped identity they would share one FleetNode and the race would
be real. Filed as #1640.

So maxSurge: 0 is the conservative default, not an enforced invariant. An
earlier revision of this branch hard-failed a non-zero surge; that would be an
unbypassable policy ruling resting on a bug staying unfixed, so it is gone. If
#1640 is resolved toward node-scoped identity, this is worth revisiting.

values.yaml also now states something the option's own pitch obscured: at the
chart's default replicaCount: 1, RollingUpdate with maxSurge: 0 is
behaviourally identical to Recreate — the pool must scale to zero before the
new pod starts. It only helps at replicaCount > 1.

What the chart does fail the render for

Only values Kubernetes itself rejects, so the error arrives with a message rather
than at apply time:

Input	Why
strategy.type other than Recreate/RollingUpdate, case-exact	the API server rejects rollingUpdate too
maxUnavailable: 0 with maxSurge: 0, including 0%	"may not be 0 when maxSurge is 0"; 0% slips past a numeric check
negative or non-numeric terminationGracePeriodSeconds	API-rejected

Messages name the offending pool (agents.<pool>.strategy.type), since that is
the key to edit and bisecting a multi-pool values file by hand was the alternative.

Three silent-bad-manifest fixes

• Empty strategy: key is a null, not a dict, so dereferencing .type
  nil-pointered the entire render — the natural way to write "leave this at the
  default". Now falls back to Recreate.
• hasKey, not | default, for maxSurge/maxUnavailable. Go templates treat
  0 as empty, so default silently rewrote a deliberately configured 0.
• terminationGracePeriodSeconds | int64. Values arrive via fromYaml as
  float64 and %v switches to exponent form at 1e6; YAML 1.1 does not resolve
  1e+06 as a number, so the API server rejects the Deployment.

The rendered comment above strategy: is now accurate in both modes. It
previously shipped Recreate's rationale verbatim above a rendered
type: RollingUpdate, so every opted-in manifest contradicted itself.

Verification

• Default and multi-pool renders differ from upstream/main only in comment
  text (the corrected wording)
• helm unittest: 57/57, up from 46
• Mutation-tested: neutering both new guards fails exactly 3 tests
• Negative cases covered: null strategy:, maxUnavailable: 0, maxSurge: 25%,
  0+0%, negative/non-numeric/1e6 grace periods, per-pool message, and the
  deep-merge leak where a pool forcing Recreate inherits a global rollingUpdate

Checklist

• Tests added/updated
• make test passes locally
• make lint passes locally
• Commit messages follow conventional commits
• All commits are signed off (git commit -s) per DCO
• AI assistance (if any) is disclosed above, per CONTRIBUTING.md
• Documentation updated — values.yaml

Assisted-by: Claude Code via the Foreman harness (an agentic coder produced the
first implementation and one revision; an adversarial review found thirteen
defects including the false race premise, and I hand-fixed all of them, removed
the maxSurge hard-fail after establishing the race is unreachable, and ran the
renders, mutation tests and full chart suite before submitting).

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[joryirving]

This is close to mergeable and the reasoning is mostly right, but there's one real bug in the grace-period guard that would bite a multi-pool release, plus two places where values.yaml documents a mechanism the code doesn't have.

The bug: agent-deployment.yaml:54 gates on kindIs "float64" $tgps, and that's a type check standing in for a range check. Values that arrive through include "foreman.agents" | fromYaml are float64, so the implicit-pool path is fine, but $defaults = deepCopy $.Values.agent at line 12 is not round-tripped, and --set goes through Helm's strvals parser, which yields int64. So on any release using the agents: map, helm upgrade --set agent.terminationGracePeriodSeconds=60 fails the render with agents.default.terminationGracePeriodSeconds: 60 must be a non-negative integer, which is both wrong and pointing at a key the operator didn't set. I reproduced it with helm template t charts/foreman -f charts/foreman/tests/values-multi.yaml --set agent.terminationGracePeriodSeconds=60. Same value in a -f file renders fine, and --set agents.default.terminationGracePeriodSeconds=60 renders fine, which is what makes it easy to miss. Widen the guard to accept int64 and int alongside float64 and keep the lt (float64 $tgps) 0.0 arm as the actual range check. Note that helm unittest can't catch this class at all, because its set: round-trips through YAML, so the negative-value tests pass for the wrong reason; a helm template --set render step in the foreman job of helm-chart.yml would, and there's precedent for exactly that in the llmkube job at .github/workflows/helm-chart.yml:238.

On values.yaml:283, "raising this buys a longer window for a slow local-model turn to be interrupted cleanly by the scheduler's retry path" describes something that doesn't happen. On SIGTERM, watcher.Run returns on ctx.Done() at pkg/foreman/agent/watcher.go:141, the executor runs in a goroutine that nothing waits on (watcher.go:311), and the registrar's drain patch is capped at 5s (pkg/foreman/agent/fleetnode.go:293), so the process exits within a few seconds no matter what the grace period says. The knob is forward-wiring for #1438 and a way to lower the value; it does not currently lengthen anything. I'd say that plainly instead. Worth adding what actually recovers the killed task: not recoverOrphanedTasks, which only matches Status.AssignedNode == w.NodeName (watcher.go:177) and so never matches after a pod replacement, but the operator's claim expiry at internal/foreman/controller/agentictask_controller.go:774, which releases the task once the heartbeat goes stale and terminal-fails it on the third strike. That means a rollout during long runs spends expiry strikes, which is a genuine cost of rolling more often.

The other doc point is the maxSurge framing at values.yaml:296. Pinning the risk to the claim race undersells it. Under #1640 option (a), the sharper hazard is recoverOrphanedTasks: a surging pod that shares a node-scoped identity with a live sibling resets every one of that sibling's Running tasks to Pending at startup (watcher.go:165-190), so the scheduler re-dispatches work that is still executing and two agents push the same branch. That's worse than losing a Scheduled claim, and it's the sentence I'd want next to "raise it knowingly".

Smaller things. terminationGracePeriodSeconds: 30.5 passes the guard and truncates to 30 while the comment says non-integers fail the render. maxSurge: 1000000 from a values file renders 1e+06, which is exactly the float64 %v bug you fixed for the grace period, so the | int64 treatment arguably belongs on the numeric surge path too. And the null-strategy coverage doesn't bite: I deleted | default dict from line 41 and the suite still passed 57/57, because the two-step form ($strategy := ... then $strategy.type) tolerates a nil, and only the single-expression $agent.strategy.type nil-pointers. Keep the guard, but that fixture file is documentation rather than a test, and I'd either say so or drop it.

Credit where it's due, and I checked rather than assumed: default and multi-pool renders differ from upstream/main only in the comment text, helm lint is clean, and the guards are real. I neutered eight of them one at a time; six were caught (surge default, the maxUnavailable hasKey, | int64, the type validation, the 0/0 pair, and the Recreate-drops-rollingUpdate case, which caught five tests on its own). The FLEET_NODE_NAME finding holds up: the chart sets it at agent-deployment.yaml:158 and no Go code reads it, --fleet-node-name is never populated from the environment, so cmd/foreman-agent/main.go:279 falls back to the pod name. The pool-scoped merge behaves, including a pool overriding an inherited non-zero maxSurge back to 0. And the preStop argument is right; with the executor goroutine unawaited, a preStop sleep would be theatre.

Rebase note against #1635, since we're both editing the same three lines. Whichever lands second, the comment at agent-deployment.yaml:43-49 needs to keep #1635's blast-radius text: an agent holds one in-process task plus up to --max-supervised-tasks Job-mode supervisions, so an upgrade yanks back more than one run at a time. That matters more now that RollingUpdate is selectable, not less. The values.yaml additions don't collide (yours at 278, mine near 330). I'll also fix my own wording in that comment on rebase, because "recoverOrphanedTasks resets EVERY Running task on restart" is only true for an in-place container restart where the hostname survives; for a Deployment pod replacement it's the claim-expiry path that does it.

[Defilan]

The grace-period bug is real and I reproduced it before fixing: helm template ... -f values-multi.yaml --set agent.terminationGracePeriodSeconds=60 failed the render pointing at a key the operator never set, because --set goes through strvals and yields int64 while $defaults := deepCopy is not round-tripped. Widened to accept int64 and int, keeping lt (float64 $tgps) 0.0 as the actual range check. It now renders 60 across all three pools.

Pushed a2e45923d and a1dd18d79.

Your point that helm unittest cannot catch this class at all is the more important half, so the negative-value tests were passing for the wrong reason. Added a helm template --set render step to the foreman job following the precedent you pointed at in the llmkube job, so this fails CI in future rather than depending on someone widening a guard correctly.

On 30.5: I made the guard reject it rather than softening the comment. Silent truncation ships a value nobody asked for. | int64 now covers maxUnavailable as well as maxSurge, string-guarded so 25% passes through.

Corrected both doc claims. The grace period genuinely does not lengthen anything today given the unawaited executor goroutine and the 5s drain cap, and I have said so plainly along with what actually recovers the task, which is claim expiry rather than recoverOrphanedTasks, plus the cost that a rollout during long runs spends expiry strikes. The maxSurge comment now leads with the recoverOrphanedTasks hazard rather than the claim race.

On the null-strategy fixture: you are right that it does not bite. I labelled it as documentation of the render contract rather than pretending otherwise, because making it bite in isolation would mean putting the buggy single-expression form into the template just so a test could catch it.

[joryirving]

Re-reviewed at 289145c. The bug and both documentation points are fixed, and the CI addition is the part that makes it stick.

The guard now accepts float64, int64 and int and validates range separately, so --set agent.terminationGracePeriodSeconds=60 renders on a release using the agents: map. You also closed the smaller one I raised in passing: a fractional value is now rejected rather than truncated, so a 30.5 fails the render instead of silently shipping 30.

The new helm template --set step in helm-chart.yml is the right fix rather than just a fix. helm unittest structurally cannot catch this class, because its set: block round-trips through YAML and every number arrives as float64; only a real --set goes through strvals and arrives as int64. Without that step the next type guard has the same blind spot and the same green suite.

The grace-period documentation is now accurate about what actually recovers a killed task. Naming checkClaimExpiry explicitly, and saying plainly that it is not recoverOrphanedTasks because that matches only tasks whose status.assignedNode equals the agent's own FleetNode name, is the part that stops someone re-deriving the wrong mechanism. The note that a rollout during a long run spends one of the task's three expiry strikes is a real operational cost that was not written down anywhere before.

One note for sequencing rather than a change request: my #1635 has merged and shipped in v0.9.20, so the Recreate comment region this touches is now on main in its post-#1635 form. Whichever of us rebases, the blast-radius sentences should survive — an agent holds one in-process task plus up to --max-supervised-tasks Job-mode supervisions, so an upgrade can yank back more than one run at a time. That matters more now that RollingUpdate is selectable, not less.

Good to merge from my side.