Worktree fix luks soft reboot

[prestist]

Add x-initrd.attach option unconditionally to all LUKS crypttab entries.
This prevents systemd-cryptsetup-generator from adding
Conflicts=umount.target, which is necessary for soft-reboot to work
correctly with LUKS.

Summary by CodeRabbit

• Bug Fixes

  • Improved LUKS soft-reboot reliability by adding the required initramfs attachment option to crypttab entries.
  • Correctly handles encrypted devices that depend on network availability during startup.

• Tests

  • Added coverage for crypttab entries with and without network-dependent Clevis configurations.

[gemini-code-assist]

Code Review

This pull request introduces logic to detect if the system is running on ostree and updates the crypttab generation to include the 'x-initrd.attach' option when necessary. I have identified an issue where the ostree marker file is incorrectly being checked within the target sysroot instead of the host's runtime directory. Additionally, I have suggested updating the detection logic to support an environment variable override for better testability and recommended updating the test case to verify the full crypttab output.

[travier]

This looks OK. But why is this CoreOS specific? Isn't Image Mode / bootc in the same boat here?

[prestist]

@travier ah im sorry, yeah my description was a little too specific sounding, I was just giving examples. The changes also would fix it for bootc / Image Mode. I will update the description.

Unless you were talking more about why I was conditionality pivoting for ostree systems only?

[travier]

I think my question is: What happens on Image Mode? If you take a bootc image and install it via Anaconda, does soft-reboot works? Is it anaconda that does the same workaround that we are adding here? Then adding it to Ignition makes sense as we use it instead of Anaconda. Otherwise if this does not work on Image Mode then we should probably figure out a fix that does not rely on Ignition.

[prestist]

I think my question is: What happens on Image Mode? If you take a bootc image and install it via Anaconda, does soft-reboot works? Is it anaconda that does the same workaround that we are adding here? Then adding it to Ignition makes sense as we use it instead of Anaconda. Otherwise if this does not work on Image Mode then we should probably figure out a fix that does not rely on Ignition.

oooh that is a very fair concern, to be honest I am not sure; I can do some research but it is out of my initial knowledge scope.

[prestist]

Alright so I did look into anaconda, its doing the same thing mostly. Its adding x-initrd.attach unconditionally for any LUKS device. @travier , In which case maybe we just drop our conditional. Makes it cleaner?

rhinstaller/anaconda#6785

[travier]

Thanks for finding rhinstaller/anaconda#6785, this helps with the context. Reading https://redhat.atlassian.net/browse/RHEL-112702 as well, this appear to be needed (wanted?) only for the root device?

[travier]

Are we writing a crypttab entry for the root device today?

[prestist]

Are we writing a crypttab entry for the root device today?

From my understanding there is no special case for root, so yes, if root is configured to be encrypted.

ignition/docs/operator-notes.md

Line 161 in 2446e1f

Ignition generates entries in `/etc/crypttab` for each device and expects that the operating system has hooks to be able to unlock the device (e.x.: `systemd-cryptsetup-generator`).

ignition/internal/exec/stages/files/filesystemEntries.go

