#2735 PR closed: Evaluation enhancement of PRE_RECOVERY/POST_RECOVERY/PRE_BACKUP/POST_BACKUP scripts¶

Labels: enhancement, won't fix / can't fix / obsolete

rmetrich opened issue at 2021-12-28 09:21:¶

Relax-and-Recover (ReaR) Pull Request Template¶

Please fill in the following items before submitting a new pull request:

Pull Request Details:¶

• Type: Enhancement

• Impact: Normal

• Reference to related issue (URL): N/A

• How was this pull request tested? Manually

• Brief description of the changes in this pull request:

The current evaluation of the scripts is through eval ${SCRIPT[@]} which has many drawbacks, in particular when using array variables, each line must be semi-colon terminated or else the rest will be skipped, e.g.

PRE_RECOVERY_SCRIPT=( 'multipathd' )
PRE_RECOVERY_SCRIPT+=( 'sleep 10' )

resulted in executing multipathd sleep 10 instead of expected multipathd then sleep 10.

This enhancement evaluates the scripts item by item, which is more bulletproof and still supports having the script specified as a string variable.

pcahyna commented at 2022-01-07 22:24:¶

Hi @rmetrich, your change looks logical, but is contrary to the documented intent. The comment in default.conf (and comments there really serve as user documentation, because only very basic variables are documented in the user docs and most of variables are documented only in default.conf) says: NOTE: The scripts can be defined as an array to better handly spaces in parameters. So the intent of having them as arrays is not to support multiple independent scripts to be evaluated separately, but to have only one script that is better structured (command and arguments), like (perverse example) (echo 'rm -rf /') that is intended to print rm -rf / but after your change prints nothing and executes rm -rf /. That's probably why the variables are called PRE/POST_XXX_SCRIPT and not _SCRIPTS (note the singular). After your change, they would be better called PRE/POST_XXX_SCRIPTS.
Note that I do not endorse the original intent. To me it seems misguided: if one wants to provide commands as shell arrays in order to protect arguments from shell processing like whitespace splitting, fine, but then one should not feed them to eval. The problem is, as the man page of the Almquist shell succinctly states, "eval string ... Concatenate all the arguments with spaces. Then re-parse and execute the command.", specifically, the "re-parse" part. If one tries to provide a list of arguments with embedded spaces to eval as a shell array, like CMD=(ls 'a filename with spaces'); eval "${CMD[@]}", then the "${CMD[@]}" expansion will work properly, but eval will then re-split the resulting arguments on whitespace, turning them into "ls" "a" "filename" "with" "spaces" instead of the intended "ls" "a filename with spaces". Sure, one can quote the string inside, like CMD=(ls '"a filename with spaces"'), but then one does not need the bash array at all, because CMD='ls "a filename with spaces"' is exactly equivalent and certainly simpler.

One has IMO basically two options how to use bash arrays to specify commands:

1. use them to provide multiple inline scripts, each of the scripts will be evaluated separately and can use various shell metacharacters, like quotes. If one wants to provide arguments with spaces to some commands in the scripts, one has to use quotes inside the scripts, like in any other shell command (and another level of quotes around the whole script).
2. use them to provide a command with arguments. The arguments can contain whitespace and do not need to contain quotes (they will need quoting only when constructing the array). But then one can not use eval, thus one is prevented from using various special shell constructs.

This PR changes the interpretation of the variables to 1. while the original intent seems to have been 2. (but due to the use of eval it does not work as probably intended).

What about introducing new variables called PRE/POST_xxx_SCRIPTS and letting the old variables as they are (and remove them in the next major version)?

pcahyna commented at 2022-01-07 22:32:¶

Of course, the scripts that one provide in the arrays are not really independent because they run in the same shell, not in a subshell. So they could be considered only multiple commands of one script (one can pass variables from one to a subsequent one, for example). Still, I think that calling the variables _SCRIPTS instead of _SCRIPT expresses better the intent and the difference from the former behavior. Or maybe _COMMADS would be even better?

rmetrich commented at 2022-01-08 08:00:¶

Right, I completely missed this. Having new variables seems indeed better.

jsmeix commented at 2022-01-10 08:19:¶

Regarding ..._COMMANDS:
We have currently in default.conf
NETWORKING_PREPARATION_COMMANDS
ZYPPER_NETWORK_SETUP_COMMANDS
YUM_NETWORK_SETUP_COMMANDS

The variable name ..._SCRIPT could be misleading because it looks
as if it means the filename of a script and not the commands of a script.
A variable name ..._SCRIPTS is misleading because it certainly
means filenames of scripts and not commands of a single script.

