#3077 PR `closed`: BACKUP=NSR set NSRSERVER properly in 650_check_iso_recoverable.sh¶

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

jsmeix opened issue at 2023-11-13 08:43:¶

Type: Bug Fix
Impact: Normal
Reference to related issue (URL):
https://github.com/rear/rear/issues/3069
How was this pull request tested?
I cannot test it because I do not use BACKUP=NSR
Description of the changes in this pull request:

In layout/save/NSR/default/650_check_iso_recoverable.sh
also read NSR server name from var/lib/rear/recovery/nsr_server
by using the same code snippet as in verify/NSR/default/400_verify_nsr.sh
see https://github.com/rear/rear/issues/2162#issuecomment-541343374
and https://github.com/rear/rear/issues/3069

jsmeix commented at 2023-11-13 08:46:¶

@hpannenb @viper1986
could you please have a look here
and ideally could you test
if things still work for you with this changes
as you need it in your particular use cases?

@rear/contributors
could you please also have a look here
perhaps you could spot some obvious mistake?

jsmeix commented at 2023-11-13 09:17:¶

As far as I currently see
from plain looking at the NSR related code
I think the curent changes here still do not set
NSRSERVER properly in 650_check_iso_recoverable.sh

Reason (a far as I see):

$VAR_DIR/recovery/nsr_server is written in
rescue/NSR/default/460_save_nsr_server_name.sh via

echo "$NSRSERVER" > $VAR_DIR/recovery/nsr_server

which is (as far as I see) the only place where
$VAR_DIR/recovery/nsr_server is written.

But during "rear mkrescue"
layout/save/NSR/default/650_check_iso_recoverable.sh
is run before
rescue/NSR/default/460_save_nsr_server_name.sh
(the 'layout' stage runs before the 'rescue' stage).

So
for the very first run of "rear mkrescue"
$VAR_DIR/recovery/nsr_server
would not exist
and
if the NSR server name changes then
layout/save/NSR/default/650_check_iso_recoverable.sh
would read an old/outdated NSR server name
in $VAR_DIR/recovery/nsr_server
from the previous run of "rear mkrescue".

schlomo commented at 2023-11-13 09:59:¶

Looking at this whole code area again I'm starting to wonder what we are trying to achieve there. Most other layout/save additions for backup software actually only go and extend the CHECK_CONFIG_FILES variable with the configuration directories of the backup software, for example like this:
https://github.com/rear/rear/blob/e035d18034137b30b7b47fe2863fb578f0e9933d/usr/share/rear/layout/save/GALAXY11/default/400_check_galaxy11_configuration.sh#L3

Can we maybe clarify why we try to do much more for NSR? Why do we need to verify the client or server configuration or the existence of backups in NSR during the checklayout workflow? I'd expect this to happen during the mkrescue workflow, if at all.

Maybe the actual "fix" would be getting rid of this all together?

jsmeix commented at 2023-11-13 10:23:¶

As far as I see from the

git log --follow -p usr/share/rear/layout/save/NSR/default/650_check_iso_recoverable.sh

output
layout/save/NSR/default/650_check_iso_recoverable.sh
was created (as .../65_check_iso_recoverable.sh)
because of
https://github.com/rear/rear/issues/653

jsmeix commented at 2023-11-13 10:31:¶

As far as I understand what @tomglx requested in
https://github.com/rear/rear/issues/653
is basically that the checklayout workflow
should be enhanced to also check if the backup still exists.

It think this is (as far as I understand it) bad
because is mixes up to check the disk layout
with checking the backup
so it implements RFC 1925 item (5)

It is always possible to aglutenate [sic]
multiple separate problems into a single
complex interdependent solution.
In most cases this is a bad idea.

schlomo commented at 2023-11-13 10:32:¶

Thanks @jsmeix for finding it, I apparently wasn't part of the conversation back then.

Looking at it again I'd like to either

remove this functionality as it is actually not really part of the ReaR job
make it optionally behind a configuration flag NSR_CHECKLAYOUT_CHECK_BACKUPS that is off by default
maybe add CHECKLAYOUT_CUSTOM_COMMANDS to simplify running extra code as part of checklayout workflows.

My reasoning is that ReaR checklayout main purpose is alerting for the need to re-create the rescue image because the layout of the local system changed. Using it to monitor the backup software is in my opinion another problem that ReaR cannot fulfil well, as it depends on a lot more context than what ReaR typically has.

therefore I'd like to make backup monitoring at least optional, and simplify integrating custom code for this purpose.

WDYT @rear/contributors ?

jsmeix commented at 2023-11-13 10:45:¶

@schlomo
I fully agree with your reasoning
but I would not even make that optional,
at least not in the check_LAYOUT_ workflow
because check_BACKUP_ tasks do not belong there
so if at all a well separated checkbackup workflow
might be implemented if someone implements it.

So I vote for

remove this functionality as it is actually not really part of the ReaR job

I described my personal reasoning in the section
"Relax-and-Recover versus backup and restore" in
https://en.opensuse.org/SDB:Disaster_Recovery

schlomo commented at 2023-11-13 10:50:¶

I think we should try to be nice to our users, which is why I suggest to remove the functionality from checklayout and add a new feature to run custom code during checklayout.

It seems that for users it is simpler to refer to a custom script or put custom code in ReaR config compared to plugging their script into the ReaR filesystem hierarchy to be executed as part of a workflow. That was the original idea of ReaR extensibility that apparently doesn't find much acceptance from users.

jsmeix commented at 2023-11-13 11:39:¶

@schlomo
I can confirm that at least certain customers
do not want to add own scripts into ReaR directories
or adapt existing ReaR scripts as they need it
but only can specify things in config files.
I never got a reason or even an explanation why.
My blind guess is that they perhaps in general
do not want or cannot touch what is installed as RPMs?
I guess perhaps because of issues with RPM package updates
or issues with support for "touched" software ('rpm -V' checks)?
So it seems at least for certain customers the whole idea of
"enhance and adapt the software as needed"
does not fit?
Perhaps actually many customers appreciate it that they can
"enhance and adapt the ReaR scripts as needed"
but I never hear something from them?

schlomo commented at 2023-11-13 12:11:¶

I'd guess that it mostly an aversion to go and patch 3rd party software, even though it is intended to be used that way.

Adding a file into the ReaR directories will not break RPM or DEB validation, which is why I proposed it as the original way of extending ReaR (via a custom-rear package that ships some files to extend ReaR and depends on the rear package to bring in ReaR)

Anyway, would you like to rework this PR as discussed here?

jsmeix commented at 2023-11-13 12:41:¶

No, I cannot.
Reason:
In general there is nothing what I could do
in case of issues with third-party backup tools
because I do not have such software
so I can neither test nor reproduce anything.

[Export of Github issue for rear/rear.]

#3077 PR closed: BACKUP=NSR set NSRSERVER properly in 650_check_iso_recoverable.sh¶