Add per-function policy overrides via the Function header by scott-rc · Pull Request #137 · gadget-inc/skipper

scott-rc · 2026-05-05T05:36:13Z

Tenants on Skipper run very different workloads -- long-running jobs, latency-sensitive request paths, batch throughput, idempotent vs. non-idempotent calls -- but heartbeat timeout, HPA tuning, router retry policy, function-assignment timeout, and PASETO token TTL are all global controller / router flags that every tenant inherits. This branch carves those decisions per-function via the existing x-skipper-function header.

Solution

The Function proto gains four grouped policy sub-messages alongside the existing Scale:

HpaPolicy -- tolerance, downscale_stabilization, initial_readiness_delay
HeartbeatPolicy -- timeout (the *Policy suffix disambiguates from the top-level skipper.Heartbeat RPC envelope; the other three carry it for symmetry)
ProxyPolicy -- max_attempts, retry_min_backoff, retry_max_backoff
LifecyclePolicy -- assign_timeout, token_ttl

Each knob has a single resolver method on *Function (fn.HeartbeatTimeout(clusterDefault), fn.HPATolerance(clusterDefault), etc.). Resolvers use proto Has*() accessors so unset fields fall back to the cluster default while a tenant who explicitly chooses zero (strict tolerance=0, instant scale-down, no startup grace, no retry backoff) is honored. Controller and router decision sites call these accessors instead of reading cfg.* directly.

Function.Validate rejects negative durations and inverted backoff bounds. For the four fields where zero would drive runtime to a degenerate state -- heartbeat.timeout, lifecycle.assign_timeout, lifecycle.token_ttl, proxy.max_attempts -- explicit zero is also rejected; omit the field to fall back to the cluster default.

Header as source of truth

The x-skipper-function header carries policy alongside identity. No new CRD, no new control surface. FunctionHash depends only on identity fields, so a tenant changing a knob updates the in-memory state on the next request via the existing last-writer-wins path -- no new pod pool, no new identity.

Cluster default story

Cluster flags become defaults; tenants who omit a sub-message see identical behavior to today. A new --token-ttl controller flag (default 168h) makes the token lifetime operator-tunable. No caps or bounds on overrides beyond the validation rules above -- operator caps can layer on later if abuse surfaces.

Two heartbeat-timeout reads in the controller stay on the cluster flag and document why at the call site: the router-heartbeat GC tracks per-router liveness (each supervisor handles one function; the map crosses every router whose heartbeats arrive for that function), and the controller-wide startup grace is a fleet-startup concern, not per-function.

Snapshot consistency

Every per-function decision in a converge tick reads from one snapshot. recordRecommendation, cleanupStuckInstances, and the metric-target lookup in calculateDesiredInstancesForMetric all receive *Function explicitly, so a tenant who tightens or loosens a per-function field mid-flight drives the next tick's behavior immediately, without waiting for instance replacement. The compiler enforces this -- per-function decision sites take *Function as a parameter rather than reading live policy from s.fn.Load() or instance.GetFunction().

Decision-site observability

The HPA decision context (in converge) and the router's per-attempt retry context (in RoundTrip) attach the resolved (effective) policy values as structured attributes, so logs reveal whether a tenant override or the cluster default drove a given decision. When the runtime resolver produces an inverted retry-backoff pair (tenant retry_min_backoff exceeds the cluster retry_max_backoff ceiling), clampBackoffBounds returns wasInverted and RoundTrip emits a log.Warn once per call so the silent demotion of the tenant's minimum is observable.

Out of scope

PASETO token consolidation (folding the entire Function into signed claims), router HTTP transport tuning, web-UI surfacing of the new sub-messages, hash-ring virtual-node count, operator caps / bounds flags, and any new CRD or external config store.

