⚠️ ClusterExtentionRevision conditions #2340
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the ClusterExtensionRevision conditions system to better distinguish between availability and progression states. The changes introduce a new "Progressing" condition type and update condition reasons to be more descriptive of the revision lifecycle states.
Key changes:
- Introduces
Progressingcondition type alongside refactoredAvailableandSucceededconditions - Replaces generic condition reasons with specific lifecycle states (e.g.,
RollingOut,RolledOut,Retrying,ProbesSucceeded) - Adds comprehensive E2E tests for ClusterExtensionRevision condition behavior
- Removes enum validation for
CollisionProtectionfield in CRDs
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| api/v1/clusterextensionrevision_types.go | Refactors condition type constants and adds Progressing type, updates condition reason constants to reflect specific lifecycle states |
| internal/operator-controller/controllers/clusterextensionrevision_controller.go | Implements condition setting logic with new helper functions and updates reconciliation flow to properly set Progressing/Available conditions |
| internal/operator-controller/controllers/clusterextensionrevision_controller_test.go | Updates unit tests to validate new condition reasons and adds test coverage for error scenarios |
| internal/operator-controller/applier/boxcutter.go | Adds handling for Retrying reason in progressing condition |
| test/e2e/cluster_extension_revision_test.go | Adds comprehensive E2E test covering revision conditions, availability probes, and archiving |
| test/e2e/e2e_suite_test.go | Adds Kubernetes clientset for pod exec operations in E2E tests |
| manifests/experimental.yaml, manifests/experimental-e2e.yaml, helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml | Adds Progressing column to kubectl output and removes CollisionProtection enum validation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
perdasilva
changed the title
perdasilva
force-pushed
the
cer-conditions
branch
from
793f821 to
ae7d78d
Compare
openshift-merge-robot
added
the
needs-rebase
perdasilva
force-pushed
the
cer-conditions
branch
from
ae7d78d to
8a492cb
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/controllers/clusterextensionrevision_controller.go
Show resolved
Hide resolved
perdasilva
force-pushed
the
cer-conditions
branch
from
8a492cb to
033aedf
Compare
openshift-merge-robot
removed
the
needs-rebase
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2340 +/- ##
==========================================
+ Coverage 70.58% 71.04% +0.45%
==========================================
Files 93 93
Lines 7333 7294 -39
==========================================
+ Hits 5176 5182 +6
+ Misses 1721 1680 -41
+ Partials 436 432 -4
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:
|
perdasilva
changed the title
perdasilva
force-pushed
the
cer-conditions
branch
from
033aedf to
537ad96
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
internal/operator-controller/controllers/clusterextensionrevision_controller_test.go:915
- Corrected spelling of 'InTransistion' to 'InTransition'.
func (m mockRevisionResult) InTransistion() bool {
internal/operator-controller/controllers/clusterextensionrevision_controller_test.go:953
- Corrected spelling of 'InTransistion' to 'InTransition'.
func (m mockPhaseResult) InTransistion() bool {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
perdasilva
force-pushed
the
cer-conditions
branch
from
537ad96 to
d9d9924
Compare
perdasilva
force-pushed
the
cer-conditions
branch
from
98b7083 to
af87fe6
Compare
perdasilva
force-pushed
the
cer-conditions
branch
from
af87fe6 to
2e4f6dd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
openshift-merge-robot
added
the
needs-rebase
perdasilva
force-pushed
the
cer-conditions
branch
from
2e4f6dd to
7fc2962
Compare
openshift-merge-robot
removed
the
needs-rebase
perdasilva
force-pushed
the
cer-conditions
branch
from
7fc2962 to
98112f6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pedjak
left a comment
pedjak
left a comment
There was a problem hiding this comment.
please update/extend PR title to summarize what this PR is about.
internal/operator-controller/controllers/clusterextensionrevision_controller.go
Outdated
Show resolved
Hide resolved
| if err != nil { | ||
| if rres != nil { | ||
| l.Error(err, "revision reconcile failed") | ||
| l.V(1).Info("reconcile failure report", "report", rres.String()) |
There was a problem hiding this comment.
In case of error I think it would still very useful to log et error level, assuming that errors are happening occasionally.
internal/operator-controller/controllers/clusterextensionrevision_controller.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/controllers/clusterextensionrevision_controller_test.go
Outdated
Show resolved
Hide resolved
perdasilva
force-pushed
the
cer-conditions
branch
from
98112f6 to
2c73a3c
Compare
pedjak
left a comment
pedjak
left a comment
There was a problem hiding this comment.
Given that I am PR co-author, I would prefer that some else gives lgtm
| // | ||
| // The Succeeded condition represents whether the revision has successfully completed its rollout: | ||
| // - When status is True and reason is RolloutSuccess, the revision has successfully completed its rollout. This condition is set once and persists even if the revision later becomes unavailable. | ||
| // - When status is True and reason is RolloutSuccess, the ClusterExtensionRevision has successfully completed its rollout. This condition is set once and persists even if the revision later becomes unavailable. |
There was a problem hiding this comment.
I'm not seeing RolloutSuccess as a valid reason (it was removed)
| // - When Progressing is not present, the revision is not currently in transition. | ||
| // - When status is True and reason is RollingOut, the ClusterExtensionRevision rollout is actively making progress and is in transition. | ||
| // - When status is True and reason is Retrying, the ClusterExtensionRevision has encountered an error that could be resolved on subsequent reconciliation attempts. | ||
| // - When status is True and reason is Succeeded, the ClusterExtensionRevision has reached the desired state. |
There was a problem hiding this comment.
I don't see Succeeded as a reason (There's ProbesSucceeded, but it's not the same)
There was a problem hiding this comment.
It's an existing reason in common_types: https://github.com/operator-framework/operator-controller/pull/2340/files#diff-7ad21c885e2b008e93dfa228f394825987e36cb8e539fe80be5661d692dbc47aL35
| // The Progressing condition represents whether the revision is actively rolling out: | ||
| // - When status is True and reason is Progressing, the revision rollout is actively making progress and is in transition. | ||
| // - When Progressing is not present, the revision is not currently in transition. | ||
| // - When status is True and reason is RollingOut, the ClusterExtensionRevision rollout is actively making progress and is in transition. |
There was a problem hiding this comment.
I don't see RollingOut as a reason.
There was a problem hiding this comment.
| // | ||
| // Reconcile | ||
| // | ||
| if err := c.ensureFinalizer(ctx, rev, clusterExtensionRevisionTeardownFinalizer); err != nil { |
There was a problem hiding this comment.
Status update was removed here? Is that on purpose?
| func markAsArchived(cer *ocv1.ClusterExtensionRevision) bool { | ||
| const msg = "revision is archived" | ||
| updated := markAsNotProgressing(cer, ocv1.ClusterExtensionRevisionReasonArchived, msg) | ||
| return markAsAvailableUnknown(cer, ocv1.ClusterExtensionRevisionReasonArchived, msg) || updated |
There was a problem hiding this comment.
This feels like it ought to be "Unavailable" rather than "Unknown"? Is there a reason for the distinction here?
There was a problem hiding this comment.
I'm wondering if we'll remove it all together in a follow-up. The reason for it being unknown is that there are situation in which it could be considered to be available. If rev-2 is rev-1 + some random resource, its possible that rev-1 could be available.
internal/operator-controller/controllers/clusterextensionrevision_controller.go
Show resolved
Hide resolved
| ) | ||
|
|
||
| func setRetryingConditions(cer *ocv1.ClusterExtensionRevision, message string) { | ||
| markAsProgressing(cer, ocv1.ClusterExtensionRevisionReasonRetrying, message) |
There was a problem hiding this comment.
setting Progressing=True here can conflict with Available=True, should we consider setting Available=Unknown?
There was a problem hiding this comment.
If the Available status conditions exists, we set it to unknown below. If it hasn't been set yet, we leave it unset.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/controllers/clusterextensionrevision_controller.go
Show resolved
Hide resolved
internal/operator-controller/controllers/clusterextensionrevision_controller.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/controllers/clusterextensionrevision_controller_test.go
Show resolved
Hide resolved
...operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml
Outdated
Show resolved
Hide resolved
perdasilva
force-pushed
the
cer-conditions
branch
from
8f618bc to
4ca502a
Compare
Co-authored-by: Per Goncalves da Silva <pegoncal@redhat.com> Co-authored-by: Predrag Knezevic <pknezevi@redhat.com Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
perdasilva
force-pushed
the
cer-conditions
branch
from
4ca502a to
04de603
Compare
7 participants
Description
Summary
This PR refactors the ClusterExtensionRevision status conditions to provide clearer, more actionable feedback about revision lifecycle states. The changes introduce a new Progressing condition, simplify condition reasons, and ensure that revision-level errors (retrying states) are properly surfaced to the parent ClusterExtension.
This PR does introduce API breaking changes. But, only to experimental APIs. It should be ok to override.
Key Changes
Simplified condition types:
Updated condition reasons to be more semantic and actionable:
API Changes:
Modified the Boxcutter applier to propagate retrying errors from ClusterExtensionRevision to the parent ClusterExtension. When a revision is in a Progressing=True state with Reason=Retrying, the error is now surfaced to the ClusterExtension, providing better visibility into failed reconciliation attempts. When the revision is Progressing=True/Succeeded the applier returns success.
Added end-to-end tests for ClusterExtensionRevision lifecycle scenarios to validate the new condition states and transitions.
ProgressionreasonClusterExtension Status Conditions
Progressing Condition (experimental only)
ClusterExtensionRevision Status Conditions
Progressing Condition
Indicates whether the revision is progressing to its next state. It follows the same semantic as the ClusterExtension and Deployment
Progressingcondition.Not a part of this PR yet, but
Progressingcan also beBlockedfor non-retryable errors.Available Condition
Indicates whether the revision's objects are available and passing the object status probes.
Succeeded Condition
Indicates whether the revision has successfully completed its rollout.
Migration Notes
Reviewer Checklist