pcahyna commented at 2022-01-10 10:38:¶

thanks @jsmeix for looking for prior art, *_COMMANDS seems to be the best choice indeed.

pcahyna commented at 2022-01-10 11:19:¶

@rmetrich by the way I am curious about the original motivation: doesn't ReaR start multipathd automatically when it detects multipath devices? I recall testing ReaR with multipath and it seemed to work.

jsmeix commented at 2022-01-10 11:41:¶

multipathd is started during "rear recover" via
usr/share/rear/layout/prepare/GNU/Linux/210_load_multipath.sh

rmetrich commented at 2022-01-10 11:56:¶

@rmetrich by the way I am curious about the original motivation: doesn't ReaR start multipathd automatically when it detects multipath devices? I recall testing ReaR with multipath and it seemed to work.

Yes it does. This is just an example where I had to implement this on Red Hat's 2.4 rear which was not always starting multipathd.
I'm changing this seems it may be confusing in the end and the admin may believe multipathd needs to be started manually.

jsmeix commented at 2022-01-12 12:53:¶

Above I made some comments that start with

jsmeix started a review        Pending    View Changes

where I don't get a GitHub mail for those comments
and when I click on 'View Changes' there I get
We went looking everywhere, but couldn’t find those commits

So it seems something is somehow messed up there
but one can at least see my comments here
(strictly speaking at least I can see my comments here)
so please have a look at my above comments
that are currently marked with Pending.

If you don't see Pending comments from me above
I would re-post them here.

pcahyna commented at 2022-01-12 13:12:¶

@jsmeix I think that "Pending" means that only you can see them until you submit them. I don't see any comments from you except https://github.com/rear/rear/pull/2735#discussion_r781056780 https://github.com/rear/rear/pull/2735#issuecomment-1008793731 and https://github.com/rear/rear/pull/2735#issuecomment-1008631221

jsmeix commented at 2022-01-12 13:50:¶

@pcahyna
thank you for the description what you see.
I find nothing at my 'Pending' comments where I could submit them.
The only active button is 'View Changes' but that results only
We went looking everywhere, but couldn’t find those commits

I will re-post a summary here.

jsmeix commented at 2022-01-12 13:51:¶

Regarding POST_BACKUP_SCRIPT as exit task I had written:

I guess the intent behind is that with a simple

PRE_BACKUP_SCRIPT=( 'database stop' )
POST_BACKUP_SCRIPT=( 'database start' )

after a "rear mkbackup" error during backup
database start is run as exit task.

I wonder if it is the by users usually expected behaviour
when they had a "rear mkbackup" error during backup.
Why automatically restarting the database when the backup had failed?
Perhaps an up-to-date backup is intended before restarting the database?

Also the implementation runs all commands in POST_BACKUP_SCRIPT
as exit task after a "rear mkbackup" error during backup
and I think it is likely not right to "just run" all those commands.

From my current point of view I would not implement that
oversophisticated looking automated exit task stuff
also for the new ..._COMMANDS arrays
to keep things more safe and simple.

jsmeix commented at 2022-01-12 13:55:¶

Regarding the code in backup/default/010_pre_backup_script.sh

for command in "${POST_BACKUP_COMMANDS[@]}"; do
    AddExitTask "$command"
done

I had written:

I still do not like it when all POST_BACKUP_COMMANDS
get hardcoded added as exit tasks.

...

Because I like to provide "final power to the user"
I think it must be possible for the user to specify
which one of his POST_BACKUP_COMMANDS
he also wants to run in case of error exit during backup.

My immediate idea is that
when one element in POST_BACKUP_COMMANDS
has the special value add_next_as_exit_task
the next element in POST_BACKUP_COMMANDS
gets added as exit task.

So I would like to propose (totally untested) code like

add_as_exit_task="no"
for command in "${POST_BACKUP_COMMANDS[@]}" ; do
    if test "$command" = "add_next_as_exit_task" ; then
        add_as_exit_task="yes"
        continue
    fi
    is_true $add_as_exit_task || continue
    AddExitTask "$command"
    add_as_exit_task="no"
done

and matching code in backup/default/990_post_backup_script.sh
like

exists_as_exit_task="no"
for command in "${POST_BACKUP_COMMANDS[@]}"; do
    if test "$command" = "add_next_as_exit_task" ; then
        exists_as_exit_task="yes"
        continue
    fi
    if is_true $exists_as_exit_task ; then
        RemoveExitTask "$command"
        exists_as_exit_task="no"
    fi
    Log "Running POST_BACKUP_COMMANDS: '$command'"
    eval "$command"
