#503 PR `closed`: add custom script support besides PRE_RECOVERY_SCRIPT and PORT_RECOVERY_...¶

Labels: enhancement

tbsky opened issue at 2014-11-04 06:42:¶

hi:
this is a patch for adding custom script support. this feature works like PRE_RECOVERY_SCRIPT and POST_RECOVERY_SCRIPT , but add that ability to other stages by hooking every script at all stages. definition at config file like below:

PRE_BACKUP_97_SCRIPT=(/usr/local/bin/rear-drbd_vm_backup)
POST_RESTORE_98_SCRIPT=(/usr/local/bin/rear-drbd_vm_restore)

the patch is made by suggestion/hint from schlomo:

working at function "Source", not "SourceStage". but I did change one line at fucntion "SourceStage". this can be prevented by add more lines at function "Source", so it is a coding choice. I found function "Source" seems only be called by "SourceStage", but maybe that is just a coincidence. please comment which coding style is preferred.
the format of script name is "PRE_$stage_$priority_SCRIPT" and "POST_$stage_$priority_SCRIPT"
the processing of script is the same as PRE_RECOVERY_SCRIPT and POST_RECOVERY_SCRIPT.

schlomo commented at 2014-11-04 10:45:¶

Hi @tbsky,

thanks - this looks already much better.

I was wondering, maybe we can simplify things by leaving out the numbers? So instead of having a PRE_BACKUP_97_SCRIPT just having a PRE_BACKUP_SCRIPT that runs before any other script from the backup stage? Won't that be enough for your purpose?

That way you could simplify the patch to actually be in SourceStage() only and to generalize what happens now with the POST_RECOVERY_SCRIPT variable (see https://github.com/rear/rear/blob/master/usr/share/rear/wrapup/default/50_post_recovery_script.sh).

We would then add a new Eval() function that would do the same step-by-step and simulate stuff as found in Source() but to actually eval the arguments.

While looking at the code I realized that the following does not work:

POST_RECOVERY_SCRIPT=(date "+%Y %M")
eval "${POST_RECOVERY_SCRIPT[@]}"

(and [*] also does not help either)

I actually really would like to see a solution that would be more robust and allow arbitrary content.

BTW, ReaR uses Bash so bashism are actually favoured over calling external tools. For example:

set -- /foo/76_bar.sh foo
script_file_name=${1##*/}
script_number=${script_file_name:0:2}
script_stage=${2^^}
echo $script_file_name $script_number $script_stage

@gdha, @dagwieers, @jhoekx: What do you think about this feature and how it should be implemented?

tbsky commented at 2014-11-04 12:31:¶

hi schlomo:
when making patches, I like to do minimal changes to make maximum flexibility and maintain compatibility. so in fact, I prefer my first patch, and this second patch is ok for me too. but "PRE_BACKUP_SCRIPT" is not enough for me. since my custom script needs to run after rear mount the backup point, and before rear umount the backup point.

about eval custom scripts, I don't really understand why rear use this design, maybe to maintain consistency for other parameters? since many rear parameters use array content.

I don't quite understand the need to put arbitrary content to script array. to me 'date "+%Y %M' is code and can be put to script file or bash function. so I would do below in config file:

funtion my_function {
date "+%Y %M"
}
POST_RECOVERY_SCRIPT=(my_function)

rear use eval "${POST_RECOVERY_SCRIPT[@]}". so I can still use function in local.conf. (I love that..)
but others who want to separate code and configuration can put the code to script files. I think it is a win-win situation.

about bashism, it is hard for me since I don't know what feature comes with what bash version. so I prevent use advanced bash feature unless necessary. since linux coreutils is a very old tool so I think it is safer to maintain compatibility. I saw codes in rear specifically made for RHEL4, so I try to maintain that compatibility. for example, code 'script_stage=${2^^}' seems only valid for bash 4.x. and RHEL4 only have bash 3.x. I think it is a coding choice. so either way is ok for me.

schlomo commented at 2014-11-04 13:04:¶

Hi @tbsky,

very good point about RHEL3, I checked the Bash 3.2 man page (http://git.savannah.gnu.org/cgit/bash.git/plain/doc/bashref.html?id=28089d04354f1834f109bcb4730c9200234861bc) and it indeed does not support ^^ :-(

I also like your idea about putting complex stuff into a function before calling it as a POST_RECOVERY_SCRIPT. In fact, I like it so much that I would consider to change the documentation (but not the code) to recommend using POST_RECOVERY_SCRIPT (and the others you suggest) as scalars instead to prevent users from assuming that something like ( date "+%y %M" ) would work.

Seeing that I start to see that this patch is a reasonable compromise. Let me ask you a question: What do you think is more complexity: Putting your extra scripts into /usr/share/rear/* to fit into ReaR or to add your suggested code to Source() and SourceStage()?

Another question: Why do we need this PRE_* and POST_* stuff when it basically would be enough to have an INTERNAL_SCRIPT_BACKUP_87= in order to add something at position 87 in the backup stage? I would then add these scripts (with ${!INTERNAL_SCRIPT_BACKUP_*}) to the sorting in SourceStage() and sort real scripts and internal scripts together like this.

I also would like to hear the opinion of the other maintainers.

tbsky commented at 2014-11-04 14:15:¶

hi schlomo:

thanks for your kindly support :) but I don't quite understand your first question. do you mean which way is more complexity when adding custom stuff for me? put custom stuff directly into /usr/share/rear is simple at first, but it is easier to maintain when doing custom stuff at configuration files. and once you figure out how to do it, I think it is much simpler than putting things to /usr/share/rear.

about your second question, indeed we don't need these "PRE" and "POST" stuff, if we can have a script variable name "INTERNAL_SCRIPT_BACKUP_87". I totally support this idea. it is much simpler,direct and easier to understand than "custom function" or "PRE" & "POST" stuff.

schlomo commented at 2014-11-04 15:42:¶

I mean this:

Putting files into /usr/share/rear adds a (IMHO very small) amount of complexity to the deployment and configuration management process.

Extending the ReaR framework to inject internal calls at arbitrary places adds a (IMHO medium) amount of complexity to the framework, since then one has to contend with the fact that the workflow actually changes much more with the configuration as it does at the moment. ATM the output of rear -s will always be the same for the same OUTPUT and BACKUP configuration. With this change that no longer holds true.

My interpretation of what you tell us is that it is more convenient for you to put this extra code into local.conf instead of extra ReaR scriptlets because you actually don't have any deployment and configuration management in place. For other people, who have that in place, it might be actually easier to deploy some extra scripts into the right place within ReaR.

So that is why I ask the question about the complexity. For me it is a decision about where to put this extra complexity: In ReaR as part of the framework or in the deployment/configuration management of the user.

tbsky commented at 2014-11-04 16:50:¶

hi schlomo:

ok. now i understand your question. in my opinion it is often better when you can do more things without sacrificing other features. so if you like to do things as usual (put custom scripts to /usr/share/rear). you can just do it and ignore the patch. if you want to put your code to other places or inside configuration files, you can have your choices with this kind of patch. I can see your view is stand at higher point: if you maintain many rear systems you may want a single way to implement things, so there won't be too much surprise when you met one rear system which is not configured by you. but a few observations:

we already have PRE_RECOVERY_SCRIPT and PORT_RECOVERY_SCRIPT. these two are not absolutely necessary. you can just plug your scripts into /usr/share/rear and take the same affect. so there is already a door opened for end users. with these two parameter you won't have consistent "rear -s" with the same files at /usr/share/rear when recover. if you already open a door, why not open
other windows.
the modular rear design already let user to plugin codes easily. so the difference with or without the patch is just "where" to plugin the codes. in my opinion it seems not a very big issue. in fact if you plugin your custom scripts into /usr/share/rear directly, although you have consistent "rear -s" every time, but it is now harder to find out what is the custom scripts, since under "rear -s" every script name looks like similar. but if you plugin your code via script parameter at local.conf, things are clear.
that's why I prefer put changes into configuration file and take "usr/share/rear" as "official" place. I must again use "Freepbx" design as example, when you "ls -la *custom.conf" at Freepbx system, you immediately figure out the extra custom configuration. you won't forget things you had done or miss the configuration made by other admins.

schlomo commented at 2014-11-04 20:10:¶

Argument 1 was my reason to suggest to generalize that.

Argument 2 tells me that in our mission to make users happy we should leave
the choice to them.

So I will be happy to help you with a suitable patch, I would also be happy
if it would not do the PRE_ / POST_ stuff but more something like
INTERNAL_SCRIPT_$stage_$number_optional_text.

Want to give it another shot? I think that this patch is already very close
to that idea.

Please bear with me, I tend to be very careful about changes to the
framework itself :-)

On 4 November 2014 17:50, tbsky notifications@github.com wrote:

hi schlomo:

ok. now i understand your question. in my opinion it is often better when you can do more things without sacrificing other features. so if you like to do things as usual (put custom scripts to /usr/share/rear). you can just do it and ignore the patch. if you want to put your code to other places or inside configuration files, you can have your choices with this kind of patch. I can see your view is stand at higher point: if you maintain many rear systems you may want a single way to implement things, so there won't be too much surprise when you met one rear system which is not configured by you. but a few observations:

we already have PRE_RECOVER_SCRIPT and PORT_RECOVER_SCRIPT. these two
are not absolutely necessary. you can just plug your scripts into
/usr/share/rear and take the same affect. so there is already a door opened
for end users. with these two parameter you won't have consistent "rear -s"
with the same files at /usr/share/rear when recover. if you already open a
door, why not open
other windows.
2.

the modular rear design already let user to plugin codes easily. so
the difference with or without the patch is just "where" to plugin the
codes. in my opinion it seems not a very big issue. in fact if you plugin
your custom scripts into /usr/share/rear directly, although you have
consistent "rear -s" every time, but it is now harder to find out what is
the custom scripts, since under "rear -s" every script name looks like
similar. but if you plugin your code via script parameter at local.conf,
things are clear.
that's why I prefer put changes into configuration file and take
"usr/share/rear" as "official" place. I must again use "Freepbx" design as
example, when you "ls -la *custom.conf" at Freepbx system, you immediately
figure out the extra custom configuration. you won't forget things you do
or miss the configuration made by other admins.

—
Reply to this email directly or view it on GitHub
https://github.com/rear/rear/pull/503#issuecomment-61672096.

tbsky commented at 2014-11-05 03:06:¶

hi schlomo:
I totally support your idea. I think "INTERNAL_SCRIPT_$stage_$number_optional_text." is better than custom function or PRE/POST stuff. I will try to do another patch with the idea.

I want to make sure what is the best naming convention of the script variable? we have PRE_RECOVERY_SCRIPT now, so to make consistency, the "_SCRIPT" seems at last word. we may have some choices:

INTERNAL_SCRIPT_BACKUP_87_CUSTOM
INTERNAL_SCRIPT_BACKUP_87_myscript
INTERNAL_SCRIPT_BACKUP_87
INTERNAL_BACKUP_87_SCRIPT

other suggestions?

schlomo commented at 2014-11-05 06:43:¶

$ INTERNAL_BACKUP_87_foo_SCRIPT=1
$ echo ${!INTERNAL*SCRIPT}
bash: ${!INTERNAL*SCRIPT}: bad substitution
$ echo ${!INTERNAL*}
INTERNAL_BACKUP_87_foo_SCRIPT

Based on that I would suggest to do INTERNAL_SCRIPT_$stage_$number as the mandatory part so that we can match with the beginning. The variable names could be longer though and numbers can be used more than once. Just like with the shell script snippets.

Correct names would thus be

INTERNAL_SCRIPT_BACKUP_87
INTERNAL_SCRIPT_BACKUP_87_schlomo
INTERNAL_SCRIPT_BACKUP_87_I_dont_know_what_to_put_here
...

Maybe coding it will impose further requirements, like that there always be something after the number or some other constraints.

pavoldomin commented at 2014-11-05 08:01:¶

Hi,

Let me enter this discussion. While I understand the need to insert user code to the rear workflow, the introduction of the new set of the configuration variables does not seem very nice: there is too many configuration variables already. Honestly, it is not nice to read conf/default.conf already now.

Wouldn't it make better sense, if ReaR supports two roots for stage scripts: the "package-shipped" /usr/share/rear/ and the "sysadmin-shipped" (e.g. /etc/rear/share/) and ReaR would merge them when creating the workflow? This way we don't need any new config variable, while we permit admin to have the full control of the ReaR code, without touching the package scripts content.

schlomo commented at 2014-11-05 08:15:¶

Hi @pavoldomin, thanks for entering.

I like your idea as it would greatly simplify the framework compared to the internal script variable solution.

However, @tbsky specifically wanted to keep code in configuration. @tbsky, would this new idea help you also?

BTW, that change could be as simple as adding 2-3 lines in SourceStage()!

tbsky commented at 2014-11-05 08:49:¶

hi schlomo &pavoldomin:

that would be ok for me. although I prefer maintain only 1 configuration file, but that is not really necessary.

I want to mention one thing about this custom script. when I do my first patch, I emulate the behavior of rear official scripts, so when custom script failed, the rear process stopped. when I do my second patch, I emulate the behavior of PRE_RECOVERY_SCRIPT. so when custom script failed, the rear process continue. I think maybe there should be consistent behavior of custom scripts. I think about parameters to control the behavior. but as you guys mentioned, there are already too many parameters :)

schlomo commented at 2014-11-05 09:56:¶

I also think we should make it consistent with the general ReaR framework meaning we should fail if a custom script fails.

To make it simple for users I would like to add a deprecation warning to the PRE_RECOVERY_SCRIPT and POST_RECOVERY_SCRIPT features.

I did not try it out but I would start coding from something like this in Source():

    scripts=(
        $(
        cd $SHARE_DIR/$stage ;
        # We always source scripts in the same subdirectory structure. The {..,..,..} way of writing
        # it is just a shell shortcut that expands as intended.
        ls -d   
            "$CONFIG_DIR/workflow/$stage"/*.sh \
{default,"$ARCH","$OS","$OS_MASTER_VENDOR","$OS_MASTER_VENDOR_ARCH","$OS_MASTER_VENDOR_VERSION","$OS_VENDOR","$OS_VENDOR_ARCH","$OS_VENDOR_VERSION"}/*.sh \
            "$BACKUP"/{default,"$ARCH","$OS","$OS_MASTER_VENDOR","$OS_MASTER_VENDOR_ARCH","$OS_MASTER_VENDOR_VERSION","$OS_VENDOR","$OS_VENDOR_ARCH","$OS_VENDOR_VERSION"}/*.sh \
            "$OUTPUT"/{default,"$ARCH","$OS","$OS_MASTER_VENDOR","$OS_MASTER_VENDOR_ARCH","$OS_MASTER_VENDOR_VERSION","$OS_VENDOR","$OS_VENDOR_ARCH","$OS_VENDOR_VERSION"}/*.sh \
            "$OUTPUT"/"$BACKUP"/{default,"$ARCH","$OS","$OS_MASTER_VENDOR","$OS_MASTER_VENDOR_ARCH","$OS_MASTER_VENDOR_VERSION","$OS_VENDOR","$OS_VENDOR_ARCH","$OS_VENDOR_VERSION"}/*.sh \
        | sed -e 's#/\([0-9][0-9]\)_#/!\1!_#g' | sort -t \! -k 2 | tr -d \!
        )
        # This sed hack is neccessary to sort the scripts by their 2-digit number INSIDE indepentand of the
        # directory depth of the script. Basicall sed inserts a ! before and after the number which makes the
        # number always field nr. 2 when dividing lines into fields by !. The following tr removes the ! to
        # restore the original script name. But now the scripts are already in the correct order.
        )

Maybe that is all that we need to change.

tbsky commented at 2014-11-12 11:09:¶

hi gdha:
because the "Source" function need to know what stage it is in. in can find out that itself, just need more lines of codes, with more codes in "Source" function you don't need to add the parameters.

BTW, I think the patch is abandoned. schlomo would write a better one to fit rear framework.

gdha commented at 2014-11-12 12:07:¶

@schlomo is it true you are working on an improved version of the pre/post_recovery_scripts?

schlomo commented at 2014-11-12 12:12:¶

@gdha Yes and No. As indicated above I would prefer an approach of including "$CONFIG_DIR/workflow/$stage"/*.sh style scripts in the SourceStage() function.

My understanding of this discussion here was that providing another location for admin-supplied script fragments would be an acceptable alternative to creating custom functions.

My personal opinion is that this approach would be much better aligned with how the ReaR framework works and thinks internally and it would avoid the extra complexity of mixing functions and scriptles.

I was actually hoping that @tbsky would try out this approach and I can commit to help polishing a patch that he would submit along this idea.

tbsky commented at 2014-11-12 13:32:¶

hi:
my real interest was using local.conf to manage all the custom stuff. but as I said that's is not absolutely necessary. so now I use rpm to deploy the custom scripts skeleton as schlomo suggested before. and these skeletons would call real custom function defined in my local.conf.

as schlomo said, the error handling of custom scripts should be the same as official script, and current PRE_RECOVERY_SCRIPT is not acting that way, mabye that function will be depreciated.
so I think the whole design/decision is better made by core developers.

gdha commented at 2014-11-18 19:10:¶

@schlomo will you try to add this? I would like to concentrate on some other parts (like btrfs and systemd/f21)

schlomo commented at 2014-11-19 11:08:¶

@gdha I'll stay on this topic. It for sure falls into my main interest area

gdha commented at 2015-01-25 15:52:¶

Don't we better post-pone this to next release rear-1.18?

schlomo commented at 2015-01-25 18:09:¶

Well, why not simply add "$CONFIG_DIR/workflow/$stage"/*.sh as described above and add a deprecation warning to the POST_* and PRE_* script variables?

@gdha, @dagwieers, @jhoekx please tell me what you think about this approach?

gdha commented at 2015-01-25 19:37:¶

@schlomo I don't mind as long everybody agrees on the way forward. So +1

pavoldomin commented at 2015-01-27 09:01:¶

Hi, looks fine to me. But, if I get it right, the suggestion:

   scripts=(
   ...
        ls -d   
            "$CONFIG_DIR/workflow/$stage"/*.sh \
        ...

means, we add our custom scripts to the ones shipped by ReaR. It is not exactly what @tbsky originally aimed for I think: we call our workflow scripts added to the ReaR ones, but would be more flexible if we replace the ReaR script with our one, if the script with same name exists in ReaR's own workflow path.

But perhaps I speculate too much here.

schlomo commented at 2015-01-27 11:28:¶

I see what you mean.

Do we now have an actual requirement to overlay ReaR scripts?

@tbsky, can you please let us know if adding scripts would be enough for
your current need.

IMHO adding the option to overlay or disable ReaR scripts is a very
dangerous feature as many scripts depend on each other. If users need to be
able to disable some feature or behaviour then we should include variables
for that.

On 27 January 2015 at 10:01, pavoldomin notifications@github.com wrote:

Hi, looks fine to me. But, if I get it right, the suggestion:

scripts=(
...
ls -d
"$CONFIG_DIR/workflow/$stage"/*.sh
...

means, we add our custom scripts to the ones shipped by ReaR. It is not
exactly what @tbsky https://github.com/tbsky originally aimed for I
think: we call our workflow scripts added to the ReaR ones, but would
be more flexible if we replace the ReaR script with our one, if the
script with same name exists in ReaR's own workflow path.

But perhaps I speculate too much here.

—
Reply to this email directly or view it on GitHub
https://github.com/rear/rear/pull/503#issuecomment-71611976.

tbsky commented at 2015-01-27 12:45:¶

@schlomo

I think it is good to separate rear official scripts and custom scripts. for my need, I already use rpm to deploy custom scripts to call my custom functions, so I can manage all the configurations in one config file. that's enough for me.

schlomo commented at 2015-01-27 13:43:¶

@tbsky, does that mean that we can close this issue and do nothing?

Or would it help you if I implement the addition I suggested above?

On 27 January 2015 at 13:45, tbsky notifications@github.com wrote:

@schlomo https://github.com/schlomo

I think it is good to separate rear official scripts and custom scripts. for me need, I already use rpm to deploy custom scripts to call my custom functions, so I can manage all the configurations in one config file. that's enough for me.

—
Reply to this email directly or view it on GitHub
https://github.com/rear/rear/pull/503#issuecomment-71642287.

tbsky commented at 2015-01-28 02:09:¶

@schlomo

yes we can close it since it is not really a problem, just an enhancement. but as you said, I think implement a infrastructure for user-defined custom scripts is a good thing.

gdha commented at 2015-11-24 17:28:¶

@schlomo Shall we silently close this pull request as there is no general demand for it. Rear is more then complex enough.

schlomo commented at 2015-11-24 18:47:¶

@gdha yes, in favour. I prefer users to plug scripts into the ReaR tree and I would ask them to accept that this is how we want them to extend ReaR.

jsmeix commented at 2015-11-25 09:52:¶

A note only FYI:

I appreciate everything that reduces complexity in Rear.

Keep It Simple and Straightforward cf. RFC 1925 (6a)
and
Keep Separated Issues Separated cf. RFC 1925 (5)

[Export of Github issue for rear/rear.]

#503 PR closed: add custom script support besides PRE_RECOVERY_SCRIPT and PORT_RECOVERY_...¶

tbsky opened issue at 2014-11-04 06:42:¶

schlomo commented at 2014-11-04 10:45:¶

tbsky commented at 2014-11-04 12:31:¶

schlomo commented at 2014-11-04 13:04:¶

tbsky commented at 2014-11-04 14:15:¶

schlomo commented at 2014-11-04 15:42:¶

tbsky commented at 2014-11-04 16:50:¶

schlomo commented at 2014-11-04 20:10:¶

tbsky commented at 2014-11-05 03:06:¶

schlomo commented at 2014-11-05 06:43:¶

pavoldomin commented at 2014-11-05 08:01:¶

schlomo commented at 2014-11-05 08:15:¶

tbsky commented at 2014-11-05 08:49:¶

schlomo commented at 2014-11-05 09:56:¶

tbsky commented at 2014-11-12 11:09:¶

gdha commented at 2014-11-12 12:07:¶

schlomo commented at 2014-11-12 12:12:¶

tbsky commented at 2014-11-12 13:32:¶

gdha commented at 2014-11-18 19:10:¶

schlomo commented at 2014-11-19 11:08:¶

gdha commented at 2015-01-25 15:52:¶

schlomo commented at 2015-01-25 18:09:¶

gdha commented at 2015-01-25 19:37:¶

pavoldomin commented at 2015-01-27 09:01:¶

schlomo commented at 2015-01-27 11:28:¶

tbsky commented at 2015-01-27 12:45:¶

schlomo commented at 2015-01-27 13:43:¶

tbsky commented at 2015-01-28 02:09:¶

gdha commented at 2015-11-24 17:28:¶

schlomo commented at 2015-11-24 18:47:¶

jsmeix commented at 2015-11-25 09:52:¶

#503 PR `closed`: add custom script support besides PRE_RECOVERY_SCRIPT and PORT_RECOVERY_...¶