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

Skip LVM filesystem check on LV reduction #417

Merged
merged 1 commit into from
May 27, 2024

Conversation

BrooklynDewolf
Copy link
Contributor

@BrooklynDewolf BrooklynDewolf commented May 23, 2024

Since LVM v2.03.17 (lvmteam/lvm2@f6f2737), reducing a logical volume (LV) requires the LV to be active due to the default 'checksize' option, which requires an active LV. This results in the following error when attempting to reduce a non-active LV:

err=[\' The LV must be active to safely reduce (see --fs options.)\']' ----

To resolve this, we now check if the LVM version is newer than 2.03.17. If so, we bypass the 'checksize' by using the '--fs ignore' option. This approach is viable because oVirt already handles checksize, making lvreduce redundant.

@dupondje
Copy link
Member

Reviewed-by: Jean-Louis Dupond [email protected]

Quite an important one. As if the lvreduce fails, the snapshot disk becomes ILLEGAL.
This only affects el9 hosts btw, as el8 has older lvm version without this --fs option.

Copy link
Member

@aesteve-rh aesteve-rh left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM in general. Could you try to add a test (lvm_test.py) checking the new cli option is added or not depending on the version? Maybe with mocked _get_lvm_version.

Also, worth checking that lvm appears at osinfo_test.py.

lib/vdsm/storage/lvm.py Show resolved Hide resolved
@BrooklynDewolf
Copy link
Contributor Author

Thank you for you feedback. I have added a parameterized test that tests the reduce function with different LVM versions which runs without any problems!

@aesteve-rh
Copy link
Member

Great, can you please check for lvm at osinfo_test.py too?

@BrooklynDewolf
Copy link
Contributor Author

I am not quite sure what you mean. I've added an extra assert to the test_package_versions() function.

@aesteve-rh
Copy link
Member

/ost

@BrooklynDewolf
Copy link
Contributor Author

No changes, just squashed my commits together.

@aesteve-rh
Copy link
Member

/ost

Since LVM v2.03.17 (lvmteam/lvm2@f6f2737), reducing a logical volume (LV) requires the LV to be active due to the default 'checksize' option, which requires an active LV. This results in the following error when attempting to reduce a non-active LV:

----
err=[\'  The LV must be active to safely reduce (see --fs options.)\']'
----

To resolve this, we now check if the LVM version is newer than 2.03.17. If so, we bypass the 'checksize' by using the '--fs ignore' option. This approach is viable because the oVirt already handles checksize, making lvreduce redundant.

Signed-off-by: Brooklyn Dewolf <[email protected]>
@sandrobonazzola sandrobonazzola merged commit 1b1a210 into oVirt:master May 27, 2024
17 checks passed
@sandrobonazzola
Copy link
Member

Merging based on previous review and approval

@sandrobonazzola sandrobonazzola added this to the ovirt-4.5.7 milestone Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants