overlay/systemd: add boot.mount unit #105
Conversation
|
Related: #39 |
|
LGTM! Just one minor thing, probably worth a comment at the top of the file saying it came from fedora-coreos-config and probably basically the same text as the commit message? |
|
Fixed. |
|
LGTM! |
| Description=Boot partition | ||
| # From fedora-coreos-config | ||
| # This prevents the systemd-gpt-auto-generator from generating a mount unit for | ||
| # /boot using the ESP. |
There was a problem hiding this comment.
(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 ?
There was a problem hiding this comment.
No need for the Before=
https://www.freedesktop.org/software/systemd/man/systemd.mount.html#Default%20Dependencies
+1 to the doc
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I like the Documentation= idea.
Add boot.mount unit so the systemd gpt mount generator doesn't think the efi system partition should be /boot.
Add unit to mount the ESP as well.
|
Ok, added Tested on x86_64 with the rm-anaconda PR. |
| [Unit] | ||
| Description=EFI System Partition | ||
| Documentation=https://github.com/coreos/fedora-coreos-config | ||
| ConditionPathExists=/boot/efi |
There was a problem hiding this comment.
How about ConditionPathExists=/sys/firmware/efi/efivars as used by /usr/lib/systemd/system/dbxtool.service ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hmm. What value would it be to mount the ESP on BIOS?
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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...)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| Where=/boot/efi | ||
|
|
||
| [Install] | ||
| RequiredBy=local-fs.target |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
OK, let's merge this to unblock the Anaconda work. We can do tweaks as follow-ups! |
|
So oddly, the metal BIOS image for me right now hangs on trying to mount the ESP, which of course fails because Anaconda didn't create one. It's like the condition isn't inhibiting it. Also tried #106 with the same result, but haven't really debugged further. I guess this'll be fixed once we stop using Anaconda for the metal images as well, though we'll probably need to get to the bottom of this if we do decide we don't want to mount the ESP on BIOS systems but systemd disregards our conditionals. |
Add boot.mount unit so the systemd gpt mount generator doesn't think the
efi system partition should be /boot.