#2937 PR merged: Add support for Commvault Galaxy 11

Labels: enhancement, fixed / solved / done, sponsored, Dedicated Priority Support

schlomo opened issue at 2023-02-17 09:14:

Relax-and-Recover (ReaR) Pull Request Template

Please fill in the following items before submitting a new pull request:

Pull Request Details:

  • Type: Enhancement

  • Impact: Normal

  • Reference to related issue (URL):

#2918

  • How was this pull request tested?

Manual tests with Comvault 11

  • Brief description of the changes in this pull request:

Clone GALAXY10 integration and adjust for Commvault Galaxy 11

I'm creating this PR for visibility and will continue to work on it, no need to review it while in DRAFT state

schlomo commented at 2023-02-17 14:40:

Thanks a lot @jsmeix for your review, I still have to go over the code and will take all of your remarks into consideration.

jsmeix commented at 2023-02-17 14:45:

@schlomo
I had only a quick look over the code and
added offhanded comments without much thinking
where I spotted something.

Now it's weekend time for me.

I wish you all a relaxed and recovering weekend!

schlomo commented at 2023-02-17 14:53:

Just an observation:

I think that most of the developers who worked on this code before didn't realise that during rescue system startup we also read the ReaR configuration in
https://github.com/rear/rear/blob/ea900c4ec9589c7acb3d90c01f75ad7781fbdd8c/usr/share/rear/skel/default/etc/scripts/system-setup#L69-L77

which makes it super simple to tweak the rescue startup for specific configurations, e.g. mounting a tmpfs for backup software that wants to confirm some free disk space.

Also, I think modifying the PATH should be done on a system level and not per-ReaR-run level.

I'll see what can be done with that.

jsmeix commented at 2023-02-21 10:48:

In general regarding new so called "boolean" config variables
i.e. config variables where any non-empty value means 'true', cf.
https://github.com/rear/rear/blob/master/usr/share/rear/conf/default.conf#L31

In this case here it is USE_RAMDISK:

I suggest to avoid new so called "boolean" config variables
because such config variables evaluate to 'true' if set to 'false',
e.g. in this case

USE_RAMDISK="no"

will
"Configuring Rescue system with default ramdisk size"
in the current
rescue/GNU/Linux/295_configure_ramdisk_rootfs.sh
https://github.com/rear/rear/pull/2937/files/8d873cdca24d87e65c085dedfe9c1a8c2cdb4f04..77567f37567b947ab1797018868d934799b20c45#diff-c7ff10e36d2be7dc506e70939111fc5ff53cdcd4f4bf978ff6aeab5430c32eeb

For new config variables I recommend explicit coding
by using our is_true and is_false functions, see
lib/global-functions.sh how to use them
https://github.com/rear/rear/blob/rear-2.7/usr/share/rear/lib/global-functions.sh#L95

Then default.conf could be like

##
# USE_RAMDISK
#
# Configure rescue system to create and pivot to a ramdisk instead of initramfs.
#
# This is required by some backup software that wants to check for actually available disk space.
# Examples are: GALAXY11
#
# Set to 'true' (or any value that is recognized as 'yes' by the is_true function)
# to use a ramdisk with default size (usually 50% of physical RAM, see 'size' in "man tmpfs").
# Set to a number to use a ramdisk with available free disk space in MiB.
# This will probably crash if you don't have enough physical memory
# for the ReaR recovery system (minimum about 250 MiB up to more than 1 GiB)
# plus the specified available free disk space, see KEEP_BUILD_DIR how to check
# the ReaR recovery system content in $TMPDIR/rear.XXXXXXXXXXXXXXX/rootfs/
USE_RAMDISK="no"

and
rescue/GNU/Linux/295_configure_ramdisk_rootfs.sh
could be like

# Prepare rescue system to switch_root to a ramdisk:

is_false "$USE_RAMDISK" && return

if is_positive_integer "$USE_RAMDISK" ; then
    DebugPrint "Configuring Rescue system with ramdisk and free disk space of $USE_RAMDISK MiB"
    echo $USE_RAMDISK >$ROOTFS_DIR/etc/ramdisk-free-space
else
    is_true "$USE_RAMDISK" || return 0
    DebugPrint "Configuring Rescue system with default ramdisk size (usually 50% of physical RAM)"
fi

REQUIRED_PROGS+=(switch_root du)
KERNEL_CMDLINE+=" rdinit=/etc/scripts/ramdisk-rootfs"

I also exchanged by the way "MB" by "MiB"
according to what "man tmpfs" tells about 'size'.

FYI
regarding usual ReaR recovery system size values, see
https://github.com/rear/rear/discussions/2640#discussioncomment-908335

jsmeix commented at 2023-02-21 11:06:

@schlomo
thank you for the USE_RAMDISK functionality!

I did not know that initramfs behaves fundamentally different
compared to hwo a normal "disk thingy" behaves.

I am reading now things like
https://stackoverflow.com/questions/10603104/the-difference-between-initrd-and-initramfs

Ugh!
Why has all that computing stuff become so complicated :-(
I want my home computer back - the one from 40 years ago ;-)

schlomo commented at 2023-02-21 12:22:

@jsmeix I had similar thoughts and was also considering introducing 2 variables:

  1. USE_RAMDISK as true boolean
  2. RAMDISK_FREE_SPACE for the free space

Or maybe we should use a numeric value and only use a RESCUE_RAMDISK_FREE_SPACE which defaults to 0 to indicate not to do anything and can be set to a number to indicate that we need a ramdisk with X MiB free space.

I'm also really not sure where in the long default.conf to add this new variable, any suggestions about the name or place are most welcome.

jsmeix commented at 2023-02-21 12:32:

Regarding the place in default.conf
I think before or after the REAR_INITRD_COMPRESSION part
looks reasonable because ramdisk is related to initrd.

jsmeix commented at 2023-02-21 12:54:

Regarding two variables for "one thing":

I introduced several variables with "ternary semantics", cf.
https://github.com/rear/rear/blob/rear-2.7/usr/share/rear/conf/default.conf#L35
and personally I have only good experience with it.

It makes all simpler in practice because
there is only one thing to deal with
so the code is simpler (at least this is how I experienced it)
and it is simpler for the user to specify what he wants.

From a theoretical point of view "ternary semantics"
mixes boolean type with data (integer/string/array) type
but because there is no real boolean type in bash
this does not matter - as long as the boolean values
'True T true t Yes Y yes y 1' and 'False F false f No N no n 0'
cannot appear as data values so one can reliably distinguish
the three cases with 'is_true', 'is-false', and "something else".

E.g. if USE_RAMDISK would be the size in GiB then

USE_RAMDISK=1

to specify 1 GiB ramdisk size (or free space)
could conflict with is_true "$USE_RAMDISK".

Spontaneously I like
"only use a RESCUE_RAMDISK_FREE_SPACE which defaults to 0"
most.

In particular because the variable name tells what it does
and even is_false "$RESCUE_RAMDISK_FREE_SPACE" can be used
as a generic test when the user does not want a ramdisk
i.e. the user can also specify RESCUE_RAMDISK_FREE_SPACE="no"
when he does not want a ramdisk so the code could be like

# Prepare rescue system to switch_root to a ramdisk:

is_false "$RESCUE_RAMDISK_FREE_SPACE" && return

is_positive_integer "$RESCUE_RAMDISK_FREE_SPACE" || Error "RESCUE_RAMDISK_FREE_SPACE='$RESCUE_RAMDISK_FREE_SPACE' is not a positive integer"

DebugPrint "Configuring Rescue system with ramdisk and free disk space of $RESCUE_RAMDISK_FREE_SPACE MiB"
echo $RESCUE_RAMDISK_FREE_SPACE >$ROOTFS_DIR/etc/ramdisk-free-space

REQUIRED_PROGS+=(switch_root du)
KERNEL_CMDLINE+=" rdinit=/etc/scripts/ramdisk-rootfs"

schlomo commented at 2023-02-21 13:06:

Sounds good, however I'd like to keep the size in MiB and not GiB

jsmeix commented at 2023-02-21 13:35:

@schlomo
I think you misunderstood.
The size should be in MiB because if it was in GiB then
a conflict could appear what the value '1' means.
In this particular case it would be even OK if
the value '1' would mean both 'true' and '1 GiB'
but at least this would look "hairy".

Furthermore any non-integer numbers call for problems
and strings are the worst of all (because all what
hardware can actually do is computing integers).

jsmeix commented at 2023-02-22 07:31:

@schlomo
I like your last ramdisk implementation via
https://github.com/rear/rear/pull/2937/files/549cb99adb2566725310fbcfab6ec74c061fbde1..15b763af73e7ade480c6e226062bdb59efad0e01
very much because it keeps the simple boolean
USE_RAMDISK="yes" vs. USE_RAMDISK="no" functionality
where USE_RAMDISK="yes" creates a ramdisk with default size
which helps the user if he cannot decide what the right size is
and 50% of of physical RAM by default looks reasonable
on nowadays systems with usually 4 GiB RAM and more because
for me only 2 GiB RAM "just work" on my VMs for testing ReaR
but normally I use a small ReaR recovery system of about 200 MiB, cf.
https://github.com/rear/rear/discussions/2640#discussioncomment-908335

schlomo commented at 2023-02-22 07:35:

@rear/contributors can you imagine any reason why we shouldn't be able to remove support for older CommVault versions?

According to https://ma.commvault.com/Support/ProductEOLMessages currently only GALAXY11 is actually supported by CommVault and users with a legacy CommVault version probably also use a legacy Linux distro and can also keep using their legacy ReaR version
image

Personally, I really would like to reduce the amount of untestable and unmaintainable code in ReaR

jsmeix commented at 2023-02-22 10:25:

OK - let's be venturous here
and disable support for older CommVault versions.

Note the different wording "remove" vs. "disable".

With "disable" I mean something like a new
usr/share/rear/init/default/500_disable_deprecated.sh
script that errors out for deprecated functionality
so users can if needed modify that script
to still use deprecated functionality
until we actually remove the code.

For example
(only an offhanded quick initial attempt):

# Error out for deprecated functionality.
# If really needed users may skip this:
#return 0

local deprecated_error=''

# As of this writing on Wed Feb 22 2023 according to
# https://ma.commvault.com/Support/ProductEOLMessages
# currently only GALAXY11 is actually supported by CommVault:
for deprecated_backup_galaxy in GALAXY GALAXY7 GALAXY10 ; do
    if test "$BACKUP" = $deprecated_backup_galaxy ; then
        LogPrintError "BACKUP=$deprecated_backup_galaxy is deprecated functionality"
        deprecated_error="yes"
    fi
done

is_true $deprecated_error || return 0
Error "Deprecated functionality is no longer supported"

Instead of using a local deprecated_error variable
we may introduce a new DEPRECATED_ERROR user confing variable
but then users may too easily set it to "no" without telling us
that they really need this or that deprecated functionality?

schlomo commented at 2023-02-22 10:38:

I'm actually against this approach @jsmeix because it won't remove the code from ReaR and that is my objective here: Getting rid of untested and unmaintainable code.

What is bad with pointing users of old CommVault to either use old ReaR or sponsor/contribute proper support in new ReaR? After all, it won't be difficult to bring the scripts back from an older version, if needed. Or somebody could extract them into an add-on that can be installed on top of ReaR (ReaR was always meant to be extensible via dropping more files into it), which would allow the truly interested parties to maintain legacy CommVault support outside of our upstream development.

I'm really talking about upstream ReaR here, whatever we do won't affect anybody with ReaR 2.7 installed.

schlomo commented at 2023-02-22 13:10:

I'll add the decrepation feature here as well, no problems. Good discussion and arguments, especially from #2944

schlomo commented at 2023-02-27 18:54:

Thanks @jsmeix for your thoughts. Here is the new deprecation feature:

deprecation error

