Add metric for admitted active workloads #291
Conversation
k8s-ci-robot
added
kind/feature
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor 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 |
k8s-ci-robot
added
approved
alculquicondor
force-pushed
the
admitted_metrics
branch
3 times, most recently
from
50227bb
to
eb7103d
Compare
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
alculquicondor
force-pushed
the
admitted_metrics
branch
from
eb7103d
to
d75b5d7
Compare
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
pkg/metrics/metrics.go
Outdated
| Subsystem: subsystemName, | ||
| Name: "admitted_active_workloads", | ||
| Help: "Number of admitted workloads that are active (unsuspended and not finished)", | ||
| }, []string{"cluster_queue", "queue"}, |
There was a problem hiding this comment.
Since users can create as many queues as they wish, aren't we concerned about this dimension getting too big? do you remember what the limits where?
There was a problem hiding this comment.
There is no limit. The more queues there are, the more memory we use. Let me try to get more information on how bad this could be for a gauge.
Note that when a queue or clusterQueue is removed, I'm deleting the label values .
There was a problem hiding this comment.
We chatted offline and concluded that it's best to only keep track of active workloads per clusterqueue, similarly to #293
k8s-ci-robot
added
the
needs-rebase
alculquicondor
force-pushed
the
admitted_metrics
branch
from
d75b5d7
to
a258ba7
Compare
k8s-ci-robot
removed
the
needs-rebase
alculquicondor
force-pushed
the
admitted_metrics
branch
2 times, most recently
from
006151a
to
9ef94e9
Compare
pkg/workload/workload.go
Outdated
| @@ -34,7 +34,8 @@ type Info struct { | |||
| Obj *kueue.Workload | |||
| // list of total resources requested by the podsets. | |||
| TotalRequests []PodSetResources | |||
| // Populated from queue. | |||
| // Populated from the queue during admission of from the admission field if | |||
| Obj: w, | ||
| TotalRequests: totalRequests(&w.Spec), | ||
| } | ||
| if w.Spec.Admission != nil { | ||
| info.ClusterQueue = string(w.Spec.Admission.ClusterQueue) |
There was a problem hiding this comment.
No, we were only using this field during scheduling, where the information comes from the queues, rather than the Workload.
Now I'm populating the field when adding the workload to the cache, for consistency and convenience (and to avoid potential bugs in the future).
Change-Id: I6f1607f6c3560d83e85e19976a986e638ea5a8b0
alculquicondor
force-pushed
the
admitted_metrics
branch
from
9ef94e9
to
0abbc39
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
What type of PR is this?
/kind feature
What this PR does / why we need it:
Add a metric that keeps track of admitted workloads that are still active (running).
This requires keeping track of admitted workloads per queue in the cache, which can also be used to report queue status.
Which issue(s) this PR fixes:
Part of #199
Special notes for your reviewer:
The metric will under-report active workloads if the queue is deleted, or the queue switches cluster queue, which should be disallowed with finalizers and webhooks #171