Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Commvault Galaxy 11 #2937

Merged
merged 25 commits into from Mar 21, 2023
Merged

Add support for Commvault Galaxy 11 #2937

merged 25 commits into from Mar 21, 2023

Conversation

schlomo
Copy link
Member

@schlomo schlomo commented Feb 17, 2023

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 schlomo self-assigned this Feb 17, 2023
@schlomo
Copy link
Member Author

schlomo commented Feb 17, 2023

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
Copy link
Member

jsmeix commented Feb 17, 2023

@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
Copy link
Member Author

schlomo commented Feb 17, 2023

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

# Sourcing of the conf/default.conf file as we may use some defined variables or arrays
# E.g. UDEV_NET_MAC_RULE_FILES is used by script 55-migrate-network-devices.sh
test -f /usr/share/rear/conf/default.conf && source /usr/share/rear/conf/default.conf
# Sourcing user and rescue configuration as we need some variables
# (EXCLUDE_MD5SUM_VERIFICATION right now and other variables in the system setup scripts):
# The order of sourcing should be 'site' then 'local' and as last 'rescue'
for conf in site local rescue ; do
test -f /etc/rear/$conf.conf && source /etc/rear/$conf.conf
done

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.

@schlomo schlomo added this to the ReaR v2.8 milestone Feb 21, 2023
@schlomo schlomo added sponsored This issue will get solved on a paid base by ReaR upstream. Dedicated Priority Support labels Feb 21, 2023
@jsmeix
Copy link
Member

jsmeix commented Feb 21, 2023

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
#2640 (comment)

@jsmeix
Copy link
Member

jsmeix commented Feb 21, 2023

@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
Copy link
Member Author

schlomo commented Feb 21, 2023

@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
Copy link
Member

jsmeix commented Feb 21, 2023

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
Copy link
Member

jsmeix commented Feb 21, 2023

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
Copy link
Member Author

schlomo commented Feb 21, 2023

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

@jsmeix
Copy link
Member

jsmeix commented Feb 21, 2023

@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).

Sascha Lucas and others added 24 commits March 21, 2023 17:55
…r tools that need to check for free disk space.

rework GALAXY11 to use ramdisk feature and set PATH globally
@schlomo schlomo merged commit 6987252 into master Mar 21, 2023
@schlomo schlomo deleted the galaxy11 branch March 21, 2023 17:05
@jsmeix jsmeix added the enhancement Adaptions and new features label Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dedicated Priority Support enhancement Adaptions and new features fixed / solved / done sponsored This issue will get solved on a paid base by ReaR upstream.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants