OCPBUGS-98210: Use release payload image for OSImageStream rebuild detection - #6284
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@yuqi-zhang: This pull request explicitly references no jira issue. 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
jira/valid-reference
WalkthroughThe OSImageStream creation path now records the release payload image annotation, and the operator rebuild check now derives the current payload digest from ClusterVersion and compares it against that annotation instead of the MCO binary hash. ChangesOSImageStream release payload annotation flow
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
|
/lgtm |
|
Scheduling tests matching the |
The OSImageStream rebuild check compared the CR's annotation against version.Hash (the MCO binary's git commit hash). In CI upgrade jobs where only RHCOS/kubernetes images are rebuilt but the MCO binary is unchanged, this caused the OSImageStream to never be refreshed with the new payload's RHCOS image digests. Nodes would complete the upgrade without actually updating the OS image, leaving them on the old kubelet version. Replace version.Hash with the release payload image digest (ClusterVersion.Status.Desired.Image) as the rebuild key. The release payload digest changes on every upgrade — including CI jobs where only a subset of payload images are rebuilt — so the OSImageStream is correctly re-inspected and updated with new RHCOS image references. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
a4143f1 to
0d62e45
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: 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/osimagestream/osimagestream.go`:
- Around line 99-100: The Create path in osimagestream.Create is passing
createOptions.ReleaseImage directly into newOSImageStream, which can leave
ReleasePayloadImageAnnotationKey empty when ReleaseImageStream is set and
ReleaseImage is unset. Resolve the release image from the stream before
constructing the OS image stream, and pass that resolved payload digest into
newOSImageStream so the annotation is populated correctly. Refer to Create and
newOSImageStream when updating the call site.
🪄 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: 79063fbc-92ca-4d6e-a015-a3f13d26474c
📒 Files selected for processing (3)
pkg/controller/common/constants.gopkg/operator/osimagestream_ocp.gopkg/osimagestream/osimagestream.go
| return newOSImageStream(createOptions.ExistingOSImageStream, streams, defaultStream, createOptions.ReleaseImage), nil | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all callers of Create/CreateOptions to check if ReleaseImageStream path is used without ReleaseImage set
rg -n -B3 -A10 'ReleaseImageStream:' --type=goRepository: openshift/machine-config-operator
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== pkg/osimagestream/osimagestream.go ==\n'
wc -l pkg/osimagestream/osimagestream.go
sed -n '1,220p' pkg/osimagestream/osimagestream.go
printf '\n== Search for CreateOptions and ReleaseImageStream usage ==\n'
rg -n -S 'type CreateOptions|ReleaseImageStream|ReleaseImage:' pkg . --type=go
printf '\n== Search for newOSImageStream and osImageStreamRequiresRebuild ==\n'
rg -n -S 'newOSImageStream|osImageStreamRequiresRebuild' pkg --type=goRepository: openshift/machine-config-operator
Length of output: 15697
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Candidate files referencing OSImageStream creation ==\n'
git ls-files 'pkg/**/*.go' | rg 'osimagestream|operator/osimagestream'
printf '\n== Search for ReleaseImageStream field and callers across the repo ==\n'
rg -n -S 'ReleaseImageStream|ReleaseImage:' . --type=go
printf '\n== Show the CreateOptions definition and Create implementation ==\n'
sed -n '1,220p' pkg/osimagestream/osimagestream.go
printf '\n== Show osImageStreamRequiresRebuild ==\n'
sed -n '1,220p' pkg/operator/osimagestream_ocp.goRepository: openshift/machine-config-operator
Length of output: 21155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== cmd/machine-config-osimagestream/helpers.go ==\n'
wc -l cmd/machine-config-osimagestream/helpers.go
sed -n '1,160p' cmd/machine-config-osimagestream/helpers.go
printf '\n== pkg/operator/osimagestream_ocp.go rebuild logic ==\n'
sed -n '300,380p' pkg/operator/osimagestream_ocp.go
printf '\n== osimagestream tests covering ReleaseImageStream ==\n'
sed -n '230,520p' pkg/osimagestream/osimagestream_test.goRepository: openshift/machine-config-operator
Length of output: 17336
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the CLI helper that populates CreateOptions and the rebuild predicate.
sed -n '1,140p' cmd/machine-config-osimagestream/helpers.go
sed -n '315,360p' pkg/operator/osimagestream_ocp.go
# Find every Create(...) call site and whether ReleaseImage is passed alongside ReleaseImageStream.
rg -n -C 4 'Create\(' cmd pkg --type=goRepository: openshift/machine-config-operator
Length of output: 50389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== bootstrap CreateOptions around the call site ==\n'
sed -n '500,545p' pkg/controller/bootstrap/bootstrap.go
printf '\n== CLI option struct and flag wiring ==\n'
rg -n -C 4 'type getOpts|releaseImage|imageStreamPath|imageStream' cmd/machine-config-osimagestream --type=go
printf '\n== Any validation that requires releaseImage when imageStreamPath is used ==\n'
rg -n -C 4 'imageStreamPath|releaseImage' cmd/machine-config-osimagestream --type=goRepository: openshift/machine-config-operator
Length of output: 14495
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== ReleasePayloadImageAnnotationKey usage ==\n'
rg -n -C 3 'ReleasePayloadImageAnnotationKey' . --type=go
printf '\n== Any tests asserting the annotation value on OSImageStream creation ==\n'
rg -n -C 3 'ReleasePayloadImageAnnotationKey|Release Image digest|release image annotation' pkg cmd --type=go
printf '\n== Any code paths that read the annotation outside osImageStreamRequiresRebuild ==\n'
rg -n -C 3 'ReleasePayloadImageAnnotationKey' pkg cmd --type=goRepository: openshift/machine-config-operator
Length of output: 10810
Pass the resolved release image here
Create is reachable with ReleaseImageStream set and ReleaseImage empty, so this writes ReleasePayloadImageAnnotationKey: "". That makes osImageStreamRequiresRebuild treat the stream as stale on every reconcile; plumb the actual payload digest into the annotation instead.
🤖 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/osimagestream/osimagestream.go` around lines 99 - 100, The Create path in
osimagestream.Create is passing createOptions.ReleaseImage directly into
newOSImageStream, which can leave ReleasePayloadImageAnnotationKey empty when
ReleaseImageStream is set and ReleaseImage is unset. Resolve the release image
from the stream before constructing the OS image stream, and pass that resolved
payload digest into newOSImageStream so the annotation is populated correctly.
Refer to Create and newOSImageStream when updating the call site.
There was a problem hiding this comment.
So it's not stale on every reconcile, just the first reconcile post-bootstrap I believe. The first reconcile should be writing the correct stream for future syncs
There was a problem hiding this comment.
Oops, something went wrong! Please try again later. 🐰 💔
yuqi-zhang
changed the title
|
@yuqi-zhang: This pull request references MCO-2415 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. |
|
/lgtm |
|
Scheduling tests matching the |
openshift-ci-robot
added
the
verified
|
@bertinatto: 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. |
yuqi-zhang
changed the title
openshift-ci-robot
added
jira/severity-moderate
|
@yuqi-zhang: This pull request references Jira Issue OCPBUGS-98210, which is valid. 3 validation(s) were run on this bug
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. |
|
|
||
| // Check if an update is needed | ||
| if !osImageStreamRequiresRebuild(existingOSImageStream) { | ||
| if !osImageStreamRequiresRebuild(existingOSImageStream, releaseImage) { |
There was a problem hiding this comment.
One thing, should we store the digest part only? If the image is mirrored using oc-mirror or similar the digest in disconnected envs will be preserved and no matter where the image is stored we won't rebuild. Not a blocker.
There was a problem hiding this comment.
Thanks for the note - I think re-mirroring shouldn't be a constant operation, so we should be fine with a extra rebuild once every so often. I lean towards keeping the full hash for now as a extra redundancy.
Given that you plan on improving the general process, I would lean towards doing this as a followup or rework (so that we don't have to conditionally rebuild). WDYT?
There was a problem hiding this comment.
I'm fine re-visting this later, let's go with the current state.
|
@neisw: This PR was included in a payload test run from openshift/kubernetes#2653
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/f8c99e70-7ba1-11f1-8798-27164a784cfa-0 |
|
@jubittajohn: This PR was included in a payload test run from openshift/kubernetes#2653
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/702b7290-7ba2-11f1-8082-d0d0bb944598-0 |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bertinatto, pablintino, yuqi-zhang 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 |
|
@jubittajohn: This PR was included in a payload test run from openshift/kubernetes#2653
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/9384b2b0-7bcf-11f1-913b-789f2c5fb949-0 |
|
@jubittajohn: This PR was included in a payload test run from openshift/kubernetes#2653
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/975cbcc0-7bcf-11f1-825c-f79f64c57335-0 |
|
@yuqi-zhang: 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. |
|
@yuqi-zhang: Jira Issue Verification Checks: Jira Issue OCPBUGS-98210 Jira Issue OCPBUGS-98210 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. |
|
@jacobsee: This PR was included in a payload test run from openshift/kubernetes#2653
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/47faa100-7be9-11f1-91a2-d8776496eeed-0 |
|
@jacobsee: This PR was included in a payload test run from openshift/kubernetes#2653
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/0682fe70-7bf8-11f1-80c5-f94a2b8c4115-0 |
|
@jacobsee: This PR was included in a payload test run from openshift/kubernetes#2653
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/1bedc830-7bf8-11f1-897d-282127a80781-0 |
|
@jubittajohn: This PR was included in a payload test run from openshift/kubernetes#2653
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/61f545c0-7c2e-11f1-9e2d-0fdc4266c21e-0 |
|
@jubittajohn: This PR was included in a payload test run from openshift/kubernetes#2653
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/a2ff82b0-7c2e-11f1-85f4-727d938ab794-0 |
|
@jubittajohn: This PR was included in a payload test run from openshift/kubernetes#2653
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/c3badfe0-7c2e-11f1-8822-0d29a23deec0-0 |
|
@jubittajohn: This PR was included in a payload test run from openshift/kubernetes#2653
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/d9f30bc0-7c2e-11f1-90c9-5bd68f48cc9a-0 |
|
@jubittajohn: This PR was included in a payload test run from openshift/kubernetes#2653
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/4001cbd0-7c30-11f1-963a-0606db8acd44-0 |
|
Fix included in release 5.0.0-0.nightly-2026-07-10-030545 |
|
Fix included in release 5.0.0-0.nightly-2026-07-14-100335 |
5 participants
The OSImageStream rebuild check compared the CR's annotation against version.Hash (the MCO binary's git commit hash). In CI upgrade jobs where only RHCOS/kubernetes images are rebuilt but the MCO binary is unchanged, this caused the OSImageStream to never be refreshed with the new payload's RHCOS image digests. Nodes would complete the upgrade without actually updating the OS image, leaving them on the old kubelet version.
Replace version.Hash with the release payload image digest (ClusterVersion.Status.Desired.Image) as the rebuild key. The release payload digest changes on every upgrade — including CI jobs where only a subset of payload images are rebuilt — so the OSImageStream is correctly re-inspected and updated with new RHCOS image references.
Will attach Jira once we validate this helps with openshift/kubernetes#2653
Summary by CodeRabbit