done

rmetrich commented at 2022-01-12 13:55:¶

Hi Johannes,
I think it makes sense to execute POST_BACKUP_SCRIPT even on backup failure, this in order to put the system is a stable state again.
This one would indeed use this for databases to avoid having corruptions in the database if not shut down while creating the backup.

rmetrich commented at 2022-01-12 13:57:¶

And I don't see why there would be a matching POST_BACKUP_COMMAND for a given PRE_BACKUP_COMMAND.
So I think it's up to the user to provide some error-safe commands.

jsmeix commented at 2022-01-12 13:57:¶

Because we must keep things working backward compatible
adding POST_BACKUP_SCRIPT as exit task must be kept.

I question if also all commands in POST_BACKUP_COMMANDS
should be hardcoded added as exit tasks.

rmetrich commented at 2022-01-12 14:02:¶

Because we must keep things working backward compatible adding POST_BACKUP_SCRIPT as exit task must be kept.

It is. It was just moved to line 13-15 because where it was originally wasn't making sense, specially if there was no POST_BACKUP_SCRIPT.

jsmeix commented at 2022-01-12 14:05:¶

I think almost all usual users assume their commands
that are intented to run after backup was made
run under the implicit precondition
that the backup has been actually (sucessfully) made.

So I would not expect usual users to provide commands
that are safe to be run after it failed to make the backup.

And what should users specify who do not want their commands
(or some of their commands) to be run after it failed to make the backup?

jsmeix commented at 2022-01-12 14:35:¶

As far as I see the whole automated adding more than one single command
as exit task works plain wrong because

AddExitTask "COMMAND1"
AddExitTask "COMMAND2"

will run COMMAND2 before COMMAND1 because AddExitTask prepends,
see the function AddExitTask() in lib/_input-output-functions.sh
so

AddExitTask "database start"
AddExitTask "use database"

will let 'use database' just fail because it is run before 'database start'.

As a test I added at the beginning of init/default/030_update_recovery_system.sh

AddExitTask "echo 'first' >>/tmp/rear_exit_tasks"
AddExitTask "echo 'second' >>/tmp/rear_exit_tasks"
AddExitTask "echo 'third' >>/tmp/rear_exit_tasks"
Error "see /tmp/rear_exit_tasks"

and with that

# usr/sbin/rear -D mkrescue
Relax-and-Recover 2.6 / Git
Running rear mkrescue (PID 5256 date 2022-01-12 15:40:16)
Command line options: usr/sbin/rear -D mkrescue
Using log file: /root/rear.github.master/var/log/rear/rear-linux-h9wr.log
Using build area: /var/tmp/rear.I9m9oHrMaFTY1zg
Running 'init' stage ======================
ERROR: see /tmp/rear_exit_tasks
Some latest log messages since the last called script 030_update_recovery_system.sh:
  2022-01-12 15:40:18.754599328 Entering debugscript mode via 'set -x'.
  2022-01-12 15:40:18.761106430 Added 'echo 'first' >>/tmp/rear_exit_tasks' as an exit task
  2022-01-12 15:40:18.764607684 Added 'echo 'second' >>/tmp/rear_exit_tasks' as an exit task
  2022-01-12 15:40:18.768153511 Added 'echo 'third' >>/tmp/rear_exit_tasks' as an exit task
Aborting due to an error, check /root/rear.github.master/var/log/rear/rear-linux-h9wr.log for details
Exiting rear mkrescue (PID 5256) and its descendant processes ...
Running exit tasks
To remove the build area you may use (with caution): rm -Rf --one-file-system /var/tmp/rear.I9m9oHrMaFTY1zg
Terminated

# cat /tmp/rear_exit_tasks
third
second
first

pcahyna commented at 2022-01-12 14:48:¶

will run COMMAND2 before COMMAND1 because AddExitTask prepends

Indeed. Note that the original form AddExitTask "${POST_BACKUP_SCRIPT[@]}" worked well, because if you give multiple tasks to one AddExitTask command, it will add them in sequence, rather than in reverse.

jsmeix commented at 2022-01-19 10:39:¶

@rmetrich @pcahyna
I would like to progress here.

My proposal:

Either
do not call AddExitTask for any POST_BACKUP_COMMANDS
which means we cannot deprecate POST_BACKUP_SCRIPT

Or
have some more sophisticated code where the user can specify
what in POST_BACKUP_COMMANDS should be added as exit tasks
cf. https://github.com/rear/rear/pull/2735#issuecomment-1011070624
plus explanatory documentation in default.conf that

POST_BACKUP_COMMANDS=( "add_next_as_exit_task" "database start"
                       "add_next_as_exit_task" "use database" )

won't work and

POST_BACKUP_COMMANDS=( "add_next_as_exit_task" "database start ; use database" )

must be used instead which should work because

# cmds=( "echo first ; echo second" )

# ( set -x ; for cmd in "${cmds[@]}" ; do eval "$cmd" ; done )
+ for cmd in "${cmds[@]}"
+ eval 'echo first ; echo second'
++ echo first
first
++ echo second
second

but what "man bash" reads seems to contradict

control operator
  A token that performs a control function.
  It is one of the following symbols:
  || & && ; ;; ;& ;;& ( ) | |& <newline>
...
Simple Commands
  A simple command is a sequence of optional variable assignments
  followed by blank-separated words and redirections,
  and terminated by a control operator.
...
eval [arg ...]
  The args are read and concatenated together into a single command.
  This command is then read and executed by the shell

so I wonder what eval 'echo first ; echo second' actually does
because the one argument echo first ; echo second
is not a single (simpe) command.
The confusion happens because "man bash" uses the
plain word "command" with unclear meaning - i.e. whether
a "simple command" or a "compound command" is meant.

jsmeix commented at 2022-02-02 07:33:¶

@rmetrich @pcahyna
could you find some time to progress here - at least a bit?

pcahyna commented at 2022-02-02 08:22:¶

@jsmeix I will, but not immediately. I need to focus on the layout code now, I find it more important.

jsmeix commented at 2022-02-02 09:06:¶

@pcahyna
thank you for your prompt reply!
Of course no rush and as your time permits.
Clearly issues in the layout code are more important.

github-actions commented at 2022-04-04 02:54:¶

Stale pull request message

pcahyna commented at 2022-04-06 15:32:¶

I added it to the 2.7 milestone, so that users can migrate from the old variables ASAP.

pcahyna commented at 2022-04-06 15:45:¶

