#2951 Issue `closed`: Scripts in prep stage shouldn't touch ROOTFS_DIR¶

Labels: bug, cleanup, no-issue-activity

schlomo opened issue at 2023-03-04 20:53:¶

I just noticed that we have some scripts in prep that write to $ROOTFS_DIR even though the README there clearly states not to do so.

REAR rear-ol8u7:/src/rear/usr/share/rear # cat prep/README

This is the preparation part of ReaR before starting the rescue build phase.

Please note that you should not put any scripts that reference
$ROOTFS_DIR or $VAR_DIR/recovery into this section. Scripts for $ROOTFS_DIR
go to rescue/ and for $VAR_DIR/recovery go to layout/ respectively

That way we keep only the real prep stuff here in the prep section.


REAR rear-ol8u7:/src/rear/usr/share/rear # grep -r ROOTFS_DIR prep

prep/BORG/default/200_prep_borg.sh:mkdir -p "$ROOTFS_DIR/usr/lib/locale"
prep/BORG/default/200_prep_borg.sh:localedef -f UTF-8 -i en_US "$ROOTFS_DIR/usr/lib/locale/en_US.UTF-8"
prep/NETFS/default/100_check_nfs_version.sh:# in $ROOTFS_DIR
prep/NETFS/default/100_check_nfs_version.sh:    mkdir -p $v ${ROOTFS_DIR}${src} >&2
prep/NETFS/GNU/Linux/205_inspect_tar_capabilities.sh:# save the BACKUP_PROG_OPTIONS array content to the $ROOTFS_DIR/etc/rear/rescue.conf
prep/NETFS/GNU/Linux/205_inspect_tar_capabilities.sh:echo "BACKUP_PROG_OPTIONS=( ${BACKUP_PROG_OPTIONS[@]} )" >> $ROOTFS_DIR/etc/rear/rescue.conf
prep/README:$ROOTFS_DIR or $VAR_DIR/recovery into this section. Scripts for $ROOTFS_DIR
prep/default/005_remove_workflow_conf.sh:mkdir -p $v $ROOTFS_DIR/etc/rear >&2
prep/default/005_remove_workflow_conf.sh:rm -f $v $ROOTFS_DIR/etc/rear/rescue.conf >&2
prep/default/100_init_workflow_conf.sh:cat - <<EOF >> "$ROOTFS_DIR/etc/rear/rescue.conf"
prep/default/490_store_write_protect_settings.sh:} >> "$ROOTFS_DIR/etc/rear/rescue.conf"
prep/GNU/Linux/210_include_dhclient.sh:    cat - <<EOF >> "$ROOTFS_DIR/etc/rear/rescue.conf"
prep/DUPLICITY/default/200_find_duply_profile.sh:    echo "DUPLY_PROFILE=$DUPLY_PROFILE" >> "$ROOTFS_DIR/etc/rear/rescue.conf"
prep/DUPLICITY/default/220_define_backup_prog.sh:echo "BACKUP_PROG=$BACKUP_PROG" >> "$ROOTFS_DIR/etc/rear/rescue.conf"

The reason this is now a problem is that I must run the prep stage before the layouts/save stage in the checklayout and savelayout workflows in order to implement auto-discovery of the GALAXY11 configuration (which happens in prep).

As a result, in #2937 I will have to move those scripts mentioned above to the rescue stage.

@rear/contributors I won't be able to test all those BACKUP methods, can you please have a look and try to think what could go wrong?

jsmeix commented at 2023-03-06 11:40:¶

@schlomo

(1)
I do not understand the reason behind what
usr/share/rear/prep/README
states.

I wonder what could go wrong when $ROOTFS_DIR
is accessed already in the prep stage?

ROOTFS_DIR is set in sbin/rear

BUILD_DIR="$( mktemp -d -t rear.XXXXXXXXXXXXXXX || Error "Could not create build area '$BUILD_DIR'" )"
ROOTFS_DIR=$BUILD_DIR/rootfs

so ROOTFS_DIR is a local directory
that should be always accessible.

By the way:
I don't see readonly ROOTFS_DIR and readonly BUILD_DIR
but I don't see why they should not be set readonly?

(2)
I do not understand why you need to move all the above
mentioned scripts to the resuce stage regardless that
they are not for GALAXY11?

Why can't you do only for GALAXY11 what you need only for GALAXY11
in the checklayout and savelayout workflows?

