Uh oh!
There was an error while loading. Please reload this page.
OCPCRT-598: Fix CRD/annotation phase desync for release signing - #810
OCPCRT-598: Fix CRD/annotation phase desync for release signing#810hoxhaeris wants to merge 1 commit into
Conversation
The signer reads release phases from istag annotations via GetTagPhase() but syncAudit() never calls populatePayloadPhases(), so PayloadPhases is nil and it always falls back to the annotation. When the ReleasePayload CRD advances to Accepted faster than the sync loop can propagate phase transitions to the annotation, the annotation stays Pending permanently and the signer skips the release. This has been affecting OKD releases since PR openshift#792 migrated phase routing to CRD-derived phases without updating the signer path. Fix: 1. Call populatePayloadPhases() in syncAudit() so the signer reads phases from CRDs instead of annotations. 2. Add annotation reconciliation in syncAccepted() to bring stale annotations forward through the proper phase sequence when the CRD is ahead. 3. Handle Accepted payloads in syncPending() for the within-sync race where the payload advances while a tag is being processed.
@hoxhaeris: This pull request references OCPCRT-598 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 bug 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. |
📝 WalkthroughWalkthroughRelease auditing now populates payload phases before recording releases. Pending stable and non-stable releases transition accepted payloads through ChangesRelease phase reconciliation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
[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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 393-399: Preserve changelog pre-caching in all new fast paths: in
cmd/release-controller/sync.go lines 393-399, call c.precacheChangelog(release,
tag) after markReleaseReady; in lines 455-461, add the same call before
markReleaseAccepted; and in lines 544-550, add it between the two phase
transitions. Keep the existing error handling and transition ordering intact.
- Around line 555-559: Update the reconciliation flow around the default branch
and subsequent publish block so skipped annotation phases prevent publishing.
Track that reconciliation was blocked or gate the publish path on the newest
acceptedTags entry having annotation phase Accepted, and return before using
acceptedTags[0] when its phase is Rejected, Failed, or unexpected; preserve the
existing warning and successful Accepted flow.
🪄 Autofix
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: Pro Plus
Run ID: b1907287-203d-49b0-94bc-748d2d136fba
📒 Files selected for processing (2)
cmd/release-controller/audit.gocmd/release-controller/sync.go
| case releasecontroller.ReleasePhaseAccepted: | ||
| if err := c.markReleaseReady(release, nil, tag.Name); err != nil { | ||
| return err | ||
| } | ||
| if err := c.markReleaseAccepted(release, nil, tag.Name); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Preserve changelog pre-caching on every new fast path.
Each new Pending/Accepted → Ready → Accepted path bypasses the c.precacheChangelog call used by the existing Ready path.
cmd/release-controller/sync.go#L393-L399: callc.precacheChangelog(release, tag)aftermarkReleaseReady.cmd/release-controller/sync.go#L455-L461: add the same call beforemarkReleaseAccepted.cmd/release-controller/sync.go#L544-L550: add the same call between the two phase transitions.
📍 Affects 1 file
cmd/release-controller/sync.go#L393-L399(this comment)cmd/release-controller/sync.go#L455-L461cmd/release-controller/sync.go#L544-L550
🤖 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 393 - 399, Preserve changelog
pre-caching in all new fast paths: in cmd/release-controller/sync.go lines
393-399, call c.precacheChangelog(release, tag) after markReleaseReady; in lines
455-461, add the same call before markReleaseAccepted; and in lines 544-550, add
it between the two phase transitions. Keep the existing error handling and
transition ordering intact.
| default: | ||
| klog.Warningf("Tag %s has payload Accepted but annotation phase %q; skipping reconciliation", tag.Name, annotationPhase) | ||
| continue | ||
| } | ||
| return nil |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Block publishing when annotation reconciliation is skipped.
When acceptedTags[0] has annotation phase Rejected, Failed, or another unexpected value, this branch logs and continues. After the loop, Line 562 still enters the publish block and uses acceptedTags[0]. The controller can publish a payload whose annotation is not Accepted. Track a blocked reconciliation result and return before publishing, or gate publishing on the newest tag having annotation phase Accepted.
🤖 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 555 - 559, Update the
reconciliation flow around the default branch and subsequent publish block so
skipped annotation phases prevent publishing. Track that reconciliation was
blocked or gate the publish path on the newest acceptedTags entry having
annotation phase Accepted, and return before using acceptedTags[0] when its
phase is Rejected, Failed, or unexpected; preserve the existing warning and
successful Accepted flow.
@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. |
hoxhaeris
commented
Aug 7, 2026
Closing in favor of #811 which covers the signer fix. The annotation reconciliation changes from this PR are not needed for now since the signer -- the primary consumer -- now reads from CRDs. |
Summary
The signer reads release phases from istag annotations, but
syncAudit()never callspopulatePayloadPhases(), so it always falls back to the annotation. When the ReleasePayload CRD advances to Accepted faster than the sync loop can propagate phase transitions to the annotation (common for OKD releases with few blocking verification jobs), the annotation stays Pending permanently and the signer skips the release.This has been affecting OKD releases since PR #792 migrated phase routing to CRD-derived phases without updating the signer path. Currently 67 of 224 tags on
release-scosin origin namespace have annotations stuck at Pending while CRDs show Accepted or Rejected.Changes
Complete the signer migration (
audit.go): callpopulatePayloadPhases()insyncAudit()so the signer reads phases from CRDs instead of annotations.Annotation reconciliation (
sync.go): insyncAccepted(), before the publish steps, check if any accepted tag has a stale annotation and bring it forward through the proper phase sequence (Pending -> Ready -> Accepted).Handle within-sync race (
sync.go): insyncPending(), handle the case whereGetReleasePhase(payload)returns Accepted while a tag is still being processed as pending.Fixes: https://redhat.atlassian.net/browse/OCPCRT-598
Summary by CodeRabbit