Skip to content

tests/kola: add soft-reboot test for LUKS root encryption#4201

Open
prestist wants to merge 2 commits into
coreos:testing-develfrom
prestist:testing-devel
Open

tests/kola: add soft-reboot test for LUKS root encryption#4201
prestist wants to merge 2 commits into
coreos:testing-develfrom
prestist:testing-devel

Conversation

@prestist

@prestist prestist commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Validate that soft-reboot works correctly when the root filesystem is on a LUKS device. This exercises the x-initrd.attach crypttab option added in coreos/ignition#2219.

Validate that soft-reboot works correctly when the root filesystem
is on a LUKS device. This exercises the x-initrd.attach crypttab
option added in coreos/ignition#2219.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new integration test to verify that soft-rebooting works correctly with LUKS root encryption. The feedback suggests refactoring duplicated verification logic into a helper function and handling potential trailing whitespace in the lsblk output to ensure robust string comparison.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread tests/kola/root-reprovision/luks/soft-reboot/test.sh
@prestist

prestist commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

I did end up verifying this test works locally as expected. i.e fails without ignition change and passes when ignition is provided with the latest change found in ignition/2219

@travier

travier commented Jun 9, 2026

Copy link
Copy Markdown
Member

Hum, I'm not sure I follow. Where do we set that ostree should do a soft reboot here? Don't we need to create something that trigger an update?

@travier

travier commented Jun 9, 2026

Copy link
Copy Markdown
Member

I think you should be able to do the workaround manually in this test for now (i.e. updating the /etc/fstab entry), thus proving that the fix is correct.

@prestist

prestist commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Hum, I'm not sure I follow. Where do we set that ostree should do a soft reboot here? Don't we need to create something that trigger an update?

From what I could tell the bug is systemd-cryptsetup-generator adding Conflicts=umount.target — that conflict fires during any soft-reboot, regardless of what triggered it, so I figured we could avoid the heavy'er path of using ostree. We can see the same error repo'd with these changes in the ci.

… test

Manually add x-initrd.attach to crypttab if not already present so the
test can validate the fix against images that don't yet include
coreos/ignition#2219. This commit should be reverted once the Ignition
fix is in the base image.
@prestist

Copy link
Copy Markdown
Contributor Author

@travier with the workaround the test passes, what should be the next steps?

@prestist prestist marked this pull request as ready for review July 6, 2026 15:24

@marmijo marmijo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

ok "root is on LUKS device"

# Workaround: manually add x-initrd.attach to crypttab if not
# already present. This can be removed once coreos/ignition#2219

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: since this is intentionally temporary and depends on the ignition PR, could we use the full PR URL here? It'll make it easier for someone cleaning this up later to find the dependency directly.

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.

3 participants