Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

boot.mount: mount by UUID instead #1256

Merged
merged 7 commits into from
Oct 26, 2021

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Oct 1, 2021

This makes the mount more resistant to other filesystems labeled boot
that may get plugged in at any point.

Part of: coreos/fedora-coreos-tracker#976

@jlebon jlebon force-pushed the pr/boot-mount-uuid branch from 22fcb47 to fd174d9 Compare October 1, 2021 21:55
@jlebon
Copy link
Member Author

jlebon commented Oct 1, 2021

Updated!

Comment on lines +37 to +38
# And similarly, only inject boot= if it's not already present.
boot=$(karg boot)
Copy link
Member Author

Choose a reason for hiding this comment

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

E.g. in FIPS mode, we'll already have a boot= karg there. (We should change that code to use boot=UUID too.)

cgwalters
cgwalters previously approved these changes Oct 1, 2021
Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

I didn't do a line-by-line analysis, but this looks great to me!

# Copied from
# https://github.com/dracutdevs/dracut/blob/9491e599282d0d6bb12063eddbd192c0d2ce8acf/modules.d/99base/dracut-lib.sh#L586
# rather than sourcing it.
label_uuid_to_dev() {
Copy link
Member

Choose a reason for hiding this comment

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

Another thing we could put in rdcore in the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe. This runs in the real root, and so far rdcore has been about the initramfs only. It'd be a bit awkward, though I guess we could just embed the full dracut path to it?

Though in this case, since we want to just implement the same API as dracut/fips, it seems simpler to just copy-paste the API implementation itself. If there are bugs in there, at least we'll have the same bugs as dracut. :)

@cgwalters
Copy link
Member

I think this is also going to want some careful manual testing, definitely for things like upgrades etc.

@bgilbert
Copy link
Contributor

From a commit message:

There is precedence for this in the FIPS module.

I think you mean that there's precedent in the FIPS module, and not that the FIPS module has precedence over coreos-boot-mount-generator?

bgilbert
bgilbert previously approved these changes Oct 21, 2021
Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

LGTM!

Lifted from `coreos-boot-edit.sh`.

Prep for future patch.
With the new `karg` helper, this is trivial to do now.
There are already other places which assume that `rdcore` is in `PATH`
so drop the full path to be more generic.
This is analogous to `systemd-fstab-generator` parsing `root=` kargs.
There is a precedent for this in the FIPS module. In fact, we lift code
from there to make sure we're API compatible.

The goal of supporting a `boot` karg is to eliminate possible races for
the `by-label/boot` symlink in the real root if multiple exist.

Part of: coreos/fedora-coreos-tracker#976
@jlebon jlebon dismissed stale reviews from bgilbert and cgwalters via d1c79d7 October 21, 2021 20:46
@jlebon jlebon force-pushed the pr/boot-mount-uuid branch from fd174d9 to d1c79d7 Compare October 21, 2021 20:46
@jlebon
Copy link
Member Author

jlebon commented Oct 21, 2021

From a commit message:

There is precedence for this in the FIPS module.

I think you mean that there's precedent in the FIPS module, and not that the FIPS module has precedence over coreos-boot-mount-generator?

Ahh yup, thanks!

I rebased this now and added a simple test. I forgot to do it in two force-pushes to make it easier to see the interdiff. But essentially the only changes are the additions in tests/kola/root-reprovision/luks/test.sh and fixing the precedent/precedence wording in the commit message.

@jlebon
Copy link
Member Author

jlebon commented Oct 26, 2021

Can I get another stamp on this?

Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

The new test is good as far as it goes, but it only tests the case where we're reprovisioning the rootfs. Should we also have a test for the completely vanilla boot case?

tests/kola/root-reprovision/luks/test.sh Show resolved Hide resolved
This ensures that the rootfs will always mount the correct boot
filesystem in the future (see previous patch).

Part of: coreos/fedora-coreos-tracker#976
The new `boot` karg will only show up on the second boot and onwards. So
for the first boot, let's persist it in `/run` to tell
`coreos-boot-mount-generator` the UUID to use.
@jlebon jlebon force-pushed the pr/boot-mount-uuid branch from d1c79d7 to 89066d5 Compare October 26, 2021 17:20
All our reboot tests currently target some special feature which
significantly modifies the system. But we should also have a reboot test
which is just using the defaults. This patch adds such a test.
@jlebon
Copy link
Member Author

jlebon commented Oct 26, 2021

The new test is good as far as it goes, but it only tests the case where we're reprovisioning the rootfs. Should we also have a test for the completely vanilla boot case?

Sure, done! I initially didn't do this because we don't have a vanilla test which reboots and it felt funny to tack it onto another existing rebooting test which was testing some orthogonal thing.

But I think we should cover the vanilla case more explicitly. I added a new rebooting testcase for this. Would be interesting to somehow merge that with misc-ro and other non-exclusive tests though it'd require some thought.

@jlebon
Copy link
Member Author

jlebon commented Oct 26, 2021

I think this is also going to want some careful manual testing, definitely for things like upgrades etc.

I added some tests here, but one test I did manually was boot up the very first stable release, bring it fully up to date and then update to this PR to confirm that the boot mount still works fine (i.e. correctly falls back to mounting by label). (Related: coreos/coreos-assembler#2519).

@jlebon jlebon merged commit 64c97fa into coreos:testing-devel Oct 26, 2021
@jlebon jlebon deleted the pr/boot-mount-uuid branch October 26, 2021 22:09
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