OCPBUGS-87635: Fix MCP.status.osImageStream - #6154
Conversation
The logic that computes the MCP .status.osImageStream was wrongly using the "old" status of the MCP instead of the status thas is currently computed for the MCP, leading to races where the rendered MC used to determine the osImageURL in use can be the old one instead of the current one leading to a wrong stream to be reported. This change also adds an explicit check of the source MCs to ensure that the logic doesn't report a osImageStream if a user provided osImageUrl is in place. This check is a formality. Signed-off-by: Pablo Rodriguez Nava <git@amail.pablintino.eu>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-important
|
@pablintino: This pull request references Jira Issue OCPBUGS-87635, 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. |
WalkthroughThis PR updates the OS image stream status calculation in the machine config controller. The ChangesOS Image Stream Status Calculation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Command failed Comment |
openshift-ci
Bot
added
the
approved
| // Use the current fresh status instead of the MCP's old one | ||
| renderedConfigName := status.Configuration.Name |
There was a problem hiding this comment.
Note for the reviewer: This is the real fix for the issue.
| } | ||
|
|
||
| // If any source MC has osImageURL set, the OS image is user-managed | ||
| for _, srcRef := range status.Configuration.Source { |
There was a problem hiding this comment.
Note for the reviewer: This check is a formality but worth to do.
![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
🤖 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/node/status.go`:
- Around line 558-560: The lookup error handling for the source MachineConfig
(variable srcRef) in pkg/controller/node/status.go should fail closed instead of
continuing and risking a false-positive OS image stream; when the get for the
source MachineConfig returns an error, set/return an empty reference for srcRef
(e.g., an empty ObjectReference) rather than executing continue so the status
logic treats this as a retrieval failure consistent with other failures (refer
to srcRef and pool in the status aggregation function).
🪄 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: c2b377b9-c470-4ee3-b505-a70bc303696a
📒 Files selected for processing (1)
pkg/controller/node/status.go
| if err != nil { | ||
| klog.Warningf("Could not retrieve source MachineConfig %s for pool %s: %v", srcRef.Name, pool.Name, err) | ||
| continue |
There was a problem hiding this comment.
Fail closed when a source MachineConfig cannot be retrieved
If a source MC lookup fails, continue can incorrectly report an OS image stream (false positive) despite missing evidence that the pool is not user-managed. Return an empty reference on lookup error to keep status conservative and consistent with other retrieval failures in this function.
Proposed fix
for _, srcRef := range status.Configuration.Source {
srcMC, err := ctrl.mcLister.Get(srcRef.Name)
if err != nil {
klog.Warningf("Could not retrieve source MachineConfig %s for pool %s: %v", srcRef.Name, pool.Name, err)
- continue
+ return mcfgv1.OSImageStreamReference{}
}
if srcMC.Spec.OSImageURL != "" {
klog.V(4).Infof("Source MachineConfig %s has osImageURL set, skipping OSImageStream status for pool %s", srcMC.Name, pool.Name)
return mcfgv1.OSImageStreamReference{}
}
}🤖 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/node/status.go` around lines 558 - 560, The lookup error
handling for the source MachineConfig (variable srcRef) in
pkg/controller/node/status.go should fail closed instead of continuing and
risking a false-positive OS image stream; when the get for the source
MachineConfig returns an error, set/return an empty reference for srcRef (e.g.,
an empty ObjectReference) rather than executing continue so the status logic
treats this as a retrieval failure consistent with other failures (refer to
srcRef and pool in the status aggregation function).
|
|
||
| // getOSImageStream gets the OSImageStream for a pool based on the calculated updated and degraded state | ||
| func (ctrl *Controller) getOSImageStream(pool *mcfgv1.MachineConfigPool, isUpdated, isDegraded bool) mcfgv1.OSImageStreamReference { | ||
| func (ctrl *Controller) getOSImageStream(pool *mcfgv1.MachineConfigPool, status *mcfgv1.MachineConfigPoolStatus, isUpdated, isDegraded bool) mcfgv1.OSImageStreamReference { |
There was a problem hiding this comment.
Why does the new param need to be a pointer?
There was a problem hiding this comment.
There's no strong reason to pass it as a pointer, as well as there's no strong reason to pass it by value as a copy. As a pointer, there's always a risk that the function at some point in the future performs unexpected modifications to the status. On the other hand, the cost of passing the pointer is almost zero. With the pass by value approach, the risk is partially mitigated as some fields are primitives, but a good amount of them are not, so to fully mitigate the risk we would need a deep copy at the cost of speed.
isabella-janssen
left a comment
isabella-janssen
left a comment
There was a problem hiding this comment.
/lgtm
This seems like a fair fix to me
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: isabella-janssen, pablintino 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 |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@isabella-janssen: This pull request references Jira Issue OCPBUGS-87635, 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. |
|
/retest-required |
|
Pre-merge verified: Environment Setup: Run the below TC multiple times, the TC run without any error. /verified by @ptalgulk01 |
openshift-ci
Bot
added
the
qe-approved
|
@pablintino: This pull request references Jira Issue OCPBUGS-87635, 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. |
openshift-ci-robot
added
the
verified
|
@ptalgulk01: 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. |
|
@pablintino: 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. |
|
@pablintino: Jira Issue Verification Checks: Jira Issue OCPBUGS-87635 Jira Issue OCPBUGS-87635 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-06-10-003138 |
|
/cherry-pick release-4.22 |
|
@pablintino: new pull request created: #6164 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
Closes: #OCPBUGS-87635
- What I did
The logic that computes the MCP .status.osImageStream was wrongly using the "old" status of the MCP instead of the status thas is currently computed for the MCP, leading to races where the rendered MC used to determine the osImageURL in use can be the old one instead of the current one leading to a wrong stream to be reported. This change also adds an explicit check of the source MCs to ensure that the logic doesn't report a osImageStream if a user provided osImageUrl is in place. This check is a formality.
- How to verify it
Run "[PolarionID:88366][Skipped:Disconnected] osImageStream should be empty when osImageURL is set" a few (more than 5) times to ensure it doesn't fail anymore.
- Description for the changelog
Fix the osImageStream status field of the MCPs to report based on the latest computed state.
Summary by CodeRabbit