New RECOVERY_COMMANDS array #3089

jsmeix · 2023-11-21T14:26:49Z

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.

Type: Enhancement
Impact: Normal
Reference to related issue (URL):
'-c' and '-C' options are ignored with 'automatic/auto_recover' and 'unattended' modes #2942
Add REBOOT_COMMANDS (issue 3068) #3070 (comment)
How was this pull request tested?

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.

In skel/default/etc/scripts/system-setup added RECOVERY_COMMANDS (plus RECOVERY_COMMANDS_LABEL)

In [skel/default]/etc/scripts/system-setup implemented RECOVERY_COMMANDS together with RECOVERY_COMMANDS_LABEL for automatic_recovery and unattended_recovery plus several enhancements for debug mode and 'read' timeouts for unattended_recovery

Deleted the old system-setup code

jsmeix · 2023-11-22T15:55:47Z

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 · 2023-11-23T15:30:01Z

@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 · 2023-11-23T15:42:07Z

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 · 2023-11-27T06:35:50Z

@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...

In default.conf set and describe RECOVERY_COMMANDS and RECOVERY_COMMANDS_LABEL

Set rear_options to '-D' instead of '-v -D' because '-D' implies '-v'

jsmeix · 2023-11-27T11:39:55Z

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

typo fix and better wording in default.conf comment

jsmeix · 2023-11-29T06:35:15Z

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

jsmeix · 2023-12-01T10:23:18Z

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

usr/share/rear/skel/default/etc/scripts/system-setup

pcahyna · 2023-12-01T13:04:57Z

@jsmeix please squash commits e746717 and 2aba2a3 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).

usr/share/rear/skel/default/etc/scripts/system-setup

jsmeix · 2023-12-04T13:14:08Z

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

Do no longer call eval echo "... $command ..." but instead call plain echo "... $command ..." (default command='rear $rear_options recover') to keep the code simple and fail-safe because "Better very simple code than oversophisticated (possibly fragile) constructs.", cf. https://github.com/rear/rear/wiki/Coding-Style

Use the simple default RECOVERY_COMMANDS_LABEL='rear recover' instead of RECOVERY_COMMANDS_LABEL='rear $rear_options recover' because the latter would need fragile code like eval echo "... $RECOVERY_COMMANDS_LABEL ..." or complicated code to safely evaluate nested variables and this is only meant as some meaningful user info about what is going on but not as debug info what the exact called command is.

jsmeix · 2023-12-06T12:40:06Z

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 · 2023-12-06T13:09:00Z

@jsmeix please see the note about rebase and squash.

pcahyna · 2023-12-06T13:12:21Z

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 · 2023-12-06T13:16:36Z

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 · 2023-12-06T13:40:59Z

I squashed all the commits together.

jsmeix · 2023-12-07T10:18:26Z

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:

# 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 · 2023-12-07T10:46:03Z

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 · 2023-12-07T10:58:21Z

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.

Walking back the chain of parents finally leads to
09c81c9
which points to the wrong pull request,
i.e. not the one where
b131e47
belongs to (which is this pull request here).

In contrast without squashing commits
for a commit "inside" a pull request like
4960ca8
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 · 2023-12-07T11:04:23Z

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 b131e47, 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. b131e47 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 · 2023-12-07T11:15:17Z

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.

New RECOVERY_COMMANDS array

0a7e32f

In skel/default/etc/scripts/system-setup added RECOVERY_COMMANDS (plus RECOVERY_COMMANDS_LABEL)

jsmeix added the enhancement Adaptions and new features label Nov 21, 2023

jsmeix added this to the ReaR v2.8 milestone Nov 21, 2023

jsmeix self-assigned this Nov 21, 2023

jsmeix marked this pull request as draft November 21, 2023 14:27

jsmeix mentioned this pull request Nov 22, 2023

Add REBOOT_COMMANDS (issue 3068) #3070

Merged

jsmeix added 2 commits November 22, 2023 16:50

Update system-setup

e746717

In [skel/default]/etc/scripts/system-setup implemented RECOVERY_COMMANDS together with RECOVERY_COMMANDS_LABEL for automatic_recovery and unattended_recovery plus several enhancements for debug mode and 'read' timeouts for unattended_recovery

Update system-setup

2aba2a3

Deleted the old system-setup code

jsmeix added 2 commits November 27, 2023 10:11

Update default.conf

c9964fd

In default.conf set and describe RECOVERY_COMMANDS and RECOVERY_COMMANDS_LABEL

Update system-setup

9fa58c7

Set rear_options to '-D' instead of '-v -D' because '-D' implies '-v'

Update default.conf

b23995c

typo fix and better wording in default.conf comment

jsmeix marked this pull request as ready for review November 29, 2023 06:35

jsmeix requested a review from a team November 29, 2023 06:35

pcahyna reviewed Dec 1, 2023

View reviewed changes

usr/share/rear/skel/default/etc/scripts/system-setup Show resolved Hide resolved

pcahyna reviewed Dec 1, 2023

View reviewed changes

usr/share/rear/skel/default/etc/scripts/system-setup Outdated Show resolved Hide resolved

jsmeix added 2 commits December 5, 2023 14:04

jsmeix merged commit fec92e2 into master Dec 6, 2023
21 checks passed

jsmeix deleted the jsmeix-RECOVERY_COMMANDS branch December 6, 2023 13:39

jsmeix added the fixed / solved / done label Dec 6, 2023

jsmeix mentioned this pull request Dec 22, 2023

Set 'dmesg -n [4-6]' in new 'setup' script 007_set_dmesg_level.sh #3113

Merged

jsmeix mentioned this pull request Jan 22, 2024

Hardcoded 'eval $command' is problematic and against "final power to the user" #3135

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New RECOVERY_COMMANDS array #3089

New RECOVERY_COMMANDS array #3089

jsmeix commented Nov 21, 2023 •

edited by gdha

jsmeix commented Nov 22, 2023 •

edited

pcahyna commented Nov 23, 2023

pcahyna commented Nov 23, 2023

jsmeix commented Nov 27, 2023 •

edited

jsmeix commented Nov 27, 2023

jsmeix commented Nov 29, 2023

jsmeix commented Dec 1, 2023

pcahyna commented Dec 1, 2023

jsmeix commented Dec 4, 2023

jsmeix commented Dec 6, 2023 •

edited

pcahyna commented Dec 6, 2023

pcahyna commented Dec 6, 2023

pcahyna commented Dec 6, 2023

jsmeix commented Dec 6, 2023

jsmeix commented Dec 7, 2023

pcahyna commented Dec 7, 2023

jsmeix commented Dec 7, 2023 •

edited

pcahyna commented Dec 7, 2023 •

edited

jsmeix commented Dec 7, 2023 •

edited

New RECOVERY_COMMANDS array #3089

New RECOVERY_COMMANDS array #3089

Conversation

jsmeix commented Nov 21, 2023 • edited by gdha

jsmeix commented Nov 22, 2023 • edited

pcahyna commented Nov 23, 2023

pcahyna commented Nov 23, 2023

jsmeix commented Nov 27, 2023 • edited

jsmeix commented Nov 27, 2023

jsmeix commented Nov 29, 2023

jsmeix commented Dec 1, 2023

pcahyna commented Dec 1, 2023

jsmeix commented Dec 4, 2023

jsmeix commented Dec 6, 2023 • edited

pcahyna commented Dec 6, 2023

pcahyna commented Dec 6, 2023

pcahyna commented Dec 6, 2023

jsmeix commented Dec 6, 2023

jsmeix commented Dec 7, 2023

pcahyna commented Dec 7, 2023

jsmeix commented Dec 7, 2023 • edited

pcahyna commented Dec 7, 2023 • edited

jsmeix commented Dec 7, 2023 • edited

jsmeix commented Nov 21, 2023 •

edited by gdha

jsmeix commented Nov 22, 2023 •

edited

jsmeix commented Nov 27, 2023 •

edited

jsmeix commented Dec 6, 2023 •

edited

jsmeix commented Dec 7, 2023 •

edited

pcahyna commented Dec 7, 2023 •

edited

jsmeix commented Dec 7, 2023 •

edited