Conversation
WalkthroughThe changes introduce an EndOfSupport field to ReleaseConfig and implement guards across the release controller to prevent new periodic jobs, image synchronization, and mark releases appropriately in HTTP responses when EndOfSupport is enabled. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 NOT APPROVED This pull-request has been approved by: sarthakpurohit The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
243-259:⚠️ Potential issue | 🟡 MinorEndOfSupport guard doesn't suppress the interval-delay re-queue path.
When
EndOfSupportistrueand afirstTagexists,IsReleaseDelayedForIntervalstill executes and can setqueueAfter > 0. This causessync()to re-queue the release (line 130) at every interval even though no new tag will ever be created. For streams withMinCreationIntervalSecondsconfigured, this fires repeated unnecessary sync cycles for the lifetime of the EndOfSupport state.
hasNewImagesis already false at this point, so theMaxUnreadyReleasescheck also redundantly runs.🛠️ Proposed fix — break out of the switch after setting the EndOfSupport gate
default: // gate creating new releases when end of support, at max unready, or in the cooldown interval if release.Config.EndOfSupport { klog.V(2).Infof("Release %s has reached end of support, will not launch new tags", release.Config.Name) hasNewImages = false + break } if release.Config.MaxUnreadyReleases > 0 && unreadyTagCount >= release.Config.MaxUnreadyReleases {Using
breakinside aswitchcase exits the switch entirely, skipping both theMaxUnreadyReleasesandIsReleaseDelayedForIntervalchecks, and preventingqueueAfterfrom being populated for EndOfSupport releases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/release-controller/sync.go` around lines 243 - 259, When release.Config.EndOfSupport is true we must avoid running the subsequent checks (MaxUnreadyReleases and IsReleaseDelayedForInterval) that can set queueAfter and requeue the release; inside the switch case where you set hasNewImages = false for EndOfSupport, add a break/exit from the switch immediately after setting the flag so the logic that calls releasecontroller.IsReleaseDelayedForInterval(firstTag) and assigns queueAfter is skipped (refer to symbols: release.Config.EndOfSupport, hasNewImages, firstTag, releasecontroller.IsReleaseDelayedForInterval, queueAfter).
🧹 Nitpick comments (1)
cmd/release-controller-api/http.go (1)
1718-1731: Triplicated EndOfSupport + delay-computation block — extract a shared helper.The identical
if EndOfSupport { … } else if non-stable && tags { … }pattern is copy-pasted intohttpReleases,httpReleaseStreamTable, andhttpDashboardOverview. Additionally,var delays []stringis declared at the outer scope but only ever populated in theelse ifbranch.♻️ Suggested helper extraction
// computeReleaseDelay returns the appropriate *ReleaseDelay for a stream, // or nil if no delay applies. func computeReleaseDelay(r *releasecontroller.Release, tags []*imagev1.TagReference) *ReleaseDelay { if r.Config.EndOfSupport { return &ReleaseDelay{Message: "Release has reached end of support"} } if r.Config.As == releasecontroller.ReleaseConfigModeStable || len(tags) == 0 { return nil } var delays []string if ok, _, queueAfter := releasecontroller.IsReleaseDelayedForInterval(r, tags[0]); ok { delays = append(delays, fmt.Sprintf("waiting for %s", queueAfter.Truncate(time.Second))) } if r.Config.MaxUnreadyReleases > 0 && releasecontroller.CountUnreadyReleases(r, tags) >= r.Config.MaxUnreadyReleases { delays = append(delays, fmt.Sprintf("no more than %d pending", r.Config.MaxUnreadyReleases)) } if len(delays) > 0 { return &ReleaseDelay{Message: fmt.Sprintf("Next release may not start: %s", strings.Join(delays, ", "))} } return nil }Then in each handler replace the current block with:
s.Delayed = computeReleaseDelay(r, s.Tags)Also applies to: 1970-1983, 2129-2142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/release-controller-api/http.go` around lines 1718 - 1731, Extract a shared helper function computeReleaseDelay(r *releasecontroller.Release, tags []*imagev1.TagReference) *ReleaseDelay that encapsulates the repeated logic: check r.Config.EndOfSupport and return &ReleaseDelay{Message:"Release has reached end of support"}; return nil if r.Config.As == releasecontroller.ReleaseConfigModeStable || len(tags) == 0; otherwise build a local delays []string, append the "waiting for ..." message when releasecontroller.IsReleaseDelayedForInterval(r, tags[0]) returns ok, append the "no more than N pending" message when r.Config.MaxUnreadyReleases > 0 and releasecontroller.CountUnreadyReleases(r, tags) >= r.Config.MaxUnreadyReleases, and return &ReleaseDelay{Message: fmt.Sprintf("Next release may not start: %s", strings.Join(delays, ", "))} when non-empty; replace the duplicated blocks in httpReleases, httpReleaseStreamTable, and httpDashboardOverview (and the other occurrences) with s.Delayed = computeReleaseDelay(r, s.Tags).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/release-controller/sync.go`:
- Around line 243-259: When release.Config.EndOfSupport is true we must avoid
running the subsequent checks (MaxUnreadyReleases and
IsReleaseDelayedForInterval) that can set queueAfter and requeue the release;
inside the switch case where you set hasNewImages = false for EndOfSupport, add
a break/exit from the switch immediately after setting the flag so the logic
that calls releasecontroller.IsReleaseDelayedForInterval(firstTag) and assigns
queueAfter is skipped (refer to symbols: release.Config.EndOfSupport,
hasNewImages, firstTag, releasecontroller.IsReleaseDelayedForInterval,
queueAfter).
---
Nitpick comments:
In `@cmd/release-controller-api/http.go`:
- Around line 1718-1731: Extract a shared helper function computeReleaseDelay(r
*releasecontroller.Release, tags []*imagev1.TagReference) *ReleaseDelay that
encapsulates the repeated logic: check r.Config.EndOfSupport and return
&ReleaseDelay{Message:"Release has reached end of support"}; return nil if
r.Config.As == releasecontroller.ReleaseConfigModeStable || len(tags) == 0;
otherwise build a local delays []string, append the "waiting for ..." message
when releasecontroller.IsReleaseDelayedForInterval(r, tags[0]) returns ok,
append the "no more than N pending" message when r.Config.MaxUnreadyReleases > 0
and releasecontroller.CountUnreadyReleases(r, tags) >=
r.Config.MaxUnreadyReleases, and return &ReleaseDelay{Message: fmt.Sprintf("Next
release may not start: %s", strings.Join(delays, ", "))} when non-empty; replace
the duplicated blocks in httpReleases, httpReleaseStreamTable, and
httpDashboardOverview (and the other occurrences) with s.Delayed =
computeReleaseDelay(r, s.Tags).
|
/retest |
|
@sarthakpurohit: 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
Currently, the release controller has an EndOfLife flag that completely hides releases from the UI.
As a trick, minCreationIntervalSeconds is increased to a big number.
However, there's a need for an intermediate state where a release stream:
This is useful for release streams that are no longer actively maintained but should still be accessible.
Usage
Set "endOfSupport": true in the release configuration to enable this behavior:
Summary by CodeRabbit