feat: aggregate group readiness in cluster status - #7023
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe cluster API now defines ChangesCluster condition aggregation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ClusterStatusUpdater
participant GroupStatuses
participant ConditionHelpers
participant ClusterStatus
ClusterStatusUpdater->>GroupStatuses: Read observed Group conditions
ClusterStatusUpdater->>ConditionHelpers: Aggregate Ready and Synced states
ConditionHelpers-->>ClusterStatusUpdater: Return status, reason, and message
ClusterStatusUpdater->>ClusterStatus: Set cluster conditions
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7023 +/- ##
==========================================
+ Coverage 40.84% 40.94% +0.09%
==========================================
Files 439 439
Lines 25472 25515 +43
==========================================
+ Hits 10403 10446 +43
Misses 15069 15069
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds Cluster-level Ready and Synced conditions so higher-level controllers can determine pause/resume completion from the Cluster status by aggregating readiness/sync state across all observed Group types.
Changes:
- Aggregate Group
ReadyandSyncedinto new Cluster conditions (ClusterCondReady,ClusterCondSynced) based on current Group + Cluster generations. - Refactor the per-Group-type condition checks into a generic helper (
groupsConditionsTrue+allGroupConditionsTrue). - Extend status task tests to cover the new Cluster conditions and the “no observed Groups” case.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/controllers/cluster/tasks/status.go | Adds Cluster Ready/Synced aggregation logic and refactors Group condition evaluation helpers. |
| pkg/controllers/cluster/tasks/status_test.go | Updates baseline Cluster condition expectations and adds readiness aggregation test coverage. |
| api/core/v1alpha1/cluster_types.go | Introduces new Cluster condition type constants: Ready and Synced. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/retest |
Investigated the failed |
| ClusterCondSuspended = "Suspended" | ||
| ClusterSuspendReason = "ClusterSuspend" | ||
|
|
||
| ClusterCondReady = "Ready" |
|
|
||
| ClusterCondReady = "Ready" | ||
| ClusterReadyReason = "ClusterReady" | ||
| ClusterCondSynced = "Synced" |
| }, | ||
| } { | ||
| status := metav1.ConditionFalse | ||
| message := fmt.Sprintf("Not all Groups have %s=True for the current Cluster generation", condition.conditionType) |
There was a problem hiding this comment.
add cond=false groups into msg?
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liubog2008 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 |
[LGTM Timeline notifier]Timeline:
|
What
ReadyandSyncedconditionsTruefor the current Group and Cluster generationsSuspendedaggregation on the shared condition helperReady=FalseandSynced=Falsewhen no Groups are observedWhy
Suspended=Falseonly indicates that the desired state has switched to resume. It does not guarantee that all compute Groups have finished reconciling and become usable.With these Cluster-level conditions, an upper-level controller can determine resume completion by reading only the Cluster:
suspendCompute == falseSuspended=FalseReady=TrueSynced=TrueReadyandSyncedare aggregated independently, so they also show whether resume is waiting for readiness or synchronization.Notes
The Cluster API does not own the desired Group topology. The aggregation therefore covers Groups currently observed by the controller cache. Callers must ensure that the Group topology is complete and stable while using these conditions as pause/resume completion signals.
For now,
observedClusterGenerationis validated by the Cluster aggregation. Binding it directly to Group/InstanceReadyandSyncedupdates is left as a follow-up.Tests
go test ./pkg/controllers/cluster/...Summary by CodeRabbit
New Features
Bug Fixes