Extend Function with HpaPolicy, HeartbeatPolicy, ProxyPolicy, and LifecyclePolicy sub-messages so tenants can override cluster-wide heartbeat, HPA, retry, and lifecycle defaults via the in-flight x-skipper-function header. Each knob has a single resolver method on *Function that returns the function value when set and the cluster default otherwise; Validate rejects negative durations and inverted backoff bounds. Decision sites still read cluster flags directly -- later phases route them through the resolvers.

Route the controller's per-function idle / scale-to-zero decision through the resolver: calculateDesiredInstances now takes *Function and reads fn.HeartbeatTimeout(cfg.HeartbeatTimeout); the heartbeat half of the scale-down protectionPeriod follows suit. Tenants who set heartbeat.timeout get their override; tenants who omit it keep today's cluster-default behavior. Router-heartbeat GC and the controller-wide startup grace stay on the cluster flag -- the comments at each site explain why.

Route the controller's HPA decisions through the resolver: calculateDesiredInstancesForMetric now takes *Function and reads fn.HPATolerance and fn.HPAInitialReadinessDelay, recordRecommendation prunes the stabilization window using the supervisor's current function spec, and the scale-down protectionPeriod resolves both halves of the max via fn.HPADownscaleStabilization and fn.HeartbeatTimeout. The converge breadcrumb adds resolved policy values to the log context so a reader can tell whether a tenant override or the cluster default drove a given decision.

Route the router's RoundTrip retry loop and calculateBackoff bounds through the resolver: the request's *Function drives max_attempts and the min / max backoff window, falling back to the cluster flags when proxy policy is unset. Each attempt's log line carries the resolved values so debugging can tell whether the cluster default or a tenant override drove the curve.

Add the --token-ttl flag (7-day default) and route the controller's PASETO expiration through fn.TokenTTL so tenants with strict rotation needs can shorten the issued token's exp claim. Move the assign-timeout reads at pod.go and the stuck-instance cleanup threshold at supervisor.go to fn.AssignTimeout so slow-cold-start tenants can extend the deadline without bumping the cluster default.

The adjusted-tolerance check in calculateDesiredInstancesForMetric was still reading cfg.HPATolerance instead of the local resolved tolerance variable, so a tenant setting a tight hpa.tolerance would hit the cluster default in the missing-metric path. Read through the resolved variable consistently with the basic check, and add a regression test that exercises the adjusted-ratio branch with same-sign ratios so the divergent decision is visible.

scott-rc · 2026-05-05T05:36:15Z

This change is part of the following stack:

Add per-function policy overrides via the Function header #137 ◀

_{Change managed by git-spice.}

The protection-period max in converge's scale-down branch must wait long enough for routers to send heartbeats to a freshly-started controller before treating heartbeat silence as scale-to-zero. Router heartbeat propagation is a cluster-wide concern -- the router's heartbeat interval is not per-function -- so the heartbeat half of the max reads s.ctrl.config.HeartbeatTimeout instead of the per-function resolver. This matches the oneshot startup grace at supervisor.go's oneshot branch, which already stays on the cluster flag for the same reason. The stabilization half stays per-function: tenants who tighten that window opt into thin data.

recordRecommendation re-loaded s.fn to resolve HPADownscaleStabilization, but converge already captured fn := s.fn.Load() under s.mu and updateFunction CAS-writes s.fn without the mutex. A concurrent policy swap between the two loads would prune the stabilization window against a different snapshot than the scaling decision and protection period used. Thread the captured fn through recordRecommendation so one snapshot drives the entire converge tick. The compiler now enforces the invariant -- callers must pass the function explicitly.

A negative tolerance silently disables the HPA dead-band -- the guard usageDiscrepancy <= tolerance can never be satisfied because usageDiscrepancy is non-negative via math.Abs -- so a tenant who sets hpa.tolerance = -0.5 sees the HPA scale on every converge tick. Reject negative values at validation, matching the pattern already used for durations and inverted backoff bounds.

