[Feature] Enable prometheus metrics for local queues #2516
Conversation
k8s-ci-robot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: varshaprasad96 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: yes
This PR introduces an enhancement to enable collection of prometheus metrics for local queues. Addresses issue: kubernetes-sigs#1833 Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
k8s-ci-robot
added
the
size/L
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
varshaprasad96
force-pushed
the
kep-local-queue-metrics
branch
from
b73eee4
to
49f4265
Compare
|
@astefanutti @alculquicondor @tenzen-y Could you please take a look at the proposal and provide your inputs. Thank you! |
|
/assign @PBundyra |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: b3460149244f8070ae015034e8bba83771759ecc
|
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
is this ready for a pass from approvers? |
Yes |
k8s-ci-robot
removed
the
lgtm
|
New changes are detected. LGTM label has been removed. |
| because they are global and cannot be filtered by namespaces. Furthermore, accessing cluster-scoped metrics | ||
| in secured Prometheus instances is generally restricted to cluster admin users with cluster level permissions across all namespaces and | ||
| tenants. This restriction makes it challenging for batch users to obtain the specific metrics they need for effective workload | ||
| management and to gain insights into their workloads within their limited scope of access. |
There was a problem hiding this comment.
Could this KEP also specify how we will prevent users with insufficient permission accessing metrics they shouldn't be able to?
There was a problem hiding this comment.
If this proposal is not suggesting how to prevent access from namespaces that shouldn't have the permission, then I would remove this phrase.
There was a problem hiding this comment.
I agree, maybe we could narrow the scope of this KEP @varshaprasad96 ? At first glance managing permissions seems to be challenging, or it would require external mechanism
There was a problem hiding this comment.
Sorry for the late reply! I've been considering various options, and it's clear that publishing local queue metrics in each namespace could lead to complications. This approach would require multiple endpoints with respective Service Monitors or, if using a single central Service Monitor, we would need the correct RBAC setup to allow client access. The centralised approach could still poses some issues, such as namespace admins potentially being able to view metrics from other namespaces if not provided with right SA.
One potential solution for cluster admins is to scaffold out a Service Monitor (SM) with metrics labeling for specific namespaces from the same service endpoint. This would enable a common service endpoint for all local queues. Admins could then provide a service account with right RBAC to specific batch user, restricting their access to that particular service monitor.
However, this solution is difficult to implement in Kueue right away by figuring out the right set of scaffolds, and seems to be the responsibility of the cluster admin. This could probably be a customisation which can be documented for now.
That being said, given the complexity, this topic may probably warrant a further brainstorming and deserves a separate KEP. For now, I'll remove this reference and update it to indicate that for this iteration metrics will be exported in the controller namespace, alongside cluster queue metrics, at the same endpoint. Does that sound reasonable?
There was a problem hiding this comment.
Yes it does.
You can remove the per-namespace topic from the motivation and add a note in non-goals.
| NamespaceSelector *metav1.LabelSelector `json:"namespaceSelector,omitempty"` | ||
|
|
||
| // QueueSelector can be used to choose the local queues that need metrics to be collected. | ||
| QueueSelector *metav1.LabelSelector `json:"queueSelector,omitempty"` |
There was a problem hiding this comment.
What happens if the below situations?
- only namespeceSelector is specified
- only localQueueSelector is specified
- both namespaceSelector and localQueueSelector are specified
There was a problem hiding this comment.
Had a predicate based filtering kind of approach for this case.
- Only NS selector: All LocalQueues in the specific namespaces are selected. If it's nil, all the local queues across all namespaces are considered.
- LocalQueue selector: Local queues's matching the labels are selected across all namespaces. If not, all the local queues across namespaces are considered.
- If both are specified: Both the selectors are applied to local queues (logical AND).
Had a Venn diagram in mind :)) Please let me know if I'm missing anything.
There was a problem hiding this comment.
Have added a configuration table for this knob in the KEP.
|
|
||
| In the first iteration, following are the list of metrics that would contain information on Local Queue statuses: | ||
|
|
||
| | Metrics Name | Prometheus Type | Description | |
There was a problem hiding this comment.
A part of the clusterqueue metrics is imported here like local_queue_status. Is there any reason that you want to drop a few metrics rather than clusterqueue ones?
There was a problem hiding this comment.
Updated the list to contain all the local_queue relevant ones. One additional metric is the local_queue_wait_time_seconds which is a sum of admission and reserved wait time.
varshaprasad96
force-pushed
the
kep-local-queue-metrics
branch
from
e86c944
to
bc697cc
Compare
This commit addresses reviews by adding additional metrics for local queue. Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
varshaprasad96
force-pushed
the
kep-local-queue-metrics
branch
from
bc697cc
to
15ef983
Compare
|
@PBundyra @alculquicondor @tenzen-y I've updated the proposal based on the reviews. Please take a look when you get a chance. Thank you! |
| | True | - | Specified | All LocalQueues matching the label selector have metrics enabled. | | ||
| | True | - | Specified | All LocalQueues matching the label selector have metrics enabled. | |
| | local_queue_admission_wait_time_seconds | Histogram | The time from when a workload got the quota reservation until admission in local queue. | | ||
| | local_queue_reserved_wait_time_seconds | Histogram | The time between a workload was created or re-queued until it got quota reservation in local queue. | | ||
| | local_queue_wait_time_seconds | Histogram | Time taken to accept the resources from cluster queue. | | ||
| | local_admission_checks_wait_time_seconds | Histogram | The time from when a workload got the quota reservation until admission. | |
There was a problem hiding this comment.
Could you please clarify the differences between local_queue_admission_wait_time_seconds and local_admission_checks_wait_time_seconds? Maybe local_queue_admission_wait_time_seconds could describe time from creating (or requeueing) a Workload until admission - similarly to the clusterqueue metrics. In that case it would cover the local_queue_wait_time_seconds metric usage.
What type of PR is this?
/kind feature
/kind documentation
What this PR does / why we need it:
This PR introduces an enhancement to enable collection of prometheus metrics for local queues.
Addresses issue: #1833
Which issue(s) this PR fixes:
Fixes # Partially #1833
Special notes for your reviewer:
Does this PR introduce a user-facing change?