Make initial agent bootstrap repeatable with strict reset - #742
Open
Philip Lombardi (plombardi89) wants to merge 38 commits into
Open
Philip Lombardi (plombardi89) wants to merge 38 commits into
Philip Lombardi (plombardi89) wants to merge 38 commits into
Conversation
…into acl-extract/bootstrap-recovery
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Resume and reset paths can incorrectly trust incomplete state, lose ownership after failure, or persist configuration that was never applied.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds resumable, ownership-aware agent bootstrap, serialized host lifecycle operations, completed-install daemon repair, and strict retryable reset behavior.
Changes:
- Introduces durable installation state, locking, and checkpointed bootstrap stages.
- Adds ownership-aware rootfs/node replay, daemon repair, and stricter cleanup.
- Expands unit and end-to-end recovery coverage and documentation.
File summaries
| File | Description |
|---|---|
pkg/agent/phases/rootfs/provision.go |
Adds owned rootfs replay. |
pkg/agent/phases/rootfs/oci/task.go |
Rebuilds incomplete owned rootfs trees. |
pkg/agent/phases/rootfs/oci/task_test.go |
Tests rootfs replay behavior. |
pkg/agent/phases/rootfs/nspawn.go |
Selects owned OCI provisioning. |
pkg/agent/phases/reset/strict_test.go |
Tests strict cleanup failures. |
pkg/agent/phases/reset/routes.go |
Enumerates and removes owned routes. |
pkg/agent/phases/reset/nspawn.go |
Tightens nspawn and bpffs cleanup. |
pkg/agent/phases/reset/network.go |
Propagates network cleanup failures. |
pkg/agent/phases/reset/machine.go |
Verifies machine teardown. |
pkg/agent/phases/reset/helpers.go |
Returns filesystem cleanup errors. |
pkg/agent/phases/nodestart/preflight_bind_address.go |
Validates listener ownership. |
pkg/agent/phases/nodestart/preflight_bind_address_test.go |
Tests owned listener detection. |
pkg/agent/phases/nodestart/preflight_api_server.go |
Enables ownership-aware bind checks. |
pkg/agent/phases/nodestart/nspawn.go |
Preserves running nspawn instances. |
pkg/agent/phases/nodestart/nspawn_test.go |
Tests replay-safe node startup. |
pkg/agent/phases/host/preflight_existing_deployment.go |
Exports the clean-host check name. |
pkg/agent/installstate/store.go |
Implements durable installation records. |
pkg/agent/installstate/store_test.go |
Tests state admission and lifecycle. |
pkg/agent/installstate/mutation.go |
Adds lifecycle mutation admission. |
pkg/agent/installstate/lock.go |
Implements the installation lock. |
pkg/agent/bootstrap/coordinator.go |
Coordinates checkpointed bootstrap. |
pkg/agent/bootstrap/coordinator_test.go |
Tests resume and repair flows. |
internal/provision/assets/unbounded-agent-install.sh |
Runs bootstrap from a staged binary. |
internal/fsutil/fsutil.go |
Adds atomic and durable filesystem helpers. |
internal/fsutil/fsutil_test.go |
Tests filesystem helpers. |
hack/agent/e2e-kind/test_reliability.py |
Tests recovery harness behavior. |
hack/agent/e2e-kind/e2e.py |
Adds bootstrap recovery and repair scenarios. |
docs/content/guides/agent.md |
Documents retry, repair, and reset. |
cmd/agent/internal/daemon/reset.go |
Adds locked, durable reset orchestration. |
cmd/agent/internal/daemon/reset_test.go |
Tests reset ownership retention. |
cmd/agent/internal/daemon/persist_config.go |
Removes obsolete cleanup helper. |
cmd/agent/internal/daemon/nodeoperator.go |
Integrates reset ownership handling. |
cmd/agent/internal/daemon/migration_test.go |
Tests startup lock contention. |
cmd/agent/internal/daemon/lifecycle.go |
Adds bootstrap binary repair and strict cleanup. |
cmd/agent/internal/daemon/lifecycle_test.go |
Tests binary installation behavior. |
cmd/agent/internal/daemon/installation_test.go |
Tests controller lock contention. |
cmd/agent/internal/daemon/daemon.go |
Serializes startup discovery and migration. |
cmd/agent/internal/daemon/controller.go |
Injects installation state into reconcilers. |
cmd/agent/internal/daemon/controller_test.go |
Updates reconciler fixtures. |
cmd/agent/internal/daemon/controller_node.go |
Locks repave reconciliation. |
cmd/agent/internal/daemon/controller_machineoperation.go |
Locks host mutation operations. |
cmd/agent/internal/daemon/assets/unbounded-agent-daemon-recovery.sh |
Documents lock-free rollback behavior. |
cmd/agent/internal/cmd/testdata/bootstrap-v1/resetting.json |
Adds resetting-state fixture. |
cmd/agent/internal/cmd/testdata/bootstrap-v1/preparing-rootfs.json |
Adds rootfs checkpoint fixture. |
cmd/agent/internal/cmd/testdata/bootstrap-v1/input.json |
Adds bootstrap identity fixture. |
cmd/agent/internal/cmd/testdata/bootstrap-v1/complete.json |
Adds completed-state fixture. |
cmd/agent/internal/cmd/start.go |
Uses the bootstrap coordinator. |
cmd/agent/internal/cmd/reset.go |
Delegates to strict reset. |
cmd/agent/internal/cmd/preflight.go |
Makes preflight ownership-aware. |
cmd/agent/internal/cmd/bootstrap.go |
Implements concrete bootstrap stages. |
cmd/agent/internal/cmd/bootstrap_test.go |
Tests identity compatibility. |
cmd/agent/internal/cmd/agentupgrade.go |
Locks host-driven activation. |
cmd/agent/internal/cmd/agentupgrade_test.go |
Tests upgrade lock acquisition. |
.github/workflows/agent-e2e-kind.yaml |
Adds Ubuntu bootstrap-recovery CI. |
Review details
- Files reviewed: 54/54 changed files
- Comments generated: 6
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return err | ||
| } | ||
|
|
||
| if err := phases.Serial(s.log, daemon.PersistAppliedConfig(s.log, "kube1", &s.cfg.AgentConfig), daemon.EnableDaemon(s.log)).Do(ctx); err != nil { |
Collaborator
Author
There was a problem hiding this comment.
Fixed in 2e186af. The fingerprint now hashes a canonicalized reference: for an https:// rootfs archive the query is dropped and trailing path slashes trimmed, so a refreshed signature is the same installation while the host and path still identify the artifact. Registry and oci-layout references are unchanged. Covered by TestBootstrapFingerprintIgnoresSignedImageQuery and TestCanonicalImageIdentityLeavesNonHTTPSReferencesAlone.
Comment on lines
+129
to
+133
| if err := os.Remove(s.StatePath()); err != nil && !errors.Is(err, os.ErrNotExist) { | ||
| return err | ||
| } | ||
|
|
||
| return fsutil.SyncDir(s.root) |
Collaborator
Author
There was a problem hiding this comment.
Fixed in f9830e4. Remove reads the record before unlinking and restores it if the directory sync fails, so a reset that reports failure always leaves the host visibly owned instead of letting the next start be admitted as fresh. A record that does not load is not restored, since it granted no usable ownership and admission rejects it either way. TestRemoveRestoresOwnershipWhenUndurable and TestRemoveDoesNotRestoreUnusableOwnership cover both. The file moved to cmd/agent/internal/installstate/store.go earlier in the branch.
Comment on lines
+66
to
+70
| if _, err := os.Stat(filepath.Join(d.machineDir, ".unbounded-rootfs-complete")); err == nil { | ||
| return nil | ||
| } else if !errors.Is(err, os.ErrNotExist) { | ||
| return err | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Leaving this one as is. Forging the marker requires an image that contains .unbounded-rootfs-complete at its root, and that image is the artifact the operator has chosen to become the node's entire root filesystem. In the compromised-image threat model the attacker already controls everything the marker would protect, so the marker buys nothing; in the accidental case, a file with that exact name appearing by coincidence is not a realistic failure. Moving it out would mean threading a path through ProvisionOwned and DownloadOwnedRootFS plus adding reset cleanup, which is not worth it for that risk. Worth noting the marker is only written after a successful extraction and filesystem sync, so a partial unpack of ours cannot produce one.
| KubernetesVersion string | ||
| OCIImage string | ||
| APIServer string | ||
| }{strings.TrimPrefix(cfg.Cluster.Version, "v"), cfg.OCIImage, cfg.Kubelet.ApiServer}) |
Collaborator
Author
There was a problem hiding this comment.
Good catch, this was a real bug. Fixed in 2e186af by moving PersistAppliedConfig out of the daemon stage and into the stage that starts the node, after kubelet bootstraps. The record now always describes the configuration the running node was built from, and a retry that resumes at installing-daemon skips that stage entirely so it cannot overwrite it. I preferred this over widening the fingerprint, which would have made a rotated token or CA demand a full reset again. AgentConfigDir moved into the node stage's SyncFilesystems too, so the file is durable before that checkpoint is taken. TestNodeStartPersistsAppliedConfig pins both the placement and the ordering, and the recovery e2e now retries with a changed node label and requires the recorded config to stay byte-identical.
Comment on lines
+116
to
+120
| if _, err := os.Lstat(goalstates.DaemonBinaryPath); err == nil { | ||
| return nil | ||
| } else if !errors.Is(err, os.ErrNotExist) { | ||
| return err | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Fixed in f9830e4. The check now resolves symlinks and requires a regular file with an executable bit, so a dangling link or one aimed at a non-executable installs the staged binary instead of being mistaken for a healthy layout. That was the state that sent a completed install into repair, which could not replace the link either. The healthy BinaryPath -> CurrentPath -> slot chain still resolves and short-circuits. Covered by TestUsableDaemonBinaryRequiresAResolvableExecutable.
Comment on lines
+33
to
+34
| if err := executil.RunCmd(ctx, t.log, executil.Machinectl(), "disable", t.machineName); err != nil { | ||
| t.log.Warn("failed to disable machine (may not have been enabled)", "machine", t.machineName, "error", err) | ||
| t.log.Warn("failed to disable machine; continuing with stop and removal", "machine", t.machineName, "error", err) |
Collaborator
Author
There was a problem hiding this comment.
Fixed in f9830e4. A failed machinectl disable is now tolerated only when systemctl show --property=UnitFileState positively reports a state that cannot start the unit at boot (disabled, not-found, masked, static, indirect, or no unit file). Anything else, including an inspection that fails to answer, returns the error so ownership stays resetting. Covered by TestConfirmNotEnabledOnlyAcceptsUnstartableStates and TestStopMachineFailsWhenDisableLeavesUnitEnabled; a live reset was also checked to leave both units reporting disabled.
pkg/agent was extracted in #76 so external consumers could reuse the agent as a library, and that same change deliberately moved host-lifecycle operations such as EnableDaemon, StopDaemon and PersistAppliedConfig back out of the public tree into cmd/agent/internal/daemon. installstate and bootstrap belong on that side of the line. They manage this agent's own record and lock at its own paths, and every consumer is already under cmd/agent. Keeping them private also avoids publishing an on-disk schema and admission policy that is about to change: Record validation currently accepts only the default install prefix, which the configurable-prefix work alters. Promoting an internal package later is not a breaking change, so this can be revisited if an external consumer needs resumable bootstrap.
A reviewer had to ask what this package is for, so document it: the record and lock answer whether an installation exists and is ours, how far it got, and whether anything else is mutating the host. Now that nothing outside cmd/agent can reach it, drop the accumulated surface. Checkpoint.NodeMayBeRunning was dead once bind-address preflight became unconditionally ownership-aware. StatePath, AcquireLockAt and DefaultLockPath had no callers outside the package. Decide is folded into Admit so admission has a single entry point; the fixture test now admits through a store, which also proves the record survives a load round-trip.
Sweeping the exported symbols this branch adds turned up several with no caller outside their own package, including two in the public pkg/ tree. CheckOwnedBindAddress was exported so the command layer could substitute checkers by name. That substitution is gone now that nodestart.Preflight is unconditionally ownership-aware, leaving Preflight as the only caller. CheckBindAddress became unreachable at the same time, so remove it and rename the three tests that were named after it; they exercise the checker type directly rather than the constructor. daemon.ResetAgentResources had no production caller either: the node operator calls resetUnderLock directly because the MachineOperation already holds the lock. Only a test still referenced it, so drop it and have that test compose resetResources itself. The identically named nodeOperator interface method is unaffected. Also unexport fsutil.WriteFile, which only WriteFileDurable uses, and the installstate schemaVersion and defaultHostPrefix constants. The exported Record.SchemaVersion field stays because the JSON format needs it.
Two problems with what a retry is allowed to change and what it records. An HTTPS rootfs reference can carry an expiring signed query, so hashing it whole made a refreshed signature look like a different installation and stranded the retry. Hash the scheme, host and path instead, which is what actually selects the artifact; the reference is also trailing-slash normalized to match how it gets resolved before being fetched. The applied config was persisted in the daemon stage, so a retry that resumed there recorded the new attempt's configuration even though the node had already been started from the old one. Drift is measured against that file, so the node kept the superseded labels, taints and kubelet configuration with nothing left to reconcile them. Persist it in the stage that starts the node, after kubelet bootstraps, and sync the directory holding it before that stage is checkpointed. The stage composition is now asserted by name so the placement cannot regress silently, and the recovery e2e retries with a changed label and requires the recorded config to stay untouched.
Three places reported success while leaving the host in a state that contradicts the report. Dropping the ownership record is itself a durable step. The record was unlinked and the error from the directory sync returned, so a failure left the removal in page cache only: reset told the operator it failed, while the next start was admitted as a fresh install onto a host that was only partially torn down. Restore the record when the removal cannot be made durable, so a failed reset always leaves the host visibly owned. The bootstrap binary was considered present whenever anything existed at its path, including a dangling symlink or one aimed at a non-executable. That is precisely the state that sends a completed install into repair, and repair cannot replace a bad link either, so the host was stranded. Resolve the link and require a regular executable. Ignoring every machinectl disable failure could leave the nspawn unit enabled. The enablement symlink outlives the config and rootfs that reset deletes, so the host would try to start a machine that no longer exists on the next boot. A failed disable is now tolerated only when systemd positively reports a state that cannot start the unit, and an inspection that does not answer counts as unconfirmed.
The record carried a host prefix that nothing in this change reads. It was written on every install and then rejected by validation unless it held the one value this release can produce, purely to reserve the field for the configurable-prefix work that follows. Recording something we refuse is a confusing contract to ship, and the follow-up does not need it reserved: an absent prefix already means the default, so a record written here reads correctly once the field exists. Validation still refuses a record whose schema version it does not know, which is the same protection by a mechanism that is already there. Nothing else changes. The fingerprint never covered the prefix, so installation identity is unaffected and the compatibility fixtures keep their recorded value.
The installer stopped writing /usr/local/bin/unbounded-agent and delegated it to InstallBootstrapBinary, which this same change introduces inside start. That coupled two components that are versioned independently. The agent version comes from AGENT_VERSION, from AGENT_URL, or from the default of tracking the latest published release, while the installer is embedded in and served by metalman. Any agent older than this change therefore never writes the binary, and bootstrap fails at enable-daemon with "no executable agent binary found for daemon link initialization", naming neither the installer nor the version skew. That is not a corner case. A Machine that sets no agent URL or version, the documented default, downloads the latest release, so every existing user breaks the moment a metalman carrying this installer is deployed, and stays broken until a release containing it exists. The metalman layered HTTP smoke test has been failing on this branch since its first commit for exactly this reason: it supplies its own cloud-init user data without the install environment, so it exercises the released agent. Seed the binary again, but only when the path holds nothing usable. The test follows symlinks, so a host this installation already owns resolves through the compatibility symlink to a live blue-green slot and is skipped, which keeps the property the original change wanted: admission still runs from the staged executable and a retry cannot overwrite a live link before its intent has been accepted. A dangling link resolves to nothing and is replaced, matching how the daemon decides whether an existing binary is usable, since install would otherwise write through it to a stale location. The uninstall script already removed this path, so the two are symmetric again. The existing script test only asserted that the download overrides are honored, which is why the install could disappear unnoticed; it now pins both the install and the guard.
hbc (bcho)
reviewed
Sep 17, 2026
|
|
||
| syncAttestedKubeletConfig(&s.cfg.AgentConfig, s.gs.NodeStart) | ||
|
|
||
| if s.reporter == nil { |
Member
There was a problem hiding this comment.
nit: any special reason why we want to have an optional reporter and create on demand? I also saw some checks like if s.reporter != nil. Wondering if we can just create it in constructor and hold a non-nil ref for the whole lifecylce
Collaborator
Author
There was a problem hiding this comment.
Good question - it cannot move, and I have added a comment at the construction site saying why in b32be689.
The reporter captures credentials when it is built: NewBootstrapStatusReporter reads cfg.Kubelet.Auth.BootstrapToken, and when that is empty it returns a permanently degraded reporter that logs and skips every update. It also registers the Machine over the API as part of construction. On an attested host the token does not exist until ApplyAttestation has run immediately above this, so constructing it in the constructor would silently disable status reporting for the entire bootstrap on every attested host, and would issue the registration call before admission and before any host preparation.
You are right about the nil checks though - two of them were doing nothing. The reporter already reports through a nil-receiver check in set, so StageFailed and the Succeeded call now invoke it directly. Since the callers now depend on that property rather than guarding it, I pinned it with a test instead of leaving it incidental.
I left the two checks in coordinator.go. Its reporter is an interface rather than a concrete pointer, so a nil value there cannot absorb a method call, and the coordinator tests pass nil deliberately.
|
|
||
| // acquireLockAt is nonblocking. The kernel releases flock on process exit; a | ||
| // leftover lock file does not imply a held lock and must not be deleted by reset. | ||
| func acquireLockAt(path string) (*Lock, error) { |
Member
There was a problem hiding this comment.
do we want to use something like https://github.com/gofrs/flock ?
Collaborator
Author
There was a problem hiding this comment.
Agreed, done in 6627c6ea.
It turned out to be a cheap trade: gofrs/flock was already in the dependency graph, pulled in indirectly by the OCI SDK, so this promotes an existing entry to a direct requirement rather than adding a module. go.sum is unchanged.
I checked the swap preserves what the callers depend on, since the lock is load-bearing across reset, the daemon, both controllers, agent upgrade and the bootstrap coordinator:
setFhopens with the same0600mode, and takes the sameLOCK_EX|LOCK_NB.Closeis documented as not removing the file, which is the contract reset depends on: a leftover lock file must never be read as a held lock, pinned byTestInstallationLockSurvivesStateRemoval.TryLockreports contention as afalsereturn rather thanEWOULDBLOCK, so that is mapped onto the existing sentinel error.- The parent directory is still created here, because the library does not do it.
We also gain something: it reopens and retries on a stale file handle, which the previous implementation treated as a hard failure.
The existing lock tests pass untouched, which is the result I wanted - acquireLockAt builds a fresh flock.New per call, so a second acquire in the same process still opens a separate file description and contends as before.
One thing worth flagging for review rather than leaving incidental: NOTICE is regenerated and gains a BSD 3-Clause entry, because the notice generator collects direct dependencies only.
| Initial bootstrap records installation ownership before changing the host. If a | ||
| stage fails or the process is interrupted, rerun the same saved bootstrap script | ||
| or invoke `unbounded-agent start` with the same original configuration. Completed | ||
| stages are skipped; unfinished stages are replayed. A running nspawn machine is |
Member
There was a problem hiding this comment.
for the "completed stages", I think current code is using the stored config as source of truth? Will that be a case where the host got mutated after the first attempt of start, which resulted in drifted state?
Collaborator
Author
There was a problem hiding this comment.
hbc (@bcho) are you thinking about this from the perspective of there was a manual touch that drifted state? It should just pick up and continue but if something (human, external process) comes in and modifies the host after a stage has been completed then it won't be replayed because the install-state.json tracks what work it already successfully completed so something undoing a previous step won't get fixed on a second run but it will fail.
Collaborator
Author
There was a problem hiding this comment.
Following up on this: you were right, and it turned out to be worth fixing rather than documenting.
The record no longer stores how far an attempt got. checkpoint became phase, with only three values (installing, complete, resetting), and all four stages now run on every attempt. Each one decides from the host what it still has to do:
- packages already present are not reinstalled
- a rootfs a machine is registered from is left in place rather than rebuilt
- a running machine is left running
- node service configuration is rewritten, and the service restarted only if the content actually changed
- the nftables ruleset a running node depends on is not flushed
So the case you asked about, a host mutated after a stage completed, is now repaired instead of skipped past, because nothing is assumed from the previous attempt. A record that only names the phase cannot disagree with the host, which was the underlying problem with keeping stage state at all.
Two things fell out of this that are worth flagging:
The applied config is the one place that deliberately does not reconverge. A retry that finds the machine already registered leaves <machine>-applied-config.json alone, because it did not build what is running. Otherwise a changed node label would be recorded as applied, and since kubelet only takes --node-labels at registration, the node would never actually get it while the record claimed otherwise, suppressing the drift repave that would have delivered it.
The reapply also has to restart services whose config it rewrote, or the files on disk and the running processes disagree with nothing to reconcile them. That is restart-reconfigured-services, and it only fires on a real content change, so an unchanged retry costs nothing. Verified on a live node: an unchanged retry restarted neither containerd nor kubelet and left both PIDs untouched, and a retry carrying a changed label restarted kubelet only.
Thanks for pushing on this one.
Review asked why the status reporter is optional and created on demand rather than held from construction. It cannot move: the reporter captures credentials when it is built. An empty bootstrap token makes it a permanent no-op, and it registers the Machine over the API as part of construction. On an attested host the token does not exist until ApplyAttestation has run, so building it in the constructor would silently disable status reporting for the whole bootstrap and issue the registration call before admission. Say so at the construction site so the next reader does not have to work it out. The nil checks around the reporting calls were doing nothing, though. The reporter already reports through a nil-receiver check, so the two guarded call sites now call it directly. That property is what the callers depend on now, so it is pinned by a test rather than left incidental. The coordinator keeps its checks: its reporter is an interface, which cannot absorb a call when nil, and its tests supply nil deliberately.
Review suggested using a maintained library rather than a hand-rolled flock wrapper. It is a good trade here: the module is already in the dependency graph, pulled in indirectly by the OCI SDK, so this promotes an existing entry to a direct requirement rather than adding one. go.sum is unchanged. The swap preserves the behavior the callers depend on. The library opens the lock file with the same 0600 mode, takes the same non-blocking exclusive flock, and documents that closing does not remove the file, which is the contract reset relies on: a leftover lock file must never be read as a held lock. TryLock reports contention as a false return rather than EWOULDBLOCK, so that is mapped onto the existing sentinel error, and the parent directory is still created here because the library does not do it. Behavior gained: the library reopens and retries on a stale file handle, which the previous implementation treated as a hard failure. NOTICE is regenerated, and gains a BSD 3-Clause entry for the new direct dependency. That is a licensing surface change riding in this PR rather than an incidental one, so it is called out here.
ConfigureNFTables starts nftables-flush.service, whose unit applies `flush ruleset` and erases every nftables rule on the host. That is the point on a fresh host, and it is safe at boot because the unit is ordered before the nspawn machine and unbounded-localdns-network.service re-adds its NOTRACK table after it. Starting it imperatively mid-run is a different thing. The nspawn container shares the host network namespace, so a running node's kube-proxy and CNI rules are in the ruleset being erased, along with LocalDNS's table. Nothing puts LocalDNS back: systemd ordering only sequences units within a single transaction, so starting this unit alone does not pull in the LocalDNS unit, and that unit otherwise runs only when the machine starts. kube-proxy resyncs on its own; LocalDNS does not, so it stays broken until the machine restarts. Only start the unit when no machine is registered. The flush exists to hand a clean slate to a node that has not started yet, so once one is registered it has already served its purpose. Installing and enabling the unit is unchanged, so the next boot still gets its clean slate in the right order. Inspection failure is not read as "nothing registered", since that would flush a ruleset a running node may depend on. This makes the task safe to re-run against any host state, which is a prerequisite for a retry that reapplies work rather than trusting a record of what was already done.
Reapplying node configuration writes files that a running service has already read. When this sequence boots the machine that is harmless, because the services start afterwards and read the new files. When it runs against a machine that is already up it is not: the files on disk and the running services would disagree with nothing to reconcile them, which is worse than not reapplying at all. Track whether each configuration file actually differed, and restart the service that reads it only when something did and the machine was already running. An identical reapply, which is the ordinary case when bootstrap is rerun after a failure, leaves the node completely alone. WriteFileIfChanged reports whether it had to write. Only content is compared: WriteFile preserves an existing file's permissions rather than resetting them, so a drifted mode cannot be corrected there, and reporting it as a change would restart the reader on every call forever. containerd restarts before kubelet, since kubelet talks to it and would otherwise just retry against a runtime that is coming back up. The exported ConfigureContainerd and ConfigureKubelet keep their signatures. StartNode builds the tasks concretely instead, because it is the only place that knows both whether the configuration changed and whether the machine was already running. This makes the node-start sequence safe to reapply against a live node, which is a prerequisite for a retry that reapplies work rather than trusting a record of what was already done.
ProvisionOwned rebuilds a rootfs in place and must never be pointed at a slot that has started a node; its own doc says so, because doing that pulls the filesystem out from under a running one. This stage guarded that by refusing outright when a machine was registered. Refusing is the wrong answer to the question being asked. A registered machine means the rootfs this stage would build is already built and in use, so the requirement is met and there is nothing to do. Skip it and continue. The condition is a property of the host rather than of how far a previous attempt got, so it holds whether the machine was started by an earlier attempt of this installation or independently afterwards. This makes the stage safe to reapply against any host state, which is a prerequisite for a retry that reapplies work rather than trusting a record of what was already done.
…-recovery # Conflicts: # go.mod
The record tracked which stage bootstrap had completed, and a retry resumed from there. That made it a claim about the host: "host preparation is finished" can stop being true without anyone noticing, and a retry that trusted it skipped work the host no longer had. The failure surfaced later, at the first stage that needed the missing thing, naming the symptom rather than the cause. Reapply every stage instead. Each already decides what to do by looking at the host: host preparation leaves a live nftables ruleset alone, the rootfs is left in place when a machine is registered from it, an already running machine is not restarted, and node services are restarted only when their configuration actually changed. A host that drifted between attempts is now repaired rather than skipped past. What is left worth persisting is which mode we are in, not how far we got. Installing says only that an installation is under way, so there is nothing in it that can go stale. Complete routes a later start to verify and repair from the applied config, which matters because after an ordinary repave the bootstrap inputs describe a retired slot. Resetting is the one thing the host cannot be asked: a half-removed installation and a half-built one look identical, because direction of travel is not observable. Stage names survive as a label for status reporting and logs, but are not written down. Recording the stage is precisely what let the record disagree with the host; reporting it costs nothing and keeps the Machine condition specific. That reporting also had a gap. wait-for-kubelet-bootstrap is its own task inside the node-start stage, and the classifier only matched start-kubelet, so the most common real failure - a rejected token, an unreachable API server, a CA mismatch - was reported as a generic failure. Main reports KubeletBootstrapFailed for it, so this restores parity. The record format changes and the fixtures change with it. Nothing has shipped, so no migration is owed.
The recovery scenario asserted the record had reached a named stage before the retry, and reached complete after. The record no longer says how far an attempt got, because that was a claim about the host rather than a fact about it. Assert what the host shows instead. The failure landing late is proved by the node being up, and the retry converging around it is proved by the nspawn PID being unchanged, the installation ID being the same, and the applied config staying byte-identical while the retry carries a changed label. That is the same evidence as before, taken from the thing it is actually about.
The guide said completed stages are skipped and unfinished ones replayed, which described the record as the source of truth for what had been done. Every stage now runs on every attempt and decides from the host what it still has to do, so say that, and say what each stage looks at: packages already present, a rootfs a machine is registered from, a running machine, configuration that did not change, a live nftables ruleset. That is also the answer to the question this section invited, which is what happens when the host changed between attempts. It is repaired rather than skipped past, because nothing is assumed from how far a previous attempt got.
…de up Every stage reapplies now, so the node stage runs again even when the node is already running, and it was rewriting the applied config on the way through. The old code could not do this: a retry resumed past the node stage, so the comment justifying the write said it was unreachable on a retry. That is no longer true, and the write is wrong. The applied config records what the running node was built from, and the daemon diffs it against the desired config to decide whether to repave. An attempt that finds a machine already registered did not build that node and has nothing to say about how it was built. The case that shows the harm is a changed node label. Labels sit outside the installation fingerprint, so a retry carrying a new one is admitted, but kubelet takes --node-labels at registration and a restart under an existing node does not revise them. Recording the new label would make applied match desired, which reads as no drift, which suppresses the repave that is the only thing that would have delivered it. The label would be lost silently. Leaving the record alone keeps the difference visible and lets the daemon resolve it. Gate the write on whether a machine was registered when the stage began, asked before the stage runs because afterwards the answer is always yes. PrepareRootFS already asked the same question, so both now share one helper. Repave is unaffected; it persists explicitly, outside this composition. Also sweep the vocabulary the phase change left behind. The installstate package doc still offered "how far did the last attempt get" as a question it answers, which is now precisely the question it refuses to answer.
Four small things, no behavior change. The kubelet and containerd tasks had grown identical change-tracking: same field, same comment, and a byte-identical write method. They share an embedded changeTracker now. Both the nftables task and bootstrap had grown their own loop over the two node slots calling RegisteredMachine, which inventories machinectl on every call, so each scan spawned two processes and a bootstrap attempt six. reset now offers FirstRegisteredMachine, which inventories once and returns the occupied slot, and both callers use it. restartReconfigured reached its three collaborators three different ways: it held the two configure tasks and read a field off each, but took a *bool aimed at a local in StartNode. It holds the start task now and reads wasRunning the same way, which drops the pointer, the local, and four nil checks no caller could reach. The coordinator converted its stage labels to string to build the loop and back to Stage twice to report them. The field is typed. Two of these turned out to be untested rather than merely undertested, which is why they are here rather than left alone. Nothing failed when FirstRegisteredMachine was made to never find a slot, and nothing failed when the machine-was-running observation was hardcoded either way, so the handoff that decides whether a reapply restarts a reconfigured service was resting on nothing. Both are now pinned, including that the scan inventories once, which is the reason it exists.
Philip Lombardi (plombardi89)
changed the title
An earlier commit on this branch swept exported symbols with no caller and removed this one. The sweep was right that nothing calls it: Preflight is unconditionally ownership-aware and uses checkOwnedBindAddress. It was wrong that this made it dead, because the symbol is on main in a pkg/ package, so removing it breaks anyone outside this repository who composes their own preflight set. Restore it with main's signature and behavior. The checker already guards c.owned != nil, so leaving owned unset gives the original meaning: any listener fails, including one that does belong to this installation. Comparing the exported surface of pkg/... against main, this was the only removal on the branch; everything else there is additive. The reason the sweep could not see it was that the tests exercised the checker type directly, so the constructor had no caller of any kind. Test it through the exported constructor instead, which both documents why it stays and fails the build if it is removed again.
The daemon refuses to run while an installation owns the host, which is right: it must not reconfigure a machine a bootstrap is still changing. It refused by returning an error, which is wrong, because systemd cannot tell that apart from the binary being broken. What followed was Restart=always, three starts inside StartLimitBurst, the unit in failed state, and OnFailure running the last-resort binary rollback for a problem the binary does not have. Where a previous AgentUpgrade had left a last-good binary, that rollback silently downgraded the agent. Two ways in, and the second is the common one. A bootstrap can die between starting the daemon and marking the record complete. More often, an install interrupted after the daemon was enabled means every later boot starts the daemon alongside a retry bootstrap that holds ownership for minutes while it downloads a rootfs; the daemon gave up after thirty seconds and failed. So each attempt to recover the host re-armed the rollback, and the rollback ran while the retry was still writing binaries, since recovery deliberately does not wait on lifecycle locks. Exiting zero would not have helped. Restart=always restarts a clean exit too, and the start limit counts starts rather than failures, so the unit reaches the same place. Say it in a way systemd understands instead. Standing down exits 69, and the unit names that code in SuccessExitStatus and RestartPreventExitStatus, so the unit goes inactive: not restarted, never approaching the start limit, never reaching OnFailure. Restart=always still covers real crashes. The daemon comes back the next time anything runs bootstrap, because both the install path and the repair path end by starting it. The wait for a held lock stays at thirty seconds. Only the giving up changed. Also reset-failed before start. A unit that already exhausted its start limit cannot be started until the failure is cleared, manual starts included, so without this a retry could not repair the hosts this bug has already broken.
Reset now asks machinectl and nft what is on the host, and fails when it cannot ask. That is the right answer for bootstrap and the wrong one for reset, because the tools it asks with are the ones bootstrap installs. A bootstrap that dies inside host preparation leaves an ownership record on a host with no systemd-container and no nftables. Reset cannot finish there, so the record stays; start is refused against an installation that nothing can clear. Before this branch those inspections were tolerant and the host stayed recoverable. The two callers want opposite answers to the same question, so they get different functions rather than a shared one with a flag. Admission keeps failing closed: a host it cannot inspect is not a host it can prove is clean, and assuming otherwise risks building over a running node. Cleanup treats a tool that is not installed as proof of absence, because nothing of ours can be running if the things that run it were never there. The tolerance is only for a missing executable. A tool that is present and fails still stops reset, since it may be reporting a machine that really is there and reset must not delete around it. The test drives the reset tasks rather than the helper, and its PATH keeps systemctl and ip while dropping machinectl and nft. Hiding everything would have been the easier fixture and a meaningless one: systemctl always exists on a systemd host, and a first version of this test passed while the tasks still called the strict predicate.
Reset tolerated a missing ownership record and refused any other read error. decide refuses the same record, so a file that cannot be parsed took away both exits at once: start would not run, and reset could not clear the thing stopping it. The guide tells operators to keep this file intact, so the documented advice leads straight into the trap. Reset deletes the record moments later regardless. Reading it is a courtesy that keeps the machine name and fingerprint accurate through teardown, not a prerequisite for tearing down, so an unreadable one is replaced with the same synthetic record an absent one already produced, and the reason is logged. The decision moved into a helper because resetUnderLock syncs real host filesystems and needs root, so the behavior was otherwise only reachable from an e2e. Putting the sole store.Load behind that helper also means reintroducing a direct read leaves it uncalled, which staticcheck reports.
VerifyDaemonInstalled resolves the current binary link and fails when its target is gone. Link initialization stat'd the link itself, and Lstat succeeds on a dangling symlink, so it saw a healthy link and left it alone. That combination made a dangling link the one fault verify could report and repair could not fix. start on a completed installation verified, repaired nothing, verified again, and returned the same stat error on every run, with no path back short of editing the link by hand. Resolve it instead, which is what the last-good link two lines down already did. A link that cannot resolve is now replaced the same way a missing one is, and the repair is checked by resolving it rather than by its name.
The installer stages the agent in a temporary directory and then runs it: admission runs from the staged binary rather than the installed one, so that a retry cannot overwrite a live binary link before its intent is accepted. That made the default temporary directory the wrong place for it. A host that mounts /tmp noexec cannot execute what was just staged there, and bootstrap fails before it starts. Hardened, image-based hosts are both the ones most likely to mount it that way and the ones this work is aimed at. Stage under /var/lib/unbounded instead, which the agent already owns, and keep the existing cleanup. Before this branch the staged binary was only copied, never run, so the placement did not matter. The mount options of any specific image are unverified; this removes the dependency on them rather than accommodating a measured one.
…re not true Three small corrections, all to work added on this branch. The rootfs and node stages asked whether any slot was registered, to decide whether to rebuild kube1's rootfs and whether this attempt built the node. Bootstrap only ever manages gs.NodeStart.MachineName, so a machine in the other slot answers a question neither stage asked. It is unreachable today because the second slot cannot exist before an installation completes, which is why consolidating the two predicates looked free when it was not. They ask about their own slot now. FirstRegisteredMachine keeps its one caller, the nftables flush, where either slot occupied is genuinely what matters because the ruleset is shared across the netns, and its doc says which question it answers. A store test fed a record carrying a stale "checkpoint" field, implying the format rejects fields it does not know. It does not: the decoder ignores them, and the case passed because the record had no phase. Ignoring unknown fields is deliberate and load-bearing, since it is what lets a record written by a newer agent stay readable by an older one, so the misleading case is now the missing-phase case it always was and a new test pins the tolerance. The intuitive hardening, DisallowUnknownFields, would take that guarantee away. Fingerprint claimed omitted optional fields stay omitted across releases. The fingerprinted struct has no omitempty tags, so they do not. Adding a field there rehashes every host that lacked it and each reads as a different installation demanding a reset. The comment now says what is actually required, and points at the fixture test that enforces it; confirmed by adding a field and watching that test fail. Also narrow restartReconfigured's doc, which claimed node services generally. It covers containerd and kubelet. LocalDNS and the NVIDIA drop-in write directly and are not tracked, and the reason that is survivable is that a retry does not rewrite the applied config, so the daemon still sees drift and repaves.
Standing down for an unfinished installation is reported as a warning and then returned as an error so the command layer can map it to the exit code systemd expects. Cobra printed that return as "Error:" and followed it with the full flag listing, so the journal showed a fault and an apparent misuse of the command for what is an ordinary state. That is the confusion this path exists to remove, in the one place the behavior is observed, so the fix was undone where it counted. Silence the usage block for the daemon command, which systemd invokes with fixed arguments and never misuses, and silence the error only for the deferred sentinel. Real failures are still reported. Found by reading the journal on a live host rather than from a test; the exit code, the unit state and the absent rollback were all already correct. ErrDeferred is exported in place of the IsDeferred predicate so the wrapping is testable with errors.Is, which is also how it reaches the command layer: Run adds context before returning it.
An operator who finds the daemon inactive after an interrupted install has no way to tell that from a broken one, and the obvious reaction, resetting the host, is the wrong one: the install only needs finishing. Say what the state means, what to look for, and what to do. Record the two cases where reset now proceeds rather than failing, since both look like reset ignoring a problem unless the reason is stated.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Make
unbounded-agent startsafely repeatable. The agent records what aninstallation owns before it changes the host, then reapplies every stage on each
attempt, each deciding from the host what it still has to do, instead of refusing
an interrupted install or starting it over. Reset becomes
strict: it keeps ownership when cleanup fails, so a half-finished teardown is
visibly retryable rather than silently partial. Extracted from #713.
Why
unbounded-agent startis single-shot. It requires a clean host and aborts with"existing node deployment detected; node reset is needed before running start
again" when it finds artifacts from an earlier attempt, including its own. That
is the right contract when a person is driving it and the wrong one for the
non-interactive paths we already ship:
--variant cloud-initruns the installscript from
runcmdon first boot, and the PXE vendor-data does the same.The existing netboot template shows the cost. It writes
/etc/cloud/cloud-init.disabledso nothing can run the payload a second time,and traps failures to POST the install log back to metalman. Re-execution has to
be suppressed because it cannot be made safe, and a partial failure can only be
reported. Recovering one means an operator runs
reset, tearing down whateverprogress exists including a node that may already have joined, or the machine is
reprovisioned: an API call for a cloud VM, another PXE cycle for bare metal.
This change makes the same invocation idempotent. Re-running an identical payload
reapplies each stage against what is actually on the host, leaves a running node
alone, and verifies the daemon rather than rebuilding when the install is already
complete. That applies to the cloud-init and PXE paths today, not only to what
comes next.
Where this is going
We are working toward running unbounded worker nodes on Azure Container Linux
(ACL), an image-based host OS. ACL breaks three assumptions:
/usr/localisread-only, there is no package manager for host prerequisites, and the bootstrap
arrives as an Ignition-written systemd unit rather than a script someone runs.
This change addresses the third. A systemd unit is re-executed by nature, on
reboot and on restart. A marker file answers "already finished" cheaply, and the
Ignition unit will still want one so a healthy host does no work on every boot.
What a marker cannot answer is what a partial run left behind: it is written only
on success, so an interrupted bootstrap has none, the unit correctly retries, and
that retry meets the existing-deployment refusal on every boot from then on.
Recording identity before the first mutation is what lets the retry be recognized
as the same installation rather than refused, and reapplying every stage against
the host is what lets it finish whatever the interrupted attempt left undone.
The record is also where a later release will note the install prefix, so an
installation somewhere other than
/usr/localcan be found rather than guessedat. Records written here carry no prefix and mean the default, which is what a
prefix-aware release will read them as.
Landed so far: #735 publishes bare agent binaries that a declarative first-boot
config can place directly instead of unpacking an archive; #736 keeps daemon
binary rollback working when host policy denies
systemctl reset-failed; #737lets a Machine declare whether its image takes cloud-init or Ignition and refuses
a replacement the controller cannot provision; #738 made the existing-host e2e
suites reliable enough to trust as a baseline.
Still to come: a configurable install prefix with image-provided prerequisite
validation, then Ignition generation and delivery with ACL acceptance coverage.
Each lands on main independently, and main stays releasable after every merge.
What changes
startwrites/var/lib/unbounded/agent/install-state.jsonholding the machine name, afingerprint of the Kubernetes version, rootfs image and API server endpoint,
and which phase the installation is in. The record never says how much of the
installation has been done, so it cannot fall out of step with the host.
Credentials and artifact locations can be refreshed between attempts, including the expiring signature on a signed rootfs URL,
which is excluded from the fingerprint so a refreshed one still reads as the
same installation; changing the fingerprinted identity requires an explicit
reset.
install all run on every attempt, and each decides what it still has to do by
looking at the host. An owned but incomplete rootfs is rebuilt; one a machine
is registered from is left in place. A running nspawn instance is left
running. Node service configuration is rewritten and the service restarted
only when that configuration actually changed. The nftables ruleset a running
node depends on is not flushed. Nothing is assumed from how far a previous
attempt got, so a host modified between attempts is repaired rather than
skipped past.
starts the node writes
<machine>-applied-config.json, and only when itactually built the node. A retry that finds the machine already registered
leaves it alone, because it did not build what is running. Configuration that
may change between attempts but only takes effect at registration, node labels
in particular, therefore reaches the node through the daemon's ordinary drift
repave rather than through the retry.
accepted only when the listening process's root and executable belong to this
installation. Foreign and uninspectable owners still fail.
starton a completed installationchecks the daemon's files, permissions and service state, and repairs them from
the current applied configuration. It needs neither the original node image nor
the original download sources, and it preserves the active slot after an
ordinary repave instead of recreating the retired one.
reconfigure a machine bootstrap is still changing, so it waits briefly for a
handoff and otherwise exits 69, which the unit names in
SuccessExitStatusand
RestartPreventExitStatus. The unit goes inactive rather than failed, soit is never restarted into its start limit and never reaches the
OnFailurebinary rollback.
Restart=alwaysstill covers genuine crashes, and the daemonreturns the next time anything runs
start, since both the install path andthe repair path end by starting it.
EnableDaemonalso clears a priorstart-limit failure, so a retry can recover a host whose daemon is already
rate-limited into failure.
absence from failed inspection, and ownership is released only after teardown
and its filesystem barriers succeed. A failed reset stays in
resettingand isretryable. Two cases are deliberately not treated as failures, because in both
the only thing stopping the host from being cleaned would be the record that
cleaning it removes: an inspection tool that is not installed, since bootstrap
installs those tools and cannot have started anything without them, and a
record reset cannot parse, since it is about to be deleted. Admission keeps
failing closed in both cases.
activation and the AgentUpgrade operation share an installation lock, and
contended operations requeue instead of running concurrently. Last-resort
daemon rollback deliberately does not wait on that lock, since the activation
it is recovering from may still hold it.
docs/content/guides/agent.mdgains a "Retrying bootstrap and repairing thedaemon" section covering retry, repair and reset behavior.
Compatibility
Installs only at
/usr/local. The exported surface ofpkg/...is additiveagainst main: existing entry points keep their signatures, and the
ownership-aware variants are added alongside them.
Main's ordinary repave and discovery behavior is unchanged, including its
two-applied-config intermediate state. Installations created before this change
keep working: daemon operations and reset still apply, and
startstill requiresa clean host before creating new ownership.
The install script is versioned independently of the agent it downloads, since
the agent comes from
AGENT_VERSION,AGENT_URL, or the default of tracking thelatest published release. It therefore still places the agent binary itself, so
an agent older than this change installs exactly as it does today. It does that
only when the path holds nothing usable, so a retry on a host this installation
already owns leaves the live binary link alone and admission still runs from the
staged executable. Because that staged binary is executed rather than only
copied, it is staged under
/var/lib/unboundedinstead of the default temporarydirectory, which a host mounting
/tmpnoexec could not run.Not in this change
No configurable install prefix, immutable-host classification, or
image-prerequisite validation. No Ignition generation or delivery. No persistent
repave redesign, recovery operation, or public recovery API. The daemon-rollback
retry-timer follow-up discussed in #736 remains deferred.