Repository navigation
✨ Give ClusterObjectSet object collisions a dedicated Ready reason - #2989
Conversation
Object collisions previously shared the generic RetryableError reason on the ClusterObjectSet Ready condition. Give them their own reason, Ready=False/ObjectCollision, so the conflict is distinguishable from transient errors. The collision state is stable, so the condition is written once and does not flap. Per boxcutter, a collision is raised when an object is controlled by another (non-sibling) owner, or is unowned while collision protection prevents adoption (a sibling revision owning the object is handled as progression/adoption, not a collision). The reason documentation reflects these conditions. The condition message now identifies the phase by name and lists each colliding object tersely (GroupKind, namespaced name, and the conflicting owner when known) instead of dumping boxcutter's full per-object report. Behavior is otherwise unchanged: collisions are still requeued (10s) and remain deadline-aware (ProgressDeadlineExceeded once the deadline passes). The ClusterExtension reconstruction is preserved — ObjectCollision maps to Progressing=True/Retrying and Installed=Failed, exactly as RetryableError did for collisions, so no ClusterExtension-facing behavior changes. Add the ObjectCollision reason constant, update the status documentation, concept doc, unit test, e2e collision assertions, and regenerate the CRD, apply configurations, reference docs, and manifests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
✅ 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⚙️ Run configuration
📒 Files selected for processing (10)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe change adds ChangesClusterObjectSet collision condition
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~12 minutes Change: Feature Suggested reviewers: Merge Risk: ⚪ Minimal · up to Collisions remain retryable and deadline-limited, and the new reason is reflected in operator status. No material merge risk is identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 5 files. (5 skipped: 5 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
| if ores.Action() != machinery.ActionCollision { | ||
| continue | ||
| } | ||
| obj := ores.Object() | ||
| name := obj.GetName() | ||
| if ns := obj.GetNamespace(); ns != "" { | ||
| name = ns + "/" + name | ||
| } | ||
| gvk := obj.GetObjectKind().GroupVersionKind() | ||
| desc := fmt.Sprintf("%s.%s %q", gvk.Kind, gvk.GroupVersion().String(), name) | ||
| if coll, ok := ores.(machinery.ObjectResultCollision); ok { | ||
| if owner, hasOwner := coll.ConflictingOwner(); hasOwner { | ||
| desc += fmt.Sprintf(" is owned by %s %q", owner.Kind, owner.Name) | ||
| } | ||
| } | ||
| collidingObjs = append(collidingObjs, desc) |
There was a problem hiding this comment.
This is a little wonky because we're just building the list of colliding objects solely for the purposes of logging them. I left it like this as it might be useful for @dtfranz's work on the phase status. It also reduces the vebosity of the collision error output to something more tenable that won't blow up the logs. TL;DR I think its ok if this is here temporarily, I hope it won't generate too much of a conflict for @dtfranz and I encourage @dtfranz to move this out to a helper function in his PR.
fgiudici
left a comment
There was a problem hiding this comment.
We now lose the details of the colliding objects: we need those, but the promise here is that it will be covered in a follow-up adding them to the status.
One nit: the hint for the recovery of conflicting objects may be not always clear. Reporting but not something that should block this PR. LGTM!
/lgtm
| setRetryableErrorConditions(cos, fmt.Sprintf("revision object collisions in phase %d\n%s", i, strings.Join(collidingObjs, "\n\n")), isDeadlineExceeded) | ||
| l.Error(fmt.Errorf("object collision detected"), "object collision, retrying after 10s", "phase", pres.GetName(), "collisions", collidingObjs) | ||
| setReadyWithDeadline(cos, metav1.ConditionFalse, ocv1.ClusterObjectSetReasonObjectCollision, | ||
| fmt.Sprintf("Cannot take ownership of %d object(s) in phase %q because they are owned by another controller or already exist. Remove the conflicting owner or object, or create a new ClusterObjectSet with a more permissive collisionProtection setting.", len(collidingObjs), pres.GetName()), |
There was a problem hiding this comment.
This message may confuse some users if the COS is not manually created.
What about specifying that creating a new ClusterObjectSet is for directly created ClusterObjectSet only?
Something like:
| fmt.Sprintf("Cannot take ownership of %d object(s) in phase %q because they are owned by another controller or already exist. Remove the conflicting owner or object, or create a new ClusterObjectSet with a more permissive collisionProtection setting.", len(collidingObjs), pres.GetName()), | |
| fmt.Sprintf("Cannot take ownership of %d object(s) in phase %q because they are owned by another controller or already exist. Resolve the ownership conflict or remove the conflicting object. For directly managed ClusterObjectSets, a new revision may use an appropriate collisionProtection setting.", len(collidingObjs), pres.GetName()), |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fao89, tmshort 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 |
c59c988
into
operator-framework:main
Description
Object collisions (an object this revision wants is controlled by another owner, or already exists and collision protection won't adopt it) previously shared the generic
RetryableErrorreason on theClusterObjectSetReadycondition. This gives them their own reason,Ready=False/ObjectCollision, so the conflict is distinguishable from transient errors. The collision state is stable, so the condition is written once and does not flap.Changes
Add the
ClusterObjectSetReasonObjectCollisionreason; collisions now emitReady=False/ObjectCollision.Per boxcutter, a collision is raised when an object is controlled by another non-sibling owner, or is unowned while collision protection prevents adoption (a sibling revision owning the object is handled as progression/adoption, not a collision). The reason documentation reflects these conditions.
Tighten and make the condition message actionable (no per-object list — that will move to
statusin a follow-up):The remedy is accurate:
collisionProtectionis immutable on an existing revision, so the way to relax it is a new ClusterObjectSet. Specific enum values are intentionally omitted (resolving a foreign-owned collision requiresNone, which takes ownership from another live controller — a caveat that belongs in field docs, not a status line). Per-object detail remains in the structured log.Behavior preserved
ProgressDeadlineExceededonce the deadline passes).ObjectCollisionmaps toProgressing=True/RetryingandInstalled=Failed, exactly asRetryableErrordid for collisions — so no ClusterExtension-facing reason changes.Scope
Updates the status documentation, concept doc, unit test, and e2e collision assertions; regenerates the CRD, apply configurations, reference docs, and manifests. Experimental API only; no exported identifiers removed (
go-apidiffshould not flag this).🤖 Generated with Claude Code
Summary by CodeRabbit
ObjectCollisionstatus, with the affected object count, phase, and ownership details where available.