Add metric for disk info & last update #83

dannykopping · 2024-01-17T10:16:48Z

This PR adds a unused_disks_last_used_at metric which exports the disk, PVC, and PV identifiers. The value of the gauge is the last time the disk was mounted (although this is only available on GCP for now).

I want to use this metric to create an alert for a PVC which exists without its underlying disk/volume, which happened to my team recently when we ran a script to cleanup unused disks (but it didn't clean up the associated PVCs).

To my knowledge, neither opencost nor kube-state-metrics export a metric which allows 3-way correlation between PVCs, PVs, and their underlying disks.

Signed-off-by: Danny Kopping <[email protected]>

fedordikarev

That looks great to me 👍
Thanks for introducing methods for Meta, that's very helpful!

dannykopping · 2024-01-19T08:27:53Z

Thanks @fedordikarev
Do we need anyone else's approval before we merge?

dannykopping

Heads up @Pokom @fedordikarev @inkel

aws/provider.go

fedordikarev · 2024-01-19T11:00:58Z

@dannykopping one review was enough for the past PRs
from what I see on changes:

allign aws provider with other providers by adding logging
helpers functions for Meta
adding new prometheus metric similar to existing ones
my guess is that such changes shouldn't require much efforts (well, maybe an issue created but with detailed PR description that also could be not required here).

we may wait for @inkel review as he is initial author of the tool and introduced structure for code and modules.

Pokom

Left a minor nit and suggestion to add some tests for the new meta methods. Tested locally and was able to see the metrics, thanks Danny!

cmd/unused-exporter/exporter.go

Pokom · 2024-01-19T13:26:43Z

meta.go

+func (m Meta) CreatedForPV() string {
+	return m.coalesce("kubernetes.io/created-for/pv/name", "kubernetes.io-created-for-pv-name")
+}
+
+func (m Meta) CreatedForPVC() string {
+	return m.coalesce("kubernetes.io/created-for/pvc/name", "kubernetes.io-created-for-pvc-name")
+}
+
+func (m Meta) CreatedForNamespace() string {
+	return m.coalesce("kubernetes.io/created-for/pvc/namespace", "kubernetes.io-created-for-pvc-namespace")
+}
+
+func (m Meta) CreatedBy() string {
+	return m.coalesce("storage.gke.io/created-by", "created-by")
+}
+
+func (m Meta) Zone() string {
+	return m.coalesce("zone", "location")
+}
+
+func (m Meta) coalesce(keys ...string) string {
+	for _, k := range keys {
+		v, ok := m[k]
+		if !ok {
+			continue
+		}
+
+		return v
+	}
+
+	return ""
+}


This is great and really like how it cleaned up the outside code. Do you think there's value in creating a small test suite for these?

Yes, please.

To be clear: do you just want a test suite for the coalesce function?
I don't see much value in testing the getters.

coalesce and Meta.Equal 🙏🏽

inkel

Thank you @dannykopping for your contribution! In general LGTM, but I've some questions before merging this one. Also I'd like to see @Pokom comments addressed too, particularly regarding the tests for the new Meta methods.

aws/provider.go

cmd/unused-exporter/exporter.go

inkel · 2024-01-19T13:43:51Z

meta.go

+	akeys := m.Keys()
+	bkeys := b.Keys()
+
+	if len(akeys) != len(bkeys) {
+		return false
+	}
+
+	sort.Strings(akeys)
+	sort.Strings(bkeys)


Why pulling and sorting the keys? They don't seem to be used at all. The same could be achieved in a more performant way like this:

Suggested change

akeys := m.Keys()

bkeys := b.Keys()

if len(akeys) != len(bkeys) {

return false

}

sort.Strings(akeys)

sort.Strings(bkeys)

if len(m) != len(b) {

return false

}

My bad, I missed something here. I should be iterating over akeys to see if bkeys exist in the reference map and its value is equivalent. I'll fix this up.

Well, there's no need to sort the keys for that either. Just ranging on m will test that b indeed has the same keys and value.

True, I wonder what I was thinking here 🙃 I'll try remind myself when I come back to this.

inkel · 2024-01-19T13:44:20Z

meta.go

+		if !ok {
+			return false
+		}
+
+		if av != bv {
+			return false
+		}


How about this:

Suggested change

if !ok {

return false

}

if av != bv {

return false

}

if !ok || av != bv {

return false

}

inkel · 2024-01-19T13:44:48Z

meta.go

+func (m Meta) CreatedForPV() string {
+	return m.coalesce("kubernetes.io/created-for/pv/name", "kubernetes.io-created-for-pv-name")
+}
+
+func (m Meta) CreatedForPVC() string {
+	return m.coalesce("kubernetes.io/created-for/pvc/name", "kubernetes.io-created-for-pvc-name")
+}
+
+func (m Meta) CreatedForNamespace() string {
+	return m.coalesce("kubernetes.io/created-for/pvc/namespace", "kubernetes.io-created-for-pvc-namespace")
+}
+
+func (m Meta) CreatedBy() string {
+	return m.coalesce("storage.gke.io/created-by", "created-by")
+}
+
+func (m Meta) Zone() string {
+	return m.coalesce("zone", "location")
+}
+
+func (m Meta) coalesce(keys ...string) string {
+	for _, k := range keys {
+		v, ok := m[k]
+		if !ok {
+			continue
+		}
+
+		return v
+	}
+
+	return ""
+}


Yes, please.

Co-authored-by: Mark <[email protected]>

Pokom · 2024-01-19T16:05:03Z

cmd/unused-exporter/exporter.go

+ts := 0.0
+lastUsed := d.LastUsedAt()
+if !lastUsed.IsZero() {
+    ts = float64(lastUsed.UnixMilli())
+}				


@dannykopping looks like the suggestion wasn't formatted properly 😭

I has spaces at the end! Fire up your Emacs, M-x delete-trailing-whitespace et voilà! Or just add a before-save-hook that runs it automatically 😬

My guess is that Danny used my suggestion, which wasn't formatted properly.

Well, my comment still stands 😉

Also changed test package so unexported functions could be tested Signed-off-by: Danny Kopping <[email protected]>

dannykopping · 2024-01-22T09:55:40Z

@inkel @Pokom please have another look; I've addressed your comments

inkel

LGTM. Left a small question but I don't think it's blocking this.

cmd/unused-exporter/exporter.go

inkel · 2024-01-22T14:42:35Z

Thank you for your contribution, @dannykopping!

Add metric for disk info & last update

f54e3e3

Signed-off-by: Danny Kopping <[email protected]>

dannykopping requested review from inkel, Pokom and fedordikarev January 17, 2024 10:18

Use helper method

8e3b744

Signed-off-by: Danny Kopping <[email protected]>

fedordikarev approved these changes Jan 19, 2024

View reviewed changes

dannykopping commented Jan 19, 2024

View reviewed changes

aws/provider.go Show resolved Hide resolved

Pokom previously approved these changes Jan 19, 2024

View reviewed changes

inkel reviewed Jan 19, 2024

View reviewed changes

dannykopping dismissed Pokom’s stale review via e6be5c8 January 19, 2024 13:52

Update cmd/unused-exporter/exporter.go

e6be5c8

Co-authored-by: Mark <[email protected]>

Pokom reviewed Jan 19, 2024

View reviewed changes

Adding tests, addressing review feedback

582d6e3

Also changed test package so unexported functions could be tested Signed-off-by: Danny Kopping <[email protected]>

inkel approved these changes Jan 22, 2024

View reviewed changes

cmd/unused-exporter/exporter.go Show resolved Hide resolved

Pokom approved these changes Jan 22, 2024

View reviewed changes

inkel merged commit 54bcedb into main Jan 22, 2024
2 checks passed

inkel deleted the dannykopping/last-used branch January 22, 2024 14:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add metric for disk info & last update #83

Add metric for disk info & last update #83

dannykopping commented Jan 17, 2024 •

edited

Loading

fedordikarev left a comment

dannykopping commented Jan 19, 2024

dannykopping left a comment

fedordikarev commented Jan 19, 2024

Pokom left a comment

Pokom Jan 19, 2024

inkel Jan 19, 2024

dannykopping Jan 19, 2024

inkel Jan 19, 2024

inkel left a comment

inkel Jan 19, 2024

dannykopping Jan 19, 2024

inkel Jan 19, 2024

dannykopping Jan 19, 2024

inkel Jan 19, 2024

inkel Jan 19, 2024

Pokom Jan 19, 2024

inkel Jan 19, 2024

Pokom Jan 19, 2024

inkel Jan 19, 2024

dannykopping commented Jan 22, 2024

inkel left a comment

inkel commented Jan 22, 2024

Add metric for disk info & last update #83

Add metric for disk info & last update #83

Conversation

dannykopping commented Jan 17, 2024 • edited Loading

fedordikarev left a comment

Choose a reason for hiding this comment

dannykopping commented Jan 19, 2024

dannykopping left a comment

Choose a reason for hiding this comment

fedordikarev commented Jan 19, 2024

Pokom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inkel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannykopping commented Jan 22, 2024

inkel left a comment

Choose a reason for hiding this comment

inkel commented Jan 22, 2024

dannykopping commented Jan 17, 2024 •

edited

Loading