#3138 PR `merged`: Add support for Dell PowerProtect Data Manager¶

schlomo opened issue at 2024-01-26 10:32:¶

Relax-and-Recover (ReaR) Pull Request Template¶

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

Pull Request Details:¶

Type: New Feature
Impact: Normal
Reference to related issue (URL): Fixes #3126
How was this pull request tested?

Manual tests in Dell lab environment on RHEL/compatible 7,8,9 and SLES 12,15 and Ubuntu 20,22

Description of the changes in this pull request:

Add support for PPDM and fix bugs we encounted.

Introduces Bash associative arrays, mandating Bash 4

schlomo commented at 2024-01-26 10:33:¶

@rear/contributors Please have a look, I'd like to do the last fixes on Monday and merge because that is the limit for the Dell lab where I can run tests.

pcahyna commented at 2024-01-26 10:51:¶

First note: please fix the commit message of f15344afe91ac07b6015563f917dc7b141519bed and force-push.

pcahyna commented at 2024-01-26 10:56:¶

Second impression: please do not mix formatting changes and actual functionality changes in the same commit. 24c122798406e0d62887e0e2909bb942d44a88dd is about "Ensure minimum Bash version ", but it contains hundreds of unrelated changed lines.
I also have the impression that many of the formatting changes should be avoided, as they make the code look worse (and less in line with the examples in https://github.com/rear/rear/wiki/Coding-Style ).

schlomo commented at 2024-01-26 11:03:¶

Thanks @pcahyna for helping us to be better contributors, you are right in both points.

schlomo commented at 2024-01-26 11:08:¶

Im actually unhappy with
https://github.com/rear/rear/pull/3138/files#diff-0cf7e0a3b217a0feaf68c285ada67dbf21b981b3c6525c79d3fc29b9d956ada8R98-R103

because it mixes in all the backup methods into a single file instead of using our directory based overlay architecture to separate the concerns for each backup software. If it is OK with @rear/contributors then I'd gladly pull this code apart by introducing a new LD_LIBRARY_PATH_ADDITIONS variable that can be set for every backup tool.

The reason why I didn't do that is that I cannot test this change for all backup tools so that we would have to rely on eyeball inspection and later tests by others.

schlomo commented at 2024-01-26 13:11:¶

Another thought: I added a feature to backup and restore the ESP because PPDM doesn't backup vfat filesystems. I'm wondering if I should change this code to be more generic so that it would either happen automatically for any recovery that doesn't restore the ESP or that it can be activated for those commercial backup tools that can't backup the ESP themselves.

In my test case the ESP was ~7MB, not much at all.

pcahyna commented at 2024-01-29 09:04:¶

Another thought: I added a feature to backup and restore the ESP because PPDM doesn't backup vfat filesystems. I'm wondering if I should change this code to be more generic so that it would either happen automatically for any recovery that doesn't restore the ESP or that it can be activated for those commercial backup tools that can't backup the ESP themselves.

I must say that a backup software that does not backup vfat filesystems does not looks suitable as a backup method for ReaR. What if the user has a vfat filesystem mounted somewhere else, and includes it in layout?

pcahyna commented at 2024-01-29 09:11:¶

Would it work to add a PPDM-specific check that there are not vfat filesystems in the layout (except the ESP)?

pcahyna commented at 2024-01-29 10:08:¶

Thanks @pcahyna for helping us to be better contributors, you are right in both points.

thanks for the update, it looks much better now, but there are still some places with unrelated white space changes remaining. Please also drop the huge autogenerated html files from the PR, unless they have been in the source tree before.

schlomo commented at 2024-01-29 12:22:¶

@pcahyna about the white space changes like do source $script; vs. $script ; I have the challenge that there doesn't seem to be a shfmt setting to allow that.

I really would like us to find a way to use automated formatting, as it significantly reduces mental overhead for us developers. BTW, The Google Shell Guide also doesn't require a blank before the ; in a do statement, so maybe we don't need to require that.

What I would specifically suggest is to amend our .editorconfig file like this:

root = true
[*]
end_of_line = lf
insert_final_newline = true
indent_style = space
indent_size = 4
shell_variant = bash
binary_next_line = true
switch_case_indent = true
space_redirects = true
keep_padding = true
function_next_line = false

[[bash]]
shell_variant = bash

and be happy what shfmt makes out of it.

schlomo commented at 2024-01-29 12:31:¶

about the HTML files: I added them to our .gitignore so that future runs of make doc won't have this effect

schlomo commented at 2024-01-29 14:43:¶

BTW, I'm now reworking the LD_LIBRARY_PATH code to be modularized and the /boot/efi code to be reusable.

pcahyna commented at 2024-01-29 17:07:¶

@pcahyna about the white space changes like do source $script; vs. $script ; I have the challenge that there doesn't seem to be a shfmt setting to allow that.

I really would like us to find a way to use automated formatting, as it significantly reduces mental overhead for us developers.

@schlomo a commit should not include changes unrelated to the purpose of the commit, and should not mix formatting changes/style changes and functional changes. It seems that your proposal implies reformatting every source code file that one touches, which will result in a huge number of unrelated whitespace changes, especially for longer files. This will significantly increase the mental overhead for those reading the diffs, searching in git log or using git blame (like I find myself doing very often).

BTW, The Google Shell Guide also doesn't require a blank before the ; in a do statement, so maybe we don't need to require that.

This seems though to be our preferred style according to the examples in https://github.com/rear/rear/wiki/Coding-Style and most of the existing code. I am not saying that the style can't be changed, but a PR about adding support for Dell PowerProtect Data Manager is not the good place to discuss it or change the style.

schlomo commented at 2024-01-29 17:35:¶

@jsmeix I might have found an example where the ldd logic doesn't work as expected, specifically the cd to the library directory before running ldd
https://github.com/rear/rear/blob/58d6ec23941557af097def87ea25b13d340f834a/usr/share/rear/build/default/990_verify_rootfs.sh#L179
doesn't seem to work in this example:

REAR lvrh9reart01:~/rear-ppdm/usr/share/rear # show libr
declare -- DP_LD_LIBRARY_PATH="/opt/omni/lib:/opt/omni/lib64"
declare -- LD_LIBRARY_PATH_FOR_BACKUP_TOOL="/opt/dpsapps/fsagent/lib:/opt/dpsapps/agentsvc/lib"
declare -- NBU_LD_LIBRARY_PATH="/usr/openv/lib:/usr/openv/netbackup/sec/at/lib/"
declare -- NON_FATAL_BINARIES_WITH_MISSING_LIBRARY=""
declare -- TSM_LD_LIBRARY_PATH="/opt/tivoli/tsm/client/ba/bin:/opt/tivoli/tsm/client/api/bin64:/opt/tivoli/tsm/client/api/bin:/opt/tivoli/tsm/client/api/bin64/cit/bin"
REAR lvrh9reart01:~/rear-ppdm/usr/share/rear # cd /opt/dpsapps/agentsvc/lib/
REAR lvrh9reart01:/opt/dpsapps/agentsvc/lib # ldd libssl.so.1.0.0 
        linux-vdso.so.1 (0x00007fff53181000)
        libcrypto.so.1.0.0 => not found
        libdl.so.2 => /lib64/libdl.so.2 (0x00007f3d18ed0000)
        libc.so.6 => /lib64/libc.so.6 (0x00007f3d18800000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f3d18edd000)

instead, it only works with specifically setting LD_LIBRARY_PATH:

REAR lvrh9reart01:/opt/dpsapps/agentsvc/lib # LD_LIBRARY_PATH=$LD_LIBRARY_PATH_FOR_BACKUP_TOOL ldd libssl.so.1.0.0 
        linux-vdso.so.1 (0x00007ffcb9fec000)
        libcrypto.so.1.0.0 => /opt/dpsapps/fsagent/lib/libcrypto.so.1.0.0 (0x00007fbba6200000)
        libdl.so.2 => /lib64/libdl.so.2 (0x00007fbba6ab0000)
        libc.so.6 => /lib64/libc.so.6 (0x00007fbba5e00000)
        /lib64/ld-linux-x86-64.so.2 (0x00007fbba6abd000)

The system is RHEL 9.3.

I also found another false positive for the systemd libraries which is apparently caused by the same problem (ldd not picking up libraries from the current working directory):

Testing each binary with 'ldd' and look for 'not found' libraries within the recovery system
/usr/lib64/systemd/libsystemd-core-252.so requires additional libraries
        libsystemd-shared-252.so => not found

Actually, libsystemd-shared-252.so is also in the same /usr/lib64/systemd/ directory, and the manual test shows the same problem:

[root@lvrh9reart01 systemd]# ldd libsystemd-core-252.so 
        linux-vdso.so.1 (0x00007ffc89307000)
        libsystemd-shared-252.so => not found
        libseccomp.so.2 => /lib64/libseccomp.so.2 (0x00007f886f256000)
        libpam.so.0 => /lib64/libpam.so.0 (0x00007f886f244000)
        libaudit.so.1 => /lib64/libaudit.so.1 (0x00007f886f216000)
        libkmod.so.2 => /lib64/libkmod.so.2 (0x00007f886efe5000)
        libselinux.so.1 => /lib64/libselinux.so.1 (0x00007f886efb8000)
        libmount.so.1 => /lib64/libmount.so.1 (0x00007f886ef73000)
        libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x00007f886ef58000)
        libc.so.6 => /lib64/libc.so.6 (0x00007f886ec00000)
        libeconf.so.0 => /lib64/libeconf.so.0 (0x00007f886ef4d000)
        libm.so.6 => /lib64/libm.so.6 (0x00007f886ee72000)
        libcap-ng.so.0 => /lib64/libcap-ng.so.0 (0x00007f886ee69000)
        libzstd.so.1 => /lib64/libzstd.so.1 (0x00007f886eb29000)
        liblzma.so.5 => /lib64/liblzma.so.5 (0x00007f886ee3d000)
        libz.so.1 => /lib64/libz.so.1 (0x00007f886ee23000)
        libcrypto.so.3 => /lib64/libcrypto.so.3 (0x00007f886e600000)
        libpcre2-8.so.0 => /lib64/libpcre2-8.so.0 (0x00007f886ea8d000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f886f27e000)
        libblkid.so.1 => /lib64/libblkid.so.1 (0x00007f886ea55000)

