fix(template): probe envd readiness before persisting envd capability#741
fix(template): probe envd readiness before persisting envd capability#741zyl1121 wants to merge 1 commit into
Conversation
Probe envd /health during template snapshot flows before reporting an envd version from Cubelet. This makes the envd capability annotation mean that the template has a usable envd service, not just an envd binary that may be stopped or still starting. Apply the same readiness check to both AppSnapshot and CommitSandbox. Only persist cube.master.components.envd.version when all READY replicas report the same valid, ready envd version. Otherwise, omit the annotation so later sandbox create flows do not rely on an optimistic capability signal. Signed-off-by: zhengyilei <zheng_yilei@qq.com>
zyl1121
requested review from
chenhengqi,
fslongjin,
ls-ggg,
tinklone and
up2wing
as code owners
zyl1121
changed the title
Review Summary for PR #741This PR improves envd capability detection by adding an HTTP readiness probe before persisting the envd version annotation. The code is clean and well-tested overall. Notable Issues1. runProbeCall goroutine leak on context cancellation (envd_version.go:93-116) 2. Serial latency on snapshot/commit paths (appsnapshot.go:169 template_ops.go:187) 3. Test coverage gap: reconcileTemplateEnvdVersion persist/no-op paths (store_test.go) 4. Missing IP validation (envd_version.go:319-328) 5. Fragile sentinel (envd_version.go:193) 6. ProbeConfig mutation (envd_version.go:213-224) Positive Highlights
4 inline comments posted on specific code locations. |
![cubesandboxbot[bot]](https://avatars.githubusercontent.com/u/20101770?s=60&v=4){kind=small}
|
|
||
| func (l *local) probeEnvdReadiness(ctx context.Context, sandboxIP string, port int) error { | ||
| sandboxIP = strings.TrimSpace(sandboxIP) | ||
| if sandboxIP == "" || sandboxIP == "<nil>" { |
There was a problem hiding this comment.
The "<nil>" string comparison is a fragile sentinel check — if the upstream source of sandboxIP changes its nil formatting (e.g. Go version upgrade, different serialization), this guard silently passes through a clearly invalid IP. Consider validating with net.ParseIP() here or (better) at the point of origin so the root cause is addressed rather than papered over.
| if cb == nil { | ||
| return "", fmt.Errorf("cubebox %s not found", sandboxID) | ||
| } | ||
| return strings.TrimSpace(cb.IP), nil |
There was a problem hiding this comment.
The sandbox IP is returned without validation via net.ParseIP(). While the IP originates from an internal store (not direct user input), adding net.ParseIP() here as defense-in-depth would prevent a corrupted store entry from causing the Cubelet to make HTTP requests to unintended hosts. The IP flows directly into buildEnvdReadinessProbe → formatURL → http.Client.Do, making this a potential SSRF vector if the store is ever corrupted.
| if client == nil { | ||
| client = newEnvdReadinessHTTPClient() | ||
| } | ||
| if cfg.Timeout <= 0 { |
There was a problem hiding this comment.
probeEnvdReadinessWithClient mutates the caller's *ProbeConfig by writing default values into cfg.Timeout, cfg.ProbeTimeout, cfg.Period, and cfg.FailureThreshold when they are <= 0. This is a surprising side effect — if a caller ever reuses a ProbeConfig across multiple probes, the first call permanently overwrites zero-valued fields, masking what the original caller-supplied values were. Consider using local variables for defaults instead of mutating cfg.
|
Perhaps we just need to use the |
I considered that too, but I think it would change the CLI contract too much. Today, This PR intentionally keeps those two concerns separate: the existing probe continues to represent workload readiness, while the envd probe is an internal capability check whose only effect is whether CubeMaster publishes the envd capability annotation. If we want to express envd readiness through probe configuration in the future, I think we should first add support for multiple probes, or introduce a dedicated internal probe slot, rather than overloading the existing user-facing one. |
2 participants
Motivation
In #566, create-time env injection started relying on
cube.master.components.envd.versionas the envd capability signal, using the mechanism introduced in #650. During review, we agreed that the main envd readiness guarantee should move to template construction time instead of rediscovering the same readiness race on every sandbox creation.Before this change, the template envd annotation could be written after only collecting
envd --version. That proved the binary existed, but not that the envd service was reachable and ready on the sandbox runtime endpoint.As a result, downstream sandbox create flows could treat a template as envd-capable even when envd was absent, stopped, or still starting.
Summary
http://<sandboxIP>:49983/healthbefore reporting an envd version during template snapshot flows. This uses a dedicated bounded HTTP probe instead of the generictelnet.Telnethelper so the readiness path can honor parent context cancellation and reject HTTP redirects explicitly.AppSnapshotandCommitSandbox, keeping image-based template creation and runtime commit paths consistent.cube.master.components.envd.versiononly when all READY replicas report the same valid envd version; otherwise, it suppresses the annotation instead of publishing an optimistic capability signal.templateIDand the stricter convergence check fails, CubeMaster now clears any previously persisted envd annotation instead of leaving stale capability data behind.Validation
go test -vet=off ./pkg/templatecenter -run 'Envd|Template' -count=1go test -vet=off ./services/cubebox -run 'Envd|Probe' -count=1Manual validation on Linux:
success path: without the temporary REJECT rule, template info showed:
After enabling INFO logs, Cubelet also logged:
failure path: with a temporary REJECT rule blocking Cubelet ->
sandboxIP:49983, template build still succeeded, but the envd annotation was omitted. Cubelet logged:redo path: running
cubemastercli cubebox template redoagainst the sametemplateIDalso updated the annotation as expected. A template that previously carriedcube.master.components.envd.versionlost it after a redo under the REJECT rule, confirming that redo re-evaluates and reconciles the envd capability signal instead of leaving the previous value sticky.Scope
This change is limited to the template/snapshot paths that produce and persist the envd capability annotation. It does not change sandbox create-time env var propagation semantics from #566.
Follow-up
A later follow-up can make envd capability suppression visible to users, so a missing annotation can distinguish between "envd is absent" and "envd was detected but not ready during template-time probing". That would let users understand why the template is not marked envd-capable and decide whether to run
redofor transient startup jitter.