#3102 PR `merged`: OUTPUT=USB: add a check that OUTPUT_URL is mounted¶

Labels: enhancement, fixed / solved / done

pcahyna opened issue at 2023-12-07 15:06:¶

Pull Request Details:¶

Type: Enhancement
Impact: Low
Reference to related issue (URL): closes #3098
How was this pull request tested?
- full backup and recovery with OUTPUT=USB on RHEL 8
- rear mkrescue with

OUTPUT=USB
BACKUP=NETFS
BACKUP_URL=usb:///dev/disk/by-label/REAR-000
OUTPUT_URL=file:///srv

fails with

file:// OUTPUT_URL is currently unsupported for OUTPUT=USB, use usb://
ERROR: OUTPUT_URL 'file:///srv' is not mounted at /var/tmp/rear.9WxhsNfKKQjmgsP/outputfs
Some latest log messages since the last called script 105_check_mounted_usb.sh:
  2023-12-08 13:31:11.869249473 file:// OUTPUT_URL is currently unsupported for OUTPUT=USB, use usb://
Some messages from /var/tmp/rear.9WxhsNfKKQjmgsP/tmp/rear.mkrescue.stdout_stderr since the last called script 105_check_mounted_usb.sh:
  mountpoint: /var/tmp/rear.9WxhsNfKKQjmgsP/outputfs: No such file or directory

- rear mkrescue with

OUTPUT=USB
BACKUP=NETFS
BACKUP_URL=usb:///dev/disk/by-label/REAR-000
OUTPUT_URL=nfs://example.com/mnt/qa
OUTPUT_OPTIONS=ro,noatime

fails with

OUTPUT=USB needs OUTPUT_URL to refer to a block device.
Use the usb:// scheme for OUTPUT_URL
ERROR: OUTPUT_URL 'nfs://example.com/mnt/qa' refers to mounted 'example.com:/mnt/qa' which is not a block device
Some latest log messages since the last called script 105_check_mounted_usb.sh:
  2023-12-10 10:54:53.662592565 OUTPUT=USB needs OUTPUT_URL to refer to a block device.
  2023-12-10 10:54:53.665507523 Use the usb:// scheme for OUTPUT_URL
Some messages from /var/tmp/rear.LtDKhi18fRteQRF/tmp/rear.mkrescue.stdout_stderr since the last called script 105_check_mounted_usb.sh:
  /var/tmp/rear.LtDKhi18fRteQRF/outputfs is a mountpoint

Description of the changes in this pull request:

If OUTPUT_URL uses the file:// scheme, ReaR aborts with a weird error message "BUG: Filesystem where the booting related files are on ... could not be found" in output/USB/Linux-i386/850_make_USB_bootable.sh.

Check if OUTPUT_URL got actually mounted at the right place ($BUILD_DIR/outputfs) by output/default/100_mount_output_path.sh and abort earlier with a meaningful error message if not.

jsmeix commented at 2023-12-08 08:20:¶

I made the same kind of non-fail-safe code in
output/USB/Linux-i386/850_make_USB_bootable.sh

usb_filesystem=$( grep " $BUILD_DIR/outputfs " /proc/mounts | cut -d' ' -f3 | tail -1 )
case "$usb_filesystem" in
...
    ("")
        BugError "Filesystem where the booting related files are on $RAW_USB_DEVICE could not be found"
        ;;

because it reports a bug in ReaR when

grep " $BUILD_DIR/outputfs " /proc/mounts | cut -d' ' -f3 | tail -1

fails for all arbitrary reasons, for example as in
https://github.com/rear/rear/issues/3098

jsmeix commented at 2023-12-08 08:26:¶

@pcahyna
perhaps it is more clear to enhance the checks in
output/USB/Linux-i386/850_make_USB_bootable.sh
directly where all that is done instead of implementing
things that belong together in various separated scripts?

jsmeix commented at 2023-12-08 09:00:¶

