MCO-1540: Set up events for builds - #6252
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-1540 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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds an ChangesBuild lifecycle events
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant osbuildcontroller
participant buildReconciler
participant OCLEventRecorder
participant KubernetesEventRecorder
osbuildcontroller->>buildReconciler: Construct with eventRecorder
buildReconciler->>OCLEventRecorder: Record lifecycle event
OCLEventRecorder->>KubernetesEventRecorder: Emit typed event with reason and message
Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/controller/build/reconciler.go (2)
122-134: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEarly
return nilnow skipssyncMachineOSConfigs(ctx)on the spec-changed path.Previously this branch fell through to
return b.syncMachineOSConfigs(ctx)at line 133 after creating/reusing the build. The new code addsreturn nilright afterRecordConfigReconciled, sosyncMachineOSConfigs(ctx)is now unconditionally skipped whenever the MachineOSConfig spec changed. IfsyncMachineOSConfigsperforms reconciliation work beyond whatcreateNewMachineOSBuildOrReuseExistingdoes (e.g. syncing other configs, updating aggregate status), this is an unrelated functional regression introduced alongside the event-recording change.🐛 Proposed fix to preserve prior sync behavior
if !equality.Semantic.DeepEqual(old.Spec, cur.Spec) { klog.Infof("Detected MachineOSConfig change for %s", cur.Name) b.eventRecorder.RecordConfigReconciling(cur) if err := b.createNewMachineOSBuildOrReuseExisting(ctx, cur, false); err != nil { return err } b.eventRecorder.RecordConfigReconciled(cur) - return nil + return b.syncMachineOSConfigs(ctx) }🤖 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 122 - 134, The spec-changed branch in reconciler.go is returning too early and bypassing the existing sync path. In the reconcile flow around createNewMachineOSBuildOrReuseExisting, RecordConfigReconciled should not end the method; preserve the prior behavior by letting the code continue to the shared syncMachineOSConfigs(ctx) reconciliation work instead of unconditionally returning nil after the event recording.
520-526: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
RecordBuildStartedfires before the build job is actually created.
RecordBuildStartedis emitted at line 520, but the actual job creation (imagebuilder...Start(ctx)) happens afterward and can fail and return an error at line 525-526. IfStart(ctx)fails, the Events list on theMachineOSBuildwill already show "Started build for pool ..." even though no build job was ever created — misleading for the exactoc describeworkflow this PR is meant to support. Consider emittingRecordBuildStartedonly afterStart(ctx)succeeds.🐛 Proposed fix to emit after success
- b.eventRecorder.RecordBuildStarted(mosb, mosc) b.eventRecorder.RecordBuildPreparing(mosb, fmt.Sprintf("creating build job for pool %q", mosc.Spec.MachineConfigPool.Name)) // Next, create our new MachineOSBuild. if err := imagebuilder.NewJobImageBuilder(b.kubeclient, b.mcfgclient, mosb, mosc).Start(ctx); err != nil { return fmt.Errorf("imagebuilder could not start build for MachineOSBuild %q: %w", mosb.Name, err) } + + b.eventRecorder.RecordBuildStarted(mosb, mosc)🤖 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 520 - 526, `RecordBuildStarted` is emitted too early in `reconciler.go`, before `imagebuilder.NewJobImageBuilder(...).Start(ctx)` succeeds, which can leave a misleading “started” event when job creation fails. Move the `RecordBuildStarted(mosb, mosc)` call to after the `Start(ctx)` call in the build flow, so it only fires once the build job is actually created; keep `RecordBuildPreparing` before the start attempt and preserve the existing error handling in the reconciler path.
🧹 Nitpick comments (3)
pkg/controller/build/ocl_events_test.go (1)
78-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate duplicated test-runner loop.
The
for _, tt := range tests { t.Run(...) {...} }body is identical across all four test functions. Extracting a shared helper would remove ~40 lines of duplication and make adding new cases less error-prone.♻️ Suggested helper extraction
type eventCase struct { name string record func(*OCLEventRecorder) wantType string wantReason string } func runEventCases(t *testing.T, tests []eventCase) { t.Helper() for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() recorder, fake := newTestOCLRecorder(1) tt.record(recorder) assertEvent(t, fake.Events, tt.wantType, tt.wantReason) assertNoEvent(t, fake.Events) }) } }Then each test becomes:
- for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - recorder, fake := newTestOCLRecorder(1) - tt.record(recorder) - assertEvent(t, fake.Events, tt.wantType, tt.wantReason) - assertNoEvent(t, fake.Events) - }) - } + runEventCases(t, tests)Also applies to: 111-133, 140-162, 171-194
🤖 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_events_test.go` around lines 78 - 103, The test functions in OCLEventRecorder all repeat the same t.Run loop over event cases, so extract that shared runner into a helper and reuse it across the tests. Define a common case type and a helper like runEventCases that owns the t.Run/t.Parallel/newTestOCLRecorder/assertEvent/assertNoEvent flow, then replace each duplicated loop in the OCLEventRecorder test cases with a call to that helper while keeping the per-test case lists intact.pkg/controller/build/ocl_events.go (1)
199-209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
mcpparameter alongsidemosc.
RecordBuildDegraded/RecordBuildRecoveredboth takemoscandmcp, but only usemcp.Namefor the message whilemosc.Spec.MachineConfigPool.Namealready provides the pool name. Could drop themcpparameter for a smaller, more consistent API surface (other methods only take the object types they actually need fields from).Also note
RecordBuildDegradedquotes the pool name with%q(line 202) whileRecordBuildRecovereduses%s(line 208) — purely cosmetic but worth aligning.♻️ Possible simplification
-func (r *OCLEventRecorder) RecordBuildDegraded(mosc *mcfgv1.MachineOSConfig, mcp *mcfgv1.MachineConfigPool) { +func (r *OCLEventRecorder) RecordBuildDegraded(mosc *mcfgv1.MachineOSConfig) { r.recorder.Event(mosc, corev1.EventTypeWarning, EventBuildDegraded, - fmt.Sprintf("Build for pool %q degraded; see MachineOSBuild status conditions for details", mcp.Name)) + fmt.Sprintf("Build for pool %q degraded; see MachineOSBuild status conditions for details", mosc.Spec.MachineConfigPool.Name)) } -func (r *OCLEventRecorder) RecordBuildRecovered(mosc *mcfgv1.MachineOSConfig, mcp *mcfgv1.MachineConfigPool) { +func (r *OCLEventRecorder) RecordBuildRecovered(mosc *mcfgv1.MachineOSConfig) { r.recorder.Event(mosc, corev1.EventTypeNormal, EventBuildRecovered, - fmt.Sprintf("Build for pool %s recovered from degraded state", mcp.Name)) + fmt.Sprintf("Build for pool %q recovered from degraded state", mosc.Spec.MachineConfigPool.Name)) }🤖 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_events.go` around lines 199 - 209, RecordBuildDegraded and RecordBuildRecovered in OCLEventRecorder take mosc and mcp, but only need the pool name already available from mosc.Spec.MachineConfigPool.Name, so remove the redundant mcp parameter and update the callers accordingly. While touching these methods, make the pool-name formatting consistent between RecordBuildDegraded and RecordBuildRecovered by using the same verb style in both event messages.pkg/controller/build/reconciler.go (1)
352-368: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueExtra live API GET on every successful build transition.
liveMCPis fetched via a direct API call (bypassing the lister cache) on every build success transition specifically to detect recovery for the event. This adds an uncached API round-trip to a path that already does several lister/API calls; for clusters with frequent build churn this adds incremental API server load. Consider verifying this is acceptable, or scoping the live fetch to only fire when the lister-cachedmcpindicates a possible stale degraded condition.🤖 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 352 - 368, The successful-build path in reconciler.go always performs a live MachineConfigPool GET in the build completion flow, which adds an uncached API call on every transition. Update the logic around the current build-recovery check in the Build reconciler (using mcp, liveMCP, and RecordBuildRecovered) to avoid the unconditional live fetch, either by gating it behind a cached indication that recovery is possible or otherwise narrowing when the direct API call is needed. Keep the recovery event behavior intact while reducing API-server load on frequent build completions.
🤖 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/reconciler.go`:
- Line 981: Move the RecordBuildDeleted event in deleteMachineOSBuild so it is
emitted only after deleteMOSBImage, the digest ConfigMap cleanup, and the
MachineOSBuild delete call have all succeeded; right now
b.eventRecorder.RecordBuildDeleted(mosb, "build resources removed") is called
too early and can report success before deletion completes. Keep the existing
deleteMachineOSBuild flow, but place the event at the end of the successful path
so the emitted “build resources removed” message reflects an actually completed
deletion.
---
Outside diff comments:
In `@pkg/controller/build/reconciler.go`:
- Around line 122-134: The spec-changed branch in reconciler.go is returning too
early and bypassing the existing sync path. In the reconcile flow around
createNewMachineOSBuildOrReuseExisting, RecordConfigReconciled should not end
the method; preserve the prior behavior by letting the code continue to the
shared syncMachineOSConfigs(ctx) reconciliation work instead of unconditionally
returning nil after the event recording.
- Around line 520-526: `RecordBuildStarted` is emitted too early in
`reconciler.go`, before `imagebuilder.NewJobImageBuilder(...).Start(ctx)`
succeeds, which can leave a misleading “started” event when job creation fails.
Move the `RecordBuildStarted(mosb, mosc)` call to after the `Start(ctx)` call in
the build flow, so it only fires once the build job is actually created; keep
`RecordBuildPreparing` before the start attempt and preserve the existing error
handling in the reconciler path.
---
Nitpick comments:
In `@pkg/controller/build/ocl_events_test.go`:
- Around line 78-103: The test functions in OCLEventRecorder all repeat the same
t.Run loop over event cases, so extract that shared runner into a helper and
reuse it across the tests. Define a common case type and a helper like
runEventCases that owns the
t.Run/t.Parallel/newTestOCLRecorder/assertEvent/assertNoEvent flow, then replace
each duplicated loop in the OCLEventRecorder test cases with a call to that
helper while keeping the per-test case lists intact.
In `@pkg/controller/build/ocl_events.go`:
- Around line 199-209: RecordBuildDegraded and RecordBuildRecovered in
OCLEventRecorder take mosc and mcp, but only need the pool name already
available from mosc.Spec.MachineConfigPool.Name, so remove the redundant mcp
parameter and update the callers accordingly. While touching these methods, make
the pool-name formatting consistent between RecordBuildDegraded and
RecordBuildRecovered by using the same verb style in both event messages.
In `@pkg/controller/build/reconciler.go`:
- Around line 352-368: The successful-build path in reconciler.go always
performs a live MachineConfigPool GET in the build completion flow, which adds
an uncached API call on every transition. Update the logic around the current
build-recovery check in the Build reconciler (using mcp, liveMCP, and
RecordBuildRecovered) to avoid the unconditional live fetch, either by gating it
behind a cached indication that recovery is possible or otherwise narrowing when
the direct API call is needed. Keep the recovery event behavior intact while
reducing API-server load on frequent build completions.
🪄 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: f4854e10-9eea-4ba2-aa36-a5da140be606
📒 Files selected for processing (4)
pkg/controller/build/ocl_events.gopkg/controller/build/ocl_events_test.gopkg/controller/build/osbuildcontroller.gopkg/controller/build/reconciler.go
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eric200428, umohnani8 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 |
openshift-ci
Bot
added
the
approved
dkhater-redhat
left a comment
dkhater-redhat
left a comment
There was a problem hiding this comment.
nice work! leaving some comments for you to review. LMK when you're ready for a second round :)
| EventBuildCompleted = "BuildCompleted" | ||
| EventBuildFailed = "BuildFailed" | ||
| EventBuildInterrupted = "BuildInterrupted" | ||
| EventBuildRetrying = "BuildRetrying" |
There was a problem hiding this comment.
nit: I may be incorrect but I dont believe we have a retrying condition in the MOSB API. we do have a controller-level concept of deleting an old build and recreating a new one though. AFAIK the MOSB state machine only knows: Prepared → Building → (Succeeded | Failed | Interrupted). I would remove this event entirely.
| // RecordBuildRetrying records when an existing build is replaced due to a config change | ||
| func (r *OCLEventRecorder) RecordBuildRetrying(mosb *mcfgv1.MachineOSBuild) { | ||
| r.recorder.Event(mosb, corev1.EventTypeNormal, EventBuildRetrying, | ||
| "Existing build replaced due to config change") |
There was a problem hiding this comment.
technically when we are doing a "retry", the config doesn't change, the MOSB actually stays the same. Imo it would make sense to remove this event because we dont have this notion of retrying. we have more of a notion that a rebuild is a new build lifecycle.
| EventCleanupStarted = "CleanupStarted" | ||
| EventCleanupCompleted = "CleanupCompleted" | ||
| EventCleanupFailed = "CleanupFailed" |
There was a problem hiding this comment.
nit: I dont know if these are useful to the user. I think this may be noisy and its just for our internal bookkeeping
| // RecordConfigChanged records when a config changes triggering a new build | ||
| func (r *OCLEventRecorder) RecordConfigChanged(mosc *mcfgv1.MachineOSConfig, oldConfig, newConfig string) { | ||
| r.recorder.Event(mosc, corev1.EventTypeNormal, EventConfigChanged, | ||
| fmt.Sprintf("Configuration changed from %s to %s, triggering new build", oldConfig, newConfig)) | ||
| } |
There was a problem hiding this comment.
nit: I think this function was only called in tests? Also, the event signature assumes there's a simple "old config" and "new config" string, but the MOSC spec is a complex object. The existing events you have (RecordConfigReconciling and RecordConfigReconciled) should already be sufficient. I would consider removing.
| b.eventRecorder.RecordConfigReconciling(cur) | ||
| if err := b.createNewMachineOSBuildOrReuseExisting(ctx, cur, false); err != nil { | ||
| return err | ||
| } | ||
| b.eventRecorder.RecordConfigReconciled(cur) |
There was a problem hiding this comment.
nit: This leaves dangling "ConfigReconciling" events when reconciliation fails. Every reconcile loop emits "reconciling" but only successful ones emit "reconciled", creating noise.
| if !oldState.IsBuildFailure() && curState.IsBuildFailure() { | ||
| klog.Infof("MachineOSBuild %s failed, leaving ephemeral objects in place for inspection", current.Name) | ||
|
|
||
| b.eventRecorder.RecordBuildFailed(current) |
There was a problem hiding this comment.
nit: the event fires before the MCP fetch that could fail. Event says "build failed" but degradation state might not get set if the MCP fetch errors. so consider recording the event after successfully updating the degraded condition
|
|
||
| // If we need to rebuild, then we need to create a new MachineOSBuild | ||
| if imageNeedsRebuild { | ||
| b.eventRecorder.RecordBuildRetrying(existingMosb) |
There was a problem hiding this comment.
nit: the event is recorded on the old MOSB right before deletion, so the event disappears with the object. we should record on MOSC or new MOSB instead.
| // Cleanup event recording | ||
|
|
||
| // RecordCleanupStarted records when cleanup of build artifacts starts | ||
| func (r *OCLEventRecorder) RecordCleanupStarted(mosb *mcfgv1.MachineOSBuild) { | ||
| r.recorder.Event(mosb, corev1.EventTypeNormal, EventCleanupStarted, | ||
| "Started cleanup of build artifacts") | ||
| } | ||
|
|
||
| // RecordCleanupCompleted records when cleanup completes | ||
| func (r *OCLEventRecorder) RecordCleanupCompleted(mosb *mcfgv1.MachineOSBuild) { | ||
| r.recorder.Event(mosb, corev1.EventTypeNormal, EventCleanupCompleted, | ||
| "Cleanup of build artifacts completed") | ||
| } | ||
|
|
||
| // RecordCleanupFailed records when cleanup fails | ||
| func (r *OCLEventRecorder) RecordCleanupFailed(mosb *mcfgv1.MachineOSBuild) { | ||
| r.recorder.Event(mosb, corev1.EventTypeWarning, EventCleanupFailed, | ||
| fmt.Sprintf("Cleanup of build artifacts failed; see MachineOSBuild %q status conditions for details", mosb.Name)) | ||
| } |
There was a problem hiding this comment.
see my comment above
|
/pipeline required |
|
Scheduling tests matching the |
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/reconciler.go`:
- Around line 280-283: Update the deleteJob flow to deep-copy the
informer-provided Job before calling SetDeletionTimestamp, mutate the copied
Job, and pass that copy to updateMachineOSBuildWithStatus. Keep DeleteJob
operating on the original Job as currently intended.
🪄 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: c4ad3be6-7fad-4775-b973-87fcadc2878d
📒 Files selected for processing (4)
pkg/controller/build/ocl_events.gopkg/controller/build/ocl_events_test.gopkg/controller/build/osbuildcontroller.gopkg/controller/build/reconciler.go
|
/pipeline required |
|
Scheduling tests matching the |
|
/retest |
|
|
||
| // Performs the deletion reconciliation of the MachineOSConfig. | ||
| func (b *buildReconciler) deleteMachineOSConfig(ctx context.Context, mosc *mcfgv1.MachineOSConfig) error { | ||
| b.eventRecorder.RecordConfigDeleted(mosc) |
There was a problem hiding this comment.
looks like this is recording a deleted event before the deletion s successful. There is a chance it might fail, I would suggest moving this towards the end of the function once the deletion is actually successful.
|
@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. |
4 participants
- What I did
- How to verify it
Run the command below for the unit test
or
MachineOSConfig, then get the build name withoc get machineosbuild -n openshift-machine-config-operatoroc describe machineosbuild <name> -n openshift-machine-config-operatorand scroll to theEvents:section at the bottomNormal/Warningevents likeBuildStarted,BuildBuilding,BuildCompleted(orBuildFailed/BuildDegradedon failure) with timestamps and messages- Description for the changelog
Add Kubernetes event recording for OCL build lifecycle transitions
Summary by CodeRabbit