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

feat(lvm disk detection): add support to detect disks used by LVM localpv #619

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

Ab-hishek
Copy link

@Ab-hishek Ab-hishek commented Aug 25, 2021

Signed-off-by: Abhishek Agarwal abhishek.agarwal@mayadata.io

Why is this PR required? What issue does it fix?:
openebs/openebs#3407

What this PR does?:
This PR adds the functionality to detect disk that are used by lvm-locapv and tag them with block-device-tag=lvm-localpv during blockdevice creation.

Does this PR require any upgrade changes?:

If the changes in this PR are manually verified, list down the scenarios covered::
TODO: Manual test for the change

Any additional information for your reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs

Checklist:

Signed-off-by: Abhishek Agarwal <abhishek.agarwal@mayadata.io>
@Ab-hishek Ab-hishek self-assigned this Aug 25, 2021
@Ab-hishek Ab-hishek added the pr/hold-review Needs rework. label Aug 25, 2021
@codecov-commenter
Copy link

codecov-commenter commented Aug 25, 2021

Codecov Report

Merging #619 (e209899) into master (426eeaa) will decrease coverage by 0.89%.
The diff coverage is 43.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #619      +/-   ##
==========================================
- Coverage   47.26%   46.36%   -0.90%     
==========================================
  Files          78       78              
  Lines        3781     3811      +30     
==========================================
- Hits         1787     1767      -20     
- Misses       1838     1884      +46     
- Partials      156      160       +4     
Impacted Files Coverage Δ
cmd/ndm_daemonset/probe/usedbyprobe.go 14.44% <0.00%> (-1.22%) ⬇️
cmd/ndm_daemonset/probe/addhandler.go 70.25% <56.52%> (-7.23%) ⬇️
cmd/ndm_daemonset/probe/eventhandler.go 30.55% <0.00%> (-8.34%) ⬇️
cmd/ndm_daemonset/probe/udevprobe.go 48.53% <0.00%> (-3.77%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 426eeaa...e209899. Read the comment docs.

Signed-off-by: Abhishek Agarwal <abhishek.agarwal@mayadata.io>
@Ab-hishek Ab-hishek removed the pr/hold-review Needs rework. label Aug 27, 2021
// checking for lvm localPV
usedByProbe := newUsedByProbe(blockDevice.DevPath)
// check for LVM file system
fstype := usedByProbe.BlkidIdentifier.GetOnDiskFileSystem()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a reliable way to check if the device is in use by LVM localPV. Because nodes having LVM setup will also have the same lvm filesystem on the disks.

we need to check if this method helps us to identify between lvm localPV and LVM that has been setup by user.

Copy link
Contributor

Choose a reason for hiding this comment

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

This same issue is there for cstor and zfs -localpv. To identify that, we check if the device is exclusively locked. which happens only if kernel zfs is using the disk.

Copy link
Author

Choose a reason for hiding this comment

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

Because nodes having LVM setup will also have the same lvm filesystem on the disks.

By this you mean, disks that are used manually by LVM utils?

Copy link
Contributor

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

@Ab-hishek Some changes are needed on how to differentiate between disks used by LVM localPV and LVM that has been setup by the user.

@Ab-hishek Ab-hishek added the pr/hold-review Needs rework. label Sep 14, 2021
@Ab-hishek Ab-hishek added the pr/wip Working on review comments or more changes label Oct 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/hold-review Needs rework. pr/wip Working on review comments or more changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants