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

Include GRUB tools unconditionally and don't create $VAR_DIR/recovery/bootdisk in prep #3136

Merged
merged 2 commits into from Feb 6, 2024

Conversation

pcahyna
Copy link
Member

@pcahyna pcahyna commented Jan 25, 2024

Pull Request Details:
  • Type: Bug Fix

  • Impact: Normal

  • Reference to related issue (URL): see discussion in commit ccae513

  • How was this pull request tested?

    • touch /boot/grub2FAIL full backup and recovery. Original code fails to recover: "Installing GRUB Legacy boot loader:
      ERROR: Cannot install GRUB Legacy boot loader because there is no 'grub' program." New code works.
    • touch /boot/grub2FAIL && shopt -s nullglob && rear -d mkrescue && find /*/tmp/rear.*/rootfs /tmp/rear.*/rootfs -name grub\*-\* -executable
      original code: nothing
      new code:
/usr/tmp/rear.noYr1PET9AefVbz/rootfs/bin/grub2-mkimage
/usr/tmp/rear.noYr1PET9AefVbz/rootfs/bin/grub2-mkpasswd-pbkdf2
/usr/tmp/rear.noYr1PET9AefVbz/rootfs/bin/grub2-mkrelpath
/usr/tmp/rear.noYr1PET9AefVbz/rootfs/bin/grub2-bios-setup
/usr/tmp/rear.noYr1PET9AefVbz/rootfs/bin/grub2-install
/usr/tmp/rear.noYr1PET9AefVbz/rootfs/bin/grub2-mkconfig
/usr/tmp/rear.noYr1PET9AefVbz/rootfs/bin/grub2-probe
/usr/tmp/rear.noYr1PET9AefVbz/rootfs/bin/grub2-reboot
/usr/tmp/rear.noYr1PET9AefVbz/rootfs/bin/grub2-set-default
/var/tmp/rear.noYr1PET9AefVbz/rootfs/bin/grub2-mkimage
/var/tmp/rear.noYr1PET9AefVbz/rootfs/bin/grub2-mkpasswd-pbkdf2
/var/tmp/rear.noYr1PET9AefVbz/rootfs/bin/grub2-mkrelpath
/var/tmp/rear.noYr1PET9AefVbz/rootfs/bin/grub2-bios-setup
/var/tmp/rear.noYr1PET9AefVbz/rootfs/bin/grub2-install
/var/tmp/rear.noYr1PET9AefVbz/rootfs/bin/grub2-mkconfig
/var/tmp/rear.noYr1PET9AefVbz/rootfs/bin/grub2-probe
/var/tmp/rear.noYr1PET9AefVbz/rootfs/bin/grub2-reboot
/var/tmp/rear.noYr1PET9AefVbz/rootfs/bin/grub2-set-default
  • Description of the changes in this pull request:
    • Do not detect GRUB before including GRUB tools. When there was a file matching grub* in /boot (e.g. /boot/grub2FAIL), the code got confused by the glob pattern intended to match /boot/grub or /boot/grub2 and subsequently the rest of the script was skipped, as grubdir got assigned something like "/boot/grub2 /boot/grub2FAIL", which does not exist, so grubdir was set to /boot/grub, which does not exist either, and grub-probe fails.
      As a result, the GRUB tools were not included in the recovery image.
      The code have been proceeding anyway when neither grub-probe nor grub2-probe was found, so the tests have not been very useful.
      Fix and simplify by not checking for the existence of GRUB and just trying to include the GRUB tools always.
    • Don't create $VAR_DIR/recovery/bootdisk in prep. This file is unused and creating it in prep stage is against the guideline in prep/README:
You should not put scripts into this 'prep' stage that modify things
in ROOTFS_DIR or in VAR_DIR/recovery and VAR_DIR/layout because
scripts for ROOTFS_DIR belong to the 'rescue' stage and scripts
for VAR_DIR/recovery and VAR_DIR/layout belong to the 'layout' stages.

This file is unused and creating it in prep stage is against the
guideline in prep/README:

You should not put scripts into this 'prep' stage that modify things
in ROOTFS_DIR or in VAR_DIR/recovery and VAR_DIR/layout because
scripts for ROOTFS_DIR belong to the 'rescue' stage and scripts
for VAR_DIR/recovery and VAR_DIR/layout belong to the 'layout' stages.
When there was a file matching grub* in /boot (e.g. /boot/grub2FAIL),
the code got confused by the glob pattern intended to match /boot/grub
or /boot/grub2 and subsequently the rest of the script was skipped, as
grubdir got assigned something like "/boot/grub2 /boot/grub2FAIL", which
does not exist, so grubdir was set to /boot/grub, which does not exist
either, and grub-probe fails.

As a result, the GRUB tools were not included in the recovery image.

The code have been proceeding anyway when neither grub-probe nor
grub2-probe was found, so the tests have not been very useful.

Fix and simplify by not checking for the existence of GRUB and just
trying to include the GRUB tools always.

See the discussion in
rear@ccae513
@pcahyna pcahyna added bug The code does not do what it is meant to do cleanup labels Jan 25, 2024
@pcahyna pcahyna added this to the ReaR v2.8 milestone Jan 25, 2024
@pcahyna pcahyna requested a review from a team January 25, 2024 16:06
@pcahyna pcahyna self-assigned this Jan 25, 2024
pcahyna referenced this pull request Jan 25, 2024
…inux

Fedora 15 and others still work with these modifications
Copy link
Member

@schlomo schlomo left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot for cleaning up prep.

@pcahyna
Copy link
Member Author

pcahyna commented Jan 28, 2024

Note to myself:

the whole check in usr/share/rear/prep/Linux-s390/305_include_s390_tools.sh should now be deleted, it is useless and potentially harmful (any failure during checking /boot will result in all s390 tools omitted from the recovery image, with disastrous results).
Actually this whole file can then be deleted and the PROG+= lines moved to usr/share/rear/conf/Linux-s390x.conf or whatever is appropriate for s390 (this file got deleted in cba3590 without explanation).

@pcahyna
Copy link
Member Author

pcahyna commented Feb 6, 2024

@schlomo thanks for the review, merging!

@pcahyna pcahyna merged commit 096bfde into rear:master Feb 6, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The code does not do what it is meant to do cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants