From 45f90d8515a39a749dcdb033c45b06c5924e1205 Mon Sep 17 00:00:00 2001 From: Maya Wang Date: Mon, 3 Aug 2026 15:53:46 -0700 Subject: [PATCH 1/5] readyz: make the overall wait timeout configurable per template The 30s readiness deadline was a package constant, so a workload that legitimately takes longer to bind its HTTP server could not be accommodated without raising the ceiling for every actor in the cluster. How long a workload takes to become ready is a property of that workload, so this plumbs a per-template timeout through the existing readyz chain: ContainerReadyz.timeoutSeconds -> ateletpb.Readyz -> ateompb.Readyz -> readyz.Wait. Unset means the ateom's default, which is the renamed DefaultOverallTimeout, still 30s. Zero is not a meaningful deadline here -- unlike a warmup delay, a zero timeout could never be met -- so a non-positive value on the wire is read as "unset" and falls back to the default. The CRD field is a pointer with Minimum=1 so the API rejects it outright rather than silently substituting. No readyz.WaitAll call site changes: the timeout rides on the probe. The e2e probe fixture now declares a readyz probe with a non-default timeoutSeconds. That gives the readyz path its first e2e coverage and exercises the value crossing ateapi -> atelet -> ateom on real binaries, on both the run and restore paths. --- .../internal/controlapi/workload_spec.go | 3 + .../internal/controlapi/workload_spec_test.go | 22 ++++++- cmd/atelet/main.go | 1 + cmd/atelet/main_test.go | 6 +- internal/e2e/fixtures/probe/probe.yaml.tmpl | 9 +++ internal/proto/ateletpb/atelet.pb.go | 23 ++++++-- internal/proto/ateletpb/atelet.proto | 3 + internal/proto/ateompb/ateom.pb.go | 23 ++++++-- internal/proto/ateompb/ateom.proto | 3 + internal/readyz/readyz.go | 29 +++++++--- internal/readyz/readyz_test.go | 58 +++++++++++++++++++ .../generated/ate.dev_actortemplates.yaml | 18 ++++++ pkg/api/v1alpha1/actortemplate_types.go | 18 ++++++ .../v1alpha1/actortemplate_validation_test.go | 49 ++++++++++++++++ pkg/api/v1alpha1/zz_generated.deepcopy.go | 5 ++ 15 files changed, 247 insertions(+), 23 deletions(-) diff --git a/cmd/ateapi/internal/controlapi/workload_spec.go b/cmd/ateapi/internal/controlapi/workload_spec.go index cce9e02bf..58507070b 100644 --- a/cmd/ateapi/internal/controlapi/workload_spec.go +++ b/cmd/ateapi/internal/controlapi/workload_spec.go @@ -178,6 +178,9 @@ func toAteletReadyz(in *atev1alpha1.ContainerReadyz) *ateletpb.Readyz { Port: in.HTTPGet.Port, } } + if in.TimeoutSeconds != nil { + out.TimeoutSeconds = *in.TimeoutSeconds + } return out } diff --git a/cmd/ateapi/internal/controlapi/workload_spec_test.go b/cmd/ateapi/internal/controlapi/workload_spec_test.go index d55d44562..59ba31672 100644 --- a/cmd/ateapi/internal/controlapi/workload_spec_test.go +++ b/cmd/ateapi/internal/controlapi/workload_spec_test.go @@ -358,7 +358,17 @@ func TestWorkloadSpecFromActorTemplatePropagatesReadyz(t *testing.T) { Name: "with-probe", Image: "main", Readyz: &atev1alpha1.ContainerReadyz{ - HTTPGet: &atev1alpha1.HTTPGetAction{Path: "/health", Port: 8080}, + HTTPGet: &atev1alpha1.HTTPGetAction{Path: "/health", Port: 8080}, + TimeoutSeconds: ptr.To(int32(45)), + }, + }, + { + // An unset timeout stays zero on the wire, which the ateom + // reads as "use the default". + Name: "probe-without-timeout", + Image: "slow", + Readyz: &atev1alpha1.ContainerReadyz{ + HTTPGet: &atev1alpha1.HTTPGetAction{Port: 9090}, }, }, { @@ -378,7 +388,15 @@ func TestWorkloadSpecFromActorTemplatePropagatesReadyz(t *testing.T) { Name: "with-probe", Image: "main", Readyz: &ateletpb.Readyz{ - HttpGet: &ateletpb.HTTPGetAction{Path: "/health", Port: 8080}, + HttpGet: &ateletpb.HTTPGetAction{Path: "/health", Port: 8080}, + TimeoutSeconds: 45, + }, + }, + { + Name: "probe-without-timeout", + Image: "slow", + Readyz: &ateletpb.Readyz{ + HttpGet: &ateletpb.HTTPGetAction{Port: 9090}, }, }, { diff --git a/cmd/atelet/main.go b/cmd/atelet/main.go index b3f0241c9..f7892caad 100644 --- a/cmd/atelet/main.go +++ b/cmd/atelet/main.go @@ -951,6 +951,7 @@ func toAteomReadyz(in *ateletpb.Readyz) *ateompb.Readyz { Port: hg.GetPort(), } } + out.TimeoutSeconds = in.GetTimeoutSeconds() return out } diff --git a/cmd/atelet/main_test.go b/cmd/atelet/main_test.go index 50fe8a82d..a50e201a9 100644 --- a/cmd/atelet/main_test.go +++ b/cmd/atelet/main_test.go @@ -588,7 +588,8 @@ func TestBuildAteomWorkloadSpecForwardsReadyz(t *testing.T) { Name: "with-probe", Image: "main", Readyz: &ateletpb.Readyz{ - HttpGet: &ateletpb.HTTPGetAction{Path: "/health", Port: 8080}, + HttpGet: &ateletpb.HTTPGetAction{Path: "/health", Port: 8080}, + TimeoutSeconds: 45, }, }, { @@ -601,7 +602,8 @@ func TestBuildAteomWorkloadSpecForwardsReadyz(t *testing.T) { { Name: "with-probe", Readyz: &ateompb.Readyz{ - HttpGet: &ateompb.HTTPGetAction{Path: "/health", Port: 8080}, + HttpGet: &ateompb.HTTPGetAction{Path: "/health", Port: 8080}, + TimeoutSeconds: 45, }, }, {Name: "without-probe"}, diff --git a/internal/e2e/fixtures/probe/probe.yaml.tmpl b/internal/e2e/fixtures/probe/probe.yaml.tmpl index 6d2098d06..f79e0cdfe 100644 --- a/internal/e2e/fixtures/probe/probe.yaml.tmpl +++ b/internal/e2e/fixtures/probe/probe.yaml.tmpl @@ -43,6 +43,15 @@ spec: - name: probe image: ko://github.com/agent-substrate/substrate/internal/e2e/fixtures/probe command: ["/ko-app/probe"] + # The probe binary binds :80 immediately, so this gates actor start on a + # readiness signal rather than a guess, and carries a non-default + # timeoutSeconds so e2e covers the value crossing ateapi -> atelet -> ateom + # instead of only the ateom's built-in default. + readyz: + httpGet: + path: /healthz + port: 80 + timeoutSeconds: 60 workerSelector: matchLabels: workload: probe diff --git a/internal/proto/ateletpb/atelet.pb.go b/internal/proto/ateletpb/atelet.pb.go index 021fc8dad..993097710 100644 --- a/internal/proto/ateletpb/atelet.pb.go +++ b/internal/proto/ateletpb/atelet.pb.go @@ -907,10 +907,13 @@ func (x *EnvEntry) GetValue() string { // Readyz describes how to check that a container is ready to serve. // Only HTTP is supported today. type Readyz struct { - state protoimpl.MessageState `protogen:"open.v1"` - HttpGet *HTTPGetAction `protobuf:"bytes,1,opt,name=http_get,json=httpGet,proto3" json:"http_get,omitempty"` - unknownFields protoimpl.UnknownFields - sizeCache protoimpl.SizeCache + state protoimpl.MessageState `protogen:"open.v1"` + HttpGet *HTTPGetAction `protobuf:"bytes,1,opt,name=http_get,json=httpGet,proto3" json:"http_get,omitempty"` + // How long to keep polling before giving up and failing the actor start. + // Zero means the ateom's default. + TimeoutSeconds int32 `protobuf:"varint,2,opt,name=timeout_seconds,json=timeoutSeconds,proto3" json:"timeout_seconds,omitempty"` + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache } func (x *Readyz) Reset() { @@ -950,6 +953,13 @@ func (x *Readyz) GetHttpGet() *HTTPGetAction { return nil } +func (x *Readyz) GetTimeoutSeconds() int32 { + if x != nil { + return x.TimeoutSeconds + } + return 0 +} + // HTTPGetAction performs an HTTP GET against the container. type HTTPGetAction struct { state protoimpl.MessageState `protogen:"open.v1"` @@ -1612,9 +1622,10 @@ const file_atelet_proto_rawDesc = "" + "\rvolume_mounts\x18\x06 \x03(\v2\x13.atelet.VolumeMountR\fvolumeMounts\"4\n" + "\bEnvEntry\x12\x12\n" + "\x04name\x18\x01 \x01(\tR\x04name\x12\x14\n" + - "\x05value\x18\x02 \x01(\tR\x05value\":\n" + + "\x05value\x18\x02 \x01(\tR\x05value\"c\n" + "\x06Readyz\x120\n" + - "\bhttp_get\x18\x01 \x01(\v2\x15.atelet.HTTPGetActionR\ahttpGet\"7\n" + + "\bhttp_get\x18\x01 \x01(\v2\x15.atelet.HTTPGetActionR\ahttpGet\x12'\n" + + "\x0ftimeout_seconds\x18\x02 \x01(\x05R\x0etimeoutSeconds\"7\n" + "\rHTTPGetAction\x12\x12\n" + "\x04path\x18\x01 \x01(\tR\x04path\x12\x12\n" + "\x04port\x18\x02 \x01(\x05R\x04port\"\r\n" + diff --git a/internal/proto/ateletpb/atelet.proto b/internal/proto/ateletpb/atelet.proto index cee9e6a89..0f52cdf0e 100644 --- a/internal/proto/ateletpb/atelet.proto +++ b/internal/proto/ateletpb/atelet.proto @@ -130,6 +130,9 @@ message EnvEntry { // Only HTTP is supported today. message Readyz { HTTPGetAction http_get = 1; + // How long to keep polling before giving up and failing the actor start. + // Zero means the ateom's default. + int32 timeout_seconds = 2; } // HTTPGetAction performs an HTTP GET against the container. diff --git a/internal/proto/ateompb/ateom.pb.go b/internal/proto/ateompb/ateom.pb.go index f738c0578..7cd226315 100644 --- a/internal/proto/ateompb/ateom.pb.go +++ b/internal/proto/ateompb/ateom.pb.go @@ -387,10 +387,13 @@ func (x *DurableDirVolumeMount) GetMountPath() string { // Readyz describes how to check that a container is ready to serve. // Only HTTP is supported today. type Readyz struct { - state protoimpl.MessageState `protogen:"open.v1"` - HttpGet *HTTPGetAction `protobuf:"bytes,1,opt,name=http_get,json=httpGet,proto3" json:"http_get,omitempty"` - unknownFields protoimpl.UnknownFields - sizeCache protoimpl.SizeCache + state protoimpl.MessageState `protogen:"open.v1"` + HttpGet *HTTPGetAction `protobuf:"bytes,1,opt,name=http_get,json=httpGet,proto3" json:"http_get,omitempty"` + // How long to keep polling before giving up and failing the actor start. + // Zero means the ateom's default. + TimeoutSeconds int32 `protobuf:"varint,2,opt,name=timeout_seconds,json=timeoutSeconds,proto3" json:"timeout_seconds,omitempty"` + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache } func (x *Readyz) Reset() { @@ -430,6 +433,13 @@ func (x *Readyz) GetHttpGet() *HTTPGetAction { return nil } +func (x *Readyz) GetTimeoutSeconds() int32 { + if x != nil { + return x.TimeoutSeconds + } + return 0 +} + // HTTPGetAction performs an HTTP GET against the container. type HTTPGetAction struct { state protoimpl.MessageState `protogen:"open.v1"` @@ -916,9 +926,10 @@ const file_ateom_proto_rawDesc = "" + "\vvolume_name\x18\x01 \x01(\tR\n" + "volumeName\x12\x1d\n" + "\n" + - "mount_path\x18\x02 \x01(\tR\tmountPath\"9\n" + + "mount_path\x18\x02 \x01(\tR\tmountPath\"b\n" + "\x06Readyz\x12/\n" + - "\bhttp_get\x18\x01 \x01(\v2\x14.ateom.HTTPGetActionR\ahttpGet\"7\n" + + "\bhttp_get\x18\x01 \x01(\v2\x14.ateom.HTTPGetActionR\ahttpGet\x12'\n" + + "\x0ftimeout_seconds\x18\x02 \x01(\x05R\x0etimeoutSeconds\"7\n" + "\rHTTPGetAction\x12\x12\n" + "\x04path\x18\x01 \x01(\tR\x04path\x12\x12\n" + "\x04port\x18\x02 \x01(\x05R\x04port\"\x15\n" + diff --git a/internal/proto/ateompb/ateom.proto b/internal/proto/ateompb/ateom.proto index 9a2176e38..c5f2293bd 100644 --- a/internal/proto/ateompb/ateom.proto +++ b/internal/proto/ateompb/ateom.proto @@ -104,6 +104,9 @@ message DurableDirVolumeMount { // Only HTTP is supported today. message Readyz { HTTPGetAction http_get = 1; + // How long to keep polling before giving up and failing the actor start. + // Zero means the ateom's default. + int32 timeout_seconds = 2; } // HTTPGetAction performs an HTTP GET against the container. diff --git a/internal/readyz/readyz.go b/internal/readyz/readyz.go index 1700f3bd8..abb3ae28b 100644 --- a/internal/readyz/readyz.go +++ b/internal/readyz/readyz.go @@ -39,11 +39,14 @@ import ( // a few seconds to bind; HTTPClient below is a var so tests can substitute // a transport that targets a test server's loopback address. const ( - OverallTimeout = 30 * time.Second - RequestTimeout = 250 * time.Millisecond - PollInterval = 1 * time.Millisecond - DefaultPath = "/readyz" - maxIdleConnsHost = 1 + // DefaultOverallTimeout applies to probes that do not set + // timeout_seconds. A workload that needs longer says so on its + // ActorTemplate rather than having every actor wait as long. + DefaultOverallTimeout = 30 * time.Second + RequestTimeout = 250 * time.Millisecond + PollInterval = 1 * time.Millisecond + DefaultPath = "/readyz" + maxIdleConnsHost = 1 ) // HTTPClient builds a keep-alive HTTP client tuned for fast, repeated @@ -87,8 +90,9 @@ func Wait(ctx context.Context, containerName string, probe *ateompb.Readyz, acto client := HTTPClient() defer client.CloseIdleConnections() + timeout := overallTimeout(probe) start := time.Now() - deadline := start.Add(OverallTimeout) + deadline := start.Add(timeout) attempts := 0 var lastErr error for { @@ -98,7 +102,7 @@ func Wait(ctx context.Context, containerName string, probe *ateompb.Readyz, acto } if time.Now().After(deadline) { return fmt.Errorf("readyz for %q never returned 200 within %s (%d attempts, last error: %v)", - containerName, OverallTimeout, attempts, lastErr) + containerName, timeout, attempts, lastErr) } attempts++ @@ -126,6 +130,17 @@ func Wait(ctx context.Context, containerName string, probe *ateompb.Readyz, acto } } +// overallTimeout resolves how long Wait polls before giving up. A +// non-positive timeout_seconds falls back to the default: unlike a warmup +// delay, a zero deadline is never a meaningful request, so it means "unset" +// rather than "fail immediately". +func overallTimeout(probe *ateompb.Readyz) time.Duration { + if s := probe.GetTimeoutSeconds(); s > 0 { + return time.Duration(s) * time.Second + } + return DefaultOverallTimeout +} + func tryOnce(ctx context.Context, client *http.Client, url string) (bool, error) { req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { diff --git a/internal/readyz/readyz_test.go b/internal/readyz/readyz_test.go index 4bd1fc56f..db7f8e9b3 100644 --- a/internal/readyz/readyz_test.go +++ b/internal/readyz/readyz_test.go @@ -159,6 +159,64 @@ func TestWait_ContextCancellation(t *testing.T) { } } +func TestOverallTimeout(t *testing.T) { + tests := []struct { + name string + probe *ateompb.Readyz + want time.Duration + }{ + { + name: "unset falls back to the default", + probe: &ateompb.Readyz{}, + want: DefaultOverallTimeout, + }, + { + name: "explicit value is honored", + probe: &ateompb.Readyz{TimeoutSeconds: 300}, + want: 300 * time.Second, + }, + { + // A zero deadline could never be met, so it means "unset" + // rather than "fail immediately". + name: "negative falls back to the default", + probe: &ateompb.Readyz{TimeoutSeconds: -1}, + want: DefaultOverallTimeout, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := overallTimeout(tt.probe); got != tt.want { + t.Errorf("overallTimeout = %v, want %v", got, tt.want) + } + }) + } +} + +func TestWait_GivesUpAtProbeTimeout(t *testing.T) { + // Nothing ever binds this port, so the poll loop runs until the + // probe's own deadline rather than the package default. + port := pickFreePort(t) + probe := &ateompb.Readyz{ + HttpGet: &ateompb.HTTPGetAction{Port: int32(port)}, + TimeoutSeconds: 1, + } + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + start := time.Now() + err := Wait(ctx, "main", probe, "127.0.0.1") + if err == nil { + t.Fatalf("Wait returned nil, expected a timeout error") + } + elapsed := time.Since(start) + if elapsed < time.Second { + t.Errorf("Wait gave up after %v, before the probe's 1s timeout", elapsed) + } + if elapsed > 5*time.Second { + t.Errorf("Wait took %v; the probe timeout was ignored in favor of the %v default", elapsed, DefaultOverallTimeout) + } +} + func TestWaitAll_SkipsContainersWithoutProbe(t *testing.T) { // No server bound, but no probes => should return nil immediately. containers := []*ateompb.Container{ diff --git a/manifests/ate-install/generated/ate.dev_actortemplates.yaml b/manifests/ate-install/generated/ate.dev_actortemplates.yaml index 2f8a23a2c..68b5e8e25 100644 --- a/manifests/ate-install/generated/ate.dev_actortemplates.yaml +++ b/manifests/ate-install/generated/ate.dev_actortemplates.yaml @@ -200,6 +200,24 @@ spec: required: - port type: object + timeoutSeconds: + description: |- + TimeoutSeconds is how long to keep polling HTTPGet before giving up. + Exceeding it fails the actor start rather than proceeding with a + container that never reported ready. + + How long a workload takes to become ready is a property of that workload, + which is why this is set per template rather than cluster-wide: a heavy + runtime that needs minutes should not force every other template to wait + as long before its failures surface. + + Unset means the ateom's default of 30s. Zero is not a meaningful setting + here — unlike a warmup delay, a zero deadline could never be met — so it + is rejected in favor of the default. + format: int32 + maximum: 3600 + minimum: 1 + type: integer required: - httpGet type: object diff --git a/pkg/api/v1alpha1/actortemplate_types.go b/pkg/api/v1alpha1/actortemplate_types.go index e3cd176d5..79021f593 100644 --- a/pkg/api/v1alpha1/actortemplate_types.go +++ b/pkg/api/v1alpha1/actortemplate_types.go @@ -158,6 +158,24 @@ type ContainerReadyz struct { // // +required HTTPGet *HTTPGetAction `json:"httpGet"` + + // TimeoutSeconds is how long to keep polling HTTPGet before giving up. + // Exceeding it fails the actor start rather than proceeding with a + // container that never reported ready. + // + // How long a workload takes to become ready is a property of that workload, + // which is why this is set per template rather than cluster-wide: a heavy + // runtime that needs minutes should not force every other template to wait + // as long before its failures surface. + // + // Unset means the ateom's default of 30s. Zero is not a meaningful setting + // here — unlike a warmup delay, a zero deadline could never be met — so it + // is rejected in favor of the default. + // + // +optional + // +kubebuilder:validation:Minimum=1 + // +kubebuilder:validation:Maximum=3600 + TimeoutSeconds *int32 `json:"timeoutSeconds,omitempty"` } // HTTPGetAction describes an HTTP GET request to perform against the diff --git a/pkg/api/v1alpha1/actortemplate_validation_test.go b/pkg/api/v1alpha1/actortemplate_validation_test.go index d4de40e17..28e8f67d6 100644 --- a/pkg/api/v1alpha1/actortemplate_validation_test.go +++ b/pkg/api/v1alpha1/actortemplate_validation_test.go @@ -491,6 +491,55 @@ func TestActorTemplateValidation(t *testing.T) { }, wantErr: true, errMsg: "should match", + }, { + name: "Readyz TimeoutSeconds unset falls back to the ateom default", + mutate: func(at *ActorTemplate) { + at.Spec.Containers[0].Readyz = &ContainerReadyz{ + HTTPGet: &HTTPGetAction{Port: 80}, + } + }, + wantErr: false, + }, { + name: "valid Readyz TimeoutSeconds", + mutate: func(at *ActorTemplate) { + at.Spec.Containers[0].Readyz = &ContainerReadyz{ + HTTPGet: &HTTPGetAction{Port: 80}, + TimeoutSeconds: ptr.To(int32(300)), + } + }, + wantErr: false, + }, { + // A zero deadline could never be met, so it is rejected rather + // than silently read as "unset". + name: "Readyz TimeoutSeconds zero", + mutate: func(at *ActorTemplate) { + at.Spec.Containers[0].Readyz = &ContainerReadyz{ + HTTPGet: &HTTPGetAction{Port: 80}, + TimeoutSeconds: ptr.To(int32(0)), + } + }, + wantErr: true, + errMsg: "should be greater than or equal to 1", + }, { + name: "Readyz TimeoutSeconds negative", + mutate: func(at *ActorTemplate) { + at.Spec.Containers[0].Readyz = &ContainerReadyz{ + HTTPGet: &HTTPGetAction{Port: 80}, + TimeoutSeconds: ptr.To(int32(-1)), + } + }, + wantErr: true, + errMsg: "should be greater than or equal to 1", + }, { + name: "Readyz TimeoutSeconds above the maximum", + mutate: func(at *ActorTemplate) { + at.Spec.Containers[0].Readyz = &ContainerReadyz{ + HTTPGet: &HTTPGetAction{Port: 80}, + TimeoutSeconds: ptr.To(int32(3601)), + } + }, + wantErr: true, + errMsg: "should be less than or equal to 3600", }, { name: "valid SandboxClass microvm", mutate: func(at *ActorTemplate) { diff --git a/pkg/api/v1alpha1/zz_generated.deepcopy.go b/pkg/api/v1alpha1/zz_generated.deepcopy.go index 610cb4546..c198793bc 100644 --- a/pkg/api/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/api/v1alpha1/zz_generated.deepcopy.go @@ -206,6 +206,11 @@ func (in *ContainerReadyz) DeepCopyInto(out *ContainerReadyz) { *out = new(HTTPGetAction) **out = **in } + if in.TimeoutSeconds != nil { + in, out := &in.TimeoutSeconds, &out.TimeoutSeconds + *out = new(int32) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ContainerReadyz. From 51935453bb79889cd19f0761aec4cbe56eb27aba Mon Sep 17 00:00:00 2001 From: Maya Wang Date: Mon, 3 Aug 2026 20:49:47 -0700 Subject: [PATCH 2/5] readyz: surface the timeout default on the CRD Per review: declare the 30s default with +kubebuilder:default=30 so it is visible on the stored object and in `kubectl explain`, rather than being a constant an author has to know the ateom applies. With the API server defaulting the field, the nil check in toAteletReadyz is no longer load-bearing and becomes a plain deref. It stays nil-safe: the conversion is also reachable with an in-process template that never went through admission, and zero on the wire already means the same 30s. envtest now asserts the read-back value is 30, so the default is checked against a real API server rather than only the marker. --- cmd/ateapi/internal/controlapi/workload_spec.go | 8 +++++--- .../internal/controlapi/workload_spec_test.go | 6 ++++-- .../generated/ate.dev_actortemplates.yaml | 8 +++++--- pkg/api/v1alpha1/actortemplate_types.go | 8 +++++--- .../v1alpha1/actortemplate_validation_test.go | 16 +++++++++++++--- 5 files changed, 32 insertions(+), 14 deletions(-) diff --git a/cmd/ateapi/internal/controlapi/workload_spec.go b/cmd/ateapi/internal/controlapi/workload_spec.go index 58507070b..d406084a4 100644 --- a/cmd/ateapi/internal/controlapi/workload_spec.go +++ b/cmd/ateapi/internal/controlapi/workload_spec.go @@ -29,6 +29,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" + "k8s.io/utils/ptr" ) const envSecretCacheTTL = 30 * time.Second @@ -178,9 +179,10 @@ func toAteletReadyz(in *atev1alpha1.ContainerReadyz) *ateletpb.Readyz { Port: in.HTTPGet.Port, } } - if in.TimeoutSeconds != nil { - out.TimeoutSeconds = *in.TimeoutSeconds - } + // The CRD defaults TimeoutSeconds to 30, so this is nil only for a template + // built in process rather than read from the API server. Zero on the wire + // means the ateom's default, which is the same 30s. + out.TimeoutSeconds = ptr.Deref(in.TimeoutSeconds, 0) return out } diff --git a/cmd/ateapi/internal/controlapi/workload_spec_test.go b/cmd/ateapi/internal/controlapi/workload_spec_test.go index 59ba31672..be0a44ef7 100644 --- a/cmd/ateapi/internal/controlapi/workload_spec_test.go +++ b/cmd/ateapi/internal/controlapi/workload_spec_test.go @@ -363,8 +363,10 @@ func TestWorkloadSpecFromActorTemplatePropagatesReadyz(t *testing.T) { }, }, { - // An unset timeout stays zero on the wire, which the ateom - // reads as "use the default". + // The CRD defaults TimeoutSeconds, so a nil one only reaches + // the conversion from an in-process template like this. It + // must not panic, and stays zero on the wire, which the ateom + // reads as the same default. Name: "probe-without-timeout", Image: "slow", Readyz: &atev1alpha1.ContainerReadyz{ diff --git a/manifests/ate-install/generated/ate.dev_actortemplates.yaml b/manifests/ate-install/generated/ate.dev_actortemplates.yaml index 68b5e8e25..67249cac7 100644 --- a/manifests/ate-install/generated/ate.dev_actortemplates.yaml +++ b/manifests/ate-install/generated/ate.dev_actortemplates.yaml @@ -201,6 +201,7 @@ spec: - port type: object timeoutSeconds: + default: 30 description: |- TimeoutSeconds is how long to keep polling HTTPGet before giving up. Exceeding it fails the actor start rather than proceeding with a @@ -211,9 +212,10 @@ spec: runtime that needs minutes should not force every other template to wait as long before its failures surface. - Unset means the ateom's default of 30s. Zero is not a meaningful setting - here — unlike a warmup delay, a zero deadline could never be met — so it - is rejected in favor of the default. + Unset defaults to 30s, applied by the API server so the effective value is + visible in the stored object rather than only in the ateom. Zero is not a + meaningful setting here — unlike a warmup delay, a zero deadline could + never be met — so it is rejected rather than read as "unset". format: int32 maximum: 3600 minimum: 1 diff --git a/pkg/api/v1alpha1/actortemplate_types.go b/pkg/api/v1alpha1/actortemplate_types.go index 79021f593..2ab79f94a 100644 --- a/pkg/api/v1alpha1/actortemplate_types.go +++ b/pkg/api/v1alpha1/actortemplate_types.go @@ -168,11 +168,13 @@ type ContainerReadyz struct { // runtime that needs minutes should not force every other template to wait // as long before its failures surface. // - // Unset means the ateom's default of 30s. Zero is not a meaningful setting - // here — unlike a warmup delay, a zero deadline could never be met — so it - // is rejected in favor of the default. + // Unset defaults to 30s, applied by the API server so the effective value is + // visible in the stored object rather than only in the ateom. Zero is not a + // meaningful setting here — unlike a warmup delay, a zero deadline could + // never be met — so it is rejected rather than read as "unset". // // +optional + // +kubebuilder:default=30 // +kubebuilder:validation:Minimum=1 // +kubebuilder:validation:Maximum=3600 TimeoutSeconds *int32 `json:"timeoutSeconds,omitempty"` diff --git a/pkg/api/v1alpha1/actortemplate_validation_test.go b/pkg/api/v1alpha1/actortemplate_validation_test.go index 28e8f67d6..793ba8e53 100644 --- a/pkg/api/v1alpha1/actortemplate_validation_test.go +++ b/pkg/api/v1alpha1/actortemplate_validation_test.go @@ -492,7 +492,7 @@ func TestActorTemplateValidation(t *testing.T) { wantErr: true, errMsg: "should match", }, { - name: "Readyz TimeoutSeconds unset falls back to the ateom default", + name: "Readyz TimeoutSeconds unset is defaulted by the API server", mutate: func(at *ActorTemplate) { at.Spec.Containers[0].Readyz = &ContainerReadyz{ HTTPGet: &HTTPGetAction{Port: 80}, @@ -1265,7 +1265,11 @@ func TestActorTemplateValidation(t *testing.T) { } } -func TestActorTemplateReadyzPathDefault(t *testing.T) { +// TestActorTemplateReadyzDefaults asserts that a probe declaring only a port +// reads back with the omitted fields populated, so a template author can see +// the effective readiness settings on the stored object rather than having to +// know what the ateom would have substituted. +func TestActorTemplateReadyzDefaults(t *testing.T) { ctx := t.Context() at := &ActorTemplate{ @@ -1295,9 +1299,15 @@ func TestActorTemplateReadyzPathDefault(t *testing.T) { if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(at), got); err != nil { t.Fatalf("get: %v", err) } - if want, gotPath := "/readyz", got.Spec.Containers[0].Readyz.HTTPGet.Path; gotPath != want { + gotReadyz := got.Spec.Containers[0].Readyz + if want, gotPath := "/readyz", gotReadyz.HTTPGet.Path; gotPath != want { t.Errorf("Readyz.HTTPGet.Path = %q, want %q (CRD default)", gotPath, want) } + if gotReadyz.TimeoutSeconds == nil { + t.Errorf("Readyz.TimeoutSeconds = nil, want 30 (CRD default)") + } else if want := int32(30); *gotReadyz.TimeoutSeconds != want { + t.Errorf("Readyz.TimeoutSeconds = %d, want %d (CRD default)", *gotReadyz.TimeoutSeconds, want) + } } func TestActorTemplateSpecImmutability(t *testing.T) { From 10777f76384673feab83aa5fcb994281b4f30f2c Mon Sep 17 00:00:00 2001 From: Maya Wang Date: Mon, 3 Aug 2026 21:38:25 -0700 Subject: [PATCH 3/5] readyz: drop the pointer on TimeoutSeconds Per review, following the API convention for CRDs: when the zero value is not valid, a pointer buys nothing. Minimum=1 rejects an explicit 0 before any controller sees the object, and +kubebuilder:default=30 fills in the rest, so the field always holds valid data and the conversion is a plain assignment rather than a deref behind a nil check. Also folds the readyz defaulting assertions into TestActorTemplateValidation alongside the other readyz cases, via an optional verify hook for cases that check what the API server stored rather than whether it accepted the create. The standalone path-default test goes away with it. The explicit-zero case goes too: omitempty makes 0 indistinguishable from unset through a typed client, so it now defaults to 30 rather than being rejected. The -1 case still covers the Minimum=1 bound that rejects a manifest spelling out 0. --- .../internal/controlapi/workload_spec.go | 6 +- .../internal/controlapi/workload_spec_test.go | 10 +- .../generated/ate.dev_actortemplates.yaml | 8 +- pkg/api/v1alpha1/actortemplate_types.go | 10 +- .../v1alpha1/actortemplate_validation_test.go | 93 +++++++------------ pkg/api/v1alpha1/zz_generated.deepcopy.go | 5 - 6 files changed, 48 insertions(+), 84 deletions(-) diff --git a/cmd/ateapi/internal/controlapi/workload_spec.go b/cmd/ateapi/internal/controlapi/workload_spec.go index d406084a4..53b38c5cc 100644 --- a/cmd/ateapi/internal/controlapi/workload_spec.go +++ b/cmd/ateapi/internal/controlapi/workload_spec.go @@ -29,7 +29,6 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" - "k8s.io/utils/ptr" ) const envSecretCacheTTL = 30 * time.Second @@ -179,10 +178,7 @@ func toAteletReadyz(in *atev1alpha1.ContainerReadyz) *ateletpb.Readyz { Port: in.HTTPGet.Port, } } - // The CRD defaults TimeoutSeconds to 30, so this is nil only for a template - // built in process rather than read from the API server. Zero on the wire - // means the ateom's default, which is the same 30s. - out.TimeoutSeconds = ptr.Deref(in.TimeoutSeconds, 0) + out.TimeoutSeconds = in.TimeoutSeconds return out } diff --git a/cmd/ateapi/internal/controlapi/workload_spec_test.go b/cmd/ateapi/internal/controlapi/workload_spec_test.go index be0a44ef7..d7af21a76 100644 --- a/cmd/ateapi/internal/controlapi/workload_spec_test.go +++ b/cmd/ateapi/internal/controlapi/workload_spec_test.go @@ -359,14 +359,14 @@ func TestWorkloadSpecFromActorTemplatePropagatesReadyz(t *testing.T) { Image: "main", Readyz: &atev1alpha1.ContainerReadyz{ HTTPGet: &atev1alpha1.HTTPGetAction{Path: "/health", Port: 8080}, - TimeoutSeconds: ptr.To(int32(45)), + TimeoutSeconds: 45, }, }, { - // The CRD defaults TimeoutSeconds, so a nil one only reaches - // the conversion from an in-process template like this. It - // must not panic, and stays zero on the wire, which the ateom - // reads as the same default. + // Templates from the API server always carry a timeout, + // since the CRD defaults it. This is the in-process shape, + // which stays zero on the wire — the same default again, + // this time resolved by the ateom. Name: "probe-without-timeout", Image: "slow", Readyz: &atev1alpha1.ContainerReadyz{ diff --git a/manifests/ate-install/generated/ate.dev_actortemplates.yaml b/manifests/ate-install/generated/ate.dev_actortemplates.yaml index 67249cac7..e4fa9fca0 100644 --- a/manifests/ate-install/generated/ate.dev_actortemplates.yaml +++ b/manifests/ate-install/generated/ate.dev_actortemplates.yaml @@ -212,10 +212,10 @@ spec: runtime that needs minutes should not force every other template to wait as long before its failures surface. - Unset defaults to 30s, applied by the API server so the effective value is - visible in the stored object rather than only in the ateom. Zero is not a - meaningful setting here — unlike a warmup delay, a zero deadline could - never be met — so it is rejected rather than read as "unset". + Unset defaults to 30, applied by the API server so the effective value is + visible on the stored object rather than only in the ateom. A manifest + asking for 0 is rejected: unlike a warmup delay, a zero deadline could + never be met, so it is never what a template author means. format: int32 maximum: 3600 minimum: 1 diff --git a/pkg/api/v1alpha1/actortemplate_types.go b/pkg/api/v1alpha1/actortemplate_types.go index 2ab79f94a..fb18c267f 100644 --- a/pkg/api/v1alpha1/actortemplate_types.go +++ b/pkg/api/v1alpha1/actortemplate_types.go @@ -168,16 +168,16 @@ type ContainerReadyz struct { // runtime that needs minutes should not force every other template to wait // as long before its failures surface. // - // Unset defaults to 30s, applied by the API server so the effective value is - // visible in the stored object rather than only in the ateom. Zero is not a - // meaningful setting here — unlike a warmup delay, a zero deadline could - // never be met — so it is rejected rather than read as "unset". + // Unset defaults to 30, applied by the API server so the effective value is + // visible on the stored object rather than only in the ateom. A manifest + // asking for 0 is rejected: unlike a warmup delay, a zero deadline could + // never be met, so it is never what a template author means. // // +optional // +kubebuilder:default=30 // +kubebuilder:validation:Minimum=1 // +kubebuilder:validation:Maximum=3600 - TimeoutSeconds *int32 `json:"timeoutSeconds,omitempty"` + TimeoutSeconds int32 `json:"timeoutSeconds,omitempty"` } // HTTPGetAction describes an HTTP GET request to perform against the diff --git a/pkg/api/v1alpha1/actortemplate_validation_test.go b/pkg/api/v1alpha1/actortemplate_validation_test.go index 793ba8e53..b59abfab9 100644 --- a/pkg/api/v1alpha1/actortemplate_validation_test.go +++ b/pkg/api/v1alpha1/actortemplate_validation_test.go @@ -107,6 +107,9 @@ func TestActorTemplateValidation(t *testing.T) { mutate func(*ActorTemplate) wantErr bool errMsg string + // verify runs on the created object for cases that assert what the API + // server stored rather than whether it accepted the create. + verify func(*testing.T, *ActorTemplate) }{{ name: "base template", mutate: func(at *ActorTemplate) {}, @@ -492,40 +495,50 @@ func TestActorTemplateValidation(t *testing.T) { wantErr: true, errMsg: "should match", }, { - name: "Readyz TimeoutSeconds unset is defaulted by the API server", + // A probe that declares only a port reads back with the omitted fields + // filled in, so a template author can see the effective readiness + // settings on the object rather than having to know what the ateom + // would substitute. + name: "Readyz omitted fields are defaulted by the API server", mutate: func(at *ActorTemplate) { at.Spec.Containers[0].Readyz = &ContainerReadyz{ HTTPGet: &HTTPGetAction{Port: 80}, } }, wantErr: false, + verify: func(t *testing.T, at *ActorTemplate) { + readyz := at.Spec.Containers[0].Readyz + if want, got := "/readyz", readyz.HTTPGet.Path; got != want { + t.Errorf("Readyz.HTTPGet.Path = %q, want %q (CRD default)", got, want) + } + if want, got := int32(30), readyz.TimeoutSeconds; got != want { + t.Errorf("Readyz.TimeoutSeconds = %d, want %d (CRD default)", got, want) + } + }, }, { name: "valid Readyz TimeoutSeconds", mutate: func(at *ActorTemplate) { at.Spec.Containers[0].Readyz = &ContainerReadyz{ HTTPGet: &HTTPGetAction{Port: 80}, - TimeoutSeconds: ptr.To(int32(300)), + TimeoutSeconds: 300, } }, wantErr: false, - }, { - // A zero deadline could never be met, so it is rejected rather - // than silently read as "unset". - name: "Readyz TimeoutSeconds zero", - mutate: func(at *ActorTemplate) { - at.Spec.Containers[0].Readyz = &ContainerReadyz{ - HTTPGet: &HTTPGetAction{Port: 80}, - TimeoutSeconds: ptr.To(int32(0)), + verify: func(t *testing.T, at *ActorTemplate) { + if want, got := int32(300), at.Spec.Containers[0].Readyz.TimeoutSeconds; got != want { + t.Errorf("Readyz.TimeoutSeconds = %d, want %d (explicit value must survive defaulting)", got, want) } }, - wantErr: true, - errMsg: "should be greater than or equal to 1", }, { - name: "Readyz TimeoutSeconds negative", + // A zero deadline could never be met. The field omits its zero value, + // so 0 from a Go client is indistinguishable from unset and defaults to + // 30; a manifest that spells out 0 is rejected by the same bound this + // case exercises. + name: "Readyz TimeoutSeconds below the minimum", mutate: func(at *ActorTemplate) { at.Spec.Containers[0].Readyz = &ContainerReadyz{ HTTPGet: &HTTPGetAction{Port: 80}, - TimeoutSeconds: ptr.To(int32(-1)), + TimeoutSeconds: -1, } }, wantErr: true, @@ -535,7 +548,7 @@ func TestActorTemplateValidation(t *testing.T) { mutate: func(at *ActorTemplate) { at.Spec.Containers[0].Readyz = &ContainerReadyz{ HTTPGet: &HTTPGetAction{Port: 80}, - TimeoutSeconds: ptr.To(int32(3601)), + TimeoutSeconds: 3601, } }, wantErr: true, @@ -1259,57 +1272,17 @@ func TestActorTemplateValidation(t *testing.T) { t.Errorf("wrong error:\n wanted: %q\n got: %q", tt.errMsg, err.Error()) } if err == nil { + // Create writes the API server's response back into at, so + // verify sees the object as stored — defaults included. + if tt.verify != nil { + tt.verify(t, at) + } _ = k8sClient.Delete(ctx, at) } }) } } -// TestActorTemplateReadyzDefaults asserts that a probe declaring only a port -// reads back with the omitted fields populated, so a template author can see -// the effective readiness settings on the stored object rather than having to -// know what the ateom would have substituted. -func TestActorTemplateReadyzDefaults(t *testing.T) { - ctx := t.Context() - - at := &ActorTemplate{ - ObjectMeta: metav1.ObjectMeta{ - Name: "readyz-default", - Namespace: "default", - }, - Spec: ActorTemplateSpec{ - PauseImage: "gcr.io/gke-release/pause@sha256:bcbd57ba5653580ec647b16d8163cdd1112df3609129b01f912a8032e48265da", - Containers: []Container{{ - Name: "main", - Image: "busybox@sha256:326e0e090a9a4057e62a1b94236e7a2df2f2f76722f67232e0e47854e4df9c53", - Readyz: &ContainerReadyz{ - HTTPGet: &HTTPGetAction{Port: 8080}, - }, - }}, - SnapshotsConfig: SnapshotsConfig{Location: "gs://test-bucket/test-folder"}, - WorkerSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"pool": "test-pool"}}, - }, - } - if err := k8sClient.Create(ctx, at); err != nil { - t.Fatalf("create: %v", err) - } - defer func() { _ = k8sClient.Delete(ctx, at) }() - - got := &ActorTemplate{} - if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(at), got); err != nil { - t.Fatalf("get: %v", err) - } - gotReadyz := got.Spec.Containers[0].Readyz - if want, gotPath := "/readyz", gotReadyz.HTTPGet.Path; gotPath != want { - t.Errorf("Readyz.HTTPGet.Path = %q, want %q (CRD default)", gotPath, want) - } - if gotReadyz.TimeoutSeconds == nil { - t.Errorf("Readyz.TimeoutSeconds = nil, want 30 (CRD default)") - } else if want := int32(30); *gotReadyz.TimeoutSeconds != want { - t.Errorf("Readyz.TimeoutSeconds = %d, want %d (CRD default)", *gotReadyz.TimeoutSeconds, want) - } -} - func TestActorTemplateSpecImmutability(t *testing.T) { ctx := t.Context() diff --git a/pkg/api/v1alpha1/zz_generated.deepcopy.go b/pkg/api/v1alpha1/zz_generated.deepcopy.go index c198793bc..610cb4546 100644 --- a/pkg/api/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/api/v1alpha1/zz_generated.deepcopy.go @@ -206,11 +206,6 @@ func (in *ContainerReadyz) DeepCopyInto(out *ContainerReadyz) { *out = new(HTTPGetAction) **out = **in } - if in.TimeoutSeconds != nil { - in, out := &in.TimeoutSeconds, &out.TimeoutSeconds - *out = new(int32) - **out = **in - } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ContainerReadyz. From 4a1e49b0bd0d26871a4e01f44bc748a82b2397bf Mon Sep 17 00:00:00 2001 From: Maya Wang Date: Mon, 3 Aug 2026 21:58:06 -0700 Subject: [PATCH 4/5] readyz: drop the redundant zero-timeout conversion case With TimeoutSeconds a plain int32, toAteletReadyz assigns it unconditionally, so a container whose probe omits the timeout no longer exercises a distinct path through the conversion. --- .../internal/controlapi/workload_spec_test.go | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/cmd/ateapi/internal/controlapi/workload_spec_test.go b/cmd/ateapi/internal/controlapi/workload_spec_test.go index d7af21a76..2732b5c09 100644 --- a/cmd/ateapi/internal/controlapi/workload_spec_test.go +++ b/cmd/ateapi/internal/controlapi/workload_spec_test.go @@ -362,17 +362,6 @@ func TestWorkloadSpecFromActorTemplatePropagatesReadyz(t *testing.T) { TimeoutSeconds: 45, }, }, - { - // Templates from the API server always carry a timeout, - // since the CRD defaults it. This is the in-process shape, - // which stays zero on the wire — the same default again, - // this time resolved by the ateom. - Name: "probe-without-timeout", - Image: "slow", - Readyz: &atev1alpha1.ContainerReadyz{ - HTTPGet: &atev1alpha1.HTTPGetAction{Port: 9090}, - }, - }, { Name: "without-probe", Image: "side", @@ -394,13 +383,6 @@ func TestWorkloadSpecFromActorTemplatePropagatesReadyz(t *testing.T) { TimeoutSeconds: 45, }, }, - { - Name: "probe-without-timeout", - Image: "slow", - Readyz: &ateletpb.Readyz{ - HttpGet: &ateletpb.HTTPGetAction{Port: 9090}, - }, - }, { Name: "without-probe", Image: "side", From 08bc49e237187ce7b0e9b347dc1d1831c1a48de3 Mon Sep 17 00:00:00 2001 From: Maya Wang Date: Mon, 3 Aug 2026 22:03:34 -0700 Subject: [PATCH 5/5] readyz: set TimeoutSeconds in the Readyz literal --- cmd/ateapi/internal/controlapi/workload_spec.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cmd/ateapi/internal/controlapi/workload_spec.go b/cmd/ateapi/internal/controlapi/workload_spec.go index 53b38c5cc..db70ddb77 100644 --- a/cmd/ateapi/internal/controlapi/workload_spec.go +++ b/cmd/ateapi/internal/controlapi/workload_spec.go @@ -171,14 +171,13 @@ func toAteletReadyz(in *atev1alpha1.ContainerReadyz) *ateletpb.Readyz { if in == nil { return nil } - out := &ateletpb.Readyz{} + out := &ateletpb.Readyz{TimeoutSeconds: in.TimeoutSeconds} if in.HTTPGet != nil { out.HttpGet = &ateletpb.HTTPGetAction{ Path: in.HTTPGet.Path, Port: in.HTTPGet.Port, } } - out.TimeoutSeconds = in.TimeoutSeconds return out }