Prevent sync phase errors from blocking subsequent phases

A regression in 1dcc49e ("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.

Author: hoxhaeris

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)
 

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")
+		}
+	}
+}