Repository navigation
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 25 functions across 8 files. (6 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title accurately names the main change: the COD controller now enforces revision-level progression deadlines. It follows the 🐛 bug-fix prefix convention in the description template and includes th… Description check The description covers the change in detail. It explains the deadline start and end points, the shared phase budget, the exclusion of COS creation wait time, the direct COD enforcement with ProgressDe… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title accurately names the main change: the COD controller now enforces revision-level progression deadlines. It follows the 🐛 bug-fix prefix convention in the description template and includes the Jira key. The title is 66 characters, which is within a normal title length.
Full details: Description check
-
Explanation
The description covers the change in detail. It explains the deadline start and end points, the shared phase budget, the exclusion of COS creation wait time, the direct COD enforcement with ProgressDeadlineExceeded, backoff capping, continued reconciliation after timeout, and preserved availability. It also lists validation steps and links the Jira issue. The Reviewer Checklist is kept but none of its items are ticked, and the 🐛 prefix from the template is not stated as a prefix on the title line. These are minor gaps; the core content is complete and matches the template's Description section.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 25 functions across 8 files. (6 skipped: 6 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
f0e33ed to
8293ba7
Compare
Document one deadline for the entire revision, starting at COS creation. Time spent waiting to create the COS is outside the MVP budget. Cover a deployment older than its deadline, five phases sharing one budget, late completion, later health regressions, and a fresh budget for the next revision without depending on phase completion timestamps. Signed-off-by: Fabricio Aguiar <fabricio.aguiar@gmail.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
8293ba7 to
c01bfa4
Compare
There was a problem hiding this comment.
let's also remove this. If you want to move the rest of the clean up (removing checks for Ready=False/ProgressDeadlineExceeded on the COS) for a later PR to keep this one small, I think that's fine
There was a problem hiding this comment.
Done. COD no longer copies progressDeadlineMinutes into new COS revisions or consults their Ready=False/ProgressDeadlineExceeded condition. It calculates the budget from its template and the revision's creation and completion timestamps. COS deadline support remains for ClusterExtension installations until that path is migrated.
| if !hasDeadline { | ||
| r.pastDeadline.Delete(item) | ||
| return backoff | ||
| } |
There was a problem hiding this comment.
I wonder if we should distinguish between errors and no deadline, something like:
| if !hasDeadline { | |
| r.pastDeadline.Delete(item) | |
| return backoff | |
| } | |
| remaining, hasDeadline, err := r.remaining(item) | |
| if err != nil { | |
| // Couldn't determine the deadline (transient read error). Do NOT touch the | |
| // past-deadline marker — deleting it here is lets intermittent errors | |
| // re-arm the one-shot immediate requeue. We have no remaining | |
| // value to cap against, so fall back to the delegate backoff. | |
| return backoff | |
| } | |
| if !hasDeadline { | |
| r.pastDeadline.Delete(item) | |
| return backoff | |
| } |
There was a problem hiding this comment.
Done. Deadline lookups now return errors separately from the absence of a deadline. A transient Get or List failure uses the delegate backoff without clearing the expired-deadline marker. Regression tests cover intermittent failures and verify that recovery does not re-arm the immediate retry.
| remaining: func(req ctrl.Request) (time.Duration, bool) { | ||
| ctx := context.Background() | ||
| cod := &ocv1.ClusterObjectDeployment{} | ||
| if err := reader.Get(ctx, req.NamespacedName, cod); err != nil || !cod.DeletionTimestamp.IsZero() { | ||
| return 0, false | ||
| } | ||
| var revisions ocv1.ClusterObjectSetList | ||
| if err := reader.List(ctx, &revisions, client.MatchingFields{clusterObjectSetGroupIndex: cod.Name}); err != nil { | ||
| return 0, false | ||
| } | ||
| latest := latestActiveRevision(revisions.Items) | ||
| if latest == nil || latest.Labels[labels.TemplateHashKey] != templateHash(cod.Spec.Template) { | ||
| // Creating the desired revision is outside the MVP deadline. | ||
| return 0, false | ||
| } | ||
| return durationUntilDeploymentDeadline(clk, latest) | ||
| }, |
There was a problem hiding this comment.
maybe we should also consider the deletion case?
| remaining: func(req ctrl.Request) (time.Duration, bool) { | |
| ctx := context.Background() | |
| cod := &ocv1.ClusterObjectDeployment{} | |
| if err := reader.Get(ctx, req.NamespacedName, cod); err != nil || !cod.DeletionTimestamp.IsZero() { | |
| return 0, false | |
| } | |
| var revisions ocv1.ClusterObjectSetList | |
| if err := reader.List(ctx, &revisions, client.MatchingFields{clusterObjectSetGroupIndex: cod.Name}); err != nil { | |
| return 0, false | |
| } | |
| latest := latestActiveRevision(revisions.Items) | |
| if latest == nil || latest.Labels[labels.TemplateHashKey] != templateHash(cod.Spec.Template) { | |
| // Creating the desired revision is outside the MVP deadline. | |
| return 0, false | |
| } | |
| return durationUntilDeploymentDeadline(clk, latest) | |
| }, | |
| remaining: func(req ctrl.Request) (time.Duration, bool, error) { | |
| ctx := context.Background() | |
| cod := &ocv1.ClusterObjectDeployment{} | |
| if err := reader.Get(ctx, req.NamespacedName, cod); err != nil { | |
| return 0, false, err // unknown | |
| } | |
| if !cod.DeletionTimestamp.IsZero() { | |
| return 0, false, nil // deleting: definitively no deadline | |
| } | |
| var revisions ocv1.ClusterObjectSetList | |
| if err := reader.List(ctx, &revisions, client.MatchingFields{clusterObjectSetGroupIndex: cod.Name}); err != nil { | |
| return 0, false, err // unknown | |
| } | |
| latest := latestActiveRevision(revisions.Items) | |
| if latest == nil || latest.Labels[labels.TemplateHashKey] != templateHash(cod.Spec.Template) { | |
| return 0, false, nil // creating desired revision: outside MVP deadline | |
| } | |
| d, has := durationUntilDeploymentDeadline(clk, latest) | |
| return d, has, nil | |
| }, |
There was a problem hiding this comment.
Done. Missing or deleting deployments and missing or deleting desired revisions definitively have no active deadline and clear the retry marker. Transient lookup errors preserve it. Added regression coverage for both cases.
Calculate the deployment deadline from the desired COS creation timestamp through its first successful completion. Enforce the deadline even when COS readiness remains Pending, PreconditionsNotMet, or another non-deadline reason. Schedule the COD at the deadline and cap retry backoff so a quiet or failing revision cannot leave the deployment progressing indefinitely. Reuse the revision completion latch to allow late recovery without timing completed revisions again. Keep time waiting for COS creation outside the MVP budget. Cover fixed budgets across phases, blocked revisions, retry scheduling, creation retries, late completion, and availability from an older revision. Signed-off-by: Fabricio Aguiar <fabricio.aguiar@gmail.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
Regenerate apply configuration comments, API reference documentation, the COD CRD, and experimental manifests from the clarified deadline contract. Signed-off-by: Fabricio Aguiar <fabricio.aguiar@gmail.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
c01bfa4 to
d97b31b
Compare
Description
The progression timeout covers how long a ClusterObjectDeployment (COD) takes
to progress to its desired new revision. The timer starts when that revision's
ClusterObjectSet (COS) is created and ends when the COS records its first
status.completedAt, meaning the entire revision has completed rollout. The CODthen reports
Progressing=Falsewith reasonNewClusterObjectSetProgressed.All phases share one budget. A five-phase revision with a ten-minute deadline
gets ten minutes total; completing a phase does not reset the timer. Time spent
waiting to create the COS is excluded, and each new revision gets a fresh budget.
Enforce the deadline directly in the COD controller so it reports
Progressing=Falsewith reasonProgressDeadlineExceededwhen the budget expiresbefore completion, even if the COS reports
PreconditionsNotMet,InvalidSpecification, or no readiness condition. Schedule a deadline check evenwhen the COS emits no further events, and cap error backoff at the remaining
budget.
Reconciliation continues after timeout, allowing late revision completion to
recover. Completed revisions are not timed again after health regressions.
Availability from an older ready revision is preserved.
Jira: OPRUN-4791
Validation:
direct COD enforcement.
retries, late completion, health regression, and retained availability.
make test-unit: all 51 test packages passed with race detection.make lint: zero issues.make verify: passed with no generated-file changes.make lint-api-diff BASELINE_BRANCH=upstream/main: no new findings.Reviewer Checklist
Summary by CodeRabbit