Repository navigation
⚠ remove ClusterObjectSet Ready RetryableError reason #3012
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -222,11 +222,12 @@ Indicates whether all objects in the revision have been successfully rolled out | |
| | False | `ObjectCollision` | One or more objects are controlled by another owner, or are unowned and collision protection prevents adoption | | ||
| | False | `PreconditionsNotMet` | A precondition for applying the revision is not yet satisfied (a referenced Secret is mutable or has invalid or changed content, or a preflight dry-run was rejected, e.g. an immutable field, a conflicting object, or RBAC/dependency CRDs not yet propagated); the revision keeps retrying and recovers once the precondition is met | | ||
| | False | `InvalidSpecification` | The revision spec is invalid (a malformed revision/phase object or progression probe); requires a spec change | | ||
| | False | `Archiving` | The revision is being archived; owned objects are still being torn down | | ||
| | False | `Archived` | Objects torn down after archival | | ||
| | Unknown | `Migrated` | Migrated from existing release; probes not yet observed | | ||
|
|
||
| A transient internal error (e.g. an API read failure) leaves the `Ready` condition unchanged and is retried with backoff; it is not reported as a distinct reason, so on a first reconcile the condition may remain absent (`Unknown`). | ||
|
Member
There was a problem hiding this comment. If
Contributor
Author
There was a problem hiding this comment. Addressed in follow-up #3023: the controller now initializes |
||
|
|
||
| ### Succeeded | ||
|
|
||
| A terminal condition set once the rollout completes. It persists even if the revision later becomes unavailable, marking that this version was successfully deployed at least once. | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -150,10 +150,9 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl | |
| markAsNotReady(cos, ocv1.ClusterObjectSetReasonPreconditionsNotMet, err.Error()) | ||
| return ctrl.Result{RequeueAfter: validationRetryInterval}, nil | ||
| } | ||
| // A Secret read error is transient; leave the Ready condition unchanged and | ||
| // return the error for rate-limited backoff. | ||
| return ctrl.Result{}, fmt.Errorf("verifying referenced Secrets: %v", err) | ||
| } | ||
|
|
||
| phases, currentPhases, opts, err := c.buildBoxcutterPhases(ctx, cos) | ||
|
|
@@ -175,7 +174,6 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl | |
| markAsNotReady(cos, ocv1.ClusterObjectSetReasonInvalidSpecification, fmt.Sprintf("The object set spec is invalid: %v.", err)) | ||
| return ctrl.Result{}, nil | ||
| } | ||
| return ctrl.Result{}, fmt.Errorf("converting to boxcutter revision: %v", err) | ||
|
Comment on lines
-178
to
179
Member
There was a problem hiding this comment. How do we know that all other errors from
Contributor
Author
There was a problem hiding this comment. Addressed in follow-up #3023. Audit of buildBoxcutterPhases: every deterministic failure is already a typed |
||
| } | ||
|
|
||
|
|
@@ -192,7 +190,6 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl | |
|
|
||
| revisionEngine, err := c.RevisionEngineFactory.CreateRevisionEngine(ctx, cos) | ||
| if err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("failed to create revision engine: %v", err) | ||
| } | ||
|
|
||
|
|
@@ -206,7 +203,6 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl | |
|
|
||
| if cos.Spec.LifecycleState == ocv1.ClusterObjectSetLifecycleStateArchived { | ||
| if err := c.TrackingCache.Free(ctx, cos); err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("error stopping informers: %v", err) | ||
| } | ||
| return c.archive(ctx, revisionEngine, cos, revision) | ||
|
|
@@ -217,9 +213,7 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl | |
| } | ||
|
|
||
| if err := c.establishWatch(ctx, cos, revision); err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("establish watch: %v", err) | ||
| } | ||
|
|
||
| rres, err := revisionEngine.Reconcile(ctx, revision, opts...) | ||
|
|
@@ -228,7 +222,6 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl | |
| // Log detailed reconcile reports only in debug mode (V(1)) to reduce verbosity. | ||
| l.V(1).Info("reconcile report", "report", rres.String()) | ||
| } | ||
| return ctrl.Result{}, fmt.Errorf("revision reconcile: %v", err) | ||
| } | ||
|
|
||
|
|
@@ -393,7 +386,6 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl | |
|
|
||
| func (c *ClusterObjectSetReconciler) delete(ctx context.Context, cos *ocv1.ClusterObjectSet) (ctrl.Result, error) { | ||
| if err := c.TrackingCache.Free(ctx, cos); err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("error stopping informers: %v", err) | ||
| } | ||
| if err := c.removeFinalizer(ctx, cos, clusterObjectSetTeardownFinalizer); err != nil { | ||
|
|
@@ -405,9 +397,9 @@ func (c *ClusterObjectSetReconciler) delete(ctx context.Context, cos *ocv1.Clust | |
| func (c *ClusterObjectSetReconciler) archive(ctx context.Context, revisionEngine RevisionEngine, cos *ocv1.ClusterObjectSet, revision boxcutter.RevisionBuilder) (ctrl.Result, error) { | ||
| tdres, err := revisionEngine.Teardown(ctx, revision, machinerytypes.WithObserveAfterIncomplete{}) | ||
| if err != nil { | ||
| // Transient teardown error: leave the Ready condition unchanged and return | ||
| // the error for rate-limited backoff. | ||
| return ctrl.Result{}, fmt.Errorf("error archiving revision: %v", err) | ||
| } | ||
| // Set object counts | ||
| status.FromTeardown(cos, tdres) | ||
|
|
@@ -775,10 +767,6 @@ func setReadyWithDeadline(cos *ocv1.ClusterObjectSet, status metav1.ConditionSta | |
| }) | ||
| } | ||
|
|
||
| func markAsReady(cos *ocv1.ClusterObjectSet, reason, message string) bool { | ||
| return meta.SetStatusCondition(&cos.Status.Conditions, metav1.Condition{ | ||
| Type: ocv1.ClusterObjectSetTypeReady, | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT about setting
ReadytoUnknownwith reasonRetryableError? That is the most truthful thing we can say.Or maybe we can figure out a way for a RetryableError to only cause a condition to flip after a certain number of consecutive failed retries?
I'm worried about a situation where our code has decided that an error is transient, but then it turns out that it is not transient and our status gives no indication of that.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of following this PR with generating an event for these transient errors - that way we at least get some kind of external signal. I really think we shouldn't be surfacing controller state through the object - otherwise we're being kinda edge based. But, I agree this leaves that case open =/ ultimately, I think we need to cover this gap through monitoring and metrics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on the eventing idea. Need to be careful there to avoid sending too many duplicate events. There's an OLMv0 bug about that right now, I think.
But on the COS status, if we say
Ready/Unknown/Indeterminate/could not evaluate readiness due to transient error, that's not controller state. That's accurate COS state, saying 🤷 ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the question is whether we want to produce a new level on a transient error. It seems like the right thing for a controller to do is eat these kinds of spurious transition changes, and surface sticky reconciliation failures through controller metrics, no?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this is the case for the
Progressingcondition? OrDegraded?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's what I've found with Claude asking it to check Deployment, ResultSet, StatefulSet, CRD, etc.
The rule holds everywhere I checked: no controller flaps a condition to Unknown on a transient error. Transient = return + requeue, leave status.
Failures that get surfaced use a dedicated condition (ReplicaFailure=True, NamesAccepted=False/reason), cleared/recomputed on recovery. The only
persisted Unknown is the Node controller's, and it's gated on a sustained grace period from the last successful observation — never a single error.
So for the COS:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's convincing! I definitely agree that it would not be ideal to flap Ready on a momentary hiccup.
It seems like no other core controllers immediately flap on the first error. So I'm convinced on this change, and we can always add a separate status condition or wait to flip to Unknown until we decide the transient error is sustained.
Thanks for the perseverance!