calculateDesiredInstances now reads scale targets, oneshot, and the min / max clamp from the explicit fn parameter rather than from heartbeat.GetFunction(), so a future caller cannot accidentally apply one function's policy to another function's scale targets. Router-side, Function.Validate cannot catch inverted backoff bounds when the tenant sets only one half of the pair and the cluster default for the other half is on the wrong side. Resolve both halves at the RoundTrip site and clamp via clampBackoffBounds so calculateBackoff never receives an inverted pair -- the operator's hard ceiling on a single backoff stays in force when the tenant's minimum exceeds it.

cleanupStuckInstances read the assign timeout from instance.GetFunction(), which is the function serialized into the pod annotation at assignment time -- not the converge tick's fresh snapshot. A tenant who shortens lifecycle.assign_timeout after assignment expects existing stuck instances to be cleaned up faster, but the per-instance lookup stranded them behind the old (longer) annotation policy. Thread fn through cleanupStuckInstances, matching recordRecommendation's pattern, so every per-function decision in a converge tick reads from one snapshot.

Resolvers fell back to the cluster default when the function-side value was zero, which silently overrode tenants who legitimately chose zero (strict tolerance, instant scale-down, no startup grace, no retry backoff). Use the proto Has*() accessors to distinguish "unset" from "explicitly zero" -- explicit zeros now drive behavior while omitted sub-messages and unset fields continue to fall back.

calculateDesiredInstancesForMetric reads target_cpu_usage_milli (and the memory equivalent) from the converge-tick fn argument instead of instance.GetFunction(). The annotation snapshot is the function serialized into the pod at assignment time; reading it meant a tenant who tightened or loosened the target mid-flight got the old target until every instance was replaced. The fn parameter is already in scope, so the fix is a one-site swap. Adds CPU and memory regression tests that update the target mid-test and assert the new target drives the desired-instance count.

Two small observability changes on the per-function policy layer. Rewrite the router-heartbeat GC comment in supervisor.go: a Supervisor handles one function, so the map crosses every router (not every function). Cluster-default usage stays -- the GC tracks router liveness, which is governed by the cluster-wide router heartbeat interval, not by any tenant's idle timeout. Make clampBackoffBounds report whether it fired: it now returns (min, max, wasInverted bool), and RoundTrip emits a log.Warn once per call when wasInverted is true. Without this signal, an operator debugging "my retry_min_backoff has no effect" had nothing to grep for: when a tenant set only retry_min_backoff against a cluster default for retry_max_backoff that was shorter, the runtime clamp silently demoted the tenant's minimum to the cluster ceiling.

calculateDesiredInstancesForMetric had three switch-on-metric blocks: one in the per-instance filtering loop, one for the target lookup, and one in the per-instance accumulator loop. Dispatch once at the top to a (target, sample func) pair, then use the closure everywhere. Same behavior under the existing suite; the function is shorter and the per-iteration branch is gone.

After the resolver swap to presence checks, a tenant who explicitly sets heartbeat.timeout, lifecycle.assign_timeout, lifecycle.token_ttl, or proxy.max_attempts to zero drives runtime to a degenerate state: instant scale-to-zero, immediately-cancelled assignment context, already-expired tokens, "failed after 0 attempts" on every request. Validate now requires these four fields to be > 0 when set; tenants who want the cluster default leave the field unset, as before.

cursor

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 31c1fd0. Configure here.}

replaceStaleInstances used proto.Equal on the full Function to detect staleness, which compared the four new policy sub-messages alongside metadata and scale. Any tenant updating a policy knob (heartbeat, hpa, proxy, lifecycle) would diverge from the annotation captured at assignment time and mark every running pod as stale, triggering a rolling replacement of the entire pool -- the opposite of the per-function policy contract that resolvers apply on the next converge tick without new pod identity. Compare metadata and scale explicitly so policy-only updates pass the staleness check.

scott-rc added 6 commits May 5, 2026 01:02