Skip to content

fix(sdk): create the cloud-config directory before writing the boot event - #4870

Open
pujitha24 wants to merge 2 commits into
kairos-io:masterfrom
pujitha24:auto/issue-4864
Open

pujitha24 wants to merge 2 commits into
kairos-io:masterfrom
pujitha24:auto/issue-4864

Conversation

@pujitha24

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

On a UKI (trusted-boot) install or reset, the first boot's kairos-agent notify agent.boot step runs the cluster plugin's onBoot handler in the initramfs stage, before the pivot. That handler writes the provider's cluster config to /usr/local/cloud-config/cluster.kairos.yaml (or a custom ClusterConfigPath) by calling OpenFile with O_CREATE, which creates the file but not any missing parent directory.

On the GRUB (non-UKI) install/reset path, that directory already exists by the time this runs, because an earlier chrooted yip stage (after-install-chroot / after-reset-chroot, bind-mounting the persistent partition at /usr/local) has already run RunStage, which creates every GetCloudInitPaths() entry as a side effect. pkg/uki/install.go and pkg/uki/reset.go have no equivalent chrooted hook, so on first UKI boot the directory does not exist yet, OpenFile fails with ENOENT, the write is silently dropped (the failure is only visible in the plugin's own error field), no cluster config is ever written, and the node cannot bootstrap until a second boot recreates the directory through the normal post-pivot stage.

Approach: the issue suggests two alternative fixes: creating the directory explicitly in the UKI install/reset paths (mirroring the GRUB chroot hook), or making the initramfs-stage notify tolerant of a missing directory. This PR takes the second, smaller option: onBoot now creates the parent directory of the target config file (fsutils.MkdirAll, matching the 0755 permission already used for this exact directory in agent/pkg/utils/runstage.go) before opening the file. This is provider-agnostic (any provider writing under a not-yet-created cloud-config path benefits, not only the cluster plugin), a smaller diff, and does not depend on partition-mount timing during UKI install/reset. MkdirAll is idempotent, so the already-working GRUB path is unaffected.

Validation: added sdk/clusterplugin/plugin_test.go and sdk/clusterplugin/clusterplugin_suite_test.go, using vfst.NewTestFS to reproduce the exact failure from the issue at the unit level. Ran:

  • go test ./sdk/clusterplugin/... — passes (3/3 specs).
  • Confirmed the reproduction is real: reverted only the plugin.go fix (keeping the new tests) and reran — 2 of the 3 specs failed with the same open .../cluster.kairos.yaml: no such file or directory error quoted in the issue; restoring the fix made them pass again.
  • go build ./sdk/clusterplugin/..., go vet ./sdk/clusterplugin/..., and golangci-lint run ./sdk/clusterplugin/... (repo-pinned v2.13.2 per .golangci-version) all clean.
  • A full root go test ./... fails in many unrelated packages on this machine (macOS/arm64) due to Linux-only syscalls (syscall.Mount, netlink.FAMILY_ALL, CGO screenshot bindings, etc.) that this change does not touch; CI itself runs on ubuntu-latest.

This is a unit-level reproduction of the reported ENOENT, not a live UKI boot test. It was not verified against an actual QEMU/hardware UKI install or reset.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Fixes #4864

@ci-robbot

Copy link
Copy Markdown
Collaborator

Triage: the reproduction and the fix look right. One coordination note: this makes a third site that creates /usr/local/cloud-config, after agent/pkg/utils/runstage.go and the kairos-init fs.after stage whose dead guard is tracked in #4874 (fix in #4875), and each picks its own mode, so whichever runs first decides the permissions on that boot.

Worth agreeing the intended mode with #4875 rather than leaving 0755 here by default.

@ci-robbot

Copy link
Copy Markdown
Collaborator

One more coordination point: #4867's fix in #4873 rewrites this same onBoot block and already creates the parent directory, so the two PRs conflict textually. I have noted there that #4873 should rebase behind this one.

…vent

On a UKI (trusted-boot) install or reset, the first boot's `kairos-agent
notify agent.boot` step runs the cluster plugin's onBoot handler in the
initramfs stage, before the pivot. That handler writes the provider's
cluster config to /usr/local/cloud-config/cluster.kairos.yaml (or a
custom ClusterConfigPath) via OpenFile with O_CREATE, which creates the
file but not a missing parent directory.

On the GRUB (non-UKI) path the directory already exists by the time this
runs, because an earlier chrooted yip stage (after-install-chroot /
after-reset-chroot, with the persistent partition bind-mounted at
/usr/local) has already run RunStage, which creates every
GetCloudInitPaths() entry as a side effect. pkg/uki/install.go and
pkg/uki/reset.go have no equivalent chrooted hook, so on first UKI boot
the directory does not exist yet, OpenFile fails with ENOENT, the write
is silently dropped, and the node cannot bootstrap until a second boot
recreates the directory through the normal post-pivot stage.

Approach: the issue suggests two alternative fixes: creating the
directory explicitly in the UKI install/reset paths (mirroring the GRUB
chroot hook), or making the initramfs-stage notify tolerant of a missing
directory. This takes the second, smaller option: onBoot now creates the
parent directory of the target config file before opening it, using the
same 0755 permission already used for this exact directory in
agent/pkg/utils/runstage.go. This is provider-agnostic, a smaller diff,
and does not depend on partition-mount timing during UKI install/reset.
MkdirAll is idempotent, so the already-working GRUB path is unaffected.

Validation: added sdk/clusterplugin/plugin_test.go and
clusterplugin_suite_test.go, using vfst.NewTestFS to reproduce the exact
failure from the issue at the unit level. `go test ./sdk/clusterplugin/...`
passes (3/3 specs). Reverting only the plugin.go change while keeping the
new tests reproduced the reported failure: 2 of 3 specs failed with the
same "open .../cluster.kairos.yaml: no such file or directory" error
quoted in the issue; restoring the fix made them pass again. Also ran
`go build ./sdk/clusterplugin/...`, `go vet ./sdk/clusterplugin/...`, and
`golangci-lint run ./sdk/clusterplugin/...` (repo-pinned v2.13.2 per
.golangci-version), all clean. A full root `go test ./...` fails in many
unrelated packages on this machine (macOS/arm64) due to Linux-only
syscalls this change does not touch; CI runs on ubuntu-latest. This is a
unit-level reproduction of the reported ENOENT, not a live UKI boot test;
it was not verified against an actual QEMU/hardware UKI install or reset.

Report: kairos-io#4864
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Assisted-by: claude-sonnet-5 (via Claude Code)
@pujitha24

Copy link
Copy Markdown
Contributor Author

Heads up @[2026-09-23 @06:47:08] @Sweep @NVIDIA/TensorRT-LLM#19322: @clone/fetch @failed @[2026-09-23 @06:47:09] @Sweep @theagentrouter/agent-router#2517: @clone/fetch @failed @[2026-09-23 @06:47:11] @Sweep @vllm-project/aibrix#2773: @clone/fetch @failed @[2026-09-23 @06:47:11] @Sweep @future-agi/future-agi#2989: @clone/fetch @failed @[2026-09-23 @06:47:12] @Sweep @kubescape/kubescape#3905: @clone/fetch @failed @[2026-09-23 @06:47:14] @Sweep @vllm-project/vllm-omni#7265: @clone/fetch @failed @[2026-09-23 @06:47:15] @Sweep @vllm-project/vllm-omni#7267: @clone/fetch @failed @[2026-09-23 @06:47:16] @Sweep @open-telemetry/opentelemetry-go-contrib#9474: @clone/fetch @failed @[2026-09-23 @06:47:17] @Sweep @open-telemetry/opentelemetry-go-contrib#9476: @clone/fetch @failed @[2026-09-23 @06:47:18] @Sweep @open-telemetry/opentelemetry-go-contrib#95 — rebasing this onto the current base to clear a merge conflict dismissed your approval. The branch is now at a2494d6. Could you take another look and re-approve when you have a moment? Sorry for the extra round trip.

@mauromorales

Copy link
Copy Markdown
Member

@mauro-agent please review

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

The repository requires an end-to-end boot test for this UKI boot-path change.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Creates missing cloud-config parent directories before cluster providers write boot configuration.

Changes:

  • Creates default or custom configuration directories.
  • Adds unit coverage for missing directories and nil configuration.
File Description
sdk/​clusterplugin/​plugin.go Creates the target parent directory before writing.
sdk/​clusterplugin/​plugin_test.go Tests default, custom, and nil-config behavior.
sdk/​clusterplugin/​clusterplugin_suite_test.go Adds the Ginkgo suite entry point.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

// anything else has created the cloud-config directory (that normally
// happens as a side effect of the first RunStage call, post-pivot). Create
// it here rather than rely on that ordering.
if err := fsutils.MkdirAll(filesystem, filepath.Dir(configFilePath), 0755); err != nil {
@mauro-agent

Copy link
Copy Markdown
Contributor

⏳ Work claimed by mauro-agent, an AI agent operated by @mauromorales, at 2026-09-30T07:21:56Z. If you are another lane about to start substantial work here, please check for a follow-up comment before duplicating it -- this marker exists specifically to prevent two lanes from independently doing the same work.

@itxaka-agent

Copy link
Copy Markdown
Collaborator

Automated triage agent (kairos-triage-agent) running as @itxaka-agent.
This comment was generated by software. A human maintainer can override any
action taken here.

Picking this up for review: pulling the branch locally and checking the fix against the UKI install/reset path before posting findings.

@itxaka-agent itxaka-agent left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Automated triage agent (kairos-triage-agent) running as @itxaka-agent.
This comment was generated by software. A human maintainer can override any
action taken here.

Approved. The MkdirAll on the parent directory before OpenFile closes the ENOENT from #4864, the UKI boot DAG confirms the write lands on the persistent mount (not an ephemeral path that a later mount would hide), and the new tests fail without the fix and pass with it.

@mauro-agent mauro-agent 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.

🤖 Automated review by mauro-agent — an AI agent operated by @mauromorales. This is not Mauro's personal review; he has not read this PR unless he says so himself. If you want a human, feel free to ping him directly.

Static review only. I did not build this, run the tests, install anything, or execute any code in it. Every finding below comes from reading the diff and the changed files.

Summary: the change looks correct and small. No blocking findings.

What I checked

  • onBoot now calls fsutils.MkdirAll(filesystem, filepath.Dir(configFilePath), 0755) before OpenFile. That closes the ENOENT the linked issue describes, when the cloud-config directory does not exist yet in the initramfs stage.
  • It uses the same configFilePath as the write, so a custom ClusterConfigPath gets its parent created too. The second test covers this.
  • MkdirAll is a no-op when the directory exists, so the normal (post-pivot) path does not change.
  • The error is returned in response.Error with a clear message, like the neighbouring OpenFile error.
  • CI on the PR shows unit tests, lint, and DCO passing.

Small notes, none blocking

  1. The 0755 mode is hard-coded. I could not see from the diff what mode the first RunStage call gives this directory later. If the two differ, the mode depends on which code runs first. A shared constant, or a comment naming the expected mode, would make that clear.
  2. MkdirAll in sdk/utils/fs refuses on a read-only filesystem with a permission error. On a read-only root this now fails at MkdirAll instead of at OpenFile. The behaviour is the same (an error either way), but the message is different. Fine to leave.
  3. The three tests cover the missing default directory, a custom path, and a nil config.Cluster. A test for "directory already exists" would show the no-op claim, but it is a low-risk case.

I have not judged whether other code paths on a UKI install or reset also expect this directory. The diff only touches onBoot.

@pujitha24

Copy link
Copy Markdown
Contributor Author

On the mode question: 0755 here is meant to match what runstage.go ends up creating, since it passes constants.DirPerm (ModePerm) to MkdirAll and the umask brings that down to 0755. So the two sites agree today. I haven't changed anything in this PR. The tightening to root:admin 0770 looks like it belongs in #4875 (it covers the first-boot ordering), and if that lands first with a different mode I'm happy to rebase and follow it rather than pick one here.

On the boot test: I agree the unit specs can't show the pre-pivot ordering, and I haven't run a UKI install or reset boot for this change, only the in-memory specs. I'd like to extend tests/uki_test.go to check the provider config exists after the first boot and after reset, but I can't run that here. Would you rather I add it untested for CI to exercise, or hold it for someone who can boot it?

@ci-robbot

Copy link
Copy Markdown
Collaborator

Checked both points against master.

The mode claim holds: constants.DirPerm is os.ModeDir | os.ModePerm, so fsutils.MkdirAll at agent/pkg/utils/runstage.go:52 asks for 0777 and umask 022 lands it on 0755, the same as here. Agreed that the tightening belongs in #4875, which takes the directory to root:admin 0770 in the fs.after stage of 00_rootfs.yaml; rebasing behind it is the right order.

On the test: add it. tests/uki_test.go is not dead weight, it is the only place that can show the ordering. It is run by _uki-test.yaml, which master.yaml calls on every push to master (install-upgrade on standard, boot-assessment on core), so the assertion gets exercised as soon as this merges even though the default PR pipeline (pr.yaml) does not boot anything. On this PR itself it needs the ci/full label, and the authorize job in pr-full.yaml rejects the triage role, so a maintainer has to add it.

This branch was previously deployed

1 inactive deployment
fork-pr-builds — 15aafbc2 Deployed Sep 30, 2026 by pujitha24 via authorize #887
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.

UKI install and reset never create /usr/local/cloud-config, so a provider cannot bootstrap on first boot

6 participants