Hi @rmetrich , I have noticed an asymmetry in these variables. PRE_BACKUP_COMMANDS does not seem to execute at the beginning of the ReaR run, but before the backup of data gets created (the rescue image with the disk layout has been created already): "Call this before Relax-and-Recover starts to do anything in the mkbackup/mkbackuponly workflow." But PRE_RECOVERY_COMMANDS runs at the very beginning of the ReaR run: "You have the rescue system but nothing else". Would it make sense to introduce PRE_RESTORE_COMMANDS that would be symmetrical with PRE_BACKUP_COMMANDS and run just before the backup is restored? (It could be used for custom commands that need to make the backup archive accessible. For example, mount a disk with the archive, if one uses a file:// URL.)

pcahyna commented at 2022-05-02 16:24:¶

My immediate idea is that
when one element in POST_BACKUP_COMMANDS
has the special value add_next_as_exit_task
the next element in POST_BACKUP_COMMANDS
gets added as exit task.

@jsmeix returning to this old topic, sorry for the delay. I must say, I don't like this idea, or generally the idea of using such special values in lists (I know that there is all_modules, but I would prefer to avoid more such cases). If there is really a need for running some commands only when the backup has been successful, I see two cleaner options:

1. introduce another variable, like POST_SUCCESSFUL_BACKUP_COMMANDS
2. in the command, let the user test the EXIT_CODE variable, like this: "(( EXIT_CODE )) || command"

Users can do the latter already.
I am not sure if there is much demand for this, though. @rmetrich provided an example where you want to execute the commands regardless of the backup status (one may want to halt some service temporarily during the backup, but one does not want to leave it halted, even if the backup fails). I can imagine other use cases: one might want to enable the access to the storage where the backup is to be located in PRE_BACKUP_COMMANDS and disable it in POST_BACKUP_COMMANDS, or one might want to take some kind of snapshot before the backup and back up the snapshot. In all those cases, it is needed to run the appropriate cleanup command among POST_BACKUP_COMMANDS regardless of the success of the backup job (you don't want the snapshot to linger and take space, nor the backup storage being left mounted and thus vulnerable to accidental damage). Have there been any complaints about the current behavior, or do you have a concrete use case in mind that requires this change? If such cases are sufficiently rare, one might leave them to be covered by explicit testing of EXIT_CODE in the command by those who need them.

pcahyna commented at 2022-05-02 17:03:¶

@jsmeix @rmetrich I am taking a day off tomorrow, so I can continue the discussion on Wednesday.

jsmeix commented at 2022-05-03 07:11:¶

@pcahyna
enjoy your vacation!

jsmeix commented at 2022-05-03 08:02:¶

@pcahyna
I do not understand how it should work what you wrote:
"let the user test the EXIT_CODE variable ... Users can do the latter already"

I think the EXIT_CODE variable is not set when those
PRE_RECOVERY/POST_RECOVERY/PRE_BACKUP/POST_BACKUP
scripts are run - at least as far as I see according to what

# find usr/sbin/rear usr/share/rear -type f | xargs grep 'EXIT_CODE'

shows me.

Do you perhaps mean "let the user test the EXIT_CODE variable"
as an enhancement that the EXIT_CODE variable should be set
when those scripts (or commands) are run?

Furthermore - as far as I see - in current master code
it seems those scripts (or commands) are "just run"
without any kind of error handling which is against
"Try hard to care about possible errors" in
https://github.com/rear/rear/wiki/Coding-Style

In general I have no personal experience with
PRE_RECOVERY/POST_RECOVERY/PRE_BACKUP/POST_BACKUP
because I do not use them.
I have no user or customer reports related to them.

I guess in most cases users do not use them but instead they call
the right perparation commands outside of ReaR before they call rear
and the right cleanup commands outside of ReaR after rear finished
in particular for PRE_BACKUP/POST_BACKUP scripts
because those are called during "rear mkbackup"
via backup/default/010_pre_backup_script.sh
and backup/default/990_post_backup_script.sh
which only happens for ReaR internal backup methods
so all users with third-party backup methods
cannot use PRE_BACKUP/POST_BACKUP
but must use something outside of ReaR.

In contrast PRE_RECOVERY/POST_RECOVERY
could be more useful in general because those
can be helpful for automated recovery in general
regardless what backup method is used
and the initial issue description here
https://github.com/rear/rear/pull/2735#issue-1089733641
is about PRE_RECOVERY_SCRIPT

The exit tasks complication happens
only for PRE_BACKUP/POST_BACKUP
but not for PRE_RECOVERY/POST_RECOVERY.

I suggest to leave PRE_BACKUP/POST_BACKUP as is
and only enhance PRE_RECOVERY/POST_RECOVERY by
PRE_RECOVERY_COMMANDS and POST_RECOVERY_COMMANDS
via this pull request here.

This way we could move one step forward
in an area that is actually used by at least one user.

Because all users with third-party backup methods
cannot use PRE_BACKUP/POST_BACKUP
but must use something outside of ReaR
and because I do not know about a user request
to have such functionality for third-party backup methods
(regardless that it would be impossible to implement
such functionality for third-party backup methods in ReaR)
I assume users won't have problems with calling (outside of ReaR)
the right perparation commands before they call "rear mkbackup[only]"
and the right cleanup commands after "rear mkbackup[only]" finished
so PRE_BACKUP/POST_BACKUP is no needed functionality in ReaR
and we can leave it "rest in peace".

jsmeix commented at 2022-05-03 08:22:¶

FYI:
PRE_BACKUP/POST_BACKUP originated via
https://github.com/rear/rear/commit/397728d7ed6ccb22338bcb6af03949c8ccf58a91
from
https://github.com/rear/rear/pull/977
where I already had issues with them added as exit tasks
https://github.com/rear/rear/pull/977#issuecomment-242041391
and now - of course - that strikes back :-(

jsmeix commented at 2022-05-03 09:54:¶

In general I also do not like the idea of using special values in lists
when the special values can appear intermixed with normal values
as it would happen in this case here.
It was just "my immediate idea" and I had no better one.

On the other hand I also do not like several config variables
to specify one same thing because usually that leads to complications
with interdependencies of those variables (i.e. "herding cats")
that usually results both complicated code and complicated
to understand by the user how to specify things correctly.

What I do like is special values for config variables
when the special values do not appear intermixed with normal values
as we use it often for config variables with "ternary semantics" like

• either unset or empty,
• or boolean 'true' or 'false',
• or "something else"

(the "either ... or" makes things simple and straightforward)
e.g. as in KEEP_BUILD_DIR or AUTORESIZE_PARTITIONS
or FIRMWARE_FILES and so on.

The MODULES=( 'all_modules' ) is an unfortunate exception
because of its long history of issues how it evolved over time.
I think I vaguely remember that I did not like using
MODULES=('yes') instead of MODULES=('all_modules')
and MODULES=('no') instead of MODULES=('no_modules')
because I feared a module named 'yes' or 'no' might appear
(Unix tradition to use very short names and play with words)
while I assumed there will never be a module 'all_modules' or 'no_modules'
and the latter special values tell more explicitly what they mean.

pcahyna commented at 2022-05-04 09:04:¶

hi @jsmeix, quick note - I also don't object against "magic" values for scalar variables that are otherwise mainly boolean, i.e. ternary (or quaternary, or even Hexenvariables). My objection was against mixing special and regular values in lists indeed.

jsmeix commented at 2022-05-23 13:32:¶

As a proposal how to proceed here
for the ReaR 2.7 release I did now
https://github.com/rear/rear/pull/2811
where I implemented what I suggested here in
https://github.com/rear/rear/pull/2735#issuecomment-1115833664
therein this part (excerpt):

In general I have no personal experience with
PRE_RECOVERY/POST_RECOVERY/PRE_BACKUP/POST_BACKUP
because I do not use them.
I have no user or customer reports related to them.

I guess in most cases users do not use them
but instead they call the right perparation commands
outside of ReaR before they call rear
and the right cleanup commands
outside of ReaR after rear finished
in particular for PRE_BACKUP/POST_BACKUP scripts
because those are called during "rear mkbackup"
via backup/default/010_pre_backup_script.sh
and backup/default/990_post_backup_script.sh
which only happens for ReaR internal backup methods
so all users with third-party backup methods
cannot use PRE_BACKUP/POST_BACKUP
but must use something outside of ReaR.

In contrast PRE_RECOVERY/POST_RECOVERY
could be more useful in general because those
can be helpful for automated recovery in general
regardless what backup method is used
and the initial issue description here
https://github.com/rear/rear/pull/2735#issue-1089733641
is about PRE_RECOVERY_SCRIPT

The exit tasks complication happens
only for PRE_BACKUP/POST_BACKUP
but not for PRE_RECOVERY/POST_RECOVERY.

I suggest to leave PRE_BACKUP/POST_BACKUP as is
and only enhance PRE_RECOVERY/POST_RECOVERY by
PRE_RECOVERY_COMMANDS and POST_RECOVERY_COMMANDS
via this pull request here.

This way we could move one step forward
in an area that is actually used by at least one user.

Because all users with third-party backup methods
cannot use PRE_BACKUP/POST_BACKUP
but must use something outside of ReaR
and because I do not know about a user request
to have such functionality for third-party backup methods
(regardless that it would be impossible to implement
such functionality for third-party backup methods in ReaR)
I assume users won't have problems with calling
(outside of ReaR) the right perparation commands
before they call "rear mkbackup[only]"
and the right cleanup commands
after "rear mkbackup[only]" finished
so PRE_BACKUP/POST_BACKUP is no needed functionality in ReaR
and we can leave it "rest in peace".

jsmeix commented at 2022-06-02 13:40:¶

With https://github.com/rear/rear/pull/2811 merged
this pull request got partially obsoleted by it.

The PRE_RECOVERY_COMMANDS and POST_RECOVERY_COMMANDS part
of this pull request got obsoleted by
https://github.com/rear/rear/pull/2811

The PRE_BACKUP_COMMANDS and POST_BACKUP_COMMANDS part
of this pull request is not implemented because
there are some issues how to implement it cleanly
(see the comments above).

Because there is no real activity here about
PRE_BACKUP_COMMANDS and POST_BACKUP_COMMANDS
I close this pull request at least for now.
Of course it can be reopened when needed.

pcahyna commented at 2022-06-02 15:36:¶

@jsmeix

I think the EXIT_CODE variable is not set when those
PRE_RECOVERY/POST_RECOVERY/PRE_BACKUP/POST_BACKUP
scripts are run

You are right, I am not very familiar with the error and exit handling code - I find it quite complicated. (EXIT_FAIL_MESSAGE does not do the trick either.)

I have just realized what is the most ugly part of the current POST_BACKUP_SCRIPT handling: it is not that it is executed as an exit task in case of an error exit (that seems fairly OK to me), but the fact that it is added to exit tasks only if PRE_BACKUP_SCRIPT is set, and if it is unset POST_BACKUP_SCRIPT will not be executed in case of an error exit:

if test "$PRE_BACKUP_SCRIPT" ; then
    Log "Running PRE_BACKUP_SCRIPT '${PRE_BACKUP_SCRIPT[@]}'"
    AddExitTask "${POST_BACKUP_SCRIPT[@]}"

I have not realized this before and this complicated logic and interdependency between the two variables is something I would prefer not to replicate in any newly introduced variable indeed.

jsmeix commented at 2022-06-03 08:22:¶

So there is an error in the default.conf description:

In case of any error during backup, if POST tasks were defined,
ReaR will run those POST tasks within ExitTasks Array.

I will correct the default.conf description to match what
the current implementation of PRE/POST Backup scripts does
(I won't touch the current implementation now).

didacog commented at 2022-06-03 08:45:¶

Hi, just seen this issue, what is the problem with PRE/POST backup scripts? I've implemented this few years ago. So if any help is needed to understand why was implemented just tell me.

jsmeix commented at 2022-06-03 09:09:¶

Via
https://github.com/rear/rear/commit/fcc5005ee73e73d22360b3a6e4cca6af9d4b03ba
the PRE_BACKUP_SCRIPT and POST_BACKUP_SCRIPT description in default.conf
matches what the current implementation does.

jsmeix commented at 2022-06-03 09:24:¶

@didacog
from my current point of view nothing
is wrong with the code which implements
PRE_BACKUP_SCRIPT and POST_BACKUP_SCRIPT

The documentation in default.conf was not sufficiently precise
which I (hopefully) fixed right now.

What leads to issues is when new config variables
PRE_BACKUP_COMMANDS and POST_BACKUP_COMMANDS
should be implemented according to how
PRE_RECOVERY_COMMANDS and POST_RECOVERY_COMMANDS
are now implemented.
See the above comments what those issues would be.
In current code there are no issues.

didacog commented at 2022-06-03 09:36:¶

Ok @jsmeix , just wanted to help if needed to clarify why was implemented, as I've seen some doubts in the comments.
The explanation now is way better and more specific than before.

Regards,
Didac

pcahyna commented at 2022-06-03 10:35:¶

@didacog we had a discussion on the current semantics of PRE_BACKUP_SCRIPT and POST_BACKUP_SCRIPT and whether it should be translated to new PRE_BACKUP_COMMANDS and POST_BACKUP_COMMANDS. (The motivation for this translation is described in the original description of the PR.) Briefly, the issues with the current sematics are related to the exit handling. It is a question whether to run POST_BACKUP commands even when the workflow has failed (i.e. add them to exit tasks), when it is not done for other POST_ commands (why should POST_BACKUP be exceptional?) Also, adding POST_BACKUP_SCRIPT to exit tasks only when there is a PRE_BACKUP_SCRIPT seems overcomplicated and not obvious. And there is a general question of whether there is any real need for PRE/POST_BACKUP stuff, since one can simply run the commands befopre and after the rear run, instead of putting them into the configuration file (and one has to do it for external backup methods that are not supported by rear mkbackup anyway).
I would be interested what was the motivation behind these semantics decisions (why there is a need for PRE/POST_BACKUP stuff and if it would be a problem if POST_BACKUP is executed only after a successful run).
(Of course, the intent is not to change the actual semantics of PRE/POST_BACKUP_SCRIPT, but to use the possible introduction of PRE/POST_BACKUP_COMMANDS to improve the semantics, if desired. Since we have not agreed yet what counts as an improvement, we have not added PRE/POST_BACKUP_COMMANDS yet.)

didacog commented at 2022-06-03 13:29:¶

@pcahyna the motivation for this implementation was a need for a customer to get a full DR of the system with their small sized databases (mysql, pgsql) with the data in /var/lib/... in a consistent way, being small DBs is safe and fast to get them with the backup in the system, so is fast full DR when recovered with no extra steps.

Can be changed do PRE_BACKUP_COMMANDS and POST_BACKUP_COMMANDS names if desired, and where implemented as a pair of variables PRE+POST to stop services before backup starts (PRE) and start services again after backup finishes OK or fails, so there is the reason for the exit tasks of the POST_BACKUP_SCRIPT.

The other reason to not run before and after rear execution is that in case of mkbackup (mkrescue + mkbackuponly) there is more downtime for the services stopped during mkrescue with no reason, as they need to be stopped due to data consistency only during the backup phase.

Hopefully this clarified you your doubts about the decisions that made us implement this way.

Regards,
Didac

pcahyna commented at 2022-06-03 14:55:¶

@didacog thank you for your explanation. Do you recall why the decision to add POST_BACKUP_SCRIPT to exit tasks only when there is a PRE_BACKUP_SCRIPT defined? The reasoning might be that only if some command is used before mkbackup to bring the system in a quiet state, another command is needed to bring it back to the normal state even in case of errors, otherwise not. But I think that this logic is overcomplicated and against POLA.

didacog commented at 2022-06-07 09:18:¶

@pcahyna no problem at all to add it at the very beginning of 010_pre_backup_script.sh script instead of inside the IF statement.

Regards,
Didac

jsmeix commented at 2022-06-07 09:33:¶

No - there is not "no problem at all" here.
It would be an incompatible change when
POST_BACKUP_SCRIPT gets added to EXIT_TASKS in any case
because then things would behave different
in case of an error during backup when a user
has no PRE_BACKUP_SCRIPT but only a POST_BACKUP_SCRIPT.
Then with current code in case of an error during backup
POST_BACKUP_SCRIPT is not run.

didacog commented at 2022-06-07 09:46:¶

@jsmeix, yes will behave different, yes, but is not going to cause any issue, is just going to allow the POST_BACKUP_SCRIPT to execute anyway in case of failed backup, even if no PRE_BACKUP_SCRIPT, but is not a big problem, just a small "change/enhancement" depending of the point of view. But i was just saying that if you want to change this to allow only POST to be run anyway in case of error, I do not see problems as when PRE is going to be present, things would behave the same as now, IMHO.

Kind regards,
Didac

jsmeix commented at 2022-06-07 09:48:¶

Via
https://github.com/rear/rear/commit/6fcbba895c280c68e8ccf54290348ea845ecf201
I described in default.conf more clearly
when POST_BACKUP_SCRIPT is normally run or run as exit task
versus when it is not run
with current code.

didacog commented at 2022-06-07 09:53:¶

Via 6fcbba8 I described in default.conf more clearly when POST_BACKUP_SCRIPT is normally run or run as exit task versus when it is not run with current code.

Yes, I've seen that, is far better explained than before. Just wanted to clarify to @pcahyna that if wanted to change the behavior of this adding POST to EXIT_TASKS even if no PRE is present, I would have no complaints, as it behaves the same when both PRE/POST are present.

pcahyna commented at 2022-06-07 10:01:¶

@didacog what @jsmeix proposed was, I believe, one of the two:

• do not translate PRE/POST_BACKUP_SCRIPT to the proposed better _COMMANDS syntax at all, and gradually deprecate it (it can be replaced by running a command before and after the backup is made manually)
• translate PRE/POST_BACKUP_SCRIPT to the proposed _COMMANDS syntax, but do not add POST_BACKUP_COMMANDS to exit tasks at all, because running it even in case of errors is unexpected (PRE/POST_RECOVERY stuff does not behave this way).

Hopefully I am summarizing the discussion correctly.
If I understand correctly, you have a use for PRE/POST_BACKUP stuff and you need POST_BACKUP to be run even in case of errors, so those options do not sound very good to you?

jsmeix commented at 2022-06-07 10:56:¶

I assume my wording in my above
https://github.com/rear/rear/pull/2735#issuecomment-1134686196

... PRE_BACKUP/POST_BACKUP is no needed functionality in ReaR
and we can leave it "rest in peace".

was misleading because I did not intend to drop it
because I also wrote there

I suggest to leave PRE_BACKUP/POST_BACKUP as is
and only enhance PRE_RECOVERY/POST_RECOVERY by
PRE_RECOVERY_COMMANDS and POST_RECOVERY_COMMANDS

I meant leave it "rest in peace" literally
i.e. as leave PRE_BACKUP/POST_BACKUP as is
and not as "bury it".

With
"PRE_BACKUP/POST_BACKUP is no needed functionality in ReaR"
I meant we do not need to care much about it and enhance it
so we can just leave PRE_BACKUP/POST_BACKUP as is.

didacog commented at 2022-06-07 12:31:¶

Ok, @jsmeix so it is ok to me to keep it as is, or modify it a little as I explained before, but I'm not agree to deprecate this feature at all.

jsmeix commented at 2022-06-07 12:46:¶

I prefer to leave PRE_BACKUP_SCRIPT/POST_BACKUP_SCRIPT as is.

Regarding
adding POST_BACKUP_SCRIPT to EXIT_TASKS
only when PRE_BACKUP_SCRIPT is set:

If a user wants to have POST_BACKUP_SCRIPT added to EXIT_TASKS
but he does not have a PRE_BACKUP_SCRIPT, he can set a dummy

PRE_BACKUP_SCRIPT=( "true" )

If a user does not want to have POST_BACKUP_SCRIPT added to EXIT_TASKS
he can prepend a dummy empty first element to his PRE_BACKUP_SCRIPT

PRE_BACKUP_SCRIPT=( '' "COMMANDS" )

because backup/default/010_pre_backup_script.sh
tests the first element in the PRE_BACKUP_SCRIPT array

... test "$PRE_BACKUP_SCRIPT" ...

and an empty first element in PRE_BACKUP_SCRIPT
should not cause issues with

eval "${PRE_BACKUP_SCRIPT[@]}"

---

[Export of Github issue for rear/rear.]