Skip to content

Support immutable hosts: configurable install prefix and Ignition provisioning - #815

Open
Philip Lombardi (plombardi89) wants to merge 46 commits into
mainfrom
acl-extract/immutable-host-paths
Open

Philip Lombardi (plombardi89) wants to merge 46 commits into
mainfrom
acl-extract/immutable-host-paths

Conversation

@plombardi89

@plombardi89 Philip Lombardi (plombardi89) commented Sep 17, 2026 •

Copy link
Copy Markdown
Collaborator

Lets the agent run on hosts with a read-only /usr and 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-prefix moves them:

  • <prefix>/bin: daemon binaries, recovery script, nspawn lifecycle helper
  • <prefix>/libexec: LocalDNS network helper

Paths inside the nspawn machine, /etc/unbounded/agent and /var/lib/unbounded don't change. With no prefix set, the agent uses /usr/local as 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/local now 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 ignition emits 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=acl runs the lifecycle and fresh-bootstrap suites on ACL. The image comes from the published latest.json manifest 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; see ukiboot.py for why the boot chain is left in place.

CI reads the image with a federated identity that can only read the images container. The ACL entry is skipped on fork PRs and whenever the credentials aren't set.

Bugs the e2e found

  • Preflight checked /usr/local/bin instead of the prefix, so it rejected hosts that could have been provisioned.
  • Reset failed on absent files under a read-only prefix, because unlinking there returns EROFS, not ENOENT.
  • Reset disabled the first-boot unit but didn't stop it, so a later reinstall's enable --now did nothing.
  • The nftables flush has to run after the image's own iptables.service. That fix is already on main from Report host provisioning capability truthfully #814; this adds a test for it.

Base automatically changed from acl-extract/bootstrap-recovery to main September 21, 2026 21:18
@plombardi89 Philip Lombardi (plombardi89) changed the title [WIP] Support immutable hosts and configurable installation paths [WIP] Support immutable hosts: configurable install prefix and Ignition provisioning Sep 21, 2026
Comment thread hack/agent/e2e-kind/test_host_image.py Fixed
Comment thread hack/agent/e2e-kind/test_ignition.py Fixed
@plombardi89
Philip Lombardi (plombardi89) force-pushed the acl-extract/immutable-host-paths branch 2 times, most recently from 8cd41ba to 9dc51dc Compare September 23, 2026 11:51
@plombardi89 Philip Lombardi (plombardi89) changed the title [WIP] Support immutable hosts: configurable install prefix and Ignition provisioning Support immutable hosts: configurable install prefix and Ignition provisioning Sep 23, 2026
@plombardi89
Philip Lombardi (plombardi89) marked this pull request as ready for review September 23, 2026 17:11
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.
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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 start likely sees the daemon inactive and repairs it. This is inferred from unit ordering; I haven't reproduced it.
  • Before=systemd-nspawn@.service in nftables-flush.service doesn't order the flush before the machines. systemd expands it to systemd-nspawn@nftables-flush.service, which I confirmed with systemd-analyze verify on systemd 258. The unit comes from #814, but the new test pins that line as correct.
  • The docs example reads a .sha256 file that releases don't publish. Only checksums.txt is published.
  • Reset gets the prefix from two sources. The task list is built from ResolveHostPrefix outside the lock, while the sync uses r.HostPrefix. They disagree when the record is unreadable.
  • A malformed --agent-sha256 is only rejected after contacting the cluster.
  • The OwnedHostFiles comment, 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-e2e before the cache post step runs.
  • The storage bearer token can leak into public logs through the CalledProcessError traceback.
  • sha256 is skipped for any image file that already exists, whether cache-restored or a partial download.
  • ACL_IMAGE_BUILD_ID doesn't pin a build, although the README says it does. An empty build_id is also accepted.
  • Each e2e.py process 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-failed is now best-effort on every host, not just ACL.
  • The README's run-local.sh command also runs the configuration suite, which, as far as I can tell, can't work with Ignition.
  • _reinstall_ignition_payload filters payloads although its test says they're asserted, and its digest check is circular.

Simplification, biggest wins first

  1. Resolve the prefix once per entry point and pass it down. ResolveHostPrefix reads the record from disk at 11 call sites.
  2. Revert the signal-operator plumbing. SignalPath doesn't depend on the prefix.
  3. Collapse KnownHostPrefixes, MergeHostPrefixes and OwnedHostFilesAcross into one function. Every caller passes a single prefix.
  4. Put the binary slot paths on HostPaths and derive AgentUpgradePaths from it, which removes the (paths, hostPaths) argument pairs.
  5. Keep one remove-if-exists helper in internal/fsutil instead of two copies plus an inline third.
  6. Reject surrounding whitespace in ValidateHostPrefix, then drop the scattered TrimSpace calls and their tests.
  7. Ignition: validate once, including the digest, and render the unit from an embedded template.
  8. Drop tests that restate constants or can't fail (called out inline).
  9. 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 or designs/. That would noticeably shrink a 5.8k-line diff.
  10. 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 five Deprecated: wrappers and the tests that pin their signatures can go.
  • HostPrefixFromAppliedConfig reads the applied config without the checksum check that FindActiveMachine does. 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")

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-active fails;
  • RepairDaemon rewrites the units and runs daemon-reload and systemctl start;
  • MarkComplete rewrites 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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread docs/content/guides/agent.md Outdated
--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)" \

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
--agent-sha256 "$(cat unbounded-agent-linux-amd64.sha256)" \
--agent-sha256 "$(grep ' unbounded-agent-linux-amd64$' checksums.txt)" \

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 08ef542, anchored so it does not also match the .tar.gz line.

Comment thread pkg/agent/goalstates/hostpaths.go Outdated

// OwnedHostFiles returns every file the agent installs under a single prefix.
//
// Teardown and the existing-deployment preflight both need this list, and they

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cmd/agent/internal/daemon/reset.go Outdated
}

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)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prefix comes from two different sources here:

  • ResetAgent (line 29) and ResetAgentResources (nodeoperator.go:238) call ResolveHostPrefix when the task is built. That happens before the lock is held, and it falls back to the applied config.
  • This line uses r.HostPrefix from recordForTeardown, 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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread hack/agent/e2e-kind/test_reinstall.py Outdated


if __name__ == "__main__":
unittest.main()

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e87af05, in both files.

]},
}

def test_delivers_only_the_agent_payloads(self):

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_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="").

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e87af05; the test also asserts the returned invocation. Running the file directly used to spend 20s on that connect timeout.

Comment thread hack/agent/e2e-kind/ukiboot.py Outdated
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"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 if len(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 call close() on the qemu-nbd exited path (line 174), so the temp directory leaks.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +28 to 31
// 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) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 High severity · 5 Medium severity · 1 Low severity

Open (7)
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.

Comment on lines +169 to +173
for _, candidate := range goalstates.MergeHostPrefixes(prefix) {
artifacts = append(artifacts, existingDeploymentArtifact{
description: "agent daemon recovery script",
path: goalstates.DaemonRecoveryScriptPath,
},
path: goalstates.ResolveHostPaths(candidate).DaemonRecoveryScript,
})
Comment on lines +82 to +85
# 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 }}
Comment on lines +99 to +104
switch parsed.Scheme {
case "http", "https", "tftp", "s3", "arn", "gs":
return true
default:
return false
}
Comment on lines +402 to +404
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)
Comment on lines +242 to 244
func agentInstallDirs(prefix string) []string {
return []string{goalstates.ResolveHostPaths(prefix).BinDir}
}
Comment thread docs/content/guides/agent.md Outdated
--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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants