#2353 PR `merged`: Fix #2351, #2352: relax checking for a valid BACKUP method¶

Labels: enhancement, bug, cleanup, fixed / solved / done

OliverO2 opened issue at 2020-03-30 21:49:¶

Pull Request Details:¶

Type: Bug Fix
Impact: Normal
Reference to related issue (URL): #2351, #2352
How was this pull request tested? On Ubuntu 18.04.4 LTS
Brief description of the changes in this pull request: See issues.

jsmeix commented at 2020-03-31 08:17:¶

That will not work.

Reason:

# SHARE_DIR=usr/share/rear

# grep 'BACKUP=' $SHARE_DIR/conf/default.conf | grep -v '_' | cut -d= -f2 | awk '{print $1}' | sort -u

AVA
BACULA
BAREOS
BLOCKCLONE
BORG
CDM
DP
DUPLICITY
EXTERNAL
FDRUPSTREAM
GALAXY
GALAXY10
GALAXY7
NBKDC
NBU
NETFS
NSR
RBME
REQUESTRESTORE
RSYNC
SESAM
TSM
YUM
ZYPPER

# ls -1 $SHARE_DIR/backup/
BLOCKCLONE
BORG
DUPLICITY
EXTERNAL
NETFS
OBDR
RSYNC
TSM
YUM
ZYPPER
default
readme

so it seems the new test does not find many BACKUP methods
that are mentioned in default.conf

The reason is indirectly described in my comments in my code before.
Please do not just remove all those comments but read and understand them
so that you can adapt them so that they match the new code and still
explain to other users what all that stuff is about, cf.
"Code must be easy to understand" in
https://github.com/rear/rear/wiki/Coding-Style

The reason is that most external BACKUP methods
do not implement making a backup (see "man rear")
but only implement to restore a backup, so

# ls -1 $SHARE_DIR/restore/

AVA
BACULA
BAREOS
BLOCKCLONE
BORG
CDM
DP
DUPLICITY
EXTERNAL
FDRUPSTREAM
Fedora
GALAXY
GALAXY10
GALAXY7
NBKDC
NBU
NETFS
NSR
OBDR
RBME
REQUESTRESTORE
RSYNC
SESAM
SUSE_LINUX
TSM
YUM
ZYPPER
default
readme

is a better test that still finds all what the test before also found

# for b in $( grep 'BACKUP=' $SHARE_DIR/conf/default.conf | grep -v '_' | cut -d= -f2 | awk '{print $1}' | sort -u ) ; do ls -1d $SHARE_DIR/restore/$b ; done

usr/share/rear/restore/AVA
usr/share/rear/restore/BACULA
usr/share/rear/restore/BAREOS
usr/share/rear/restore/BLOCKCLONE
usr/share/rear/restore/BORG
usr/share/rear/restore/CDM
usr/share/rear/restore/DP
usr/share/rear/restore/DUPLICITY
usr/share/rear/restore/EXTERNAL
usr/share/rear/restore/FDRUPSTREAM
usr/share/rear/restore/GALAXY
usr/share/rear/restore/GALAXY10
usr/share/rear/restore/GALAXY7
usr/share/rear/restore/NBKDC
usr/share/rear/restore/NBU
usr/share/rear/restore/NETFS
usr/share/rear/restore/NSR
usr/share/rear/restore/RBME
usr/share/rear/restore/REQUESTRESTORE
usr/share/rear/restore/RSYNC
usr/share/rear/restore/SESAM
usr/share/rear/restore/TSM
usr/share/rear/restore/YUM
usr/share/rear/restore/ZYPPER

jsmeix commented at 2020-03-31 08:19:¶

@OliverO2
thank you so much for your continuous testing
of the current ReaR master code!
It helps so much to reveal hidden regressions.

jsmeix commented at 2020-03-31 11:35:¶

With
https://github.com/rear/rear/commit/e98c8adcd75d043672bbed9564bedef377d9639e
I simplified prep/default/035_valid_backup_methods.sh
to make it work more predictable and fail-safe according to
https://github.com/rear/rear/pull/2353#issuecomment-606473207

@OliverO2
thank you for pointing me in the right direction how to improve
my vague "grep in default.conf" stuff into a more reliable test.

jsmeix commented at 2020-03-31 11:38:¶

@OliverO2
I think all you need to do here is to move your
usr/share/rear/backup/OPALPBA/readme
to
usr/share/rear/restore/OPALPBA/readme
and remove your changed prep/default/035_valid_backup_methods.sh
from this pull request.

jsmeix commented at 2020-03-31 11:45:¶

@OliverO2
according to
https://github.com/rear/rear/issues/2352
The BACKUP method 'INTERNAL' is not known to ReaR
I wonder if you may also need a
usr/share/rear/restore/INTERNAL/readme
file?

But I cannot find a script where BACKUP=INTERNAL is set.
I even cannot find the word INTERNAL in the scripts at all
so currently I do not understand how the BACKUP variable
could get the value 'INTERNAL'?

OliverO2 commented at 2020-03-31 12:06:¶

@jsmeix
Thank you for reviewing, good catch!

Actually, I had read the comments. I removed them as i found them to no longer correctly describe the actual situation:

For "rear mkbackup" and also for "rear mkrescue" [...]

As we know now, 035_valid_backup_methods.sh is used in workflows other than mkbackup and mkrescue. Even more workflows might be added to ReaR in the future (without being aware of the above comment). So I did not want to just extend the above comment, as doing so would not be future-proof.

Your final solution is just what I was now considering as a fix. Clean and simple. You were just too fast for my lunch break, and I was too hungry ;-).

Yes, there is no INTERNAL backup method in ReaR as it's just what I'm using over here. Actually, I have these directories, which integrate nicely with ReaR updates as there is no common code:

backup/INTERNAL
build/INTERNAL
prep/INTERNAL
restore/INTERNAL

Configuration is done entirely via one line in site.conf:

BACKUP=INTERNAL

And yes, I'll adapt the PR as suggested right away - hopefully, I won't get too hungry, too soon again.

jsmeix commented at 2020-03-31 12:57:¶

First and foremost enjoy your meals - each and every one!

OliverO2 commented at 2020-03-31 13:14:¶

With the latest commits to this branch, the fix should be complete. I have successfully tested the mkopalpba workflow.

jsmeix commented at 2020-03-31 14:00:¶

prep/default/035_valid_backup_methods.sh
is run for those workflows that source the prepstage:

# for w in usr/share/rear/lib/*workflow.sh ; do grep -q 'SourceStage "prep"' $w && echo $( basename $w ) ; done

mkbackup-workflow.sh
mkbackuponly-workflow.sh
mkopalpba-workflow.sh
mkrescue-workflow.sh

so mkopalpba is the only workflow that does not belong to
the usual "mkbackup/mkrescue" workflows.

I think I even remember when I did
https://github.com/rear/rear/commit/8eb72c1d845d2b91fa76f18d47c74454ed5432ec#diff-0ee6186ad0db8266a03846e3642551e5
that I had checked the mkopalpba workflow but at that time
I did not see how that workflow will become affected.

jsmeix commented at 2020-03-31 14:16:¶

@OliverO2
thank you for the explanation about your BACKUP=INTERNAL usage.

I did not get an example from a ReaR user that he actually adapted and
enhanced ReaR in this way - which is exactly how ReaR is meant to be
adapted and enhanced as needed by a particular user.

To be more on the safe side I would suggest to use a less generic
name than INTERNAL because who knows if or when someone
at ReaR upstream may all of a sudden use BACKUP=INTERNAL
for whatever other generic ReaR-internal reason?

[Export of Github issue for rear/rear.]

#2353 PR merged: Fix #2351, #2352: relax checking for a valid BACKUP method¶