Don't fail if we can't mkdir efivars
Closes: #1031183
Merge request reports
Activity
BTW I think I have a simpler fix, as mentioned here:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1031183#49
Tomorrow I should have time to put together an MR based on that, probably combined with the improved template, and some of the other changes.
added 6 commits
Toggle commit listJust pushed some changes in here.
In case you're wondering why the pipeline ends up cloning the two test jobs on openQA, it's because I ran the pipeline by hand, and set
OQA_JOBSTOCLONEto181897 181911(I could have done that by specifying
-o ci.variable="OQA_JOBSTOCLONE=181897 181911"as an option togit pushif I'd remembered, but I forgot, hence the cancelled pipeline)Hmm -- I thought that the permissions problem might be due to you not having access to philh/debian-installer> but apparently not -- I'd like to get to the bottom of this, so am likely to keep triggering this pipeline until I persuade it to work (which, given that the error message contains no useful information, may take a little while).
Ah, I see -- I think that's failing to run because I don't have the right to trigger a child pipeline in your repo (which is probably a gitlab bug, because the MR does give me the right to push and trigger the top level of the pipeline, so it's a bit silly that if then decides I don't have the right for the second bit).
Anyway, I tried doing it from a test user and as you can see, the triggering of D-I works fine for them.
@arnaudr: please launch the pipeline yourself to see that the D-I bit gets at least as far as building a mini.iso for you?
It currently fails with permissions issues at the
openqa-linkstage, so that's the next bit to fix, but at least this should generate a mini.iso to test.
@philh I just started a pipeline by hand: https://salsa.debian.org/arnaudr/grub-installer/-/pipelines/568414. I left all the variables to their default value.
-
@philh I started the
D-Ijob by hand, and it failed atopenqa-link: "no permissions to trigger downstream pipeline". Yeah, the
openqa-linkjob is going to take a bit more thought, because it really ought to be using credentials belonging to the triggering user, so that they are seen as the owner of the jobs in openQA, but then again it's a bit of a pain to force people to do that, so I had thought that having a job that knew my credentials would do the trick, but gitlab doesn't want to launch that pipeline without the person doing it having full access to the repo, at which point the secrets are no longer secret :-/Anyway, the bit I was most keen to fix was letting people build a patched mini.iso with no extra effort, which works! \o/
@philh IIUC the purpose of @ema's commit c90e6643 was to address efivarfs mount failures on some EFI systems as described in bug #1031183, but your patchset reverts this.
Also efivarfs should not be mounted in /target when the installer booted in EFI mode but partman did not force UEFI installation and grub-installer installed grub-pc, else update-grub may add a "UEFI firmware" menu entry which makes no sense in legacy/BIOS mode. See commit c3d50c51 "Do no run EFI probes in target chroot if ignore_uefi is set" in MR !16 (merged) and bug #1035085 for details.
Edited by Pascal HambourgYes please, as @pham mentioned failures to mount efivarfs should be non-fatal.
@ema: Sorry, I'd not noticed any mention in the bugs of this being about EFI systems failing. It seemed likely that your non-fatal fix was only required to work around the bug of trying to mount efivarfs on a non-EFI system.
If you're saying that it also happens on EFI systems, then of course we need to make it non-fatal (although, in that case, what's actually going on?).
BTW I note that the
trap "umount ...code is bogus, because it only unmounts the most recently mounted FS, rather than all three of them.I guess that
mountvirtfsneeds to accumulate the filesystems in a variable, and then have anunmountvirtfsthat can deal with multiple parameters to use in thetrap.@philh Non-EFI systems were already handled by checking if $fstype exists in /proc/filesystems, which worked until a change in linux 6.3 registers efivarfs when loading the efivarfs module even if efivars are not available. The module is unconditionnally loaded by partman-efi/init.d/efi regardless of the architecture. @ema's patch addressed only the special case when efivars are not available on an EFI system.
About fixing the trap code, umount (either util-linux or busybox implementation) can unmount multiple paths, so all you need is to accumulate mounted paths in a variable, last mounted first. Something like this: mounted_path="$path $mounted_path" trap 'umount $mounted_path' HUP INT QUIT KILL PIPE TERM EXIT
Edited by Pascal Hambourg@ema's patch does actually fix the BIOS case AFAICS (once the kernel change provoked a problem on BIOS systems) so while it may only have been intended to address the special case, the fact that it fixed both is where my flawed assumption seems to have come from.
BTW I notice that there are versions of
mountvirtfs()to be found in:rescue.d/80grub-reinstall rescue.d/81grub-efi-force-removableGiven that checking for the filesystem type in
/proc/filesystemsis no longer a worthwhile test of when a system is EFI, do these also need to be modified to deal with that? Is it useful to check for efi witharchdetectinstead/as well?Oh, and it seems that accumulating the unmounts in this case breaks things (one gets a failure with a log message saying it's not been possible to unmount /proc) so we seem to have been relying on that code being "wrong" -- it should probably be changed to explicitly attempt only to umount the efivarsfs, to make it clear that that is intentional.
@ema's patch does actually fix the BIOS case AFAICS (once the kernel change provoked a problem on BIOS systems) so while it may only have been intended to address the special case, the fact that it fixed both is where my flawed assumption seems to have come from.
Maybe I missed something but in my understanding @ema's patch predates the kernel change and does not fix the BIOS case, hence @arnaudr's follow-up to the bug report and MR. Maybe filing a new bug report would have been less confusing.
BTW I notice that there are versions of
mountvirtfs()to be found in:rescue.d/80grub-reinstall rescue.d/81grub-efi-force-removableDuplicated code is a plague for maintenance... Move it to functions.sh ?
Given that checking for the filesystem type in
/proc/filesystemsis no longer a worthwhile test of when a system is EFI, do these also need to be modified to deal with that? Is it useful to check for efi witharchdetectinstead/as well?Oh, and it seems that accumulating the unmounts in this case breaks things (one gets a failure with a log message saying it's not been possible to unmount /proc)
I guess this happens because the grub-installer script also mounts /proc if not mounted and unconditionnally unmounts it at the end. Maybe it is enough to just ignore unmount failures ?
By the way why is that code in postinst instead of the grub-installer script ? Why isn't efivarfs mounted by grub-installer only when it selects a GRUB variant for EFI boot ?
-
hence @arnaudr's follow-up to the bug report and MR. Maybe filing a new bug report would have been less confusing.
Correct, it's a different bug, a new bug should have been created, but it was not obvious at the beginning. Sorry for the confusion.
@pham I also thought this should be in
functions.sh, hence the commits leading to philh/grub-installer@63041427I think the idea of just not having this in the postinst is worth a look though, so I'll see if I can do something about that sortly ...
@pham - wow!
Thanks for spotting that, because I would never have seen it at this point -- for the simple reason that I know that it should be in reverse order, and was certain that I got it right (which is true, as you can see: philh/grub-installer@7117f4be).
The problem is that I then tried to minimise the diff, and in doing so copied the pre-existing bug in the
rescue.dscripts.Oh, and I guess that'll probably be the source of that /proc umount failure -- I'll take the workaround out and see how that goes while I'm about it...nope.Edited by Philip HandsI guess that'll probably be the source of that /proc umount failure
I don't think so, because there is no nested mount under /target/proc. As I wrote previously, I guess the failure happens because the grub-installer script also mounts /target/proc [1] if not mounted and unconditionnally unmounts it at the end, so it is already unmounted when postinst tries to unmount it. IMO /target/proc should be mounted only once either in postinst or grub-installer, and unmounted only if it was mounted by the current script.
[1] Note that grub-installer uses different options to mount /proc on kfreebsd, hurd and others whereas postinst doesn't make any difference.
Yeah, I worked that out rather soon after writing it (I edited the comment on salsa, but I guess that doesn't get seen via email)
I do agree that it seems that the (un)mounting should live in grub-installer rather than postinst.
I had a quick look at the grub-installer code, but didn't immediately see where the similar changes should be made. Suggestions welcome :-)
@philh Not sure what you mean by "similar changes". At first, I guess we could just remove mounting /target/proc from postinst, and let grub-installer mount it. Subsequently, it appears that $ROOT/proc and $ROOT/run (un)mounting in grub-installer has issues:
- the test used to determine if they are already mounted (presence of some contents) looks weak;
- they are unmounted on normal exit even if they were already mounted before entering the script and not mounted by the script;
- they are not unmounted on error exit.
I guess these issues could be fixed by using mountvirtfs for $ROOT/proc and a similar EXTRA_PATHS+trap logic for $ROOT/run, and removing the unconditional unmounts at the end of the script.
But looking further, the EXTRA_PATHS+trap logic is also used to (un)mount /target/boot/efi in rescue.d scripts, so what about moving this logic into a separate unmount_on_exit() function usable with any temporary mount ?
Looking even further, rescue-mode postinst already prompts to mount /boot/efi in /target, so do grub-installer rescue.d scripts need to mount it too ?
But isn't all this going well beyond the initial efivarfs topic ?
I pushed a draft (not tested) to https://salsa.debian.org/pham/grub-installer/-/commits/tmp-mount-and-files-draft/ Feedback welcome.
added 10 commits
- f86e8bc1 - move duplicated functions into functions.sh
- 1fa7db73 - give _gefr_log it's own name (it's not quite log)
- 3691edcb - merge 'efivarsfs' special case into functions.sh
- 04b5c84d - Don't fail if we can't mkdir efivars
- c923474b - mountvirtfs: separate success vs. fatality logic
- 13ec1318 - avoid tagging expected(ish) failure as 'error:'
- 1a2c13e0 - only try to mount /sys.../efi/... on efi systems
- 6d6ec9fc - mountvirtfs: introduce optional 'fatality' param
- da15ab00 - grub-installer/mounterr: say which path failed
- 6ea0de08 - insert new paths at the start of $EXTRA_PATHS
Toggle commit listBTW the full CI pipeline (including openQA tests) for the just-pushed (6ea0de08), can be seen here:
https://salsa.debian.org/philh/grub-installer/-/pipelines/569301 [updated link 2023-08-24]
I think that's looking pretty good now (
assuming it finisheswith green ticks across the board,which it should).Edited by Philip HandsI've slightly lost track of where we've got to with this, but I get the impression that philh/grub-installer@0321f5b3
efivars(rebased onto 1.195 and with the bits from @phamtmp-mount-and-files-draftcombined withefivarsfrom @arnaudr) is pretty decent.Unless there's a reason to keep
tmp-mount-and-files-draftseparate?Is this ready to merge do we think?
added 20 commits
-
db0c6672 - 1 commit from branch
installer-team:master - db0c6672...fd7440b8 - 9 earlier commits
- 0de831a5 - insert new paths at the start of $EXTRA_PATHS
- 7b45252a - Move unmount-on-exit logic into its own function
- d2c72ac5 - grub-installer: use unmount_on_exit with /proc and /run
- 37a23496 - Move /sys and efivarfs mounts from postinst to grub-installer
- a3e20267 - functions.sh: move trap action into its own function
- 43a81a87 - grub-installer: propagate ignore_uefi flag to target system for os-prober
- fbc9b10a - Use a combined stack for on_exit() cleanup tasks
- 715e1398 - tidy up log messages slightly
- 0321f5b3 - grub-installer: harden the creation of partman/ignore_uefi
- c0e0bbc5 - log: append the script's name if DEBCONF_DEBUG set
Toggle commit list-
db0c6672 - 1 commit from branch
mentioned in commit pham/grub-installer@01f88a65
added 20 commits
-
90dd2663 - 1 commit from branch
installer-team:master - 90dd2663...d9bb1437 - 9 earlier commits
- 9cf5afa7 - insert new paths at the start of $EXTRA_PATHS
- ceed8552 - Move unmount-on-exit logic into its own function
- 3d7fa495 - grub-installer: use unmount_on_exit with /proc and /run
- 2330279d - Move /sys and efivarfs mounts from postinst to grub-installer
- ff539681 - functions.sh: move trap action into its own function
- 3e2b2530 - grub-installer: propagate ignore_uefi flag to target system for os-prober
- ae69a6bc - Use a combined stack for on_exit() cleanup tasks
- 678867cc - tidy up log messages slightly
- 4694c89d - grub-installer: harden the creation of partman/ignore_uefi
- adfe0c27 - log: append the script's name if DEBCONF_DEBUG set
Toggle commit list-
90dd2663 - 1 commit from branch
I've just pushed the combined version (including @pham's branch) onto @arnaudr's -- I hope that's OK.
BTW pham/grub-installer@01f88a65 (comment 425317) describes how 3e2b2530 doesn't actually do what it is trying to do, so that still needs some work.
Fixup commit 0924e770 available at https://salsa.debian.org/pham/grub-installer/-/commits/efivars/
I tested that filesystem mount and unmount, ignore_uefi propagation and removal, OS detection and grub.cfg generation work as expected with the following setup:
- amd64 installer
- EFI Windows boot manager in the ESP
- legacy Windows boot manager in a FAT partition
in the following situations:
- EFI boot, install
- EFI boot, rescue, reinstall GRUB, force to the removable media path
- EFI boot, install, do not force EFI installation
- BIOS boot, install
- BIOS boot, rescue, reinstall GRUB
I could not test @ema's case of efivarfs mount failures on some EFI systems because I do not have such system.
Edited by Pascal Hambourg
added 1 commit
- 0924e770 - grub-installer: Move ignore_uefi propagation outside case $ARCH
added 14 commits
- 0924e770...5d7b0988 - 4 earlier commits
- bc1945e4 - avoid tagging expected(ish) failure as 'error:'
- 2b80ed9f - only try to mount /sys.../efi/... on efi systems
- da766c97 - mountvirtfs: introduce optional 'fatality' param
- a1e22352 - grub-installer/mounterr: say which path failed
- b878d83e - insert new paths at the start of $EXTRA_PATHS
- 01126606 - Move unmount-on-exit logic into its own function
- 4ce7d6c3 - Move /sys and efivarfs mounts from postinst to grub-installer
- e3b2ae33 - functions.sh: move trap action into its own function
- 136e0d72 - grub-installer: propagate ignore_uefi flag to target system for os-prober
- f14f43cf - Use a combined stack for on_exit() cleanup tasks
Toggle commit listmentioned in merge request !16 (merged)
