Update reconciliation with status conditions and event-driven updates#90
Open
shreyabiradar07 wants to merge 2 commits into
Open
Update reconciliation with status conditions and event-driven updates#90shreyabiradar07 wants to merge 2 commits into
shreyabiradar07 wants to merge 2 commits into
Conversation
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Reviewer's GuideRefactors the Kruize controller reconciliation loop to validate cluster type early, centralize finalizer handling, add non-blocking deployment readiness checks, standardize status conditions/phases, and register owned resources for event-driven reconciliation. Sequence diagram for updated Kruize reconciliation loopsequenceDiagram
participant Manager
participant KruizeReconciler
participant API_Server
Manager->>KruizeReconciler: Reconcile(req)
KruizeReconciler->>API_Server: Get(Kruize)
KruizeReconciler->>KruizeReconciler: IsBeingDeleted(kruize)
alt not being deleted
KruizeReconciler->>KruizeReconciler: IsValidClusterType(kruize.Spec.Cluster_type)
alt invalid cluster type
KruizeReconciler-->>Manager: error (unsupported cluster type)
else valid cluster type
KruizeReconciler->>KruizeReconciler: HandleFinalizer(kruize, finalizeKruize)
alt finalizer error
KruizeReconciler-->>Manager: error
else needsRequeue
KruizeReconciler-->>Manager: ctrl.Result{Requeue: true}
else ready to reconcile
KruizeReconciler->>KruizeReconciler: deployKruize(ctx, kruize)
alt deploy error
KruizeReconciler->>KruizeReconciler: updateStatus(Progressing, Failed)
KruizeReconciler-->>Manager: ctrl.Result{RequeueAfter: 30s}
else deploy ok
KruizeReconciler->>KruizeReconciler: checkAllDeploymentsReady(namespace)
alt check error
KruizeReconciler->>KruizeReconciler: updateStatus(Progressing, Unknown)
KruizeReconciler-->>Manager: ctrl.Result{RequeueAfter: 10s}
else not all ready
KruizeReconciler->>KruizeReconciler: updateStatus(Progressing, Progressing)
KruizeReconciler-->>Manager: ctrl.Result{RequeueAfter: 15s}
else all ready
KruizeReconciler->>KruizeReconciler: updateStatus(Ready, Ready)
KruizeReconciler-->>Manager: ctrl.Result{}
end
end
end
end
else being deleted
KruizeReconciler->>KruizeReconciler: HandleFinalizer(kruize, finalizeKruize)
KruizeReconciler-->>Manager: ctrl.Result{}
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
updateStatus, theconditionTypeparameter is never used and the condition type is hard-coded to"Available"; either wireconditionTypethrough tometa.SetStatusConditionor remove the parameter to avoid confusion. - The
updateStatushelper callsr.Status().Updateonce without handlingConflicterrors, which could lead to dropped status updates under concurrent reconciles; consider wrapping the status update in a retry loop (e.g., usingclient.RetryOnConflict) to make this more robust. - In the
if !allReadybranch, the closing brace alignment/indentation looks off in the diff (the block ends with a tab-indented}); double-check the block structure/formatting there to avoid accidental logic or formatting issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `updateStatus`, the `conditionType` parameter is never used and the condition type is hard-coded to `"Available"`; either wire `conditionType` through to `meta.SetStatusCondition` or remove the parameter to avoid confusion.
- The `updateStatus` helper calls `r.Status().Update` once without handling `Conflict` errors, which could lead to dropped status updates under concurrent reconciles; consider wrapping the status update in a retry loop (e.g., using `client.RetryOnConflict`) to make this more robust.
- In the `if !allReady` branch, the closing brace alignment/indentation looks off in the diff (the block ends with a tab-indented `}`); double-check the block structure/formatting there to avoid accidental logic or formatting issues.
## Individual Comments
### Comment 1
<location path="internal/controller/kruize_controller.go" line_range="168" />
<code_context>
if err != nil {
logger.Error(err, "Failed to deploy Kruize")
- return ctrl.Result{RequeueAfter: time.Minute}, err
+ r.updateStatus(ctx, kruize, "Progressing", "Failed", fmt.Sprintf("Deployment failed: %v", err))
+ return ctrl.Result{RequeueAfter: 30 * time.Second}, err
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Status phase on deployment failure should likely be set to a failure phase instead of "Progressing".
In this failure branch, `phase` is set to "Progressing" even though the message and `conditionType` indicate a failure. This makes it difficult for consumers of the CR to distinguish an in-progress state from a terminal error. Please set `phase` to something like "Failed" here, or otherwise ensure it accurately reflects the lifecycle state when deployment fails.
</issue_to_address>
### Comment 2
<location path="internal/controller/kruize_controller.go" line_range="252-253" />
<code_context>
+ return allReady, notReadyDeployments, nil
+ }
+
+ // updateStatus updates the Kruize CR status with conditions
+ func (r *KruizeReconciler) updateStatus(ctx context.Context, kruize *kruizev1alpha1.Kruize,
+ phase, conditionType, message string) {
+ logger := log.FromContext(ctx)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The `conditionType` parameter is unused and the condition type is hard-coded to "Available".
`updateStatus` takes `conditionType` but always sets the condition `Type` to "Available", so callers can’t actually control it. Either remove `conditionType` from the signature or use it to set the `Type` field (e.g., "Available", "Progressing", "Degraded"), and update call sites accordingly to keep the status semantics consistent.
Suggested implementation:
```golang
// updateStatus updates the Kruize CR status with conditions
func (r *KruizeReconciler) updateStatus(ctx context.Context, kruize *kruizev1alpha1.Kruize,
phase, conditionType, message string) {
logger := log.FromContext(ctx)
```
```golang
Type: conditionType,
```
1. Ensure all call sites of `updateStatus` pass an appropriate `conditionType` value (e.g. `"Available"`, `"Progressing"`, `"Degraded"`) instead of assuming it is always `"Available"`.
2. If there are existing callers that relied on the hard‑coded `"Available"` behavior and do not yet pass a meaningful type, update them to pass `"Available"` explicitly to preserve current semantics.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if err != nil { | ||
| logger.Error(err, "Failed to deploy Kruize") | ||
| return ctrl.Result{RequeueAfter: time.Minute}, err | ||
| r.updateStatus(ctx, kruize, "Progressing", "Failed", fmt.Sprintf("Deployment failed: %v", err)) |
There was a problem hiding this comment.
issue (bug_risk): Status phase on deployment failure should likely be set to a failure phase instead of "Progressing".
In this failure branch, phase is set to "Progressing" even though the message and conditionType indicate a failure. This makes it difficult for consumers of the CR to distinguish an in-progress state from a terminal error. Please set phase to something like "Failed" here, or otherwise ensure it accurately reflects the lifecycle state when deployment fails.
Comment on lines
+252
to
+253
| // updateStatus updates the Kruize CR status with conditions | ||
| func (r *KruizeReconciler) updateStatus(ctx context.Context, kruize *kruizev1alpha1.Kruize, |
There was a problem hiding this comment.
suggestion (bug_risk): The conditionType parameter is unused and the condition type is hard-coded to "Available".
updateStatus takes conditionType but always sets the condition Type to "Available", so callers can’t actually control it. Either remove conditionType from the signature or use it to set the Type field (e.g., "Available", "Progressing", "Degraded"), and update call sites accordingly to keep the status semantics consistent.
Suggested implementation:
// updateStatus updates the Kruize CR status with conditions
func (r *KruizeReconciler) updateStatus(ctx context.Context, kruize *kruizev1alpha1.Kruize,
phase, conditionType, message string) {
logger := log.FromContext(ctx) Type: conditionType,- Ensure all call sites of
updateStatuspass an appropriateconditionTypevalue (e.g."Available","Progressing","Degraded") instead of assuming it is always"Available". - If there are existing callers that relied on the hard‑coded
"Available"behavior and do not yet pass a meaningful type, update them to pass"Available"explicitly to preserve current semantics.
…ate_reconciliation
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by Sourcery
Update Kruize controller reconciliation to use condition-based status reporting and event-driven updates for managed resources.
New Features:
Enhancements: