Avoid vgcfgrestore on thin volumes/pools and any other unsupported volume types.
#3210
+33
−1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
and any other unsupported volume types. vgcfgrestore is not supposed to be able to restore any logical volumes that use kernel metadata. All volume types except linear and striped use kernel metadata. Main purpose of vgcfgrestore (with mandatory --force option) is to let users fix existing thin-pool, not to recreate the pool on empty disks. Do not even try vgcfgrestore on VGs that need any kernel metadata, because it might lead to an inconsistent state (if there are data that the kernel might interpret as LV metadata present on the disks). For VGs that have any volume with kernel metadata and are thus unsupported by vgcfgrestore, switch automatically to LV creation using lvcreate, similarly to MIGRATION_MODE. Avoid vgcfgrestore --force entirely, since it should not be needed now. This mostly reverts changes in commits 311bfb3 and 1b779ab. The former code is preserved and gets enabled if FORCE_VGCFGRESTORE=y. This option is on purpose undocumented though and may be removed in the future.
|
According to |
schlomo
requested changes
May 24, 2024
There was a problem hiding this comment.
I'm not a big fan of hidden features, they tend to get forgotton.
I therefore would kindly ask you to add the variable to our default.conf and document it there. If you think that this shouldn't be used then please add a deprecation warning if the variable is set, linking to this issue and asking people to provide feedback why they need this feature.
That will make actually removing it so much easier in the future, IMHO.
| # doesn't contain any LVs that use kernel metadata. | ||
| # If the function returns true, we can safely use vgcfgrestore to restore the VG. | ||
| function lvmgrp_supports_vgcfgrestore() { | ||
| if is_true "${FORCE_VGCFGRESTORE-no}"; then |
There was a problem hiding this comment.
Suggested change
| if is_true "${FORCE_VGCFGRESTORE-no}"; then | |
| if is_true "${FORCE_VGCFGRESTORE:-no}"; then |
| @@ -124,6 +124,9 @@ if lvm vgcfgrestore -f "$VAR_DIR/layout/lvm/${vg}.cfg" $vg >&2 ; then | |||
| create_volume_group=( \$( RmInArray "$vg" "\${create_volume_group[@]}" ) ) | |||
| create_logical_volumes=( \$( RmInArray "$vg" "\${create_logical_volumes[@]}" ) ) | |||
|
|
|||
| EOF | |||
| if is_true "${FORCE_VGCFGRESTORE-no}"; then | |||
There was a problem hiding this comment.
Suggested change
| if is_true "${FORCE_VGCFGRESTORE-no}"; then | |
| if is_true "${FORCE_VGCFGRESTORE:-no}"; then |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request Details:
Type: Bug Fix
Impact: Normal
Reference to related issue (URL):
https://bugzilla.redhat.com/show_bug.cgi?id=1747468
Thin pool recreation logic / use of vgcfgrestore is broken #2222
How was this pull request tested?
In production of RHEL 8 and RHEL 9.
Description of the changes in this pull request:
This patch was originally created in 2021 by @pcahyna in 5d5d1db and is applied in Fedora and RHEL 8+. Somehow it has slipped under our radar was never submitted upstream.
vgcfgrestoreis not supposed to be able to restore any logical volumes that use kernel metadata. All volume types except linear and striped use kernel metadata. Main purpose ofvgcfgrestore(with mandatory--forceoption) is to let users fix existing thin-pool, not to recreate the pool on empty disks. Do not even tryvgcfgrestoreon VGs that need any kernel metadata, because it might lead to an inconsistent state (if there are data that the kernel might interpret as LV metadata present on the disks).For VGs that have any volume with kernel metadata and are thus unsupported by
vgcfgrestore, switch automatically to LV creation usinglvcreate, similarly toMIGRATION_MODE.Avoid
vgcfgrestore --forceentirely, since it should not be needed now.This mostly reverts changes in commits 311bfb3 and 1b779ab. The former code is preserved and gets enabled if
FORCE_VGCFGRESTORE=y. This option is on purpose undocumented though and may be removed in the future.