Fix for issue 2200: support new use-case for BLOCKCLONE backup method… #2201

petroniusniger · 2019-08-05T11:34:12Z

… by attempting to unmount device before backup/restore.

Type:Enhancement
Impact: Normal
Reference to related issue (URL): New use-case for BLOCKCLONE backup method #2200
How was this pull request tested?
- test VM installed with Leap 15.0 (grub2-efi, LVM, filesystems are ext4 and xfs)
- created new, LUKS-encrypted logical volume with xfs mounted on /products
- stored an extra decryption passphrase into slot Pull fixes for partitioning and udev. #1 of LUKS header (main key in slot #0)
- created a "multiple backup" configuration where the base system is backed up the usual way (ISO + NETFS to NFS share), ignoring /products, while the block device that hosts /products is then backed up (mkbackuponly) using the BLOCKCLONE method. See configuration files below:
  site.conf.txt
  base_system.conf.txt
  products_backup.conf.txt
- the new option BLOCKCLONE_TRY_UNMOUNT is set to "yes"
- during /products backup phase (mkbackuponly), the filesystem is now successfully unmounted prior to issuing the dd command
- at the end of the backup phase, that filesystem is also successfully re-mounted
- the test VM is then shut down and its virtual HDD is deleted and recreated empty (same size, in this case)
- during base system recover phase, the encrypted filesystem is correctly recreated (the user is prompted to enter a new passphrase -- which will later be overwritten). No data is restored on that filesystem as we ignored it through BACKUP_PROG_EXCLUDE
- during /products restore phase (restoreonly) the target device (/dev/vg00/lvol4) is successfully unmounted prior to issuing the dd command, which overwrites the recreated encrypted filesystem with the original LUKS keys and all the filesystem data
- upon reboot of the VM, all filesystems are clean, /products can be decrypted using any of the 2 original passphrases and can be successfully mounted
Brief description of the changes in this pull request:
- new configuration variable BLOCKCLONE_TRY_UNMOUNT in default.conf, including short documentation
- 500_start_clone.sh now use existing global function umount_mountpoint() to try and unmount the source device if BLOCKCLONE_TRY_UNMOUNT is set to "yes". It obtains the mount point through a new global function called get_mountpoint()
- if the unmounting fails, 500_start_clone.sh proceeds as before, issuing a warning message that the backup has been taken while the source device was mounted
- if the device was initially mounted AND it could be unmounted, then 500_start_clone.sh attempts to re-mounted it before exiting
- 400_restore_clone.sh now implements the same checks and unmount attempt as 500_start_clone.sh. It does NOT try to remount the target device at the end, but issues a warning message instead
- bug fix in global function umount_mountpoint(): return value now visible by calling script
- cosmetics in global function is_device_mounted(): local variables now defined as local
- new global function get_mountpoint()
- extensive documentation of the new BLOCKCLONE use-case in the User Guide, chapter 12
- fixed typo in User Guide, chapter 11, "multiple backups"

… by attempting to unmount device before backup/restore.

jsmeix · 2019-08-07T11:53:10Z

usr/share/rear/lib/global-functions.sh

