Model distributed runner startup as boot capabilities - #1161
Conversation
The standalone runner still hid cluster access, storage recovery, host restoration, agent activation, and node readiness behind Runner.Start. That made its graph a shallow wrapper around the lifecycle the graph was supposed to own. Give each runner capability its own node, matching server startup. Let cluster access publish the remote workload issuer before telemetry starts, and publish node presence only after both agents are active. Reconstitute Runner afterward as a role-level handle rather than a lifecycle owner. Move the containerd adapter alongside the component so server and runner share daemon and client ownership without sharing composition-specific wiring.
|
Warning Review limit reachedNext included review available in 29 minutes. View limit detailsLimit details: You’ve used all 3 included reviews currently available. Your 79 included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (26)
Comment |
![miren-code-agent[bot]](https://avatars.githubusercontent.com/u/174767976?s=60&v=4){kind=small}
There was a problem hiding this comment.
🍪 biscuit: ✅ ready to merge — auto-review, non-blocking
This is a draft PR. The change is structurally sound and I'm comfortable recommending it graduate to human review.
What it does. The PR lifts the distributed-runner's flat startup sequence in runner_start.go into a proper boot graph modelled after the existing components/server pattern. ~300 lines of imperative init code give way to a set of typed boot nodes (clusterAccess → nodeStorage + telemetry → sandboxHost → storageAgent + sandboxAgent → nodePresence) wired through the pkg/boot graph. components/containerd gains its own Boot node reusable by both the server and the distributed runner, eliminating the components/server/boot_containerd.go file entirely.
What I verified.
- The dependency ordering in
startup.gomatches what the old imperative code did. The graph's failure cleanup path inruntime.Startcorrectly callsruntime.Stopon any partial start, which propagates to each boot node'sstop(). - The
b.client.HTTPdereferences inboot_telemetry.golines 93 and 107 are both reachable only whenb.client != nil(the outer guard at line 70 sets it when either address is non-empty, and the inner checks are strict subsets of that condition). No nil dereference. stop()incontainerd/boot.gois called by the graph's cleanup path even on startup failure (viaruntime.Stopin theStarterror branch).b.result.Clientis set before the readiness check, butstop()closes it, so there's no leak.stopTimeout=0in the distributed-runner's storage/sandbox/presence boot calls defers to the graph's outer timeout rather than cutting immediately — thepkg/boot/lifecycle.gocode only adds a timeout whenstopTimeout > 0.- The
resolveRunnerContainerdhelper correctly returns("", "", nil)for an external socket, andcontainerdBootConfiginboot_containerd.goroutes toExternalBootConfigin that case. The redundant path reconstruction inEmbeddedBootConfigwhenbinDir != ""is harmless. - Tests cover the graph-validates path, the cascade-failure path (containerd failure prevents sandbox host from starting), and all four telemetry configuration combinations. The
stubIssuerin the test correctly exercises the token-source arm path.
One minor note. boot_containerd.go passes a socketPath into EmbeddedBootConfig that is computed by the caller as filepath.Join(options.DataPath, "containerd", "containerd.sock"), but EmbeddedBootConfig also defaults to the same path when its socketPath argument is empty. The explicit argument is harmless — just slightly redundant. Not worth blocking on.
The refactoring is clean and the server startup path is an established reference point for this pattern. The change is ready for human review.
🍪 full review note · comment /biscuit review to run biscuit again.
| tokenSource.SetIssuer(issuer) | ||
| } else if telemetryClient != nil { | ||
| ctx.Log.Error("no workload identity issuer available; telemetry cannot be shipped") | ||
| stop := func() { |
There was a problem hiding this comment.
Why is this a function that is just called one place rather than being down below at the call site?
2 participants
#1158 exposes the runner’s startup phases as capabilities, but the standalone command would still hide them behind
Runner.Start. That would make distributed startup look graph-managed while its lifecycle and readiness boundaries remained implicit.This carries the capability model through distributed startup. Cluster access and containerd begin independently; storage and telemetry wait for cluster access; the sandbox host waits for their ready outputs; and node presence is published only after both agents are active. Telemetry now receives the coordinator-backed workload issuer before its writers start.
The shared storage-agent, sandbox-agent, and node-presence adapters live alongside the runner capabilities. Containerd’s boot adapter is shared by server and runner composition too. The CLI keeps command policy, signal handling, and the final background-task wait, while the graph owns startup rollback and reverse teardown.
Focused component and CLI tests pass, including containerd’s integration test, and repository lint is clean.
Closes MIR-1690