Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

resource_alerts: drop redundant KSM selector from KubeCPUOvercommit #947

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

rexagod
Copy link
Collaborator

@rexagod rexagod commented Jun 24, 2024

Fixes the alert KubeCPUQuotaOvercommit not firing as expected.

cc @simonpasquier

Copy link
Contributor

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather drop the "job" label selector on namespace_memory:kube_pod_container_resource_limits:sum from the KubeCPUOverCommit expression.

@rexagod rexagod changed the title rules: expose job label for namespace_memory:kube_pod_container_resource_limits:sum rules: expose job label for namespace_cpu:kube_pod_container_resource_requests:sum Jun 26, 2024
Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>
@rexagod rexagod changed the title rules: expose job label for namespace_cpu:kube_pod_container_resource_requests:sum resource_alerts: drop redundant KSM selector from KubeCPUOvercommit Jun 26, 2024
@rexagod
Copy link
Collaborator Author

rexagod commented Jun 26, 2024

ACK, I noticed the outer sum will drop the labels anyway (except cluster), and since the innermost metric already respects the KSM selector, the outermost KSM selector is redundant. I missed this earlier, fixed.

@povilasv
Copy link
Contributor

povilasv commented Jul 3, 2024

Could you elaborate why drop of this selector fixes something?

I feel like job label is the required one for all the dashboards / rules / alerts, so that you can correctly select the Prometheus job.
It's also mentioned in the spec https://github.com/kubernetes-monitoring/kubernetes-mixin/blob/master/DESIGN.md#proposal

@rexagod
Copy link
Collaborator Author

rexagod commented Jul 8, 2024

👋🏼 Thank you for looking at this, @povilasv! It's essentially the fact that namespace_cpu:kube_pod_container_resource_requests:sum has no job labels for kubeStateMetricsSelector to be respected by, so the current expression yields no metrics, which ends up affecting the alert this is used in.

However, I'd like to explicitly mention that kubeStateMetricsSelector is still respected in the inner expressions pre-filtering for the same metric.

@rexagod
Copy link
Collaborator Author

rexagod commented Jul 17, 2024

Gentle ping, @povilasv. 🙂

@povilasv
Copy link
Contributor

understood, thanks!

@povilasv povilasv merged commit 2640a3e into kubernetes-monitoring:master Jul 18, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants