#2834 PR `merged`: Add yes to REQUIRED_PROGS¶

Labels: bug, fixed / solved / done

pcahyna opened issue at 2022-07-01 20:32:¶

Pull Request Details:¶

Type: Bug Fix
Impact: High
Reference to related issue (URL):
#2827
How was this pull request tested?
Created LVM on an empty disk (mpatha) and mounted it

parted --script -a optimal /dev/mapper/mpatha -- mklabel gpt mkpart extra ext2 1M -1M set 1 lvm on
pvcreate --yes /dev/mapper/mpatha1
vgcreate sigvg  /dev/mapper/mpatha1
lvcreate -y -n  sig_lv  sigvg -L 1G
mkfs.xfs /dev/sigvg/sig_lv
mkdir /home/foo
echo "/dev/sigvg/sig_lv                             /home/foo                   xfs     defaults        0 0" >> /etc/fstab
mount /home/foo

ran rear mkbackup
and then filled the disk with signatures

EXTRADISK=/dev/mapper/mpatha1
EXTRAFS=( $(lsblk -ln -o mountpoint $EXTRADISK) )
for f in "${EXTRAFS[@]}"; do
    umount $f
done
EXTRADEVS=( $(lsblk -lnp -o name $EXTRADISK | tac) )
for d in "${EXTRADEVS[@]}"; do
    EXTRATYPE=$(lsblk -ln -o type "$d")
    if [ lvm == "$EXTRATYPE" ]; then
        wipefs -a $d
        lvremove -y $d
    fi
done
lvcreate -y sigvg -n xfsloglv -l 100%FREE
LOOPFILE=loopbackfile.img
dd if=/dev/zero of=$LOOPFILE bs=100M count=10
LOOPDEV=$(losetup -f)
losetup -f $LOOPFILE
MKFSOUT=$(mkfs.xfs -l logdev=/dev/sigvg/xfsloglv,size=2048b "$LOOPDEV")
losetup -d $LOOPDEV
BSIZE=$(echo $MKFSOUT | sed "s/.*\/dev\/sigvg\/xfsloglv bsize=\([^ ]*\)[ ]*.*/\1/")
dd if=/dev/sigvg/xfsloglv of=/dev/sigvg/xfsloglv bs=$BSIZE seek=1
lvremove -y /dev/sigvg/xfsloglv
vgremove -y /dev/sigvg
pvremove -y $EXTRADISK

and restored from the backup in migration mode

Brief description of the changes in this pull request:
Since PR #2827 we have been piping the output of yes to lvcreate, but yes has not been added to the rescue system. Fix that.
(We could have added it to REQUIRED_PROGS only if lvm is present, but let's not complicate it and add it always.)

pcahyna commented at 2022-07-01 20:36:¶

https://github.com/rear/rear/pull/2827#issuecomment-1172507654

jsmeix commented at 2022-07-04 08:11:¶

It is perfectly right to have 'yes' in any case in the recovery system
because it should always be possible for the user to do things like

yes | COMMAND

manually in the recovery system
or manually edit commands in diskrestore.sh this way.

Cf. the section "It should be possible to run ReaR unattended" in
https://github.com/rear/rear/wiki/Coding-Style
but I didn't do proper tests how far it is actually possible
to run ReaR unattended with our current code.

pcahyna commented at 2022-07-04 09:39:¶

It is perfectly right to have 'yes' in any case in the recovery system because it should always be possible for the user to do things like
yes | COMMAND
manually in the recovery system or manually edit diskrestore.sh this way.

I agree. The only question is whether it should be in PROGS or REQUIRED_PROGS in case there is nothing to require it, but hopefully systems without yes will be hard to encounter and thus the difference would not matter in practice.

but I didn't do proper tests how far it is actually possible
to run ReaR unattended with our current code.

It is definitely possible. Virtually all my restore tests are run this way.

jsmeix commented at 2022-07-04 10:28:¶

REQUIRED_PROGS is the exact right one according to
"Try hard to care about possible errors" in
https://github.com/rear/rear/wiki/Coding-Style
(excerpt)

In case of errors better abort than to blindly proceed.

I fear we have many programs in PROGS
where we do not have appropriate checks in our code
that error out when a program is actually needed but not there.

[Export of Github issue for rear/rear.]

#2834 PR merged: Add yes to REQUIRED_PROGS¶