From 309227f71b10a2f634e21943628553be3b8d99de Mon Sep 17 00:00:00 2001 From: Eris Hoxha Date: Mon, 13 Jul 2026 14:37:35 +0200 Subject: [PATCH 1/4] Prevent sync phase errors from blocking subsequent phases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A regression in 1dcc49e1 ("Sync pending migration part 2", PR #792) causes stale ReleasePayload CRDs (with all-Unknown conditions) to be misclassified as Pending by GetTagPhase, which now prefers CRD-derived phases over annotations. Old tags like 4.7.0-fc.0 re-enter syncPending, hit a mirror hash mismatch error, and abort the entire sync loop for the 4-stable stream. This prevents syncReady from ever running, so releases like 4.22.5 never get their verification jobs launched — tests stay pending indefinitely. This change makes sync phase errors non-fatal: - syncPending, syncReady, and syncAccepted errors are collected instead of causing an immediate return. All three phases always run. - Per-tag errors in the stable-mode syncPending loop are isolated via a new syncPendingStableTag helper, so one broken tag cannot block others. - An aggregate error is returned at the end so the item is still requeued. - Conflict errors remain an immediate return nil (resource was modified, resync is imminent). Adds a regression test reproducing the exact scenario: a stale CRD returning Pending for an Accepted tag must not cause an unrelated Ready tag to be misclassified. --- cmd/release-controller/sync.go | 209 ++++++++++++++-------------- cmd/release-controller/sync_test.go | 83 +++++++++++ 2 files changed, 188 insertions(+), 104 deletions(-) diff --git a/cmd/release-controller/sync.go b/cmd/release-controller/sync.go index d7e568d13..252a70f40 100644 --- a/cmd/release-controller/sync.go +++ b/cmd/release-controller/sync.go @@ -100,13 +100,15 @@ func (c *Controller) sync(key queueKey) error { pendingTags = []*imagev1.TagReference{releaseTag} } + var syncErrors []error + // ensure any pending tags have the necessary jobs/mirrors created if err := c.syncPending(release, pendingTags, inputImageHash); err != nil { if errors.IsConflict(err) { return nil } c.eventRecorder.Eventf(release.Source, corev1.EventTypeWarning, "UnableToProcessRelease", "%v", err) - return err + syncErrors = append(syncErrors, err) } // ensure verification steps are run on the ready tags @@ -115,7 +117,7 @@ func (c *Controller) sync(key queueKey) error { return nil } c.eventRecorder.Eventf(release.Source, corev1.EventTypeWarning, "UnableToVerifyRelease", "%v", err) - return err + syncErrors = append(syncErrors, err) } // ensure publish steps are run on the accepted tags @@ -124,7 +126,7 @@ func (c *Controller) sync(key queueKey) error { return nil } c.eventRecorder.Eventf(release.Source, corev1.EventTypeWarning, "UnableToVerifyRelease", "%v", err) - return err + syncErrors = append(syncErrors, err) } // if we're waiting for an interval to elapse, go ahead and queue to be woken @@ -133,7 +135,7 @@ func (c *Controller) sync(key queueKey) error { } c.gcQueue.AddAfter("", 15*time.Second) - return nil + return utilerrors.NewAggregate(syncErrors) } func calculateSyncActions(release *releasecontroller.Release, now time.Time) (adoptTags, pendingTags, removeTags []*imagev1.TagReference, hasNewImages bool, inputImageHash string, queueAfter time.Duration) { @@ -290,109 +292,14 @@ func (c *Controller) syncAdopted(release *releasecontroller.Release, adoptTags [ func (c *Controller) syncPending(release *releasecontroller.Release, pendingTags []*imagev1.TagReference, inputImageHash string) (err error) { switch release.Config.As { case releasecontroller.ReleaseConfigModeStable: + var errs []error for _, tag := range pendingTags { - // wait for import, then determine whether the requested version (tag name) matches the source version (label on image) - id := releasecontroller.FindImageIDForTag(release.Source, tag.Name) - if len(id) == 0 { - klog.V(2).Infof("Waiting for release %s to be imported before we can retrieve metadata", tag.Name) - continue - } - klog.V(2).Infof("Processing pending release %s", tag.Name) - rewriteValue := tag.Annotations[releasecontroller.ReleaseAnnotationRewrite] - if len(rewriteValue) == 0 { - klog.V(2).Infof("Rewriting pending release %s", tag.Name) - isi, err := c.imageClient.ImageStreamImages(release.Source.Namespace).Get(context.TODO(), fmt.Sprintf("%s@%s", release.Source.Name, id), metav1.GetOptions{}) - if err != nil { - return err - } - // Handle manifest list based releases... - for _, m := range isi.Image.DockerImageManifests { - if m.Architecture == "amd64" { - isi, err = c.imageClient.ImageStreamImages(release.Source.Namespace).Get(context.TODO(), fmt.Sprintf("%s@%s", release.Source.Name, m.Digest), metav1.GetOptions{}) - if err != nil { - return err - } - break - } - } - metadata := &docker10.DockerImage{} - if len(isi.Image.DockerImageMetadata.Raw) == 0 { - return fmt.Errorf("could not fetch Docker image metadata for release %s", tag.Name) - } - if err := json.Unmarshal(isi.Image.DockerImageMetadata.Raw, metadata); err != nil { - return fmt.Errorf("malformed Docker image metadata on ImageStreamTag: %v", err) - } - var name string - if metadata.Config != nil { - name = metadata.Config.Labels["io.openshift.release"] - } - rewriteValue = fmt.Sprintf("%t", name != tag.Name) - if err := c.setReleaseAnnotation(release, tag.Annotations[releasecontroller.ReleaseAnnotationPhase], map[string]string{releasecontroller.ReleaseAnnotationRewrite: rewriteValue}, tag.Name); err != nil { - return err - } - continue - } - rewrite := rewriteValue == "true" - - hash := fmt.Sprintf("%s-%d", tag.Name, *tag.Generation) - // mirror any internal images - // rewrite payload for internal images with metadata - mirror, err := c.ensureReleaseMirror(release, tag.Name, hash) - if err != nil { - return err - } - if len(tag.Annotations[releasecontroller.ReleaseAnnotationImageHash]) == 0 { - if err := c.setReleaseAnnotation(release, tag.Annotations[releasecontroller.ReleaseAnnotationPhase], map[string]string{releasecontroller.ReleaseAnnotationImageHash: mirror.Annotations[releasecontroller.ReleaseAnnotationImageHash]}, tag.Name); err != nil { - return err - } - continue - } - if mirror.Annotations[releasecontroller.ReleaseAnnotationImageHash] != tag.Annotations[releasecontroller.ReleaseAnnotationImageHash] { - // delete the mirror and exit - return fmt.Errorf("unimplemented, should regenerate contents of tag") - } - // Create the corresponding ReleasePayload object... - _, err = c.ensureReleasePayload(release, tag) - if err != nil { - return err - } - // get metadata about the release - // get upgrade graph edges - // check to see any required edges are missing? wait for latest edge? wait for pending edges? - // how do we calculate required edge set? - var job *batchv1.Job - if rewrite { - job, err = c.ensureRewriteJob(release, tag.Name, mirror, `{}`) - } else { - job, err = c.ensureImportJob(release, tag.Name, mirror) - } - if err != nil || job == nil { - return err - } - payload, err := c.releasePayloadLister.ReleasePayloads(release.Target.Namespace).Get(tag.Name) - if err != nil { - if !errors.IsNotFound(err) { - return err - } - return c.ensureRewriteJobImageRetrieved(release, job, mirror) - } - phase := releasecontroller.GetReleasePhase(payload) - switch phase { - case releasecontroller.ReleasePhaseReady: - if err := c.markReleaseReady(release, nil, tag.Name); err != nil { - return err - } - c.precacheChangelog(release, tag) - case releasecontroller.ReleasePhaseFailed: - log, _, _ := ensureJobTerminationMessageRetrieved(c.podClient, job, "status.phase=Failed", "build", false) - if err := c.transitionReleasePhaseFailure(release, []string{releasecontroller.ReleasePhasePending}, releasecontroller.ReleasePhaseFailed, withLog(reasonAndMessage("CreateReleaseFailed", "Could not create the release image"), log), tag.Name); err != nil { - return err - } - default: - return c.ensureRewriteJobImageRetrieved(release, job, mirror) + if err := c.syncPendingStableTag(release, tag); err != nil { + klog.Errorf("Error processing pending stable tag %s: %v", tag.Name, err) + errs = append(errs, err) } } - return nil + return utilerrors.NewAggregate(errs) } if len(pendingTags) > 1 { @@ -452,6 +359,100 @@ func (c *Controller) syncPending(release *releasecontroller.Release, pendingTags return nil } +func (c *Controller) syncPendingStableTag(release *releasecontroller.Release, tag *imagev1.TagReference) error { + id := releasecontroller.FindImageIDForTag(release.Source, tag.Name) + if len(id) == 0 { + klog.V(2).Infof("Waiting for release %s to be imported before we can retrieve metadata", tag.Name) + return nil + } + klog.V(2).Infof("Processing pending release %s", tag.Name) + rewriteValue := tag.Annotations[releasecontroller.ReleaseAnnotationRewrite] + if len(rewriteValue) == 0 { + klog.V(2).Infof("Rewriting pending release %s", tag.Name) + isi, err := c.imageClient.ImageStreamImages(release.Source.Namespace).Get(context.TODO(), fmt.Sprintf("%s@%s", release.Source.Name, id), metav1.GetOptions{}) + if err != nil { + return err + } + for _, m := range isi.Image.DockerImageManifests { + if m.Architecture == "amd64" { + isi, err = c.imageClient.ImageStreamImages(release.Source.Namespace).Get(context.TODO(), fmt.Sprintf("%s@%s", release.Source.Name, m.Digest), metav1.GetOptions{}) + if err != nil { + return err + } + break + } + } + metadata := &docker10.DockerImage{} + if len(isi.Image.DockerImageMetadata.Raw) == 0 { + return fmt.Errorf("could not fetch Docker image metadata for release %s", tag.Name) + } + if err := json.Unmarshal(isi.Image.DockerImageMetadata.Raw, metadata); err != nil { + return fmt.Errorf("malformed Docker image metadata on ImageStreamTag: %v", err) + } + var name string + if metadata.Config != nil { + name = metadata.Config.Labels["io.openshift.release"] + } + rewriteValue = fmt.Sprintf("%t", name != tag.Name) + if err := c.setReleaseAnnotation(release, tag.Annotations[releasecontroller.ReleaseAnnotationPhase], map[string]string{releasecontroller.ReleaseAnnotationRewrite: rewriteValue}, tag.Name); err != nil { + return err + } + return nil + } + rewrite := rewriteValue == "true" + + hash := fmt.Sprintf("%s-%d", tag.Name, *tag.Generation) + mirror, err := c.ensureReleaseMirror(release, tag.Name, hash) + if err != nil { + return err + } + if len(tag.Annotations[releasecontroller.ReleaseAnnotationImageHash]) == 0 { + if err := c.setReleaseAnnotation(release, tag.Annotations[releasecontroller.ReleaseAnnotationPhase], map[string]string{releasecontroller.ReleaseAnnotationImageHash: mirror.Annotations[releasecontroller.ReleaseAnnotationImageHash]}, tag.Name); err != nil { + return err + } + return nil + } + if mirror.Annotations[releasecontroller.ReleaseAnnotationImageHash] != tag.Annotations[releasecontroller.ReleaseAnnotationImageHash] { + return fmt.Errorf("unimplemented, should regenerate contents of tag %s", tag.Name) + } + _, err = c.ensureReleasePayload(release, tag) + if err != nil { + return err + } + var job *batchv1.Job + if rewrite { + job, err = c.ensureRewriteJob(release, tag.Name, mirror, `{}`) + } else { + job, err = c.ensureImportJob(release, tag.Name, mirror) + } + if err != nil || job == nil { + return err + } + payload, err := c.releasePayloadLister.ReleasePayloads(release.Target.Namespace).Get(tag.Name) + if err != nil { + if !errors.IsNotFound(err) { + return err + } + return c.ensureRewriteJobImageRetrieved(release, job, mirror) + } + phase := releasecontroller.GetReleasePhase(payload) + switch phase { + case releasecontroller.ReleasePhaseReady: + if err := c.markReleaseReady(release, nil, tag.Name); err != nil { + return err + } + c.precacheChangelog(release, tag) + case releasecontroller.ReleasePhaseFailed: + log, _, _ := ensureJobTerminationMessageRetrieved(c.podClient, job, "status.phase=Failed", "build", false) + if err := c.transitionReleasePhaseFailure(release, []string{releasecontroller.ReleasePhasePending}, releasecontroller.ReleasePhaseFailed, withLog(reasonAndMessage("CreateReleaseFailed", "Could not create the release image"), log), tag.Name); err != nil { + return err + } + default: + return c.ensureRewriteJobImageRetrieved(release, job, mirror) + } + return nil +} + func (c *Controller) syncReady(release *releasecontroller.Release) error { readyTags := releasecontroller.SortedRawReleaseTags(release, releasecontroller.ReleasePhaseReady) diff --git a/cmd/release-controller/sync_test.go b/cmd/release-controller/sync_test.go index 9437728dc..e05079b32 100644 --- a/cmd/release-controller/sync_test.go +++ b/cmd/release-controller/sync_test.go @@ -2,8 +2,11 @@ package main import ( "testing" + "time" + imagev1 "github.com/openshift/api/image/v1" "github.com/openshift/release-controller/pkg/apis/release/v1alpha1" + releasecontroller "github.com/openshift/release-controller/pkg/release-controller" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -92,3 +95,83 @@ func TestGetRejectionDetails(t *testing.T) { }) } } + +func TestCalculateSyncActions_StalePayloadPhaseDoesNotBlockOtherTags(t *testing.T) { + // Regression test: a tag whose ReleasePayload CRD has all-Unknown conditions + // (returning Pending from GetReleasePhase) but whose annotation says Accepted + // should not cause unrelated Ready tags to be misclassified as Pending. + // With PayloadPhases populated, GetTagPhase prefers the CRD value, so the + // stale tag appears as Pending. The Ready tag should still be counted as + // unready (not pending) so it proceeds through syncReady. + gen := int64(1) + stableRelease := &releasecontroller.Release{ + Config: &releasecontroller.ReleaseConfig{ + Name: "4-stable", + As: releasecontroller.ReleaseConfigModeStable, + }, + Source: &imagev1.ImageStream{ + ObjectMeta: metav1.ObjectMeta{ + Name: "release", + Namespace: "ocp", + }, + }, + Target: &imagev1.ImageStream{ + ObjectMeta: metav1.ObjectMeta{ + Name: "release", + Namespace: "ocp", + }, + Spec: imagev1.ImageStreamSpec{ + Tags: []imagev1.TagReference{ + { + Name: "4.7.0-fc.0", + Generation: &gen, + Annotations: map[string]string{ + releasecontroller.ReleaseAnnotationSource: "ocp/release", + releasecontroller.ReleaseAnnotationName: "4-stable", + releasecontroller.ReleaseAnnotationPhase: releasecontroller.ReleasePhaseAccepted, + }, + }, + { + Name: "4.22.5", + Generation: &gen, + Annotations: map[string]string{ + releasecontroller.ReleaseAnnotationSource: "ocp/release", + releasecontroller.ReleaseAnnotationName: "4-stable", + releasecontroller.ReleaseAnnotationPhase: releasecontroller.ReleasePhaseReady, + }, + }, + }, + }, + }, + PayloadPhases: map[string]string{ + "4.7.0-fc.0": releasecontroller.ReleasePhasePending, + "4.22.5": releasecontroller.ReleasePhaseReady, + }, + } + + adoptTags, pendingTags, _, _, _, _ := calculateSyncActions(stableRelease, time.Now()) + + // 4.7.0-fc.0 should be in pendingTags (CRD says Pending) + foundStale := false + for _, tag := range pendingTags { + if tag.Name == "4.7.0-fc.0" { + foundStale = true + } + } + if !foundStale { + t.Errorf("expected 4.7.0-fc.0 in pendingTags, got %v", releasecontroller.TagNames(pendingTags)) + } + + // 4.22.5 should NOT be in pendingTags or adoptTags — it should fall through + // to the default case (unreadyTagCount++) and be handled by syncReady + for _, tag := range pendingTags { + if tag.Name == "4.22.5" { + t.Errorf("4.22.5 should not be in pendingTags, but it was found") + } + } + for _, tag := range adoptTags { + if tag.Name == "4.22.5" { + t.Errorf("4.22.5 should not be in adoptTags, but it was found") + } + } +} From 0a74a05fe597ac206d945bf5a8bc75a173a1f539 Mon Sep 17 00:00:00 2001 From: Eris Hoxha Date: Mon, 13 Jul 2026 20:01:19 +0200 Subject: [PATCH 2/4] Bubble up conflict errors from syncPendingStableTag Conflict errors should bypass aggregation and return immediately so that sync() sees them as conflicts (fast retry) rather than generic errors (warning event + workqueue backoff). --- cmd/release-controller/sync.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cmd/release-controller/sync.go b/cmd/release-controller/sync.go index 252a70f40..88e152e58 100644 --- a/cmd/release-controller/sync.go +++ b/cmd/release-controller/sync.go @@ -295,6 +295,9 @@ func (c *Controller) syncPending(release *releasecontroller.Release, pendingTags var errs []error for _, tag := range pendingTags { if err := c.syncPendingStableTag(release, tag); err != nil { + if errors.IsConflict(err) { + return err + } klog.Errorf("Error processing pending stable tag %s: %v", tag.Name, err) errs = append(errs, err) } From a18a508c28125c7cc43d58b4cc2c6f0b27fb4c99 Mon Sep 17 00:00:00 2001 From: Eris Hoxha Date: Tue, 14 Jul 2026 14:11:31 +0200 Subject: [PATCH 3/4] Ensure accepted releases are mirrored to alternate image repository MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The mirror job (ensureReleaseMirrorJob) that copies releases to the alternate image repository (e.g. quay.io) was only called during syncReady. If a release transitions directly from Ready to Accepted before syncReady processes it — which happens when the release-payload-controller updates the CRD to Accepted before the release-controller runs syncReady — the mirror job is never created and the release never appears on quay.io. This caused all 4.23 multi nightlies after July 8 to be missing from quay.io/openshift-release-dev/dev-release, even though they were accepted by the release controller. Call ensureReleaseMirrorJob in syncAccepted as well. The function is idempotent (ensureJob checks for existing jobs), so releases that were already mirrored during syncReady are not affected. --- cmd/release-controller/sync.go | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/cmd/release-controller/sync.go b/cmd/release-controller/sync.go index 88e152e58..d885665e8 100644 --- a/cmd/release-controller/sync.go +++ b/cmd/release-controller/sync.go @@ -519,7 +519,26 @@ func (c *Controller) syncAccepted(release *releasecontroller.Release) error { klog.Infof("release=%s accepted=%v", release.Config.Name, releasecontroller.TagNames(acceptedTags)) } - if len(release.Config.Publish) == 0 || len(acceptedTags) == 0 { + if len(acceptedTags) == 0 { + return nil + } + + // Mirror accepted releases to the alternate image repository (e.g. quay.io). + // This is normally done in syncReady, but if a release transitions directly + // to Accepted (e.g. via the release-payload-controller updating the CRD + // before syncReady runs), the mirror job is never created. Calling it here + // as well ensures accepted releases are always mirrored. ensureReleaseMirrorJob + // is idempotent — it won't recreate an existing job. + for _, releaseTag := range acceptedTags { + mirror, err := releasecontroller.GetMirror(release, releaseTag.Name, c.releaseLister) + if err != nil { + klog.Errorf("Failed to identify `from` mirror for creation of release mirror job: %v", err) + } else if _, err := c.ensureReleaseMirrorJob(release, releaseTag.Name, mirror); err != nil { + klog.Errorf("Failed to create release mirror job: %v", err) + } + } + + if len(release.Config.Publish) == 0 { return nil } var errs []error From 260c141880ad8b62a66f2a27e97f01313c255965 Mon Sep 17 00:00:00 2001 From: Eris Hoxha Date: Tue, 14 Jul 2026 15:56:48 +0200 Subject: [PATCH 4/4] Revert "Ensure accepted releases are mirrored to alternate image repository" This reverts commit a18a508c28125c7cc43d58b4cc2c6f0b27fb4c99. --- cmd/release-controller/sync.go | 21 +-------------------- 1 file changed, 1 insertion(+), 20 deletions(-) diff --git a/cmd/release-controller/sync.go b/cmd/release-controller/sync.go index d885665e8..88e152e58 100644 --- a/cmd/release-controller/sync.go +++ b/cmd/release-controller/sync.go @@ -519,26 +519,7 @@ func (c *Controller) syncAccepted(release *releasecontroller.Release) error { klog.Infof("release=%s accepted=%v", release.Config.Name, releasecontroller.TagNames(acceptedTags)) } - if len(acceptedTags) == 0 { - return nil - } - - // Mirror accepted releases to the alternate image repository (e.g. quay.io). - // This is normally done in syncReady, but if a release transitions directly - // to Accepted (e.g. via the release-payload-controller updating the CRD - // before syncReady runs), the mirror job is never created. Calling it here - // as well ensures accepted releases are always mirrored. ensureReleaseMirrorJob - // is idempotent — it won't recreate an existing job. - for _, releaseTag := range acceptedTags { - mirror, err := releasecontroller.GetMirror(release, releaseTag.Name, c.releaseLister) - if err != nil { - klog.Errorf("Failed to identify `from` mirror for creation of release mirror job: %v", err) - } else if _, err := c.ensureReleaseMirrorJob(release, releaseTag.Name, mirror); err != nil { - klog.Errorf("Failed to create release mirror job: %v", err) - } - } - - if len(release.Config.Publish) == 0 { + if len(release.Config.Publish) == 0 || len(acceptedTags) == 0 { return nil } var errs []error