Lines 38 to 98 in e3c445c

	func (s *stage) createCrypttabEntries(config types.Config) error {
	if len(config.Storage.Luks) == 0 {
	return nil
	}
	s.PushPrefix("createCrypttabEntries")
	defer s.PopPrefix()
	path, err := s.JoinPath("/etc/crypttab")
	if err != nil {
	return fmt.Errorf("building crypttab filepath: %v", err)
	}
	crypttab := fileEntry{
	types.Node{
	Path: path,
	},
	types.FileEmbedded1{
	Mode: cutil.IntToPtr(0600),
	},
	}
	extrafiles := []filesystemEntry{}
	for _, luks := range config.Storage.Luks {
	out, err := exec.Command(distro.CryptsetupCmd(), "luksUUID", util.DeviceAlias(*luks.Device)).CombinedOutput()
	if err != nil {
	return fmt.Errorf("gathering luks uuid: %s: %v", out, err)
	}
	uuid := strings.TrimSpace(string(out))
	netdev := ""
	if len(luks.Clevis.Tang) > 0 || cutil.NotEmpty(luks.Clevis.Custom.Pin) && cutil.IsTrue(luks.Clevis.Custom.NeedsNetwork) {
	netdev = ",_netdev"
	}
	keyfile := "none"
	if !luks.Clevis.IsPresent() {
	keyfile = filepath.Join(distro.LuksRealRootKeyFilePath(), luks.Name)
	// Write keyfile into sysroot
	contentsUri, ok := s.State.LuksPersistKeyFiles[luks.Name]
	if !ok {
	return fmt.Errorf("missing persisted keyfile for %s", luks.Name)
	}
	keyfilePath, err := s.JoinPath(keyfile)
	if err != nil {
	return fmt.Errorf("building keyfile path: %v", err)
	}
	extrafiles = append(extrafiles, fileEntry{
	types.Node{
	Path: keyfilePath,
	},
	types.FileEmbedded1{
	Contents: types.Resource{
	Source: &contentsUri,
	},
	Mode: cutil.IntToPtr(0600),
	},
	})
	}
	uri := dataurl.EncodeBytes([]byte(fmt.Sprintf("%s UUID=%s %s luks%s\n", luks.Name, uuid, keyfile, netdev)))
	crypttab.Append = append(crypttab.Append, types.Resource{
	Source: &uri,
	})
	}

[travier]

Thanks, code LGTM but if I understand correctlty, we should do that only for the root device and not for all mounts as those would be unmounted and re-mounted as needed on a soft reboot.

[travier]

Another interesting element is that this will not fix soft reboot for existing systems, only new ones as Ignition only runs once: rhinstaller/anaconda#6785 (comment). I'll file a tracking issue so that we remember that.

[prestist]

hmm looking at this more, I am questioning if this fix belongs in Ignition at all. The root LUKS device is set up by coreos-installer right?
If so and if the soft-reboot issue is specifically about the root LUKS device, this change in Ignition wouldn't fix it. I think that adding x-initrd.attach to non-root data volumes shouldn't hurt, but its starting to seem like its not enough?

wdyt? should this be a coreos-installer change instead?

[travier]

The root LUKS device is setup by ignition on first boot via the transpose-fs logic that copies the rootfs content into ram, then sets up the LUKS device and then copies the rootfs content back. So I think this is the right place. Can you make a kola test for soft-reboot on LUKS in fedora-coreos-config? That should let us validate that this is the right fix.

[prestist]

The root LUKS device is setup by ignition on first boot via the transpose-fs logic that copies the rootfs content into ram, then sets up the LUKS device and then copies the rootfs content back. So I think this is the right place. Can you make a kola test for soft-reboot on LUKS in fedora-coreos-config? That should let us validate that this is the right fix.

Thank you, yeah I can absolutely do that.

[prestist]

I added a test here: coreos/fedora-coreos-config#4201 and it validates this change.

[Rolv-Apneseth]

Thanks, code LGTM but if I understand correctlty, we should do that only for the root device and not for all mounts as those would be unmounted and re-mounted as needed on a soft reboot.

Has this been addressed? It does look like anaconda's only doing it for the root device.

[prestist]

Thanks, code LGTM but if I understand correctlty, we should do that only for the root device and not for all mounts as those would be unmounted and re-mounted as needed on a soft reboot.

Has this been addressed? It does look like anaconda's only doing it for the root device.

I am leaning twoards keeping it unconditional. Ignition's LUKS config has no concept of "root vs non-root" if we want to determine the root device we need to cross-reference Storage.Luks with Storage.Filesystems looking for path: "/", which may be fragile logic. Adding x-initrd.attach to non-root LUKS devices should be harmless, as it just prevents systemd-cryptsetup-generator from adding Conflicts=umount.target, which should not interfere with normal unmount/remount behavior.

I think the reason Anaconda can scope to root-only because it has explicit knowledge of which device is root via storage.root_device. Ignition doesn't have that context.

[Rolv-Apneseth]

Fair enough, in that case LGTM

[coderabbitai]

📝 Walkthrough

Walkthrough

Crypttab generation now uses a helper that always adds x-initrd.attach and conditionally adds _netdev for network-dependent Clevis configurations. New table-driven tests cover both option combinations and generated crypttab lines, and release notes document the fix.

Changes

Crypttab option generation

Layer / File(s)	Summary
Crypttab option construction internal/exec/stages/files/filesystemEntries.go	Adds centralized crypttab option generation with x-initrd.attach, conditional _netdev, and updated entry construction.
Option coverage and release note internal/exec/stages/files/filesystemEntries_test.go, docs/release-notes.md	Tests network and non-network options, including Clevis entries, and documents the LUKS soft-reboot fix.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

• ostree systems with LUKS fails to boot after a systemctl soft-reboot #2216 — The change implements the proposed x-initrd.attach fix for LUKS teardown during ostree soft-reboots.

Suggested reviewers: bgilbert, jlebon, madhu-pillai, travier, yasminvalim

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title matches the main change: a fix for LUKS soft-reboot behavior.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Binary size report (bin/amd64/ignition)

	Size
Base (main)	21MiB
PR (#2219)	21MiB
Delta	+128B (0.00%)

[coderabbitai]

🧹 Nitpick comments (1)

internal/exec/stages/files/filesystemEntries.go (1)

78-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider covering hasNetworkDev derivation directly in tests.

Table-driven test composes full crypttab lines for Clevis using none luks plus the buildCrypttabOptions output, verifying correct placement of both _netdev and x-initrd.attach for network/non-network cases. The new tests exercise buildCrypttabOptions directly but not the boolean expression at Line 78 (len(luks.Clevis.Tang) > 0 || cutil.NotEmpty(luks.Clevis.Custom.Pin) && cutil.IsTrue(luks.Clevis.Custom.NeedsNetwork)), which mixes &&/|| without parentheses. The logic itself matches the existing checkNeedsNet-style contract used elsewhere, but a direct unit test (or explicit parentheses for readability) would guard against future precedence mistakes.

Also applies to: 104-105

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/exec/stages/files/filesystemEntries.go` at line 78, Make the
hasNetworkDev derivation explicit by parenthesizing the Custom.Pin and
NeedsNetwork conjunction, preserving the existing Tang-or-custom-network
behavior. Add focused table-driven coverage for Tang, custom PIN with network
enabled, and non-network combinations so the boolean expression is tested
independently of full crypttab construction and buildCrypttabOptions.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@internal/exec/stages/files/filesystemEntries.go`:
- Line 78: Make the hasNetworkDev derivation explicit by parenthesizing the
Custom.Pin and NeedsNetwork conjunction, preserving the existing
Tang-or-custom-network behavior. Add focused table-driven coverage for Tang,
custom PIN with network enabled, and non-network combinations so the boolean
expression is tested independently of full crypttab construction and
buildCrypttabOptions.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Enterprise

Run ID: 975fb57b-1289-4d95-b79a-3a2cdd88c079

📥 Commits

Reviewing files that changed from the base of the PR and between 9d28e04 and d07526d.

📒 Files selected for processing (3)

• docs/release-notes.md
• internal/exec/stages/files/filesystemEntries.go
• internal/exec/stages/files/filesystemEntries_test.go