(3)
I cannot actually test what those scripts do
at least not within foreseeable time.

schlomo commented at 2023-03-06 12:06:¶

My goal is to be able to run the prep stage also for the checklayout and savelayout stages so that the code in prep can influence what happens during the layout code. Examples for this are auto-detect configuration that is required for the layout code or ensuring that requirements are met. The GALAXY11 example is auto-discovery of the CommVault Galaxy 11 filesystem paths.

At the moment, adding the prep stage to the layout workflows would modify the filesystem and create some stuff in ROOTFS_DIR. I agree with you that this is probably a harmless side effect, however the usr/share/rear/prep/README reminded me of the original intention of the prep stage and the reason why it exists whatsoever.

I can't just move the GALAXY11 related stuff from prep to rescue because my goal is to run prep before the layout code. I don't want to duplicate code - like the auto-discovery of paths - but I want to fix the root cause, which is that people added code to the wrong stage and we didn't notice it earlier.

Yes, nobody here can test all the ReaR features. We nevertheless need to find a way to move forward even with "bold" changes as otherwise ReaR will get stuck in its own past.

jsmeix commented at 2023-03-06 12:31:¶

Now I see the actual bug here:

'rear mkbackuponly' runs the 'prep' stage and
some 'prep' scripts modify something in ROOTFS_DIR
but only 'mkrescue' and 'mkbackup' and 'mkopalpba'
which call

SourceStage "rescue"
SourceStage "build"
SourceStage "pack"
SourceStage "output"

to actually make a ReaR recovery system
should modify something in ROOTFS_DIR.

But other workflows (i.e. except those that make a ReaR recovery system)
should not modify something in ROOTFS_DIR because
all those modifications are lost (because no ReaR recovery system
with those modifications is made).

So the actual bug here is possible inconsistency,
cf. the probably different but same kind of issue in
https://github.com/rear/rear/issues/2787#issuecomment-1092853451

pcahyna commented at 2023-03-07 16:31:¶

Now I see the actual bug here:

'rear mkbackuponly' runs the 'prep' stage and some 'prep' scripts modify something in ROOTFS_DIR but only 'mkrescue' and 'mkbackup' and 'mkopalpba' which call
SourceStage "rescue"
SourceStage "build"
SourceStage "pack"
SourceStage "output"
to actually make a ReaR recovery system should modify something in ROOTFS_DIR.

But other workflows (i.e. except those that make a ReaR recovery system) should not modify something in ROOTFS_DIR because all those modifications are lost (because no ReaR recovery system with those modifications is made).

So the actual bug here is possible inconsistency, cf. the probably different but same kind of issue in #2787 (comment)

Good that the updated prep/README now explains this reasoning in detail, thank you for updating it!

schlomo commented at 2023-04-04 14:11:¶

@rear/contributors any chance that we can move forward with this problem? It is actually blocking me and prevents implementing proper config check for rear checklayout for GALAXY11. The reason I'm blocked is that I need the prep stage to determine the GALAXY11 configuration (paths primarily) which happens in prep/GALAXY11/default/400_prep_galaxy.sh

My goal is to add the GALAXY_*_DIRECTORY paths to CHECK_CONFIG_FILES in layout/save/GALAXY11/default/400_check_galaxy11_configuration.sh but I cannot do this right now because rear checklayout doesn't run the prep stage.

So I see three options here:

rear checklayout will just run the prep stage, too (regardless of the unwanted side effects this has at the moment)
I add an ugly patch for GALAXY11 similar to this line in the checklayout workflow definition: https://github.com/rear/rear/blob/c5b457395570f283345875a1da14123193fd1657/usr/share/rear/lib/checklayout-workflow.sh#L25
We move forward and work together on making the prep stage truly "hands-off" so that it can be run without harm or side effects

I actually tried option 1) (simply run prep as part of checklayout and it seems to be OK, but of course I can't test this for all configuration scenarios. I'm trying this out in #2965 and maybe that is the easiest way to go forward?

TBH, I still feel like it was a mistake of our past to not be strict about the prep stage only modifying configuration and not including it earlier in the checklayout workflow. The fact that there was a "special hack" for NETFS in the checklayout workflow only illustrates the need for us to fix this problem.

github-actions commented at 2023-06-04 02:57:¶

Stale issue message

github-actions commented at 2023-08-05 02:09:¶