Prevent sync phase errors from blocking subsequent phases#796
Prevent sync phase errors from blocking subsequent phases#796hoxhaeris wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughThe release controller now aggregates non-conflict synchronization errors across stages and pending tags, extracts stable pending-tag processing into a helper, and adds regression coverage for stale payload phases. ChangesRelease synchronization
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant ControllerSync
participant SyncPending
participant SyncPendingStableTag
participant ReleasePayload
ControllerSync->>SyncPending: process pending releases
SyncPending->>SyncPendingStableTag: process each stable pending tag
SyncPendingStableTag->>ReleasePayload: read payload phase
ReleasePayload-->>SyncPendingStableTag: return phase
SyncPendingStableTag-->>SyncPending: return per-tag result
SyncPending-->>ControllerSync: return aggregated errors
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
A regression in 1dcc49e ("Sync pending migration part 2", PR openshift#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.
hoxhaeris
force-pushed
the
hoxhaeris/sync-phase-error-isolation
branch
from
6ac3284 to
309227f
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hoxhaeris 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 |
openshift-ci
Bot
added
the
approved
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
@CodeRabbit help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![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 `@cmd/release-controller/sync.go`:
- Around line 292-303: The stable-tag branch in Controller.syncPending must
preserve conflict errors from syncPendingStableTag instead of hiding them inside
utilerrors.NewAggregate. Detect a conflict while processing pendingTags and
return it directly, while retaining aggregation for non-conflict errors and the
existing per-tag logging.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 94b16720-1200-43df-90f0-d43b4615a25b
📒 Files selected for processing (2)
cmd/release-controller/sync.gocmd/release-controller/sync_test.go
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).
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.
|
Added a third commit: call The mirror job that copies releases to the alternate image repository (e.g. quay.io) was only called during Calling |
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
…sitory" This reverts commit a18a508.
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/release-controller/sync.go (1)
407-407: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winPrevent potential nil pointer dereference on
tag.Generation.The
Generationfield inimagev1.TagReferenceis a pointer (*int64). If a tag is newly submitted or partially formed,tag.Generationcould beniluntil the API server populates it. Dereferencing it directly can cause a panic and crash the controller.🔒️ Proposed fix to add a nil check
- hash := fmt.Sprintf("%s-%d", tag.Name, *tag.Generation) + var gen int64 + if tag.Generation != nil { + gen = *tag.Generation + } + hash := fmt.Sprintf("%s-%d", tag.Name, gen)🤖 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 `@cmd/release-controller/sync.go` at line 407, Update the hash construction near the tag processing logic to handle a nil tag.Generation before dereferencing it. Preserve the existing tag.Name-generation hash format for populated generations, and define the appropriate safe behavior for newly submitted or partially formed tags without allowing the controller to panic.
🧹 Nitpick comments (1)
cmd/release-controller/sync.go (1)
127-129: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the event reason to accurately reflect the failure.
syncAcceptedhandles the publish steps for accepted tags. LoggingUnableToVerifyReleaseappears to be a copy-paste error fromsyncReady. Consider updating the event reason to better describe the publishing failure.♻️ Proposed refactor
- c.eventRecorder.Eventf(release.Source, corev1.EventTypeWarning, "UnableToVerifyRelease", "%v", err) + c.eventRecorder.Eventf(release.Source, corev1.EventTypeWarning, "UnableToPublishRelease", "%v", err)🤖 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 `@cmd/release-controller/sync.go` around lines 127 - 129, Update the Eventf call in syncAccepted to use an event reason describing the publish failure instead of the copied UnableToVerifyRelease reason, while preserving the existing warning type, message formatting, and syncErrors handling.
🤖 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.
Outside diff comments:
In `@cmd/release-controller/sync.go`:
- Line 407: Update the hash construction near the tag processing logic to handle
a nil tag.Generation before dereferencing it. Preserve the existing
tag.Name-generation hash format for populated generations, and define the
appropriate safe behavior for newly submitted or partially formed tags without
allowing the controller to panic.
---
Nitpick comments:
In `@cmd/release-controller/sync.go`:
- Around line 127-129: Update the Eventf call in syncAccepted to use an event
reason describing the publish failure instead of the copied
UnableToVerifyRelease reason, while preserving the existing warning type,
message formatting, and syncErrors handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 59f9d37b-f6c8-46df-b7d9-cd63549c9e49
📒 Files selected for processing (1)
cmd/release-controller/sync.go
|
@hoxhaeris: 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. |
1 participant
Summary
ReleasePayloadCRDs (with all-Unknown conditions) causesyncPendingto error, aborting the entire4-stablesync loop and preventingsyncReady/syncAcceptedfrom ever running4.22.5verification jobs were never launched because of this, tests stuck pending for 2+ days, requiring ART to manually force-accept (ART-21568)syncPending,syncReady,syncAccepted) are now collected instead of fatal, all three phases always runsyncPendingare isolated viasyncPendingStableTagso one broken tag cannot block otherssyncReadyRoot Cause
PR #792 changed
calculateSyncActionsto useGetTagPhase()(which prefers CRD-derived phases) instead of reading annotations directly. Old tags like4.7.0-fc.0haveReleasePayloadCRDs that were never backfilled, all conditions areUnknown, soGetReleasePhase()returnsPendingeven though the annotation saysAccepted. This causes the tag to re-entersyncPending, hit a mirror hash mismatch error, and abort the sync loop.Additionally, the mirror job (
ensureReleaseMirrorJob) was only called duringsyncReady. If a release transitions toAcceptedbeforesyncReadyprocesses it (due to the release-payload-controller updating the CRD first), the mirror job is never created and the image never appears on quay.io.Summary by CodeRabbit