Does anybody have any idea where this comes from and how to fix it?

pcahyna commented at 2024-01-29 17:40:¶

I also found another false positive for the systemd libraries which is apparently caused by the same problem (ldd not picking up libraries from the current working directory):
Testing each binary with 'ldd' and look for 'not found' libraries within the recovery system
/usr/lib64/systemd/libsystemd-core-252.so requires additional libraries
        libsystemd-shared-252.so => not found

this message is harmless, annoying though. There is an issue about it already : #3021

schlomo commented at 2024-01-29 18:08:¶

Yes, let's move the coding style change out of here to #3142

schlomo commented at 2024-01-29 18:08:¶

@pcahyna I think I fixed everything, can we now talk about PPDM?

schlomo commented at 2024-01-30 14:39:¶

If there are no more concerns then I'll merge tomorrow morning and continue with the other topics.

pcahyna commented at 2024-01-31 16:22:¶

@schlomo thanks for cleaning up the commit history, looks good now.
I have not had a chance to review the latest changes (generalized ESP restore and LD_LIBRARY_PATH stuff). I have some minor comments about them that can be addressed in a separate PR.

[Export of Github issue for rear/rear.]

#3138 PR merged: Add support for Dell PowerProtect Data Manager¶

schlomo opened issue at 2024-01-26 10:32:¶

Relax-and-Recover (ReaR) Pull Request Template¶

Pull Request Details:¶

schlomo commented at 2024-01-26 10:33:¶

pcahyna commented at 2024-01-26 10:51:¶

pcahyna commented at 2024-01-26 10:56:¶

schlomo commented at 2024-01-26 11:03:¶

schlomo commented at 2024-01-26 11:08:¶

schlomo commented at 2024-01-26 13:11:¶

pcahyna commented at 2024-01-29 09:04:¶

pcahyna commented at 2024-01-29 09:11:¶

pcahyna commented at 2024-01-29 10:08:¶

schlomo commented at 2024-01-29 12:22:¶

schlomo commented at 2024-01-29 12:31:¶

schlomo commented at 2024-01-29 14:43:¶

pcahyna commented at 2024-01-29 17:07:¶

schlomo commented at 2024-01-29 17:35:¶

pcahyna commented at 2024-01-29 17:40:¶

schlomo commented at 2024-01-29 18:08:¶

schlomo commented at 2024-01-29 18:08:¶

schlomo commented at 2024-01-30 14:39:¶

pcahyna commented at 2024-01-31 16:22:¶

#3138 PR `merged`: Add support for Dell PowerProtect Data Manager¶