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.

Sign up for GitHub

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

overlay/systemd: add boot.mount unit #105

Merged
merged 2 commits into from
Jun 20, 2019
Merged

overlay/systemd: add boot.mount unit #105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5b39633
overlay/systemd: add boot.mount unit
Jun 13, 2019
096f4c6
overlay/systemd: add /boot/efi mount unit
Jun 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions overlay/usr/lib/systemd/system-preset/42-coreos.preset
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,5 @@ enable afterburn-checkin.service
enable afterburn-firstboot-checkin.service
# Service to write SSH key snippets from cloud providers.
enable [email protected]
# mount /boot/efi
enable boot-efi.mount
11 changes: 11 additions & 0 deletions overlay/usr/lib/systemd/system/boot-efi.mount
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
[Unit]
Description=EFI System Partition
Documentation=https://github.com/coreos/fedora-coreos-config
ConditionPathExists=/boot/efi
Copy link
Member

Choose a reason for hiding this comment

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

How about ConditionPathExists=/sys/firmware/efi/efivars as used by /usr/lib/systemd/system/dbxtool.service ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still want to mount it on combined UEFI+BIOS image for consistency, yes? We want to key off what image we're using instead of what mode we booted in.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. What value would it be to mount the ESP on BIOS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In practice I can't think of any. But I was to preserve consistency. One of the things I like from CL is things largely happen the same on different platforms. That's one of the reasons why we're shipping one image to all the clouds. Ultimately this should be invisible to users.

I don't feel super strongly on this but would be curious what other historically CL/Atomic folks thought. cc @bgilbert @jlebon ?

Copy link
Member

Choose a reason for hiding this comment

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

No strong feelings either. I'm not sure of the ramifications either way. (Though related, I was just this morning catching up on the latest posts on the Silverblue board, and I saw this post about trying to install both BIOS and UEFI, which I see you've already replied to. :) -- I guess in such a case, there is some value in having it mounted so you can e.g. update ESP files even if the current machine you're on is BIOS. That use case doesn't translate quite as well for FCOS though...)

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, in that case though, why do we need this conditional at all? systemd already knows to order boot.mount before boot-efi.mount, right? Ahh, or this is to account for the fact that coreos/coreos-assembler#556 doesn't yet handle metal BIOS images as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well that, but that should be fixed soon. I was more thinking of arches other than x86_64 and arm64 where there is no EFI.


[Mount]
What=/dev/disk/by-label/EFI-SYSTEM
Where=/boot/efi

[Install]
RequiredBy=local-fs.target
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this and the preset blurb for boot.mount as well?
Could also statically enable those by dropping a symlink in the overlay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why we don't need it in boot.mount (which kind of scares me), but we don't in CL either. Maybe something about the mounts under / are special?

👍 to static enablement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I'm not sure how I feel about static enablement anymore. It would either need to go in /usr which would not be disable-able aside from masking or in /etc which isn't where vendor-provided stuff should live. Plus don't we have https://github.com/coreos/fedora-coreos-config/blob/testing-devel/fedora-coreos-base.yaml#L124 ?

Copy link
Member

Choose a reason for hiding this comment

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

Not to rehash the whole convo in #77, but yup presets are fine too. I guess I see those mounts as part of the OS (i.e. can't update without it), so you wouldn't want to disable it, unless you really know what you're doing (in which case having to resort to mask seems acceptable to me).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just a note: we can update without it, even on uefi. The only thing in /boot/efi is grub itself. Grub is configured to use /boot regardless of whether it's bios or uefi.

Copy link
Member

Choose a reason for hiding this comment

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

we can update without it, even on uefi

Right, speaking more to /boot in general. Well, we still could without a unit, but we'd have to teach OSTree to mount and and then unmount or something.

9 changes: 9 additions & 0 deletions overlay/usr/lib/systemd/system/boot.mount
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
[Unit]
Description=Boot partition
Documentation=https://github.com/coreos/fedora-coreos-config
# This prevents the systemd-gpt-auto-generator from generating a mount unit for
# /boot using the ESP.
Copy link
Member

Choose a reason for hiding this comment

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

(Just comparing to the auto-generated boot.mount on my system). I think we may want Before=local-fs.target here, though it may be implied.

Also, instead of # From fedora-coreos-config, WDYT about Documentation=https://github.com/coreos/fedora-coreos-config ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need for the Before=
https://www.freedesktop.org/software/systemd/man/systemd.mount.html#Default%20Dependencies

+1 to the doc

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's what I thought. I guess the fstab generator just plops it in to be explicit. Cool with omitting it here I suppose.

Copy link
Member

Choose a reason for hiding this comment

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

I like the Documentation= idea.


[Mount]
What=/dev/disk/by-label/boot
Where=/boot