#3089 PR merged: New RECOVERY_COMMANDS array

Labels: enhancement, fixed / solved / done

jsmeix opened issue at 2023-11-21 14:26:

New RECOVERY_COMMANDS array that specifies the "rear recover" commands
which are automatically called after the ReaR recovery system has started up
to recreate the system in 'auto_recover'/'automatic' or 'unattended' mode.

Seems to work OK for me so far
with the defaults (in default.conf)
and also with (excerpt)

ISO_RECOVER_MODE=unattended
ISO_DEFAULT=automatic
RECOVERY_COMMANDS=( "echo 'rear -Dn recover' in $USER_INPUT_INTERRUPT_TIMEOUT seconds" "sleep $USER_INPUT_INTERRUPT_TIMEOUT" "rear -Dn recover" )
RECOVERY_COMMANDS_LABEL="rear -Dn recover"
REBOOT_COMMANDS=( "echo poweroff in $USER_INPUT_UNATTENDED_TIMEOUT seconds" "sleep $USER_INPUT_UNATTENDED_TIMEOUT" "poweroff" )
REBOOT_COMMANDS_LABEL="poweroff"
  • Description of the changes in this pull request:

In skel/default/etc/scripts/system-setup
added RECOVERY_COMMANDS (plus RECOVERY_COMMANDS_LABEL)
and in default.conf set and describe defaults
so that manual "rear recover" behaves as before.

jsmeix commented at 2023-11-22 15:55:

At least for me my current code in
[skel/default]/etc/scripts/system-setup
seems to work rather well (likely not yet perfect).

Because currently there is nothing in default.conf
for RECOVERY_COMMANDS I test with things like (excerpt)

#ISO_RECOVER_MODE=unattended
#ISO_DEFAULT=automatic
RECOVERY_COMMANDS=( 'echo rear $rear_options recover in $USER_INPUT_INTERRUPT_TIMEOUT seconds'
                    'sleep $USER_INPUT_INTERRUPT_TIMEOUT'
                    'rear $rear_options recover' )
RECOVERY_COMMANDS_LABEL='rear $rear_options recover'
REBOOT_COMMANDS=( 'echo poweroff in $USER_INPUT_UNATTENDED_TIMEOUT seconds'
                  'sleep $USER_INPUT_UNATTENDED_TIMEOUT'
                  'poweroff' )
REBOOT_COMMANDS_LABEL='poweroff'
#USER_INPUT_INTERRUPT_TIMEOUT=11
#USER_INPUT_UNATTENDED_TIMEOUT=7

pcahyna commented at 2023-11-23 15:30:

@jsmeix your PR should add a default value for RECOVERY_COMMANDS to default.conf, I suspect that that's why the CI tests are failing.

pcahyna commented at 2023-11-23 15:42:

I believe this will be very useful especially for automated testing. One will be able to inject flags like -D or -d, and even test another recovery system commands, like rear mountonly, or perform examination of the recovery system (saving logs etc).

jsmeix commented at 2023-11-27 06:35:

@pcahyna
currently there is nothing in default.conf for RECOVERY_COMMANDS
because currently this pull request is a work in progress draft.
Soon I will add defaults for RECOVERY_COMMANDS
and RECOVERY_COMMANDS_LABEL together with
a description what that thingy is good for...

jsmeix commented at 2023-11-27 11:39:

I will test with the defaults and if this works OK for me
I change it from "Draft" to "Ready for review".

jsmeix commented at 2023-11-29 06:35:

I tested with the defaults and it works well for me.

jsmeix commented at 2023-12-01 10:23:

@rear/contributors
I would like to merge it next Monday afternoon
unless there are objections.

pcahyna commented at 2023-12-01 13:04:

@jsmeix please squash commits e7467178ce91b552b3d2d455f4f04a0406156903 and 2aba2a38860cbc27802eb01c34398a87800d9206 and force push, it is confusing in the history (for example in the git log -u output if there is addition of new code and then deletion of the old code).

jsmeix commented at 2023-12-04 13:14:

@rear/contributors
of course I will not merge it
until the comments were properly addressed
which I will do as time permits...

jsmeix commented at 2023-12-06 12:40:

A final test with

ISO_RECOVER_MODE=unattended
ISO_DEFAULT=automatic
OUTPUT=ISO

and the default RECOVERY_COMMANDS and REBOOT_COMMANDS
worked well for me
so I will merge it.

pcahyna commented at 2023-12-06 13:09:

@jsmeix please see the note about rebase and squash.

pcahyna commented at 2023-12-06 13:12:

maybe you can actually squash all the commits together because the latter commits merely fix problems with the earlier commits and we don't need all the commits in the history?

pcahyna commented at 2023-12-06 13:16:

A final test with

ISO_RECOVER_MODE=unattended
ISO_DEFAULT=automatic
OUTPUT=ISO

and the default RECOVERY_COMMANDS and REBOOT_COMMANDS

That's what the CI test actually does, so you maybe don't need to spend time on this kind of test (unless you want to test specifically on SUSE distributions, the CI runs on Fedora and CentOS).

jsmeix commented at 2023-12-06 13:40:

I squashed all the commits together.

jsmeix commented at 2023-12-07 10:18:

A further side note regarding
https://github.com/rear/rear/pull/3089#discussion_r1413862335
(i.e. using 'eval echo')
and
https://github.com/rear/rear/pull/3089#discussion_r1414329071
(i.e. using 'envsubst')
to evaluate nested variable values:

Neither 'eval' nor 'envsubst' properly evaluate
even deeper nested variable values:

# export toplevel='toplevel with $indirection1'

# export indirection1='indirection1 with $indirection2'

# export indirection2='no further indirection'

# echo $toplevel
toplevel with $indirection1

# eval echo $toplevel
toplevel with indirection1 with $indirection2

# eval eval echo $toplevel
toplevel with indirection1 with no further indirection

# echo $toplevel | envsubst
toplevel with indirection1 with $indirection2

# echo $toplevel | envsubst | envsubst
toplevel with indirection1 with no further indirection

When one knows the varaiable names one could do

# indirection1_evaluated="${toplevel//'$indirection1'/$indirection1}"

# indirection2_evaluated="${indirection1_evaluated//'$indirection2'/$indirection2}"

# echo $indirection2_evaluated
toplevel with indirection1 with no further indirection

Perhaps I should do the latter for 'rear_options' like

# command='rear $rear_options workflow'

# rear_options='-v'

# echo "Running '${command//\$rear_options/$rear_options}' automatically"
Running 'rear -v workflow' automatically

I don't know what is better:

# echo "... '${var//'$nested'/$nested}' ..."

or

# echo "... '${var//\$nested/$nested}' ..."

To me the latter code looks more clear.

'${parameter//pattern/string}' is old bash functionality
(I tested it also on SLES 10 SP4 with GNU bash 3.1.17).

The 'rear_options' value is safe because we set it in
[usr/share/rear/skel/default]/etc/scripts/system-setup

pcahyna commented at 2023-12-07 10:46:

A further side note regarding #3089 (comment) (i.e. using 'eval echo') and #3089 (comment) (i.e. using 'envsubst') to evaluate nested variable values:

Neither 'eval' nor 'envsubst' properly evaluate even deeper nested variable values:

But this is proper behavior! Shell itself does not evaluate this. As shown in your first example, below. So eval, which evaluates the string as interpreted by the shell, should not do it either. It would be very unsafe if the shell interpreted variable references recursively.

# export toplevel='toplevel with $indirection1'

# export indirection1='indirection1 with $indirection2'

# export indirection2='no further indirection'

# echo $toplevel
toplevel with $indirection1

(...)

When one knows the varaiable names one could do

# indirection1_evaluated="${toplevel//'$indirection1'/$indirection1}"

# indirection2_evaluated="${indirection1_evaluated//'$indirection2'/$indirection2}"

# echo $indirection2_evaluated
toplevel with indirection1 with no further indirection

Perhaps I should do the latter for 'rear_options' like

What is the problem you are trying to solve? We don't have any nested variable references like this currently. The rear_options value does not contain any variable reference. So, what would you need evaluation of deeply nested variable references for?

# command='rear $rear_options workflow'

# rear_options='-v'

# echo "Running '${command//\$rear_options/$rear_options}' automatically"
Running 'rear -v workflow' automatically

I don't know what is better:

# echo "... '${var//'$nested'/$nested}' ..."

or

# echo "... '${var//\$nested/$nested}' ..."

To me the latter code looks more clear.

'${parameter//pattern/string}' is old bash functionality (I tested it also on SLES 10 SP4 with GNU bash 3.1.17).

The 'rear_options' value is safe because we set it in [usr/share/rear/skel/default]/etc/scripts/system-setup

I thought you wanted to keep the code simple, i.e. no attempts at variable replacement in the displayed string? Anyway, if you want to perform replacement anyway, I think the latter code is clearer. Too many single quotes in the former.

jsmeix commented at 2023-12-07 10:58:

A remark regarding squashing commits:

While squashing commits makes the history
of the master branch look cleaner
it seems it causes that the squashed commits
(i.e. commits "inside" a pull request)
become somewhat "dangling" or "orphaned" to some extent
because e.g. for
https://github.com/rear/rear/commit/b131e473807f28c051f1c019d17d63c245a406da
the GitHub web frontend shows

This commit does not belong to any branch on this repository,
and may belong to a fork outside of the repository.

Walking back the chain of parents finally leads to
https://github.com/rear/rear/commit/09c81c9f93fe6a76e9e35122e2ff45f1427c1e8f
which points to the wrong pull request,
i.e. not the one where
https://github.com/rear/rear/commit/b131e473807f28c051f1c019d17d63c245a406da
belongs to (which is this pull request here).

In contrast without squashing commits
for a commit "inside" a pull request like
https://github.com/rear/rear/commit/4960ca8b4bbc5d8346470f54be289d7482e3eaf4
the GitHub web frontend shows master (#3070) with links

[master](https://github.com/rear/rear) ([#3070](https://github.com/rear/rear/pull/3070))

so without squashing for commits "inside" a pull request
it is still visible to what pull request they belong.

pcahyna commented at 2023-12-07 11:04:

A remark regarding squashing commits:

While squashing commits makes the history of the master branch look cleaner it seems it causes that the squashed commits (i.e. commits "inside" a pull request) become somewhat "dangling" or "orphaned" to some extent because e.g. for b131e47 the GitHub web frontend shows

This commit does not belong to any branch on this repository,
and may belong to a fork outside of the repository.

I see. The problem seems to be that the WebUI https://github.com/rear/rear/pull/3089/commits still shows b131e473807f28c051f1c019d17d63c245a406da, but it is dangling, which is not intuitive.

IMO, the right solution is to squash the commits yourself locally and force push. Then merge in the WebUI without using the squash option. This will update https://github.com/rear/rear/pull/3089/commits with the new commits that also the merged history will contain. b131e473807f28c051f1c019d17d63c245a406da would not be visible at all, instead of it you would see the squashed commit(s) under https://github.com/rear/rear/pull/3089/commits. Would that work?

jsmeix commented at 2023-12-07 11:15:

Regarding properly evaluating nested variables:

I only liked to show that there is no proper way to
evaluate (possibly even deeper) nested variables
so in general it is useless to try to do that.

Only under known restricted/controlled conditions
nested variables can be properly evaluated.

Currently I don't know if I will ever implement

${command//\$rear_options/$rear_options}

but at least I know that this specific case is feasible.

[Export of Github issue for rear/rear.]