@pcahyna
regarding your
https://github.com/rear/rear/issues/3098#issuecomment-1845519067
I could do it if it helps you when I would do it.
I would also only implement a single specific check
that helps with https://github.com/rear/rear/issues/3098

I think I would only implement one check that
there is something mounted at $BUILD_DIR/outputfs
and if nothing is mounted there, then error out.
Only optionally a check that what is mounted there
is a block device and if not show a LogPrintError.

I think I would implement that directly in
output/USB/Linux-i386/850_make_USB_bootable.sh
and replace that BugError() by Error() therein.

pcahyna commented at 2023-12-08 09:52:¶

I think I would only implement one check that there is something mounted at $BUILD_DIR/outputfs and if nothing is mounted there, then error out. Only optionally a check that what is mounted there is a block device and if not show a LogPrintError.

For OUTPUT=USB, having a block device is a pretty basic requirement. For this reason, I prefer to have the check for block device mandatory and let it error out when it fails.

I think I would implement that directly in output/USB/Linux-i386/850_make_USB_bootable.sh and replace that BugError() by Error() therein.

Since this is a basic requirement of OUTPUT=USB, I prefer to have the check as early as possible. I could even rename 110_check_mounted_usb.sh to 101_check_mounted_usb.sh to be executed ASAP after 100_mount_output_path.sh. I was lazy to audit all the scripts between 100_mount_output_path.sh and 850_make_USB_bootable.sh to check if they also might misbehave when the output is not mounted (or not a block device) and even if they don't, we could add some code that uses the mountpoint or its underlying device later.

jsmeix commented at 2023-12-08 11:42:¶

This are all scripts in usr/share/rear/output/
(excerpt)

# find usr/share/rear/output/ | grep -o '[0-9][0-9][0-9]_.*' | sort -n

010_set_umask.sh
100_create_efiboot.sh
100_mount_output_path.sh
150_save_copy_of_prefix_dir.sh
200_make_boot_dir.sh
...

I would recommend to leave at least one free number
between already existing scripts (better two or three free numbers)
to allow easy addition of more new scripts in between existing scripts
(in particular with at least one free number users can add own scripts)
so I don't like 101_check_mounted_usb.sh and
I think 110_check_mounted_usb.sh is fine here
or perhaps 105_check_mounted_usb.sh if you like this more.

jsmeix commented at 2023-12-08 12:03:¶

On what older Linux systems

findmnt -funo SOURCE --target $BUILD_DIR/outputfs

works as expected:

I test with

# findmnt -funo SOURCE --target /

SLES10-SP4:

# findmnt -funo SOURCE --target /
-bash: findmnt: command not found

SLES11-SP3:

# findmnt -funo SOURCE --target /
-bash: findmnt: command not found

SLES12-SP5:

# findmnt -funo SOURCE --target /
/dev/sda2

SLES15-SP3:

# findmnt -funo SOURCE --target /
/dev/mapper/system-root[/@/.snapshots/1/snapshot]

So it seems (at least for the above tested systems)
when 'findmnt' is there then findmnt -funo SOURCE --target ...
works as expected.

Note the test on SLES15-SP3
where findmnt -funo SOURCE --target ...
does not output a plain kernel device node
but a kernel device node with additional stuff,
so e.g.:

# test -b /dev/mapper/system-root[/@/.snapshots/1/snapshot] && echo y || echo n
n

# test -b /dev/mapper/system-root && echo y || echo n
y

Of course a mounted filesystem on USB device for ReaR
has likely not some (over)complicated btrfs structure
(like what is mounted at '/' on SLES).

pcahyna commented at 2023-12-08 12:09:¶

# findmnt -funo SOURCE --target /
/dev/mapper/system-root[/@/.snapshots/1/snapshot]

This would be problematic - we would have to revisit this if we want to support formatting the USB as btrfs.

jsmeix commented at 2023-12-08 12:14:¶

On SLES15-SP3:

# man findmnt
...
  -v, --nofsroot
    Do not print a [/dir] in the SOURCE column
    for bind mounts or btrfs subvolumes.

so

