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

Fix issue #2441 & #2442 #2445

Merged
merged 9 commits into from Jul 23, 2020
Merged

Fix issue #2441 & #2442 #2445

merged 9 commits into from Jul 23, 2020

Conversation

DamaniN
Copy link
Contributor

@DamaniN DamaniN commented Jun 30, 2020

Pull Request Details:
  • Type: Enhancement

  • Impact: High

  • Reference to related issue (URL):

#2441

  • How was this pull request tested?

Restored system from replica cluster running Rubrik CDM v5.2

  • Brief description of the changes in this pull request:

The RBS agent file name changed in Rubrik CDM v5.1. Failed over to the new file name if the old file doesn't work.

@jsmeix jsmeix changed the title Fix issue #2441 Fix issue #2441 Rubrik CDM module fails to restore from remote cluster on newer Rubrik releases Jun 30, 2020
@jsmeix jsmeix self-assigned this Jun 30, 2020
@jsmeix jsmeix added the enhancement Adaptions and new features label Jun 30, 2020
@jsmeix jsmeix added this to the ReaR v2.7 milestone Jun 30, 2020
Copy link
Member

@jsmeix jsmeix left a comment

Choose a reason for hiding this comment

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

I do not use BACKUP=CDM so I cannot test anything here but
from plain looking at the code things look OK to me.

I wished there was a comment in the code
(cf. "Code must be easy to understand" in https://github.com/rear/rear/wiki/Coding-Style)
that at least mentiones the issue #2441
or even better directly tells about the source of this problem
why two file names must be tried to dowload
for example something like

# The name of the tar file that is being downloaded has changed in Rubrik CDM v5.1.
# Before Rubrik CDM v5.1 it was rubrik-agent-sunos5.10.sparc.tar.gz
# since Rubrik CDM v5.1 it is rubrik-agent-solaris.sparc.tar.gz
# cf. https://github.com/rear/rear/issues/2441

@jsmeix jsmeix requested a review from a team June 30, 2020 07:16
@jsmeix
Copy link
Member

jsmeix commented Jun 30, 2020

@rear/contributors
I would appreciate a second review from any other ReaR contributor.

Copy link
Member

@jsmeix jsmeix left a comment

Choose a reason for hiding this comment

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

It seems via this pull request the file
usr/share/rear/verify/CDM/default/400_verify_cdm.sh
gets deleted.
Is that intentional?

@DamaniN
Copy link
Contributor Author

DamaniN commented Jun 30, 2020

It seems via this pull request the file
usr/share/rear/verify/CDM/default/400_verify_cdm.sh
gets deleted.
Is that intentional?

Yes this is intentional. That file was causing the Rubrik agent to be started twice. The functionality was moved to another file in an earlier update but the file was never removed at that time. Starting the agent twice was contributing to #2441.

@DamaniN
Copy link
Contributor Author

DamaniN commented Jun 30, 2020

I do not use BACKUP=CDM so I cannot test anything here but
from plain looking at the code things look OK to me.

I wished there was a comment in the code
(cf. "Code must be easy to understand" in https://github.com/rear/rear/wiki/Coding-Style)
that at least mentiones the issue #2441
or even better directly tells about the source of this problem
why two file names must be tried to dowload
for example something like

# The name of the tar file that is being downloaded has changed in Rubrik CDM v5.1.
# Before Rubrik CDM v5.1 it was rubrik-agent-sunos5.10.sparc.tar.gz
# since Rubrik CDM v5.1 it is rubrik-agent-solaris.sparc.tar.gz
# cf. https://github.com/rear/rear/issues/2441

Added this note in commit 7b1e26b.

@DamaniN
Copy link
Contributor Author

DamaniN commented Jun 30, 2020

d86ca29 also fixes #2442.

@DamaniN DamaniN changed the title Fix issue #2441 Rubrik CDM module fails to restore from remote cluster on newer Rubrik releases Fix issue #2441 & #2442 Jun 30, 2020
Copy link
Member

@jsmeix jsmeix left a comment

Choose a reason for hiding this comment

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

I do not use BACKUP=CDM so I cannot actually test it but
from plain looking at the code things look good to me.

@jsmeix
Copy link
Member

jsmeix commented Jul 1, 2020

@DamaniN
thank you for your prompt replies and your further code enhancements.

Now things are clear to me so I approve it.

@jsmeix
Copy link
Member

jsmeix commented Jul 1, 2020

@rear/contributors
a second review from another ReaR contributor would be nice to have.
If there is none and if there are no objections I would "just merge" it tomorrow afternoon.

Copy link
Member

@schlomo schlomo left a comment

Choose a reason for hiding this comment

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

Looks good to me. Could you please use the ReaR temp dir and rely on binaries being in $PATH.

I kindly also ask you to fix that with all the CDM-related code, if you can.

Obviously we can't test the code and rely on your testing here.

Copy link
Member

@gdha gdha left a comment

Choose a reason for hiding this comment

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

@DamaniN What is the reason of the removal of usr/share/rear/verify/CDM/default/400_verify_cdm.sh ?

@jsmeix
Copy link
Member

jsmeix commented Jul 1, 2020

@gdha
for the reason of the removal of usr/share/rear/verify/CDM/default/400_verify_cdm.sh
see #2445 (comment)

Tried a smaller fix for
rear#2445 (comment)
but things became almost a rewrite now
Added generic URL download method so the user has a better chance to succeed
when the already known hardcoded vaules in ReraR don't work in his particular case.
Typo fix from "it's" (which would mean "it is") to "its"
@jsmeix
Copy link
Member

jsmeix commented Jul 22, 2020

I tested it as far as I could without a Rubrik (CDM) cluster environment
by simulating things as far as possible with reasonable effort.
To me things look OK now.

If there are no objections I would like to merge it as is tomorrow afternoon.
We are in early ReaR 2.7 development phase so we can fix things if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adaptions and new features fixed / solved / done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants