OCPBUGS-105432: fix nil pointer panic in IRI controller informer race - #6385
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe controller now initializes informer listers before registering event handlers. Node helper methods return ChangesInternal release image controller
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Informers
participant ControllerNew
participant MachineConfigNodeHandler
participant IRIQueue
Informers->>ControllerNew: Start and synchronize informers
ControllerNew->>ControllerNew: Initialize listers and callbacks
ControllerNew->>MachineConfigNodeHandler: Register event handler
Informers->>MachineConfigNodeHandler: Replay MachineConfigNode event
MachineConfigNodeHandler->>IRIQueue: Enqueue InternalReleaseImage
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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 |
|
/lgtm |
|
Scheduling tests matching the |
openshift-ci
Bot
added
the
approved
|
/jira refresh |
|
@bfournie: No Jira issue is referenced in the title of this pull request. 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. |
|
/test ? |
![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
🧹 Nitpick comments (1)
pkg/controller/internalreleaseimage/internalreleaseimage_controller.go (1)
114-143: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a regression test for the startup-order case.
The supplied fixture constructs
Newbefore it starts the informer factories. It does not exercise registeringmcnInformerhandlers after the informer has started. Add or locate a test that starts and preloadsmcnInformerbeforeNew, then verifies construction completes without a panic.🤖 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/internalreleaseimage/internalreleaseimage_controller.go` around lines 114 - 143, The existing tests do not cover registering mcnInformer handlers after the informer has started. Add a regression test that preloads and starts mcnInformer before calling New, then verify New completes without panicking, while preserving the existing fixture behavior for other startup orders.
🤖 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/internalreleaseimage/internalreleaseimage_controller.go`:
- Around line 396-398: Update the warning calls in the nodeLister checks within
the internal release image controller to stop logging raw nodeName values;
remove the interpolated node name or use the repository’s approved redaction
helper while preserving the warning and return behavior.
---
Nitpick comments:
In `@pkg/controller/internalreleaseimage/internalreleaseimage_controller.go`:
- Around line 114-143: The existing tests do not cover registering mcnInformer
handlers after the informer has started. Add a regression test that preloads and
starts mcnInformer before calling New, then verify New completes without
panicking, while preserving the existing fixture behavior for other startup
orders.
🪄 Autofix
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: Pro Plus
Run ID: 76c7be4f-31fe-4ba4-83f0-9180440b5d31
📒 Files selected for processing (1)
pkg/controller/internalreleaseimage/internalreleaseimage_controller.go
|
/retitle OCPBUGS-105432: fix nil pointer panic in IRI controller informer race |
openshift-ci
Bot
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@redhat-chai-bot: This pull request references Jira Issue OCPBUGS-105432, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/retitle OCPBUGS-105432: fix nil pointer panic in IRI controller informer race |
|
/test e2e-agent-compact-ipv4-iso-no-registry |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@bfournie: This pull request references Jira Issue OCPBUGS-105432, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
The InternalReleaseImage controller's New() constructor registered informer event handlers before assigning the listers. When an informer is already started, AddEventHandler replays synthetic Add events on a separate goroutine, which could invoke isControlPlaneNode/isNodeReady before ctrl.nodeLister was assigned, causing a nil pointer panic. Move all lister and HasSynced assignments ahead of the AddEventHandler registrations (pure reordering, no logic change), and add defensive nil guards in isControlPlaneNode and isNodeReady that log a warning and return false when the nodeLister is not yet initialized. The guard warnings intentionally omit the node name to avoid logging potentially sensitive infrastructure identifiers. Add TestNewWithAlreadyStartedInformers, a regression test that starts and preloads the informers before calling New() to exercise the replayed-event path and assert construction completes without panicking. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
4363e8c to
0607190
Compare
|
Pushed
Local verification: Note: this commit supersedes the previous SHA, so the earlier AI-generated. Review for accuracy. |
|
/lgtm |
|
@redhat-chai-bot: This pull request references Jira Issue OCPBUGS-105432, which is valid. 3 validation(s) were run on this bug
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. |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bfournie, redhat-chai-bot 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 |
|
/retest-required All 3 failures are unrelated to this PR's changes:
AI-generated. Review for accuracy. |
|
/retest e2e-aws-ovn-upgrade Infrastructure failure again — AI-generated. Review for accuracy. |
|
/test e2e-aws-ovn-upgrade AI-generated. Review for accuracy. |
|
/test tls-pqc-readiness AI-generated. Review for accuracy. |
|
/verified by ci/prow/e2e-agent-compact-ipv4-iso-no-registry |
openshift-ci-robot
added
the
verified
|
@bfournie: This PR has been marked as verified by 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. |
|
/test e2e-gcp-op-part2 AI-generated. Review for accuracy. |
|
/test e2e-gcp-op-ocl-part1 AI-generated. Review for accuracy. |
|
/test tls-pqc-readiness AI-generated. Review for accuracy. |
|
/test e2e-gcp-op-part2 AI-generated. Review for accuracy. |
|
This PR fixes a critical stream-blocking nil pointer panic (OCPBUGS-105432) and has been validated by the targeted @bfournie @smg247 — would it be possible to override /test tls-pqc-readiness AI-generated. Review for accuracy. |
|
/override ci/prow/tls-pqc-readiness |
|
@bfournie: bfournie unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:openshift: openshift-release-oversight openshift-staff-engineers openshift-sustaining-engineers. 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 kubernetes-sigs/prow repository. |
|
@yuqi-zhang or @isabella-janssen would it be possible to override the tls-pqc-readiness test? Its failing for unrelated reasons as above. |
|
/test perfscale-control-plane-6nodes AI-generated. Review for accuracy. |
|
Both Retesting both overnight. At this point, these two tests likely need a maintainer override to unblock the merge. The fix itself has been validated by /test tls-pqc-readiness AI-generated. Review for accuracy. |
|
/test tls-pqc-readiness AI-generated. Review for accuracy. |
|
Retesting /test tls-pqc-readiness AI-generated. Review for accuracy. AI-generated. Review for accuracy. |
|
/override ci/prow/tls-pqc-readiness |
|
@stbenjam: Overrode contexts on behalf of stbenjam: ci/prow/tls-pqc-readiness 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 kubernetes-sigs/prow repository. |
|
@redhat-chai-bot: Jira Issue Verification Checks: Jira Issue OCPBUGS-105432 Jira Issue OCPBUGS-105432 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
|
Fix included in release 5.0.0-0.nightly-2026-08-08-200126 |
|
/cherry-pick release-4.22 |
|
@bfournie: new pull request created: #6390 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 kubernetes-sigs/prow repository. |
7 participants
Summary
Fixes a nil pointer panic in the
InternalReleaseImagecontroller caused by a race condition in informer startup ordering.The
New()constructor registeredAddEventHandlercallbacks (L114-L146) before assigning listers likectrl.nodeLister(L165). When informers are already started, they replay synthetic Add events on a separate goroutine that races the main goroutine —isControlPlaneNode(L387) readsctrl.nodeListerbefore it is assigned, causingSIGSEGV addr=0x18.Root Cause
Introduced by MCO #5841 (AGENT-1488, merged 2026-05-26). The code was latent for 71 days until openshift/api #2859 (merged 2026-08-04) flipped
NoRegistryClusterInstallto Default, activating the IRI controller for the first time.The panic is intermittent (~1.4%/run) but triggers the zero-tolerance "Pods must not panic" aggregated test, causing ~13% payload rejection rate. The 5.0 CI stream has had 5+ consecutive rejections.
Changes
New()constructor: Move all 8 lister andHasSyncedassignments to before theAddEventHandlerregistrations. Pure statement reordering — no logic change. Includes an explanatory comment about the race.isControlPlaneNode(L387) andisNodeReady(L406) — ifnodeListeris nil, log a warning and returnfalse.Testing
go build✅go vet✅go test -race -count=1✅ (5 PASS, 0 FAIL, race detector clean)gofmt✅Tracking
AI-generated. Review for accuracy.
@smg247 requested in Slack thread
Summary by CodeRabbit