Add "resource_name" to scaled_up_gpu_nodes_total and scaled_down_gpu_nodes_total metrics #5518
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
| @@ -246,7 +246,7 @@ var ( | |||
| Namespace: caNamespace, | |||
| Name: "scaled_up_gpu_nodes_total", | |||
| Help: "Number of GPU nodes added by CA, by GPU name.", | |||
| }, []string{"gpu_name"}, | |||
| }, []string{"resource_name", "gpu_name"}, | |||
There was a problem hiding this comment.
Since this is specifically for the resource name of a GPU, and will only be set on a GPU scale-up, I'd prefix "resource_name" with "gpu" as well (here and everywhere in this PR)? Otherwise the label name is quite ambiguous - CPU and memory are resources as well and the trigger for most scale-ups, but this won't be set for them.
| @@ -270,7 +270,7 @@ var ( | |||
| Namespace: caNamespace, | |||
| Name: "scaled_down_gpu_nodes_total", | |||
| Help: "Number of GPU nodes removed by CA, by reason and GPU name.", | |||
| }, []string{"reason", "gpu_name"}, | |||
| }, []string{"reason", "resource_name", "gpu_name"}, | |||
There was a problem hiding this comment.
Do you know if it's safe to add a new label in the middle of the existing ones (e.g. I could imagine that some metric collector could treat earlier "gpu_name" values as "resource_name" after new 3-value metrics are emitted)?
There was a problem hiding this comment.
it's safe, it should be exposed in prometheus format with label names, so that metric collectors have no trouble identifying the right label
cluster-autoscaler/utils/gpu/gpu.go
Outdated
|
|
||
| // no signs of GPU | ||
| return MetricsNoGPU | ||
| func GetGpuTypeForMetrics(gpuConfig *cloudprovider.GpuConfig, availableGPUTypes map[string]struct{}, node *apiv1.Node, nodeGroup cloudprovider.NodeGroup) (string, string) { |
There was a problem hiding this comment.
The name of the function no longer reflects the returned values, and it's hard to figure out what's returned just from reading the signature. Maybe we could make the name more generic (GetGpuInfoForMetrics?), and name the return values instead?
| return MetricsNoGPU | ||
| func GetGpuTypeForMetrics(gpuConfig *cloudprovider.GpuConfig, availableGPUTypes map[string]struct{}, node *apiv1.Node, nodeGroup cloudprovider.NodeGroup) (string, string) { | ||
| // There is no sign of GPU | ||
| if gpuConfig == nil { |
There was a problem hiding this comment.
nit: It took me a bit to figure out that the PR doesn't change behavior for the existing GPU logic (there's still 1 difference - this function only looks at capacity, while GetNodeGpuConfig utilizes NodeHasGpu which looks at allocatable - but capacity and allocatable should be in sync for GPUs, and allocatable is arguably more correct - so it looks fine to me). This function could really use a unit test, if you're up for it.
There was a problem hiding this comment.
This is a little bit tricky, started writing a test but I found out that previous PR #5459 introduced a "hidden" import cycle (so far the cycle happens only if you use the cloudprovider test package). If you're OK with it, I'd prefer to follow up in the next PR
There was a problem hiding this comment.
The cycle will probably have to be solved sooner or later, but a follow-up SGTM.
| return MetricsNoGPU | ||
| func GetGpuTypeForMetrics(gpuConfig *cloudprovider.GpuConfig, availableGPUTypes map[string]struct{}, node *apiv1.Node, nodeGroup cloudprovider.NodeGroup) (string, string) { | ||
| // There is no sign of GPU | ||
| if gpuConfig == nil { |
There was a problem hiding this comment.
The cycle will probably have to be solved sooner or later, but a follow-up SGTM.
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kawych, towca The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…nodes_total metrics * Added the new resource_name field to scaled_up/down_gpu_nodes_total, representing the resource name for the gpu. * Changed metrics registrations to use GpuConfig
k8s-ci-robot
removed
the
lgtm
|
/unhold |
k8s-ci-robot
added
lgtm
What type of PR is this?
/kind feature
What this PR does / why we need it:
Add a resource_name field to scaled_up/down_gpu_nodes_total to differentiate between different types of GPU, which are represented by different custom resource. Credit to @hbostan for the implementation.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: