Delete usr/share/rear/build/USB/default/800_enforce_usb_output.sh

[jsmeix]

• Type: Cleanup

• Impact: Unknown

• Reference to related issue (URL):
  OUTPUT_URL=null conflicts with OUTPUT=USB but ReaR does not error out appropriately #1571 (comment)
  OUTPUT_URL=null conflicts with OUTPUT=USB but ReaR does not error out appropriately #1571 (comment)

• How was this pull request tested?
  Impossible to test because it is unknown what that script should do.

• Description of the changes in this pull request:

Remove
build/USB/default/800_enforce_usb_output.sh
see
#1571 (comment)
and
#1571 (comment)

Furthermore this script reads

# Added by udev workflow

but it does its stuff in any case for USB
regardless of the workflow
so this script looks utterly broken.

According to

git log --follow -p usr/share/rear/build/USB/default/800_enforce_usb_output.sh

this script originated (as 80_enforce_usb_output.sh) in
4ec9ed4
which only tells

Make sure OUTPUT=USB is enforced in udev workflow during recover

but it neither explains WHY that is needed
nor does it implement what it tells
because the script is run in any case for OUTPUT=USB
regardless whether or not the udev workflow is used.

So as far as I understand it:
This script is run in any case for OUTPUT=USB
and then (i.e. when there is OUTPUT=USB in local.conf)
it "enforces" OUTPUT=USB in ROOTFS_DIR/.../local.conf
but how could OUTPUT in ROOTFS_DIR/.../local.conf
differ from OUTPUT in local.conf?

[pcahyna]

@jsmeix this is to be used by the udev workflow, triggered from etc/udev/rules.d/62-rear-usb.rules . Shouldn't the whole udev workflow be removed, if we can't test it?

[pcahyna]

the udev rules in rpm spec have been disabled in 24d57b8

[jsmeix]

FYI
regarding the udev workflow have a look at
(what I "just found" right now by searching
some of my older archived mails)
for example things like

#840
"udev workflow cannot work as it is currently implemented"

#838
"USB gets suspended"

#2180
"during boot 'rear udev' is called, causing delay during boot"

From my personal point of view
the udev workflow and all what belongs to it
should be removed the sooner the better, cf.
#2180 (comment)

[pcahyna]

RHEL has never been using the udev workflow (the udev rules have not been shipped in our RPM).

[jsmeix]

Also SUSE has never supported the udev workflow.

In particular there is no '62-rear-usb.rules' file shipped

• neither in a SUSE "rear..." RPM package for SLE-HA, cf.
  "... rear116 / rear1172a / rear118a / rear23a / rear27a"
  in https://en.opensuse.org/SDB:Disaster_Recovery
• nor in an "official" openSUSE "rear" RPM package
  (i.e. what osc search rear | grep '^openSUSE:' lists)

[jsmeix]

@rear/contributors in particular @schlomo @gdha
please also have a look here (as time permits).

If you do not object I would like to merge it
next Monday (18 Dec. 2023) afternoon.

[schlomo]

In general I share your feelings about this code, it looks horribly broken and doesn't cover a lot of what ReaR can do nowadays (e.g. multiple additional configs).

At first I was surprised why it is run whatsoever, but then I understood that we run $BACKUP and $OUTPUT and even $OUTPUT/$BACKUP files in every stage.

So looking at USB I find these scripts:

Removing one of them seems a bit risky, I'd rather prefer to add an Error here that calls back to this issue and makes people talk to us.

Yes, this is also a breaking change, but it comes with a clear call to action whereas just removing this file could be a breaking change that will leave users confused. If nobody ever comes back then we can remove it with a very clear conscience 😄

What do you think about this approach?

As a thought for the future I won't be surprised if the udev workflow will also benefit from the ability to specify multiple $OUTPUT methods. Or maybe we have to adjust the udev workflow to add a custom ReaR config that sets OUTPUT=USB.

[jsmeix]

@schlomo
thank you for having a look here!

I agree that it would be better to first inform the user.

But here I don't see how I could properly inform the user.
I mean if I "just called" Error() in that script
it would error out in any case because that script
runs unconditioned.

