Feat/pvc attribute boot disks #151
Conversation
Part one of a set of changes that will introduce a Disk class that will make it easier to determine if a Disk is a bootdisk or not. This introduces the `Disk` struct with necessary attributes to meet existing requirements. It still uses the helper methods defined in `gke.go`, which will be moved over in the next change.
Refactors most of the utility methods around parsing disk metadata into a functions for Disk. Moves over all associated tests to `disk_test.go`.
This adds an additional label to the persistent_volume costs `disk_type`. Disk Type is determined by looking at the labels associated with the disk. The current implementation only has two possible values: - `boot_disk` - `persistent_volume`
pkg/google/gke/disk.go
Outdated
| return d.DiskName | ||
| } | ||
| // first check that the key exists in the map, if it does return the value | ||
| name := coalesce(d.Description, "kubernetes.io/created-for/pv/name", "kubernetes.io-created-for/pv-name") |
There was a problem hiding this comment.
can we put the key names in line 14 as well
pkg/google/gke/disk.go
Outdated
| ) | ||
|
|
||
| var ( | ||
| keys = []string{"kubernetes.io/created-for/pvc/namespace", "kubernetes.io-created-for/pvc-namespace"} |
There was a problem hiding this comment.
| keys = []string{"kubernetes.io/created-for/pvc/namespace", "kubernetes.io-created-for/pvc-namespace"} | |
| namespaceKeys = []string{"kubernetes.io/created-for/pvc/namespace", "kubernetes.io-created-for/pvc-namespace"} |
There was a problem hiding this comment.
[]string can't be a constant, however I could make the keys themselves constants.
| } | ||
|
|
||
| // coalesce will take a map and a list of keys and return the first value that is found in the map. If no value is found it will return an empty string | ||
| func coalesce(desc map[string]string, keys ...string) string { |
There was a problem hiding this comment.
could be as well pkg/utils material
There was a problem hiding this comment.
Eh, I'm really not a fan of pkg/utils to begin with. If we find other parts of our application need this, then we can consider a better location for it.
There was a problem hiding this comment.
[NIT] - to be super annoying, if we're not going to make this a util function called other places, it doesn't really make sense to define the keys arg as variadic, since we're just declaring and unwrapping a slice to pass to the function to just be re-wrapped into a slice to iterate over.
There was a problem hiding this comment.
Well fine, I'll remove the var slices and pass in the consts I defined :crossed-arms:
There was a problem hiding this comment.
upon further consideration, keeping it variadiac does allow you to pass an individual value to the coalesce function, but I'm not sure why you'd ever do that. If you're sure that you're checking for a single value, you wouldn't use coalence. If you're not sure, you're still going to be passing an unwrapped slice. 🤔
I'm guessing that it's slightly more efficient to explictly pass a slice, since then we don't have to convert back and forth. But the go compiler is probably smarter than I am.
pkg/google/gke/disk.go
Outdated
| ) | ||
|
|
||
| type Disk struct { | ||
| Cluster string |
There was a problem hiding this comment.
should make most of them private, as you have wrapper functions for all properties?
There was a problem hiding this comment.
That's a good callout! Let me make private what should be private.
| func coalesce(desc map[string]string, keys ...string) string { | ||
| for _, key := range keys { | ||
| if val, ok := desc[key]; ok { | ||
| return val | ||
| } | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
TBH this is how I'd do it:
| func coalesce(desc map[string]string, keys ...string) string { | |
| for _, key := range keys { | |
| if val, ok := desc[key]; ok { | |
| return val | |
| } | |
| } | |
| return "" | |
| } | |
| func coalesce(desc map[string]string, keys []string) string { | |
| for _, key := range keys { | |
| if val, ok := desc[key]; ok { | |
| return val | |
| } | |
| } | |
| return "" | |
| } |
and keep the original well-defined:
var pvcNamespaces = []string{ pvcNamespaceKey, pvcNamespaceShortKey }
etc
That way we'd avoid the conversion at run-time, and still have a single way to call the function.
At the end of the day, it doesn't really matter. Unless you decide to move it to a util function, in which case I'd definitely recommend moving to variadic to keep it as generic/easy-to-read-and-use as possible. Someone with a better nose for go may disagree
Introduces a new label
disk_typewhich can be represented asboot_diskpersistent_volumeThe purpose of this is to help further differentiate the type of disks that are being exported. When comparing exported results, I found ~4000 disks that were not represented by
opencost. After investigating further, these disks were specifically related to boot disks.The neat thing about this is we can start associated the costs of boot volumes to the team that is responsible for provisioning nodes within our k8s environments.