[root@rear-ol8u7 rear]# rear -C g -v mkrescue
Relax-and-Recover 2.7 / Git
Running rear mkrescue (PID 61473 date 2023-02-27 19:27:45)
Using log file: /var/log/rear/rear-rear-ol8u7.log
Sourcing additional configuration file '/etc/rear/g.conf'
Running workflow mkrescue on the normal/original system
ERROR: Deprecation of >galaxy10<
Reason: CommVault Galaxy 10 is EOL since 2018 according to https://ma.commvault.com/Support/ProductEOLMessages

This feature or code path has been deprecated in ReaR and will
be removed eventually. If you disagree with that, then please
go to https://github.com/rear/rear/issues and create an issue
explaining for us why we should not deprecate this code path.

Meanwhile, in order to continue using this feature, you can add
DISABLE_DEPRECATION_ERRORS+=( galaxy10 )
to your ReaR configuration to disable this error.
Use debug mode '-d' for some debug messages or debugscript mode '-D' for full debug messages with 'set -x' output
Aborting due to an error, check /var/log/rear/rear-rear-ol8u7.log for details
Exiting rear mkrescue (PID 61473) and its descendant processes ...
Running exit tasks
Terminated

deprecation override

[root@rear-ol8u7 rear]# rear -C g -v mkrescue
Relax-and-Recover 2.7 / Git
Running rear mkrescue (PID 52094 date 2023-02-27 19:27:13)
Using log file: /var/log/rear/rear-rear-ol8u7.log
Sourcing additional configuration file '/etc/rear/g.conf'
Running workflow mkrescue on the normal/original system
Disabled deprecation error for >galaxy10<
Using autodetected kernel '/boot/vmlinuz-5.15.0-7.86.6.1.el8uek.x86_64' as kernel in the recovery system
...

rear dump also shows it, and -C extra configs too

[root@rear-ol8u7 rear]# rear -C g dump
# Begin dumping out configuration and system information:
# This is a 'Linux-x86_64' system, compatible with 'Linux-i386'.
# Configuration tree:
  # Linux-i386.conf : OK
  # GNU/Linux.conf : OK
  # Fedora.conf : missing/empty
  # Fedora/i386.conf : missing/empty
  # Fedora/8.7.conf : missing/empty
  # Fedora/8.7/i386.conf : missing/empty
  # RedHatEnterpriseServer.conf : missing/empty
  # RedHatEnterpriseServer/i386.conf : missing/empty
  # RedHatEnterpriseServer/8.7.conf : missing/empty
  # RedHatEnterpriseServer/8.7/i386.conf : missing/empty
  # site.conf : missing/empty
  # local.conf : OK
  # /etc/rear/g.conf : OK
# System definition:
  ARCH="Linux-i386"
  OS="GNU/Linux"
  OS_MASTER_VENDOR="Fedora"
  OS_MASTER_VERSION="8.7"
  OS_MASTER_VENDOR_ARCH="Fedora/i386"
  OS_MASTER_VENDOR_VERSION="Fedora/8.7"
  OS_MASTER_VENDOR_VERSION_ARCH="Fedora/8.7/i386"
  OS_VENDOR="RedHatEnterpriseServer"
  OS_VERSION="8.7"
  OS_VENDOR_ARCH="RedHatEnterpriseServer/i386"
  OS_VENDOR_VERSION="RedHatEnterpriseServer/8.7"
  OS_VENDOR_VERSION_ARCH="RedHatEnterpriseServer/8.7/i386"
  DISABLE_DEPRECATION_ERRORS=("galaxy10")
# Backup with GALAXY10:
  GALAXY10_BACKUPSET=""
  GALAXY10_Q_ARGUMENTFILE=""
...

jsmeix commented at 2023-02-28 10:07:

@schlomo
I like the new deprecation error functionality very much!

schlomo commented at 2023-03-19 12:10:

I updated this PR and would like to merge it now, happy to fix any small stuff you see but bigger things will have to go into a follow-up PR as this round of on-site testing is over.

schlomo commented at 2023-03-19 12:13:

FYI, some of the changes (especially around copy-as-is logic) were required to properly test ReaR without installing it each time.

[Export of Github issue for rear/rear.]