Skip to content

Conversation

cgwalters
Copy link
Member

Followup to #3513

Copy link

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

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 adds a test case to verify that unmounting /sysroot and /etc does not fail during a staged deployment. My review found a critical bug in the implementation due to a filename typo, which would cause the test to fail for the wrong reason. I've also provided a suggestion to improve the script's readability and robustness by combining two commands into one.

assert_streq "$syncfs" 2

# And verify there were no failures to unmount /etc or /sysroot
journalctl -b -1 -u sysroot.mount -u etc.mount >previous-journal.txt
Copy link
Member

Choose a reason for hiding this comment

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

Minor: looks like it accepts globs, so -u '*.mount' also works and catches more.

syncfs=$(journalctl -b -1 -u ostree-finalize-staged --grep='Completed syncfs.*for system repo' | wc -l)
assert_streq "$syncfs" 2

# And verify there were no failures to unmount /etc or /sysroot
Copy link
Collaborator

Choose a reason for hiding this comment

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

When doing soft-reboot I think we keep the full journal, but on normal reboot this would only catch failures before /var umount.
I think we want to duplicate this test for the soft-reboot case.

Best would be to have the VM serial output, but I don't know what kola can do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah you're right, moved to soft reboot.

Best would be to have the VM serial output, but I don't know what kola can do.

We do gather it, but it's currently outside what external tests can gate on with coreos-assembler. I'm working in the background on some things like bootc-dev/podman-bootc#99 which can help form the core of a more powerful container-native testing framework.

test '!' -f /run/ostree/nextroot-booted

# And verify there were no failures to unmount /etc or /sysroot
journalctl -b -1 -u sysroot.mount -u etc.mount >previous-journal.txt
Copy link
Collaborator

Choose a reason for hiding this comment

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

When you do a soft reboot, for the journal it's still the same boot, ie you must use -b0 here

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking about it more, the journal is pretty small, we can just remove the -b

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