Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[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. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (34)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughClusterObjectSet now requires an immutable, validated ChangesClusterObjectSet group scoping
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant ClusterExtension
participant Boxcutter
participant KubernetesAPI
participant RevisionEngine
participant PhaseObjects
ClusterExtension->>Boxcutter: Reconcile extension
Boxcutter->>KubernetesAPI: Create ClusterObjectSet with group
Boxcutter->>KubernetesAPI: List revisions by spec.group
KubernetesAPI-->>Boxcutter: Return matching revisions
Boxcutter->>RevisionEngine: Apply revision
RevisionEngine->>PhaseObjects: Apply with cos-group/group manager
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No concrete issue remains that should block merging after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 18.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 50 functions across 26 files. (8 skipped: 8 unsupported.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
the
needs-rebase
Require an immutable group identifier for revision tracking and display it in the Group printer column. Signed-off-by: Francesco Giudici <fgiudici@redhat.com>
Regenerate apply configurations and experimental CRDs and manifests from the Group API specification. Signed-off-by: Francesco Giudici <fgiudici@redhat.com>
Populate and index spec.group, use group-scoped revision queries and watches, and share SSA field ownership across revisions while retaining the OLM metadata prefix. Cover admission, cache isolation, event mapping, migration ownership, and SSA behavior. Signed-off-by: Francesco Giudici <fgiudici@redhat.com>
Check generated groups, shared SSA ownership during rollout, pruning isolation, and cascade cleanup. Update direct COS fixtures and query revision associations by spec.group. Signed-off-by: Francesco Giudici <fgiudici@redhat.com>
c052700 to
b4e2c87
Compare
openshift-ci
Bot
removed
the
needs-rebase
| // group identifies the ClusterExtension whose revisions belong together. | ||
| // All revisions for the same ClusterExtension must use its name as their group. |
There was a problem hiding this comment.
Do not mention ClusterExtension anywhere in the docs for the ClusterObjectDeployment or ClusterObjectSet APIs. These APIs are standalone and separate from ClusterExtension.
There was a problem hiding this comment.
Here's what my AI-ed GoDoc is for this field in orb-operator: https://github.com/joelanford/orb-operator/blob/76c7c2316efdacf9a3eeb8a68719df9781dacf1e/api/v1alpha1/types_clusterobjectset.go#L70-L72
| // +kubebuilder:validation:MaxLength=52 | ||
| // +kubebuilder:validation:XValidation:rule=`self.matches('^[a-z]([a-z0-9-]*[a-z0-9])?$')`,message="group must start with a lowercase letter, contain only lowercase letters, digits or hyphens, and end with a letter or digit" | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="group is immutable" | ||
| Group string `json:"group,omitempty"` |
There was a problem hiding this comment.
Since this field is required, no need for omitempty:
| Group string `json:"group,omitempty"` | |
| Group string `json:"group"` |
| // +optional | ||
| Spec ClusterObjectSetSpec `json:"spec,omitempty"` | ||
| // +required | ||
| Spec ClusterObjectSetSpec `json:"spec,omitzero"` |
There was a problem hiding this comment.
No need for omitzero since it is required.
| Spec ClusterObjectSetSpec `json:"spec,omitzero"` | |
| Spec ClusterObjectSetSpec `json:"spec"` |
| LifecycleState: ClusterObjectSetLifecycleStateActive, | ||
| Revision: 1, | ||
| CollisionProtection: CollisionProtectionPrevent, |
There was a problem hiding this comment.
Technically this is an invalid spec since group is required, right? We should test the real world scenarios of:
groupA->groupBfailsgroupA->groupAsucceedsgroupA-> `` fails (immutable AND required)
| spec: ClusterObjectSetSpec{ | ||
| LifecycleState: ClusterObjectSetLifecycleStateActive, | ||
| Revision: 1, | ||
| CollisionProtection: CollisionProtectionPrevent, |
There was a problem hiding this comment.
Same here. We should set the group in the "from" spec.
| {name: "missing spec", omitSpec: true}, | ||
| {name: "missing group"}, | ||
| {name: "empty group", group: ptr.To("")}, |
There was a problem hiding this comment.
Include explicit valid: false to make the test cases more readable.
| omitSpec bool | ||
| valid bool | ||
| }{ | ||
| {name: "missing spec", omitSpec: true}, |
There was a problem hiding this comment.
This is a test of the spec itself, not of the group. Move this to a separate set of tests, e.g. in TestClusterObjectSetSpecValidation
| }, | ||
| { | ||
| name: "should return empty list when owner label missing", | ||
| name: "should include revisions when owner label is missing", |
There was a problem hiding this comment.
Owner label is just not a thing anymore for the ClusterObjectSetReconciler, right? So no need for any tests that mention anything about an owner label?
| }, | ||
| { | ||
| name: "should only include revisions matching owner label", | ||
| name: "should only include revisions in the same group", |
There was a problem hiding this comment.
What is the actual grouping key with this change? Is it spec.group + controller: true ownerRef? Or is it just spec.group?
I think we should use the former. That would mean that if two different COSes have the same group, but different controller owners, they would be in different groups. And if two different COSes have the same group and do not have a controller owner at all, they would be in the same group.
There was a problem hiding this comment.
We should include tests for all of those scenarios.
There was a problem hiding this comment.
Here's what I landed on as a helper package/type/functions for building up the sibling list from the raw set of ClusterObjectSet objects sharing a spec.group: https://github.com/joelanford/orb-operator/blob/main/internal/revision/chain.go
| } | ||
| // Keep the field owner prefix unchanged so existing objects can be reconciled after migration. | ||
| if err := mgr.GetFieldIndexer().IndexField(context.Background(), &ocv1.ClusterObjectSet{}, | ||
| clusterobjectset.GroupField, clusterobjectset.ExtractGroup); err != nil { |
There was a problem hiding this comment.
Seems like the GroupField and ExtractGroup functionality are internal details of the setup for the COS field indexer.
To keep things a bit cleaner, WDYT about a SetupIndexes function defined in the COS controller code that inlines the GroupField and ExtractGroup functions?
That would eliminate the entire shared/clusterobjectset package and result in the entire index setup being handled in one place.
| if err := mgr.GetFieldIndexer().IndexField(context.Background(), &ocv1.ClusterObjectSet{}, | ||
| clusterobjectset.GroupField, clusterobjectset.ExtractGroup); err != nil { | ||
| return fmt.Errorf("indexing ClusterObjectSet group: %w", err) | ||
| } |
There was a problem hiding this comment.
Is this just temporary until we have ClusterObjectDeployment?
If so, I'm slightly worried that we're accruing code we will end up not needing/wanting with no easy way to come back around and identify things we don't need after ClusterObjectDeployment is in place.
Any ideas on how to make sure we keep a lid on this kind of thing?
|
|
||
| var ctrlBuilderOpts []controllers.ControllerBuilderOption | ||
| if features.OperatorControllerFeatureGate.Enabled(features.BoxcutterRuntime) { | ||
| ctrlBuilderOpts = append(ctrlBuilderOpts, controllers.WithOwns(&ocv1.ClusterObjectSet{})) |
There was a problem hiding this comment.
It feels like we should be able to eventually fully abstract this into a SetupWithManager functions in the COD/COS packages, rather than putting details like this in main.go.
I know we're dealing with both Helm and Boxcutter runtimes right now, but I do wonder if we could do something like:
if boxcutter {
ceController.SetupWithManagerForBoxcutter(mgr)
} else {
ceController.SetupWithManagerForHelm(mgr)
}
Then we wouldn't need more low-level details like WithClusterObjectSetWatch leaking out into main.go.
WDYT?
This branch has not been deployed
2 participants
Description
Implement OPRUN-4739 by making
ClusterObjectSet.spec.groupthe authoritative association between a ClusterExtension and its revisions. Revision lookup and event routing currently depend on owner labels or references; the new immutable group keeps that association explicit and gives sibling revisions a shared SSA field manager..spec.groupcache index, and use group-scoped queries for migration, revision state, sibling reconciliation, and pruning.cos-group/<group>for SSA ownership while preserving the OLM metadata prefix.This changes the experimental ClusterObjectSet API: new objects must supply
spec.group. Migration/backfill for already stored experimental objects and future parent API integration are outside this PR's scope.Validation
Completed during implementation:
make generate manifests crd-ref-docsmake lint-api-diff,make lint,make test-unit(race detection and coverage), andmake verifyAn optional
standardbuild-tag compile check fails identically on unchanged main; that pre-existing failure is outside this change. All four commits have DCO sign-offs.Reviewer Checklist
Summary by CodeRabbit
New Features
Changes