mirror of
https://github.com/agent-substrate/substrate.git
synced 2026-10-02 03:24:42 +08:00
Maya WangandMaya Wang
9e3ee7a3d8
> **Rescoped again.** This PR previously proposed `--golden-snapshot-warmup`, a > tunable wall-clock delay before the golden checkpoint. Per discussion, that > direction is dropped: the answer for a workload that cannot report readiness > is a readiness endpoint — a small sidecar where the workload itself cannot be > changed — not a longer timer. What survives is the piece that discussion > agreed on, and which the previous revision already flagged as a follow-up: > making the readyz deadline itself configurable. > > The warmup work is not in this branch. It is kept locally in case a workload > genuinely cannot be given a readiness signal before GA, and would come back as > its own PR if so. ## Summary `readyz.Wait` polls until the container returns 200 or a **hardcoded 30s** elapses. A workload that legitimately takes longer to bind its HTTP server cannot be accommodated without raising the ceiling for every actor in the cluster, and losing that race fails the actor start. How long a workload takes to become ready is a property of that workload, so this makes the deadline a per-template setting rather than a package constant. Adds optional `timeoutSeconds` to `ContainerReadyz`. **Unset keeps today's 30s**, so no existing template changes behavior. ## Changes The value rides on the existing probe, so it follows the chain the probe already takes and no call site needs to know about it: `ContainerReadyz.timeoutSeconds` → `toAteletReadyz` → `ateletpb.Readyz` → `toAteomReadyz` → `ateompb.Readyz` → `readyz.Wait` - `pkg/api/v1alpha1/actortemplate_types.go` — `TimeoutSeconds *int32`, `+optional`, `Minimum=1`, `Maximum=3600`. - `internal/proto/ateletpb/atelet.proto`, `internal/proto/ateompb/ateom.proto` — `int32 timeout_seconds = 2` on both `Readyz` messages. - `cmd/ateapi/internal/controlapi/workload_spec.go`, `cmd/atelet/main.go` — pass it through the two conversions. - `internal/readyz/readyz.go` — `OverallTimeout` becomes `DefaultOverallTimeout` (still 30s) and `Wait` resolves its deadline through a new `overallTimeout(probe)` helper. - Regenerated: both `.pb.go`, `zz_generated.deepcopy.go`, and the `actortemplates` CRD. None of the four `readyz.WaitAll` call sites change. **On the zero value.** Unlike a warmup delay — where zero is a real request meaning "checkpoint immediately" — a zero readiness deadline could never be met, so it is never something a template author means. A non-positive value on the wire is therefore read as "unset" and falls back to the default, and the CRD field is a pointer with `Minimum=1` so the API rejects `0` outright rather than silently substituting 30s behind the author's back. **On bounding**, which was the open question left on the previous revision: bounded at `3600`. A template asking to wait longer than an hour for readiness is expressing a broken workload, not a slow one, and the bound keeps a typo from pinning a worker for a day. ## Verification - `go build ./...`, `go vet ./...`, `gofmt`, `go test ./...` — all pass. - `internal/readyz/readyz_test.go` — `overallTimeout` resolves unset and negative to the default and honors an explicit value; `Wait` against a port nothing binds gives up at the probe's 1s deadline rather than the 30s default. - `workload_spec_test.go`, `cmd/atelet/main_test.go` — the timeout crosses both conversions, and a probe without one stays zero on the wire. - `actortemplate_validation_test.go` — the bounds are enforced by a real API server. This suite runs under envtest against the generated CRD directory, so it exercises the regenerated `actortemplates` CRD rather than the Go markers: `300` is accepted, unset is accepted, and `0`, `-1` and `3601` are all rejected by apiserver schema validation. - **On a real cluster, via CI.** `internal/e2e/fixtures/probe` now declares a `readyz` probe with `timeoutSeconds: 60`, pointed at the `/healthz` the probe binary already serves on `:80`. The kind e2e that runs on every PR therefore exercises the value crossing ateapi → atelet → ateom on real binaries, across the auth matrix, on both the run and restore paths. This is also the readyz path's first e2e coverage — no fixture declared a probe before. Wire compatibility degrades safely in both skew directions: `timeout_seconds` is a new field 2 on a `Readyz` message that has only ever had field 1, so an old ateom ignores it and an old ateapi leaves it zero, which reads as the 30s default. No GKE run. What that would add over the above is a workload whose readiness genuinely exceeds 30s, and that is the readiness-sidecar work rather than this PR. Fixes #<issue_number_goes_here> > It's a good idea to open an issue first for discussion. - [x] Tests pass - [x] Appropriate changes to documentation are included in the PR --------- Co-authored-by: Maya Wang <mymaya@google.com>