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

#2596 Have unused LVM PV devices only as comment in disklayout.conf file #2603

Merged
merged 5 commits into from May 4, 2021

Conversation

gdha
Copy link
Member

@gdha gdha commented Apr 21, 2021

Signed-off-by: Gratien D'haese gratien.dhaese@gmail.com

  • Type: Enhancement

  • Impact: Low

  • Reference to related issue (URL): Unused RHEL Physical Volume aborts mkrescue #2596

  • How was this pull request tested? manually via savelayout

  • Brief description of the changes in this pull request: Do not stop with an Error during the savelayout when we discover a PV that is not part of a VG

Signed-off-by: Gratien D'haese <gratien.dhaese@gmail.com>
@gdha gdha added this to the ReaR v2.7 milestone Apr 21, 2021
@gdha gdha self-assigned this Apr 21, 2021
@jsmeix
Copy link
Member

jsmeix commented Apr 22, 2021

@gdha
if you like I could make changes to your pull request.
Do you agree that I change your pull request?

@jsmeix jsmeix added the enhancement Adaptions and new features label Apr 22, 2021
@gdha
Copy link
Member Author

gdha commented Apr 22, 2021

@jsmeix Be my guest ;-)

Reorgainzed the error checking logic to keep error checking sufficiently complete.
In particular still error out when "PV device '$pdev' is empty or more than one word".
Additional DebugPrint when "Skipping PV $pdev that is not part of a valid VG"
and more exact wording in user messages regarding what was actually tested.
@jsmeix
Copy link
Member

jsmeix commented Apr 22, 2021

@gdha
oops - I changed it very much - I hope you don't mind.
Hopefully with my changes it is still OK for you.

@jsmeix
Copy link
Member

jsmeix commented Apr 22, 2021

@gdha
I do not like in my current code that the last command in the loop is an error exit.
I would perfer the last line is the normal output when all is ok.
I could re-reorganize the error checking code if you like.

Much simpler and more straightforward code (KISS).
In particular check abort condition first and error out directly there.
@jsmeix
Copy link
Member

jsmeix commented Apr 22, 2021

When code looks bad it is bad so I re-reorganized the error checking code:
Now it looks much simpler and it is more straightforward (KISS).
In particular it checks the abort condition first and errors out directly there.

Typo fix "cannot to make" -> "cannot make"
@gdha
Copy link
Member Author

gdha commented Apr 22, 2021

@jsmeix It is fine for me. Thanks for the rewrite

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.

So hereby I happily approve my own rewrite ;-)

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.

Still not fully good.
I need to add something more...

Added missing comments that explain why the code is as is and added
the iusse URL rear#2596 to explain why
PVs that are not part of a VG are documented as comment in disklayout.conf
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.

So now is all hopefully sufficiently explained
so that others could properly fix issues or adapt and enhance it
as needed at any time later, cf. https://github.com/rear/rear/wiki/Coding-Style

@jsmeix
Copy link
Member

jsmeix commented Apr 30, 2021

@gdha
would you like to do more tests before you merge it
or should I merge it right now (provided you agree with my changes)?

@jsmeix
Copy link
Member

jsmeix commented May 3, 2021

When there is no objection I will merge it tomorrow afternoon .

Copy link
Member Author

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

@jsmeix code seems good to me

@jsmeix jsmeix changed the title #2596 Mark unused LVM PV devices in comment in disklayout.conf file #2596 Have unused LVM PV devices only as comment in disklayout.conf file May 4, 2021
@jsmeix jsmeix merged commit ec7fb6d into rear:master May 4, 2021
pcahyna pushed a commit to pcahyna/rear that referenced this pull request Feb 17, 2023
Have unused LVM PV devices only as comment in disklayout.conf:
PVs that are not part of a VG are documented as comment in disklayout.conf
but they are not recreated because they were not used on the original system
so there is no need to recreate them by "rear recover"
see rear#2596
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

2 participants