Avoid vgcfgrestore on thin volumes/pools and any other unsupported volume types.
#3210
Conversation
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
left a comment
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.
| if is_true "${FORCE_VGCFGRESTORE-no}"; then | |
| if is_true "${FORCE_VGCFGRESTORE:-no}"; then |
| create_logical_volumes=( \$( RmInArray "$vg" "\${create_logical_volumes[@]}" ) ) | ||
| EOF | ||
| if is_true "${FORCE_VGCFGRESTORE-no}"; then |
There was a problem hiding this comment.
| if is_true "${FORCE_VGCFGRESTORE-no}"; then | |
| if is_true "${FORCE_VGCFGRESTORE:-no}"; then |
|
@pcahyna what about the change of Do we shelf this till after a 3.0 release or do you plan to merge it before? Please see my comments above. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
@pcahyna Please address the requested changes so we can perform the merge - thanks! |
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 and 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.