From e5bfcccdfab33d6a282ca040a431785fc24a0eb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Scott=20C=C3=B4t=C3=A9?= Date: Tue, 5 May 2026 01:02:25 -0400 Subject: [PATCH 01/17] Add per-function policy proto types and resolver layer 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. --- internal/dev/docssite/messagetable.go | 29 + internal/dev/docssite/messagetable_test.go | 8 + internal/key/keys.go | 9 + internal/key/testdata/keys.golden | 26 + internal/key/types.go | 4 + internal/skipper/function.go | 120 ++++ internal/skipper/function_keys_test.go | 40 +- internal/skipper/json_test.go | 26 + internal/skipper/policies.go | 50 ++ internal/skipper/policies_test.go | 266 ++++++++ internal/skipper/testdata/json.golden | 51 ++ internal/skipper/types.pb.go | 688 ++++++++++++++++++--- internal/skipper/types.pb.json.go | 56 ++ internal/skipper/types.proto | 26 + 14 files changed, 1323 insertions(+), 76 deletions(-) create mode 100644 internal/skipper/policies.go create mode 100644 internal/skipper/policies_test.go diff --git a/internal/dev/docssite/messagetable.go b/internal/dev/docssite/messagetable.go index 13faa2dd..494f8bfa 100644 --- a/internal/dev/docssite/messagetable.go +++ b/internal/dev/docssite/messagetable.go @@ -35,6 +35,10 @@ var errMessageDescriptionDrift = errors.New("docssite: messageTable description var messageTableRegistry = []string{ "Function", "Scale", + "HpaPolicy", + "HeartbeatPolicy", + "ProxyPolicy", + "LifecyclePolicy", "Instance", "Heartbeat", "GetInstanceRequest", @@ -58,6 +62,10 @@ var messageTableDescriptions = map[string]string{ "Function.metadata": "Opaque string passed verbatim to the assigned pod; not part of the function hash.", "Function.scale": "Per-function scaling targets (min, max, CPU, memory, in-flight requests).", "Function.oneshot": "True if each request gets a fresh pod; assigned pods are released after the request completes.", + "Function.hpa": "Per-function HPA tuning that overrides cluster defaults; unset fields fall back to the cluster flags.", + "Function.heartbeat": "Per-function heartbeat policy (idle timeout) that overrides the cluster default.", + "Function.proxy": "Per-function proxy retry policy (attempt count and backoff bounds) that overrides the cluster defaults.", + "Function.lifecycle": "Per-function pod-lifecycle policy (assignment timeout, token TTL) that overrides the cluster defaults.", "Scale.min_instances": "Minimum ready-instance floor (0 enables scale-to-zero).", "Scale.max_instances": "Hard ceiling on ready instances.", @@ -65,6 +73,19 @@ var messageTableDescriptions = map[string]string{ "Scale.target_memory_usage_mib": "Per-instance memory target in mebibytes.", "Scale.target_in_flight_requests": "Per-instance in-flight-request target.", + "HpaPolicy.tolerance": "Fractional dead-band on the scale-up/down trigger; smaller values react sooner. Zero means use the cluster default.", + "HpaPolicy.downscale_stabilization": "Minimum time a lower target must persist before the controller scales down. Zero means use the cluster default.", + "HpaPolicy.initial_readiness_delay": "Grace period after a pod becomes ready during which scale-down is suppressed. Zero means use the cluster default.", + + "HeartbeatPolicy.timeout": "Idle window after which the controller scales the function to zero. Zero means use the cluster default.", + + "ProxyPolicy.max_attempts": "Maximum number of proxy attempts per request. Zero means use the cluster default.", + "ProxyPolicy.retry_min_backoff": "Lower bound on backoff between retry attempts. Zero means use the cluster default.", + "ProxyPolicy.retry_max_backoff": "Upper bound on backoff between retry attempts. Zero means use the cluster default.", + + "LifecyclePolicy.assign_timeout": "Maximum wait for a pod to confirm assignment before it is considered stuck. Zero means use the cluster default.", + "LifecyclePolicy.token_ttl": "PASETO token lifetime issued to assigned pods. Zero means use the cluster default.", + "Instance.function": "Function this instance is assigned to.", "Instance.name": "Pod name in the cluster.", "Instance.addr": "Backend address (`host:port`) the router proxies to.", @@ -211,6 +232,14 @@ func lookupMessageDescriptor(name string) (protoreflect.MessageDescriptor, error msg = (&skipper.Function{}) case "Scale": msg = (&skipper.Scale{}) + case "HpaPolicy": + msg = (&skipper.HpaPolicy{}) + case "HeartbeatPolicy": + msg = (&skipper.HeartbeatPolicy{}) + case "ProxyPolicy": + msg = (&skipper.ProxyPolicy{}) + case "LifecyclePolicy": + msg = (&skipper.LifecyclePolicy{}) case "Instance": msg = (&skipper.Instance{}) case "Heartbeat": diff --git a/internal/dev/docssite/messagetable_test.go b/internal/dev/docssite/messagetable_test.go index bf1749bb..0ffbbd9a 100644 --- a/internal/dev/docssite/messagetable_test.go +++ b/internal/dev/docssite/messagetable_test.go @@ -187,6 +187,10 @@ func TestRenderMessageTable_ExtraDescriptionFails(t *testing.T) { "Function.metadata": "x", "Function.scale": "x", "Function.oneshot": "x", + "Function.hpa": "x", + "Function.heartbeat": "x", + "Function.proxy": "x", + "Function.lifecycle": "x", "Function.bogus": "extraneous", } _, err := renderMessageRows("Function", descriptions, messageTableRegistry) @@ -206,6 +210,10 @@ func TestRenderMessageTable_UnregisteredKeyFails(t *testing.T) { "Function.metadata": "x", "Function.scale": "x", "Function.oneshot": "x", + "Function.hpa": "x", + "Function.heartbeat": "x", + "Function.proxy": "x", + "Function.lifecycle": "x", "Event.type": "wrong message", } _, err := renderMessageRows("Function", descriptions, messageTableRegistry) diff --git a/internal/key/keys.go b/internal/key/keys.go index 5f8571a6..9f78bcf4 100644 --- a/internal/key/keys.go +++ b/internal/key/keys.go @@ -4,6 +4,7 @@ package key // Each key provides consistent naming across logs, traces, headers, and labels. var ( Addr = stringKey("address") + AssignTimeout = durationKey("assign_timeout") AssignedAt = timeKey("assigned_at") Attempt = intKey("attempt") CPUUsageMilli = uint32Key("cpu_usage_milli") @@ -11,14 +12,18 @@ var ( Count = intKey("count") Deployment = stringKey("deployment") DesiredInstances = uint32Key("desired_instances") + DownscaleStabilization = durationKey("downscale_stabilization") Duration = durationKey("duration") Error = errorKey("error") ExcludeInstanceNames = stringSliceKey("exclude_instance_names") ForwardedFor = newNames("forwarded_for") GetInstanceDurationMs = durationKey("get_instance_duration_ms") + HeartbeatTimeout = durationKey("heartbeat_timeout") InFlightRequests = uint32Key("in_flight_requests") + InitialReadinessDelay = durationKey("initial_readiness_delay") K8sReplicaSet = replicaSetKey("k8s.replicaset") Labels = mapStringStringKey("labels") + MaxAttempts = uint32Key("max_attempts") MaxInstances = uint32Key("max_instances") MemoryUsageMiB = uint32Key("memory_usage_mib") Metadata = stringKey("metadata") @@ -36,6 +41,8 @@ var ( Request = requestKey("http.request") Response = responseKey("http.response") ResponsibleIP = stringKey("responsible_ip") + RetryMaxBackoff = durationKey("retry_max_backoff") + RetryMinBackoff = durationKey("retry_min_backoff") RouterIP = stringKey("router_ip") TargetCPUUsageMilli = uint32Key("target_cpu_usage_milli") TargetInFlightRequests = uint32Key("target_in_flight_requests") @@ -43,6 +50,8 @@ var ( Tenant = stringKey("tenant") Timestamp = timeKey("timestamp") Token = newNames("token") + TokenTTL = durationKey("token_ttl") + Tolerance = float64Key("tolerance") URL = urlKey("url") UnclampedDesiredInstances = uint32Key("unclamped_desired_instances") UnreadyInstances = intKey("unready_instances") diff --git a/internal/key/testdata/keys.golden b/internal/key/testdata/keys.golden index ed866dae..f9ac469f 100644 --- a/internal/key/testdata/keys.golden +++ b/internal/key/testdata/keys.golden @@ -14,9 +14,18 @@ "b" ], "function.deployment": "test", + "function.heartbeat_policy.heartbeat_timeout_ms": 0, + "function.hpa.downscale_stabilization_ms": 0, + "function.hpa.initial_readiness_delay_ms": 0, + "function.hpa.tolerance": 0, + "function.lifecycle.assign_timeout_ms": 0, + "function.lifecycle.token_ttl_ms": 0, "function.metadata": "test", "function.namespace": "test", "function.oneshot": false, + "function.proxy.max_attempts": 0, + "function.proxy.retry_max_backoff_ms": 0, + "function.proxy.retry_min_backoff_ms": 0, "function.scale.max_instances": 42, "function.scale.min_instances": 42, "function.scale.target_cpu_usage_milli": 42, @@ -114,9 +123,26 @@ ], "function": { "deployment": "test", + "heartbeat_policy": { + "heartbeat_timeout_ms": 0 + }, + "hpa": { + "downscale_stabilization_ms": 0, + "initial_readiness_delay_ms": 0, + "tolerance": 0 + }, + "lifecycle": { + "assign_timeout_ms": 0, + "token_ttl_ms": 0 + }, "metadata": "test", "namespace": "test", "oneshot": false, + "proxy": { + "max_attempts": 0, + "retry_max_backoff_ms": 0, + "retry_min_backoff_ms": 0 + }, "scale": { "max_instances": 42, "min_instances": 42, diff --git a/internal/key/types.go b/internal/key/types.go index de046816..3192d04c 100644 --- a/internal/key/types.go +++ b/internal/key/types.go @@ -77,6 +77,10 @@ func uint32Key(name string) *Key[uint32] { return newKey(name, func(n Names, v uint32) slog.Attr { return slog.Int(n.Name, int(v)) }) } +func float64Key(name string) *Key[float64] { + return newKey(name, func(n Names, v float64) slog.Attr { return slog.Float64(n.Name, v) }) +} + func stringSliceKey(name string) *Key[[]string] { return newKey(name, func(n Names, v []string) slog.Attr { return slog.Any(n.Name, v) }) } diff --git a/internal/skipper/function.go b/internal/skipper/function.go index 248c5cdd..d0865d2f 100644 --- a/internal/skipper/function.go +++ b/internal/skipper/function.go @@ -5,6 +5,7 @@ import ( "fmt" "log/slog" "net/http" + "time" "github.com/cespare/xxhash/v2" "github.com/gadget-inc/skipper/internal/key" @@ -65,6 +66,10 @@ func (f *Function) LogValue() slog.Value { key.Metadata.Slog(f.GetMetadata()), key.Oneshot.Slog(f.GetOneshot()), ScaleKey.Slog(f.GetScale()), + HpaPolicyKey.Slog(f.GetHpa()), + HeartbeatPolicyKey.Slog(f.GetHeartbeat()), + ProxyPolicyKey.Slog(f.GetProxy()), + LifecyclePolicyKey.Slog(f.GetLifecycle()), ) } @@ -88,9 +93,124 @@ func (f *Function) Validate() error { if scale.GetMinInstances() > scale.GetMaxInstances() { return fmt.Errorf("scale.min_instances (%d) must be <= scale.max_instances (%d)", scale.GetMinInstances(), scale.GetMaxInstances()) } + if hpa := f.GetHpa(); hpa != nil { + if d := hpa.GetDownscaleStabilization().AsDuration(); d < 0 { + return fmt.Errorf("hpa.downscale_stabilization (%s) must be >= 0", d) + } + if d := hpa.GetInitialReadinessDelay().AsDuration(); d < 0 { + return fmt.Errorf("hpa.initial_readiness_delay (%s) must be >= 0", d) + } + } + if hb := f.GetHeartbeat(); hb != nil { + if d := hb.GetTimeout().AsDuration(); d < 0 { + return fmt.Errorf("heartbeat.timeout (%s) must be >= 0", d) + } + } + if proxy := f.GetProxy(); proxy != nil { + minBackoff := proxy.GetRetryMinBackoff().AsDuration() + maxBackoff := proxy.GetRetryMaxBackoff().AsDuration() + if minBackoff < 0 { + return fmt.Errorf("proxy.retry_min_backoff (%s) must be >= 0", minBackoff) + } + if maxBackoff < 0 { + return fmt.Errorf("proxy.retry_max_backoff (%s) must be >= 0", maxBackoff) + } + if minBackoff > 0 && maxBackoff > 0 && minBackoff > maxBackoff { + return fmt.Errorf("proxy.retry_min_backoff (%s) must be <= proxy.retry_max_backoff (%s)", minBackoff, maxBackoff) + } + } + if lc := f.GetLifecycle(); lc != nil { + if d := lc.GetAssignTimeout().AsDuration(); d < 0 { + return fmt.Errorf("lifecycle.assign_timeout (%s) must be >= 0", d) + } + if d := lc.GetTokenTtl().AsDuration(); d < 0 { + return fmt.Errorf("lifecycle.token_ttl (%s) must be >= 0", d) + } + } return nil } +// HPATolerance returns the per-function HPA tolerance, falling back to +// clusterDefault when the function does not set one. +func (f *Function) HPATolerance(clusterDefault float64) float64 { + if v := f.GetHpa().GetTolerance(); v != 0 { + return v + } + return clusterDefault +} + +// HPADownscaleStabilization returns the per-function downscale-stabilization +// window, falling back to clusterDefault when the function does not set one. +func (f *Function) HPADownscaleStabilization(clusterDefault time.Duration) time.Duration { + if v := f.GetHpa().GetDownscaleStabilization().AsDuration(); v != 0 { + return v + } + return clusterDefault +} + +// HPAInitialReadinessDelay returns the per-function initial-readiness delay, +// falling back to clusterDefault when the function does not set one. +func (f *Function) HPAInitialReadinessDelay(clusterDefault time.Duration) time.Duration { + if v := f.GetHpa().GetInitialReadinessDelay().AsDuration(); v != 0 { + return v + } + return clusterDefault +} + +// HeartbeatTimeout returns the per-function heartbeat timeout, falling back to +// clusterDefault when the function does not set one. +func (f *Function) HeartbeatTimeout(clusterDefault time.Duration) time.Duration { + if v := f.GetHeartbeat().GetTimeout().AsDuration(); v != 0 { + return v + } + return clusterDefault +} + +// MaxRoundTripAttempts returns the per-function maximum number of retry +// attempts, falling back to clusterDefault when the function does not set one. +func (f *Function) MaxRoundTripAttempts(clusterDefault uint32) uint32 { + if v := f.GetProxy().GetMaxAttempts(); v != 0 { + return v + } + return clusterDefault +} + +// RetryMinBackoff returns the per-function minimum retry backoff, falling +// back to clusterDefault when the function does not set one. +func (f *Function) RetryMinBackoff(clusterDefault time.Duration) time.Duration { + if v := f.GetProxy().GetRetryMinBackoff().AsDuration(); v != 0 { + return v + } + return clusterDefault +} + +// RetryMaxBackoff returns the per-function maximum retry backoff, falling +// back to clusterDefault when the function does not set one. +func (f *Function) RetryMaxBackoff(clusterDefault time.Duration) time.Duration { + if v := f.GetProxy().GetRetryMaxBackoff().AsDuration(); v != 0 { + return v + } + return clusterDefault +} + +// AssignTimeout returns the per-function instance-assignment timeout, falling +// back to clusterDefault when the function does not set one. +func (f *Function) AssignTimeout(clusterDefault time.Duration) time.Duration { + if v := f.GetLifecycle().GetAssignTimeout().AsDuration(); v != 0 { + return v + } + return clusterDefault +} + +// TokenTTL returns the per-function PASETO token lifetime, falling back to +// clusterDefault when the function does not set one. +func (f *Function) TokenTTL(clusterDefault time.Duration) time.Duration { + if v := f.GetLifecycle().GetTokenTtl().AsDuration(); v != 0 { + return v + } + return clusterDefault +} + func (f *Function) SetHeader(r *http.Request) { fnJSON, err := json.Marshal(f) if err != nil { diff --git a/internal/skipper/function_keys_test.go b/internal/skipper/function_keys_test.go index b825de55..a1ebde99 100644 --- a/internal/skipper/function_keys_test.go +++ b/internal/skipper/function_keys_test.go @@ -3,21 +3,27 @@ package skipper import ( "sync" "testing" + "time" "github.com/gadget-inc/skipper/internal/key" "github.com/google/go-cmp/cmp/cmpopts" "go.opentelemetry.io/otel/attribute" "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/types/known/durationpb" "gotest.tools/v3/assert" ) // Compile-time gate: each domain key binds to its concrete *T. var ( - _ *key.Key[*Function] = FunctionKey - _ *key.Key[*Heartbeat] = HeartbeatKey - _ *key.Key[*Instance] = InstanceKey - _ *key.Key[*Scale] = ScaleKey - _ *key.Key[*ScaleDecision] = ScaleDecisionKey + _ *key.Key[*Function] = FunctionKey + _ *key.Key[*Heartbeat] = HeartbeatKey + _ *key.Key[*HeartbeatPolicy] = HeartbeatPolicyKey + _ *key.Key[*HpaPolicy] = HpaPolicyKey + _ *key.Key[*Instance] = InstanceKey + _ *key.Key[*LifecyclePolicy] = LifecyclePolicyKey + _ *key.Key[*ProxyPolicy] = ProxyPolicyKey + _ *key.Key[*Scale] = ScaleKey + _ *key.Key[*ScaleDecision] = ScaleDecisionKey ) // TestFunctionKeyEquivalence pins the cached path's output to the uncached @@ -61,6 +67,30 @@ func TestFunctionKeyEquivalence(t *testing.T) { Scale: Scale_builder{MaxInstances: proto.Uint32(5)}.Build(), }.Build(), }, + { + name: "with all policies", + fn: Function_builder{ + Namespace: new("ns"), + Deployment: new("deploy"), + Tenant: new("tenant"), + Scale: Scale_builder{MaxInstances: proto.Uint32(5)}.Build(), + Hpa: HpaPolicy_builder{ + Tolerance: new(0.05), + DownscaleStabilization: durationpb.New(time.Minute), + InitialReadinessDelay: durationpb.New(time.Minute), + }.Build(), + Heartbeat: HeartbeatPolicy_builder{Timeout: durationpb.New(time.Minute)}.Build(), + Proxy: ProxyPolicy_builder{ + MaxAttempts: new(uint32(5)), + RetryMinBackoff: durationpb.New(10 * time.Millisecond), + RetryMaxBackoff: durationpb.New(time.Second), + }.Build(), + Lifecycle: LifecyclePolicy_builder{ + AssignTimeout: durationpb.New(time.Minute), + TokenTtl: durationpb.New(time.Hour), + }.Build(), + }.Build(), + }, } for _, tc := range testCases { diff --git a/internal/skipper/json_test.go b/internal/skipper/json_test.go index 82b2b997..09c9429d 100644 --- a/internal/skipper/json_test.go +++ b/internal/skipper/json_test.go @@ -7,6 +7,7 @@ import ( "github.com/go-json-experiment/json" "github.com/go-json-experiment/json/jsontext" "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/types/known/durationpb" "google.golang.org/protobuf/types/known/timestamppb" "gotest.tools/v3/assert" "gotest.tools/v3/golden" @@ -26,12 +27,37 @@ var ( TargetInFlightRequests: proto.Uint32(100), }.Build() + goldenHpaPolicy = HpaPolicy_builder{ + Tolerance: new(0.05), + DownscaleStabilization: durationpb.New(60 * time.Second), + InitialReadinessDelay: durationpb.New(30 * time.Second), + }.Build() + + goldenHeartbeatPolicy = HeartbeatPolicy_builder{ + Timeout: durationpb.New(30 * time.Minute), + }.Build() + + goldenProxyPolicy = ProxyPolicy_builder{ + MaxAttempts: new(uint32(3)), + RetryMinBackoff: durationpb.New(10 * time.Millisecond), + RetryMaxBackoff: durationpb.New(time.Second), + }.Build() + + goldenLifecyclePolicy = LifecyclePolicy_builder{ + AssignTimeout: durationpb.New(45 * time.Second), + TokenTtl: durationpb.New(time.Hour), + }.Build() + goldenFunction = Function_builder{ Namespace: new("skipper-production"), Deployment: new("my-app"), Tenant: new("tenant-123"), Metadata: new("metadata-value"), Scale: goldenScale, + Hpa: goldenHpaPolicy, + Heartbeat: goldenHeartbeatPolicy, + Proxy: goldenProxyPolicy, + Lifecycle: goldenLifecyclePolicy, }.Build() goldenHeartbeat = Heartbeat_builder{ diff --git a/internal/skipper/policies.go b/internal/skipper/policies.go new file mode 100644 index 00000000..2d82e76d --- /dev/null +++ b/internal/skipper/policies.go @@ -0,0 +1,50 @@ +package skipper + +import ( + "log/slog" + + "github.com/gadget-inc/skipper/internal/key" +) + +var ( + _ slog.LogValuer = (*HpaPolicy)(nil) + _ slog.LogValuer = (*HeartbeatPolicy)(nil) + _ slog.LogValuer = (*ProxyPolicy)(nil) + _ slog.LogValuer = (*LifecyclePolicy)(nil) +) + +var ( + HpaPolicyKey = key.New("hpa", (*HpaPolicy).LogValue) + HeartbeatPolicyKey = key.New("heartbeat_policy", (*HeartbeatPolicy).LogValue) + ProxyPolicyKey = key.New("proxy", (*ProxyPolicy).LogValue) + LifecyclePolicyKey = key.New("lifecycle", (*LifecyclePolicy).LogValue) +) + +func (p *HpaPolicy) LogValue() slog.Value { + return slog.GroupValue( + key.Tolerance.Slog(p.GetTolerance()), + key.DownscaleStabilization.Slog(p.GetDownscaleStabilization().AsDuration()), + key.InitialReadinessDelay.Slog(p.GetInitialReadinessDelay().AsDuration()), + ) +} + +func (p *HeartbeatPolicy) LogValue() slog.Value { + return slog.GroupValue( + key.HeartbeatTimeout.Slog(p.GetTimeout().AsDuration()), + ) +} + +func (p *ProxyPolicy) LogValue() slog.Value { + return slog.GroupValue( + key.MaxAttempts.Slog(p.GetMaxAttempts()), + key.RetryMinBackoff.Slog(p.GetRetryMinBackoff().AsDuration()), + key.RetryMaxBackoff.Slog(p.GetRetryMaxBackoff().AsDuration()), + ) +} + +func (p *LifecyclePolicy) LogValue() slog.Value { + return slog.GroupValue( + key.AssignTimeout.Slog(p.GetAssignTimeout().AsDuration()), + key.TokenTTL.Slog(p.GetTokenTtl().AsDuration()), + ) +} diff --git a/internal/skipper/policies_test.go b/internal/skipper/policies_test.go new file mode 100644 index 00000000..ca1ce308 --- /dev/null +++ b/internal/skipper/policies_test.go @@ -0,0 +1,266 @@ +package skipper + +import ( + "net/http/httptest" + "testing" + "time" + + "google.golang.org/protobuf/types/known/durationpb" + "gotest.tools/v3/assert" +) + +func TestResolverFallsBackToClusterDefault(t *testing.T) { + t.Parallel() + + clusterFloat := 0.10 + clusterDuration := 30 * time.Second + clusterUint32 := uint32(6) + + // All resolvers must return clusterDefault when the function-side value + // is zero or the policy sub-message is unset. + cases := []struct { + name string + fn *Function + }{ + {name: "nil sub-messages", fn: Function_builder{}.Build()}, + { + name: "zero-valued sub-messages", + fn: Function_builder{ + Hpa: HpaPolicy_builder{}.Build(), + Heartbeat: HeartbeatPolicy_builder{}.Build(), + Proxy: ProxyPolicy_builder{}.Build(), + Lifecycle: LifecyclePolicy_builder{}.Build(), + }.Build(), + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tc.fn.HPATolerance(clusterFloat), clusterFloat) + assert.Equal(t, tc.fn.HPADownscaleStabilization(clusterDuration), clusterDuration) + assert.Equal(t, tc.fn.HPAInitialReadinessDelay(clusterDuration), clusterDuration) + assert.Equal(t, tc.fn.HeartbeatTimeout(clusterDuration), clusterDuration) + assert.Equal(t, tc.fn.MaxRoundTripAttempts(clusterUint32), clusterUint32) + assert.Equal(t, tc.fn.RetryMinBackoff(clusterDuration), clusterDuration) + assert.Equal(t, tc.fn.RetryMaxBackoff(clusterDuration), clusterDuration) + assert.Equal(t, tc.fn.AssignTimeout(clusterDuration), clusterDuration) + assert.Equal(t, tc.fn.TokenTTL(clusterDuration), clusterDuration) + }) + } +} + +func TestResolverReturnsFunctionValue(t *testing.T) { + t.Parallel() + + fnTolerance := 0.05 + fnDuration := 5 * time.Minute + fnUint32 := uint32(10) + + clusterFloat := 0.20 + clusterDuration := 30 * time.Second + clusterUint32 := uint32(6) + + fn := Function_builder{ + Hpa: HpaPolicy_builder{ + Tolerance: new(fnTolerance), + DownscaleStabilization: durationpb.New(fnDuration), + InitialReadinessDelay: durationpb.New(fnDuration), + }.Build(), + Heartbeat: HeartbeatPolicy_builder{ + Timeout: durationpb.New(fnDuration), + }.Build(), + Proxy: ProxyPolicy_builder{ + MaxAttempts: new(fnUint32), + RetryMinBackoff: durationpb.New(fnDuration), + RetryMaxBackoff: durationpb.New(fnDuration * 2), + }.Build(), + Lifecycle: LifecyclePolicy_builder{ + AssignTimeout: durationpb.New(fnDuration), + TokenTtl: durationpb.New(fnDuration), + }.Build(), + }.Build() + + assert.Equal(t, fn.HPATolerance(clusterFloat), fnTolerance) + assert.Equal(t, fn.HPADownscaleStabilization(clusterDuration), fnDuration) + assert.Equal(t, fn.HPAInitialReadinessDelay(clusterDuration), fnDuration) + assert.Equal(t, fn.HeartbeatTimeout(clusterDuration), fnDuration) + assert.Equal(t, fn.MaxRoundTripAttempts(clusterUint32), fnUint32) + assert.Equal(t, fn.RetryMinBackoff(clusterDuration), fnDuration) + assert.Equal(t, fn.RetryMaxBackoff(clusterDuration), fnDuration*2) + assert.Equal(t, fn.AssignTimeout(clusterDuration), fnDuration) + assert.Equal(t, fn.TokenTTL(clusterDuration), fnDuration) +} + +func TestValidatePolicies(t *testing.T) { + t.Parallel() + + baseScale := Scale_builder{MinInstances: new(uint32(1)), MaxInstances: new(uint32(10))}.Build() + build := func(mut func(b *Function_builder)) *Function { + b := Function_builder{ + Namespace: new("ns"), + Deployment: new("d"), + Tenant: new("t"), + Scale: baseScale, + } + mut(&b) + return b.Build() + } + + cases := []struct { + name string + fn *Function + wantErr string + }{ + { + name: "all policies omitted", + fn: build(func(*Function_builder) {}), + }, + { + name: "all policies zero-valued", + fn: build(func(b *Function_builder) { + b.Hpa = HpaPolicy_builder{}.Build() + b.Heartbeat = HeartbeatPolicy_builder{}.Build() + b.Proxy = ProxyPolicy_builder{}.Build() + b.Lifecycle = LifecyclePolicy_builder{}.Build() + }), + }, + { + name: "valid populated policies", + fn: build(func(b *Function_builder) { + b.Hpa = HpaPolicy_builder{ + Tolerance: new(0.05), + DownscaleStabilization: durationpb.New(time.Minute), + InitialReadinessDelay: durationpb.New(time.Minute), + }.Build() + b.Heartbeat = HeartbeatPolicy_builder{Timeout: durationpb.New(time.Minute)}.Build() + b.Proxy = ProxyPolicy_builder{ + MaxAttempts: new(uint32(5)), + RetryMinBackoff: durationpb.New(10 * time.Millisecond), + RetryMaxBackoff: durationpb.New(time.Second), + }.Build() + b.Lifecycle = LifecyclePolicy_builder{ + AssignTimeout: durationpb.New(time.Minute), + TokenTtl: durationpb.New(time.Hour), + }.Build() + }), + }, + { + name: "negative hpa.downscale_stabilization", + fn: build(func(b *Function_builder) { + b.Hpa = HpaPolicy_builder{DownscaleStabilization: durationpb.New(-time.Second)}.Build() + }), + wantErr: "hpa.downscale_stabilization", + }, + { + name: "negative hpa.initial_readiness_delay", + fn: build(func(b *Function_builder) { + b.Hpa = HpaPolicy_builder{InitialReadinessDelay: durationpb.New(-time.Second)}.Build() + }), + wantErr: "hpa.initial_readiness_delay", + }, + { + name: "negative heartbeat.timeout", + fn: build(func(b *Function_builder) { + b.Heartbeat = HeartbeatPolicy_builder{Timeout: durationpb.New(-time.Second)}.Build() + }), + wantErr: "heartbeat.timeout", + }, + { + name: "negative proxy.retry_min_backoff", + fn: build(func(b *Function_builder) { + b.Proxy = ProxyPolicy_builder{RetryMinBackoff: durationpb.New(-time.Second)}.Build() + }), + wantErr: "proxy.retry_min_backoff", + }, + { + name: "negative proxy.retry_max_backoff", + fn: build(func(b *Function_builder) { + b.Proxy = ProxyPolicy_builder{RetryMaxBackoff: durationpb.New(-time.Second)}.Build() + }), + wantErr: "proxy.retry_max_backoff", + }, + { + name: "inverted backoff bounds", + fn: build(func(b *Function_builder) { + b.Proxy = ProxyPolicy_builder{ + RetryMinBackoff: durationpb.New(time.Second), + RetryMaxBackoff: durationpb.New(100 * time.Millisecond), + }.Build() + }), + wantErr: "must be <= proxy.retry_max_backoff", + }, + { + name: "negative lifecycle.assign_timeout", + fn: build(func(b *Function_builder) { + b.Lifecycle = LifecyclePolicy_builder{AssignTimeout: durationpb.New(-time.Second)}.Build() + }), + wantErr: "lifecycle.assign_timeout", + }, + { + name: "negative lifecycle.token_ttl", + fn: build(func(b *Function_builder) { + b.Lifecycle = LifecyclePolicy_builder{TokenTtl: durationpb.New(-time.Second)}.Build() + }), + wantErr: "lifecycle.token_ttl", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + err := tc.fn.Validate() + if tc.wantErr != "" { + assert.ErrorContains(t, err, tc.wantErr) + } else { + assert.NilError(t, err) + } + }) + } +} + +func TestFunctionFromHeaderPreservesPolicies(t *testing.T) { + t.Parallel() + + header := `{ + "namespace":"ns","deployment":"d","tenant":"t", + "scale":{"min_instances":1,"max_instances":10}, + "hpa":{"tolerance":0.05,"downscale_stabilization":"60s","initial_readiness_delay":"30s"}, + "heartbeat":{"timeout":"1800s"}, + "proxy":{"max_attempts":3,"retry_min_backoff":"0.010s","retry_max_backoff":"1s"}, + "lifecycle":{"assign_timeout":"45s","token_ttl":"3600s"} + }` + + req := httptest.NewRequest("GET", "/", nil) + req.Header.Set(FunctionKey.Header, header) + + fn, err := FunctionFromHeader(req) + assert.NilError(t, err) + + assert.Equal(t, fn.HPATolerance(0), 0.05) + assert.Equal(t, fn.HPADownscaleStabilization(0), 60*time.Second) + assert.Equal(t, fn.HPAInitialReadinessDelay(0), 30*time.Second) + assert.Equal(t, fn.HeartbeatTimeout(0), 30*time.Minute) + assert.Equal(t, fn.MaxRoundTripAttempts(0), uint32(3)) + assert.Equal(t, fn.RetryMinBackoff(0), 10*time.Millisecond) + assert.Equal(t, fn.RetryMaxBackoff(0), time.Second) + assert.Equal(t, fn.AssignTimeout(0), 45*time.Second) + assert.Equal(t, fn.TokenTTL(0), time.Hour) +} + +func TestFunctionFromHeaderOmittedPoliciesAreZero(t *testing.T) { + t.Parallel() + + header := `{"namespace":"ns","deployment":"d","tenant":"t","scale":{"min_instances":1,"max_instances":10}}` + req := httptest.NewRequest("GET", "/", nil) + req.Header.Set(FunctionKey.Header, header) + + fn, err := FunctionFromHeader(req) + assert.NilError(t, err) + + // Each resolver returns the cluster default when the policy is unset. + assert.Equal(t, fn.HPATolerance(0.10), 0.10) + assert.Equal(t, fn.HeartbeatTimeout(90*time.Second), 90*time.Second) + assert.Equal(t, fn.MaxRoundTripAttempts(6), uint32(6)) + assert.Equal(t, fn.TokenTTL(7*24*time.Hour), 7*24*time.Hour) +} diff --git a/internal/skipper/testdata/json.golden b/internal/skipper/testdata/json.golden index f02d2b6c..faab5441 100644 --- a/internal/skipper/testdata/json.golden +++ b/internal/skipper/testdata/json.golden @@ -10,6 +10,23 @@ "target_cpu_usage_milli": 500, "target_memory_usage_mib": 256, "target_in_flight_requests": 100 + }, + "hpa": { + "tolerance": 0.05, + "downscale_stabilization": "60s", + "initial_readiness_delay": "30s" + }, + "heartbeat": { + "timeout": "1800s" + }, + "proxy": { + "max_attempts": 3, + "retry_min_backoff": "0.010s", + "retry_max_backoff": "1s" + }, + "lifecycle": { + "assign_timeout": "45s", + "token_ttl": "3600s" } }, "Heartbeat": { @@ -24,6 +41,23 @@ "target_cpu_usage_milli": 500, "target_memory_usage_mib": 256, "target_in_flight_requests": 100 + }, + "hpa": { + "tolerance": 0.05, + "downscale_stabilization": "60s", + "initial_readiness_delay": "30s" + }, + "heartbeat": { + "timeout": "1800s" + }, + "proxy": { + "max_attempts": 3, + "retry_min_backoff": "0.010s", + "retry_max_backoff": "1s" + }, + "lifecycle": { + "assign_timeout": "45s", + "token_ttl": "3600s" } }, "timestamp": "2024-01-15T10:30:00Z", @@ -41,6 +75,23 @@ "target_cpu_usage_milli": 500, "target_memory_usage_mib": 256, "target_in_flight_requests": 100 + }, + "hpa": { + "tolerance": 0.05, + "downscale_stabilization": "60s", + "initial_readiness_delay": "30s" + }, + "heartbeat": { + "timeout": "1800s" + }, + "proxy": { + "max_attempts": 3, + "retry_min_backoff": "0.010s", + "retry_max_backoff": "1s" + }, + "lifecycle": { + "assign_timeout": "45s", + "token_ttl": "3600s" } }, "name": "my-app-abc123", diff --git a/internal/skipper/types.pb.go b/internal/skipper/types.pb.go index b33d7f30..1b809e6c 100644 --- a/internal/skipper/types.pb.go +++ b/internal/skipper/types.pb.go @@ -10,6 +10,7 @@ import ( protoreflect "google.golang.org/protobuf/reflect/protoreflect" protoimpl "google.golang.org/protobuf/runtime/protoimpl" _ "google.golang.org/protobuf/types/gofeaturespb" + durationpb "google.golang.org/protobuf/types/known/durationpb" timestamppb "google.golang.org/protobuf/types/known/timestamppb" reflect "reflect" unsafe "unsafe" @@ -373,6 +374,417 @@ func (b0 Scale_builder) Build() *Scale { return m0 } +type HpaPolicy struct { + state protoimpl.MessageState `protogen:"opaque.v1"` + xxx_hidden_Tolerance float64 `protobuf:"fixed64,1,opt,name=tolerance"` + xxx_hidden_DownscaleStabilization *durationpb.Duration `protobuf:"bytes,2,opt,name=downscale_stabilization,json=downscaleStabilization"` + xxx_hidden_InitialReadinessDelay *durationpb.Duration `protobuf:"bytes,3,opt,name=initial_readiness_delay,json=initialReadinessDelay"` + XXX_raceDetectHookData protoimpl.RaceDetectHookData + XXX_presence [1]uint32 + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache +} + +func (x *HpaPolicy) Reset() { + *x = HpaPolicy{} + mi := &file_types_proto_msgTypes[1] + ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) + ms.StoreMessageInfo(mi) +} + +func (x *HpaPolicy) String() string { + return protoimpl.X.MessageStringOf(x) +} + +func (*HpaPolicy) ProtoMessage() {} + +func (x *HpaPolicy) ProtoReflect() protoreflect.Message { + mi := &file_types_proto_msgTypes[1] + if x != nil { + ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) + if ms.LoadMessageInfo() == nil { + ms.StoreMessageInfo(mi) + } + return ms + } + return mi.MessageOf(x) +} + +func (x *HpaPolicy) GetTolerance() float64 { + if x != nil { + return x.xxx_hidden_Tolerance + } + return 0 +} + +func (x *HpaPolicy) GetDownscaleStabilization() *durationpb.Duration { + if x != nil { + return x.xxx_hidden_DownscaleStabilization + } + return nil +} + +func (x *HpaPolicy) GetInitialReadinessDelay() *durationpb.Duration { + if x != nil { + return x.xxx_hidden_InitialReadinessDelay + } + return nil +} + +func (x *HpaPolicy) SetTolerance(v float64) { + x.xxx_hidden_Tolerance = v + protoimpl.X.SetPresent(&(x.XXX_presence[0]), 0, 3) +} + +func (x *HpaPolicy) SetDownscaleStabilization(v *durationpb.Duration) { + x.xxx_hidden_DownscaleStabilization = v +} + +func (x *HpaPolicy) SetInitialReadinessDelay(v *durationpb.Duration) { + x.xxx_hidden_InitialReadinessDelay = v +} + +func (x *HpaPolicy) HasTolerance() bool { + if x == nil { + return false + } + return protoimpl.X.Present(&(x.XXX_presence[0]), 0) +} + +func (x *HpaPolicy) HasDownscaleStabilization() bool { + if x == nil { + return false + } + return x.xxx_hidden_DownscaleStabilization != nil +} + +func (x *HpaPolicy) HasInitialReadinessDelay() bool { + if x == nil { + return false + } + return x.xxx_hidden_InitialReadinessDelay != nil +} + +func (x *HpaPolicy) ClearTolerance() { + protoimpl.X.ClearPresent(&(x.XXX_presence[0]), 0) + x.xxx_hidden_Tolerance = 0 +} + +func (x *HpaPolicy) ClearDownscaleStabilization() { + x.xxx_hidden_DownscaleStabilization = nil +} + +func (x *HpaPolicy) ClearInitialReadinessDelay() { + x.xxx_hidden_InitialReadinessDelay = nil +} + +type HpaPolicy_builder struct { + _ [0]func() // Prevents comparability and use of unkeyed literals for the builder. + + Tolerance *float64 + DownscaleStabilization *durationpb.Duration + InitialReadinessDelay *durationpb.Duration +} + +func (b0 HpaPolicy_builder) Build() *HpaPolicy { + m0 := &HpaPolicy{} + b, x := &b0, m0 + _, _ = b, x + if b.Tolerance != nil { + protoimpl.X.SetPresentNonAtomic(&(x.XXX_presence[0]), 0, 3) + x.xxx_hidden_Tolerance = *b.Tolerance + } + x.xxx_hidden_DownscaleStabilization = b.DownscaleStabilization + x.xxx_hidden_InitialReadinessDelay = b.InitialReadinessDelay + return m0 +} + +type HeartbeatPolicy struct { + state protoimpl.MessageState `protogen:"opaque.v1"` + xxx_hidden_Timeout *durationpb.Duration `protobuf:"bytes,1,opt,name=timeout"` + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache +} + +func (x *HeartbeatPolicy) Reset() { + *x = HeartbeatPolicy{} + mi := &file_types_proto_msgTypes[2] + ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) + ms.StoreMessageInfo(mi) +} + +func (x *HeartbeatPolicy) String() string { + return protoimpl.X.MessageStringOf(x) +} + +func (*HeartbeatPolicy) ProtoMessage() {} + +func (x *HeartbeatPolicy) ProtoReflect() protoreflect.Message { + mi := &file_types_proto_msgTypes[2] + if x != nil { + ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) + if ms.LoadMessageInfo() == nil { + ms.StoreMessageInfo(mi) + } + return ms + } + return mi.MessageOf(x) +} + +func (x *HeartbeatPolicy) GetTimeout() *durationpb.Duration { + if x != nil { + return x.xxx_hidden_Timeout + } + return nil +} + +func (x *HeartbeatPolicy) SetTimeout(v *durationpb.Duration) { + x.xxx_hidden_Timeout = v +} + +func (x *HeartbeatPolicy) HasTimeout() bool { + if x == nil { + return false + } + return x.xxx_hidden_Timeout != nil +} + +func (x *HeartbeatPolicy) ClearTimeout() { + x.xxx_hidden_Timeout = nil +} + +type HeartbeatPolicy_builder struct { + _ [0]func() // Prevents comparability and use of unkeyed literals for the builder. + + Timeout *durationpb.Duration +} + +func (b0 HeartbeatPolicy_builder) Build() *HeartbeatPolicy { + m0 := &HeartbeatPolicy{} + b, x := &b0, m0 + _, _ = b, x + x.xxx_hidden_Timeout = b.Timeout + return m0 +} + +type ProxyPolicy struct { + state protoimpl.MessageState `protogen:"opaque.v1"` + xxx_hidden_MaxAttempts uint32 `protobuf:"varint,1,opt,name=max_attempts,json=maxAttempts"` + xxx_hidden_RetryMinBackoff *durationpb.Duration `protobuf:"bytes,2,opt,name=retry_min_backoff,json=retryMinBackoff"` + xxx_hidden_RetryMaxBackoff *durationpb.Duration `protobuf:"bytes,3,opt,name=retry_max_backoff,json=retryMaxBackoff"` + XXX_raceDetectHookData protoimpl.RaceDetectHookData + XXX_presence [1]uint32 + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache +} + +func (x *ProxyPolicy) Reset() { + *x = ProxyPolicy{} + mi := &file_types_proto_msgTypes[3] + ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) + ms.StoreMessageInfo(mi) +} + +func (x *ProxyPolicy) String() string { + return protoimpl.X.MessageStringOf(x) +} + +func (*ProxyPolicy) ProtoMessage() {} + +func (x *ProxyPolicy) ProtoReflect() protoreflect.Message { + mi := &file_types_proto_msgTypes[3] + if x != nil { + ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) + if ms.LoadMessageInfo() == nil { + ms.StoreMessageInfo(mi) + } + return ms + } + return mi.MessageOf(x) +} + +func (x *ProxyPolicy) GetMaxAttempts() uint32 { + if x != nil { + return x.xxx_hidden_MaxAttempts + } + return 0 +} + +func (x *ProxyPolicy) GetRetryMinBackoff() *durationpb.Duration { + if x != nil { + return x.xxx_hidden_RetryMinBackoff + } + return nil +} + +func (x *ProxyPolicy) GetRetryMaxBackoff() *durationpb.Duration { + if x != nil { + return x.xxx_hidden_RetryMaxBackoff + } + return nil +} + +func (x *ProxyPolicy) SetMaxAttempts(v uint32) { + x.xxx_hidden_MaxAttempts = v + protoimpl.X.SetPresent(&(x.XXX_presence[0]), 0, 3) +} + +func (x *ProxyPolicy) SetRetryMinBackoff(v *durationpb.Duration) { + x.xxx_hidden_RetryMinBackoff = v +} + +func (x *ProxyPolicy) SetRetryMaxBackoff(v *durationpb.Duration) { + x.xxx_hidden_RetryMaxBackoff = v +} + +func (x *ProxyPolicy) HasMaxAttempts() bool { + if x == nil { + return false + } + return protoimpl.X.Present(&(x.XXX_presence[0]), 0) +} + +func (x *ProxyPolicy) HasRetryMinBackoff() bool { + if x == nil { + return false + } + return x.xxx_hidden_RetryMinBackoff != nil +} + +func (x *ProxyPolicy) HasRetryMaxBackoff() bool { + if x == nil { + return false + } + return x.xxx_hidden_RetryMaxBackoff != nil +} + +func (x *ProxyPolicy) ClearMaxAttempts() { + protoimpl.X.ClearPresent(&(x.XXX_presence[0]), 0) + x.xxx_hidden_MaxAttempts = 0 +} + +func (x *ProxyPolicy) ClearRetryMinBackoff() { + x.xxx_hidden_RetryMinBackoff = nil +} + +func (x *ProxyPolicy) ClearRetryMaxBackoff() { + x.xxx_hidden_RetryMaxBackoff = nil +} + +type ProxyPolicy_builder struct { + _ [0]func() // Prevents comparability and use of unkeyed literals for the builder. + + MaxAttempts *uint32 + RetryMinBackoff *durationpb.Duration + RetryMaxBackoff *durationpb.Duration +} + +func (b0 ProxyPolicy_builder) Build() *ProxyPolicy { + m0 := &ProxyPolicy{} + b, x := &b0, m0 + _, _ = b, x + if b.MaxAttempts != nil { + protoimpl.X.SetPresentNonAtomic(&(x.XXX_presence[0]), 0, 3) + x.xxx_hidden_MaxAttempts = *b.MaxAttempts + } + x.xxx_hidden_RetryMinBackoff = b.RetryMinBackoff + x.xxx_hidden_RetryMaxBackoff = b.RetryMaxBackoff + return m0 +} + +type LifecyclePolicy struct { + state protoimpl.MessageState `protogen:"opaque.v1"` + xxx_hidden_AssignTimeout *durationpb.Duration `protobuf:"bytes,1,opt,name=assign_timeout,json=assignTimeout"` + xxx_hidden_TokenTtl *durationpb.Duration `protobuf:"bytes,2,opt,name=token_ttl,json=tokenTtl"` + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache +} + +func (x *LifecyclePolicy) Reset() { + *x = LifecyclePolicy{} + mi := &file_types_proto_msgTypes[4] + ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) + ms.StoreMessageInfo(mi) +} + +func (x *LifecyclePolicy) String() string { + return protoimpl.X.MessageStringOf(x) +} + +func (*LifecyclePolicy) ProtoMessage() {} + +func (x *LifecyclePolicy) ProtoReflect() protoreflect.Message { + mi := &file_types_proto_msgTypes[4] + if x != nil { + ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) + if ms.LoadMessageInfo() == nil { + ms.StoreMessageInfo(mi) + } + return ms + } + return mi.MessageOf(x) +} + +func (x *LifecyclePolicy) GetAssignTimeout() *durationpb.Duration { + if x != nil { + return x.xxx_hidden_AssignTimeout + } + return nil +} + +func (x *LifecyclePolicy) GetTokenTtl() *durationpb.Duration { + if x != nil { + return x.xxx_hidden_TokenTtl + } + return nil +} + +func (x *LifecyclePolicy) SetAssignTimeout(v *durationpb.Duration) { + x.xxx_hidden_AssignTimeout = v +} + +func (x *LifecyclePolicy) SetTokenTtl(v *durationpb.Duration) { + x.xxx_hidden_TokenTtl = v +} + +func (x *LifecyclePolicy) HasAssignTimeout() bool { + if x == nil { + return false + } + return x.xxx_hidden_AssignTimeout != nil +} + +func (x *LifecyclePolicy) HasTokenTtl() bool { + if x == nil { + return false + } + return x.xxx_hidden_TokenTtl != nil +} + +func (x *LifecyclePolicy) ClearAssignTimeout() { + x.xxx_hidden_AssignTimeout = nil +} + +func (x *LifecyclePolicy) ClearTokenTtl() { + x.xxx_hidden_TokenTtl = nil +} + +type LifecyclePolicy_builder struct { + _ [0]func() // Prevents comparability and use of unkeyed literals for the builder. + + AssignTimeout *durationpb.Duration + TokenTtl *durationpb.Duration +} + +func (b0 LifecyclePolicy_builder) Build() *LifecyclePolicy { + m0 := &LifecyclePolicy{} + b, x := &b0, m0 + _, _ = b, x + x.xxx_hidden_AssignTimeout = b.AssignTimeout + x.xxx_hidden_TokenTtl = b.TokenTtl + return m0 +} + type Function struct { state protoimpl.MessageState `protogen:"opaque.v1"` xxx_hidden_Namespace *string `protobuf:"bytes,1,opt,name=namespace"` @@ -381,6 +793,10 @@ type Function struct { xxx_hidden_Metadata *string `protobuf:"bytes,4,opt,name=metadata"` xxx_hidden_Scale *Scale `protobuf:"bytes,5,opt,name=scale"` xxx_hidden_Oneshot bool `protobuf:"varint,6,opt,name=oneshot"` + xxx_hidden_Hpa *HpaPolicy `protobuf:"bytes,7,opt,name=hpa"` + xxx_hidden_Heartbeat *HeartbeatPolicy `protobuf:"bytes,8,opt,name=heartbeat"` + xxx_hidden_Proxy *ProxyPolicy `protobuf:"bytes,9,opt,name=proxy"` + xxx_hidden_Lifecycle *LifecyclePolicy `protobuf:"bytes,10,opt,name=lifecycle"` XXX_raceDetectHookData protoimpl.RaceDetectHookData XXX_presence [1]uint32 unknownFields protoimpl.UnknownFields @@ -389,7 +805,7 @@ type Function struct { func (x *Function) Reset() { *x = Function{} - mi := &file_types_proto_msgTypes[1] + mi := &file_types_proto_msgTypes[5] ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) ms.StoreMessageInfo(mi) } @@ -401,7 +817,7 @@ func (x *Function) String() string { func (*Function) ProtoMessage() {} func (x *Function) ProtoReflect() protoreflect.Message { - mi := &file_types_proto_msgTypes[1] + mi := &file_types_proto_msgTypes[5] if x != nil { ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) if ms.LoadMessageInfo() == nil { @@ -466,24 +882,52 @@ func (x *Function) GetOneshot() bool { return false } +func (x *Function) GetHpa() *HpaPolicy { + if x != nil { + return x.xxx_hidden_Hpa + } + return nil +} + +func (x *Function) GetHeartbeat() *HeartbeatPolicy { + if x != nil { + return x.xxx_hidden_Heartbeat + } + return nil +} + +func (x *Function) GetProxy() *ProxyPolicy { + if x != nil { + return x.xxx_hidden_Proxy + } + return nil +} + +func (x *Function) GetLifecycle() *LifecyclePolicy { + if x != nil { + return x.xxx_hidden_Lifecycle + } + return nil +} + func (x *Function) SetNamespace(v string) { x.xxx_hidden_Namespace = &v - protoimpl.X.SetPresent(&(x.XXX_presence[0]), 0, 6) + protoimpl.X.SetPresent(&(x.XXX_presence[0]), 0, 10) } func (x *Function) SetDeployment(v string) { x.xxx_hidden_Deployment = &v - protoimpl.X.SetPresent(&(x.XXX_presence[0]), 1, 6) + protoimpl.X.SetPresent(&(x.XXX_presence[0]), 1, 10) } func (x *Function) SetTenant(v string) { x.xxx_hidden_Tenant = &v - protoimpl.X.SetPresent(&(x.XXX_presence[0]), 2, 6) + protoimpl.X.SetPresent(&(x.XXX_presence[0]), 2, 10) } func (x *Function) SetMetadata(v string) { x.xxx_hidden_Metadata = &v - protoimpl.X.SetPresent(&(x.XXX_presence[0]), 3, 6) + protoimpl.X.SetPresent(&(x.XXX_presence[0]), 3, 10) } func (x *Function) SetScale(v *Scale) { @@ -492,7 +936,23 @@ func (x *Function) SetScale(v *Scale) { func (x *Function) SetOneshot(v bool) { x.xxx_hidden_Oneshot = v - protoimpl.X.SetPresent(&(x.XXX_presence[0]), 5, 6) + protoimpl.X.SetPresent(&(x.XXX_presence[0]), 5, 10) +} + +func (x *Function) SetHpa(v *HpaPolicy) { + x.xxx_hidden_Hpa = v +} + +func (x *Function) SetHeartbeat(v *HeartbeatPolicy) { + x.xxx_hidden_Heartbeat = v +} + +func (x *Function) SetProxy(v *ProxyPolicy) { + x.xxx_hidden_Proxy = v +} + +func (x *Function) SetLifecycle(v *LifecyclePolicy) { + x.xxx_hidden_Lifecycle = v } func (x *Function) HasNamespace() bool { @@ -537,6 +997,34 @@ func (x *Function) HasOneshot() bool { return protoimpl.X.Present(&(x.XXX_presence[0]), 5) } +func (x *Function) HasHpa() bool { + if x == nil { + return false + } + return x.xxx_hidden_Hpa != nil +} + +func (x *Function) HasHeartbeat() bool { + if x == nil { + return false + } + return x.xxx_hidden_Heartbeat != nil +} + +func (x *Function) HasProxy() bool { + if x == nil { + return false + } + return x.xxx_hidden_Proxy != nil +} + +func (x *Function) HasLifecycle() bool { + if x == nil { + return false + } + return x.xxx_hidden_Lifecycle != nil +} + func (x *Function) ClearNamespace() { protoimpl.X.ClearPresent(&(x.XXX_presence[0]), 0) x.xxx_hidden_Namespace = nil @@ -566,6 +1054,22 @@ func (x *Function) ClearOneshot() { x.xxx_hidden_Oneshot = false } +func (x *Function) ClearHpa() { + x.xxx_hidden_Hpa = nil +} + +func (x *Function) ClearHeartbeat() { + x.xxx_hidden_Heartbeat = nil +} + +func (x *Function) ClearProxy() { + x.xxx_hidden_Proxy = nil +} + +func (x *Function) ClearLifecycle() { + x.xxx_hidden_Lifecycle = nil +} + type Function_builder struct { _ [0]func() // Prevents comparability and use of unkeyed literals for the builder. @@ -575,6 +1079,10 @@ type Function_builder struct { Metadata *string Scale *Scale Oneshot *bool + Hpa *HpaPolicy + Heartbeat *HeartbeatPolicy + Proxy *ProxyPolicy + Lifecycle *LifecyclePolicy } func (b0 Function_builder) Build() *Function { @@ -582,26 +1090,30 @@ func (b0 Function_builder) Build() *Function { b, x := &b0, m0 _, _ = b, x if b.Namespace != nil { - protoimpl.X.SetPresentNonAtomic(&(x.XXX_presence[0]), 0, 6) + protoimpl.X.SetPresentNonAtomic(&(x.XXX_presence[0]), 0, 10) x.xxx_hidden_Namespace = b.Namespace } if b.Deployment != nil { - protoimpl.X.SetPresentNonAtomic(&(x.XXX_presence[0]), 1, 6) + protoimpl.X.SetPresentNonAtomic(&(x.XXX_presence[0]), 1, 10) x.xxx_hidden_Deployment = b.Deployment } if b.Tenant != nil { - protoimpl.X.SetPresentNonAtomic(&(x.XXX_presence[0]), 2, 6) + protoimpl.X.SetPresentNonAtomic(&(x.XXX_presence[0]), 2, 10) x.xxx_hidden_Tenant = b.Tenant } if b.Metadata != nil { - protoimpl.X.SetPresentNonAtomic(&(x.XXX_presence[0]), 3, 6) + protoimpl.X.SetPresentNonAtomic(&(x.XXX_presence[0]), 3, 10) x.xxx_hidden_Metadata = b.Metadata } x.xxx_hidden_Scale = b.Scale if b.Oneshot != nil { - protoimpl.X.SetPresentNonAtomic(&(x.XXX_presence[0]), 5, 6) + protoimpl.X.SetPresentNonAtomic(&(x.XXX_presence[0]), 5, 10) x.xxx_hidden_Oneshot = *b.Oneshot } + x.xxx_hidden_Hpa = b.Hpa + x.xxx_hidden_Heartbeat = b.Heartbeat + x.xxx_hidden_Proxy = b.Proxy + x.xxx_hidden_Lifecycle = b.Lifecycle return m0 } @@ -623,7 +1135,7 @@ type Instance struct { func (x *Instance) Reset() { *x = Instance{} - mi := &file_types_proto_msgTypes[2] + mi := &file_types_proto_msgTypes[6] ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) ms.StoreMessageInfo(mi) } @@ -635,7 +1147,7 @@ func (x *Instance) String() string { func (*Instance) ProtoMessage() {} func (x *Instance) ProtoReflect() protoreflect.Message { - mi := &file_types_proto_msgTypes[2] + mi := &file_types_proto_msgTypes[6] if x != nil { ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) if ms.LoadMessageInfo() == nil { @@ -897,7 +1409,7 @@ type Heartbeat struct { func (x *Heartbeat) Reset() { *x = Heartbeat{} - mi := &file_types_proto_msgTypes[3] + mi := &file_types_proto_msgTypes[7] ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) ms.StoreMessageInfo(mi) } @@ -909,7 +1421,7 @@ func (x *Heartbeat) String() string { func (*Heartbeat) ProtoMessage() {} func (x *Heartbeat) ProtoReflect() protoreflect.Message { - mi := &file_types_proto_msgTypes[3] + mi := &file_types_proto_msgTypes[7] if x != nil { ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) if ms.LoadMessageInfo() == nil { @@ -1021,7 +1533,7 @@ type ScaleMetric struct { func (x *ScaleMetric) Reset() { *x = ScaleMetric{} - mi := &file_types_proto_msgTypes[4] + mi := &file_types_proto_msgTypes[8] ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) ms.StoreMessageInfo(mi) } @@ -1033,7 +1545,7 @@ func (x *ScaleMetric) String() string { func (*ScaleMetric) ProtoMessage() {} func (x *ScaleMetric) ProtoReflect() protoreflect.Message { - mi := &file_types_proto_msgTypes[4] + mi := &file_types_proto_msgTypes[8] if x != nil { ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) if ms.LoadMessageInfo() == nil { @@ -1131,7 +1643,7 @@ type ScaleDecision struct { func (x *ScaleDecision) Reset() { *x = ScaleDecision{} - mi := &file_types_proto_msgTypes[5] + mi := &file_types_proto_msgTypes[9] ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) ms.StoreMessageInfo(mi) } @@ -1143,7 +1655,7 @@ func (x *ScaleDecision) String() string { func (*ScaleDecision) ProtoMessage() {} func (x *ScaleDecision) ProtoReflect() protoreflect.Message { - mi := &file_types_proto_msgTypes[5] + mi := &file_types_proto_msgTypes[9] if x != nil { ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) if ms.LoadMessageInfo() == nil { @@ -1285,7 +1797,7 @@ type Event struct { func (x *Event) Reset() { *x = Event{} - mi := &file_types_proto_msgTypes[6] + mi := &file_types_proto_msgTypes[10] ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) ms.StoreMessageInfo(mi) } @@ -1297,7 +1809,7 @@ func (x *Event) String() string { func (*Event) ProtoMessage() {} func (x *Event) ProtoReflect() protoreflect.Message { - mi := &file_types_proto_msgTypes[6] + mi := &file_types_proto_msgTypes[10] if x != nil { ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) if ms.LoadMessageInfo() == nil { @@ -1475,7 +1987,7 @@ type ConfigValue struct { func (x *ConfigValue) Reset() { *x = ConfigValue{} - mi := &file_types_proto_msgTypes[7] + mi := &file_types_proto_msgTypes[11] ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) ms.StoreMessageInfo(mi) } @@ -1487,7 +1999,7 @@ func (x *ConfigValue) String() string { func (*ConfigValue) ProtoMessage() {} func (x *ConfigValue) ProtoReflect() protoreflect.Message { - mi := &file_types_proto_msgTypes[7] + mi := &file_types_proto_msgTypes[11] if x != nil { ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) if ms.LoadMessageInfo() == nil { @@ -1618,7 +2130,7 @@ type HeartbeatState struct { func (x *HeartbeatState) Reset() { *x = HeartbeatState{} - mi := &file_types_proto_msgTypes[8] + mi := &file_types_proto_msgTypes[12] ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) ms.StoreMessageInfo(mi) } @@ -1630,7 +2142,7 @@ func (x *HeartbeatState) String() string { func (*HeartbeatState) ProtoMessage() {} func (x *HeartbeatState) ProtoReflect() protoreflect.Message { - mi := &file_types_proto_msgTypes[8] + mi := &file_types_proto_msgTypes[12] if x != nil { ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) if ms.LoadMessageInfo() == nil { @@ -1724,7 +2236,7 @@ type SupervisorState struct { func (x *SupervisorState) Reset() { *x = SupervisorState{} - mi := &file_types_proto_msgTypes[9] + mi := &file_types_proto_msgTypes[13] ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) ms.StoreMessageInfo(mi) } @@ -1736,7 +2248,7 @@ func (x *SupervisorState) String() string { func (*SupervisorState) ProtoMessage() {} func (x *SupervisorState) ProtoReflect() protoreflect.Message { - mi := &file_types_proto_msgTypes[9] + mi := &file_types_proto_msgTypes[13] if x != nil { ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) if ms.LoadMessageInfo() == nil { @@ -1893,7 +2405,7 @@ type ClusterState struct { func (x *ClusterState) Reset() { *x = ClusterState{} - mi := &file_types_proto_msgTypes[10] + mi := &file_types_proto_msgTypes[14] ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) ms.StoreMessageInfo(mi) } @@ -1905,7 +2417,7 @@ func (x *ClusterState) String() string { func (*ClusterState) ProtoMessage() {} func (x *ClusterState) ProtoReflect() protoreflect.Message { - mi := &file_types_proto_msgTypes[10] + mi := &file_types_proto_msgTypes[14] if x != nil { ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) if ms.LoadMessageInfo() == nil { @@ -2046,13 +2558,26 @@ var File_types_proto protoreflect.FileDescriptor const file_types_proto_rawDesc = "" + "\n" + - "\vtypes.proto\x12\askipper\x1a!google/protobuf/go_features.proto\x1a\x1fgoogle/protobuf/timestamp.proto\"\xf8\x01\n" + + "\vtypes.proto\x12\askipper\x1a\x1egoogle/protobuf/duration.proto\x1a!google/protobuf/go_features.proto\x1a\x1fgoogle/protobuf/timestamp.proto\"\xf8\x01\n" + "\x05Scale\x12#\n" + "\rmin_instances\x18\x01 \x01(\rR\fminInstances\x12#\n" + "\rmax_instances\x18\x02 \x01(\rR\fmaxInstances\x123\n" + "\x16target_cpu_usage_milli\x18\x03 \x01(\rR\x13targetCpuUsageMilli\x125\n" + "\x17target_memory_usage_mib\x18\x04 \x01(\rR\x14targetMemoryUsageMib\x129\n" + - "\x19target_in_flight_requests\x18\x05 \x01(\rR\x16targetInFlightRequests\"\xbc\x01\n" + + "\x19target_in_flight_requests\x18\x05 \x01(\rR\x16targetInFlightRequests\"\xd0\x01\n" + + "\tHpaPolicy\x12\x1c\n" + + "\ttolerance\x18\x01 \x01(\x01R\ttolerance\x12R\n" + + "\x17downscale_stabilization\x18\x02 \x01(\v2\x19.google.protobuf.DurationR\x16downscaleStabilization\x12Q\n" + + "\x17initial_readiness_delay\x18\x03 \x01(\v2\x19.google.protobuf.DurationR\x15initialReadinessDelay\"F\n" + + "\x0fHeartbeatPolicy\x123\n" + + "\atimeout\x18\x01 \x01(\v2\x19.google.protobuf.DurationR\atimeout\"\xbe\x01\n" + + "\vProxyPolicy\x12!\n" + + "\fmax_attempts\x18\x01 \x01(\rR\vmaxAttempts\x12E\n" + + "\x11retry_min_backoff\x18\x02 \x01(\v2\x19.google.protobuf.DurationR\x0fretryMinBackoff\x12E\n" + + "\x11retry_max_backoff\x18\x03 \x01(\v2\x19.google.protobuf.DurationR\x0fretryMaxBackoff\"\x8b\x01\n" + + "\x0fLifecyclePolicy\x12@\n" + + "\x0eassign_timeout\x18\x01 \x01(\v2\x19.google.protobuf.DurationR\rassignTimeout\x126\n" + + "\ttoken_ttl\x18\x02 \x01(\v2\x19.google.protobuf.DurationR\btokenTtl\"\xfe\x02\n" + "\bFunction\x12\x1c\n" + "\tnamespace\x18\x01 \x01(\tR\tnamespace\x12\x1e\n" + "\n" + @@ -2061,7 +2586,12 @@ const file_types_proto_rawDesc = "" + "\x06tenant\x18\x03 \x01(\tR\x06tenant\x12\x1a\n" + "\bmetadata\x18\x04 \x01(\tR\bmetadata\x12$\n" + "\x05scale\x18\x05 \x01(\v2\x0e.skipper.ScaleR\x05scale\x12\x18\n" + - "\aoneshot\x18\x06 \x01(\bR\aoneshot\"\xc8\x02\n" + + "\aoneshot\x18\x06 \x01(\bR\aoneshot\x12$\n" + + "\x03hpa\x18\a \x01(\v2\x12.skipper.HpaPolicyR\x03hpa\x126\n" + + "\theartbeat\x18\b \x01(\v2\x18.skipper.HeartbeatPolicyR\theartbeat\x12*\n" + + "\x05proxy\x18\t \x01(\v2\x14.skipper.ProxyPolicyR\x05proxy\x126\n" + + "\tlifecycle\x18\n" + + " \x01(\v2\x18.skipper.LifecyclePolicyR\tlifecycle\"\xc8\x02\n" + "\bInstance\x12-\n" + "\bfunction\x18\x01 \x01(\v2\x11.skipper.FunctionR\bfunction\x12\x12\n" + "\x04name\x18\x02 \x01(\tR\x04name\x12\x12\n" + @@ -2134,50 +2664,66 @@ const file_types_proto_rawDesc = "" + "\x13EVENT_SEVERITY_WARN\x10\x02B8Z.github.com/gadget-inc/skipper/internal/skipper\x92\x03\x05\xd2>\x02\x10\x03b\beditionsp\xe8\a" var file_types_proto_enumTypes = make([]protoimpl.EnumInfo, 3) -var file_types_proto_msgTypes = make([]protoimpl.MessageInfo, 11) +var file_types_proto_msgTypes = make([]protoimpl.MessageInfo, 15) var file_types_proto_goTypes = []any{ (ScaleReason)(0), // 0: skipper.ScaleReason (EventType)(0), // 1: skipper.EventType (EventSeverity)(0), // 2: skipper.EventSeverity (*Scale)(nil), // 3: skipper.Scale - (*Function)(nil), // 4: skipper.Function - (*Instance)(nil), // 5: skipper.Instance - (*Heartbeat)(nil), // 6: skipper.Heartbeat - (*ScaleMetric)(nil), // 7: skipper.ScaleMetric - (*ScaleDecision)(nil), // 8: skipper.ScaleDecision - (*Event)(nil), // 9: skipper.Event - (*ConfigValue)(nil), // 10: skipper.ConfigValue - (*HeartbeatState)(nil), // 11: skipper.HeartbeatState - (*SupervisorState)(nil), // 12: skipper.SupervisorState - (*ClusterState)(nil), // 13: skipper.ClusterState - (*timestamppb.Timestamp)(nil), // 14: google.protobuf.Timestamp + (*HpaPolicy)(nil), // 4: skipper.HpaPolicy + (*HeartbeatPolicy)(nil), // 5: skipper.HeartbeatPolicy + (*ProxyPolicy)(nil), // 6: skipper.ProxyPolicy + (*LifecyclePolicy)(nil), // 7: skipper.LifecyclePolicy + (*Function)(nil), // 8: skipper.Function + (*Instance)(nil), // 9: skipper.Instance + (*Heartbeat)(nil), // 10: skipper.Heartbeat + (*ScaleMetric)(nil), // 11: skipper.ScaleMetric + (*ScaleDecision)(nil), // 12: skipper.ScaleDecision + (*Event)(nil), // 13: skipper.Event + (*ConfigValue)(nil), // 14: skipper.ConfigValue + (*HeartbeatState)(nil), // 15: skipper.HeartbeatState + (*SupervisorState)(nil), // 16: skipper.SupervisorState + (*ClusterState)(nil), // 17: skipper.ClusterState + (*durationpb.Duration)(nil), // 18: google.protobuf.Duration + (*timestamppb.Timestamp)(nil), // 19: google.protobuf.Timestamp } var file_types_proto_depIdxs = []int32{ - 3, // 0: skipper.Function.scale:type_name -> skipper.Scale - 4, // 1: skipper.Instance.function:type_name -> skipper.Function - 14, // 2: skipper.Instance.assigned_at:type_name -> google.protobuf.Timestamp - 14, // 3: skipper.Instance.ready_at:type_name -> google.protobuf.Timestamp - 4, // 4: skipper.Heartbeat.function:type_name -> skipper.Function - 14, // 5: skipper.Heartbeat.timestamp:type_name -> google.protobuf.Timestamp - 0, // 6: skipper.ScaleDecision.reason:type_name -> skipper.ScaleReason - 7, // 7: skipper.ScaleDecision.metrics:type_name -> skipper.ScaleMetric - 14, // 8: skipper.Event.timestamp:type_name -> google.protobuf.Timestamp - 4, // 9: skipper.Event.function:type_name -> skipper.Function - 1, // 10: skipper.Event.type:type_name -> skipper.EventType - 2, // 11: skipper.Event.severity:type_name -> skipper.EventSeverity - 6, // 12: skipper.HeartbeatState.heartbeat:type_name -> skipper.Heartbeat - 4, // 13: skipper.SupervisorState.function:type_name -> skipper.Function - 5, // 14: skipper.SupervisorState.instances:type_name -> skipper.Instance - 11, // 15: skipper.SupervisorState.router_heartbeats:type_name -> skipper.HeartbeatState - 14, // 16: skipper.ClusterState.started_at:type_name -> google.protobuf.Timestamp - 12, // 17: skipper.ClusterState.supervisors:type_name -> skipper.SupervisorState - 9, // 18: skipper.ClusterState.events:type_name -> skipper.Event - 10, // 19: skipper.ClusterState.config:type_name -> skipper.ConfigValue - 20, // [20:20] is the sub-list for method output_type - 20, // [20:20] is the sub-list for method input_type - 20, // [20:20] is the sub-list for extension type_name - 20, // [20:20] is the sub-list for extension extendee - 0, // [0:20] is the sub-list for field type_name + 18, // 0: skipper.HpaPolicy.downscale_stabilization:type_name -> google.protobuf.Duration + 18, // 1: skipper.HpaPolicy.initial_readiness_delay:type_name -> google.protobuf.Duration + 18, // 2: skipper.HeartbeatPolicy.timeout:type_name -> google.protobuf.Duration + 18, // 3: skipper.ProxyPolicy.retry_min_backoff:type_name -> google.protobuf.Duration + 18, // 4: skipper.ProxyPolicy.retry_max_backoff:type_name -> google.protobuf.Duration + 18, // 5: skipper.LifecyclePolicy.assign_timeout:type_name -> google.protobuf.Duration + 18, // 6: skipper.LifecyclePolicy.token_ttl:type_name -> google.protobuf.Duration + 3, // 7: skipper.Function.scale:type_name -> skipper.Scale + 4, // 8: skipper.Function.hpa:type_name -> skipper.HpaPolicy + 5, // 9: skipper.Function.heartbeat:type_name -> skipper.HeartbeatPolicy + 6, // 10: skipper.Function.proxy:type_name -> skipper.ProxyPolicy + 7, // 11: skipper.Function.lifecycle:type_name -> skipper.LifecyclePolicy + 8, // 12: skipper.Instance.function:type_name -> skipper.Function + 19, // 13: skipper.Instance.assigned_at:type_name -> google.protobuf.Timestamp + 19, // 14: skipper.Instance.ready_at:type_name -> google.protobuf.Timestamp + 8, // 15: skipper.Heartbeat.function:type_name -> skipper.Function + 19, // 16: skipper.Heartbeat.timestamp:type_name -> google.protobuf.Timestamp + 0, // 17: skipper.ScaleDecision.reason:type_name -> skipper.ScaleReason + 11, // 18: skipper.ScaleDecision.metrics:type_name -> skipper.ScaleMetric + 19, // 19: skipper.Event.timestamp:type_name -> google.protobuf.Timestamp + 8, // 20: skipper.Event.function:type_name -> skipper.Function + 1, // 21: skipper.Event.type:type_name -> skipper.EventType + 2, // 22: skipper.Event.severity:type_name -> skipper.EventSeverity + 10, // 23: skipper.HeartbeatState.heartbeat:type_name -> skipper.Heartbeat + 8, // 24: skipper.SupervisorState.function:type_name -> skipper.Function + 9, // 25: skipper.SupervisorState.instances:type_name -> skipper.Instance + 15, // 26: skipper.SupervisorState.router_heartbeats:type_name -> skipper.HeartbeatState + 19, // 27: skipper.ClusterState.started_at:type_name -> google.protobuf.Timestamp + 16, // 28: skipper.ClusterState.supervisors:type_name -> skipper.SupervisorState + 13, // 29: skipper.ClusterState.events:type_name -> skipper.Event + 14, // 30: skipper.ClusterState.config:type_name -> skipper.ConfigValue + 31, // [31:31] is the sub-list for method output_type + 31, // [31:31] is the sub-list for method input_type + 31, // [31:31] is the sub-list for extension type_name + 31, // [31:31] is the sub-list for extension extendee + 0, // [0:31] is the sub-list for field type_name } func init() { file_types_proto_init() } @@ -2191,7 +2737,7 @@ func file_types_proto_init() { GoPackagePath: reflect.TypeOf(x{}).PkgPath(), RawDescriptor: unsafe.Slice(unsafe.StringData(file_types_proto_rawDesc), len(file_types_proto_rawDesc)), NumEnums: 3, - NumMessages: 11, + NumMessages: 15, NumExtensions: 0, NumServices: 0, }, diff --git a/internal/skipper/types.pb.json.go b/internal/skipper/types.pb.json.go index 3a615bc8..d359e29f 100644 --- a/internal/skipper/types.pb.json.go +++ b/internal/skipper/types.pb.json.go @@ -21,6 +21,62 @@ func (msg *Scale) UnmarshalJSON(b []byte) error { }.Unmarshal(b, msg) } +// MarshalJSON implements json.Marshaler +func (msg *HpaPolicy) MarshalJSON() ([]byte, error) { + return protojson.MarshalOptions{ + UseProtoNames: true, + }.Marshal(msg) +} + +// UnmarshalJSON implements json.Unmarshaler +func (msg *HpaPolicy) UnmarshalJSON(b []byte) error { + return protojson.UnmarshalOptions{ + DiscardUnknown: true, + }.Unmarshal(b, msg) +} + +// MarshalJSON implements json.Marshaler +func (msg *HeartbeatPolicy) MarshalJSON() ([]byte, error) { + return protojson.MarshalOptions{ + UseProtoNames: true, + }.Marshal(msg) +} + +// UnmarshalJSON implements json.Unmarshaler +func (msg *HeartbeatPolicy) UnmarshalJSON(b []byte) error { + return protojson.UnmarshalOptions{ + DiscardUnknown: true, + }.Unmarshal(b, msg) +} + +// MarshalJSON implements json.Marshaler +func (msg *ProxyPolicy) MarshalJSON() ([]byte, error) { + return protojson.MarshalOptions{ + UseProtoNames: true, + }.Marshal(msg) +} + +// UnmarshalJSON implements json.Unmarshaler +func (msg *ProxyPolicy) UnmarshalJSON(b []byte) error { + return protojson.UnmarshalOptions{ + DiscardUnknown: true, + }.Unmarshal(b, msg) +} + +// MarshalJSON implements json.Marshaler +func (msg *LifecyclePolicy) MarshalJSON() ([]byte, error) { + return protojson.MarshalOptions{ + UseProtoNames: true, + }.Marshal(msg) +} + +// UnmarshalJSON implements json.Unmarshaler +func (msg *LifecyclePolicy) UnmarshalJSON(b []byte) error { + return protojson.UnmarshalOptions{ + DiscardUnknown: true, + }.Unmarshal(b, msg) +} + // MarshalJSON implements json.Marshaler func (msg *Function) MarshalJSON() ([]byte, error) { return protojson.MarshalOptions{ diff --git a/internal/skipper/types.proto b/internal/skipper/types.proto index b6a68790..321b7c45 100644 --- a/internal/skipper/types.proto +++ b/internal/skipper/types.proto @@ -2,6 +2,7 @@ edition = "2023"; package skipper; +import "google/protobuf/duration.proto"; import "google/protobuf/go_features.proto"; import "google/protobuf/timestamp.proto"; @@ -16,6 +17,27 @@ message Scale { uint32 target_in_flight_requests = 5; } +message HpaPolicy { + double tolerance = 1; + google.protobuf.Duration downscale_stabilization = 2; + google.protobuf.Duration initial_readiness_delay = 3; +} + +message HeartbeatPolicy { + google.protobuf.Duration timeout = 1; +} + +message ProxyPolicy { + uint32 max_attempts = 1; + google.protobuf.Duration retry_min_backoff = 2; + google.protobuf.Duration retry_max_backoff = 3; +} + +message LifecyclePolicy { + google.protobuf.Duration assign_timeout = 1; + google.protobuf.Duration token_ttl = 2; +} + message Function { string namespace = 1; string deployment = 2; @@ -23,6 +45,10 @@ message Function { string metadata = 4; Scale scale = 5; bool oneshot = 6; + HpaPolicy hpa = 7; + HeartbeatPolicy heartbeat = 8; + ProxyPolicy proxy = 9; + LifecyclePolicy lifecycle = 10; } message Instance { From 50a7d9e350e6e777e799adaf86890661b4ce782e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Scott=20C=C3=B4t=C3=A9?= Date: Tue, 5 May 2026 01:07:52 -0400 Subject: [PATCH 02/17] Apply per-function heartbeat timeout in supervisor 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. --- .../controller/per_function_heartbeat_test.go | 121 ++++++++++++++++++ internal/controller/supervisor.go | 15 ++- internal/controller/supervisor_test.go | 6 +- 3 files changed, 134 insertions(+), 8 deletions(-) create mode 100644 internal/controller/per_function_heartbeat_test.go diff --git a/internal/controller/per_function_heartbeat_test.go b/internal/controller/per_function_heartbeat_test.go new file mode 100644 index 00000000..534ab667 --- /dev/null +++ b/internal/controller/per_function_heartbeat_test.go @@ -0,0 +1,121 @@ +package controller + +import ( + "testing" + "time" + + "github.com/gadget-inc/skipper/internal/fixture" + "github.com/gadget-inc/skipper/internal/skipper" + "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/types/known/durationpb" + "google.golang.org/protobuf/types/known/timestamppb" + "gotest.tools/v3/assert" +) + +// silencedHeartbeat returns a heartbeat for fn whose timestamp is in the past +// by the supplied amount, simulating a function that has gone idle. +func silencedHeartbeat(fn *skipper.Function, idleFor time.Duration) *skipper.Heartbeat { + return skipper.Heartbeat_builder{ + Function: fn, + Timestamp: timestamppb.New(time.Now().Add(-idleFor)), + }.Build() +} + +func withHeartbeatTimeout(fn *skipper.Function, timeout time.Duration) *skipper.Function { + cloned := proto.Clone(fn).(*skipper.Function) + cloned.SetHeartbeat(skipper.HeartbeatPolicy_builder{ + Timeout: durationpb.New(timeout), + }.Build()) + return cloned +} + +// TestCalculateDesiredInstancesPerFunctionHeartbeat verifies that two +// functions silenced for the same wall-clock duration but configured with +// different heartbeat.timeout values are scaled to zero on independent +// schedules: the function with the shorter timeout terminates first; the +// function with the longer timeout is still running. +func TestCalculateDesiredInstancesPerFunctionHeartbeat(t *testing.T) { + t.Parallel() + + cfg := testConfig() + cfg.HeartbeatTimeout = 90 * time.Second // cluster default + + idleFor := 30 * time.Second + + shortTimeoutFn := withHeartbeatTimeout(fixture.NewFunction(t), 15*time.Second) + longTimeoutFn := withHeartbeatTimeout(fixture.NewFunction(t), 5*time.Minute) + + shortDecision := calculateDesiredInstances(t.Context(), cfg, shortTimeoutFn, silencedHeartbeat(shortTimeoutFn, idleFor), nil) + longDecision := calculateDesiredInstances(t.Context(), cfg, longTimeoutFn, silencedHeartbeat(longTimeoutFn, idleFor), nil) + + assert.Equal(t, shortDecision.GetReason(), skipper.ScaleReason_SCALE_REASON_HEARTBEAT_TIMEOUT, + "short timeout (%s) should scale to zero after %s of idle", shortTimeoutFn.HeartbeatTimeout(cfg.HeartbeatTimeout), idleFor) + assert.Equal(t, shortDecision.GetDesiredInstances(), uint32(0)) + + assert.Assert(t, longDecision.GetReason() != skipper.ScaleReason_SCALE_REASON_HEARTBEAT_TIMEOUT, + "long timeout (%s) should still be alive after %s of idle, got reason %s", + longTimeoutFn.HeartbeatTimeout(cfg.HeartbeatTimeout), idleFor, longDecision.GetReason()) +} + +// TestCalculateDesiredInstancesOmittedHeartbeatPolicy verifies that a +// function omitting the heartbeat sub-message falls back to the cluster flag +// at the heartbeat-timeout decision site -- behavior matches today. +func TestCalculateDesiredInstancesOmittedHeartbeatPolicy(t *testing.T) { + t.Parallel() + + cfg := testConfig() + cfg.HeartbeatTimeout = 90 * time.Second + + fn := fixture.NewFunction(t) + assert.Assert(t, fn.GetHeartbeat() == nil, "fixture function must omit heartbeat policy") + + // Idle for less than the cluster default -- should not scale to zero. + live := calculateDesiredInstances(t.Context(), cfg, fn, silencedHeartbeat(fn, 30*time.Second), nil) + assert.Assert(t, live.GetReason() != skipper.ScaleReason_SCALE_REASON_HEARTBEAT_TIMEOUT) + + // Idle past the cluster default -- should scale to zero. + dead := calculateDesiredInstances(t.Context(), cfg, fn, silencedHeartbeat(fn, 2*time.Minute), nil) + assert.Equal(t, dead.GetReason(), skipper.ScaleReason_SCALE_REASON_HEARTBEAT_TIMEOUT) + assert.Equal(t, dead.GetDesiredInstances(), uint32(0)) +} + +// TestSupervisorUpdateFunctionPicksUpHeartbeatTimeout exercises the existing +// CAS path in Supervisor.updateFunction with two functions that share an +// identity but differ only in heartbeat.timeout. The new value drives the +// next idle decision; no new pool is created. +func TestSupervisorUpdateFunctionPicksUpHeartbeatTimeout(t *testing.T) { + t.Parallel() + + cfg := testConfig() + cfg.HeartbeatTimeout = 90 * time.Second + + base := fixture.NewFunction(t) + + requestA := withHeartbeatTimeout(base, 15*time.Second) + requestB := withHeartbeatTimeout(base, 5*time.Minute) + + assert.Equal(t, requestA.Hash(), requestB.Hash(), "requests must share identity") + assert.Assert(t, !proto.Equal(requestA, requestB), "requests must differ in spec") + + ctrl := New(cfg, nil, nil, nil) + supervisor := &Supervisor{ + ctrl: ctrl, + } + supervisor.fn.Store(requestA) + + // Idle for 30s. Under requestA's 15s timeout this would scale to zero. + idleHeartbeat := silencedHeartbeat(requestA, 30*time.Second) + beforeUpdate := calculateDesiredInstances(t.Context(), cfg, supervisor.fn.Load(), idleHeartbeat, nil) + assert.Equal(t, beforeUpdate.GetReason(), skipper.ScaleReason_SCALE_REASON_HEARTBEAT_TIMEOUT, + "requestA with 15s timeout should scale to zero after 30s idle") + + // requestB arrives with a 5-minute timeout, which is wider than the 30s + // idle window, so the heartbeat-timeout reason should not fire. + supervisor.updateFunction(requestB) + assert.Assert(t, supervisor.fn.Load() == requestB, "updateFunction should swap to requestB via CAS") + + idleHeartbeat = silencedHeartbeat(requestB, 30*time.Second) + afterUpdate := calculateDesiredInstances(t.Context(), cfg, supervisor.fn.Load(), idleHeartbeat, nil) + assert.Assert(t, afterUpdate.GetReason() != skipper.ScaleReason_SCALE_REASON_HEARTBEAT_TIMEOUT, + "requestB with 5m timeout should not scale to zero after 30s idle, got %s", afterUpdate.GetReason()) +} diff --git a/internal/controller/supervisor.go b/internal/controller/supervisor.go index 68bb1587..f6127f09 100644 --- a/internal/controller/supervisor.go +++ b/internal/controller/supervisor.go @@ -172,7 +172,9 @@ func (s *Supervisor) heartbeat(routerIP string, heartbeat *skipper.Heartbeat) { return existing, xsync.CancelOp }) - // garbage collect expired router heartbeats + // garbage collect expired router heartbeats. This GC is keyed by router + // IP and crosses every function this supervisor handles, so we bypass + // the per-function heartbeat resolver and use the cluster default. for routerIP, heartbeat := range s.routerHeartbeats.AllRelaxed() { if time.Since(heartbeat.GetTimestamp().AsTime()) > s.ctrl.config.HeartbeatTimeout { s.routerHeartbeats.Delete(routerIP) @@ -283,7 +285,7 @@ func (s *Supervisor) converge(ctx context.Context) error { heartbeat := s.combinedHeartbeat(fn, instances) ctx = telemetry.With(ctx, skipper.FunctionKey.Attr(fn), skipper.HeartbeatKey.Attr(heartbeat)) - scalingDecision := calculateDesiredInstances(ctx, s.ctrl.config, heartbeat, instances) + scalingDecision := calculateDesiredInstances(ctx, s.ctrl.config, fn, heartbeat, instances) // For oneshot functions, the converge loop is a safety net only. // All assignment happens synchronously in GetInstance via assignPod. @@ -299,6 +301,8 @@ func (s *Supervisor) converge(ctx context.Context) error { // oneshot requests (older than HeartbeatTimeout), this would incorrectly // trigger heartbeat_timeout before the router's heartbeats have arrived. // Skip deletion until we've been running long enough to receive heartbeats. + // This grace window is a controller-wide startup concern, not per-function, + // so it stays on the cluster flag rather than the resolver. if time.Since(s.ctrl.StartedAt()) < s.ctrl.config.HeartbeatTimeout { return nil } @@ -334,7 +338,8 @@ func (s *Supervisor) converge(ctx context.Context) error { // 2. Receive heartbeats from routers (HeartbeatTimeout) - without this, functions with // instances assigned longer than HeartbeatTimeout ago would immediately trigger // heartbeat_timeout scale-to-zero when a new controller starts with empty heartbeat state - protectionPeriod := max(s.ctrl.config.HPADownscaleStabilization, s.ctrl.config.HeartbeatTimeout) + // HPADownscaleStabilization stays on the cluster flag here; per-function migration lands in Phase 3. + protectionPeriod := max(s.ctrl.config.HPADownscaleStabilization, fn.HeartbeatTimeout(s.ctrl.config.HeartbeatTimeout)) if time.Since(s.ctrl.StartedAt()) < protectionPeriod { log.Debug(ctx, "skipping scale down because controller hasn't been running long enough", slog.Time("started_at", s.ctrl.StartedAt())) return nil @@ -812,8 +817,8 @@ func calculateDesiredInstancesForMetric(_ context.Context, cfg *Config, metric M } // calculateDesiredInstances computes desired instances based on multiple metrics -func calculateDesiredInstances(ctx context.Context, cfg *Config, heartbeat *skipper.Heartbeat, instances []*skipper.Instance) *skipper.ScaleDecision { - if !heartbeat.HasTimestamp() || time.Since(heartbeat.GetTimestamp().AsTime()) >= cfg.HeartbeatTimeout { +func calculateDesiredInstances(ctx context.Context, cfg *Config, fn *skipper.Function, heartbeat *skipper.Heartbeat, instances []*skipper.Instance) *skipper.ScaleDecision { + if !heartbeat.HasTimestamp() || time.Since(heartbeat.GetTimestamp().AsTime()) >= fn.HeartbeatTimeout(cfg.HeartbeatTimeout) { decision := &skipper.ScaleDecision{} decision.SetDesiredInstances(0) decision.SetUnclampedDesiredInstances(0) diff --git a/internal/controller/supervisor_test.go b/internal/controller/supervisor_test.go index 4fe44d37..29fe11e2 100644 --- a/internal/controller/supervisor_test.go +++ b/internal/controller/supervisor_test.go @@ -1867,7 +1867,7 @@ func TestCalculateDesiredInstances(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - decision := calculateDesiredInstances(t.Context(), cfg, tc.heartbeat, tc.instances) + decision := calculateDesiredInstances(t.Context(), cfg, tc.heartbeat.GetFunction(), tc.heartbeat, tc.instances) assert.Equal(t, tc.expectedDesiredInstances, decision.GetDesiredInstances()) assert.Equal(t, tc.expectedUnclampedDesired, decision.GetUnclampedDesiredInstances()) @@ -3255,7 +3255,7 @@ func TestConvergeDoesNotReplaceStaleInstancesWhenScalingDown(t *testing.T) { // Verify the scaling decision would request 1 instance heartbeat := supervisor.combinedHeartbeat(fn, instances) - scalingDecision := calculateDesiredInstances(ctx, cfg, heartbeat, instances) + scalingDecision := calculateDesiredInstances(ctx, cfg, fn, heartbeat, instances) assert.Assert(t, scalingDecision.GetDesiredInstances() == 1, "expected scaling decision of 1 instance, got %d", scalingDecision.GetDesiredInstances()) @@ -3814,7 +3814,7 @@ func TestCalculateDesiredInstancesOneshot(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - decision := calculateDesiredInstances(t.Context(), cfg, tc.heartbeat, tc.instances) + decision := calculateDesiredInstances(t.Context(), cfg, tc.heartbeat.GetFunction(), tc.heartbeat, tc.instances) assert.Equal(t, tc.expectedDesiredInstances, decision.GetDesiredInstances()) assert.Equal(t, tc.expectedUnclampedDesired, decision.GetUnclampedDesiredInstances()) From 1202b924b741c7495863ec7400c7240e7f6e5919 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Scott=20C=C3=B4t=C3=A9?= Date: Tue, 5 May 2026 01:12:58 -0400 Subject: [PATCH 03/17] Apply per-function HPA timing in supervisor 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. --- internal/controller/per_function_hpa_test.go | 226 +++++++++++++++++++ internal/controller/supervisor.go | 30 ++- internal/controller/supervisor_test.go | 2 +- 3 files changed, 249 insertions(+), 9 deletions(-) create mode 100644 internal/controller/per_function_hpa_test.go diff --git a/internal/controller/per_function_hpa_test.go b/internal/controller/per_function_hpa_test.go new file mode 100644 index 00000000..23b2242b --- /dev/null +++ b/internal/controller/per_function_hpa_test.go @@ -0,0 +1,226 @@ +package controller + +import ( + "testing" + "time" + + "github.com/gadget-inc/skipper/internal/fixture" + "github.com/gadget-inc/skipper/internal/skipper" + "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/types/known/durationpb" + "google.golang.org/protobuf/types/known/timestamppb" + "gotest.tools/v3/assert" +) + +func withHpaPolicy(fn *skipper.Function, hpa *skipper.HpaPolicy) *skipper.Function { + cloned := proto.Clone(fn).(*skipper.Function) + cloned.SetHpa(hpa) + return cloned +} + +func liveHeartbeat(fn *skipper.Function, inFlight uint32) *skipper.Heartbeat { + return skipper.Heartbeat_builder{ + Function: fn, + Timestamp: timestamppb.Now(), + InFlightRequests: new(inFlight), + }.Build() +} + +// readyInstance returns an instance for fn that became ready a long time ago, +// so its CPU metric is always considered eligible for scaling decisions. +func readyInstance(fn *skipper.Function, cpuMilli uint32) *skipper.Instance { + return skipper.Instance_builder{ + Function: fn, + ReadyAt: timestamppb.New(time.Now().Add(-time.Hour)), + CpuUsageMilli: new(cpuMilli), + }.Build() +} + +// TestCalculateDesiredInstancesPerFunctionTolerance verifies that two +// functions sharing scaling targets but with different hpa.tolerance produce +// divergent scale decisions under identical observed load: the tighter +// tolerance scales up sooner. +func TestCalculateDesiredInstancesPerFunctionTolerance(t *testing.T) { + t.Parallel() + + cfg := testConfig() + cfg.HPATolerance = 0.10 // cluster default + + base := fixture.NewFunction(t) + // Run with one instance whose CPU usage is 5% above target. With the cluster + // default tolerance (10%), the discrepancy is within tolerance and there is + // no scaling. With a 1% override, the discrepancy is outside tolerance, so + // the override triggers a scale up. + tightFn := withHpaPolicy(base, skipper.HpaPolicy_builder{Tolerance: new(0.01)}.Build()) + loose := base // omits hpa policy entirely + + scale := tightFn.GetScale() + scale.SetTargetCpuUsageMilli(100) + scale = loose.GetScale() + scale.SetTargetCpuUsageMilli(100) + + tightInstances := []*skipper.Instance{readyInstance(tightFn, 105)} + looseInstances := []*skipper.Instance{readyInstance(loose, 105)} + + tightDesired, _ := calculateDesiredInstancesForMetric(t.Context(), cfg, tightFn, MetricCPU, tightInstances) + looseDesired, _ := calculateDesiredInstancesForMetric(t.Context(), cfg, loose, MetricCPU, looseInstances) + + assert.Equal(t, looseDesired, 1, "loose tolerance (cluster default 10%%) absorbs the 5%% over-target") + assert.Assert(t, tightDesired > looseDesired, + "tight tolerance (1%%) should scale above 1, got %d (vs loose %d)", tightDesired, looseDesired) +} + +// TestRecordRecommendationPerFunctionStabilization verifies that two +// supervisors with different hpa.downscale_stabilization values prune their +// stabilization windows on independent schedules. The supervisor with the +// shorter window forgets older recommendations sooner, which means it can +// scale down sooner after the same load dip. +func TestRecordRecommendationPerFunctionStabilization(t *testing.T) { + t.Parallel() + + cfg := testConfig() + cfg.HPADownscaleStabilization = 5 * time.Minute // cluster default + + base := fixture.NewFunction(t) + shortFn := withHpaPolicy(base, skipper.HpaPolicy_builder{ + DownscaleStabilization: durationpb.New(30 * time.Second), + }.Build()) + + ctrl := New(cfg, nil, nil, nil) + shortSup := &Supervisor{ctrl: ctrl} + shortSup.fn.Store(shortFn) + clusterSup := &Supervisor{ctrl: ctrl} + clusterSup.fn.Store(base) + + // Seed both supervisors with a high recommendation 1 minute ago. + old := time.Now().Add(-time.Minute) + for _, s := range []*Supervisor{shortSup, clusterSup} { + s.stabilizationWindow = []Recommendation{{DesiredInstances: 5, Timestamp: old}} + } + + // Now record a low recommendation. Each supervisor returns the max within + // its own window. The short-window supervisor has already evicted the old + // recommendation; the cluster-default supervisor still sees it. + shortMax := shortSup.recordRecommendation(1) + clusterMax := clusterSup.recordRecommendation(1) + + assert.Equal(t, shortMax.DesiredInstances, uint32(1), + "short stabilization window should forget the 1-minute-old high recommendation") + assert.Equal(t, clusterMax.DesiredInstances, uint32(5), + "cluster default (5m) should still hold the 1-minute-old high recommendation") +} + +// TestCalculateDesiredInstancesPerFunctionInitialReadinessDelay verifies that +// a function with a longer initial-readiness delay excludes a freshly-ready +// pod from its CPU metric, while a function on the cluster default would +// include it. +func TestCalculateDesiredInstancesPerFunctionInitialReadinessDelay(t *testing.T) { + t.Parallel() + + cfg := testConfig() + cfg.HPAInitialReadinessDelay = 30 * time.Second // cluster default + + base := fixture.NewFunction(t) + patientFn := withHpaPolicy(base, skipper.HpaPolicy_builder{ + InitialReadinessDelay: durationpb.New(10 * time.Minute), + }.Build()) + + // A pod that became ready 90 seconds ago. Past cluster default (30s), so + // the cluster-default-only function counts it. The patient function's 10m + // override still excludes it. + freshReady := timestamppb.New(time.Now().Add(-90 * time.Second)) + + patientInstance := skipper.Instance_builder{ + Function: patientFn, + ReadyAt: freshReady, + CpuUsageMilli: proto.Uint32(900), // far above target + }.Build() + patientFn.GetScale().SetTargetCpuUsageMilli(100) + + clusterFn := proto.Clone(base).(*skipper.Function) + clusterInstance := skipper.Instance_builder{ + Function: clusterFn, + ReadyAt: freshReady, + CpuUsageMilli: proto.Uint32(900), + }.Build() + clusterFn.GetScale().SetTargetCpuUsageMilli(100) + + patientDesired, _ := calculateDesiredInstancesForMetric(t.Context(), cfg, patientFn, MetricCPU, []*skipper.Instance{patientInstance}) + clusterDesired, _ := calculateDesiredInstancesForMetric(t.Context(), cfg, clusterFn, MetricCPU, []*skipper.Instance{clusterInstance}) + + assert.Equal(t, patientDesired, 1, + "patient function (10m delay) should exclude the 90s-old pod and stay at 1") + assert.Assert(t, clusterDesired > 1, + "cluster default (30s delay) should include the 90s-old pod and scale up, got %d", clusterDesired) +} + +// TestCalculateDesiredInstancesOmittedHpaPolicy verifies that a function +// omitting the hpa sub-message falls back to cluster flags at every HPA +// decision site -- behavior matches today. +func TestCalculateDesiredInstancesOmittedHpaPolicy(t *testing.T) { + t.Parallel() + + cfg := testConfig() + cfg.HPATolerance = 0.10 + cfg.HPAInitialReadinessDelay = 30 * time.Second + + fn := fixture.NewFunction(t) + assert.Assert(t, fn.GetHpa() == nil, "fixture function must omit hpa policy") + + // Tolerance fallback: 5% over-target with cluster default 10% => no scaling. + fn.GetScale().SetTargetCpuUsageMilli(100) + tolerantInstance := readyInstance(fn, 105) + desired, _ := calculateDesiredInstancesForMetric(t.Context(), cfg, fn, MetricCPU, []*skipper.Instance{tolerantInstance}) + assert.Equal(t, desired, 1, "5%% over-target should be within cluster default tolerance") + + // Initial readiness fallback: a pod ready 90s ago is past the 30s cluster + // default, so its metric is included. + freshReady := skipper.Instance_builder{ + Function: fn, + ReadyAt: timestamppb.New(time.Now().Add(-90 * time.Second)), + CpuUsageMilli: proto.Uint32(500), + }.Build() + desired, _ = calculateDesiredInstancesForMetric(t.Context(), cfg, fn, MetricCPU, []*skipper.Instance{freshReady}) + assert.Assert(t, desired > 1, "90s-old pod should be included in scaling decisions, got %d", desired) +} + +// TestRecordRecommendationOmittedHpaPolicy verifies that a function omitting +// the hpa sub-message uses the cluster default for the stabilization window. +func TestRecordRecommendationOmittedHpaPolicy(t *testing.T) { + t.Parallel() + + cfg := testConfig() + cfg.HPADownscaleStabilization = 5 * time.Minute + + ctrl := New(cfg, nil, nil, nil) + sup := &Supervisor{ctrl: ctrl} + sup.fn.Store(fixture.NewFunction(t)) + + // Recommendation 1 minute ago is well within the 5-minute cluster default, + // so it should still be in the window. + old := time.Now().Add(-time.Minute) + sup.stabilizationWindow = []Recommendation{{DesiredInstances: 7, Timestamp: old}} + + max := sup.recordRecommendation(1) + assert.Equal(t, max.DesiredInstances, uint32(7), + "function without hpa policy should use cluster default (5m) and keep the 1m-old recommendation") +} + +// liveScaleDecision drives calculateDesiredInstances with a fresh heartbeat so +// no heartbeat-timeout reason fires; useful for asserting protectionPeriod and +// other downstream logic. +func liveScaleDecision(t *testing.T, cfg *Config, fn *skipper.Function, inFlight uint32, instances []*skipper.Instance) *skipper.ScaleDecision { + t.Helper() + return calculateDesiredInstances(t.Context(), cfg, fn, liveHeartbeat(fn, inFlight), instances) +} + +// (sanity) the helper builds a fresh decision; ensure it doesn't return a +// heartbeat-timeout decision. +func TestLiveScaleDecisionHelper(t *testing.T) { + t.Parallel() + + cfg := testConfig() + fn := fixture.NewFunction(t) + dec := liveScaleDecision(t, cfg, fn, 0, nil) + assert.Assert(t, dec.GetReason() != skipper.ScaleReason_SCALE_REASON_HEARTBEAT_TIMEOUT) +} diff --git a/internal/controller/supervisor.go b/internal/controller/supervisor.go index f6127f09..edc65621 100644 --- a/internal/controller/supervisor.go +++ b/internal/controller/supervisor.go @@ -219,7 +219,8 @@ func (s *Supervisor) combinedHeartbeat(fn *skipper.Function, instances []*skippe // prunes expired entries, and returns the maximum recommendation within the window. func (s *Supervisor) recordRecommendation(desiredInstances uint32) Recommendation { now := time.Now() - cutoff := now.Add(-s.ctrl.config.HPADownscaleStabilization) + stabilization := s.fn.Load().HPADownscaleStabilization(s.ctrl.config.HPADownscaleStabilization) + cutoff := now.Add(-stabilization) s.stabilizationWindow = append(s.stabilizationWindow, Recommendation{ DesiredInstances: desiredInstances, Timestamp: now, @@ -283,6 +284,15 @@ func (s *Supervisor) converge(ctx context.Context) error { // 1. Calculate scaling decision heartbeat := s.combinedHeartbeat(fn, instances) + // Attach resolved (effective) HPA / heartbeat policy values so subsequent + // log lines reveal whether the cluster default or a tenant override drove + // the decision -- f.GetHpa() shows raw values which may be zero. + ctx = log.With(ctx, + key.HeartbeatTimeout.Slog(fn.HeartbeatTimeout(s.ctrl.config.HeartbeatTimeout)), + key.Tolerance.Slog(fn.HPATolerance(s.ctrl.config.HPATolerance)), + key.DownscaleStabilization.Slog(fn.HPADownscaleStabilization(s.ctrl.config.HPADownscaleStabilization)), + key.InitialReadinessDelay.Slog(fn.HPAInitialReadinessDelay(s.ctrl.config.HPAInitialReadinessDelay)), + ) ctx = telemetry.With(ctx, skipper.FunctionKey.Attr(fn), skipper.HeartbeatKey.Attr(heartbeat)) scalingDecision := calculateDesiredInstances(ctx, s.ctrl.config, fn, heartbeat, instances) @@ -338,8 +348,10 @@ func (s *Supervisor) converge(ctx context.Context) error { // 2. Receive heartbeats from routers (HeartbeatTimeout) - without this, functions with // instances assigned longer than HeartbeatTimeout ago would immediately trigger // heartbeat_timeout scale-to-zero when a new controller starts with empty heartbeat state - // HPADownscaleStabilization stays on the cluster flag here; per-function migration lands in Phase 3. - protectionPeriod := max(s.ctrl.config.HPADownscaleStabilization, fn.HeartbeatTimeout(s.ctrl.config.HeartbeatTimeout)) + protectionPeriod := max( + fn.HPADownscaleStabilization(s.ctrl.config.HPADownscaleStabilization), + fn.HeartbeatTimeout(s.ctrl.config.HeartbeatTimeout), + ) if time.Since(s.ctrl.StartedAt()) < protectionPeriod { log.Debug(ctx, "skipping scale down because controller hasn't been running long enough", slog.Time("started_at", s.ctrl.StartedAt())) return nil @@ -725,8 +737,10 @@ func scaleToZeroEvent(reason skipper.ScaleReason) (skipper.EventType, string) { } // calculateDesiredInstancesForMetric computes desired instances based on a single metric -func calculateDesiredInstancesForMetric(_ context.Context, cfg *Config, metric Metric, instances []*skipper.Instance) (int, float64) { +func calculateDesiredInstancesForMetric(_ context.Context, cfg *Config, fn *skipper.Function, metric Metric, instances []*skipper.Instance) (int, float64) { currentInstances := len(instances) + tolerance := fn.HPATolerance(cfg.HPATolerance) + initialReadinessDelay := fn.HPAInitialReadinessDelay(cfg.HPAInitialReadinessDelay) var instancesWithMetrics []*skipper.Instance var instancesWithoutMetrics []*skipper.Instance @@ -741,7 +755,7 @@ func calculateDesiredInstancesForMetric(_ context.Context, cfg *Config, metric M return currentInstances, 0 } - if metric == MetricCPU && (!instance.HasReadyAt() || time.Since(instance.GetReadyAt().AsTime()) <= cfg.HPAInitialReadinessDelay) { + if metric == MetricCPU && (!instance.HasReadyAt() || time.Since(instance.GetReadyAt().AsTime()) <= initialReadinessDelay) { // ignore CPU metrics for pods that have been ready for less than the initial readiness delay instancesWithoutMetrics = append(instancesWithoutMetrics, instance) continue @@ -782,7 +796,7 @@ func calculateDesiredInstancesForMetric(_ context.Context, cfg *Config, metric M usageDiscrepancy := math.Abs(1.0 - usageRatio) desiredInstances := int(math.Ceil(float64(currentInstances) * usageRatio)) - if usageDiscrepancy <= cfg.HPATolerance+1e-10 { // add a small epsilon to avoid floating point errors + if usageDiscrepancy <= tolerance+1e-10 { // add a small epsilon to avoid floating point errors // the average usage is within tolerance of the target utilization, so we should not scale return currentInstances, 0 } @@ -866,7 +880,7 @@ func calculateDesiredInstances(ctx context.Context, cfg *Config, fn *skipper.Fun } if scale.GetTargetCpuUsageMilli() > 0 { - desiredInstances, averageUsage := calculateDesiredInstancesForMetric(ctx, cfg, MetricCPU, instances) + desiredInstances, averageUsage := calculateDesiredInstancesForMetric(ctx, cfg, fn, MetricCPU, instances) metric := &skipper.ScaleMetric{} metric.SetName("cpu") metric.SetValue(averageUsage) @@ -878,7 +892,7 @@ func calculateDesiredInstances(ctx context.Context, cfg *Config, fn *skipper.Fun } if scale.GetTargetMemoryUsageMib() > 0 { - desiredInstances, averageUsage := calculateDesiredInstancesForMetric(ctx, cfg, MetricMemory, instances) + desiredInstances, averageUsage := calculateDesiredInstancesForMetric(ctx, cfg, fn, MetricMemory, instances) metric := &skipper.ScaleMetric{} metric.SetName("memory") metric.SetValue(averageUsage) diff --git a/internal/controller/supervisor_test.go b/internal/controller/supervisor_test.go index 29fe11e2..aaef93ef 100644 --- a/internal/controller/supervisor_test.go +++ b/internal/controller/supervisor_test.go @@ -1191,7 +1191,7 @@ func TestCalculateDesiredInstancesForMetric(t *testing.T) { } } - instances, _ := calculateDesiredInstancesForMetric(t.Context(), cfg, tc.metricName, tc.podMetrics) + instances, _ := calculateDesiredInstancesForMetric(t.Context(), cfg, tc.podMetrics[0].GetFunction(), tc.metricName, tc.podMetrics) assert.Assert(t, instances == tc.expectedInstances) }) } From 1c5ae0926bb59878c1c1bbb1c0021f4e07eb8fea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Scott=20C=C3=B4t=C3=A9?= Date: Tue, 5 May 2026 01:16:25 -0400 Subject: [PATCH 04/17] Apply per-function proxy retry policy in router 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. --- internal/router/per_function_proxy_test.go | 152 +++++++++++++++++++++ internal/router/router.go | 29 +++- internal/router/router_test.go | 8 +- 3 files changed, 178 insertions(+), 11 deletions(-) create mode 100644 internal/router/per_function_proxy_test.go diff --git a/internal/router/per_function_proxy_test.go b/internal/router/per_function_proxy_test.go new file mode 100644 index 00000000..1481c09b --- /dev/null +++ b/internal/router/per_function_proxy_test.go @@ -0,0 +1,152 @@ +package router + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + "time" + + "github.com/gadget-inc/skipper/internal/fixture" + "github.com/gadget-inc/skipper/internal/skipper" + "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/types/known/durationpb" + "gotest.tools/v3/assert" +) + +func withProxyPolicy(fn *skipper.Function, proxyPolicy *skipper.ProxyPolicy) *skipper.Function { + cloned := proto.Clone(fn).(*skipper.Function) + cloned.SetProxy(proxyPolicy) + return cloned +} + +// failingControllerClient counts Instance calls and always returns a temporary +// error so RoundTrip exhausts every allowed attempt before returning. +type failingControllerClient struct { + *fixture.MockControllerClient + calls atomic.Int32 +} + +func newFailingClient(t *testing.T) *failingControllerClient { + c := &failingControllerClient{MockControllerClient: fixture.NewMockControllerClient(t)} + c.HandleInstance(func(_ context.Context, _ *skipper.Function, _ ...string) (*skipper.Instance, error) { + c.calls.Add(1) + return nil, errors.New("temporary error") + }) + return c +} + +func runRoundTripExhaustion(t *testing.T, cfg *Config, fn *skipper.Function) (int, error) { + t.Helper() + mcc := newFailingClient(t) + router := New(cfg, mcc.MockControllerClient) + + req := httptest.NewRequest(http.MethodGet, "/", nil) + fn.SetHeader(req) + req = req.WithContext(withFunction(t.Context(), fn)) + + _, err := router.RoundTrip(req) + return int(mcc.calls.Load()), err +} + +// TestRoundTripPerFunctionMaxAttemptsCapsBelowClusterDefault verifies that a +// request whose Function header sets proxy.max_attempts = 1 stops after one +// attempt, regardless of --max-round-trip-attempts. +func TestRoundTripPerFunctionMaxAttemptsCapsBelowClusterDefault(t *testing.T) { + t.Parallel() + + cfg := testConfig() + cfg.MaxRoundTripAttempts = 6 + cfg.RoundTripRetryMinTimeout = time.Microsecond + cfg.RoundTripRetryMaxTimeout = time.Millisecond + + fn := withProxyPolicy(fixture.NewFunction(t), skipper.ProxyPolicy_builder{ + MaxAttempts: new(uint32(1)), + }.Build()) + + calls, err := runRoundTripExhaustion(t, cfg, fn) + assert.Assert(t, err != nil, "expected error after exhausting attempts") + assert.Equal(t, calls, 1, "proxy.max_attempts=1 should stop after 1 attempt") +} + +// TestRoundTripPerFunctionMaxAttemptsExceedsClusterDefault verifies that a +// request whose Function header sets proxy.max_attempts = 10 attempts up to +// 10 times even when --max-round-trip-attempts is 6. +func TestRoundTripPerFunctionMaxAttemptsExceedsClusterDefault(t *testing.T) { + t.Parallel() + + cfg := testConfig() + cfg.MaxRoundTripAttempts = 6 + cfg.RoundTripRetryMinTimeout = time.Microsecond + cfg.RoundTripRetryMaxTimeout = time.Millisecond + + fn := withProxyPolicy(fixture.NewFunction(t), skipper.ProxyPolicy_builder{ + MaxAttempts: new(uint32(10)), + }.Build()) + + calls, err := runRoundTripExhaustion(t, cfg, fn) + assert.Assert(t, err != nil, "expected error after exhausting attempts") + assert.Equal(t, calls, 10, "proxy.max_attempts=10 should run 10 attempts even when cluster default is 6") +} + +// TestRoundTripOmittedProxyPolicyFallsBackToClusterDefault verifies that a +// request omitting the proxy sub-message uses --max-round-trip-attempts. +func TestRoundTripOmittedProxyPolicyFallsBackToClusterDefault(t *testing.T) { + t.Parallel() + + cfg := testConfig() + cfg.MaxRoundTripAttempts = 4 + cfg.RoundTripRetryMinTimeout = time.Microsecond + cfg.RoundTripRetryMaxTimeout = time.Millisecond + + fn := fixture.NewFunction(t) + assert.Assert(t, fn.GetProxy() == nil, "fixture function must omit proxy policy") + + calls, err := runRoundTripExhaustion(t, cfg, fn) + assert.Assert(t, err != nil, "expected error after exhausting attempts") + assert.Equal(t, calls, 4, "omitted proxy policy should fall back to cluster default of 4") +} + +// TestCalculateBackoffPerFunctionBounds verifies that per-function +// proxy.retry_min_backoff / proxy.retry_max_backoff drive the curve when +// passed in -- backoff stays within the function's bounds. +func TestCalculateBackoffPerFunctionBounds(t *testing.T) { + t.Parallel() + + fnMinBackoff := 50 * time.Millisecond + fnMaxBackoff := 200 * time.Millisecond + + for range 100 { + got := calculateBackoff(2, fnMinBackoff, fnMaxBackoff) + assert.Assert(t, got >= fnMinBackoff*2, "attempt 2 backoff %s below per-function min*2 = %s", got, fnMinBackoff*2) + assert.Assert(t, got <= fnMaxBackoff, "attempt 2 backoff %s above per-function max = %s", got, fnMaxBackoff) + } +} + +// TestHeartbeatStateUpdateFunctionOnPolicyChange exercises the pointer-identity +// swap path on a policy change. A later request whose Function header carries a +// different proxy policy goes through the LRU header cache as a distinct +// pointer, and updateFunction stores it. +func TestHeartbeatStateUpdateFunctionOnPolicyChange(t *testing.T) { + t.Parallel() + + base := fixture.NewFunction(t) + state := newHeartbeatState(base) + + withRetries := withProxyPolicy(base, skipper.ProxyPolicy_builder{ + MaxAttempts: new(uint32(10)), + RetryMinBackoff: durationpb.New(50 * time.Millisecond), + RetryMaxBackoff: durationpb.New(time.Second), + }.Build()) + assert.Equal(t, base.Hash(), withRetries.Hash()) + assert.Assert(t, !proto.Equal(base, withRetries)) + + state.updateFunction(withRetries) + + hb := state.toProto() + assert.Assert(t, proto.Equal(hb.GetFunction(), withRetries), + "updateFunction should swap to the request that carries the new proxy policy") + assert.Equal(t, hb.GetFunction().MaxRoundTripAttempts(0), uint32(10)) +} diff --git a/internal/router/router.go b/internal/router/router.go index ba22d286..bf794f40 100644 --- a/internal/router/router.go +++ b/internal/router/router.go @@ -195,25 +195,37 @@ func (r *Router) RoundTrip(req *http.Request) (*http.Response, error) { defer func() { req.Body = originalBody }() } + maxAttempts := fn.MaxRoundTripAttempts(uint32(r.config.MaxRoundTripAttempts)) + minBackoff := fn.RetryMinBackoff(r.config.RoundTripRetryMinTimeout) + maxBackoff := fn.RetryMaxBackoff(r.config.RoundTripRetryMaxTimeout) + var excludedInstanceNames []string getInstanceDuration := time.Duration(0) attempt := 0 for { attempt++ - if attempt > r.config.MaxRoundTripAttempts { - return nil, fmt.Errorf("failed to proxy request after %d attempts", r.config.MaxRoundTripAttempts) + if attempt > int(maxAttempts) { + return nil, fmt.Errorf("failed to proxy request after %d attempts", maxAttempts) } if attempt > 1 { select { case <-req.Context().Done(): return nil, req.Context().Err() - case <-time.After(r.calculateBackoff(attempt)): + case <-time.After(calculateBackoff(attempt, minBackoff, maxBackoff)): } } - ctx := telemetry.With(req.Context(), key.Attempt.Attr(attempt), key.ExcludeInstanceNames.Attr(excludedInstanceNames)) + // Attach the resolved (effective) retry policy to the per-attempt log + // context so a reader can tell whether the cluster default or a tenant + // override drove the attempt cap and backoff curve. + ctx := log.With(req.Context(), + key.MaxAttempts.Slog(maxAttempts), + key.RetryMinBackoff.Slog(minBackoff), + key.RetryMaxBackoff.Slog(maxBackoff), + ) + ctx = telemetry.With(ctx, key.Attempt.Attr(attempt), key.ExcludeInstanceNames.Attr(excludedInstanceNames)) getInstanceStart := time.Now() instance, err := r.ctrl.Instance(ctx, fn, excludedInstanceNames...) @@ -340,9 +352,12 @@ func (r *Router) releaseInstance(inst *skipper.Instance) { }() } -func (r *Router) calculateBackoff(attempt int) time.Duration { - minTimeout := float64(r.config.RoundTripRetryMinTimeout) - maxTimeout := float64(r.config.RoundTripRetryMaxTimeout) +// calculateBackoff returns a randomized exponential backoff between +// minBackoff and maxBackoff for the given attempt. Bounds come from the +// resolver in RoundTrip, so per-function overrides drive this curve. +func calculateBackoff(attempt int, minBackoff, maxBackoff time.Duration) time.Duration { + minTimeout := float64(minBackoff) + maxTimeout := float64(maxBackoff) factor := 1 + rand.Float64() // randomize the factor between 1 and 2 to add jitter return time.Duration(min(factor*minTimeout*math.Pow(2, float64(attempt)), maxTimeout)) } diff --git a/internal/router/router_test.go b/internal/router/router_test.go index 9ee2869b..4287a730 100644 --- a/internal/router/router_test.go +++ b/internal/router/router_test.go @@ -914,11 +914,11 @@ func TestCalculateBackoff(t *testing.T) { cfg := testConfig() cfg.RoundTripRetryMinTimeout = tc.minTimeout cfg.RoundTripRetryMaxTimeout = tc.maxTimeout - router := New(cfg, fixture.NewMockControllerClient(t)) + _ = New(cfg, fixture.NewMockControllerClient(t)) // Run multiple times due to randomness for range 100 { - backoff := router.calculateBackoff(tc.attempt) + backoff := calculateBackoff(tc.attempt, cfg.RoundTripRetryMinTimeout, cfg.RoundTripRetryMaxTimeout) assert.Assert(t, backoff >= tc.checkMin, "attempt %d: backoff %v < min %v", tc.attempt, backoff, tc.checkMin) assert.Assert(t, backoff <= tc.checkMax, "attempt %d: backoff %v > max %v", tc.attempt, backoff, tc.checkMax) } @@ -2175,7 +2175,7 @@ func TestBackoffDoesNotOverflowAtHighAttempts(t *testing.T) { cfg := testConfig() cfg.RoundTripRetryMinTimeout = 100 * time.Millisecond cfg.RoundTripRetryMaxTimeout = 5 * time.Second - router := New(cfg, fixture.NewMockControllerClient(t)) + _ = New(cfg, fixture.NewMockControllerClient(t)) // Test various high attempt numbers that could cause overflow testAttempts := []int{10, 50, 100, 1000, 10000} @@ -2183,7 +2183,7 @@ func TestBackoffDoesNotOverflowAtHighAttempts(t *testing.T) { for _, attempt := range testAttempts { // Run multiple times due to randomness in backoff calculation for range 10 { - backoff := router.calculateBackoff(attempt) + backoff := calculateBackoff(attempt, cfg.RoundTripRetryMinTimeout, cfg.RoundTripRetryMaxTimeout) // Verify backoff is within valid range assert.Assert(t, backoff >= 0, "attempt %d: backoff should not be negative: %v", attempt, backoff) From 92cefbd7d133951e0cb0e205178c3591c280363e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Scott=20C=C3=B4t=C3=A9?= Date: Tue, 5 May 2026 01:22:15 -0400 Subject: [PATCH 05/17] Add token-ttl flag and per-function lifecycle policy 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. --- internal/cmd/testdata/help_controller.golden | 1 + internal/controller/config.go | 1 + .../controller/per_function_lifecycle_test.go | 212 ++++++++++++++++++ internal/controller/pod.go | 8 +- internal/controller/supervisor.go | 11 +- 5 files changed, 226 insertions(+), 7 deletions(-) create mode 100644 internal/controller/per_function_lifecycle_test.go diff --git a/internal/cmd/testdata/help_controller.golden b/internal/cmd/testdata/help_controller.golden index ed516774..75b274c4 100644 --- a/internal/cmd/testdata/help_controller.golden +++ b/internal/cmd/testdata/help_controller.golden @@ -41,5 +41,6 @@ Flags: --telemetry-prometheus-port int The port for the Prometheus metrics endpoint. (env SKIPPER_TELEMETRY_PROMETHEUS_PORT) (default 9090) --telemetry-shutdown-timeout duration The timeout for shutting down the telemetry. (env SKIPPER_TELEMETRY_SHUTDOWN_TIMEOUT) (default 5s) --telemetry-trace Whether to enable tracing if telemetry is enabled. (env SKIPPER_TELEMETRY_TRACE) (default true) + --token-ttl duration The lifetime of PASETO tokens issued to assigned pods. (env SKIPPER_TOKEN_TTL) (default 168h0m0s) --web-port int The port the web UI listens on. (env SKIPPER_WEB_PORT) (default 8080) --web-template-dir string When set, reload templates from this directory on each request (dev mode). (env SKIPPER_WEB_TEMPLATE_DIR) diff --git a/internal/controller/config.go b/internal/controller/config.go index 57f7f7a8..1226e80a 100644 --- a/internal/controller/config.go +++ b/internal/controller/config.go @@ -30,6 +30,7 @@ type Config struct { FunctionNamespaces []string `flag:"function-namespaces" description:"The namespaces where functions can be invoked." required:"true"` FunctionAssignPath string `flag:"function-assign-path" description:"The path used to assign a function to a pod." default:"/__skipper/assign"` FunctionAssignTimeout time.Duration `flag:"function-assign-timeout" description:"The timeout for assigning a function to a pod." default:"30s"` + TokenTTL time.Duration `flag:"token-ttl" description:"The lifetime of PASETO tokens issued to assigned pods." default:"168h"` MaxConcurrentStaleReplacements int `flag:"max-concurrent-stale-replacements" description:"Maximum number of stale instances that can be replaced concurrently." default:"10"` SkipForbiddenNamespaces bool `flag:"skip-forbidden-namespaces" description:"Whether to skip function namespaces that the service account does not have access to." default:"false"` WebPort int `flag:"web-port" description:"The port the web UI listens on." default:"8080"` diff --git a/internal/controller/per_function_lifecycle_test.go b/internal/controller/per_function_lifecycle_test.go new file mode 100644 index 00000000..563a2d1a --- /dev/null +++ b/internal/controller/per_function_lifecycle_test.go @@ -0,0 +1,212 @@ +package controller + +import ( + "context" + "net/http" + "testing" + "time" + + "aidanwoods.dev/go-paseto" + "github.com/gadget-inc/skipper/internal/fixture" + "github.com/gadget-inc/skipper/internal/key" + "github.com/gadget-inc/skipper/internal/skipper" + "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/types/known/durationpb" + "google.golang.org/protobuf/types/known/timestamppb" + "gotest.tools/v3/assert" + "k8s.io/client-go/kubernetes/fake" +) + +func withLifecyclePolicy(fn *skipper.Function, lifecyclePolicy *skipper.LifecyclePolicy) *skipper.Function { + cloned := proto.Clone(fn).(*skipper.Function) + cloned.SetLifecycle(lifecyclePolicy) + return cloned +} + +// capturedAssign records the assign request's PASETO expiration so tests can +// assert that the resolved lifecycle.token_ttl drove the claim. +type capturedAssign struct { + tokenExpiration time.Time +} + +func captureAssignHandler(t *testing.T, captured *capturedAssign) http.Handler { + t.Helper() + return http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { + assert.Equal(t, req.Method, http.MethodPost) + assert.Equal(t, req.URL.Path, "/__skipper/assign") + + parser := paseto.NewParserForValidNow() + token, err := parser.ParseV2Public(fixture.ControllerPasetoPublicKey, req.Header.Get(key.Token.Header)) + assert.NilError(t, err) + captured.tokenExpiration, err = token.GetExpiration() + assert.NilError(t, err) + + rw.WriteHeader(http.StatusOK) + }) +} + +// TestAssignPodPerFunctionTokenTTL verifies that a function with a per-function +// lifecycle.token_ttl receives a PASETO token whose exp claim reflects the +// shorter value, while a function omitting the policy uses the cluster +// default. +func TestAssignPodPerFunctionTokenTTL(t *testing.T) { + t.Parallel() + + cfg := testConfig() + cfg.TokenTTL = 7 * 24 * time.Hour // cluster default + + cases := []struct { + name string + fn func(*skipper.Function) *skipper.Function + expectedTTL time.Duration + }{ + { + name: "uses cluster default when lifecycle is omitted", + fn: func(f *skipper.Function) *skipper.Function { return f }, + expectedTTL: cfg.TokenTTL, + }, + { + name: "uses per-function override when set", + fn: func(f *skipper.Function) *skipper.Function { + return withLifecyclePolicy(f, skipper.LifecyclePolicy_builder{ + TokenTtl: durationpb.New(time.Hour), + }.Build()) + }, + expectedTTL: time.Hour, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + fn := tc.fn(fixture.NewFunction(t)) + fakeKubernetes := fake.NewClientset(fixture.NewControllerPod()) + captured := &capturedAssign{} + fakeKubernetes.Tracker().Add(fixture.NewAvailablePod(t, fn, captureAssignHandler(t, captured))) + + ctrl := New(cfg, nil, fakeKubernetes, nil) + ctx, cancel := context.WithTimeout(t.Context(), 3*time.Second) + defer cancel() + + err := ctrl.startInformers(ctx) + assert.NilError(t, err) + + before := time.Now() + _, err = ctrl.assignPod(ctx, fn) + assert.NilError(t, err) + + actualTTL := captured.tokenExpiration.Sub(before) + // Allow a small wall-clock tolerance because token issuance happens + // some moments after `before`. + lower := tc.expectedTTL - 5*time.Second + upper := tc.expectedTTL + time.Second + assert.Assert(t, actualTTL >= lower && actualTTL <= upper, + "token TTL %s should be ~%s (range %s..%s)", actualTTL, tc.expectedTTL, lower, upper) + }) + } +} + +// TestAssignPodPerFunctionAssignTimeoutOverrideSurvivesSlowHandler verifies +// that a function with a longer lifecycle.assign_timeout still completes +// when the assign handler exceeds the cluster default. The cluster-default +// case fails on the same slow handler. +func TestAssignPodPerFunctionAssignTimeoutOverrideSurvivesSlowHandler(t *testing.T) { + t.Parallel() + + cfg := testConfig() + cfg.FunctionAssignTimeout = 50 * time.Millisecond // cluster default + + slowHandler := http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { + time.Sleep(200 * time.Millisecond) // longer than cluster default, shorter than override + rw.WriteHeader(http.StatusOK) + }) + + cases := []struct { + name string + fn func(*skipper.Function) *skipper.Function + expectErr bool + }{ + { + name: "cluster default times out on slow handler", + fn: func(f *skipper.Function) *skipper.Function { return f }, + expectErr: true, + }, + { + name: "per-function override succeeds on slow handler", + fn: func(f *skipper.Function) *skipper.Function { + return withLifecyclePolicy(f, skipper.LifecyclePolicy_builder{ + AssignTimeout: durationpb.New(2 * time.Second), + }.Build()) + }, + expectErr: false, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + fn := tc.fn(fixture.NewFunction(t)) + fakeKubernetes := fake.NewClientset(fixture.NewControllerPod()) + fakeKubernetes.Tracker().Add(fixture.NewAvailablePod(t, fn, slowHandler)) + + ctrl := New(cfg, nil, fakeKubernetes, nil) + ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) + defer cancel() + + err := ctrl.startInformers(ctx) + assert.NilError(t, err) + + _, err = ctrl.assignPod(ctx, fn) + if tc.expectErr { + assert.ErrorIs(t, err, context.DeadlineExceeded) + } else { + assert.NilError(t, err) + } + }) + } +} + +// TestCleanupStuckInstancesPerFunctionAssignTimeout verifies that a stuck +// instance is cleaned up against the function's effective assign timeout. +// Two functions share a controller config (--function-assign-timeout = 30s) +// but one overrides lifecycle.assign_timeout = 1ms. The instance for the +// override is cleaned up after a few milliseconds; the cluster-default one is +// not. +func TestCleanupStuckInstancesPerFunctionAssignTimeout(t *testing.T) { + t.Parallel() + + cfg := testConfig() + cfg.FunctionAssignTimeout = 30 * time.Second + + clusterFn := fixture.NewFunction(t) + overrideFn := withLifecyclePolicy(fixture.NewFunction(t), skipper.LifecyclePolicy_builder{ + AssignTimeout: durationpb.New(time.Millisecond), + }.Build()) + + clusterPod := fixture.NewAssignedPod(t, clusterFn, nil) + overridePod := fixture.NewAssignedPod(t, overrideFn, nil) + + staleAssignedAt := timestamppb.New(time.Now().Add(-100 * time.Millisecond)) + clusterInstance := skipper.Instance_builder{ + Function: clusterFn, + Name: new(clusterPod.Name), + AssignedAt: staleAssignedAt, + }.Build() + overrideInstance := skipper.Instance_builder{ + Function: overrideFn, + Name: new(overridePod.Name), + AssignedAt: staleAssignedAt, + }.Build() + + fakeKubernetes := fake.NewClientset(fixture.NewControllerPod(), clusterPod, overridePod) + ctrl := New(cfg, nil, fakeKubernetes, nil) + sup := &Supervisor{ctrl: ctrl} + + remaining := sup.cleanupStuckInstances(t.Context(), []*skipper.Instance{clusterInstance, overrideInstance}) + + assert.Equal(t, len(remaining), 1, "override instance should be cleaned up; cluster instance retained") + assert.Equal(t, remaining[0].GetName(), clusterPod.Name, + "only the cluster-default instance should remain") +} diff --git a/internal/controller/pod.go b/internal/controller/pod.go index 6f46da18..7cfb64c3 100644 --- a/internal/controller/pod.go +++ b/internal/controller/pod.go @@ -95,8 +95,10 @@ GET_UNASSIGNED_POD: } }() + assignTimeout := fn.AssignTimeout(ctrl.config.FunctionAssignTimeout) + assignURL := "http://" + net.JoinHostPort(assignedPod.Status.PodIP, port) + ctrl.config.FunctionAssignPath - assignCtx, cancel := context.WithTimeout(ctx, ctrl.config.FunctionAssignTimeout) + assignCtx, cancel := context.WithTimeout(ctx, assignTimeout) defer cancel() now := time.Now() @@ -104,7 +106,7 @@ GET_UNASSIGNED_POD: token.SetSubject(fn.GetTenant()) token.SetIssuedAt(now) token.SetNotBefore(now) - token.SetExpiration(now.Add(7 * 24 * time.Hour)) + token.SetExpiration(now.Add(fn.TokenTTL(ctrl.config.TokenTTL))) var req *http.Request req, err = http.NewRequestWithContext(assignCtx, http.MethodPost, assignURL, nil) @@ -143,7 +145,7 @@ GET_UNASSIGNED_POD: assignedPod.Annotations[key.ReadyAt.Label] = readyAtStr go func() { - asyncCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), ctrl.config.FunctionAssignTimeout) + asyncCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), assignTimeout) defer cancel() patches := []byte(`[{ "op": "add", "path": "` + key.ReadyAt.PatchAnnotation + `", "value": "` + readyAtStr + `" }]`) diff --git a/internal/controller/supervisor.go b/internal/controller/supervisor.go index edc65621..5173080a 100644 --- a/internal/controller/supervisor.go +++ b/internal/controller/supervisor.go @@ -541,12 +541,15 @@ func (s *Supervisor) scaleWithoutLock(ctx context.Context, fn *skipper.Function, } // cleanupStuckInstances terminates instances that are stuck in the -// assigned state (not ready) for longer than FunctionAssignTimeout*2. -// This is a cheap operation (just deletes) and should be called before -// scaling execution to remove broken pods from consideration. +// assigned state (not ready) for longer than the effective assign-timeout +// times two. The effective timeout is per-function (lifecycle.assign_timeout) +// with the cluster flag as the fallback. This is a cheap operation (just +// deletes) and should be called before scaling execution to remove broken +// pods from consideration. func (s *Supervisor) cleanupStuckInstances(ctx context.Context, instances []*skipper.Instance) []*skipper.Instance { return slices.DeleteFunc(instances, func(instance *skipper.Instance) bool { - if !instance.HasReadyAt() && time.Since(instance.GetAssignedAt().AsTime()) > s.ctrl.config.FunctionAssignTimeout*2 { + assignTimeout := instance.GetFunction().AssignTimeout(s.ctrl.config.FunctionAssignTimeout) + if !instance.HasReadyAt() && time.Since(instance.GetAssignedAt().AsTime()) > assignTimeout*2 { ctx := log.With(ctx, skipper.InstanceKey.Slog(instance)) log.Warn(ctx, "terminating instance stuck in assigned state") err := s.ctrl.deletePod(ctx, instance.GetFunction().GetNamespace(), instance.GetName(), metav1.DeleteOptions{}) From ffe677cb306e0094c9d76b3bf9dfeb71fbd0a61b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Scott=20C=C3=B4t=C3=A9?= Date: Tue, 5 May 2026 01:31:19 -0400 Subject: [PATCH 06/17] Resolve adjusted HPA tolerance through per-function policy 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. --- internal/controller/per_function_hpa_test.go | 42 ++++++++++++++++++++ internal/controller/supervisor.go | 2 +- 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/internal/controller/per_function_hpa_test.go b/internal/controller/per_function_hpa_test.go index 23b2242b..ab84c946 100644 --- a/internal/controller/per_function_hpa_test.go +++ b/internal/controller/per_function_hpa_test.go @@ -70,6 +70,48 @@ func TestCalculateDesiredInstancesPerFunctionTolerance(t *testing.T) { "tight tolerance (1%%) should scale above 1, got %d (vs loose %d)", tightDesired, looseDesired) } +// TestCalculateDesiredInstancesForMetricAdjustedTolerancePerFunction +// guards the adjusted-tolerance branch in calculateDesiredInstancesForMetric: +// when one or more instances are missing their CPU metric, the function falls +// into the adjusted-ratio path. The tolerance check inside that path must +// resolve through the per-function value, not the cluster flag. +// +// Setup: 4 instances, 2 with low CPU samples (25m each, target 100m), 2 +// missing. The basic discrepancy (0.75) exceeds even the loose cluster +// tolerance, so both flows enter the adjusted-ratio branch. The adjusted +// ratio (250/400) = 0.625; adjusted discrepancy = 0.375. With tight tolerance +// 0.01 the helper falls through to the post-adjust scale-down (ceil(4*0.625) +// = 3); with cluster tolerance 0.40 the adjusted-tolerance check early-returns +// the current count (4). +func TestCalculateDesiredInstancesForMetricAdjustedTolerancePerFunction(t *testing.T) { + t.Parallel() + + cfg := testConfig() + cfg.HPATolerance = 0.40 // cluster default: loose enough to absorb the adjusted discrepancy + + base := fixture.NewFunction(t) + base.GetScale().SetTargetCpuUsageMilli(100) + tightFn := withHpaPolicy(base, skipper.HpaPolicy_builder{Tolerance: new(0.01)}.Build()) + tightFn.GetScale().SetTargetCpuUsageMilli(100) + + build := func(fn *skipper.Function) []*skipper.Instance { + return []*skipper.Instance{ + readyInstance(fn, 25), + readyInstance(fn, 25), + skipper.Instance_builder{Function: fn, ReadyAt: timestamppb.New(time.Now().Add(-time.Hour))}.Build(), + skipper.Instance_builder{Function: fn, ReadyAt: timestamppb.New(time.Now().Add(-time.Hour))}.Build(), + } + } + + tightDesired, _ := calculateDesiredInstancesForMetric(t.Context(), cfg, tightFn, MetricCPU, build(tightFn)) + clusterDesired, _ := calculateDesiredInstancesForMetric(t.Context(), cfg, base, MetricCPU, build(base)) + + assert.Equal(t, clusterDesired, 4, + "cluster default 0.40 should swallow the adjusted discrepancy 0.375 and stay at currentInstances=4") + assert.Equal(t, tightDesired, 3, + "tight tolerance 0.01 should fall through the adjusted-tolerance check and scale down to ceil(4*0.625)=3") +} + // TestRecordRecommendationPerFunctionStabilization verifies that two // supervisors with different hpa.downscale_stabilization values prune their // stabilization windows on independent schedules. The supervisor with the diff --git a/internal/controller/supervisor.go b/internal/controller/supervisor.go index 5173080a..938de2b2 100644 --- a/internal/controller/supervisor.go +++ b/internal/controller/supervisor.go @@ -819,7 +819,7 @@ func calculateDesiredInstancesForMetric(_ context.Context, cfg *Config, fn *skip if (adjustedUsageRatio > 1.0 && usageRatio < 1.0) || (adjustedUsageRatio < 1.0 && usageRatio > 1.0) || - math.Abs(1.0-adjustedUsageRatio) <= cfg.HPATolerance+1e-10 { + math.Abs(1.0-adjustedUsageRatio) <= tolerance+1e-10 { // the adjusted usage ratio is the opposite of the original // usage ratio, or the adjusted usage ratio is within // tolerance of the target utilization. either way, we From d3bd4add88f6e2a026715c904c1081005a20e76d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Scott=20C=C3=B4t=C3=A9?= Date: Tue, 5 May 2026 01:48:47 -0400 Subject: [PATCH 07/17] Keep heartbeat half of protection period on the cluster flag 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. --- .../controller/per_function_heartbeat_test.go | 63 +++++++++++++++++++ internal/controller/supervisor.go | 9 ++- 2 files changed, 69 insertions(+), 3 deletions(-) diff --git a/internal/controller/per_function_heartbeat_test.go b/internal/controller/per_function_heartbeat_test.go index 534ab667..11a8896a 100644 --- a/internal/controller/per_function_heartbeat_test.go +++ b/internal/controller/per_function_heartbeat_test.go @@ -1,15 +1,19 @@ package controller import ( + "context" "testing" "time" "github.com/gadget-inc/skipper/internal/fixture" + "github.com/gadget-inc/skipper/internal/key" "github.com/gadget-inc/skipper/internal/skipper" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/types/known/durationpb" "google.golang.org/protobuf/types/known/timestamppb" "gotest.tools/v3/assert" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/fake" ) // silencedHeartbeat returns a heartbeat for fn whose timestamp is in the past @@ -119,3 +123,62 @@ func TestSupervisorUpdateFunctionPicksUpHeartbeatTimeout(t *testing.T) { assert.Assert(t, afterUpdate.GetReason() != skipper.ScaleReason_SCALE_REASON_HEARTBEAT_TIMEOUT, "requestB with 5m timeout should not scale to zero after 30s idle, got %s", afterUpdate.GetReason()) } + +// TestProtectionPeriodIgnoresPerFunctionHeartbeatTimeout guards a +// fleet-startup invariant. The protection period at supervisor.go's converge +// scale-down branch exists in part to give routers time to send heartbeats +// to a freshly-started controller. Router heartbeat propagation is governed +// by the cluster -- the router's heartbeat interval is not per-function -- +// so the heartbeat half of the protection-period max must read the cluster +// flag, not the per-function resolver. A tenant setting heartbeat.timeout = +// 1s alongside a tight stabilization window must not be able to shrink the +// protection period below the cluster's heartbeat-propagation budget, +// because doing so would let a controller that has been up for a few +// seconds spuriously scale-to-zero before any router has reported in. +func TestProtectionPeriodIgnoresPerFunctionHeartbeatTimeout(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(t.Context(), time.Second) + defer cancel() + + cfg := testConfig() + cfg.HPADownscaleStabilization = 90 * time.Second + cfg.HeartbeatTimeout = 90 * time.Second // cluster default for router-heartbeat propagation + + // Tenant overrides both halves to a tight 1s window and the controller + // has only been running for 5 seconds. + base := fixture.NewFunction(t) + base.GetScale().SetTargetCpuUsageMilli(0) + base.GetScale().SetTargetMemoryUsageMib(0) + base.GetScale().SetTargetInFlightRequests(0) + fn := proto.Clone(base).(*skipper.Function) + fn.SetHeartbeat(skipper.HeartbeatPolicy_builder{Timeout: durationpb.New(time.Second)}.Build()) + fn.SetHpa(skipper.HpaPolicy_builder{DownscaleStabilization: durationpb.New(time.Second)}.Build()) + + fakeKubernetes := fake.NewClientset(fixture.NewControllerPod()) + fakeKubernetes.Tracker().Add(fixture.CurrentReplicaSet(t, fn)) + pod := fixture.NewAssignedPod(t, fn, nil) + staleAt := time.Now().Add(-2 * time.Hour).UTC().Format(time.RFC3339) + pod.Annotations[key.AssignedAt.Label] = staleAt + pod.Annotations[key.ReadyAt.Label] = staleAt + fakeKubernetes.Tracker().Add(pod) + + ctrl := New(cfg, nil, fakeKubernetes, nil) + ctrl.setStartedAt(time.Now().Add(-5 * time.Second)) + + err := ctrl.startInformers(ctx) + assert.NilError(t, err) + + supervisor := ctrl.supervisor(fn) + // No router heartbeats yet -- this is the post-restart state. + + err = supervisor.converge(ctx) + assert.NilError(t, err) + + pods, err := fakeKubernetes.CoreV1().Pods(fn.GetNamespace()).List(ctx, metav1.ListOptions{ + LabelSelector: key.Tenant.Label + "=" + fn.GetTenant(), + }) + assert.NilError(t, err) + assert.Equal(t, len(pods.Items), 1, + "protection period must hold against the cluster default heartbeat timeout (90s); the tenant's 1s override must not shrink the router-propagation budget") +} diff --git a/internal/controller/supervisor.go b/internal/controller/supervisor.go index 938de2b2..ec65eda2 100644 --- a/internal/controller/supervisor.go +++ b/internal/controller/supervisor.go @@ -344,13 +344,16 @@ func (s *Supervisor) converge(ctx context.Context) error { if isScalingDown { // Use the maximum of HPADownscaleStabilization and HeartbeatTimeout as the protection period. // This ensures new controllers don't scale down until they've had enough time to: - // 1. Record recommendations for the stabilization window (HPADownscaleStabilization) + // 1. Record recommendations for the stabilization window (HPADownscaleStabilization). + // Per-function override is honored: tenants who shorten the window opt into thin data. // 2. Receive heartbeats from routers (HeartbeatTimeout) - without this, functions with // instances assigned longer than HeartbeatTimeout ago would immediately trigger - // heartbeat_timeout scale-to-zero when a new controller starts with empty heartbeat state + // heartbeat_timeout scale-to-zero when a new controller starts with empty heartbeat state. + // Router heartbeat propagation is a cluster-wide concern (router heartbeat interval is + // not per-function), so this half stays on the cluster flag rather than the resolver. protectionPeriod := max( fn.HPADownscaleStabilization(s.ctrl.config.HPADownscaleStabilization), - fn.HeartbeatTimeout(s.ctrl.config.HeartbeatTimeout), + s.ctrl.config.HeartbeatTimeout, ) if time.Since(s.ctrl.StartedAt()) < protectionPeriod { log.Debug(ctx, "skipping scale down because controller hasn't been running long enough", slog.Time("started_at", s.ctrl.StartedAt())) From d450d7b4f4bdc58dfcd0516f38387f10ae843caa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Scott=20C=C3=B4t=C3=A9?= Date: Tue, 5 May 2026 01:58:17 -0400 Subject: [PATCH 08/17] Pass converge's function snapshot through recordRecommendation 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. --- internal/controller/per_function_hpa_test.go | 47 ++++++++++++++++++-- internal/controller/supervisor.go | 9 ++-- internal/controller/supervisor_test.go | 6 ++- 3 files changed, 53 insertions(+), 9 deletions(-) diff --git a/internal/controller/per_function_hpa_test.go b/internal/controller/per_function_hpa_test.go index ab84c946..9b549c98 100644 --- a/internal/controller/per_function_hpa_test.go +++ b/internal/controller/per_function_hpa_test.go @@ -143,8 +143,8 @@ func TestRecordRecommendationPerFunctionStabilization(t *testing.T) { // Now record a low recommendation. Each supervisor returns the max within // its own window. The short-window supervisor has already evicted the old // recommendation; the cluster-default supervisor still sees it. - shortMax := shortSup.recordRecommendation(1) - clusterMax := clusterSup.recordRecommendation(1) + shortMax := shortSup.recordRecommendation(shortFn, 1) + clusterMax := clusterSup.recordRecommendation(base, 1) assert.Equal(t, shortMax.DesiredInstances, uint32(1), "short stabilization window should forget the 1-minute-old high recommendation") @@ -234,20 +234,59 @@ func TestRecordRecommendationOmittedHpaPolicy(t *testing.T) { cfg := testConfig() cfg.HPADownscaleStabilization = 5 * time.Minute + fn := fixture.NewFunction(t) ctrl := New(cfg, nil, nil, nil) sup := &Supervisor{ctrl: ctrl} - sup.fn.Store(fixture.NewFunction(t)) + sup.fn.Store(fn) // Recommendation 1 minute ago is well within the 5-minute cluster default, // so it should still be in the window. old := time.Now().Add(-time.Minute) sup.stabilizationWindow = []Recommendation{{DesiredInstances: 7, Timestamp: old}} - max := sup.recordRecommendation(1) + max := sup.recordRecommendation(fn, 1) assert.Equal(t, max.DesiredInstances, uint32(7), "function without hpa policy should use cluster default (5m) and keep the 1m-old recommendation") } +// TestRecordRecommendationUsesPassedFunctionNotSupervisorState pins the +// snapshot invariant: recordRecommendation prunes the stabilization window +// against the supplied function's policy, not against whatever updateFunction +// has stored in s.fn at the moment of call. This matters because converge +// captures fn := s.fn.Load() under s.mu but updateFunction CAS-writes s.fn +// without the mutex; if recordRecommendation re-loaded s.fn, a concurrent +// policy swap could prune an inconsistent window mid-tick. +func TestRecordRecommendationUsesPassedFunctionNotSupervisorState(t *testing.T) { + t.Parallel() + + cfg := testConfig() + cfg.HPADownscaleStabilization = 5 * time.Minute + + base := fixture.NewFunction(t) + patientFn := withHpaPolicy(base, skipper.HpaPolicy_builder{ + DownscaleStabilization: durationpb.New(10 * time.Minute), + }.Build()) + tightFn := withHpaPolicy(base, skipper.HpaPolicy_builder{ + DownscaleStabilization: durationpb.New(time.Millisecond), + }.Build()) + + ctrl := New(cfg, nil, nil, nil) + sup := &Supervisor{ctrl: ctrl} + // Simulate a concurrent updateFunction CAS that swapped s.fn to the tight + // policy after converge captured the patient snapshot. + sup.fn.Store(tightFn) + + old := time.Now().Add(-2 * time.Minute) + sup.stabilizationWindow = []Recommendation{{DesiredInstances: 9, Timestamp: old}} + + // Pass the patient snapshot explicitly. Even though s.fn now holds the + // tight policy, the stabilization window must use patientFn's 10-minute + // window and keep the 2-minute-old recommendation. + max := sup.recordRecommendation(patientFn, 1) + assert.Equal(t, max.DesiredInstances, uint32(9), + "recordRecommendation must prune against the supplied fn (10m window), not s.fn (1ms window) -- a concurrent updateFunction swap must not change which recommendations the converge tick sees") +} + // liveScaleDecision drives calculateDesiredInstances with a fresh heartbeat so // no heartbeat-timeout reason fires; useful for asserting protectionPeriod and // other downstream logic. diff --git a/internal/controller/supervisor.go b/internal/controller/supervisor.go index ec65eda2..d69e2e8b 100644 --- a/internal/controller/supervisor.go +++ b/internal/controller/supervisor.go @@ -217,9 +217,12 @@ func (s *Supervisor) combinedHeartbeat(fn *skipper.Function, instances []*skippe // recordRecommendation adds a scaling recommendation to the stabilization window, // prunes expired entries, and returns the maximum recommendation within the window. -func (s *Supervisor) recordRecommendation(desiredInstances uint32) Recommendation { +// Takes fn explicitly so the caller's per-converge snapshot drives the pruning +// window -- a concurrent updateFunction CAS can swap s.fn at any time, and the +// stabilization window must remain consistent with the rest of the converge tick. +func (s *Supervisor) recordRecommendation(fn *skipper.Function, desiredInstances uint32) Recommendation { now := time.Now() - stabilization := s.fn.Load().HPADownscaleStabilization(s.ctrl.config.HPADownscaleStabilization) + stabilization := fn.HPADownscaleStabilization(s.ctrl.config.HPADownscaleStabilization) cutoff := now.Add(-stabilization) s.stabilizationWindow = append(s.stabilizationWindow, Recommendation{ DesiredInstances: desiredInstances, @@ -336,7 +339,7 @@ func (s *Supervisor) converge(ctx context.Context) error { return nil } - maxRecommendation := s.recordRecommendation(scalingDecision.GetDesiredInstances()) + maxRecommendation := s.recordRecommendation(fn, scalingDecision.GetDesiredInstances()) currentInstances := uint32(len(instances)) isScalingDown := scalingDecision.GetDesiredInstances() < currentInstances || scalingDecision.GetDesiredInstances() == 0 diff --git a/internal/controller/supervisor_test.go b/internal/controller/supervisor_test.go index aaef93ef..3e7620cd 100644 --- a/internal/controller/supervisor_test.go +++ b/internal/controller/supervisor_test.go @@ -4044,13 +4044,15 @@ func BenchmarkRecordRecommendation(b *testing.B) { return window } + fn := &skipper.Function{} + b.Run("with_300_entries", func(b *testing.B) { b.ReportAllocs() template := makeWindow() s := &Supervisor{ctrl: &Controller{config: cfg}} for b.Loop() { s.stabilizationWindow = append(s.stabilizationWindow[:0], template...) - sinkRecommendation = s.recordRecommendation(5) + sinkRecommendation = s.recordRecommendation(fn, 5) } }) @@ -4059,7 +4061,7 @@ func BenchmarkRecordRecommendation(b *testing.B) { s := &Supervisor{ctrl: &Controller{config: cfg}} for b.Loop() { s.stabilizationWindow = s.stabilizationWindow[:0] - sinkRecommendation = s.recordRecommendation(5) + sinkRecommendation = s.recordRecommendation(fn, 5) } }) } From 692febe88f7cd216f6434977862a82d8a5177520 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Scott=20C=C3=B4t=C3=A9?= Date: Tue, 5 May 2026 02:04:44 -0400 Subject: [PATCH 09/17] Reject negative hpa.tolerance in Function.Validate 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. --- internal/skipper/function.go | 6 ++++++ internal/skipper/policies_test.go | 7 +++++++ 2 files changed, 13 insertions(+) diff --git a/internal/skipper/function.go b/internal/skipper/function.go index d0865d2f..bb0f35ea 100644 --- a/internal/skipper/function.go +++ b/internal/skipper/function.go @@ -94,6 +94,12 @@ func (f *Function) Validate() error { return fmt.Errorf("scale.min_instances (%d) must be <= scale.max_instances (%d)", scale.GetMinInstances(), scale.GetMaxInstances()) } if hpa := f.GetHpa(); hpa != nil { + // Tolerance gates the HPA dead-band via `usageDiscrepancy <= tolerance`, + // where usageDiscrepancy is non-negative (math.Abs). A negative tolerance + // would silently disable the dead-band and trigger continuous scaling. + if t := hpa.GetTolerance(); t < 0 { + return fmt.Errorf("hpa.tolerance (%g) must be >= 0", t) + } if d := hpa.GetDownscaleStabilization().AsDuration(); d < 0 { return fmt.Errorf("hpa.downscale_stabilization (%s) must be >= 0", d) } diff --git a/internal/skipper/policies_test.go b/internal/skipper/policies_test.go index ca1ce308..95ac12ca 100644 --- a/internal/skipper/policies_test.go +++ b/internal/skipper/policies_test.go @@ -145,6 +145,13 @@ func TestValidatePolicies(t *testing.T) { }.Build() }), }, + { + name: "negative hpa.tolerance", + fn: build(func(b *Function_builder) { + b.Hpa = HpaPolicy_builder{Tolerance: new(-0.5)}.Build() + }), + wantErr: "hpa.tolerance", + }, { name: "negative hpa.downscale_stabilization", fn: build(func(b *Function_builder) { From 0a1bf3ab1f998fafa556adddaef14bb89e7ea192 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Scott=20C=C3=B4t=C3=A9?= Date: Tue, 5 May 2026 02:17:36 -0400 Subject: [PATCH 10/17] Address remaining bugbot findings on per-function policy 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. --- internal/controller/supervisor.go | 6 ++-- internal/router/per_function_proxy_test.go | 37 ++++++++++++++++++++++ internal/router/router.go | 23 ++++++++++++-- 3 files changed, 61 insertions(+), 5 deletions(-) diff --git a/internal/controller/supervisor.go b/internal/controller/supervisor.go index d69e2e8b..214730e9 100644 --- a/internal/controller/supervisor.go +++ b/internal/controller/supervisor.go @@ -850,9 +850,9 @@ func calculateDesiredInstances(ctx context.Context, cfg *Config, fn *skipper.Fun } // Oneshot functions scale 1:1 with in-flight requests. - if heartbeat.GetFunction().GetOneshot() { + if fn.GetOneshot() { desiredInstances := int(heartbeat.GetInFlightRequests()) - scale := heartbeat.GetFunction().GetScale() + scale := fn.GetScale() clamped := min(max(uint32(desiredInstances), scale.GetMinInstances()), scale.GetMaxInstances()) metric := &skipper.ScaleMetric{} @@ -873,7 +873,7 @@ func calculateDesiredInstances(ctx context.Context, cfg *Config, fn *skipper.Fun var scaleReason skipper.ScaleReason var scaleMetrics []*skipper.ScaleMetric - scale := heartbeat.GetFunction().GetScale() + scale := fn.GetScale() if scale.GetTargetInFlightRequests() > 0 { desiredInstances := int(math.Ceil(float64(heartbeat.GetInFlightRequests()) / float64(scale.GetTargetInFlightRequests()))) diff --git a/internal/router/per_function_proxy_test.go b/internal/router/per_function_proxy_test.go index 1481c09b..b48a9ff1 100644 --- a/internal/router/per_function_proxy_test.go +++ b/internal/router/per_function_proxy_test.go @@ -150,3 +150,40 @@ func TestHeartbeatStateUpdateFunctionOnPolicyChange(t *testing.T) { "updateFunction should swap to the request that carries the new proxy policy") assert.Equal(t, hb.GetFunction().MaxRoundTripAttempts(0), uint32(10)) } + +// TestRoundTripPerFunctionInvertedBackoffBoundsClamps guards the +// hybrid-resolution case: a tenant sets only retry_min_backoff (e.g., 5s) +// while leaving retry_max_backoff unset, and the cluster default for max is +// shorter (1s). Function.Validate cannot catch this because the function-side +// value is consistent in isolation. The router must clamp at the call site so +// the resolved pair is never inverted; otherwise calculateBackoff's min(...) +// silently caps at the cluster max and the tenant's minimum is lost. +func TestRoundTripPerFunctionInvertedBackoffBoundsClamps(t *testing.T) { + t.Parallel() + + cfg := testConfig() + cfg.RoundTripRetryMinTimeout = 100 * time.Millisecond + cfg.RoundTripRetryMaxTimeout = time.Second // cluster cap + + // Tenant sets only min, larger than the cluster max. + fn := withProxyPolicy(fixture.NewFunction(t), skipper.ProxyPolicy_builder{ + RetryMinBackoff: durationpb.New(5 * time.Second), + }.Build()) + + // Simulate the resolution that happens at the top of RoundTrip. + minBackoff := fn.RetryMinBackoff(cfg.RoundTripRetryMinTimeout) + maxBackoff := fn.RetryMaxBackoff(cfg.RoundTripRetryMaxTimeout) + resolvedMin, resolvedMax := clampBackoffBounds(minBackoff, maxBackoff) + + assert.Assert(t, resolvedMin <= resolvedMax, + "clamped bounds must satisfy min <= max; got min=%s max=%s", resolvedMin, resolvedMax) + assert.Equal(t, resolvedMax, cfg.RoundTripRetryMaxTimeout, + "cluster max ceiling stays in force when the tenant did not override it") + + // Backoff must be within the clamped pair on every attempt. + for attempt := 1; attempt < 6; attempt++ { + got := calculateBackoff(attempt, resolvedMin, resolvedMax) + assert.Assert(t, got <= resolvedMax, + "attempt %d backoff %s exceeded resolved max %s", attempt, got, resolvedMax) + } +} diff --git a/internal/router/router.go b/internal/router/router.go index bf794f40..4ffce58d 100644 --- a/internal/router/router.go +++ b/internal/router/router.go @@ -196,8 +196,13 @@ func (r *Router) RoundTrip(req *http.Request) (*http.Response, error) { } maxAttempts := fn.MaxRoundTripAttempts(uint32(r.config.MaxRoundTripAttempts)) - minBackoff := fn.RetryMinBackoff(r.config.RoundTripRetryMinTimeout) - maxBackoff := fn.RetryMaxBackoff(r.config.RoundTripRetryMaxTimeout) + // Resolve min and max independently, then clamp -- a tenant who sets only + // one half of the pair against a cluster default for the other can produce + // an inverted runtime pair that Function.Validate cannot catch. + minBackoff, maxBackoff := clampBackoffBounds( + fn.RetryMinBackoff(r.config.RoundTripRetryMinTimeout), + fn.RetryMaxBackoff(r.config.RoundTripRetryMaxTimeout), + ) var excludedInstanceNames []string getInstanceDuration := time.Duration(0) @@ -361,3 +366,17 @@ func calculateBackoff(attempt int, minBackoff, maxBackoff time.Duration) time.Du factor := 1 + rand.Float64() // randomize the factor between 1 and 2 to add jitter return time.Duration(min(factor*minTimeout*math.Pow(2, float64(attempt)), maxTimeout)) } + +// clampBackoffBounds enforces min <= max on the resolved retry-backoff pair. +// Function.Validate already rejects inverted pairs that come purely from the +// proto, but the resolver pair (function value, cluster default) can land +// inverted when the function sets only one half and the cluster default for +// the other half is on the wrong side. Clamping the minimum to the maximum +// preserves the operator's hard ceiling on a single backoff while keeping the +// math in calculateBackoff well-defined. +func clampBackoffBounds(minBackoff, maxBackoff time.Duration) (time.Duration, time.Duration) { + if minBackoff > maxBackoff { + minBackoff = maxBackoff + } + return minBackoff, maxBackoff +} From bab87cbf32e7627f6a1526bb095c722b74ae9e8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Scott=20C=C3=B4t=C3=A9?= Date: Tue, 5 May 2026 02:28:43 -0400 Subject: [PATCH 11/17] Pass converge's snapshot through cleanupStuckInstances 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. --- .../controller/per_function_lifecycle_test.go | 80 ++++++++++++------- internal/controller/supervisor.go | 11 ++- internal/controller/supervisor_test.go | 4 +- 3 files changed, 63 insertions(+), 32 deletions(-) diff --git a/internal/controller/per_function_lifecycle_test.go b/internal/controller/per_function_lifecycle_test.go index 563a2d1a..aa674f90 100644 --- a/internal/controller/per_function_lifecycle_test.go +++ b/internal/controller/per_function_lifecycle_test.go @@ -168,45 +168,71 @@ func TestAssignPodPerFunctionAssignTimeoutOverrideSurvivesSlowHandler(t *testing } } -// TestCleanupStuckInstancesPerFunctionAssignTimeout verifies that a stuck -// instance is cleaned up against the function's effective assign timeout. -// Two functions share a controller config (--function-assign-timeout = 30s) -// but one overrides lifecycle.assign_timeout = 1ms. The instance for the -// override is cleaned up after a few milliseconds; the cluster-default one is -// not. -func TestCleanupStuckInstancesPerFunctionAssignTimeout(t *testing.T) { +// TestCleanupStuckInstancesUsesFreshFunctionSnapshot pins the snapshot +// invariant: cleanupStuckInstances must read lifecycle.assign_timeout from +// the converge-tick fn snapshot, not from instance.GetFunction(). The +// per-instance function pointer is the policy serialized into the pod +// annotation at assignment time -- a tenant who shortens +// lifecycle.assign_timeout after assignment expects existing stuck instances +// to be cleaned up faster, not stranded behind the old (longer) timeout. +func TestCleanupStuckInstancesUsesFreshFunctionSnapshot(t *testing.T) { t.Parallel() cfg := testConfig() - cfg.FunctionAssignTimeout = 30 * time.Second + cfg.FunctionAssignTimeout = 30 * time.Second // cluster default - clusterFn := fixture.NewFunction(t) - overrideFn := withLifecyclePolicy(fixture.NewFunction(t), skipper.LifecyclePolicy_builder{ + // A pod was assigned earlier when the function carried no override -- + // instance.GetFunction() therefore captures the long cluster default. + staleFn := fixture.NewFunction(t) + staleAssignedAt := timestamppb.New(time.Now().Add(-100 * time.Millisecond)) + pod := fixture.NewAssignedPod(t, staleFn, nil) + instance := skipper.Instance_builder{ + Function: staleFn, + Name: new(pod.Name), + AssignedAt: staleAssignedAt, + }.Build() + + // The tenant has since shortened assign_timeout to 1ms via the next + // request. The converge tick captures the fresh snapshot. + freshFn := withLifecyclePolicy(staleFn, skipper.LifecyclePolicy_builder{ AssignTimeout: durationpb.New(time.Millisecond), }.Build()) - clusterPod := fixture.NewAssignedPod(t, clusterFn, nil) - overridePod := fixture.NewAssignedPod(t, overrideFn, nil) + fakeKubernetes := fake.NewClientset(fixture.NewControllerPod(), pod) + ctrl := New(cfg, nil, fakeKubernetes, nil) + sup := &Supervisor{ctrl: ctrl} - staleAssignedAt := timestamppb.New(time.Now().Add(-100 * time.Millisecond)) - clusterInstance := skipper.Instance_builder{ - Function: clusterFn, - Name: new(clusterPod.Name), - AssignedAt: staleAssignedAt, - }.Build() - overrideInstance := skipper.Instance_builder{ - Function: overrideFn, - Name: new(overridePod.Name), - AssignedAt: staleAssignedAt, + remaining := sup.cleanupStuckInstances(t.Context(), freshFn, []*skipper.Instance{instance}) + + assert.Equal(t, len(remaining), 0, + "fresh fn snapshot (1ms assign_timeout) must drive cleanup; the stale annotation policy must not strand the instance behind a 30s cluster default") +} + +// TestCleanupStuckInstancesOmittedLifecyclePolicy verifies the cluster default +// path: a function omitting lifecycle.assign_timeout uses +// --function-assign-timeout * 2 as the cleanup threshold. +func TestCleanupStuckInstancesOmittedLifecyclePolicy(t *testing.T) { + t.Parallel() + + cfg := testConfig() + cfg.FunctionAssignTimeout = time.Millisecond + + fn := fixture.NewFunction(t) + assert.Assert(t, fn.GetLifecycle() == nil, "fixture function must omit lifecycle policy") + + pod := fixture.NewAssignedPod(t, fn, nil) + instance := skipper.Instance_builder{ + Function: fn, + Name: new(pod.Name), + AssignedAt: timestamppb.New(time.Now().Add(-100 * time.Millisecond)), }.Build() - fakeKubernetes := fake.NewClientset(fixture.NewControllerPod(), clusterPod, overridePod) + fakeKubernetes := fake.NewClientset(fixture.NewControllerPod(), pod) ctrl := New(cfg, nil, fakeKubernetes, nil) sup := &Supervisor{ctrl: ctrl} - remaining := sup.cleanupStuckInstances(t.Context(), []*skipper.Instance{clusterInstance, overrideInstance}) + remaining := sup.cleanupStuckInstances(t.Context(), fn, []*skipper.Instance{instance}) - assert.Equal(t, len(remaining), 1, "override instance should be cleaned up; cluster instance retained") - assert.Equal(t, remaining[0].GetName(), clusterPod.Name, - "only the cluster-default instance should remain") + assert.Equal(t, len(remaining), 0, + "cluster default 1ms * 2 = 2ms threshold should clean up a 100ms-stuck instance") } diff --git a/internal/controller/supervisor.go b/internal/controller/supervisor.go index 214730e9..b998a4bf 100644 --- a/internal/controller/supervisor.go +++ b/internal/controller/supervisor.go @@ -390,7 +390,7 @@ func (s *Supervisor) converge(ctx context.Context) error { } // 3. Cleanup stuck instances (cheap, run before scaling execution) - instances = s.cleanupStuckInstances(ctx, instances) + instances = s.cleanupStuckInstances(ctx, fn, instances) // 4. Execute scaling ready, unready, err := s.scaleWithoutLock(ctx, fn, instances, scalingDecision) @@ -552,9 +552,14 @@ func (s *Supervisor) scaleWithoutLock(ctx context.Context, fn *skipper.Function, // with the cluster flag as the fallback. This is a cheap operation (just // deletes) and should be called before scaling execution to remove broken // pods from consideration. -func (s *Supervisor) cleanupStuckInstances(ctx context.Context, instances []*skipper.Instance) []*skipper.Instance { +// +// Takes fn explicitly so the caller's converge-tick snapshot drives the +// cleanup threshold. instance.GetFunction() is the function serialized into +// the pod annotation at assignment time and may be stale relative to the +// tenant's current lifecycle.assign_timeout override. +func (s *Supervisor) cleanupStuckInstances(ctx context.Context, fn *skipper.Function, instances []*skipper.Instance) []*skipper.Instance { + assignTimeout := fn.AssignTimeout(s.ctrl.config.FunctionAssignTimeout) return slices.DeleteFunc(instances, func(instance *skipper.Instance) bool { - assignTimeout := instance.GetFunction().AssignTimeout(s.ctrl.config.FunctionAssignTimeout) if !instance.HasReadyAt() && time.Since(instance.GetAssignedAt().AsTime()) > assignTimeout*2 { ctx := log.With(ctx, skipper.InstanceKey.Slog(instance)) log.Warn(ctx, "terminating instance stuck in assigned state") diff --git a/internal/controller/supervisor_test.go b/internal/controller/supervisor_test.go index 3e7620cd..64292086 100644 --- a/internal/controller/supervisor_test.go +++ b/internal/controller/supervisor_test.go @@ -2212,7 +2212,7 @@ func TestCleanupStuckInstances(t *testing.T) { assert.NilError(t, err) supervisor := state.ctrl.supervisor(state.fn) - instances = supervisor.cleanupStuckInstances(ctx, instances) + instances = supervisor.cleanupStuckInstances(ctx, state.fn, instances) tc.check(t, state, instances) }) @@ -4159,7 +4159,7 @@ func TestSupervisorEvents(t *testing.T) { instances, err := state.ctrl.getInstances(ctx, state.fn) assert.NilError(t, err) supervisor := state.ctrl.supervisor(state.fn) - supervisor.cleanupStuckInstances(ctx, instances) + supervisor.cleanupStuckInstances(ctx, state.fn, instances) }, check: func(t *testing.T, state *testState) { events := state.ctrl.events.snapshot() From 9fd66249c5b5d83213250bfe47c3a03f04db94b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Scott=20C=C3=B4t=C3=A9?= Date: Tue, 5 May 2026 12:32:45 -0400 Subject: [PATCH 12/17] Honor explicit zero on per-function policy resolvers 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. --- internal/skipper/function.go | 40 ++++++++++--------- internal/skipper/policies_test.go | 66 +++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 19 deletions(-) diff --git a/internal/skipper/function.go b/internal/skipper/function.go index bb0f35ea..26830251 100644 --- a/internal/skipper/function.go +++ b/internal/skipper/function.go @@ -137,10 +137,12 @@ func (f *Function) Validate() error { } // HPATolerance returns the per-function HPA tolerance, falling back to -// clusterDefault when the function does not set one. +// clusterDefault when the function does not set one. Presence is checked via +// HasTolerance so a tenant who explicitly sets zero (strict mode) is honored +// rather than silently overridden by the cluster default. func (f *Function) HPATolerance(clusterDefault float64) float64 { - if v := f.GetHpa().GetTolerance(); v != 0 { - return v + if hpa := f.GetHpa(); hpa.HasTolerance() { + return hpa.GetTolerance() } return clusterDefault } @@ -148,8 +150,8 @@ func (f *Function) HPATolerance(clusterDefault float64) float64 { // HPADownscaleStabilization returns the per-function downscale-stabilization // window, falling back to clusterDefault when the function does not set one. func (f *Function) HPADownscaleStabilization(clusterDefault time.Duration) time.Duration { - if v := f.GetHpa().GetDownscaleStabilization().AsDuration(); v != 0 { - return v + if hpa := f.GetHpa(); hpa.HasDownscaleStabilization() { + return hpa.GetDownscaleStabilization().AsDuration() } return clusterDefault } @@ -157,8 +159,8 @@ func (f *Function) HPADownscaleStabilization(clusterDefault time.Duration) time. // HPAInitialReadinessDelay returns the per-function initial-readiness delay, // falling back to clusterDefault when the function does not set one. func (f *Function) HPAInitialReadinessDelay(clusterDefault time.Duration) time.Duration { - if v := f.GetHpa().GetInitialReadinessDelay().AsDuration(); v != 0 { - return v + if hpa := f.GetHpa(); hpa.HasInitialReadinessDelay() { + return hpa.GetInitialReadinessDelay().AsDuration() } return clusterDefault } @@ -166,8 +168,8 @@ func (f *Function) HPAInitialReadinessDelay(clusterDefault time.Duration) time.D // HeartbeatTimeout returns the per-function heartbeat timeout, falling back to // clusterDefault when the function does not set one. func (f *Function) HeartbeatTimeout(clusterDefault time.Duration) time.Duration { - if v := f.GetHeartbeat().GetTimeout().AsDuration(); v != 0 { - return v + if hb := f.GetHeartbeat(); hb.HasTimeout() { + return hb.GetTimeout().AsDuration() } return clusterDefault } @@ -175,8 +177,8 @@ func (f *Function) HeartbeatTimeout(clusterDefault time.Duration) time.Duration // MaxRoundTripAttempts returns the per-function maximum number of retry // attempts, falling back to clusterDefault when the function does not set one. func (f *Function) MaxRoundTripAttempts(clusterDefault uint32) uint32 { - if v := f.GetProxy().GetMaxAttempts(); v != 0 { - return v + if proxy := f.GetProxy(); proxy.HasMaxAttempts() { + return proxy.GetMaxAttempts() } return clusterDefault } @@ -184,8 +186,8 @@ func (f *Function) MaxRoundTripAttempts(clusterDefault uint32) uint32 { // RetryMinBackoff returns the per-function minimum retry backoff, falling // back to clusterDefault when the function does not set one. func (f *Function) RetryMinBackoff(clusterDefault time.Duration) time.Duration { - if v := f.GetProxy().GetRetryMinBackoff().AsDuration(); v != 0 { - return v + if proxy := f.GetProxy(); proxy.HasRetryMinBackoff() { + return proxy.GetRetryMinBackoff().AsDuration() } return clusterDefault } @@ -193,8 +195,8 @@ func (f *Function) RetryMinBackoff(clusterDefault time.Duration) time.Duration { // RetryMaxBackoff returns the per-function maximum retry backoff, falling // back to clusterDefault when the function does not set one. func (f *Function) RetryMaxBackoff(clusterDefault time.Duration) time.Duration { - if v := f.GetProxy().GetRetryMaxBackoff().AsDuration(); v != 0 { - return v + if proxy := f.GetProxy(); proxy.HasRetryMaxBackoff() { + return proxy.GetRetryMaxBackoff().AsDuration() } return clusterDefault } @@ -202,8 +204,8 @@ func (f *Function) RetryMaxBackoff(clusterDefault time.Duration) time.Duration { // AssignTimeout returns the per-function instance-assignment timeout, falling // back to clusterDefault when the function does not set one. func (f *Function) AssignTimeout(clusterDefault time.Duration) time.Duration { - if v := f.GetLifecycle().GetAssignTimeout().AsDuration(); v != 0 { - return v + if lc := f.GetLifecycle(); lc.HasAssignTimeout() { + return lc.GetAssignTimeout().AsDuration() } return clusterDefault } @@ -211,8 +213,8 @@ func (f *Function) AssignTimeout(clusterDefault time.Duration) time.Duration { // TokenTTL returns the per-function PASETO token lifetime, falling back to // clusterDefault when the function does not set one. func (f *Function) TokenTTL(clusterDefault time.Duration) time.Duration { - if v := f.GetLifecycle().GetTokenTtl().AsDuration(); v != 0 { - return v + if lc := f.GetLifecycle(); lc.HasTokenTtl() { + return lc.GetTokenTtl().AsDuration() } return clusterDefault } diff --git a/internal/skipper/policies_test.go b/internal/skipper/policies_test.go index 95ac12ca..0087350e 100644 --- a/internal/skipper/policies_test.go +++ b/internal/skipper/policies_test.go @@ -92,6 +92,72 @@ func TestResolverReturnsFunctionValue(t *testing.T) { assert.Equal(t, fn.TokenTTL(clusterDuration), fnDuration) } +func TestResolverHonorsExplicitZero(t *testing.T) { + t.Parallel() + + clusterFloat := 0.10 + clusterDuration := 30 * time.Second + + cases := []struct { + name string + fn *Function + want func(*testing.T, *Function) + }{ + { + name: "tolerance explicit zero", + fn: Function_builder{ + Hpa: HpaPolicy_builder{Tolerance: new(0.0)}.Build(), + }.Build(), + want: func(t *testing.T, fn *Function) { + assert.Equal(t, fn.HPATolerance(clusterFloat), 0.0) + }, + }, + { + name: "downscale_stabilization explicit zero", + fn: Function_builder{ + Hpa: HpaPolicy_builder{DownscaleStabilization: durationpb.New(0)}.Build(), + }.Build(), + want: func(t *testing.T, fn *Function) { + assert.Equal(t, fn.HPADownscaleStabilization(clusterDuration), time.Duration(0)) + }, + }, + { + name: "initial_readiness_delay explicit zero", + fn: Function_builder{ + Hpa: HpaPolicy_builder{InitialReadinessDelay: durationpb.New(0)}.Build(), + }.Build(), + want: func(t *testing.T, fn *Function) { + assert.Equal(t, fn.HPAInitialReadinessDelay(clusterDuration), time.Duration(0)) + }, + }, + { + name: "retry_min_backoff explicit zero", + fn: Function_builder{ + Proxy: ProxyPolicy_builder{RetryMinBackoff: durationpb.New(0)}.Build(), + }.Build(), + want: func(t *testing.T, fn *Function) { + assert.Equal(t, fn.RetryMinBackoff(clusterDuration), time.Duration(0)) + }, + }, + { + name: "retry_max_backoff explicit zero", + fn: Function_builder{ + Proxy: ProxyPolicy_builder{RetryMaxBackoff: durationpb.New(0)}.Build(), + }.Build(), + want: func(t *testing.T, fn *Function) { + assert.Equal(t, fn.RetryMaxBackoff(clusterDuration), time.Duration(0)) + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + tc.want(t, tc.fn) + }) + } +} + func TestValidatePolicies(t *testing.T) { t.Parallel() From bdb60169d37d696d448bccfd459ee2da298a63b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Scott=20C=C3=B4t=C3=A9?= Date: Tue, 5 May 2026 12:35:28 -0400 Subject: [PATCH 13/17] Read live target in metric scaling math 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. --- internal/controller/per_function_hpa_test.go | 72 ++++++++++++++++++++ internal/controller/supervisor.go | 9 ++- 2 files changed, 78 insertions(+), 3 deletions(-) diff --git a/internal/controller/per_function_hpa_test.go b/internal/controller/per_function_hpa_test.go index 9b549c98..7e2d9250 100644 --- a/internal/controller/per_function_hpa_test.go +++ b/internal/controller/per_function_hpa_test.go @@ -287,6 +287,78 @@ func TestRecordRecommendationUsesPassedFunctionNotSupervisorState(t *testing.T) "recordRecommendation must prune against the supplied fn (10m window), not s.fn (1ms window) -- a concurrent updateFunction swap must not change which recommendations the converge tick sees") } +// TestCalculateDesiredInstancesForMetricUsesLiveFunctionTargetCPU pins the +// snapshot invariant: calculateDesiredInstancesForMetric reads +// target_cpu_usage_milli from the converge-tick fn argument, not from the +// pod-annotation snapshot in instance.GetFunction(). A tenant who tightens or +// loosens the CPU target mid-flight must drive scaling math on the next tick +// without waiting for every instance to be replaced. +func TestCalculateDesiredInstancesForMetricUsesLiveFunctionTargetCPU(t *testing.T) { + t.Parallel() + + cfg := testConfig() + + // Old function: target=100m. Instances are pinned with this snapshot to + // simulate pods assigned before the tenant updated the target. + oldFn := fixture.NewFunction(t) + oldFn.GetScale().SetTargetCpuUsageMilli(100) + + // New function: target=25m. This is the converge-tick argument the + // controller passes after the tenant updated the target. + newFn := proto.Clone(oldFn).(*skipper.Function) + newFn.GetScale().SetTargetCpuUsageMilli(25) + + // Two instances at 50m usage each, captured against oldFn's snapshot. + instances := []*skipper.Instance{ + readyInstance(oldFn, 50), + readyInstance(oldFn, 50), + } + + desired, _ := calculateDesiredInstancesForMetric(t.Context(), cfg, newFn, MetricCPU, instances) + + // newFn target=25, average usage=50 -> ratio=2.0 -> ceil(2*2.0)=4. + // If the function reads the stale annotation (oldFn target=100), it would + // compute ratio=0.5 -> ceil(2*0.5)=1 (scale down to 1). + assert.Equal(t, desired, 4, + "live newFn target (25m) must drive scaling, not the stale oldFn annotation (100m)") +} + +// TestCalculateDesiredInstancesForMetricUsesLiveFunctionTargetMemory mirrors +// the CPU test for the memory metric: a mid-flight target_memory_usage_mib +// update must drive scaling on the next tick without waiting for every +// instance to be replaced. +func TestCalculateDesiredInstancesForMetricUsesLiveFunctionTargetMemory(t *testing.T) { + t.Parallel() + + cfg := testConfig() + + oldFn := fixture.NewFunction(t) + oldFn.GetScale().SetTargetMemoryUsageMib(200) + + newFn := proto.Clone(oldFn).(*skipper.Function) + newFn.GetScale().SetTargetMemoryUsageMib(50) + + // Two instances at 100 MiB usage each, captured against oldFn's snapshot. + mkInst := func(fn *skipper.Function, mib uint32) *skipper.Instance { + return skipper.Instance_builder{ + Function: fn, + ReadyAt: timestamppb.New(time.Now().Add(-time.Hour)), + MemoryUsageMib: new(mib), + }.Build() + } + instances := []*skipper.Instance{ + mkInst(oldFn, 100), + mkInst(oldFn, 100), + } + + desired, _ := calculateDesiredInstancesForMetric(t.Context(), cfg, newFn, MetricMemory, instances) + + // newFn target=50, average usage=100 -> ratio=2.0 -> ceil(2*2.0)=4. + // Stale annotation (oldFn target=200) would give ratio=0.5 -> 1. + assert.Equal(t, desired, 4, + "live newFn memory target (50 MiB) must drive scaling, not the stale oldFn annotation (200 MiB)") +} + // liveScaleDecision drives calculateDesiredInstances with a fresh heartbeat so // no heartbeat-timeout reason fires; useful for asserting protectionPeriod and // other downstream logic. diff --git a/internal/controller/supervisor.go b/internal/controller/supervisor.go index b998a4bf..8e823670 100644 --- a/internal/controller/supervisor.go +++ b/internal/controller/supervisor.go @@ -787,15 +787,18 @@ func calculateDesiredInstancesForMetric(_ context.Context, cfg *Config, fn *skip } var targetUsage uint32 + switch metric { + case MetricCPU: + targetUsage = fn.GetScale().GetTargetCpuUsageMilli() + case MetricMemory: + targetUsage = fn.GetScale().GetTargetMemoryUsageMib() + } var totalUsage uint32 for _, instance := range instancesWithMetrics { - // accumulate total usage and keep track of target usage (they should all be identical) switch metric { case MetricCPU: - targetUsage = instance.GetFunction().GetScale().GetTargetCpuUsageMilli() totalUsage += instance.GetCpuUsageMilli() case MetricMemory: - targetUsage = instance.GetFunction().GetScale().GetTargetMemoryUsageMib() totalUsage += instance.GetMemoryUsageMib() } } From c993df6d23db730ee45f2a6b4868ff135e19d92b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Scott=20C=C3=B4t=C3=A9?= Date: Tue, 5 May 2026 12:38:03 -0400 Subject: [PATCH 14/17] Make policy-fallback decisions observable 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. --- internal/controller/supervisor.go | 9 ++- internal/router/per_function_proxy_test.go | 69 ++++++++++++++-------- internal/router/router.go | 26 +++++--- 3 files changed, 69 insertions(+), 35 deletions(-) diff --git a/internal/controller/supervisor.go b/internal/controller/supervisor.go index 8e823670..ab6ac6ae 100644 --- a/internal/controller/supervisor.go +++ b/internal/controller/supervisor.go @@ -172,9 +172,12 @@ func (s *Supervisor) heartbeat(routerIP string, heartbeat *skipper.Heartbeat) { return existing, xsync.CancelOp }) - // garbage collect expired router heartbeats. This GC is keyed by router - // IP and crosses every function this supervisor handles, so we bypass - // the per-function heartbeat resolver and use the cluster default. + // Garbage-collect entries from routers that have stopped heartbeating. + // This is router-liveness GC, not function-scale GC: each entry tracks + // one router's last heartbeat, and a router going silent is bounded by + // the cluster-wide router heartbeat interval -- not by any tenant's + // idle timeout. Use the cluster default so the GC cadence tracks the + // cluster property the entries actually represent. for routerIP, heartbeat := range s.routerHeartbeats.AllRelaxed() { if time.Since(heartbeat.GetTimestamp().AsTime()) > s.ctrl.config.HeartbeatTimeout { s.routerHeartbeats.Delete(routerIP) diff --git a/internal/router/per_function_proxy_test.go b/internal/router/per_function_proxy_test.go index b48a9ff1..7c3ea576 100644 --- a/internal/router/per_function_proxy_test.go +++ b/internal/router/per_function_proxy_test.go @@ -157,33 +157,54 @@ func TestHeartbeatStateUpdateFunctionOnPolicyChange(t *testing.T) { // shorter (1s). Function.Validate cannot catch this because the function-side // value is consistent in isolation. The router must clamp at the call site so // the resolved pair is never inverted; otherwise calculateBackoff's min(...) -// silently caps at the cluster max and the tenant's minimum is lost. +// silently caps at the cluster max and the tenant's minimum is lost. The +// clamp also reports whether it fired so the caller can warn the operator +// that a tenant override was demoted to the cluster ceiling. func TestRoundTripPerFunctionInvertedBackoffBoundsClamps(t *testing.T) { t.Parallel() - cfg := testConfig() - cfg.RoundTripRetryMinTimeout = 100 * time.Millisecond - cfg.RoundTripRetryMaxTimeout = time.Second // cluster cap + t.Run("inverted", func(t *testing.T) { + t.Parallel() + + cfg := testConfig() + cfg.RoundTripRetryMinTimeout = 100 * time.Millisecond + cfg.RoundTripRetryMaxTimeout = time.Second // cluster cap + + // Tenant sets only min, larger than the cluster max. + fn := withProxyPolicy(fixture.NewFunction(t), skipper.ProxyPolicy_builder{ + RetryMinBackoff: durationpb.New(5 * time.Second), + }.Build()) + + // Simulate the resolution that happens at the top of RoundTrip. + minBackoff := fn.RetryMinBackoff(cfg.RoundTripRetryMinTimeout) + maxBackoff := fn.RetryMaxBackoff(cfg.RoundTripRetryMaxTimeout) + resolvedMin, resolvedMax, wasInverted := clampBackoffBounds(minBackoff, maxBackoff) + + assert.Assert(t, wasInverted, + "inverted resolved pair (tenant min=%s, cluster max=%s) must report wasInverted=true", + minBackoff, maxBackoff) + assert.Assert(t, resolvedMin <= resolvedMax, + "clamped bounds must satisfy min <= max; got min=%s max=%s", resolvedMin, resolvedMax) + assert.Equal(t, resolvedMax, cfg.RoundTripRetryMaxTimeout, + "cluster max ceiling stays in force when the tenant did not override it") + + // Backoff must be within the clamped pair on every attempt. + for attempt := 1; attempt < 6; attempt++ { + got := calculateBackoff(attempt, resolvedMin, resolvedMax) + assert.Assert(t, got <= resolvedMax, + "attempt %d backoff %s exceeded resolved max %s", attempt, got, resolvedMax) + } + }) - // Tenant sets only min, larger than the cluster max. - fn := withProxyPolicy(fixture.NewFunction(t), skipper.ProxyPolicy_builder{ - RetryMinBackoff: durationpb.New(5 * time.Second), - }.Build()) + t.Run("ordered", func(t *testing.T) { + t.Parallel() - // Simulate the resolution that happens at the top of RoundTrip. - minBackoff := fn.RetryMinBackoff(cfg.RoundTripRetryMinTimeout) - maxBackoff := fn.RetryMaxBackoff(cfg.RoundTripRetryMaxTimeout) - resolvedMin, resolvedMax := clampBackoffBounds(minBackoff, maxBackoff) - - assert.Assert(t, resolvedMin <= resolvedMax, - "clamped bounds must satisfy min <= max; got min=%s max=%s", resolvedMin, resolvedMax) - assert.Equal(t, resolvedMax, cfg.RoundTripRetryMaxTimeout, - "cluster max ceiling stays in force when the tenant did not override it") - - // Backoff must be within the clamped pair on every attempt. - for attempt := 1; attempt < 6; attempt++ { - got := calculateBackoff(attempt, resolvedMin, resolvedMax) - assert.Assert(t, got <= resolvedMax, - "attempt %d backoff %s exceeded resolved max %s", attempt, got, resolvedMax) - } + // Already-ordered pair: clamp does not fire and wasInverted is false. + resolvedMin, resolvedMax, wasInverted := clampBackoffBounds(100*time.Millisecond, time.Second) + + assert.Assert(t, !wasInverted, + "already-ordered pair must report wasInverted=false") + assert.Equal(t, resolvedMin, 100*time.Millisecond) + assert.Equal(t, resolvedMax, time.Second) + }) } diff --git a/internal/router/router.go b/internal/router/router.go index 4ffce58d..4f456758 100644 --- a/internal/router/router.go +++ b/internal/router/router.go @@ -199,10 +199,18 @@ func (r *Router) RoundTrip(req *http.Request) (*http.Response, error) { // Resolve min and max independently, then clamp -- a tenant who sets only // one half of the pair against a cluster default for the other can produce // an inverted runtime pair that Function.Validate cannot catch. - minBackoff, maxBackoff := clampBackoffBounds( - fn.RetryMinBackoff(r.config.RoundTripRetryMinTimeout), - fn.RetryMaxBackoff(r.config.RoundTripRetryMaxTimeout), - ) + rawMin := fn.RetryMinBackoff(r.config.RoundTripRetryMinTimeout) + rawMax := fn.RetryMaxBackoff(r.config.RoundTripRetryMaxTimeout) + minBackoff, maxBackoff, wasInverted := clampBackoffBounds(rawMin, rawMax) + if wasInverted { + // Warn once per RoundTrip, before the retry loop assembles its + // per-attempt log context, so a reader can see that a tenant override + // was silently demoted to the cluster ceiling. + log.Warn(req.Context(), "retry backoff bounds inverted; clamping minimum to cluster maximum", + key.RetryMinBackoff.Slog(rawMin), + key.RetryMaxBackoff.Slog(rawMax), + ) + } var excludedInstanceNames []string getInstanceDuration := time.Duration(0) @@ -373,10 +381,12 @@ func calculateBackoff(attempt int, minBackoff, maxBackoff time.Duration) time.Du // inverted when the function sets only one half and the cluster default for // the other half is on the wrong side. Clamping the minimum to the maximum // preserves the operator's hard ceiling on a single backoff while keeping the -// math in calculateBackoff well-defined. -func clampBackoffBounds(minBackoff, maxBackoff time.Duration) (time.Duration, time.Duration) { +// math in calculateBackoff well-defined. wasInverted reports whether the +// clamp fired so the caller can warn the operator that a tenant override +// was silently demoted to the cluster ceiling. +func clampBackoffBounds(minBackoff, maxBackoff time.Duration) (time.Duration, time.Duration, bool) { if minBackoff > maxBackoff { - minBackoff = maxBackoff + return maxBackoff, maxBackoff, true } - return minBackoff, maxBackoff + return minBackoff, maxBackoff, false } From 812238bdeb9e3dd4be1d800e2611ffda323fece0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Scott=20C=C3=B4t=C3=A9?= Date: Tue, 5 May 2026 13:06:27 -0400 Subject: [PATCH 15/17] Collapse duplicate metric dispatch in scaling helper 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. --- internal/controller/supervisor.go | 40 +++++++++++++------------------ 1 file changed, 16 insertions(+), 24 deletions(-) diff --git a/internal/controller/supervisor.go b/internal/controller/supervisor.go index ab6ac6ae..10825a11 100644 --- a/internal/controller/supervisor.go +++ b/internal/controller/supervisor.go @@ -758,27 +758,31 @@ func calculateDesiredInstancesForMetric(_ context.Context, cfg *Config, fn *skip currentInstances := len(instances) tolerance := fn.HPATolerance(cfg.HPATolerance) initialReadinessDelay := fn.HPAInitialReadinessDelay(cfg.HPAInitialReadinessDelay) + + var targetUsage uint32 + var sample func(*skipper.Instance) uint32 + switch metric { + case MetricCPU: + targetUsage = fn.GetScale().GetTargetCpuUsageMilli() + sample = (*skipper.Instance).GetCpuUsageMilli + case MetricMemory: + targetUsage = fn.GetScale().GetTargetMemoryUsageMib() + sample = (*skipper.Instance).GetMemoryUsageMib + default: + return currentInstances, 0 + } + var instancesWithMetrics []*skipper.Instance var instancesWithoutMetrics []*skipper.Instance for _, instance := range instances { - var usage uint32 - switch metric { - case MetricCPU: - usage = instance.GetCpuUsageMilli() - case MetricMemory: - usage = instance.GetMemoryUsageMib() - default: - return currentInstances, 0 - } - if metric == MetricCPU && (!instance.HasReadyAt() || time.Since(instance.GetReadyAt().AsTime()) <= initialReadinessDelay) { // ignore CPU metrics for pods that have been ready for less than the initial readiness delay instancesWithoutMetrics = append(instancesWithoutMetrics, instance) continue } - if usage == 0 { + if sample(instance) == 0 { instancesWithoutMetrics = append(instancesWithoutMetrics, instance) } else { instancesWithMetrics = append(instancesWithMetrics, instance) @@ -789,21 +793,9 @@ func calculateDesiredInstancesForMetric(_ context.Context, cfg *Config, fn *skip return currentInstances, 0 } - var targetUsage uint32 - switch metric { - case MetricCPU: - targetUsage = fn.GetScale().GetTargetCpuUsageMilli() - case MetricMemory: - targetUsage = fn.GetScale().GetTargetMemoryUsageMib() - } var totalUsage uint32 for _, instance := range instancesWithMetrics { - switch metric { - case MetricCPU: - totalUsage += instance.GetCpuUsageMilli() - case MetricMemory: - totalUsage += instance.GetMemoryUsageMib() - } + totalUsage += sample(instance) } if targetUsage == 0 { From 31c1fd0faef3bff151e99a4b7719b55942f73108 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Scott=20C=C3=B4t=C3=A9?= Date: Tue, 5 May 2026 13:08:38 -0400 Subject: [PATCH 16/17] Reject explicit zero on degenerate policy fields 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. --- internal/skipper/function.go | 27 ++++++++++++++++++++------- internal/skipper/policies_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/internal/skipper/function.go b/internal/skipper/function.go index 26830251..31f50a5d 100644 --- a/internal/skipper/function.go +++ b/internal/skipper/function.go @@ -107,9 +107,11 @@ func (f *Function) Validate() error { return fmt.Errorf("hpa.initial_readiness_delay (%s) must be >= 0", d) } } - if hb := f.GetHeartbeat(); hb != nil { - if d := hb.GetTimeout().AsDuration(); d < 0 { - return fmt.Errorf("heartbeat.timeout (%s) must be >= 0", d) + if hb := f.GetHeartbeat(); hb != nil && hb.HasTimeout() { + // Zero would scale the function to zero on the first converge tick; + // honour the cluster default by leaving the field unset. + if d := hb.GetTimeout().AsDuration(); d <= 0 { + return fmt.Errorf("heartbeat.timeout (%s) must be > 0 when set", d) } } if proxy := f.GetProxy(); proxy != nil { @@ -124,13 +126,24 @@ func (f *Function) Validate() error { if minBackoff > 0 && maxBackoff > 0 && minBackoff > maxBackoff { return fmt.Errorf("proxy.retry_min_backoff (%s) must be <= proxy.retry_max_backoff (%s)", minBackoff, maxBackoff) } + // Zero attempts fails every request before the first proxy try. + if proxy.HasMaxAttempts() && proxy.GetMaxAttempts() == 0 { + return fmt.Errorf("proxy.max_attempts (0) must be > 0 when set") + } } if lc := f.GetLifecycle(); lc != nil { - if d := lc.GetAssignTimeout().AsDuration(); d < 0 { - return fmt.Errorf("lifecycle.assign_timeout (%s) must be >= 0", d) + if lc.HasAssignTimeout() { + // Zero immediately cancels the assign context; every assignment + // fails. Honour the cluster default by leaving the field unset. + if d := lc.GetAssignTimeout().AsDuration(); d <= 0 { + return fmt.Errorf("lifecycle.assign_timeout (%s) must be > 0 when set", d) + } } - if d := lc.GetTokenTtl().AsDuration(); d < 0 { - return fmt.Errorf("lifecycle.token_ttl (%s) must be >= 0", d) + if lc.HasTokenTtl() { + // Zero issues an already-expired token; every backend rejects. + if d := lc.GetTokenTtl().AsDuration(); d <= 0 { + return fmt.Errorf("lifecycle.token_ttl (%s) must be > 0 when set", d) + } } } return nil diff --git a/internal/skipper/policies_test.go b/internal/skipper/policies_test.go index 0087350e..a66b853c 100644 --- a/internal/skipper/policies_test.go +++ b/internal/skipper/policies_test.go @@ -277,6 +277,34 @@ func TestValidatePolicies(t *testing.T) { }), wantErr: "lifecycle.token_ttl", }, + { + name: "explicit zero heartbeat.timeout rejected", + fn: build(func(b *Function_builder) { + b.Heartbeat = HeartbeatPolicy_builder{Timeout: durationpb.New(0)}.Build() + }), + wantErr: "heartbeat.timeout", + }, + { + name: "explicit zero lifecycle.assign_timeout rejected", + fn: build(func(b *Function_builder) { + b.Lifecycle = LifecyclePolicy_builder{AssignTimeout: durationpb.New(0)}.Build() + }), + wantErr: "lifecycle.assign_timeout", + }, + { + name: "explicit zero lifecycle.token_ttl rejected", + fn: build(func(b *Function_builder) { + b.Lifecycle = LifecyclePolicy_builder{TokenTtl: durationpb.New(0)}.Build() + }), + wantErr: "lifecycle.token_ttl", + }, + { + name: "explicit zero proxy.max_attempts rejected", + fn: build(func(b *Function_builder) { + b.Proxy = ProxyPolicy_builder{MaxAttempts: new(uint32(0))}.Build() + }), + wantErr: "proxy.max_attempts", + }, } for _, tc := range cases { From e20342715eb66b13796e2bf9a6fa38df7d4ed33a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Scott=20C=C3=B4t=C3=A9?= Date: Tue, 5 May 2026 13:26:50 -0400 Subject: [PATCH 17/17] Exclude policy fields from stale-instance detection 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. --- internal/controller/supervisor.go | 11 ++++++-- internal/controller/supervisor_test.go | 37 ++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/internal/controller/supervisor.go b/internal/controller/supervisor.go index 10825a11..d7a7d3ce 100644 --- a/internal/controller/supervisor.go +++ b/internal/controller/supervisor.go @@ -618,8 +618,15 @@ func (s *Supervisor) replaceStaleInstances(ctx context.Context, fn *skipper.Func } // Stale if the replica set was scaled to zero (deployment rollout) - // or if the function config (metadata/scale) has changed. - isStale := replicaSet.Status.Replicas == 0 || !proto.Equal(instance.GetFunction(), fn) + // or if the pod-level function spec (metadata, scale) has changed. + // Per-function policy fields (hpa, heartbeat, proxy, lifecycle) + // resolve at the controller / router on the next converge tick, so + // changing them must not trigger pod replacement -- otherwise every + // policy update would roll the entire pool. + instFn := instance.GetFunction() + isStale := replicaSet.Status.Replicas == 0 || + instFn.GetMetadata() != fn.GetMetadata() || + !proto.Equal(instFn.GetScale(), fn.GetScale()) if !isStale { continue } diff --git a/internal/controller/supervisor_test.go b/internal/controller/supervisor_test.go index 64292086..f22587cf 100644 --- a/internal/controller/supervisor_test.go +++ b/internal/controller/supervisor_test.go @@ -2621,6 +2621,43 @@ func TestReplaceStaleInstances(t *testing.T) { assert.Assert(t, availablePods >= 1, "should have at least 1 available pod") }, }, + { + // A tenant changing only a per-function policy knob (heartbeat / + // hpa / proxy / lifecycle) must NOT trigger pod replacement: the + // resolver layer applies the new value on the next converge tick, + // no new pod pool. Without this guarantee, every policy update + // would cause a rolling replacement of every assigned pod -- the + // opposite of the per-function policy design. + name: "policy-only function change does not mark instance stale", + setup: func(t *testing.T, state *testState) { + // Pod was assigned when the function had no per-function policy. + oldFn := proto.Clone(state.fn).(*skipper.Function) + assignedPod := fixture.NewAssignedPod(t, oldFn, nil) + state.fakeKubernetes.Tracker().Add(assignedPod) + + // Current replica set is still active (Replicas > 0 means not + // stale by replica-set scale-down). + state.fakeKubernetes.Tracker().Add(fixture.CurrentReplicaSet(t, state.fn)) + + // The live function gains an hpa policy. Metadata and scale + // are unchanged. + state.fn.SetHpa(skipper.HpaPolicy_builder{ + Tolerance: new(0.05), + }.Build()) + }, + check: func(t *testing.T, state *testState, instances []*skipper.Instance) { + // Instance must survive: policy-only change is not staleness. + assert.Assert(t, len(instances) == 1, "expected 1 instance, got %d", len(instances)) + // No stale-replacement event must fire -- the replacement + // pipeline runs even if the eventual assignPod errors out, so + // asserting on the event captures the staleness verdict + // itself, not the outcome of the replacement. + for _, event := range state.ctrl.events.snapshot() { + assert.Assert(t, event.GetType() != skipper.EventType_EVENT_TYPE_STALE_REPLACEMENT, + "policy-only change must not mark the instance stale: %s", event.GetMessage()) + } + }, + }, { // Oneshot functions assign a fresh pod per request, so stale replacement // would create a pod that serves nothing. Verify it's skipped entirely.