Support immutable hosts: configurable install prefix and Ignition provisioning - #815
Philip Lombardi (plombardi89) wants to merge 46 commits into
Conversation
e56881a to
364cb10
Compare
9d0ff46 to
1122db8
Compare
8cd41ba to
9dc51dc
Compare
0ac98af to
b131542
Compare
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.
…t missed it Three test defects and the gap one of them was hiding. Two doc comments sat on TestKnownHostPrefixes describing tests that were not in the file. One named TestResolvedAgentUpgradePathsEnvOverridesPrefix, and that behaviour was genuinely untested: every test that set an environment override used an empty prefix, and the only test with a prefix set no overrides, so the interaction between them was never exercised. The doc on ResolvedAgentUpgradePathsFor promises overrides win, and the nspawn lifecycle hooks rely on it to pin a binary through an upgrade. That test now exists, and checks that a partial override leaves the rest resolving from the prefix. The applied-config lookup was tested by calling it for real, so it read /etc/unbounded/agent on whatever machine ran it. On a provisioned host the answer depends on that host; it passed only because nothing in the field sets the field yet. It now takes the config directory, the way the first-boot unit removal does, and covers both machine slots, a corrupt config, and an absent one. A test named for round-tripping the prefix through the applied config never called the lookup at all. It marshalled a struct and unmarshalled it, which is a test of encoding/json. Replaced by the cases above. MergeHostPrefixes had no test and no caller, and its ordering is not obvious: with more than one candidate the default lands in the middle, because KnownHostPrefixes appends it per candidate. Teardown is about to read that list, so the order is pinned here rather than discovered there. Separately, the prefix now comes from the ownership record first and the applied config second. The record is written before the first host mutation, so it is the only source that survives a bootstrap which failed before the node started, and that is exactly where teardown runs. Reading the applied config there returns the default, which is the one prefix known unwritable on a host that configured one. Both lookups log rather than swallow, for the same reason.
The prefix reached the paths the agent resolved but not the files it wrote, so a host configuring one got a daemon installed under the default and a recovery unit pointing into it. On the hosts this exists for, that directory sits inside a read-only /usr. The recovery unit is the sharpest edge. Its ExecStart is the only reference to the recovery script, so rendering the default path while installing the script under the prefix produced a unit aimed at a file that was not there. Nothing observes that until the daemon fails and systemd runs OnFailure, which is the worst moment to discover it. Both layouts are now resolved from one prefix at each call site and passed together, so they cannot drift apart. InstallBootstrapBinary takes the prefix rather than resolving it. Its callers know it from different places: bootstrap has the config it is applying, which is the prefix by definition, while repair has only what the host recorded. Resolving internally would have made the first host mutation of a bootstrap depend on state written elsewhere for a value already in hand. That install now also goes through the resolved upgrade paths, so an environment override puts the binary where VerifyDaemonInstalled looks for it. Previously install used the bare constant and verification used the override, which disagreed whenever an override was set. TestRenderDaemonAsset read the real host state and asserted the default constants, so it passed only on a host that had no prefix configured, and it covered neither the recovery unit nor the prefix. It now renders all three assets under both prefixes and checks every path they carry. Three tests cover InstallBootstrapBinary, which had none: installing under the prefix, keeping a usable incumbent, and replacing an unusable one. renderDaemonAsset had no remaining production caller once EnableDaemon resolved its own paths, and was kept alive only by the test above. Removed.
The helper is installed as a file and named inside two generated systemd units, and both used the same fixed path. Under a configured prefix the install moved but the units did not, so the hooks pointed at a file that was not there. Nothing detects that at install time. The units are only executed when systemd starts the machine, so the failure appears as a machine that will not start, well after the bootstrap that caused it reported success. The path now comes from the nspawn goal state, resolved once from the config that is being applied, and both the installer and the template read it from there. Because the hook units are written once and have to survive an agent upgrade, a single resolved value is the point: recomputing it independently in the two places is what allowed them to disagree. Tests cover the resolution and the population separately, because they fail independently. The first checks the goal state puts the helper under the prefix. The second goes through writeNSpawnConfigs rather than hand-built template data, since the defect it guards against is that step reverting to the constant, which a test supplying its own data passes either way. A third covers the install task, whose existing tests exercised the copy underneath it and so said nothing about where the task chose to write.
Third instance of the same defect. The helper script was written under the installation prefix while the unit that executes it carried a fixed path, so a host with a configured prefix got unbounded-localdns-network.service pointing into a directory the script was never written to. It surfaces when systemd runs the unit during node start, not when the file is written. The path is resolved once on the LocalDNS goal state and read from there by both the writer and the unit template, which is the same shape used for the daemon recovery script and the nspawn lifecycle helper. Reset still removes the helper from the default prefix only. That is one of several teardown paths with the same gap, and they are fixed together in a later commit rather than each growing its own way of asking what prefixes exist. resolveLocalDNS gained a dependency seam so the resolution can be tested without a host resolv.conf. This follows resolveMachine, which already takes its GPU discovery the same way. The seam is what makes the resolution testable at all: a first attempt covered only the unit template, which supplies the path itself and so passed against a resolution that ignored the prefix entirely.
Five call sites made the agent's writes durable by syncing /usr/local. Once the prefix became configurable those writes moved and the sync did not, so on a host with a prefix the agent persisted a filesystem it had not written to. A crash before the kernel flushed could lose exactly the work the sync existed to protect. On an immutable host the mismatch is total rather than partial. /usr/local is a real directory inside a read-only /usr, so it opens successfully and the sync appears to succeed while touching an entirely different device from the one holding the files. The three bootstrap stages and the daemon repair now sync the prefix they just wrote under. Teardown syncs every prefix the host might hold files under rather than only the recorded one, because a host reprovisioned with a different prefix still has the earlier layout on disk, and the removal of those files has to be durable too. A prefix that does not exist costs nothing there, since the teardown sync already walks up to the nearest existing ancestor. This is not reachable without a configured prefix: with none, the prefix is /usr/local and the old behavior was correct. It is incomplete propagation rather than a defect that shipped. The decision of what to sync is extracted in both places so it can be tested. The sync itself calls unix.Syncfs on real paths and is not worth faking; the part that was wrong was the choice of directory.
Teardown removed the agent's files from a fixed /usr/local and the existing-deployment preflight looked for them there. On a host with a configured prefix neither found anything, which fails in both directions at once: reset reports success while leaving a complete installation on disk, and the next bootstrap sees a clean host and provisions straight over the live one. The reprovisioning case is worse than the simple one. A host installed under a default prefix and later given a configured one carries both layouts, so sweeping only the current prefix orphans the earlier files, and because the preflight reads the same list those orphans then refuse a bootstrap on a host the operator was just told is clean. The layout is now defined once, in goalstates, and both callers read it from there. That shared definition is the point: teardown and the preflight have to agree about what an installation consists of, and they were previously two hand-maintained lists that already disagreed about the LocalDNS helper. The LocalDNS reset gap left open by the earlier LocalDNS commit closes here, with the rest of the teardown rather than growing its own way of asking what prefixes exist. RemoveAgentArtifacts now resolves its file and directory lists at construction. Do removes real system paths, so a test that had to go through the exported constructor could not run it at all; with the lists supplied it runs against a temporary tree, including the repeat pass that a partially provisioned host needs to survive.
The install script pre-stages the agent binary before running it, and did so at a fixed /usr/local/bin. On a host with a configured prefix that is not where the agent then installs itself, so the host ends up carrying a stray binary in a directory nothing else uses. On a host that mounts /usr read-only the install fails outright, before the agent runs at all. The staging exists for backward compatibility: the agent version is chosen independently of the script, so an installer that relied on the agent to place its own binary would break every agent released before that behavior existed. That reasoning is unchanged; only the location follows the prefix now, with the historical path kept as the script's own default. The prefix is added to the install environment in the manual bootstrap handler rather than in AgentInstallEnv, because it comes from the agent config and not the agent spec. The Machine CR has no prefix field at all, so the controller-driven callers that share AgentInstallEnv have nothing to pass and correctly keep the default. install gains -D so the prefix's bin directory is created. A configured prefix will not already have one, and the previous form would have failed on the directory rather than the file.
…off its retries Two changes to the first-boot unit, both about what an operator can see. The binary check asserted nothing. ConditionPathExists is not an error when it fails: systemd marks the unit inactive and moves on, so a host whose agent binary Ignition never placed sat there looking like a host with nothing to do. AssertPathExists puts the unit in the failed state instead, where systemctl status and any watchdog can find it. Neither form starts the service, so this changes only whether the reason is discoverable. The retry had no ceiling. StartLimitIntervalSec=0 is deliberate, because bootstrap gets no second chance and a burst of early failures must not disable it permanently, but combined with a flat ten second RestartSec it means a host that cannot reach the network spawns the agent several thousand times a day and scrolls the journal entry that would explain why out of reach. It now backs off towards a five minute ceiling while keeping the first retry prompt. RestartSteps and RestartMaxDelaySec need systemd 254. Older versions log an unknown key and continue with the fixed RestartSec, which is exactly the behavior being replaced, so nothing breaks where they are not understood.
…d document the prefix The Ignition flag rules were enforced in the renderer, which runs after a Kubernetes client is built and a site is resolved. An operator who forgot --host-prefix waited for all of that to be told about a flag, and only heard it at all if the connection succeeded. They are now checked in validate, which runs first. The rules live in one place and take the prefix as a parameter, because the two callers hold different values of it: validate sees the flag before a config exists, and the renderer sees the config it is about to interpolate. Checking the flag in the renderer would leave it trusting a value it does not use. TestRecordCarriesTheInstallationPrefix justified the omitted field with two claims that do not hold. It said a default installation records nothing, but bootstrap records the resolved prefix, so a host that sets none records /usr/local explicitly, deliberately, so that teardown reads a real directory instead of inferring what the default was when the host was built. It also said older agents would otherwise see a field they did not write, but records are decoded without DisallowUnknownFields precisely so that cannot matter, which TestStoreIgnoresUnknownFields already pins. Corrected, and a test added for the resolution the corrected reasoning depends on. The remaining changes are small. ignitionRemoteFetchable listed data among the schemes it accepts while deliberately excluding it, because a data URL is inline content rather than a fetch. ignitionModeData had no caller and was kept alive by a test asserting it equalled its own literal. The --variant help did not mention ignition. boolPtr is replaced by ptr.To from k8s.io/utils, already a dependency. A test name used a British spelling. The agent guide had no mention of immutable hosts, the Ignition variant, or the prefix, and the agent-upgrade design still described the prefix-less path resolution as the only one.
Azure Container Linux boots a Unified Kernel Image through shim and systemd-boot, and QEMU has no way to append to the command line of a UKI booted that way. The command line is where an Ignition config source and early networking are named, so the harness cannot provision such a host without getting at it. Booting the kernel and initrd directly with -append would work once and then break everything after. systemd-boot only appends flatcar.first_boot while firstboot.addon.efi exists, and ignition-quench.service deletes that addon after a successful first boot. Bypassing the boot chain makes every boot look like a first boot, so Ignition re-runs, re-fetches from a file server that is no longer there, and the guest isolates to emergency.target. So this appends to the boot chain rather than replacing it. The shipped addons' .cmdline sections are padded well past their contents, which means one can be extended in place: no cluster allocation, no directory entry change, just rewritten bytes and a corrected section VirtualSize. firstboot.addon.efi is skipped on purpose, because the addition has to survive its deletion. Writes go through qemu-nbd over a unix socket, so there is no loop device, no nbd kernel module and no privilege involved, and pointing it at an overlay leaves the backing image untouched. The patched section is read back through the same cluster mapping before anything boots it, because an in-place FAT write is only as good as that mapping. The tests cover the PE header arithmetic rather than the disk path, which needs a real image and is exercised by running the ACL host in the suite. That arithmetic is worth pinning because it fails quietly: the VirtualSize write is the only one that lands inside a PE header, and four bytes out overwrites the section's VirtualAddress instead, producing an executable that loads its command line from nowhere. Computing that offset was extracted from the disk-facing function so it can be tested at all.
The cloud-init path reaches a fresh VM through create-vm, which launches it. An Ignition host cannot be launched there, because its config has to carry the bootstrap token and the API server address, so the launch is deferred to run-agent. That leaves nothing to clear the previous VM. The overlay it still holds open cannot be recreated underneath it, and qemu-img fails on a second provision of the same host with no indication that a VM is the reason. The firmware variables are discarded along with the disk. They record the boot entries of the disk being replaced, so keeping them leaves the new VM's firmware describing one that no longer exists.
Teardown sweeps every prefix the host might hold files under, and on an immutable host one of those sits on a read-only filesystem. Unlinking a path that is not there returns EROFS rather than ENOENT, because the kernel checks the parent directory for write permission before it resolves the final component, so the ENOENT the code tolerated never arrived. The effect was a reset that failed on a file that had never existed: remove owned artifact /usr/local/bin/unbounded-agent: read-only file system and failed late, after the daemon unit and the machines were already gone, so the host was left half torn down with no agent to finish the job. The existence check now comes first. Lstat rather than Stat, because a dangling symlink is still a file the agent left behind and has to be removed rather than read as absent. The two syscalls are injected so the ordering between them can be tested. It cannot be observed otherwise: a unit test cannot arrange a read-only mount, and an unwritable directory is not a substitute, because unlink returns ENOENT there. A test built that way passes against the original bug, which is how the first attempt at this test was written.
…ists The entry needs a federated Azure login to read its image, and adding it before one is configured makes every pull request red for a reason no reviewer can act on. The matrix now includes it only when the credential is present, so it stays absent today and appears on its own once the secrets are set, with no further change here. They have to be repository secrets. The credentials this repository already has live in the azure-ci environment, which requires a reviewer, and using that would put a manual approval in front of every pull request rather than the occasional quickstart run it was set up for. The check reads whether the secret is empty, never its value, and GitHub masks it regardless.
`az storage blob download` requires a seekable target, so it cannot write to a pipe and fails with "Target stream handle must be seekable" rather than anything about storage. The manifest is a 951 byte JSON document that only needs reading, so it is fetched with a bearer token instead. This is the same call e2e.py already makes to resolve the image, for the same reason. The build id is checked before use. An empty or absent one would otherwise produce a cache key and a file name with a hole in them, and the failure would surface later as a download of the wrong thing.
The check resolved the agent's install directory from the fixed daemon binary path, so on a host with a configured prefix it probed /usr/local/bin. That directory is read-only on exactly the hosts a prefix exists for, so preflight failed and reported that the host could not be provisioned, naming a directory the agent was never going to write to. It refuses rather than warns, and it runs as ExecStartPre of the first-boot unit, so bootstrap never started. The unit retried indefinitely by design and the host sat in activating, which is the shape a genuine preflight failure has, making this hard to tell apart from a host that really was unusable. The directory now comes from the installation prefix. The intent recorded on the original was already right, that the check derive from where the agent installs rather than restate it; the prefix is simply newer than the check. Found by running the suite on an immutable host, which is also why it was not caught earlier: the check arrived while this branch was in flight, so local runs predating the rebase never exercised it.
reinstall-agent exists to prove a reset host can be provisioned again from what is already on it, and asserts the boot id is unchanged to show the disk was reused. The Ignition path replaced the disk and booted a fresh VM, so that assertion failed on the one host the step most needs to cover. Ignition is not rerun. Only the agent binary, its config and the bootstrap unit are delivered over SSH; identity, networking, filesystem and boot state have to survive a reset, and recreating them here would hide the cleanup defects this step exists to find. The payload set is asserted rather than filtered, so a file appearing that the harness does not know about stops the run instead of being skipped, and the binary is checked against the digest in the rendered config, since it is fetched by URL and that digest is the only thing tying what gets installed to the build under test. Separately, host_image is no longer cached. Tests select a host by patching HOST_BASE_OS, and a cache there silently returned whichever image was resolved first, so they rendered the wrong distribution and failed somewhere unrelated. It passed only because the test that cleared the cache happened to run first alphabetically; in isolation it did not. The cache now sits on the manifest lookup, which is the network call it was added for.
Naming the Azure Container Linux blob means reading a published manifest, which is a network call and an Azure token. host_image is asked for the ssh user and the installation prefix far more often than for the image itself, including at module import, so that round trip ended up behind importing this module at all. The consequence was that the Python unit tests could not run in the Azure Container Linux job without Azure reachable, and a test that patched the harness's command runner had it consumed by the manifest lookup instead. That test passed locally, where the default host needs no manifest, and failed only in the job whose environment selects that host. Resolution now happens in the two places that actually fetch or open the image. Everywhere else keeps the cheap form. The tests that cover this run under every host the matrix defines, since that is how they run in CI, where each job sets its own HOST_BASE_OS.
Three places still assumed a host the harness can reach and prepare before it boots. A review of what this branch left out when it scoped the Ignition work down to the suites that exist on main found them; two are unreachable from those suites today and would have become traps the moment they were not. Blocked-network preparation installed host packages over SSH. An image-managed host has no package manager and a read-only /usr, so there is nothing to install and nowhere to install it, and reaching that code at all means the premise of the host entry is wrong. Offline bootstrap delivers an artifact bundle over SSH before the agent runs. An Ignition host has no such window, and discovering that late costs an agent build and a VM boot first. It is refused up front, naming the scenario setting that does work. Log collection asked for cloud-init's logs on a host with no cloud-init, which left three empty files that read as a host where cloud-init had failed. The Ignition journal is collected instead. Also restores a binding removed with the call that used to produce it, which left launch_vm reading an undefined name. Nothing caught that: the module still imports, and no test reaches the function.
The unit is a oneshot with RemainAfterExit=yes, so once it has run it stays active. Reset disabled it and deleted the file, neither of which changes that: systemd keeps the loaded unit active until something stops it. A host provisioned again afterwards writes the unit back and starts it, systemd finds a unit that is already active and does nothing, and the agent never runs. Nothing fails. The reinstall reports success, and the node simply never appears, with no entry in any log between the start and the timeout to say why. Reset now stops it. The harness stops trusting "active" on its own as well: it reads the unit's invocation id before starting it and requires a different one afterwards, so a start that did nothing is reported as a start that did nothing rather than as a completed bootstrap. Without that, the same class of defect would go on presenting as a node that never registers. Found by the Azure Container Linux suite, which is the only host that reinstalls onto a disk whose bootstrap unit ran from Ignition rather than from a script.
b131542 to
00e40d2
Compare
Same bug as the owned-artifact removal fixed earlier, in the helpers under pkg/agent/phases/reset that other consumers of the library use. CleanupLocalDNSRules sweeps the default prefix as well as the configured one. On a host with a read-only /usr, unlinking a path that does not exist there returns EROFS rather than ENOENT, because the kernel checks the parent for write access before it looks up the name. Reset then fails on a file that was never there. The Azure Container Linux e2e did not hit this only because that image has no /usr/local/libexec, so the lookup fails first with ENOENT. Both helpers now Lstat first. os.RemoveAll has the same problem, so removeAllIfExists gets the same check even though none of its callers remove anything under /usr today.
Four exported functions in pkg/agent/phases changed signature to take the installation prefix: CheckExistingDeployment, EnsureNoExistingDeployment, CheckHostOSConfiguration and EnsureNSpawnLifecycleHelper. That breaks any consumer outside this repository that calls them. The old signatures are back as deprecated wrappers that use the default prefix, and the prefix-aware versions take new names, following ResolvedAgentUpgradePathsFor: CheckExistingDeploymentFor, EnsureNoExistingDeploymentFor, CheckHostOSConfigurationFor and EnsureNSpawnLifecycleHelperAt. Callers in this repository use the new names. CleanupNetwork and CleanupLocalDNSRules gained variadic parameters, so existing calls still compile and they keep their names. The tests assign each wrapper to a variable of its old function type, so a signature change fails the build.
Philip Lombardi (plombardi89)
left a comment
There was a problem hiding this comment.
Review focused on bugs and on places where the change can be smaller. Details and suggested fixes are in the inline comments; this is the index.
Likely bugs: agent and CLI
- Ignition hosts probably run a daemon repair on every reboot. The first-boot unit isn't ordered after the daemon unit, so
startlikely sees the daemon inactive and repairs it. This is inferred from unit ordering; I haven't reproduced it. Before=systemd-nspawn@.serviceinnftables-flush.servicedoesn't order the flush before the machines. systemd expands it tosystemd-nspawn@nftables-flush.service, which I confirmed withsystemd-analyze verifyon systemd 258. The unit comes from #814, but the new test pins that line as correct.- The docs example reads a
.sha256file that releases don't publish. Onlychecksums.txtis published. - Reset gets the prefix from two sources. The task list is built from
ResolveHostPrefixoutside the lock, while the sync usesr.HostPrefix. They disagree when the record is unreadable. - A malformed
--agent-sha256is only rejected after contacting the cluster. - The
OwnedHostFilescomment, and several test comments, say preflight checks the same list. It doesn't, and it mustn't, because Ignition places the binary before preflight runs. - Design doc: the sentence "chooses the inactive slot:" is now separated from the block it introduces.
Likely bugs: e2e and CI
- The image cache is never saved. Cleanup deletes
.vm-e2ebefore the cache post step runs. - The storage bearer token can leak into public logs through the
CalledProcessErrortraceback. - sha256 is skipped for any image file that already exists, whether cache-restored or a partial download.
ACL_IMAGE_BUILD_IDdoesn't pin a build, although the README says it does. An emptybuild_idis also accepted.- Each
e2e.pyprocess resolves the manifest again, so a build published mid-job breaks the run. - Unit test issues:
unittest.main()runs before some test classes are defined, and one unit test makes a real SSH call. reset-failedis now best-effort on every host, not just ACL.- The README's
run-local.shcommand also runs the configuration suite, which, as far as I can tell, can't work with Ignition. _reinstall_ignition_payloadfilters payloads although its test says they're asserted, and its digest check is circular.
Simplification, biggest wins first
- Resolve the prefix once per entry point and pass it down.
ResolveHostPrefixreads the record from disk at 11 call sites. - Revert the signal-operator plumbing.
SignalPathdoesn't depend on the prefix. - Collapse
KnownHostPrefixes,MergeHostPrefixesandOwnedHostFilesAcrossinto one function. Every caller passes a single prefix. - Put the binary slot paths on
HostPathsand deriveAgentUpgradePathsfrom it, which removes the(paths, hostPaths)argument pairs. - Keep one remove-if-exists helper in
internal/fsutilinstead of two copies plus an inline third. - Reject surrounding whitespace in
ValidateHostPrefix, then drop the scatteredTrimSpacecalls and their tests. - Ignition: validate once, including the digest, and render the unit from an embedded template.
- Drop tests that restate constants or can't fail (called out inline).
- Shorten comments. Many functions and tests carry 10-20 line narratives (
bootstrapIdentity,removeFirstBootBootstrapUnit,ignitionBootstrapUnitContents, most new test doc comments). Keep the non-obvious why at each site and move the history to the PR description ordesigns/. That would noticeably shrink a 5.8k-line diff. - e2e: resolve the manifest once in the workflow and hand it to
e2e.py, and delete the duplicate module-level definitions.
Questions
- Does anything outside this repo import
pkg/agent? If not, the fiveDeprecated:wrappers and the tests that pin their signatures can go. HostPrefixFromAppliedConfigreads the applied config without the checksum check thatFindActiveMachinedoes. Is that intended?
| // binaries, so it needs the network even though Ignition already fetched | ||
| // the agent itself. Ordering after systemd-sysext keeps any extension | ||
| // merged before the agent runs. | ||
| b.WriteString("After=network-online.target nss-lookup.target systemd-sysext.service\n") |
There was a problem hiding this comment.
Likely bug (inferred from unit ordering, not reproduced): a reboot probably triggers a daemon repair every time.
On reboot, this unit and unbounded-agent-daemon.service start in the same boot transaction, and nothing orders this one after the daemon. The daemon unit waits for machines.target, which waits for the nspawn machine to boot. So start most likely reaches VerifyDaemonInstalled before the daemon is active, and then:
systemctl is-activefails;RepairDaemonrewrites the units and runsdaemon-reloadandsystemctl start;MarkCompleterewrites the record.
That contradicts the doc comment above ("return immediately") and undoes the coordinator change that skips rewriting the record on a healthy host.
Suggested fix: add goalstates.DaemonUnit to this After=. On first boot the daemon unit doesn't exist yet, so the ordering does nothing. It also can't deadlock when start starts the daemon itself, because the ordering only runs one way.
To confirm, reboot an Ignition host in the e2e and check this unit's journal for daemon binary links initialized or daemon unit started.
There was a problem hiding this comment.
Fixed in 0d6ab1f: the unit now orders after unbounded-agent-daemon.service.
That exposed the next overlap. The daemon holds the installation lock during its startup migration and start used TryLock, so each reboot would have logged a failed attempt before the retry. start now waits up to 30s for the lock.
validate-host-reboot on an Ignition host now fails on NRestarts > 0, daemon unit started in this boot's journal, or a rewritten install record. In a local QEMU run the repair race itself did not reproduce (the machine was up 0.2s before start checked), but all three reboots hit the lock collision and the wait handled it.
|
|
||
| // The flush still has to precede the machine, which is what gives the node | ||
| // a clean ruleset rather than merely a later one. | ||
| assert.Contains(t, unit, "Before=systemd-nspawn@.service") |
There was a problem hiding this comment.
This pins a line that doesn't order anything. In a [Unit] section, systemd fills in a bare template dependency with the depending unit's own name. So inside nftables-flush.service, Before=systemd-nspawn@.service becomes Before=systemd-nspawn@nftables-flush.service. Checked on systemd 258 against the rendered unit:
$ SYSTEMD_LOG_LEVEL=debug systemd-analyze verify ./nftables-flush.service 2>&1 | grep 'Before:'
Before: shutdown.target (origin-default)
Before: systemd-nspawn@nftables-flush.service (origin-file)
RequiredBy=systemd-nspawn@.service under [Install] adds a Requires= to every instance but no ordering. The flush and systemd-nspawn@kube1.service can therefore start in parallel. The unit comes from #814, but this test now declares it correct.
Suggested fix: add After=nftables-flush.service to the per-machine service-override.conf drop-in, which already orders the machine after the config-regeneration unit the same way. Then assert on the rendered override instead of this line.
There was a problem hiding this comment.
You're right. Fixed in 8343f08: the per-machine override now has After=nftables-flush.service, the inert Before= is gone, this test asserts its absence, and a rendering test pins the override's ordering. systemd-analyze on the rendered units shows the machine after the flush and the flush before systemd-nspawn@kube1.service. The line predates #814 (it came from #61/#76); this PR only added the assertion.
| --variant ignition \ | ||
| --host-prefix /opt/unbounded \ | ||
| --agent-url https://github.com/Azure/unbounded/releases/download/v0.8.1/unbounded-agent-linux-amd64 \ | ||
| --agent-sha256 "$(cat unbounded-agent-linux-amd64.sha256)" \ |
There was a problem hiding this comment.
Releases don't publish unbounded-agent-linux-amd64.sha256; .goreleaser.yml only writes checksums.txt. ignitionHashFromSHA256 already accepts sha256sum-style lines, so after downloading checksums.txt this works:
| --agent-sha256 "$(cat unbounded-agent-linux-amd64.sha256)" \ | |
| --agent-sha256 "$(grep ' unbounded-agent-linux-amd64$' checksums.txt)" \ |
There was a problem hiding this comment.
Fixed in 08ef542, anchored so it does not also match the .tar.gz line.
|
|
||
| // OwnedHostFiles returns every file the agent installs under a single prefix. | ||
| // | ||
| // Teardown and the existing-deployment preflight both need this list, and they |
There was a problem hiding this comment.
This comment says the existing-deployment preflight uses this list, but it doesn't. existingDeploymentHostArtifacts (preflight_existing_deployment.go:157) checks only the two units and the recovery script. The doc comments on TestOwnedHostFilesAcrossCoversTheAbandonedLayout and TestRemoveAgentArtifactsSweepsEveryPrefix say the same thing.
The difference is what keeps Ignition working. Ignition writes <prefix>/bin/unbounded-agent before the first-boot preflight runs, so a preflight that checked this whole list would refuse every Ignition host. Please reword these comments to say that preflight deliberately checks a subset, and why, so nobody "fixes" the code to match them.
There was a problem hiding this comment.
Reworded in 08ef542: here, in the three test comments, in RemoveAgentArtifacts, and in the validate_reset_cleanup docstring. They now say preflight deliberately checks a subset, and why.
| } | ||
|
|
||
| return durableReset(ctx, store, inner, []string{"/etc", "/var/lib/machines", "/usr/local", store.Root()}, unix.Syncfs) | ||
| return durableReset(ctx, store, inner, teardownSyncPaths(r.HostPrefix, store.Root()), unix.Syncfs) |
There was a problem hiding this comment.
The prefix comes from two different sources here:
ResetAgent(line 29) andResetAgentResources(nodeoperator.go:238) callResolveHostPrefixwhen the task is built. That happens before the lock is held, and it falls back to the applied config.- This line uses
r.HostPrefixfromrecordForTeardown, which is""when the record was unreadable.
When the record is unreadable, teardown removes files under /opt/... but syncs only /usr/local.
Suggested fix: resolve the prefix once inside resetUnderLock (record first, then applied config) and pass the same value to both resetResources and teardownSyncPaths. For example, have ownedReset take a func(prefix string) phases.Task rather than a task that is already built. This also removes a read done outside the lock.
There was a problem hiding this comment.
Fixed in a296d7a as suggested. ownedReset and resetUnderLock take a func(prefix string) phases.Task, and the prefix is chosen once under the lock (record, then applied config) and used for both the teardown and the sync. It is also saved in the resetting record, so a retried reset still finds it after the applied config is gone. beginTeardown has a test per source.
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() |
There was a problem hiding this comment.
unittest.main() runs before TestBootstrapChoosesThePath and TestBootstrapCompletionIsFresh are defined, so python3 test_reinstall.py skips both classes. Discovery in CI isn't affected. test_ignition.py:181 has the same problem and skips TestIgnitionHostBoundaries. Move the guard to the end of each file.
| ]}, | ||
| } | ||
|
|
||
| def test_delivers_only_the_agent_payloads(self): |
There was a problem hiding this comment.
_reinstall_ignition_payload calls ignition_bootstrap_invocation(), which goes through the real bounded_ssh because that isn't patched here. The test passes only because a failed ssh returns "". Meanwhile it waits up to 15s on a connect timeout, or talks to a live e2e VM at VM_IP if one happens to be running.
Suggested fix: add patch.object(e2e, "ignition_bootstrap_invocation", return_value="").
There was a problem hiding this comment.
Fixed in e87af05; the test also asserts the returned invocation. Running the file directly used to spend 20s on that connect timeout.
| ukis = [n for n in fat.list_names("/EFI/Linux") if n.lower().endswith(".efi")] | ||
| if not ukis: | ||
| raise RuntimeError(f"{image} has no UKI under /EFI/Linux") | ||
| addon_dir = f"/EFI/Linux/{sorted(ukis)[0]}.extra.d" |
There was a problem hiding this comment.
A few robustness nits in this function:
sorted(ukis)[0]picks the alphabetically first UKI. With more than one (an A/B layout) that may not be the one systemd-boot boots, and the only symptom would be a long SSH wait. I'd raise iflen(ukis) != 1.- The capacity check and padding (lines 485, 498, 504) use
len(merged), which counts characters rather than encoded bytes. NbdServer.__init__doesn't callclose()on theqemu-nbd exitedpath (line 174), so the temp directory leaks.
There was a problem hiding this comment.
All three fixed in 35a416a: exactly one UKI is required, lengths are in encoded bytes (the read-back compares bytes, and the capacity test now calls the code), and a failed qemu-nbd start removes its temporary directory. Checked by patching and reading back an ACL image with a non-ASCII argument.
| // Deprecated: use ResolvedAgentUpgradePathsFor, which resolves the binaries | ||
| // under a configured installation prefix. This entry point is equivalent to | ||
| // passing an empty prefix and is kept for callers outside this repository. | ||
| func ResolvedAgentUpgradePaths() (AgentUpgradePaths, error) { |
There was a problem hiding this comment.
Question: does anything outside this repo import pkg/agent? After this PR nothing in the repo calls this or the other four Deprecated: wrappers (CheckExistingDeployment, EnsureNoExistingDeployment, CheckHostOSConfiguration, EnsureNSpawnLifecycleHelper). If there are no outside callers, the wrappers and the three tests that pin their signatures can be removed or the functions simply changed in place.
There was a problem hiding this comment.
The only outside importer I can find is AKSFlexNode (GitHub code search, so private repositories aren't covered), and neither its main branch nor its ACL PR calls these five. We're keeping them anyway, since private consumers can't be ruled out and a deprecated wrapper costs little.
| // default on a host where bootstrap failed before then. Callers that must be | ||
| // right in that case should ask the installation record first, which carries the | ||
| // same prefix and is written before the first host mutation. | ||
| func HostPrefixFromAppliedConfig(log *slog.Logger) string { |
There was a problem hiding this comment.
Question: FindActiveMachine checks the applied config's checksum sidecar before trusting the file. This reads the same files without that check. The result decides which directories get written to and swept, so should it go through the same verification, or take the prefix from FindActiveMachine's result?
There was a problem hiding this comment.
Yes, it should. 62918ff applies the same sidecar check: a slot that fails it is skipped like an unreadable one, and a slot with no sidecar is still used, as FindActiveMachine does. I didn't route it through FindActiveMachine itself, because that fails hard, logs on every call, and would drop the behavior where a corrupt slot does not mask the other one.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Ignition format reporting, preflight coverage, input validation, CI credential gating, and documentation contain unresolved correctness issues.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (7)
Check all owned host files for stale artifacts · New Require all Azure login inputs before enabling ACL matrix · New Validate fetch source authority for hierarchical schemes · New Validate digest syntax before contacting the cluster · New Record Ignition provisioning format in agent configuration · New Check LocalDNS helper directory writability · New Align documented checksum filename with release artifact · New
What changed in this PR
Adds immutable-host support through configurable installation prefixes, Ignition provisioning, reset/preflight updates, and Azure Container Linux end-to-end coverage.
Changes:
- Resolves host binaries and helpers beneath a configurable prefix.
- Adds validated Ignition bootstrap generation and lifecycle handling.
- Extends reset, upgrade, documentation, and CI coverage for immutable hosts.
| File | Description |
|---|---|
pkg/agent/phases/rootfs/nspawn.go |
Uses the resolved lifecycle-helper path. |
pkg/agent/phases/rootfs/nspawn_render_test.go |
Tests prefixed nspawn unit rendering. |
pkg/agent/phases/rootfs/lifecycle_helper.go |
Supports configurable helper installation paths. |
pkg/agent/phases/rootfs/lifecycle_helper_test.go |
Tests configurable and legacy helper paths. |
pkg/agent/phases/reset/reset_test.go |
Tests read-only-prefix cleanup behavior. |
pkg/agent/phases/reset/network.go |
Cleans LocalDNS artifacts across prefixes. |
pkg/agent/phases/reset/helpers.go |
Checks artifact existence before removal. |
pkg/agent/phases/nodestart/localdns.go |
Uses the resolved LocalDNS helper path. |
pkg/agent/phases/nodestart/localdns_test.go |
Tests prefixed LocalDNS unit rendering. |
pkg/agent/phases/nodestart/assets/unbounded-localdns-network.service |
Parameterizes the helper executable path. |
pkg/agent/phases/host/preflight_host.go |
Makes install-directory checks prefix-aware. |
pkg/agent/phases/host/preflight_host_test.go |
Tests prefixed preflight and deployment detection. |
pkg/agent/phases/host/preflight_existing_deployment.go |
Searches configured and default prefixes. |
pkg/agent/phases/host/configure_nftables_test.go |
Tests firewall-unit ordering. |
pkg/agent/goalstates/rootfs.go |
Carries the resolved lifecycle-helper path. |
pkg/agent/goalstates/resolve.go |
Resolves prefixed rootfs paths. |
pkg/agent/goalstates/resolve_test.go |
Tests lifecycle-helper resolution. |
pkg/agent/goalstates/localdns.go |
Resolves the LocalDNS helper path. |
pkg/agent/goalstates/localdns_test.go |
Tests LocalDNS prefix resolution. |
pkg/agent/goalstates/hostpaths.go |
Defines host-prefix layouts and ownership lists. |
pkg/agent/goalstates/hostpaths_test.go |
Tests host-path resolution and prefix merging. |
pkg/agent/goalstates/constants.go |
Shares the first-boot unit name. |
pkg/agent/goalstates/agentupgrade.go |
Makes upgrade paths prefix-aware. |
pkg/agent/goalstates/agentupgrade_test.go |
Tests prefixed upgrade layouts. |
pkg/agent/config/config.go |
Adds and validates HostPrefix. |
pkg/agent/config/config_test.go |
Tests host-prefix validation. |
internal/provision/script_test.go |
Tests prefixed installer behavior. |
internal/provision/assets/unbounded-agent-install.sh |
Installs the agent under AGENT_PREFIX. |
hack/agent/e2e-kind/test_ukiboot.py |
Tests UKI PE-header manipulation. |
hack/agent/e2e-kind/test_reliability.py |
Preserves disks during reinstall tests. |
hack/agent/e2e-kind/test_reinstall.py |
Tests same-disk Ignition reinstall behavior. |
hack/agent/e2e-kind/test_ignition.py |
Tests Ignition harness transformations. |
hack/agent/e2e-kind/test_host_image.py |
Tests ACL image selection and verification. |
hack/agent/e2e-kind/README.md |
Documents ACL e2e operation. |
docs/content/guides/agent.md |
Documents immutable-host provisioning. |
designs/agent-upgrade.md |
Documents prefix-aware upgrade paths. |
cmd/kubectl-unbounded/app/machine_manual_bootstrap.go |
Adds Ignition generation and prefix flags. |
cmd/kubectl-unbounded/app/machine_manual_bootstrap_test.go |
Tests Ignition bootstrap output and validation. |
cmd/kubectl-unbounded/app/ignition.go |
Defines the emitted Ignition schema subset. |
cmd/kubectl-unbounded/app/ignition_test.go |
Tests Ignition encoding and validation helpers. |
cmd/agent/internal/installstate/store.go |
Persists the installation prefix. |
cmd/agent/internal/installstate/store_test.go |
Tests prefix persistence and compatibility. |
cmd/agent/internal/daemon/reset.go |
Makes reset prefix-aware and removes first-boot units. |
cmd/agent/internal/daemon/reset_test.go |
Tests reset ordering and durability paths. |
cmd/agent/internal/daemon/nodeoperator.go |
Propagates prefixes through lifecycle operations. |
cmd/agent/internal/daemon/migration_test.go |
Updates installation-record fixtures. |
cmd/agent/internal/daemon/lifecycle.go |
Makes daemon assets prefix-aware. |
cmd/agent/internal/daemon/lifecycle_test.go |
Tests prefixed daemon installation and cleanup. |
cmd/agent/internal/daemon/hostupgrade.go |
Renders upgrade assets using host paths. |
cmd/agent/internal/daemon/hostupgrade_test.go |
Updates host-upgrade construction tests. |
cmd/agent/internal/daemon/hostprefix.go |
Resolves prefixes from durable host state. |
cmd/agent/internal/daemon/controller_test.go |
Updates failure-signal invocation. |
cmd/agent/internal/daemon/controller_machineoperation.go |
Supplies logging during prefix resolution. |
cmd/agent/internal/daemon/agentupgrade.go |
Uses prefixed upgrade and signal paths. |
cmd/agent/internal/cmd/cmd.go |
Passes command context to signal handling. |
cmd/agent/internal/cmd/bootstrap.go |
Includes prefixes in bootstrap identity and syncing. |
cmd/agent/internal/cmd/bootstrap_test.go |
Tests fingerprint and sync behavior. |
cmd/agent/internal/cmd/agentupgrade.go |
Resolves host-upgrade paths dynamically. |
cmd/agent/internal/cmd/agentupgrade_test.go |
Updates upgrade command tests. |
cmd/agent/internal/bootstrap/coordinator.go |
Records prefixes and avoids redundant writes. |
cmd/agent/internal/bootstrap/coordinator_test.go |
Tests repair persistence and no-op behavior. |
.github/workflows/agent-e2e-kind.yaml |
Adds conditional ACL e2e execution. |
.github/actions/agent-e2e-kind-control-plane/action.yaml |
Installs UEFI firmware dependencies. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| for _, candidate := range goalstates.MergeHostPrefixes(prefix) { | ||
| artifacts = append(artifacts, existingDeploymentArtifact{ | ||
| description: "agent daemon recovery script", | ||
| path: goalstates.DaemonRecoveryScriptPath, | ||
| }, | ||
| path: goalstates.ResolveHostPaths(candidate).DaemonRecoveryScript, | ||
| }) |
| # Presence only. The image needs a federated login, and the entry is | ||
| # left out entirely until one is configured, rather than added and | ||
| # failed. GitHub masks the value; nothing here reads it. | ||
| ACL_CREDENTIAL: ${{ secrets.ACL_IMAGE_CLIENT_ID }} |
| switch parsed.Scheme { | ||
| case "http", "https", "tftp", "s3", "arn", "gs": | ||
| return true | ||
| default: | ||
| return false | ||
| } |
| if isEmpty(h.agentSHA256) { | ||
| return fmt.Errorf("--agent-sha256 is required with --variant %s; the digest for each release binary is published in checksums.txt", variantIgnition) | ||
| } |
| // the agent re-reads it long after bootstrap: the daemon and the nspawn | ||
| // lifecycle hooks are started by systemd and cannot inherit it from the | ||
| // environment that provisioned the host. | ||
| cfg.HostPrefix = strings.TrimSpace(h.hostPrefix) |
| func agentInstallDirs(prefix string) []string { | ||
| return []string{goalstates.ResolveHostPaths(prefix).BinDir} | ||
| } |
| --variant ignition \ | ||
| --host-prefix /opt/unbounded \ | ||
| --agent-url https://github.com/Azure/unbounded/releases/download/v0.8.1/unbounded-agent-linux-amd64 \ | ||
| --agent-sha256 "$(cat unbounded-agent-linux-amd64.sha256)" \ |
The first-boot unit runs start on every boot. On a reboot the daemon unit starts in the same transaction and is only active once the nspawn machine is up, and nothing ordered the first-boot unit after it. start therefore found the daemon not yet running, repaired it, and rewrote the install record. The unit now orders after the daemon unit. That exposes the next overlap: the daemon holds the installation lock while it migrates the host on startup, and start gave up at once if the lock was held, so each reboot logged a failed attempt before the retry succeeded. start now waits up to 30 seconds for the lock, as the daemon already does for start. The host reboot step on an Ignition host now fails if the unit retried, repaired the daemon, or rewrote the record, which it could not see before.
nftables-flush.service said Before=systemd-nspawn@.service. In a unit that is not itself a template, systemd fills in the missing instance with the unit's own name, so that meant systemd-nspawn@nftables-flush.service and ordered nothing. The [Install] RequiredBy= made every machine require the flush, which does not order them either, so the flush and the machine could start together. Only with LocalDNS enabled did anything order them, indirectly. Each machine's service override now orders it after the flush, and the inert line is gone. The test that pinned it as correct now pins its absence, and a rendering test pins the override's ordering. systemd-analyze on the rendered units reports the machine After the flush.
Reset built its task list from ResolveHostPrefix before taking the lock, which falls back to the applied config, but synced the filesystems of the record's prefix, which is empty when the record is unreadable. On such a host teardown removed files under the configured prefix and made only /usr/local durable. The prefix is now chosen once the lock is held, record first and then the applied config, and the same value builds the teardown and picks what to sync. It is also saved in the resetting record, so a reset retried after the applied config is gone still finds the same files.
validate only checked that --agent-sha256 was set. Its format was checked when rendering, after the cluster had been contacted, so a mistyped digest was still reported late, which is what the early check was added to prevent. validate now parses the digest and keeps the result, and the renderer uses it instead of checking the inputs again. The render-time refusal test goes, and its malformed-digest case joins the validate test, which already had the others.
The Ignition example read a .sha256 file that releases do not publish; they publish checksums.txt, and --agent-sha256 accepts a line from it. OwnedHostFiles and several tests said the existing-deployment preflight checks the same list teardown removes. It checks only the daemon units and the recovery script, and has to: the install script and Ignition place the agent binary before preflight runs, so checking the whole list would refuse every fresh host. The comments now say so. In the upgrade design, the sentence introducing the slot choice had been separated from the block it introduces.
HostPrefixFromAppliedConfig read the applied configs without the checksum sidecar check FindActiveMachine applies. The prefix it returns decides which directories are written to and swept, so a config that fails its checksum is now skipped like an unreadable one. A config with no sidecar is still used, as FindActiveMachine does.
- TestIgnitionSpecVersionIsPinned and the constant checks at the end of TestIgnitionFileModesSerializeAsDecimal compared constants with literals. TestIgnitionConfigOmitsEmptySections still pins the emitted version, and the "mode":384 check stays. - TestFirstBootBootstrapUnitNameIsShared compared a constant with a literal; the writer and reset already share the constant. - TestInstallBootstrapBinaryInstallsUnderThePrefix asserted that a t.TempDir() is not /usr/local. - TestNewRecordIsGivenAResolvedPrefix only read back what NewRecord was given; TestBootstrapFingerprintTracksTheInstallationPrefix covers what it claimed. - TestAgentStagesSyncThePrefixTheyWroteTo tested a one-line wrapper around HostPrefixOrDefault, not what the stages pass to SyncFilesystems.
The image cache was never saved. The combined cache action saves in a post step, which runs after Cleanup has deleted .vm-e2e. Restore and save are now separate, and the save runs once create-vm has verified the image. Both use the v6.1.0 pin the rest of the repository uses, and azure/login moves to v3.1.0 to match. An existing image was trusted without checking its digest, whether it came from the cache or from an interrupted download, which wrote to the final name. Existing images are now checked and downloaded again if they do not match, and downloads go to a .part file that is renamed once verified. The storage bearer token was passed in curl's arguments, which a failed command prints in the traceback, and GitHub does not mask a token minted during the job. It now reaches curl through --config on stdin, a failed download no longer prints the command, and the token is registered as a mask in both e2e.py and the workflow.
The workflow read latest.json to key the cache, and then every e2e.py process that needed the image read it again. A build published during the job left the cache key and the processes disagreeing, or a later process looking for a file that was never downloaded, and each read minted another storage token. The workflow now runs a new e2e.py resolve-host-image, which reads the manifest once and exports ACL_IMAGE_URL, ACL_IMAGE_SHA256 and ACL_IMAGE_BUILD_ID. Set together, they pin a build and the manifest is not read, which also gives local runs a real pin. On its own ACL_IMAGE_BUILD_ID only checks the manifest's build, which the README and docstrings now say instead of calling it a pin. The step also honors ACL_IMAGE_MANIFEST_URL now, which the README documents. A build id that is empty, not a string, or not a plain name is refused, since it names the cached file.
- The reinstall filtered the rendered config down to the agent's files, so an unexpected file or unit was skipped, although its test said the set was asserted. It now refuses anything but the agent's payloads and the harness's own additions. Its digest check compared the staged binary with a digest computed from that same file; the binary installed on the VM is checked instead. - reset-failed had become best-effort on every host. It is strict again except on Ignition hosts, where the refusal was seen. - The README's ACL example ran the configuration suite, which cannot work there: its scenarios supply their own agent, and the Ignition path only boots the one it staged. The example runs lifecycle, and both the suite and run_agent with AGENT_URL refuse an Ignition host up front. - unittest.main() sat above later test classes, so running test_reinstall.py or test_ignition.py directly skipped them, and one reinstall test made a real SSH call. Both are fixed.
- With more than one UKI under /EFI/Linux it patched the first by name, which may not be the one systemd-boot boots, and Ignition would then get no config URL. It now requires exactly one. - The capacity check, padding, and VirtualSize counted characters rather than encoded bytes, so a non-ASCII command line could overrun the section. They use the encoded length, the read-back compares bytes, and the test calls the code instead of restating its arithmetic. - A failed qemu-nbd start left its temporary directory, since __exit__ does not run for a constructor that raised. Checked by patching and reading back an Azure Container Linux image.
|
|
||
| with tempfile.TemporaryDirectory() as tmp: | ||
| with self.assertRaises(SystemExit): | ||
| downloads = self._acquire(tmp, b"wrong bytes") |
| patch.object(e2e, "die", side_effect=SystemExit) as died: | ||
| try: | ||
| e2e.check_reset_failed() | ||
| except SystemExit: |
The workflow now exports the resolved image as ACL_IMAGE_URL, ACL_IMAGE_SHA256 and ACL_IMAGE_BUILD_ID for the rest of the job, and the harness unit tests run in that job. The tests that resolve from a fake manifest took the pinned path instead and failed. They now clear the pin first.



Lets the agent run on hosts with a read-only
/usrand no package manager, such as Azure Container Linux (ACL), and adds an ACL e2e.Install prefix
The agent put its own files in
/usr/local, which is read-only on these hosts.--host-prefixmoves them:<prefix>/bin: daemon binaries, recovery script, nspawn lifecycle helper<prefix>/libexec: LocalDNS network helperPaths inside the nspawn machine,
/etc/unbounded/agentand/var/lib/unboundeddon't change. With no prefix set, the agent uses/usr/localas before.The prefix is stored in the ownership record before bootstrap first changes the host, and it's part of the bootstrap fingerprint only when it isn't the default, so existing hosts keep theirs.
Every path that was hardcoded to
/usr/localnow uses the prefix: install, the paths in generated systemd units, filesystem sync, reset, the existing-deployment preflight, and the install script. Reset and preflight check both the configured prefix and the default, so files left by an earlier install under a different prefix are still found.Ignition
--variant ignitionemits an Ignition config. It requires--host-prefix,--agent-url(the bare binary, not the tarball) and--agent-sha256, and checks them before contacting the cluster.The first-boot unit uses
AssertPathExists, so a missing binary fails visibly, and its retries back off to a 5-minute cap.E2E
HOST_BASE_OS=aclruns the lifecycle and fresh-bootstrap suites on ACL. The image comes from the publishedlatest.jsonmanifest and is checked against its sha256. The Ignition config URL is added to the kernel command line by patching a UKI addon on the EFI partition; seeukiboot.pyfor why the boot chain is left in place.CI reads the image with a federated identity that can only read the
imagescontainer. The ACL entry is skipped on fork PRs and whenever the credentials aren't set.Bugs the e2e found
/usr/local/bininstead of the prefix, so it rejected hosts that could have been provisioned.EROFS, notENOENT.enable --nowdid nothing.iptables.service. That fix is already on main from Report host provisioning capability truthfully #814; this adds a test for it.