I also don't know under which conditions that script
should be run because I do not understand for what reason
it is there at all.
I mean that I do not yet understand what it is that made you
"understood that we run $BACKUP and $OUTPUT and
even $OUTPUT/$BACKUP files in every stage".

It is neither about that $BACKUP and $OUTPUT and
even $OUTPUT/$BACKUP scripts are run in every stage
nor about "multiple $OUTPUT methods".

It is about WHY build/USB/default/800_enforce_usb_output.sh
exists at all because - as I explained above - I don't see
any reason why it needs to do what it does because
I don't see how OUTPUT in ROOTFS_DIR/.../local.conf
could differ from OUTPUT in local.conf ?
I.e. as far as I see OUTPUT is same in both.

[schlomo]

@jsmeix I was referring to SourceStage():

rear/usr/share/rear/lib/framework-functions.sh

Lines 110 to 114 in e0609c7

	local scripts=( $( cd $SHARE_DIR/$stage
	ls -d {default,"$ARCH","$OS","$OS_MASTER_VENDOR","$OS_MASTER_VENDOR_ARCH","$OS_MASTER_VENDOR_VERSION","$OS_VENDOR","$OS_VENDOR_ARCH","$OS_VENDOR_VERSION"}/*.sh \
	"$BACKUP"/{default,"$ARCH","$OS","$OS_MASTER_VENDOR","$OS_MASTER_VENDOR_ARCH","$OS_MASTER_VENDOR_VERSION","$OS_VENDOR","$OS_VENDOR_ARCH","$OS_VENDOR_VERSION"}/*.sh \
	"$OUTPUT"/{default,"$ARCH","$OS","$OS_MASTER_VENDOR","$OS_MASTER_VENDOR_ARCH","$OS_MASTER_VENDOR_VERSION","$OS_VENDOR","$OS_VENDOR_ARCH","$OS_VENDOR_VERSION"}/*.sh \
	"$OUTPUT"/"$BACKUP"/{default,"$ARCH","$OS","$OS_MASTER_VENDOR","$OS_MASTER_VENDOR_ARCH","$OS_MASTER_VENDOR_VERSION","$OS_VENDOR","$OS_VENDOR_ARCH","$OS_VENDOR_VERSION"}/*.sh \

Fun fact, this script build/USB/default/800_enforce_usb_output.sh would be included for both BACKUP=USB and for OUTPUT=USB.

I did some more thinking about this issue and I would like to propose a different approach:

Insert an Error at line 8 or so of the script to error out with a deprecation warning and a link to this issue and a request to provide more context for the use case.

It would hit only if this script would do something.

I know, breaking stuff is bad but it seems to me that this is the only way how we can force feedback from users of this code.

---

If, OTOH, everybody else wants to just remove it, then please go ahead. Worst case, somebody will come and complain why we broke ReaR and why it took them 3 days to figure out why.

[jsmeix]

BACKUP=USB is not supported and
not listed in "man rear" or in default.conf
and for a test with BACKUP=USB I get

# usr/sbin/rear -D mkbackup
...
ERROR: The BACKUP method 'USB' is not supported (no /root/rear.github.master/usr/share/rear/restore/USB directory)
Some latest log messages since the last called script 035_valid_backup_methods.sh:

so in practice build/USB/default/800_enforce_usb_output.sh
will not be run for BACKUP=USB
so it should be only run for OUTPUT=USB

[jsmeix]

This one is superseded by
#3110

[pcahyna]

@jsmeix regarding the udev output, I found this: https://github.com/rear/rear/blob/master/doc/user-guide/04-scenarios.adoc#using-relax-and-recover-with-usb-storage-devices

We have implemented a specific workflow: inserting a REAR-000 labeled USB stick will invoke rear udev and adds a rescue environment to the USB stick (updating the bootloader if needed)

but no further explanation. I suppose this whole point should be deleted.

[jsmeix]

Via
eadcc68
in doc/user-guide/04-scenarios.adoc
I removed parts that mention 'udev'
from the section about USB devices
and by the way
I also removed some other parts there
which are outdated, could be misleading,
or are questionable or discursive
to make the text simpler and more to the point.