# findmnt -funvo SOURCE --target /
/dev/mapper/system-root

does what we need
and now I have to check on what older Linux systems
findmnt supports '-v, --nofsroot' ...

pcahyna commented at 2023-12-08 12:20:¶

RHEL 6 supports it:

[root@pcahyna-1mt-rhel-6 ~]# findmnt -funvo SOURCE --target /mnt/redhat
...redhat.com:/devops_engineering_nfs/devarchive/redhat
[root@pcahyna-1mt-rhel-6 ~]# findmnt -funo SOURCE --target /mnt/redhat
...redhat.com:/devops_engineering_nfs/devarchive/redhat[/redhat]

for some reason, it is useful even with NFS.

jsmeix commented at 2023-12-08 12:20:¶

On what older Linux systems
findmnt supports '-v, --nofsroot':

SLES12-SP5:

# findmnt -funvo SOURCE --target /
/dev/sda2

SLES15-SP3

# findmnt -funvo SOURCE --target /
/dev/mapper/system-root

So it seems (at least for the above tested systems)
when 'findmnt' is there then findmnt -funvo SOURCE --target ...
works as expected.

pcahyna commented at 2023-12-08 12:23:¶

I can add -v then, but it could be better to leave it out? I suspect that formatting the USB as btrfs with subvolumes could lead to various funny problems later, I doubt the USB code is adapted to it.

jsmeix commented at 2023-12-08 12:23:¶

@pcahyna
but in your output
https://github.com/rear/rear/pull/3102#issuecomment-1847079040
it seems there is a

...redhat.com:

prefix so that prefix could also get in the way?

pcahyna commented at 2023-12-08 12:24:¶

@pcahyna but in your output #3102 (comment) it seems there is a
...redhat.com:
prefix so that prefix could also get in the way?

... which is what I want. I want the check to fail with NFS mounts.

jsmeix commented at 2023-12-08 12:25:¶

@pcahyna
feel free to add 'v' or leave it out in this case
as you imagine what (hopefully) will work better
in practice out there at our users.

jsmeix commented at 2023-12-08 12:38:¶

I was wondering whether or not bash 3 versus bash 4
could be also used to distinguish reasonably well
if findmnt is there or not.

So I tested SLES11-SP4 (above I had tested SLES11-SP3).

SLES11-SP4:

# findmnt -funvo SOURCE --target /
/dev/sda2

SLES11 and older have bash 3.
SLES12 and newer have bash 4.
So at least for SLE bash 3 does not mean there is no findmnt.
But at least for SLE bash 4 does mean there is findmnt.

So if we would make bash 4 mandatory for next ReaR version 2.8
we could more safely assume certain other programs are there.

pcahyna commented at 2023-12-08 12:55:¶

feel free to add 'v' or leave it out in this case as you imagine what (hopefully) will work better in practice out there at our users.

@jsmeix I will leave it out, but if you want to test with btrfs-formatted USB, and you find that the check is the only thing that prevents this case from working, I will add it.

jsmeix commented at 2023-12-08 14:29:¶

Now it may error out with only those texts:

'findmnt -funo SOURCE --target $BUILD_DIR/outputfs' returned an empty string

or

'findmnt -funo SOURCE --target $BUILD_DIR/outputfs' failed with exit code ...

But this could be too little context for the user
so he may not understand where to look for a cause.

Therefore I would like to suggest code like

if usbdev="$(findmnt -funo SOURCE --target $BUILD_DIR/outputfs)" ; then
    if [ -z "$usbdev" ] ; then
        LogPrintError "'findmnt -funo SOURCE --target $BUILD_DIR/outputfs' returned an empty string"
        Error "Could not check that OUTPUT_URL '$OUTPUT_URL' refers to a mounted block device"
    fi
    # It needs to be a disk-based filesystem (not e.g. NFS)
    # as OUTPUT=USB basically means "disk" (not necessarily USB).
    if ! [ -b "$usbdev" ] ; then
        Error "OUTPUT_URL '$OUTPUT_URL' refers to mounted '$usbdev' which is not a block device"
    fi
else
    # needs to be in the "else" branch. "! usbdev=$(findmnt ...)" would clobber
    # the exit status of "findmnt" and $? would become useless.
    LogPrintError "'findmnt -funo SOURCE --target $BUILD_DIR/outputfs' failed with exit code $?"
    Error "Failed to check that OUTPUT_URL '$OUTPUT_URL' refers to a mounted block device"
fi

If you like you may downgrade those LogPrintError to DebugPrint.

By the way:
I prefer when all Error() messages are different
to be able to find the right single place in our code
where a particular Error() exit happened
(e.g. when a user only reported an Error() message without context).

pcahyna commented at 2023-12-08 18:01:¶

@jsmeix done, thanks for the suggestion.

While testing, I discovered a bigger issue: file:// does not work with USB even if the user had mounted the filesystem themselves. There are many places in the USB code that expect the output to be mounted at $BUILD_DIR/outputfs. It would be a much bigger project to fix this, so I merely adjusted the error messages and documented it.

jsmeix commented at 2023-12-11 07:57:¶

@pcahyna
thank you so much for your testing!

Better merge it sooner than later now
before you find more and more issues
lurking in our labyrinthine USB code ;-)

FYI:
Now I remember that we have
prep/default/040_check_backup_and_output_scheme.sh
to
"Check conditions that depend on BACKUP_URL or OUTPUT_URL
in particular conditions that depend on their schemes."
and in the code there I found

# OUTPUT_URL=null conflicts with OUTPUT=USB
# because for OUTPUT=USB output/USB/Linux-i386/850_make_USB_bootable.sh
# wants to make the USB device bootable which cannot work with OUTPUT_URL=null
# see https://github.com/rear/rear/issues/1571#issuecomment-343467593

and
https://github.com/rear/rear/issues/1571
was basically the same kind of issue as now
https://github.com/rear/rear/issues/3098
i.e. false BUG Error in
output/USB/Linux-i386/850_make_USB_bootable.sh
because of unsupported OUTPUT_URL for OUTPUT=USB

pcahyna commented at 2023-12-11 14:52:¶

@jsmeix

regarding your
https://github.com/rear/rear/issues/3098#issuecomment-1845519067
I could do it if it helps you when I would do it.

thanks for the offer, cleanup of this area is certainly welcome. Not sure if it has the highest priority given we have not had many bug reports about that, though (and I don't have any immediate use for it ATM).

Some remarks on what would IMO need to be cleaned up:

confusion between setting USB_DEVICE from OUTPUT_URL vs. BACKUP_URL. The current code tries to determine USB_DEVICE from OUTPUT_URL, but if OUTPUT_URL is not usb://, it determines it from BACKUP_URL instead. THat's why I was able to reproduce the problem with

BACKUP_URL=usb:///dev/disk/by-label/REAR-000
OUTPUT_URL=file:///srv

This should be unified and USB_DEVICE determined always from OUTPUT_URL since it represents the location of boot files, nto of the backup.

if one wants a separate boot partition on the USB, it is a bit complicated, one has to set several things manually: https://github.com/rear/rear/pull/2660#issuecomment-885643937 in particular OUTPUT_URL different from BACKUP_URL. It would be good to be able to do it with a single setting. IMO, this usage can conflict with OUTPUT_URL being different from BACKUP_URL in case one wants to use two USB devices: one for booting and another for backup (which should be a legitimate use).
maybe all of BACKUP_URL, OUTPUT_URL, USB_DEVICE should be set based on OUTPUT=USB and the setting of USB_DEVICE_BOOT_LABEL and USB_DEVICE_FILESYSTEM_LABEL, setting them manually by user only risks inconsistencies (but BACKUP_URL can be something else than usb:// ).

pcahyna commented at 2023-12-11 14:53:¶

thank you @jsmeix for the review, I am going to merge it.

[Export of Github issue for rear/rear.]

#3102 PR merged: OUTPUT=USB: add a check that OUTPUT_URL is mounted¶