[WIP] Add Ignition provisioning for image-based hosts - #823
Closed
Philip Lombardi (plombardi89) wants to merge 46 commits into
Closed
Philip Lombardi (plombardi89) wants to merge 46 commits into
Philip Lombardi (plombardi89) wants to merge 46 commits into
Conversation
…into acl-extract/bootstrap-recovery
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.
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.
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.
The agent writes its own host-side files to hard-coded paths under /usr/local: the daemon binaries and their blue-green links, the nspawn lifecycle helper, the daemon recovery script, and the LocalDNS network helper. On a host with a read-only /usr none of those writes can succeed, so the agent cannot be installed at all. Add AgentConfig.HostPrefix and a resolver that derives the host-side layout from it. Paths inside the nspawn machine are untouched: they are relative and joined with the machine directory, and conflating the two would break every host. The prefix is declared, never inferred. Where the agent may write is a property of the filesystem, not of the distribution, so keying on distro identity would misclassify a hardened host with a read-only /usr and would silently relocate files on any host whose os-release changed. A wrong guess is expensive to recover from, because the lifecycle helper path is baked as an absolute path into the nspawn drop-in and the config regeneration unit. The accepted syntax is narrow on purpose. The prefix is interpolated into generated systemd units and into a shell script, neither of which quotes it, so rather than adding two kinds of escaping that every consumer must keep correct, the value is constrained to be inert in both. Teardown and existing-deployment detection need to sweep both the configured prefix and the default, so that changing the prefix cannot orphan files or let a dirty host be silently reprovisioned; KnownHostPrefixes and MergeHostPrefixes exist for that and are used by the callers that follow. Nothing consumes the resolver yet. This is the model and its validation, so the changes that convert each caller can be read on their own. Hosts that do not set a prefix resolve to exactly the paths they had before, pinned by a regression test against the existing constants.
The blue-green agent binaries were absolute constants under /usr/local/bin. A host whose /usr is read-only cannot hold them there, which is the whole reason the prefix exists. ResolvedAgentUpgradePathsFor resolves them under a prefix instead. An empty prefix selects the default, and a test pins that the result is exactly the constants this package used before, because those paths are baked into generated units and into the blue-green symlinks of every host already installed. If the default drifted, an upgraded agent would look for its binaries where the host does not have them. The original entry point stays, deprecated, delegating to an empty prefix. It is published from pkg/ and callers outside this repository compose their own phases from it, so removing it would break them at compile time. Every caller inside the repository moves to the new one in this commit, because staticcheck's SA1019 is enabled and a split would not lint. All of them run under systemd or on the host with no config in hand, so they take the prefix from the applied config, which is what that lookup exists for. On a host that configures no prefix this resolves the default and nothing changes. The AgentUpgrade signal path is deliberately not prefixed: it is state about an upgrade rather than part of the installed layout, and it already lives under the agent config directory, which stays writable on such hosts. One caller passed the function as a value rather than calling it, so a search for call sites missed it and only the linter found it. It is now wrapped, so the prefix is read when the command runs rather than when it is constructed.
Teardown has to find the agent's own files. On a host that configures a prefix they are not under /usr/local, and after a bootstrap that failed before the node started there is nothing on the host that says where they are: the applied config carries the prefix but is not written until the node runs. The ownership record is written before any mutation, which makes it the only source that covers that window, so it carries the resolved prefix. Optional, and the schema version does not move. A record written by an agent that knows about the prefix stays readable by one that does not, because unknown fields are ignored, and a default installation writes no field at all so its record is byte-identical to one written before this existed. A test pins that, since the value of the compatibility is entirely in the absence. Resolved rather than configured, so the record names a real directory instead of an empty string meaning whatever the default happened to be. NewRecord takes it as a parameter rather than leaving it a field to set afterwards. Forgetting it would be silent and would only surface at teardown, on a host whose files are somewhere reset does not look. Also corrects a comment in the prefix lookup that pointed at this field before it existed.
The agent's own binaries live under the prefix, so starting with a different one is not a retry of the same installation. Continuing would leave the first installation's files where they are and build a second one beside them. Admission has to refuse and ask for a reset, which is what a changed fingerprint does. The delicate half is the other one. Every host already installed was fingerprinted without this input. If the default contributed a value, all of them would hash differently under an agent carrying this change, read as a different installation, and demand an explicit reset on upgrade over a field they never set. So the prefix enters the hash only when it resolves somewhere other than the default, and carries omitempty so that at the default it contributes nothing rather than an empty string. It is the resolved prefix that counts, not how it was written. Leaving it unset and naming /usr/local explicitly put the files in the same place, so they hash alike; telling an operator who wrote down what was already true that they must reset the host would be a poor trade for the precision. Verified by mutation, since all three ways to get this wrong are silent and affect every host in the field rather than the one under test: dropping omitempty, hashing the default instead of eliding it, and never hashing the prefix at all each fail a test. The fixtures carry a literal fingerprint, which is what makes the first two detectable at all.
Ignition is the only provisioning mechanism Azure Container Linux consumes; it has no cloud-init, so a cloud-init payload passed as customData is never acted on and nothing reports an error. This is the encoding layer on its own, before anything emits a document. The types are hand-written rather than pulled from github.com/coreos/ignition, which would bring the whole specification along for the handful of fields used here. Three things carry a cost that is only visible on a host that has already failed to provision, so each is pinned by a test: The spec version. Ignition refuses a config whose version it does not implement, on first boot, with no shell and no agent yet installed. There is nothing there to report the mismatch. Which schemes Ignition can fetch. This decides whether a file lands before dbus starts or has to wait for the agent, which is after. oci is the one that matters, because it is the agent's own artifact scheme and Ignition has no idea what to do with it. File modes, which Ignition serializes as decimal. A mode written 600 rather than 0o600 is 0o1130 on disk, and for the agent config that means credentials readable by everyone. The test asserts the decimal the emitted document would actually contain.
Adds --variant ignition, which writes the agent config, fetches the agent binary to its final location, and installs a oneshot unit that bootstraps on boot. Everything Ignition writes is in place before any service starts, because it runs from the initramfs. Every input this variant needs is required rather than defaulted. Ignition declares state: it cannot resolve a version, detect an architecture, or extract an archive at boot, so the artifact has to be named exactly. The digest is required rather than optional because an unattended host that silently accepts whatever a URL returns is worse than a bootstrap that refuses to render. The prefix is required because Ignition places the binary itself, and the default /usr/local is read-only on exactly the hosts this variant exists to serve. All three are refused at render time, where the message reaches a person, rather than on a machine with no shell. The unit carries no completion condition and so runs on every boot. A condition needs a marker file, and a marker is a second record of completion that can disagree with the ownership record the agent already keeps. Both commands the unit runs return immediately once that record says the installation is complete: preflight reports an empty result and start verifies the daemon, repairing it only if it is not running, and neither resolves artifacts or touches the network. The cost is two short-lived processes per boot; the benefit is that a node whose daemon was stopped or damaged comes back on reboot. Two settings come from failures seen on real hardware rather than reasoned about. network-online.target means a link is configured, not that DNS resolves, so the unit retries instead of ordering against a guarantee that target does not carry. And bootstrap has no later opportunity to run, so StartLimitIntervalSec=0 keeps a burst of early failures from permanently disabling it. The prefix is carried in the agent config, not only in the generated output, because the daemon and the nspawn lifecycle hooks are started by systemd later and cannot inherit it from the environment that provisioned the host.
The Ignition unit carries no completion condition and runs on every boot, deciding there is nothing to do from the agent's ownership record. Reset deletes that record. A unit left behind would find an uninstalled host on the next boot and bootstrap it, quietly undoing the reset. Removal runs before the artifacts are deleted, so a failure stops the reset while the host is still recognizably installed rather than half torn down with something that will rebuild it. Disabling as well as deleting, because the file and the enablement symlink in multi-user.target.wants are separate: removing only the file leaves systemd with a dangling want. Absent on every host not provisioned through Ignition, which is the common case, so a missing unit is success. The unit name moved to goalstates. The command that writes it and the reset that removes it live in packages that cannot import each other, and a name that drifted between them would leave the unit enabled on a host that had just been reset. It is a named task rather than a step inside another one so the reset composition can be asserted. A first version tested the removal in isolation and passed while nothing called it, which is the failure this arrangement makes visible.
Re-running start on a completed installation verified the daemon and then rewrote the record regardless. Harmless when that happened once per manual rerun. The Ignition unit carries no completion condition and runs on every boot, so it becomes a durable write per boot on every node, and a write is a chance to fail: an entirely healthy host would be taking one for no reason. Only a repair can have changed anything, so only a repair is committed. Also stop discarding the verify error that triggered the repair. The first verify says what is broken; the repair failure says only that fixing it did not work. Reporting the second alone sends an operator after the wrong thing, so both are now wrapped together. The tests for this were wrong twice before they were right, both times passing against code that did the opposite. Comparing the record's contents cannot see a rewrite, because MarkComplete on an already-complete record writes identical bytes; the test now compares the inode, which changes on any write because the store replaces the file atomically. And asserting the reported error matched the injected one proved nothing while verify and repair failed with the same error, so they now fail differently.
Philip Lombardi (plombardi89)
force-pushed
the
acl-extract/immutable-host-paths
branch
from
September 21, 2026 23:39
9d0ff46 to
1122db8
Compare
Collaborator
Author
|
Superseded by #815, which now carries these four commits. The stack these sat in is gone: #742 merged, so #815 targets Commits carried over unchanged, verified content-identical across the rebase:
|
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
1 participant
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.
Status: WIP. Do not merge. Stacked on #815, which is stacked on #742.
This PR targets
acl-extract/immutable-host-paths(#815), notmain. It needsthe installation prefix that #815 introduces and the ownership record from #742,
so it cannot build against
mainuntil both land. Basing it on the branch keepsthis diff to its own commits rather than re-proposing theirs.
It will be rebased onto
mainand retargeted once the stack merges.Why
Azure Container Linux ships no cloud-init. The binary is absent, the services
report
not-found, and/var/lib/clouddoes not exist, so a cloud-init payloadpassed as user data is never acted on and nothing reports an error. Ignition is
the only provisioning mechanism such an image consumes.
Ignition also runs from the initramfs, so everything it writes is in place
before any service starts.
What is here
An Ignition config encoder. Spec 3.4.0, hand-written rather than pulled from
github.com/coreos/ignition, which would carry the whole specification for thehandful of fields used here.
--variant ignition. Writes the agent config, fetches the agent binary toits final location under the configured prefix, and installs a oneshot unit that
bootstraps on boot.
Reset removes the first-boot unit, so a reset host does not bootstrap itself
again on the next boot.
The record is written only when a repair changed something, because the unit
below reaches that path on every boot.
Decisions worth reviewing
Everything this variant needs is required, not defaulted. Ignition declares
state: it cannot resolve a version, detect an architecture, or extract an archive
at boot.
--agent-urlmust name the bare binary,--agent-sha256is requiredbecause an unattended host that accepts whatever a URL returns is worse than a
bootstrap that refuses to render, and
--host-prefixis required becauseIgnition places the binary itself and the default
/usr/localis read-only onexactly the hosts this serves. All three are refused at render time, where the
message reaches a person rather than a machine with no shell.
The unit carries no completion condition, so it runs on every boot. A
condition needs a marker file, and a marker is a second record of completion that
can disagree with the ownership record the agent already keeps. Both commands the
unit runs return immediately once that record says the installation is complete:
preflight reports an empty result and start verifies the daemon, repairing it
only if it is not running. Neither resolves artifacts nor touches the network.
The cost is two short-lived processes per boot; the benefit is that a node whose
daemon was stopped or damaged comes back on reboot.
Two unit settings come from failures seen on hardware, not reasoned about.
network-online.targetmeans a link is configured, not that DNS resolves: a unitcan start in the same second that target is reached, while
systemd-resolvedisstill coming up, and fail on an unresolved host. And bootstrap has no later
opportunity to run, so
StartLimitIntervalSec=0keeps a burst of early failuresfrom permanently disabling it.
The unit name lives in
goalstatesbecause the command that writes it andthe reset that removes it are in packages that cannot import each other. A name
that drifted between them would leave the unit enabled on a host that had just
been reset.
Compatibility
Additive. The script and cloud-init variants are untouched, and the new flags are
only required by the new variant. Hosts that set no prefix resolve exactly the
paths they did before.
Not in this change
No UKI/OVMF disposable-overlay harness, no ACL boot or same-disk reinstall
coverage. Those are a test-infrastructure body of work and belong on their own.
The system extension that supplies
systemd-nspawnis also not here: AzureContainer Linux is expected to ship
systemd-containerusable on the first boot,which removes the need for it.