Repository navigation
Add kueue_pod_scheduling_gate_removal_seconds metric. - #12137
kubernetes-prow[bot] merged 1 commit into
Conversation
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
release-note
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds a new Prometheus histogram metric ChangesPod Scheduling Gate Removal Latency Metric
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
k8s-ci-robot
added
size/L
|
/test all |
1 similar comment
|
/test all |
| | --- | --- | --- | --- | | ||
| | `kueue_admission_attempt_duration_seconds` | Histogram | The latency of an admission attempt.<br>The label 'result' can have the following values:<br>- 'success' means that at least one workload was admitted.,<br>- 'inadmissible' means that no workload was admitted. | `result`: possible values are `success` or `inadmissible`<br> `replica_role`: one of `leader`, `follower`, or `standalone` | | ||
| | `kueue_admission_attempts_total` | Counter | The total number of attempts to admit workloads.<br>Each admission attempt might try to admit more than one workload.<br>The label 'result' can have the following values:<br>- 'success' means that at least one workload was admitted.,<br>- 'inadmissible' means that no workload was admitted. | `result`: possible values are `success` or `inadmissible`<br> `replica_role`: one of `leader`, `follower`, or `standalone` | | ||
| | `kueue_pod_scheduling_gate_removal_seconds` | Histogram | The time between workload admission and scheduling gate removal. | `cluster_queue`: the name of the ClusterQueue<br> `is_group`: whether the gate removal applies to a pod group or a single pod | |
There was a problem hiding this comment.
How are multi-pod metrics handled? Does every pod record a sample or there is an aggregated one for the entire workload? Why is is_group needed?
There was a problem hiding this comment.
How are multi-pod metrics handled? Does every pod record a sample or there is an aggregated one for the entire workload?
We record a single sample when the entire group is unscheduled while running.
Why is is_group needed?
This is required in issue.
bc53d0a to
a075143
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mbobrovskyi The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
a075143 to
8923e94
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
pkg/metrics/metrics.go (1)
581-587: ⚡ Quick winUse explicit buckets for minute-scale gate-removal latency.
Line 581 currently relies on default histogram buckets, which are coarse for the minutes-long delays described in the PR objective. Defining explicit buckets here would make this metric more actionable.
Suggested fix
PodSchedulingGateRemovalSeconds = prometheus.NewHistogramVec( prometheus.HistogramOpts{ Subsystem: constants.KueueName, Name: "pod_scheduling_gate_removal_seconds", Help: "The time between workload admission and scheduling gate removal.", + Buckets: generateExponentialBuckets(14), }, append([]string{"cluster_queue", "is_group"}, extraLabels...), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/metrics/metrics.go` around lines 581 - 587, The PodSchedulingGateRemovalSeconds histogram is relying on Prometheus default buckets which are too coarse for measuring minute-scale latencies. Add an explicit Buckets field to the prometheus.HistogramOpts struct (within the prometheus.NewHistogramVec call) to define custom bucket boundaries appropriate for minute-scale gate-removal delays measured in seconds. Choose bucket values that provide granular visibility across the expected latency range.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/jobs/pod/pod_controller.go`:
- Around line 330-332: The error handling block for the parallelize.Until call
in pod_controller.go currently suppresses the error by returning nil, which
masks pod-update failures and misleads callers into thinking the operation
succeeded. Replace the nil return value with the actual error variable (err) so
that failures are properly propagated to the caller, allowing the reconciliation
logic to handle and report errors correctly according to controller guidelines.
- Around line 340-341: In the pod_controller.go reconciliation logic, the
latency calculation on line 340 using
p.clock.Now().Sub(cond.LastTransitionTime.Time) can produce negative values due
to clock skew between the API server and controller clocks. Clamp the latency to
a non-negative value (minimum of 0) immediately after calculating it, before
passing the latency to the metrics.RecordPodSchedulingGateRemovalSeconds() call
on line 341. This ensures the histogram metric always receives valid
non-negative duration samples regardless of clock skew conditions.
In `@pkg/metrics/metrics.go`:
- Line 1405: The new PodSchedulingGateRemovalSeconds metric registered at line
1405 is not being cleared in the ClearClusterQueueMetrics function. To fix this,
locate the ClearClusterQueueMetrics function and add code to delete the
cluster_queue series for the PodSchedulingGateRemovalSeconds metric, ensuring it
follows the same cleanup pattern used for other cluster queue metrics in that
function to prevent stale series from persisting after queue lifecycle events.
- Around line 203-204: The metricsdoc comment for
PodSchedulingGateRemovalSeconds documents three labels (cluster_queue, is_group,
and name), but the histogram vector is only created with cluster_queue and
is_group labels. Remove the `name` label documentation from the metricsdoc
comment at line 203, or alternatively add the `name` label to the histogram
vector creation when PodSchedulingGateRemovalSeconds is initialized (at line
586) to match the documented labels. Ensure the documented labels match the
actual labels used when creating the histogram vector.
In `@test/util/metrics.go`:
- Around line 383-387: The
ExpectPodSchedulingGateRemovalSecondsMetricLessOrEqual function currently uses
expectHistogramMetric with a numerical matcher to compare histogram sample count
instead of actual latency values, making the assertion meaningless. Replace the
expectHistogramMetric call with logic that directly reads the histogram buckets
by calling metric.WithLabelValues with the cqName and isGroup labels, then
collects the metric and verifies that the cumulative bucket count up to
duration.Seconds() equals the total sample count, confirming all observations
fall within the specified threshold.
---
Nitpick comments:
In `@pkg/metrics/metrics.go`:
- Around line 581-587: The PodSchedulingGateRemovalSeconds histogram is relying
on Prometheus default buckets which are too coarse for measuring minute-scale
latencies. Add an explicit Buckets field to the prometheus.HistogramOpts struct
(within the prometheus.NewHistogramVec call) to define custom bucket boundaries
appropriate for minute-scale gate-removal delays measured in seconds. Choose
bucket values that provide granular visibility across the expected latency
range.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cc0388da-4871-4f5a-9a6d-c0da573cb974
📒 Files selected for processing (8)
pkg/controller/jobframework/interface.gopkg/controller/jobframework/reconciler.gopkg/controller/jobs/pod/pod_controller.gopkg/controller/jobs/pod/pod_controller_test.gopkg/metrics/metrics.gosite/content/en/docs/reference/metrics.mdtest/integration/singlecluster/controller/jobs/pod/pod_controller_test.gotest/util/metrics.go
| latency := p.clock.Now().Sub(cond.LastTransitionTime.Time) | ||
| metrics.RecordPodSchedulingGateRemovalSeconds(wl.Status.Admission.ClusterQueue, p.isGroup, latency) |
There was a problem hiding this comment.
Clamp negative latency before recording the histogram sample.
Line 340 derives latency from API-server condition time vs controller clock; clock skew can yield negative durations. Record a non-negative value before observing.
As per coding guidelines, pkg/controller/**/*.go reconciliation logic should preserve correctness for runtime edge cases.
Suggested fix
- latency := p.clock.Now().Sub(cond.LastTransitionTime.Time)
+ latency := p.clock.Now().Sub(cond.LastTransitionTime.Time)
+ if latency < 0 {
+ latency = 0
+ }
metrics.RecordPodSchedulingGateRemovalSeconds(wl.Status.Admission.ClusterQueue, p.isGroup, latency)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| latency := p.clock.Now().Sub(cond.LastTransitionTime.Time) | |
| metrics.RecordPodSchedulingGateRemovalSeconds(wl.Status.Admission.ClusterQueue, p.isGroup, latency) | |
| latency := p.clock.Now().Sub(cond.LastTransitionTime.Time) | |
| if latency < 0 { | |
| latency = 0 | |
| } | |
| metrics.RecordPodSchedulingGateRemovalSeconds(wl.Status.Admission.ClusterQueue, p.isGroup, latency) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/controller/jobs/pod/pod_controller.go` around lines 340 - 341, In the
pod_controller.go reconciliation logic, the latency calculation on line 340
using p.clock.Now().Sub(cond.LastTransitionTime.Time) can produce negative
values due to clock skew between the API server and controller clocks. Clamp the
latency to a non-negative value (minimum of 0) immediately after calculating it,
before passing the latency to the
metrics.RecordPodSchedulingGateRemovalSeconds() call on line 341. This ensures
the histogram metric always receives valid non-negative duration samples
regardless of clock skew conditions.
Source: Coding guidelines
| func ExpectPodSchedulingGateRemovalSecondsMetricLessOrEqual(cqName kueue.ClusterQueueReference, isGroup bool, duration time.Duration) { | ||
| ginkgo.GinkgoHelper() | ||
| matcher := gomega.BeNumerically("<=", duration.Seconds()) | ||
| expectHistogramMetric(metrics.PodSchedulingGateRemovalSeconds, matcher, string(cqName), fmt.Sprintf("%t", isGroup)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the current helper compares duration to sample COUNT (not observed latency values).
rg -n 'ExpectPodSchedulingGateRemovalSecondsMetricLessOrEqual|expectHistogramMetric|GetHistogramMetricCount|BeNumerically\("<="' test/util/metrics.goRepository: kubernetes-sigs/kueue
Length of output: 2397
🏁 Script executed:
sed -n '313,320p' test/util/metrics.goRepository: kubernetes-sigs/kueue
Length of output: 450
🏁 Script executed:
# Check what GetHistogramMetricCount is and how it's used in other test helpers
rg -n 'GetHistogramMetric|prometheus\.HistogramVec' test/util/metrics.go | head -30Repository: kubernetes-sigs/kueue
Length of output: 262
🌐 Web query:
k8s.io/component-base/metrics/testutil GetHistogramMetricCount documentation
💡 Result:
GetHistogramMetricCount is implemented in k8s.io/component-base/metrics/testutil as a helper that writes the provided ObserverMetric into a dto.Metric protobuf and returns the histogram’s SampleCount. Specifically, the function: 1) creates metricProto:= &dto.Metric{} 2) calls m.(metrics.Metric).Write(metricProto) 3) returns metricProto.Histogram.GetSampleCount as (uint64, nil), or (0, error) if Write fails [1]. So “HistogramMetricCount” (as exposed by this helper) corresponds to the Prometheus client’s histogram “count” / the histogram proto field sample_count (total number of observations) stored in dto.Histogram.SampleCount [1].
Citations:
🏁 Script executed:
# Check if there are other patterns for asserting histogram latency/buckets in the codebase
rg -n 'histogram|bucket|Bucket|SampleCount' test/util/metrics.go -iRepository: kubernetes-sigs/kueue
Length of output: 2448
Use a latency-aware assertion instead of sample count comparison.
The new helper at line 383–386 attempts to assert that pod scheduling gate removal latency is <= duration, but expectHistogramMetric compares the histogram sample count (total observations), not observed latency values. Passing gomega.BeNumerically("<=", duration.Seconds()) with a sample count yields a meaningless comparison (e.g., "observation count ≤ 5.0 seconds") that silently passes with poor latency or fails for valid sub-second thresholds.
To validate latency bounds, inspect histogram buckets directly instead of sample count. For example, read metric.WithLabelValues(...).Collect() and verify that the cumulative bucket count up to the target latency equals the total sample count, confirming all observations fall within the threshold.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/util/metrics.go` around lines 383 - 387, The
ExpectPodSchedulingGateRemovalSecondsMetricLessOrEqual function currently uses
expectHistogramMetric with a numerical matcher to compare histogram sample count
instead of actual latency values, making the assertion meaningless. Replace the
expectHistogramMetric call with logic that directly reads the histogram buckets
by calling metric.WithLabelValues with the cqName and isGroup labels, then
collects the metric and verifies that the cumulative bucket count up to
duration.Seconds() equals the total sample count, confirming all observations
fall within the specified threshold.
583f7da to
69c74de
Compare
mbobrovskyi
changed the title
k8s-ci-robot
added
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/integration/singlecluster/controller/jobs/pod/pod_controller_test.go (1)
140-216:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid absolute histogram assertions on shared labels; this can make the spec flaky.
At Line 174, asserting
<= 0forcluster_queue="cluster-queue"depends on prior suite state for the same label set, and at Line 216, a hard<= 1sbound can fail under CI load even when behavior is correct. Prefer isolating labels per test and using a less brittle upper bound.Suggested adjustment
- const cqName = "cluster-queue" + cqName := kueue.ClusterQueueReference(fmt.Sprintf("cluster-queue-%s", ns.Name)) ... - util.ExpectPodSchedulingGateRemovalSecondsMetricLessOrEqual(cqName, false, 0) + util.ExpectPodSchedulingGateRemovalSecondsMetricLessOrEqual(cqName, false, 0) ... - clusterQueue := utiltestingapi.MakeClusterQueue(cqName). + clusterQueue := utiltestingapi.MakeClusterQueue(string(cqName)). ResourceGroup( *utiltestingapi.MakeFlavorQuotas("default").Resource(corev1.ResourceCPU, "1").Obj(), ).Obj() ... - util.ExpectPodSchedulingGateRemovalSecondsMetricLessOrEqual(cqName, false, time.Second) + util.ExpectPodSchedulingGateRemovalSecondsMetricLessOrEqual(cqName, false, util.Timeout)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/integration/singlecluster/controller/jobs/pod/pod_controller_test.go` around lines 140 - 216, The test contains two assertions on shared histogram metrics that depend on prior suite state and use brittle time bounds, making the test flaky. The calls to util.ExpectPodSchedulingGateRemovalSecondsMetricLessOrEqual at line 174 (asserting <= 0 seconds) and line 216 (asserting <= 1 second with cqName "cluster-queue") can fail due to shared label state and CI load variations. Either remove these two metric assertions entirely or replace them with assertions using isolated labels per test and significantly higher upper bounds that are less susceptible to timing variations under load.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/metrics/metrics.go`:
- Around line 920-922: In the RecordPodSchedulingGateRemovalSeconds function,
clamp the latency value to zero before passing it to the Observe method to
prevent negative duration samples from being recorded in the histogram. This can
occur due to clock skew or delivery delays, and should follow the same pattern
that is already implemented for eviction latency handling elsewhere in the
metrics file. Check if the latency is negative and set it to zero before calling
the Observe method with latency.Seconds().
- Around line 581-587: The PodSchedulingGateRemovalSeconds histogram declaration
includes extraLabels in its label set, but the
RecordPodSchedulingGateRemovalSeconds function only provides two label values
via WithLabelValues, causing a cardinality mismatch that will panic if custom
labels are configured. Fix this by either: (1) removing the
append([]string{"cluster_queue", "is_group"}, extraLabels...) and using only the
base labels in the histogram declaration, or (2) adding a customLabelValues
[]string parameter to RecordPodSchedulingGateRemovalSeconds and passing it
through to WithLabelValues alongside the "cluster_queue" and "is_group" values,
following the same pattern as RecordWorkloadCreationLatency and
QuotaReservedWorkload metrics.
---
Outside diff comments:
In `@test/integration/singlecluster/controller/jobs/pod/pod_controller_test.go`:
- Around line 140-216: The test contains two assertions on shared histogram
metrics that depend on prior suite state and use brittle time bounds, making the
test flaky. The calls to
util.ExpectPodSchedulingGateRemovalSecondsMetricLessOrEqual at line 174
(asserting <= 0 seconds) and line 216 (asserting <= 1 second with cqName
"cluster-queue") can fail due to shared label state and CI load variations.
Either remove these two metric assertions entirely or replace them with
assertions using isolated labels per test and significantly higher upper bounds
that are less susceptible to timing variations under load.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8d6e718b-b6b7-41d0-8c2a-162bf93ec310
📒 Files selected for processing (8)
pkg/controller/jobframework/interface.gopkg/controller/jobframework/reconciler.gopkg/controller/jobs/pod/pod_controller.gopkg/controller/jobs/pod/pod_controller_test.gopkg/metrics/metrics.gosite/content/en/docs/reference/metrics.mdtest/integration/singlecluster/controller/jobs/pod/pod_controller_test.gotest/util/metrics.go
✅ Files skipped from review due to trivial changes (1)
- site/content/en/docs/reference/metrics.md
🚧 Files skipped from review as they are similar to previous changes (4)
- test/util/metrics.go
- pkg/controller/jobframework/interface.go
- pkg/controller/jobs/pod/pod_controller_test.go
- pkg/controller/jobs/pod/pod_controller.go
| PodSchedulingGateRemovalSeconds = prometheus.NewHistogramVec( | ||
| prometheus.HistogramOpts{ | ||
| Subsystem: constants.KueueName, | ||
| Name: "pod_scheduling_gate_removal_seconds", | ||
| Help: "The time between workload admission and scheduling gate removal.", | ||
| }, append([]string{"cluster_queue", "is_group"}, extraLabels...), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Metric declaration (check variable labels):"
rg -n -C2 'PodSchedulingGateRemovalSeconds = prometheus.NewHistogramVec|append\(\[]string\{"cluster_queue", "is_group"\}, extraLabels\.\.\.\)' pkg/metrics/metrics.go
echo
echo "2) Recorder helper (check label values provided):"
rg -n -C2 'func RecordPodSchedulingGateRemovalSeconds|WithLabelValues\(' pkg/metrics/metrics.go
echo
echo "3) Call sites (to validate what can be threaded):"
rg -n --type go 'RecordPodSchedulingGateRemovalSeconds\('Repository: kubernetes-sigs/kueue
Length of output: 18321
🏁 Script executed:
rg -n 'extraLabels' pkg/metrics/metrics.go | head -20Repository: kubernetes-sigs/kueue
Length of output: 1872
🏁 Script executed:
grep -n 'var extraLabels\|const extraLabels\|extraLabels :=' pkg/metrics/metrics.goRepository: kubernetes-sigs/kueue
Length of output: 47
🏁 Script executed:
rg -n 'extraLabels' --type go -B2 -A2Repository: kubernetes-sigs/kueue
Length of output: 35750
Prevent label-count panic in RecordPodSchedulingGateRemovalSeconds.
The histogram declaration at lines 581-587 includes extraLabels in its label set, but the recorder function at lines 920-921 only provides two label values via WithLabelValues. If custom labels are configured, this cardinality mismatch will cause a panic at runtime.
Align the metric and recorder: either remove extraLabels from the histogram declaration (since this metric doesn't currently support custom labels), or add a customLabelValues []string parameter to RecordPodSchedulingGateRemovalSeconds and thread it through to match the pattern used by similar metrics like RecordWorkloadCreationLatency and QuotaReservedWorkload.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/metrics/metrics.go` around lines 581 - 587, The
PodSchedulingGateRemovalSeconds histogram declaration includes extraLabels in
its label set, but the RecordPodSchedulingGateRemovalSeconds function only
provides two label values via WithLabelValues, causing a cardinality mismatch
that will panic if custom labels are configured. Fix this by either: (1)
removing the append([]string{"cluster_queue", "is_group"}, extraLabels...) and
using only the base labels in the histogram declaration, or (2) adding a
customLabelValues []string parameter to RecordPodSchedulingGateRemovalSeconds
and passing it through to WithLabelValues alongside the "cluster_queue" and
"is_group" values, following the same pattern as RecordWorkloadCreationLatency
and QuotaReservedWorkload metrics.
| func RecordPodSchedulingGateRemovalSeconds(clusterQueue kueue.ClusterQueueReference, isGroup bool, latency time.Duration) { | ||
| PodSchedulingGateRemovalSeconds.WithLabelValues(string(clusterQueue), strconv.FormatBool(isGroup)).Observe(latency.Seconds()) | ||
| } |
There was a problem hiding this comment.
Clamp negative latency before observing this histogram.
This path measures API transition time vs controller clock, so skew/delivery delay can yield negative durations (you already handle this for eviction latency around Line 1017). Clamp to zero before Observe to avoid invalid latency samples.
Suggested patch
func RecordPodSchedulingGateRemovalSeconds(clusterQueue kueue.ClusterQueueReference, isGroup bool, latency time.Duration) {
- PodSchedulingGateRemovalSeconds.WithLabelValues(string(clusterQueue), strconv.FormatBool(isGroup)).Observe(latency.Seconds())
+ seconds := max(0, latency.Seconds())
+ PodSchedulingGateRemovalSeconds.WithLabelValues(string(clusterQueue), strconv.FormatBool(isGroup)).Observe(seconds)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func RecordPodSchedulingGateRemovalSeconds(clusterQueue kueue.ClusterQueueReference, isGroup bool, latency time.Duration) { | |
| PodSchedulingGateRemovalSeconds.WithLabelValues(string(clusterQueue), strconv.FormatBool(isGroup)).Observe(latency.Seconds()) | |
| } | |
| func RecordPodSchedulingGateRemovalSeconds(clusterQueue kueue.ClusterQueueReference, isGroup bool, latency time.Duration) { | |
| seconds := max(0, latency.Seconds()) | |
| PodSchedulingGateRemovalSeconds.WithLabelValues(string(clusterQueue), strconv.FormatBool(isGroup)).Observe(seconds) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/metrics/metrics.go` around lines 920 - 922, In the
RecordPodSchedulingGateRemovalSeconds function, clamp the latency value to zero
before passing it to the Observe method to prevent negative duration samples
from being recorded in the histogram. This can occur due to clock skew or
delivery delays, and should follow the same pattern that is already implemented
for eviction latency handling elsewhere in the metrics file. Check if the
latency is negative and set it to zero before calling the Observe method with
latency.Seconds().
69c74de to
83f27cd
Compare
mbobrovskyi
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
83f27cd to
a38c69b
Compare
cab3a47 to
e696192
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pkg/controller/jobs/pod/pod_controller.go (2)
334-341:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClamp negative latency before recording the histogram sample.
Line 339 derives latency from API-server condition time vs controller clock; clock skew can yield negative durations. Record a non-negative value before observing.
As per coding guidelines,
pkg/controller/**/*.goreconciliation logic should preserve correctness for runtime edge cases.Suggested fix
latency := p.clock.Now().Sub(cond.LastTransitionTime.Time) + if latency < 0 { + latency = 0 + } metrics.RecordPodSchedulingGateRemovalSeconds(podconstants.SchedulingGateName, wl.Status.Admission.ClusterQueue, p.isGroup, latency)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/jobs/pod/pod_controller.go` around lines 334 - 341, In the recordPodSchedulingGateRemovalSeconds function, the latency calculation derived from clock time subtraction can produce negative values due to clock skew between the API server and controller. Clamp the latency to a non-negative minimum value (zero) before passing it to the metrics.RecordPodSchedulingGateRemovalSeconds call to ensure the histogram metric receives valid duration samples. Apply a max operation on the latency variable to ensure it never drops below zero.Source: Coding guidelines
267-267:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard workload/admission dereferences before recording the metric.
Line 267 accepts a pointer workload, but Line 340 dereferences
wl.Status.Admission.ClusterQueuewithout validatingwlorwl.Status.Admission. A nil workload (or missing admission block) will panic the reconciler path.Suggested fix
func (p *Pod) Run(ctx context.Context, c client.Client, wl *kueue.Workload, podSetsInfo []podset.PodSetInfo, recorder events.EventRecorder, msg string) error { + if wl == nil { + return fmt.Errorf("workload is required") + } log := ctrl.LoggerFrom(ctx)func (p *Pod) recordPodSchedulingGateRemovalSeconds(wl *kueue.Workload) { + if wl == nil || wl.Status.Admission == nil { + return + } cond := apimeta.FindStatusCondition(wl.Status.Conditions, kueue.WorkloadAdmitted) if cond == nil || cond.Status != metav1.ConditionTrue { return } latency := p.clock.Now().Sub(cond.LastTransitionTime.Time) metrics.RecordPodSchedulingGateRemovalSeconds(podconstants.SchedulingGateName, wl.Status.Admission.ClusterQueue, p.isGroup, latency) }Also applies to: 334-341
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/jobs/pod/pod_controller.go` at line 267, The Run function accepts a workload pointer parameter but at line 340 dereferences wl.Status.Admission.ClusterQueue without first validating that wl is not nil and that wl.Status.Admission exists. Add nil checks to guard against dereferencing a nil workload or missing admission status before accessing the ClusterQueue field in the metric recording code around lines 334-341. Ensure the validation occurs before any attempt to access nested fields of the workload to prevent runtime panics.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@pkg/controller/jobs/pod/pod_controller.go`:
- Around line 334-341: In the recordPodSchedulingGateRemovalSeconds function,
the latency calculation derived from clock time subtraction can produce negative
values due to clock skew between the API server and controller. Clamp the
latency to a non-negative minimum value (zero) before passing it to the
metrics.RecordPodSchedulingGateRemovalSeconds call to ensure the histogram
metric receives valid duration samples. Apply a max operation on the latency
variable to ensure it never drops below zero.
- Line 267: The Run function accepts a workload pointer parameter but at line
340 dereferences wl.Status.Admission.ClusterQueue without first validating that
wl is not nil and that wl.Status.Admission exists. Add nil checks to guard
against dereferencing a nil workload or missing admission status before
accessing the ClusterQueue field in the metric recording code around lines
334-341. Ensure the validation occurs before any attempt to access nested fields
of the workload to prevent runtime panics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: df1522cc-4c23-4fa2-9300-5ffb4055cab8
📒 Files selected for processing (8)
pkg/controller/jobframework/interface.gopkg/controller/jobframework/reconciler.gopkg/controller/jobs/pod/pod_controller.gopkg/controller/jobs/pod/pod_controller_test.gopkg/metrics/metrics.gosite/content/en/docs/reference/metrics.mdtest/integration/singlecluster/controller/jobs/pod/pod_controller_test.gotest/util/metrics.go
✅ Files skipped from review due to trivial changes (1)
- site/content/en/docs/reference/metrics.md
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/controller/jobframework/interface.go
- test/util/metrics.go
- test/integration/singlecluster/controller/jobs/pod/pod_controller_test.go
- pkg/controller/jobframework/reconciler.go
- pkg/metrics/metrics.go
e696192 to
0a8f1ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/metrics/metrics.go (2)
581-587:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPrevent label-count panic in
RecordPodSchedulingGateRemovalSeconds.The histogram declaration at line 586 includes
extraLabelsin its label set, but the recorder function at lines 920-921 only provides three label values viaWithLabelValues. If custom labels are configured, this cardinality mismatch will cause a panic at runtime.Align the metric and recorder: either remove
extraLabelsfrom the histogram declaration (since this metric doesn't currently support custom labels), or add acustomLabelValues []stringparameter toRecordPodSchedulingGateRemovalSecondsand thread it through to match the pattern used by similar metrics likeRecordWorkloadCreationLatency(lines 915-918) andQuotaReservedWorkload(lines 924-928).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/metrics/metrics.go` around lines 581 - 587, The PodSchedulingGateRemovalSeconds histogram declaration includes extraLabels in its label set, but the RecordPodSchedulingGateRemovalSeconds recorder function only provides three label values via WithLabelValues, creating a cardinality mismatch that will panic at runtime if custom labels are configured. Fix this by either removing extraLabels from the PodSchedulingGateRemovalSeconds histogram declaration (since this metric does not currently support custom labels), or by adding a customLabelValues []string parameter to the RecordPodSchedulingGateRemovalSeconds function and threading it through to the WithLabelValues call, following the same pattern used by RecordWorkloadCreationLatency and RecordQuotaReservedWorkload.
920-922:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClamp negative latency before observing this histogram.
This path measures API transition time versus controller clock, so skew or delivery delay can yield negative durations (the same pattern is already handled for eviction latency at line 1020). Clamp to zero before
Observeto avoid invalid latency samples.🛡️ Proposed fix
func RecordPodSchedulingGateRemovalSeconds(name string, clusterQueue kueue.ClusterQueueReference, isGroup bool, latency time.Duration) { - PodSchedulingGateRemovalSeconds.WithLabelValues(name, string(clusterQueue), strconv.FormatBool(isGroup)).Observe(latency.Seconds()) + seconds := max(0, latency.Seconds()) + PodSchedulingGateRemovalSeconds.WithLabelValues(name, string(clusterQueue), strconv.FormatBool(isGroup)).Observe(seconds) }Verify that the repository's minimum Go version supports the
max()built-in (added in Go 1.21):#!/bin/bash # Check go.mod for minimum Go version echo "=== Checking go.mod for minimum Go version ===" if [ -f go.mod ]; then grep -E '^go [0-9]+\.[0-9]+' go.mod else echo "go.mod not found" fi echo "" echo "=== Checking for existing usage of max() built-in ===" rg -n '\bmax\(' --type go | head -20🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/metrics/metrics.go` around lines 920 - 922, The RecordPodSchedulingGateRemovalSeconds function does not clamp negative latency values before observing them, which can occur due to clock skew or delivery delays. Follow the same pattern already implemented for eviction latency handling (referenced at line 1020) by using the max() built-in function to clamp the latency duration to zero before passing it to the Observe call. Ensure the repository's minimum Go version in go.mod is 1.21 or later to support the max() built-in.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/metrics/metrics.go`:
- Line 203: Update the metricsdoc comment labels clause at line 203 to match the
label order used in the histogram declaration at line 586. Change the label
order from `cluster_queue, is_group, name` to `name, cluster_queue, is_group` to
ensure the documentation comment is consistent with the actual histogram metric
definition and prevents confusion for metric consumers.
---
Duplicate comments:
In `@pkg/metrics/metrics.go`:
- Around line 581-587: The PodSchedulingGateRemovalSeconds histogram declaration
includes extraLabels in its label set, but the
RecordPodSchedulingGateRemovalSeconds recorder function only provides three
label values via WithLabelValues, creating a cardinality mismatch that will
panic at runtime if custom labels are configured. Fix this by either removing
extraLabels from the PodSchedulingGateRemovalSeconds histogram declaration
(since this metric does not currently support custom labels), or by adding a
customLabelValues []string parameter to the
RecordPodSchedulingGateRemovalSeconds function and threading it through to the
WithLabelValues call, following the same pattern used by
RecordWorkloadCreationLatency and RecordQuotaReservedWorkload.
- Around line 920-922: The RecordPodSchedulingGateRemovalSeconds function does
not clamp negative latency values before observing them, which can occur due to
clock skew or delivery delays. Follow the same pattern already implemented for
eviction latency handling (referenced at line 1020) by using the max() built-in
function to clamp the latency duration to zero before passing it to the Observe
call. Ensure the repository's minimum Go version in go.mod is 1.21 or later to
support the max() built-in.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1bdad819-18b7-479b-a2a6-cef3452e2ee4
📒 Files selected for processing (8)
pkg/controller/jobframework/interface.gopkg/controller/jobframework/reconciler.gopkg/controller/jobs/pod/pod_controller.gopkg/controller/jobs/pod/pod_controller_test.gopkg/metrics/metrics.gosite/content/en/docs/reference/metrics.mdtest/integration/singlecluster/controller/jobs/pod/pod_controller_test.gotest/util/metrics.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/controller/jobframework/interface.go
- site/content/en/docs/reference/metrics.md
- test/integration/singlecluster/controller/jobs/pod/pod_controller_test.go
- test/util/metrics.go
- pkg/controller/jobframework/reconciler.go
- pkg/controller/jobs/pod/pod_controller.go
- pkg/controller/jobs/pod/pod_controller_test.go
| WorkloadCreationLatency *prometheus.HistogramVec | ||
|
|
||
| // +metricsdoc:group=health | ||
| // +metricsdoc:labels=cluster_queue="the name of the ClusterQueue",is_group="whether the gate removal applies to a pod group or a single pod",name="one of `kueue.x-k8s.io/topology`, `kueue.x-k8s.io/admission`, or `kueue.x-k8s.io/elastic-job`" |
There was a problem hiding this comment.
Label order in metricsdoc doesn't match histogram declaration.
The metricsdoc comment lists labels as cluster_queue, is_group, name, but the histogram declaration at line 586 uses name, cluster_queue, is_group. While this doesn't affect runtime behavior, it creates documentation inconsistency that can confuse metric consumers.
📝 Align label order
- // +metricsdoc:labels=cluster_queue="the name of the ClusterQueue",is_group="whether the gate removal applies to a pod group or a single pod",name="one of `kueue.x-k8s.io/topology`, `kueue.x-k8s.io/admission`, or `kueue.x-k8s.io/elastic-job`"
+ // +metricsdoc:labels=name="one of `kueue.x-k8s.io/topology`, `kueue.x-k8s.io/admission`, or `kueue.x-k8s.io/elastic-job`",cluster_queue="the name of the ClusterQueue",is_group="whether the gate removal applies to a pod group or a single pod"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/metrics/metrics.go` at line 203, Update the metricsdoc comment labels
clause at line 203 to match the label order used in the histogram declaration at
line 586. Change the label order from `cluster_queue, is_group, name` to `name,
cluster_queue, is_group` to ensure the documentation comment is consistent with
the actual histogram metric definition and prevents confusion for metric
consumers.
|
/kind feature |
k8s-ci-robot
added
the
kind/feature
|
/release-note-edit |
0a8f1ec to
dac5c9b
Compare
dac5c9b to
78c089e
Compare
mimowo
left a comment
mimowo
left a comment
There was a problem hiding this comment.
/lgtm
/approve
Thank you for the new metric 👍 Please follow up with the update for the topology and elastic-job gates.
kubernetes-prow
Bot
added
the
lgtm
|
LGTM label has been added. DetailsGit tree hash: 05c927d0d1dc03cc75a4df75c226af383ec9f03f |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mbobrovskyi, mimowo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
the
approved
What type of PR is this?
What this PR does / why we need it:
Add kueue_pod_scheduling_gate_removal_seconds metric.
Which issue(s) this PR fixes:
Part of #11170
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
kueue_pod_scheduling_gate_removal_secondsPrometheus histogram to measure latency from workload admission to scheduling gate removal, labeled byname,cluster_queue, and whether the workload is a pod group (is_group).Documentation
kueue_pod_scheduling_gate_removal_secondsand its labels.Bug Fixes
Tests