Skip to content

OCPBUGS-98210: Use release payload image for OSImageStream rebuild detection #6284

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 1 commit into from
Jul 9, 2026
Merged

OCPBUGS-98210: Use release payload image for OSImageStream rebuild detection #6284

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions pkg/controller/common/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@ const (
// ReleaseImageVersionAnnotationKey is used to tag the rendered machineconfigs & controller config with the release image version.
ReleaseImageVersionAnnotationKey = "machineconfiguration.openshift.io/release-image-version"

// ReleasePayloadImageAnnotationKey tags the OSImageStream with the release payload image
// (ClusterVersion.Status.Desired.Image). Unlike ReleaseImageVersionAnnotationKey (which
// tracks the MCO binary hash for version-skew guards), this changes on every upgrade and
// is used solely to decide whether the OSImageStream needs rebuilding.
ReleasePayloadImageAnnotationKey = "machineconfiguration.openshift.io/release-payload-image"

// OSImageURLOverriddenKey is used to tag a rendered machineconfig when OSImageURL has been overridden from default using machineconfig
OSImageURLOverriddenKey = "machineconfiguration.openshift.io/os-image-url-overridden"

Expand Down
28 changes: 20 additions & 8 deletions pkg/operator/osimagestream_ocp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import (
ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common"
"github.com/openshift/machine-config-operator/pkg/imageutils"
"github.com/openshift/machine-config-operator/pkg/osimagestream"
"github.com/openshift/machine-config-operator/pkg/version"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -153,8 +152,8 @@ func (optr *Operator) buildOSImageStream(existingOSImageStream *mcfgv1.OSImageSt
klog.Infof("Created OSImageStream with %d available streams, default stream: %s",
len(osImageStream.Status.AvailableStreams), osImageStream.Status.DefaultStream)
} else {
oldVersion := existingOSImageStream.Annotations[ctrlcommon.ReleaseImageVersionAnnotationKey]
klog.V(4).Infof("Updating OSImageStream (previous version: %s, new version: %s)", oldVersion, version.Hash)
oldPayloadImage := existingOSImageStream.Annotations[ctrlcommon.ReleasePayloadImageAnnotationKey]
klog.V(4).Infof("Updating OSImageStream (previous release image: %s, new release image: %s)", oldPayloadImage, image)
// Update metadata/spec first (mainly for annotations)
// DeepCopy to avoid mutating the shared informer cache
desired := existingOSImageStream.DeepCopy()
Expand Down Expand Up @@ -240,8 +239,18 @@ func (optr *Operator) isOSImageStreamBuildRequired() (*mcfgv1.OSImageStream, boo
return nil, true, err
}

// Get the release payload image to check if it has changed
clusterVersion, err := osimagestream.GetClusterVersion(optr.clusterVersionLister)
if err != nil {
return existingOSImageStream, true, fmt.Errorf("getting cluster version for OSImageStream rebuild check: %w", err)
}
releaseImage, err := osimagestream.GetReleasePayloadImage(clusterVersion)
if err != nil {
return existingOSImageStream, true, fmt.Errorf("getting release image for OSImageStream rebuild check: %w", err)
}

// Check if an update is needed
if !osImageStreamRequiresRebuild(existingOSImageStream) {
if !osImageStreamRequiresRebuild(existingOSImageStream, releaseImage) {

@pablintino pablintino Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@yuqi-zhang yuqi-zhang Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@pablintino pablintino Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine re-visting this later, let's go with the current state.

klog.V(4).Info("OSImageStream is already up-to-date, skipping sync")
return existingOSImageStream, false, nil
}
Expand Down Expand Up @@ -304,11 +313,14 @@ func (optr *Operator) getExistingOSImageStream() (*mcfgv1.OSImageStream, error)
}

// osImageStreamRequiresRebuild checks if the OSImageStream needs to be created or updated.
// Returns true if osImageStream is nil, if its version annotation doesn't match the current version.
func osImageStreamRequiresRebuild(osImageStream *mcfgv1.OSImageStream) bool {
// Returns true if osImageStream is nil, or if its release image annotation doesn't match the
// current release payload image. This uses the release payload image digest rather than the
// MCO binary hash so that upgrades are detected even when the MCO image itself hasn't changed
// (e.g., in CI upgrade jobs where only RHCOS images are rebuilt).
func osImageStreamRequiresRebuild(osImageStream *mcfgv1.OSImageStream, releaseImage string) bool {
if osImageStream == nil {
return true
}
releaseVersion, ok := osImageStream.Annotations[ctrlcommon.ReleaseImageVersionAnnotationKey]
return !ok || releaseVersion != version.Hash
storedImage, ok := osImageStream.Annotations[ctrlcommon.ReleasePayloadImageAnnotationKey]
return !ok || storedImage != releaseImage
}
5 changes: 3 additions & 2 deletions pkg/osimagestream/osimagestream.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,11 @@ func (f *DefaultStreamSourceFactory) Create(ctx context.Context, sysCtx *types.S
return nil, fmt.Errorf("could not find default OSImageStream in the available streams: %w", err)
}

return newOSImageStream(createOptions.ExistingOSImageStream, streams, defaultStream), nil
return newOSImageStream(createOptions.ExistingOSImageStream, streams, defaultStream, createOptions.ReleaseImage), nil
}
Comment on lines +99 to 100

@coderabbitai coderabbitai Bot Jul 8, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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=go

Repository: 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=go

Repository: 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.go

Repository: 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.go

Repository: 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=go

Repository: 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=go

Repository: 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=go

Repository: 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.

@yuqi-zhang yuqi-zhang Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@coderabbitai coderabbitai Bot Jul 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, something went wrong! Please try again later. 🐰 💔


// newOSImageStream assembles the OSImageStream CR from the resolved streams, default, and existing spec.
func newOSImageStream(existing *mcfgv1.OSImageStream, streams []mcfgv1.OSImageStreamSet, defaultStream string) *mcfgv1.OSImageStream {
func newOSImageStream(existing *mcfgv1.OSImageStream, streams []mcfgv1.OSImageStreamSet, defaultStream, releaseImage string) *mcfgv1.OSImageStream {
if existing != nil {
defaultStream = existing.Spec.DefaultStream
}
Expand All @@ -110,6 +110,7 @@ func newOSImageStream(existing *mcfgv1.OSImageStream, streams []mcfgv1.OSImageSt
Name: ctrlcommon.ClusterInstanceNameOSImageStream,
Annotations: map[string]string{
ctrlcommon.ReleaseImageVersionAnnotationKey: version.Hash,
ctrlcommon.ReleasePayloadImageAnnotationKey: releaseImage,
},
},
Spec: mcfgv1.OSImageStreamSpec{
Expand Down