MCO-1814: Setup metrics for builds - #6316
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@eric200428: This pull request references MCO-1814 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
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:
WalkthroughThe PR adds OCL Prometheus metrics, records build and image-push lifecycle transitions during reconciliation, exposes metrics through a TLS-configurable listener, tests metric behavior, and adds Prometheus alerts for OCL failures. ChangesOCL observability
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant BuildJob
participant BuildReconciler
participant OCLMetrics
participant MetricsListener
participant Prometheus
BuildJob->>BuildReconciler: add or update build job
BuildReconciler->>OCLMetrics: record lifecycle and queue metrics
MachineOSBuilder->>MetricsListener: start TLS-configured listener
Prometheus->>MetricsListener: scrape OCL metrics
Prometheus->>Prometheus: evaluate OCL alert rules
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: eric200428 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/build/ocl_metrics.go`:
- Around line 17-132: Remove the unbounded build_name and job_name labels from
lifecycle gauges including oclBuildState, oclBuildStartTime, oclBuildJobState,
and oclImagePushState, retaining only pool and state labels where needed. Use
aggregate metrics such as oclBuildTotal and the existing pool-level gauges for
historical and active tracking, and update all metric observations accordingly.
- Around line 236-246: Delete the corresponding pushStartTimes entry using the
pool/buildName key when a build reaches any terminal state. Add this cleanup to
RecordBuildInterrupted and also to RecordBuildFailed and RecordBuildCompleted,
while preserving their existing metric updates and the cleanup performed by
image-push completion/failure handlers.
- Around line 248-263: Update RecordBuildJobState so every recorded state label
combination sets oclBuildJobState to 1.0, removing the state-dependent 2.0 and
3.0 assignments while preserving the existing label cleanup and registration
flow. Also remove outdated enum-value mappings from the Help strings for
oclBuildState, oclBuildJobState, and oclImagePushState if they describe numeric
gauge values.
In `@pkg/controller/build/reconciler.go`:
- Around line 222-232: Stop suppressing lookup errors in AddJob and UpdateJob:
explicitly check errors from getMachineOSBuildForJob and
GetMachineOSConfigForMachineOSBuild, log warnings, and only continue recording
metrics when lookups succeed. In getMachineOSBuildForJob, return nil, nil for
jobs without the MachineOSBuild label while preserving genuine lookup errors;
apply these changes at pkg/controller/build/reconciler.go lines 222-232,
244-265, and 556-565.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 771cf087-ce25-40ad-a683-9b345be95df4
📒 Files selected for processing (5)
cmd/machine-os-builder/start.goinstall/0000_90_machine-config_01_prometheus-rules.yamlpkg/controller/build/ocl_metrics.gopkg/controller/build/ocl_metrics_test.gopkg/controller/build/reconciler.go
| mosb, err := b.getMachineOSBuildForJob(job) | ||
| if err == nil && mosb != nil { | ||
| mosc, err := utils.GetMachineOSConfigForMachineOSBuild(mosb, b.utilListers()) | ||
| if err == nil { | ||
| poolName := mosc.Spec.MachineConfigPool.Name | ||
| RecordBuildJobState(poolName, mosb.Name, job.Name, "active") | ||
| RecordImagePushStarted(poolName, mosb.Name) | ||
| RecordBuildQueueDuration(poolName, mosb.CreationTimestamp.Time) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not ignore error returns.
The current implementation ignores all errors returned from getMachineOSBuildForJob and GetMachineOSConfigForMachineOSBuild because the helper function intentionally returns an error for jobs missing the MachineOSBuild label. This pattern suppresses legitimate Lister lookup errors. As per path instructions, **/*.go: Go security (prodsec-skills): Never ignore error returns.
Refactor the helper to return nil, nil for unlabelled jobs, allowing the callers to explicitly check and log genuine errors.
pkg/controller/build/reconciler.go#L222-L232: UpdateAddJobto explicitly checkerr != niland log warnings instead of silently skipping withif err == nil.pkg/controller/build/reconciler.go#L244-L265: UpdateUpdateJobto explicitly check and logerr != nil.pkg/controller/build/reconciler.go#L556-L565: ModifygetMachineOSBuildForJobto returnnil, nilwhen the label is absent, rather than returning an error.
📍 Affects 1 file
pkg/controller/build/reconciler.go#L222-L232(this comment)pkg/controller/build/reconciler.go#L244-L265pkg/controller/build/reconciler.go#L556-L565
🤖 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/build/reconciler.go` around lines 222 - 232, Stop suppressing
lookup errors in AddJob and UpdateJob: explicitly check errors from
getMachineOSBuildForJob and GetMachineOSConfigForMachineOSBuild, log warnings,
and only continue recording metrics when lookups succeed. In
getMachineOSBuildForJob, return nil, nil for jobs without the MachineOSBuild
label while preserving genuine lookup errors; apply these changes at
pkg/controller/build/reconciler.go lines 222-232, 244-265, and 556-565.
Source: Path instructions
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/build/ocl_metrics.go`:
- Around line 40-45: Remove the build_name label from oclBuildEndTime and
oclBuildRetries, then remove the corresponding buildName argument from all
listed WithLabelValues calls in pkg/controller/build/ocl_metrics.go at lines
40-45, 68-73, 215, 232, 246, and 264. Keep buildName function parameters if
needed to preserve existing reconciler.go callers.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3b1f10a1-c768-458e-aa17-22737d35567b
📒 Files selected for processing (5)
cmd/machine-os-builder/start.goinstall/0000_90_machine-config_01_prometheus-rules.yamlpkg/controller/build/ocl_metrics.gopkg/controller/build/ocl_metrics_test.gopkg/controller/build/reconciler.go
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/machine-os-builder/start.go
- install/0000_90_machine-config_01_prometheus-rules.yaml
- pkg/controller/build/ocl_metrics_test.go
- pkg/controller/build/reconciler.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controller/build/ocl_metrics.go (1)
182-249: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPrevent metric drift by using
.Set()instead of.Inc()/.Dec().Reconcilers may execute idempotent updates or re-process the same logical state (e.g., due to cache syncs or retries). If
RecordBuildStartedis called multiple times without reaching a completion, or if a terminal state is recorded repeatedly, using.Inc()and.Dec()will cause theocl_active_buildsgauge to drift and potentially become unbounded or negative.Since previous states are cleared per pool (implying a maximum of one tracked active build per pool), it is safer and more robust to use
.Set(1)and.Set(0). This ensures the gauge immediately self-corrects even if events are missed or re-delivered.🛠️ Proposed fix
Update the
oclActiveBuildslogic across the lifecycle functions:func RecordBuildStarted(pool string) { ... oclBuildStartTime.WithLabelValues(pool).Set(now) - oclActiveBuilds.WithLabelValues(pool).Inc() + oclActiveBuilds.WithLabelValues(pool).Set(1) } ... (update all terminal states) func RecordBuildCompleted(pool string, startTime time.Time) { ... oclBuildTotal.WithLabelValues(pool, StateSucceeded).Inc() - oclActiveBuilds.WithLabelValues(pool).Dec() + oclActiveBuilds.WithLabelValues(pool).Set(0) } func RecordBuildFailed(pool string, startTime time.Time) { ... oclBuildTotal.WithLabelValues(pool, StateFailed).Inc() - oclActiveBuilds.WithLabelValues(pool).Dec() + oclActiveBuilds.WithLabelValues(pool).Set(0) } func RecordBuildInterrupted(pool string) { ... oclBuildTotal.WithLabelValues(pool, StateInterrupted).Inc() - oclActiveBuilds.WithLabelValues(pool).Dec() + oclActiveBuilds.WithLabelValues(pool).Set(0) }🤖 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/build/ocl_metrics.go` around lines 182 - 249, Update oclActiveBuilds usage in RecordBuildStarted, RecordBuildCompleted, RecordBuildFailed, and RecordBuildInterrupted to use Set(1) when a pool enters an active build and Set(0) when it reaches any terminal state, replacing Inc and Dec. Preserve the existing per-pool state cleanup and lifecycle transitions so repeated or retried events remain idempotent.
🤖 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.
Nitpick comments:
In `@pkg/controller/build/ocl_metrics.go`:
- Around line 182-249: Update oclActiveBuilds usage in RecordBuildStarted,
RecordBuildCompleted, RecordBuildFailed, and RecordBuildInterrupted to use
Set(1) when a pool enters an active build and Set(0) when it reaches any
terminal state, replacing Inc and Dec. Preserve the existing per-pool state
cleanup and lifecycle transitions so repeated or retried events remain
idempotent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d818b986-0b4e-4d31-a4d0-6a8b24562116
📒 Files selected for processing (5)
cmd/machine-os-builder/start.goinstall/0000_90_machine-config_01_prometheus-rules.yamlpkg/controller/build/ocl_metrics.gopkg/controller/build/ocl_metrics_test.gopkg/controller/build/reconciler.go
🚧 Files skipped from review as they are similar to previous changes (3)
- install/0000_90_machine-config_01_prometheus-rules.yaml
- pkg/controller/build/ocl_metrics_test.go
- pkg/controller/build/reconciler.go
|
/pipeline required |
|
Scheduling tests matching the |
|
/pipeline required |
|
Scheduling tests matching the |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/controller/build/ocl_metrics.go (2)
174-183: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
SetToCurrentTime()for timestamp gauges.Prometheus's
Gaugeinterface provides a built-inSetToCurrentTime()method. It is more idiomatic and abstracts away thefloat64and Unix epoch conversion.(Note: You could also apply this to
oclBuildEndTimein the terminal state handlers if desired, though relying on the pre-computednowvariable is fine there since it is also used for calculating durations).♻️ Proposed refactor
func RecordBuildStarted(pool string) { - now := float64(time.Now().Unix()) - // Clear previous states for this pool oclBuildState.DeletePartialMatch(prometheus.Labels{"pool": pool}) // Set new state oclBuildState.WithLabelValues(pool, StatePending).Set(1) - oclBuildStartTime.WithLabelValues(pool).Set(now) + oclBuildStartTime.WithLabelValues(pool).SetToCurrentTime() oclActiveBuilds.WithLabelValues(pool).Inc()🤖 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/build/ocl_metrics.go` around lines 174 - 183, Update RecordBuildStarted to use oclBuildStartTime.WithLabelValues(pool).SetToCurrentTime() instead of manually computing and setting a Unix timestamp; leave the surrounding state reset and active-build updates unchanged.
12-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the stale map key comment.
The map key was updated to just
poolin a recent commit to prevent cardinality leaks, but the comment still referencespool/buildName.♻️ Proposed refactor
-// pushStartTimes stores the time each image push began, keyed by "pool/buildName". +// pushStartTimes stores the time each image push began, keyed by pool name. // Used to compute push duration across separate AddJob and UpdateJob reconciler events.🤖 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/build/ocl_metrics.go` around lines 12 - 13, Update the comment for pushStartTimes to state that entries are keyed only by pool, removing the stale pool/buildName description while retaining its purpose for tracking push duration across reconciler events.
🤖 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/build/ocl_metrics.go`:
- Around line 200-202: Clear stale pool-specific end-time metrics before
recording terminal build states: update RecordBuildCompleted at
pkg/controller/build/ocl_metrics.go#L200-L202, RecordBuildFailed at
pkg/controller/build/ocl_metrics.go#L217-L219, and RecordBuildInterrupted at
pkg/controller/build/ocl_metrics.go#L231-L233 to delete oclBuildEndTime partial
matches for the pool, preserving only the newest state’s end time.
---
Nitpick comments:
In `@pkg/controller/build/ocl_metrics.go`:
- Around line 174-183: Update RecordBuildStarted to use
oclBuildStartTime.WithLabelValues(pool).SetToCurrentTime() instead of manually
computing and setting a Unix timestamp; leave the surrounding state reset and
active-build updates unchanged.
- Around line 12-13: Update the comment for pushStartTimes to state that entries
are keyed only by pool, removing the stale pool/buildName description while
retaining its purpose for tracking push duration across reconciler events.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 193410f4-f5fa-4237-8bbf-c5e97de830e7
📒 Files selected for processing (5)
cmd/machine-os-builder/start.goinstall/0000_90_machine-config_01_prometheus-rules.yamlpkg/controller/build/ocl_metrics.gopkg/controller/build/ocl_metrics_test.gopkg/controller/build/reconciler.go
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/machine-os-builder/start.go
- install/0000_90_machine-config_01_prometheus-rules.yaml
- pkg/controller/build/ocl_metrics_test.go
- pkg/controller/build/reconciler.go
| // Clear previous states | ||
| oclBuildState.DeletePartialMatch(prometheus.Labels{"pool": pool}) | ||
| pushStartTimes.Delete(pool) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clear stale states from oclBuildEndTime.
When a build reaches a terminal state, the new end time is recorded with a state label (e.g., state="succeeded"). Because previous end states (like a prior failed build) are not cleared, a single pool will indefinitely accumulate and expose multiple end times for different states concurrently. This can lead to ambiguous or incorrect results in Prometheus queries that check the last completion time. Ensure only the most recent end time is preserved by deleting the partial match for the pool.
pkg/controller/build/ocl_metrics.go#L200-L202: AddoclBuildEndTime.DeletePartialMatch(prometheus.Labels{"pool": pool})toRecordBuildCompleted.pkg/controller/build/ocl_metrics.go#L217-L219: Add the same clearance toRecordBuildFailed.pkg/controller/build/ocl_metrics.go#L231-L233: Add the same clearance toRecordBuildInterrupted.
📍 Affects 1 file
pkg/controller/build/ocl_metrics.go#L200-L202(this comment)pkg/controller/build/ocl_metrics.go#L217-L219pkg/controller/build/ocl_metrics.go#L231-L233
🤖 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/build/ocl_metrics.go` around lines 200 - 202, Clear stale
pool-specific end-time metrics before recording terminal build states: update
RecordBuildCompleted at pkg/controller/build/ocl_metrics.go#L200-L202,
RecordBuildFailed at pkg/controller/build/ocl_metrics.go#L217-L219, and
RecordBuildInterrupted at pkg/controller/build/ocl_metrics.go#L231-L233 to
delete oclBuildEndTime partial matches for the pool, preserving only the newest
state’s end time.
|
/retest |
|
/pipeline required |
|
Scheduling tests matching the |
|
@eric200428: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/hold Holding to allow the Kube rebase to land in #6321. Please ensure this will not cause merge conflicts for the Kube rebase before unholding this PR. |
openshift-ci
Bot
added
the
do-not-merge/hold
| } | ||
| } | ||
|
|
||
| UpdateOCLRolloutCounts(curMCP.Name, curMCP.Status.UpdatedMachineCount, curMCP.Status.MachineCount) |
There was a problem hiding this comment.
Looks like this will happen for every MCP update, we should limit it to only pools that have opted into layering
| } | ||
|
|
||
| // RecordImagePushStarted records when a build job becomes active (image push begins). | ||
| func RecordImagePushStarted(pool string) { |
There was a problem hiding this comment.
I don't see this being called anywhere in the code except of in tests.
| severity: warning | ||
| annotations: | ||
| summary: "On-Cluster Layering build is degraded for pool {{ $labels.pool }}" | ||
| description: "OCL build {{ $labels.build_name }} for MachineConfigPool {{ $labels.pool }} is in a persistent failed state. Check the MachineOSBuild {{ $labels.build_name }} status conditions and machine-os-builder pod logs." |
There was a problem hiding this comment.
I am reading https://github.com/openshift/machine-config-operator/pull/6316/changes#diff-b45c21f767c734fd138032dbf604f321aaf0decffbdeb37c127ee13954cfc52fR19 correctly, I don't see a build_name option available.
4 participants
- What I did
OCLMetricsmodule that registers Prometheus metrics for build telemetrybuildReconcilerto track the full OCL build lifecycle: build state, duration, job state, image push operations, config change counts, rollout progress, and active build counts- How to verify it
go test ./pkg/controller/build/Or on a live cluster:
curl -s http://localhost:8797/metrics | grep ocl_- Description for the changelog
Add Prometheus metrics for OCL build telemetry
Summary by CodeRabbit