🐛 fix: allow reconciliation of deadline-exceeded ClusterObjectSets#2643
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
joelanford
force-pushed
the
fix/cos-deadline-exceeded-archival
branch
from
9dece97 to
a586063
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes a reconciliation dead-end where ClusterObjectSet (COS) updates were being dropped once a revision hit ProgressDeadlineExceeded, preventing stuck revisions from being archived and cleaned up. It removes the update-blocking predicate, makes progress-deadline handling “sticky” in status updates, and adds a deadline-aware rate limiter plus an E2E scenario to validate archival cleanup.
Changes:
- Remove the
ProgressDeadlineExceeded-skipping watch predicate and introduce a controllerRateLimiterthat caps exponential backoff at the progress deadline. - Refactor progress-deadline computation into a shared
durationUntilDeadlinehelper and adjust progressing/retrying status updates to preferProgressDeadlineExceededonce exceeded. - Add an E2E scenario (and step helper) that archives a deadline-exceeded COS and verifies resource cleanup.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
internal/operator-controller/controllers/clusterobjectset_controller.go |
Removes the skip predicate, adds deadline-aware rate limiting, refactors deadline computation, and changes progressing/retrying condition behavior when the deadline is exceeded. |
test/e2e/steps/steps.go |
Adds a new Godog step to patch a COS lifecycle state to Archived. |
test/e2e/features/revision.feature |
Adds an E2E scenario that forces ProgressDeadlineExceeded, archives the COS, and asserts resources are removed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| When ClusterObjectSet "${COS_NAME}" lifecycle is set to "Archived" | ||
| Then ClusterObjectSet "${COS_NAME}" is archived | ||
| And resource "configmap/test-configmap" is eventually not found | ||
| And resource "deployment/test-deployment" is eventually not found |
There was a problem hiding this comment.
It does not seems to be testing the same scenario @joelanford
Could we ensure the same scenario here?
There was a problem hiding this comment.
- User installs a ClusterExtension. The CE controller creates COS-rev-1.
- COS-rev-1 gets stuck (e.g. a Deployment never becomes ready). After ProgressDeadlineMinutes, the reconciler sets Progressing=False/ProgressDeadlineExceeded.
- User updates the ClusterExtension. The CE controller creates COS-rev-2.
- COS-rev-2 rolls out successfully. It patches COS-rev-1 with lifecycleState: Archived so the old revision gets cleaned up.
- The watch predicate sees COS-rev-1 has ProgressDeadlineExceeded and drops the event.
- COS-rev-1 never reconciles, never processes the archival, and stays stuck forever.
There was a problem hiding this comment.
Is it not the same? Making a CE that stamps out the COS-1 and COS-2 such that the CE reconciler eventually tries to set COS-1 as archived is the same thing, but just more ceremony around a standalone COS that exceeds the deadline and then is directly archived by the test code.
I'll verify manually to make sure. Is there something else happening with the CE/COS interaction that would make the COS-only test in the PR different?
There was a problem hiding this comment.
we are not upgrading, checking it stuck and then, checking that is possible to change the resource and the changes will be reconciled. WDY about we add a test with?
joelanford
force-pushed
the
fix/cos-deadline-exceeded-archival
branch
from
a586063 to
46dcf54
Compare
joelanford
force-pushed
the
fix/cos-deadline-exceeded-archival
branch
from
46dcf54 to
aea3d72
Compare
There was a problem hiding this comment.
Pull request overview
This PR adjusts ClusterObjectSet (COS) reconciliation behavior so revisions that hit ProgressDeadlineExceeded still reconcile (enabling archival/cleanup), while making the deadline-exceeded state “sticky” to avoid the previous reconcile loop. It also adds a deadline-capped rate limiter and an e2e scenario to verify archival cleans up resources after a progress deadline is exceeded.
Changes:
- Remove the watch predicate that dropped COS update events when
ProgressDeadlineExceededwas set, allowing spec patches like archival to be reconciled. - Refactor progress-deadline handling (shared
durationUntilDeadline, stickyProgressDeadlineExceeded, requeue-at-deadline behavior) and add a deadline-aware rate limiter to cap backoff until the deadline. - Add an e2e scenario and a new step to patch COS lifecycle to
Archived, plus improve e2e cleanup synchronization.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/steps/steps.go | Adds a Godog step to patch a ClusterObjectSet’s spec.lifecycleState. |
| test/e2e/steps/hooks.go | Makes scenario cleanup wait for concurrent deletions to finish. |
| test/e2e/features/revision.feature | Adds an e2e scenario covering archival + resource cleanup after ProgressDeadlineExceeded. |
| internal/operator-controller/controllers/clusterobjectset_controller.go | Removes the skip predicate; makes deadline handling sticky; adds deadline-aware requeue + custom rate limiter. |
| internal/operator-controller/controllers/clusterobjectset_controller_test.go | Updates the progress-deadline requeue expectation to match the new scheduling behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cos := &ocv1.ClusterObjectSet{} | ||
| if err := r.client.Get(context.Background(), item.NamespacedName, cos); err != nil { |
There was a problem hiding this comment.
Our use of the rate limiter uses the informer cache which is in-memory and does not block.
joelanford
force-pushed
the
fix/cos-deadline-exceeded-archival
branch
from
aea3d72 to
e1b56c2
Compare
joelanford
force-pushed
the
fix/cos-deadline-exceeded-archival
branch
from
e1b56c2 to
00c3c75
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes a reconciliation dead-end for ClusterObjectSet (COS) revisions that hit ProgressDeadlineExceeded, ensuring they can still reconcile (e.g., to process archival) and improving deadline-related requeue behavior so deadline status is set promptly.
Changes:
- Removes the update predicate that dropped COS updates when
ProgressDeadlineExceededwas set. - Refactors progress-deadline handling into shared helpers (
durationUntilDeadline,requeueForDeadline) and adds a deadline-aware rate limiter to cap exponential backoff at the deadline. - Extends e2e coverage with a new scenario that archives a deadline-exceeded COS and verifies resource cleanup; improves scenario cleanup to wait for deletions to complete.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/steps/steps.go | Adds an e2e step to patch a COS lifecycle state (used for archival in scenarios). |
| test/e2e/steps/hooks.go | Ensures scenario cleanup waits for concurrent deletion commands to finish. |
| test/e2e/features/revision.feature | Adds an e2e scenario that drives a COS into ProgressDeadlineExceeded, archives it, and asserts resources are removed. |
| internal/operator-controller/controllers/clusterobjectset_controller.go | Removes the deadline-exceeded predicate; adds deadline-aware requeue/rate-limiting and updates Progressing condition handling. |
| internal/operator-controller/controllers/clusterobjectset_controller_test.go | Updates expected requeue timing behavior for progress deadline handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| remaining, hasDeadline := c.durationUntilDeadline(cos) | ||
| isDeadlineExceeded := hasDeadline && remaining <= 0 | ||
|
|
There was a problem hiding this comment.
This is a good point. requeueForDeadline should not perform the calculation again with a different "now" timestamp. Perhaps instead we could simply pass the result of the original calculation into requeueForDeadline.
| // deadlineAwareRateLimiter wraps a delegate rate limiter and caps the backoff | ||
| // duration to the time remaining until the COS progress deadline (+2s), ensuring | ||
| // that ProgressDeadlineExceeded is set promptly even during exponential backoff. | ||
| type deadlineAwareRateLimiter struct { | ||
| delegate workqueue.TypedRateLimiter[ctrl.Request] |
joelanford
force-pushed
the
fix/cos-deadline-exceeded-archival
branch
2 times, most recently
from
9768528 to
ef08a54
Compare
There was a problem hiding this comment.
Pull request overview
Fixes a reconciliation dead-end where ClusterObjectSet (COS) revisions marked ProgressDeadlineExceeded would no longer reconcile, preventing actions like archival/cleanup from being processed.
Changes:
- Removes the update predicate that filtered out COS updates after
ProgressDeadlineExceeded, and adjusts progressing/deadline logic to avoid the prior status flip-flop loop. - Introduces shared deadline calculation (
durationUntilDeadline), deadline-driven requeueing (requeueForDeadline), and a deadline-capped rate limiter to ensure the deadline condition is set promptly even under backoff. - Extends E2E coverage with a scenario that forces
ProgressDeadlineExceeded, archives the COS, and asserts underlying resources are removed; adds a step to patch COS lifecycle state.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
internal/operator-controller/controllers/clusterobjectset_controller.go |
Refactors progress-deadline handling, removes the predicate, adds deadline-aware requeue and rate limiting. |
internal/operator-controller/controllers/clusterobjectset_controller_test.go |
Updates expected requeue timing to match new deadline requeue logic. |
test/e2e/steps/steps.go |
Adds a new step to patch ClusterObjectSet.spec.lifecycleState. |
test/e2e/steps/hooks.go |
Adjusts scenario cleanup deletion behavior. |
test/e2e/features/revision.feature |
Adds an E2E scenario covering archival/cleanup of a deadline-exceeded COS. |
Comments suppressed due to low confidence (1)
test/e2e/steps/hooks.go:205
- ScenarioCleanup adds
--wait=falsetokubectl delete, but the PR description says cleanup now waits for resource deletions to complete. As written, deletions are still fire-and-forget goroutines with explicit non-waiting, so scenarios can finish while resources remain terminating (and any follow-on checks can observe leftovers). If the intent is to wait, run deletes synchronously (or collect goroutines with a WaitGroup) and/or add an explicitkubectl wait --for=deletephase.
for _, r := range forDeletion {
go func(res resource) {
args := []string{"delete", res.kind, res.name, "--ignore-not-found=true", "--wait=false"}
if res.namespace != "" {
args = append(args, "-n", res.namespace)
}
if _, err := k8sClient(args...); err != nil {
logger.Info("Error deleting resource", "name", res.name, "namespace", res.namespace, "stderr", stderrOutput(err))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pd := cos.Spec.ProgressDeadlineMinutes | ||
| if pd <= 0 { | ||
| return -1, false | ||
| } | ||
| // Succeeded is a latch — once set, it's never cleared. A revision that | ||
| // has already succeeded should not be blocked by the deadline, even if | ||
| // it temporarily goes back to InTransition (e.g., recovery after drift). | ||
| if meta.IsStatusConditionTrue(cos.Status.Conditions, ocv1.ClusterObjectSetTypeSucceeded) { | ||
| return -1, false |
| // requeueForDeadline returns a Result that requeues at the progress deadline | ||
| // if one is configured and has not yet been exceeded. This ensures that | ||
| // ProgressDeadlineExceeded is set promptly even when no object events occur. | ||
| func (c *ClusterObjectSetReconciler) requeueForDeadline(cos *ocv1.ClusterObjectSet) ctrl.Result { | ||
| if remaining, hasDeadline := c.durationUntilDeadline(cos); hasDeadline && remaining > 0 { | ||
| return ctrl.Result{RequeueAfter: remaining} |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2643 +/- ##
==========================================
+ Coverage 67.98% 68.06% +0.08%
==========================================
Files 144 145 +1
Lines 10595 10623 +28
==========================================
+ Hits 7203 7231 +28
- Misses 2870 2871 +1
+ Partials 522 521 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
joelanford
force-pushed
the
fix/cos-deadline-exceeded-archival
branch
2 times, most recently
from
393ff45 to
ad0859e
Compare
openshift-ci
Bot
added
the
needs-rebase
|
@joelanford can you please rebase this one and address the Copilot comments so that we can review and see if we can go with. |
|
Ran out of time to get back to this today. Hopefully tomorrow! This PR has some flaws around recomputation of the remaining time until deadline which can cause funky things to happen if we're reconciling just before the deadline and then cross over the deadline threshold prior to the rate limiter logic running, which can cause requeues not to happen right at the deadline. Might need a bit more logic in the recomputation code before it's ready. Hopefully that extra code isn't too brittle. Maybe it will be fine as a stopgap if the plan is to eventually move the progress deadline stuff to be a concern only of the ClusterExtension (leaving ClusterObjectSet with just a single "Ready" or similar condition) |
openshift-ci
Bot
removed
the
needs-rebase
joelanford
force-pushed
the
fix/cos-deadline-exceeded-archival
branch
from
4cedf95 to
14acf56
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes a reconciliation dead-end for ClusterObjectSet revisions that have hit ProgressDeadlineExceeded, ensuring they can still be reconciled (notably to process archival/spec updates) and that the deadline condition is set promptly even during error backoff.
Changes:
- Removes the update-event predicate that previously blocked reconciliation for
ProgressDeadlineExceededrevisions. - Makes progress-deadline handling “sticky” via
markAsProgressing(..., isDeadlineExceeded)and introduces a shareddurationUntilDeadlinehelper. - Adds a deadline-aware controller rate limiter and extends E2E coverage with a scenario validating archival cleans up resources after
ProgressDeadlineExceeded.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
internal/operator-controller/controllers/clusterobjectset_controller.go |
Removes the blocking predicate, wires a deadline-aware rate limiter, and updates progressing/retrying logic to respect deadline-exceeded state while continuing reconciliation. |
internal/operator-controller/controllers/clusterobjectset_controller_test.go |
Adjusts progress-deadline requeue expectation to match new deadline calculation behavior. |
internal/operator-controller/controllers/progress_deadline.go |
Adds deadlineAwareRateLimiter and the shared durationUntilDeadline helper. |
internal/operator-controller/controllers/progress_deadline_test.go |
Adds unit tests covering the new deadline-aware rate limiter behavior. |
test/e2e/steps/steps.go |
Adds a new E2E step to patch a ClusterObjectSet lifecycle state. |
test/e2e/features/revision.feature |
Adds an E2E scenario validating that archiving a ProgressDeadlineExceeded COS cleans up its resources. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Never mind: just needed to pick up the latest changes from |
joelanford
force-pushed
the
fix/cos-deadline-exceeded-archival
branch
from
14acf56 to
7e46425
Compare
pedjak
left a comment
pedjak
left a comment
There was a problem hiding this comment.
Additional review comments — focused on test coverage gaps, behavioral subtleties, and edge cases.
joelanford
force-pushed
the
fix/cos-deadline-exceeded-archival
branch
2 times, most recently
from
6229df1 to
81daf9c
Compare
pedjak
left a comment
pedjak
left a comment
There was a problem hiding this comment.
Follow-up comments on the updated PR — one test coverage suggestion and two minor nits.
Previously, a watch predicate dropped update events for ClusterObjectSets with ProgressDeadlineExceeded, preventing them from being archived or cleaned up. Remove the predicate and replace the inline deadline logic in Reconcile with an enforceProgressDeadline() helper and a progressDeadline() function that computes the absolute deadline from spec and metadata. Add a deadline-aware rate limiter that caps exponential backoff at the deadline and allows one immediate requeue when it passes, without inspecting status conditions. Add an e2e scenario that forces ProgressDeadlineExceeded, archives the COS, and verifies resource cleanup.
joelanford
force-pushed
the
fix/cos-deadline-exceeded-archival
branch
from
81daf9c to
feedd23
Compare
openshift-ci
Bot
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, pedjak 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-merge-bot
Bot
merged commit 74e39f5
into
operator-framework:main
Description
The
skipProgressDeadlineExceededPredicateblocked all update events for COS objectswith
ProgressDeadlineExceeded, which prevented archival of stuck revisions — thelifecycle state patch was silently dropped.
This PR:
markAsProgressingto setProgressDeadlineExceededinstead ofRollingOut/Retryingwhen the deadline has been exceeded, preventing the reconcileloop the predicate was masking
ProgressDeadlineExceededrather than clearing theerror and stopping requeue. This allows revisions to recover if a transient error
resolves itself, even after the deadline was exceeded
durationUntilDeadlineas a shared helper for deadline computationdeadlineAwareRateLimiterthat caps exponential backoff at the deadline soProgressDeadlineExceededis set promptly even during error retriesreconcile, using the remaining duration fromdurationUntilDeadlinedirectly forRequeueAfterdurationUntilDeadlineand recovery fromProgressDeadlineExceededtoSucceededProgressDeadlineExceededarchival/cleanup and for recoveryfrom
ProgressDeadlineExceededtoSucceededAddresses feedback from:
#2610 (comment)
Reviewer Checklist