@@ -622,7 +622,8 @@ umount_mountpoint() {
    Log "Unmounting '$mountpoint'"
    umount $v $mountpoint >&2
    if [[ $? -eq 0 ]] ; then
-        return 0
+        echo 0
+	return


From plain looking at the code here I think in general
all 'is_something' testing functions
should not provide the result as a string on stdout
but as usual via the exit code return value so that a testing function
could be used as usual by calling

is_something && ... is_something || ... if is_something ... if ! is_something ...

@jsmeix I think that this should become subject of separate PR, to avoid mixing topics ...

jsmeix · 2019-08-07T11:56:04Z

@gozora
could you have a look here?

FYI:
I am not in the office for some weeks so that I cannot do much for ReaR,
in particular I cannot try out something.

gozora · 2019-08-07T16:55:12Z

I'll take a look as time permits (most probably during this weekend) ...

V.

gozora · 2019-08-11T10:21:02Z

usr/share/rear/lib/global-functions.sh

@@ -622,7 +622,8 @@ umount_mountpoint() {
    Log "Unmounting '$mountpoint'"
    umount $v $mountpoint >&2
    if [[ $? -eq 0 ]] ; then
-        return 0
+        echo 0
+	return


@jsmeix I think that this should become subject of separate PR, to avoid mixing topics ...

gozora · 2019-08-11T10:25:40Z

usr/share/rear/lib/global-functions.sh

@@ -632,11 +633,13 @@ umount_mountpoint() {
    Log "Forced unmount of '$mountpoint'"
    umount $v -f $mountpoint >&2
    if [[ $? -eq 0 ]] ; then
-        return 0
+        echo 0


Are you sure that this will not cause trouble elsewhere ?
umount_mountpoint() is called by umount_url() which is quite heavily used across Rear:

usr/share/rear/restore/YUM/default/980_umount_YUM_dir.sh:9:umount_url $BACKUP_URL $BUILD_DIR/outputfs usr/share/rear/restore/DUPLICITY/default/980_unmount_duplicity_path.sh:8: umount_url $BACKUP_DUPLICITY_NETFS_URL $BUILD_DIR/outputfs usr/share/rear/verify/YUM/default/980_umount_YUM_dir.sh:8:umount_url $BACKUP_URL $BUILD_DIR/outputfs usr/share/rear/verify/DUPLICITY/default/980_unmount_duplicity_path.sh:8: umount_url $BACKUP_DUPLICITY_NETFS_URL $BUILD_DIR/outputfs usr/share/rear/output/default/980_umount_output_dir.sh:11:umount_url $OUTPUT_URL $BUILD_DIR/outputfs usr/share/rear/output/PXE/default/800_copy_to_tftp.sh:72: umount_url $PXE_TFTP_URL $BUILD_DIR/tftpbootfs usr/share/rear/output/PXE/default/810_create_pxelinux_cfg.sh:107: umount_url $PXE_TFTP_URL $BUILD_DIR/tftpbootfs usr/share/rear/lib/global-functions.sh:570:umount_url() { usr/share/rear/backup/BORG/default/900_umount_usb.sh:9: umount_url usb://$USB_DEVICE $borg_dst_dev usr/share/rear/backup/DUPLICITY/default/980_unmount_duplicity_path.sh:8: umount_url $BACKUP_DUPLICITY_NETFS_URL $BUILD_DIR/outputfs usr/share/rear/backup/NETFS/default/980_umount_NETFS_dir.sh:7:umount_url $BACKUP_URL $BUILD_DIR/outputfs

Hello @gozora
You're right, of course. My apologies for missing this. I looked for calls to umount_mountpoint() throughout the scripts, but missed the fact that it could also be called by other functions.
I'll revert this change and adapt my own code accordingly.

gozora · 2019-08-11T10:33:38Z

usr/share/rear/backup/BLOCKCLONE/default/500_start_clone.sh

+# try to remount it before leaving
+if [ "$umount_res" = "0" ]; then
+    LogPrint "Trying to remount $mp"
+    mount $v $mp


This will work only if $mp is listed in /etc/fstab. If you really want to mount it back, you should record device, mount point and options prior unmounting and use there date here. Something like:
mount $v -o $orig_opt $orig_dev $orig_mpt

Yes, I was aware of the fact that it would only work for mountpoints defined in /etc/fstab. I took the view that anything not present there would be transient anyway and that silently failing to remount it was acceptable behaviour -- if far from ideal.
But the approach you suggest is better by far, so I'll try to implement that ;-)

Re-align with trunk/HEAD prior to re-test with updated scripts post review.

… by attempting to unmount device before backup/restore. Now takes review comments into consideration.

petroniusniger · 2019-08-22T08:44:02Z

Updated code to fulfil both suggestions by @gozora.

Short comment about fixes in is_device_mounted() function:

local variables now defined as local (was already the case in my first commit)
fixed an 'inverse logic' bug in the test for empty parameter: not mounted or not found should return 0, not 1
added missing early exit to the test for empty parameter -- passing an empty argument to the function might have resulted in an error similar to the following:

./fn_is_mounted: line 10: [: [SWAP]: binary operator expected

All of this does nothing to address the (valid) remark of @jsmeix about the behaviour of is_something() functions, but as I call is_device_mounted() from the code I contributed, I thought it fair game to fix these bugs at this time.

Regarding testing:

I've tested extensively all three functions I modified or added by taking them out of ReaR and by feeding them different values to trigger a positive or negative result, including empty strings
I've again performed a full "2-steps backups + 2-steps recover" scenario on a VM with a LUKS-encrypted filesystem. Everything behaved as expected.

Branch issue-2200 has been re-based against trunk/HEAD before commit/push.

gozora · 2019-08-26T07:16:47Z

usr/share/rear/backup/BLOCKCLONE/default/500_start_clone.sh

+        # save mount parameters for later
+	local mount_cmd=$(build_remount_cmd $mp)
+	umount_mountpoint $mp
+	umount_res=$?


Please use correct indentation of previous 3 lines.

gozora · 2019-08-26T08:08:57Z

usr/share/rear/lib/global-functions.sh

-   disk=$1
-   [ -z "$disk" ] && echo 1
+   local disk=$1
+   [ -z "$disk" ] && echo 0 && return


Thanks for fixing this! ;-)

gozora · 2019-08-26T08:39:14Z

usr/share/rear/restore/BLOCKCLONE/default/400_restore_clone.sh

+    # try unmount
+    if [ ! -z "$mp" ]; then
+        umount_mountpoint $mp
+	umount_res=$?


Please use correct indentation.

gozora · 2019-08-26T08:47:45Z

@petroniusniger thanks for updated PR.
Please fix those couple indentation problems, and I'll merge this PR.

Thanks!

V.

… by attempting to unmount device before backup/restore. Use correct indentation.

gozora · 2019-09-02T17:26:12Z

@petroniusniger many thanks for contributing this new functionality and respective documentation to ReaR!

V.

Fix for issue 2200: support new use-case for BLOCKCLONE backup method…

0a162a3

… by attempting to unmount device before backup/restore.

jsmeix requested a review from gozora August 7, 2019 11:42

jsmeix assigned gozora Aug 7, 2019

jsmeix added the enhancement Adaptions and new features label Aug 7, 2019

jsmeix added this to the ReaR v2.6 milestone Aug 7, 2019

jsmeix reviewed Aug 7, 2019

View reviewed changes

gozora requested changes Aug 11, 2019

View reviewed changes

petroniusniger added 2 commits August 21, 2019 15:01

Merge branch 'master' of https://github.com/rear/rear into issue-2200

9ab82d7

Re-align with trunk/HEAD prior to re-test with updated scripts post review.

Fix for issue 2200: support new use-case for BLOCKCLONE backup method…

eb9d4ac

… by attempting to unmount device before backup/restore. Now takes review comments into consideration.

gozora reviewed Aug 26, 2019

View reviewed changes

gozora requested changes Aug 26, 2019

View reviewed changes

Fix for issue 2200: support new use-case for BLOCKCLONE backup method…

1f6e9ed

… by attempting to unmount device before backup/restore. Use correct indentation.

gozora approved these changes Sep 2, 2019

View reviewed changes

gozora merged commit d0e405c into rear:master Sep 2, 2019

jsmeix added the fixed / solved / done label Sep 3, 2019

jsmeix mentioned this pull request Sep 3, 2019

New use-case for BLOCKCLONE backup method #2200

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for issue 2200: support new use-case for BLOCKCLONE backup method… #2201

Fix for issue 2200: support new use-case for BLOCKCLONE backup method… #2201

petroniusniger commented Aug 5, 2019 •

edited

jsmeix Aug 7, 2019 •

edited

gozora Aug 11, 2019

jsmeix commented Aug 7, 2019

gozora commented Aug 7, 2019

gozora Aug 11, 2019

gozora Aug 11, 2019

petroniusniger Aug 14, 2019

gozora Aug 11, 2019

petroniusniger Aug 14, 2019

petroniusniger commented Aug 22, 2019

gozora Aug 26, 2019

petroniusniger Aug 27, 2019

gozora Aug 26, 2019 •

edited

gozora Aug 26, 2019

petroniusniger Aug 27, 2019

gozora commented Aug 26, 2019

gozora commented Sep 2, 2019

Fix for issue 2200: support new use-case for BLOCKCLONE backup method… #2201

Fix for issue 2200: support new use-case for BLOCKCLONE backup method… #2201

Conversation

petroniusniger commented Aug 5, 2019 • edited

jsmeix Aug 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsmeix commented Aug 7, 2019

gozora commented Aug 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petroniusniger commented Aug 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gozora Aug 26, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gozora commented Aug 26, 2019

gozora commented Sep 2, 2019

petroniusniger commented Aug 5, 2019 •

edited

jsmeix Aug 7, 2019 •

edited

gozora Aug 26, 2019 •

edited