K8SPS-498: remove unnecessary status updates #1102
Conversation
| if err := r.Get(ctx, client.ObjectKeyFromObject(cr), cr); err != nil { | ||
| return errors.Wrap(err, "get cluster") | ||
| } |
There was a problem hiding this comment.
this will overwrite anything we assign to status fields in Reconcile function, right?
There was a problem hiding this comment.
Yes, but it shouldn't cause any issues since we overwrite each field of the status in the reconcileCRStatus method
There was a problem hiding this comment.
i am not sure if it's okay, users test failure might be caused by this
There was a problem hiding this comment.
Pull Request Overview
This PR addresses an issue where cluster resourceVersion was constantly being updated during initialization due to incorrect LastTransitionTime handling in status conditions. The fix ensures that the latest cluster state is fetched before updating status conditions and removes manual LastTransitionTime management since it's handled automatically by meta.SetStatusCondition.
Key changes:
- Added cluster fetching in
reconcileCRStatusto ensure latest state is used - Removed manual
LastTransitionTimeassignments from condition objects - Added status write operation after MySQL version reconciliation
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/controller/ps/status.go | Fetches latest cluster state and removes manual LastTransitionTime |
| pkg/controller/ps/controller.go | Removes manual LastTransitionTime from bootstrap status |
| pkg/controller/psrestore/controller.go | Removes manual LastTransitionTime and reorganizes imports |
| pkg/controller/ps/version.go | Adds status write operation after version update |
| pkg/controller/ps/status_test.go | Adds test case for empty cluster scenario |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| "sync" | ||
| "time" | ||
|
|
||
| "github.com/pkg/errors" |
There was a problem hiding this comment.
[nitpick] The import reorganization moves a commonly used package to the top, but standard Go practice is to group standard library imports first, then third-party imports, then local imports. Consider keeping the original import grouping structure.
Copilot uses AI. Check for mistakes.
commit: d2aa69b |
https://perconadev.atlassian.net/browse/K8SPS-498
DESCRIPTION
Problem:
While the cluster is initializing, the cluster's
resourceVersionis constantly updated.Cause:
This happens because status conditions are continuously updated with an incorrect
latestTransitionTime. ThereconcileCRStatusmethod is used in multiple places in the reconcile loop, and whenever it is called, it updates the status conditions with an outdated/incorrectlatestTransitionTime, based on the cluster passed to the method.Solution:
Always fetch the latest cluster in the
reconcileCRStatusmethod.Note for developers: I removed
LatestTransitionTimefrom the condition objects because it is already managed by themeta.SetStatusConditionfunctionCHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
Config/Logging/Testability