openebs / lvm-localpv

Dynamically provision Stateful Persistent Node-Local Volumes & Filesystems for Kubernetes that is integrated with a backend LVM2 data storage stack.
Apache License 2.0
245 stars 92 forks source link

fix(exporter): ignore duplicate LVs when collecting metrics #212

Closed patrickjahns closed 1 year ago

patrickjahns commented 1 year ago

Pull Request template

Why is this PR required? What issue does it fix?:

When LVs have more than a single segment(i.e. spread over several), collecting the metrics does no longer work, as lvs returns several entries for the same LV

What this PR does?:

As I did not want to change the behaviour of ListLVMLogicalVolume (even though its currently only used in metrics collection. I opted for skipping any further occurrences of the LV (per UUID) during metric collection.

Does this PR require any upgrade changes?: NO

If the changes in this PR are manually verified, list down the scenarios covered::

Assuming a VG with 100GB

Checklist:

patrickjahns commented 1 year ago

@kmova thanks for running the ci tests. I am a bit confused by the failures - it seems to me that some test framework dependencies need to run on a newer golang version ( if I read this correctly https://github.com/openebs/lvm-localpv/actions/runs/3488761623/jobs/5842170432#step:6:74 )

Maybe you have any other hints on where to look

Ab-hishek commented 1 year ago

Hi @patrickjahns What do you mean by LV will end up having two segments?

patrickjahns commented 1 year ago

@Ab-hishek Please see the outputs of lvdisplay and lvs in the issue https://github.com/openebs/lvm-localpv/issues/211

LVs can have several segments, when the size does not fit into a consecutive "block" of a volgroup. lvdisplay just shows that the volume is segmented, the output of lvs shows an entry for each segment ( the start extent/block )

kmova commented 1 year ago

@patrickjahns - probably a good idea to bump up the golang version in a different PR and then merge this. Something similar to this: https://github.com/openebs/jiva-operator/pull/201/files

Would be able to take up the go bump PR?

patrickjahns commented 1 year ago

@kmova @Ab-hishek I'll take a look on the weekend

patrickjahns commented 1 year ago

@kmova would you be able to take a look at this and the corresponding go version bump in https://github.com/openebs/lvm-localpv/pull/214

Mosibi commented 1 year ago

Can somebody please take a look at this PR? I verified that this patch indeed fixes the problem on our cluster(s) and would like to have it in the next release.

The PR code is a bit older, it needed a very small tweak, but otherwise still applies clean.

patrickjahns commented 1 year ago

I've rebased the changes to the latest develop - it would be really nice if any maintainer takes a look now. But my hopes are little after that long period of time and not receiving feedback in slack either 🤷

Mosibi commented 1 year ago

I've rebased the changes to the latest develop - it would be really nice if any maintainer takes a look now. But my hopes are little after that long period of time and not receiving feedback in slack either 🤷

Thank you for your quick response, let's hope the OpenEBS maintainers can make some time free to look at this.

avishnu commented 1 year ago

@niladrih @hrudaya21 @dsharma-dc PTAL