#3070 PR merged: Add REBOOT_COMMANDS (issue 3068)

Labels: enhancement, fixed / solved / done

jsmeix opened issue at 2023-11-07 12:50:

In skel/default/etc/scripts/system-setup
replaced the hardcoded 'reboot'
by using a REBOOT_COMMANDS array
(plus REBOOT_COMMANDS_LABEL string)
so the user could specify a alternative command
like 'shutdown' or 'poweroff'
(see https://github.com/rear/rear/issues/3068)
or a sequence of commands if needed
(plus a label that is shown to the user).

Use USER_INPUT_INTERRUPT_TIMEOUT
and USER_INPUT_UNATTENDED_TIMEOUT
instead of hardcoded 'sleep' timeout values
to speed up things in unattended_recovery mode.

Show non-zero exit codes to the user
when sourcing config files failed.

Abort when /usr/share/rear/conf/default.conf
does not exist (or is empty).

Set SECRET_OUTPUT_DEV="null" in any case
(not only when sourcing default.conf)
because also in other config files (e.g. local.conf)
secret default values could be set via

{ VARIABLE='secret value' ; } 2>>/dev/$SECRET_OUTPUT_DEV

cf. https://github.com/rear/rear/commit/9629b29dbbb73efb6229c4bfc509d1fcb70b29e3

jsmeix commented at 2023-11-07 13:22:

I will test it tomorrow (as time permits).

GitarPlayer commented at 2023-11-07 21:46:

I did test it too and it would close my issue. Thanks for your work.

jsmeix commented at 2023-11-08 14:00:

It works well for me with

ISO_RECOVER_MODE=unattended
ISO_DEFAULT=automatic
RECOVERY_REBOOT_COMMAND=poweroff
USER_INPUT_INTERRUPT_TIMEOUT=5

OUTPUT=ISO
BACKUP=NETFS
...

I mean I get a full automated and unattended recovery
with 'poweroff' after recovery.

jsmeix commented at 2023-11-08 14:02:

@rear/contributors
could you please review it (as time permits)
or at least have a look if you spot some obvious mistake?

pcahyna commented at 2023-11-08 14:34:

Hi @jsmeix , I will have a look. At first glance: I would call it ..._COMMANDS and accept an array of commands, for symmetry with #2811 .

pcahyna commented at 2023-11-08 23:34:

If you wonder why would anyone want anything else than a simple command for reboot - on machines with more sophisticated firmware one may need some complicated dance with firmware settings, as we currently do in our PowerVM test here https://github.com/lzaoral/rear-testing/blob/22e4a4ec12ff88d0b55836854db1d4a0cefdbf09/Sanity/make-backup-and-restore-powervm/runtest.sh#L135 and UEFI test here : https://github.com/lzaoral/rear-testing/blob/22e4a4ec12ff88d0b55836854db1d4a0cefdbf09/Sanity/make-backup-and-restore-uefi/runtest.sh#L124

pcahyna commented at 2023-11-08 23:35:

Regarding the name - wouldn't UNATTENDED_REBOOT_COMMANDS for example be more descriptive? The setting seems to be used only in the unattended case.

jsmeix commented at 2023-11-09 07:27:

The current RECOVERY_REBOOT_COMMAND is used
both in the 'automatic_recovery'
and in the 'unattended_recovery' cases
via

reboot_command="${RECOVERY_REBOOT_COMMAND:-reboot}"
...
if automatic_recovery ; then
...
        choices+=( "$reboot_command" )
...
            (3)
                $reboot_command
...
if unattended_recovery ; then
...
        echo -e "\n'$reboot_command' in $timeout seconds (Ctrl-C to interrupt)\n"
...
        $reboot_command

(see its description in default.conf).

So UNATTENDED_REBOOT_COMMANDS would be a too specific name
and AUTOMATIC_OR_UNATTENDED_REBOOT_COMMANDS looks oververbose
and perhaps the current RECOVERY_REBOOT_COMMAND might become
useful later also for other (currently unknown) use cases.

So I think the current generic name is future-proof and
it still tells sufficiently what that thingy is about.
That RECOVERY_REBOOT_COMMAND does not matter when the user
manually types in his reboot command should be obvious.

@pcahyna
in your example
https://github.com/rear/rear/pull/3070#issuecomment-1802901263
it seems POST_RECOVERY_SCRIPT or POST_RECOVERY_COMMANDS
are sufficient for your needs so I am still wondering
if more than a simple command is ever needed
for the actual "reboot" call?

The only reason I can imagine right now is that some
reboot preparation commands are needed but then I wonder
why POST_RECOVERY_COMMANDS could ever be not sufficient.
I.e. I am wondering if ever reboot preparation commands
cannot be run "inside" 'rear' via POST_RECOVERY_COMMANDS
but must be run after "rear recover" had finished.

I ask because a RECOVERY_REBOOT_COMMANDS array
would make the code (a bit) more complicated and
unless there is a reason I would prefer the KISS principle.

On the other hand a RECOVERY_REBOOT_COMMANDS array
would make it even more future-proof for other
(currently unknown) use cases.

pcahyna commented at 2023-11-10 12:47:

regarding the errored backup & recovery CI test, I am discussing with the Testing Farm people to provide us access to the console logs for each test run. This way we will be able to debug such errors.

jsmeix commented at 2023-11-10 12:53:

Next week I will enhance it to use
a RECOVERY_REBOOT_COMMANDS array.

jsmeix commented at 2023-11-10 12:55:

Because "rear recover" won't work without default.conf
I will abort in etc/scripts/system-setup
when there is no default.conf

jsmeix commented at 2023-11-10 13:54:

@pcahyna @schlomo
thank you for your valuable review comments!

I which you and all @rear/contributors
a relaxed and recovering weekend!

pcahyna commented at 2023-11-13 16:15:

@pcahyna in your example #3070 (comment) it seems POST_RECOVERY_SCRIPT or POST_RECOVERY_COMMANDS are sufficient for your needs so I am still wondering if more than a simple command is ever needed for the actual "reboot" call?

The only reason I can imagine right now is that some reboot preparation commands are needed but then I wonder why POST_RECOVERY_COMMANDS could ever be not sufficient. I.e. I am wondering if ever reboot preparation commands cannot be run "inside" 'rear' via POST_RECOVERY_COMMANDS but must be run after "rear recover" had finished.

Preparation steps can be done in POST_RECOVERY_COMMANDS and only the actual reboot in RECOVERY_REBOOT_COMMAND. But to me it looks more logical to keep related things in one place. Otherwise, one has to think about stuff like, what if ReaR fails, won't it lead to the execution of one command without the other? (It would not, by the way, as I understand it, because if there is an error exit, POST_RECOVERY_COMMANDS are skipped, and the script which runs RECOVERY_REBOOT_COMMAND skips it as well in case of a non-zero exit code. But one has to think about it, while if one puts it into one variable, the relation is obvious.)

An user who will investigate how to automate things will see both POST_RECOVERY_COMMANDS and RECOVERY_REBOOT_COMMAND and it will be easier to use if both are named the same way (without the subtle COMMANDS vs. COMMAND difference) and use the same semantics (array of commands).

I ask because a RECOVERY_REBOOT_COMMANDS array would make the code (a bit) more complicated and unless there is a reason I would prefer the KISS principle.

On the other hand a RECOVERY_REBOOT_COMMANDS array would make it even more future-proof for other (currently unknown) use cases.

Yes, we don't know what the users' use case will be, so an array is more flexible, especially if users want to set the variable at multiple places (think site.conf vs. local.conf or managing local.conf by appending multiple pieces of content). That was the reason of preferring *_COMMANDS over *_SCRIPT: with a single script like it is difficult to append (or in the case of reboot, prepend) commands to a preexisting setting: one has to do some complicated dance with semicolons to get it right.

pcahyna commented at 2023-11-13 16:34:

So UNATTENDED_REBOOT_COMMANDS would be a too specific name
and AUTOMATIC_OR_UNATTENDED_REBOOT_COMMANDS looks oververbose
and perhaps the current RECOVERY_REBOOT_COMMAND might become
useful later also for other (currently unknown) use cases.

Thank you for pointing out that the command is also for the automatic but not unattended case. Still, to me it does not look obvious that RECOVERY_REBOOT_COMMAND is run after ReaR terminates (somehow one would expect a POST_RECOVERY thing to be run after a RECOVERY thing, not before). OTOH, I am not able to find a better name now and maybe the REBOOT part makes it clear enough (reboot must be the last thing to be done).

schlomo commented at 2023-11-13 16:49:

Maybe something like RESCUE_SYSTEM_REBOOT_COMMANDS as it is actually run by the rescue system and not ReaR itself? POST_RECOVERY_COMMANDS and RECOVERY_REBOOT_COMMANDS would also work for me, or maybe RESCUE_REBOOT_COMMANDS?

Maybe most important is to stick with the naming scheme of *_COMMANDS that makes finding those options simpler and trusting users to read the explanation we put into our default.conf for each of those.

jsmeix commented at 2023-11-14 13:48:

@pcahyna @schlomo
thank you for your valuable comments.

Perhaps the simplest and most clear name is

REBOOT_COMMANDS

without additional things like "RECOVERY" or "POST"
because REBOOT alone makes it clear what it is about.

My assumption is that users who think about setting
'REBOOT_COMMANDS' read its description in default.conf
and normally the default REBOOT_COMMANDS=( reboot )
"just works" unnoticed by users so in particular users
won't falsely fear that e.g. "rear mkbackup" may 'reboot'.

schlomo commented at 2023-11-14 13:49:

Good idea, simple is better, and rebooting is sort of very final for a program to do, so that the exact context doesn't matter so much. :-)

jsmeix commented at 2023-11-14 14:04:

@pcahyna
thank you in particular for your

Preparation steps can be done in POST_RECOVERY_COMMANDS
and only the actual reboot in RECOVERY_REBOOT_COMMAND.
But to me it looks more logical to
keep related things in one place.

I mention RFC1925 so often,
in particular RFC1925 items (5) and (6) and
I tell so often to Keep Separated Issues Separated ("KSIS")
but here I was about to implement RFC1925 items (5) and (6),
so lesson learned
(could be RFC1925 additional item (13) - its number tells all :-)

(13)
No matter how hard you try, in practice you can't avoid
to implement item (5) and/or (6) of this RFC1925
(unless others care and show you your fault).

jsmeix commented at 2023-11-14 15:27:

My last two commits
https://github.com/rear/rear/pull/3070/commits/961b4883f9a241d4b92882293c197ce25835fda5
and
https://github.com/rear/rear/pull/3070/commits/4ff2862092a1334e57ff75a0108de1f0d67561d5
is a currently totally untested attempt
to implement the REBOOT_COMMANDS array.

If you like you may have a first look,
perhaps you spot some obvious mistake.

I will try to test it tomorrow (as time permits).

pcahyna commented at 2023-11-16 11:37:

looking... generally I like the REBOOT_COMMANDS name, it clearly sets it apart from the various PRE_/POST_ stuff, indicating that it is a different kind of stuff

jsmeix commented at 2023-11-16 13:28:

With the last changes here
things seem to work OK at least for me
but I will have to test a bit more tomorrow.
Sigh - there are so many different cases to test...

jsmeix commented at 2023-11-17 13:40:

I could not do anything here today because
"other things" appeared so I will continue next week.

jsmeix commented at 2023-11-20 13:42:

Things seem to work OK at least for me
so I would like to merge it tomorrow afternoon
unless there are objections.

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

Thank you @jsmeix for this improvement. I have found another use for it : copy logs like /var/log/messages and the systemd journal to /mnt/local for later inspection. It would be more logical to do it in POST_RECOVERY_COMMANDS in fact, but it may be better to copy logs as late as possible to capture the most of them.

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

@pcahyna
currently REBOOT_COMMANDS are only useful when "rear recover"
is run in 'auto_recover'/'automatic' or 'unattended' mode.
I am thinking about how to make REBOOT_COMMANDS also useful
when "rear recover" is run manually.
The problem is how to make ReaR's internal REBOOT_COMMANDS data
available outside of ReaR so that the user could call them
on his commandline.

schlomo commented at 2023-11-27 06:51:

We don't know what happens between a user running rear recover manually and eventually rebooting the system - and we don't know if /mnt/local is still the recovered system or maybe something else.

Therefore I'd be careful about assuming too much and copying logs there.

The situation of running the recovery fully automated is much more controlled so that in that case it is safe to make those assumptions.

I think that this PR is a very good iteration and improvement for ReaR, big thanks to @jsmeix for working on this.

pcahyna commented at 2023-11-27 12:00:

currently REBOOT_COMMANDS are only useful when "rear recover"
is run in 'auto_recover'/'automatic' or 'unattended' mode.

This is not a problem for the case I mentioned, because I need this to collect data in automated testing, which runs unattended. (It could be a ReaR enhancement to copy systems logs at the end of recovery though. We are already doing the same with the recovery log.)

pcahyna commented at 2023-11-27 12:01:

I am thinking about how to make REBOOT_COMMANDS also useful
when "rear recover" is run manually.

what do you mean by "making them useful"? I would not execute them automatically at the end, that's what POST_RECOVERY_COMMANDS are for. One could preload the shell history with the commands or something similar.

jsmeix commented at 2023-11-27 14:59:

With "making them useful when 'rear recover' is run manually"
I mean that the user could relatively easily
run the REBOOT_COMMANDS manually from his
recovery system commandline prompt.

What I am currently thinking about is something like a script
e.g. /bin/reboot_commands.sh inside the recovery system
that gets generated during "rear mkrescue" so that
the user could alternatively call that script
instead of "reboot" if he wants.

[Export of Github issue for rear/rear.]