diff --git a/cmd/ateapi/internal/controlapi/workload_spec.go b/cmd/ateapi/internal/controlapi/workload_spec.go index cce9e02bf..db70ddb77 100644 --- a/cmd/ateapi/internal/controlapi/workload_spec.go +++ b/cmd/ateapi/internal/controlapi/workload_spec.go @@ -171,7 +171,7 @@ 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, diff --git a/cmd/ateapi/internal/controlapi/workload_spec_test.go b/cmd/ateapi/internal/controlapi/workload_spec_test.go index d55d44562..2732b5c09 100644 --- a/cmd/ateapi/internal/controlapi/workload_spec_test.go +++ b/cmd/ateapi/internal/controlapi/workload_spec_test.go @@ -358,7 +358,8 @@ 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: 45, }, }, { @@ -378,7 +379,8 @@ 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, }, }, { 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..e4fa9fca0 100644 --- a/manifests/ate-install/generated/ate.dev_actortemplates.yaml +++ b/manifests/ate-install/generated/ate.dev_actortemplates.yaml @@ -200,6 +200,26 @@ spec: required: - 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 + 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 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 + type: integer required: - httpGet type: object diff --git a/pkg/api/v1alpha1/actortemplate_types.go b/pkg/api/v1alpha1/actortemplate_types.go index e3cd176d5..fb18c267f 100644 --- a/pkg/api/v1alpha1/actortemplate_types.go +++ b/pkg/api/v1alpha1/actortemplate_types.go @@ -158,6 +158,26 @@ 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 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"` } // 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..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) {}, @@ -491,6 +494,65 @@ func TestActorTemplateValidation(t *testing.T) { }, wantErr: true, errMsg: "should match", + }, { + // 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: 300, + } + }, + wantErr: false, + 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) + } + }, + }, { + // 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: -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: 3601, + } + }, + wantErr: true, + errMsg: "should be less than or equal to 3600", }, { name: "valid SandboxClass microvm", mutate: func(at *ActorTemplate) { @@ -1210,47 +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) } }) } } -func TestActorTemplateReadyzPathDefault(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) - } - if want, gotPath := "/readyz", got.Spec.Containers[0].Readyz.HTTPGet.Path; gotPath != want { - t.Errorf("Readyz.HTTPGet.Path = %q, want %q (CRD default)", gotPath, want) - } -} - func TestActorTemplateSpecImmutability(t *testing.T) { ctx := t.Context()