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

OBDR fixes #2963

Merged
merged 6 commits into from Apr 23, 2023
Merged

OBDR fixes #2963

merged 6 commits into from Apr 23, 2023

Conversation

maslo64
Copy link

@maslo64 maslo64 commented Mar 28, 2023

Relax-and-Recover (ReaR) Pull Request Template

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

Pull Request Details:
  • Type: Bug Fix

  • Impact: Normal

  • Reference to related issue (URL):
    Don't deprecate OBDR because it still works and is in use #2637

  • How was this pull request tested?
    Successful restore on DL380 Gen8 with LTO5 via OUTPUT=OBDR method.

  • Brief description of the changes in this pull request:
    OBDR was not creating new ISO recover enviroment files
    consider in scripts 'hpsa' driver along with obsolete 'cciss' driver
    parse output from lsscsi also for cases when there is more Host/Channel/Id/Lun than 10

Ladislav Mate added 3 commits March 28, 2023 15:26
In case of large number of connected devices f.x.
[6:0:15:0]   tape    HP       Ultrium 5-SCSI   Y6PW  /dev/st11
devices might not be recognized properly.
…${HOSTNAME}.iso, so add from ISO

../../ISO/Linux-i386/820_create_iso_image.sh

Subsequentlyt we'll need to rename writing of ISO to tape to later stage , so rename it from 810_write_image.sh to 840_write_image.sh

Fix BACKUP_PROG_COMPRESS_OPTIONS and BACKUP_PROG_COMPRESS_SUFFIX as suggested in rear#2911
@@ -2,7 +2,7 @@
REQUIRED_PROGS+=( mt )

# Test for cciss driver and include the necessary tools
if grep -q '^cciss ' /proc/modules; then
if grep -q -e '^cciss ' -e '^hpsa' /proc/modules; then
PROGS+=( "${PROGS_OBDR[@]}" )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason that there is no trailing space character
in '^hpsa' in contrast to '^cciss ' that has one?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a more general question:
Why to make OBDR support conditional on specific drivers (cciss and hpsa)?
Is there a reason why an OBDR-capable tape should not work with any HBA, not just the ones handled by these drivers?
The code was introduced in 3ed01de with the message "OBDR support for HP specific environments (using CCISS)" but unfortunately it does not explain why it should be restricted to HP-specific environments.
@jhoekx @dagwieers do you happen to know, please?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only had HP equipment to test when we implemented it. We were consulting for HP at the time (2011?) for a customer that needed OBDR on systems that were also doing DRBD.

I was not aware that OBDR was supported by other vendors at all.

There was no information about it back then. It was quite difficult to even figure out how it worked on HP drives as the only software where the source code was available to us that supported it was MondoRescue. We eventually found a manual about the tape drive we were using that documented the SCSI commands we needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @jhoekx for the quick reply and congratulations for having found the docs back then.
According to my research, OBDR is indeed HP-specific. But isn't it implemented entirely in the tape drive firmware, making it only HP-drive specific, and not HP-adapter specific? IIUC the tape can pretend that it is a CD-ROM drive, and it should not need anything special in the HBA for this to work.
@maslo64 since you have access to OBDR-capable hardware - what do you think?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using mostly HP(E) hardware, so can't test on systems from other vendors, but would agreed that this limitation for HBA is not necessary needed in order to work with any server and HP LTO. If someone will be performing restore from OBDR, he will most likely need tools in REQUIRED_PROGS_OBDR[@].
We were previously using Mondo/Mindi , but switches to ReaR as it works more reliably to our case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this section should then go to a script under usr/share/rear/prep/OBDR/default instead of TAPE (since it will need to be invoked if and only if OBDR is used, right?) and not be conditional on any particular HBA driver.
By the way, PROGS_OBDR does not even exist (only REQUIRED_PROGS_OBDR does).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I have OBDR capable hardware available only few times per year, and mostly for DR testing, I wanted to perform absolute minimal changes to the code, which worked for me, so any additional testing would would be in far future.

@@ -1,7 +1,7 @@
### Disable OBDR mode
###

if ! grep -q '^cciss ' /proc/modules; then
if ! grep -q '^cciss ' -e '^hpsa' /proc/modules; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason that there is no trailing space character
in '^hpsa' in contrast to '^cciss ' that has one?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct, should be exact name of module. updated.

@@ -29,7 +29,7 @@ if [[ "$CDROM_DEVICE" && -b $CDROM_DEVICE ]]; then
fi

### Find Host/Channel/Id/Lun of device
HCIL="$(lsscsi | awk 'BEGIN {FS=""} / +cd\/dvd +HP +Ultrium/ { print $2, $4, $6, $8; exit }')"
HCIL="$(lsscsi | awk 'BEGIN {FS="[\\]|\\[|:]"} / +cd\/dvd +HP +Ultrium/ { print $2, $3, $4, $5; exit }')"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment what that command does
i.e. describe the implementation, cf.
"Code must be easy to read" and
"Code must be easy to understand" in
https://github.com/rear/rear/wiki/Coding-Style

It is hard to imagine what goes on
when one does not have such hardware
so an original lsscsi line as a comment
would help a lot here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I added comment with explanation, why we there is change in the code.

@jsmeix jsmeix self-assigned this Mar 31, 2023
@jsmeix jsmeix added the bug The code does not do what it is meant to do label Mar 31, 2023
@jsmeix jsmeix added this to the ReaR v2.8 milestone Mar 31, 2023
@jsmeix
Copy link
Member

jsmeix commented Mar 31, 2023

@maslo64
thank you for your contribution to ReaR
that fixes and improves OBDR support in ReaR!

@jsmeix jsmeix requested a review from a team March 31, 2023 11:01
@jsmeix
Copy link
Member

jsmeix commented Mar 31, 2023

@rear/contributors
please have a look here - as time permits.
Perhaps you may spot some obvious possible issue.

Ladislav Mate added 3 commits April 5, 2023 12:31
…e output from lsscsi

Previosly we would search only on fixed character possitions 2 4 6 8 in a line, which on some systems with larger nubmber of devices doesn't have to be correct.
@schlomo schlomo merged commit 1936946 into rear:master Apr 23, 2023
11 of 12 checks passed
@schlomo
Copy link
Member

schlomo commented Apr 23, 2023

Thanks a lot @maslo64 for your contribution! I can totally understand your reluctance to change more than the bare minimum.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The code does not do what it is meant to do fixed / solved / done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants