K8SPS-498: fix status updates #1140
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors status update operations to use a callback-based approach, preventing race conditions and ensuring atomic status updates. The changes address K8SPS-498 by modifying the writeStatus function to accept an update callback instead of a complete status object.
Key changes:
- Modified
writeStatusto use a callback function that operates on a fresh copy of the CR - Updated all status update call sites to use the new callback pattern
- Removed an unnecessary
Getcall inEnsureComponent
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/k8s/utils.go | Removed redundant object retrieval before creating PodDisruptionBudget |
| pkg/controller/ps/version.go | Converted direct status field assignments to use callback-based writeStatus calls |
| pkg/controller/ps/status.go | Refactored writeStatus to accept update callback and rewrote reconcileCRStatus to use callback pattern |
| pkg/controller/ps/controller.go | Updated bootstrap status reconciliation to use new callback-based status update pattern |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
The merge conflicts will be resolved after #1139 merge |
pooknull
requested review from
egegunes,
gkech,
hors and
nmarukovich
as code owners
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/controller/ps/status_test.go
Outdated
| // It writes the `scripts[execCount].stdout`, `scripts[execCount].stderr` to `stdin` and `stderr` parameters. | ||
| // fakeClient should have the array of fakeClientScript objects in order they are going to be executed in the tested function. | ||
| func (c *fakeClient) Exec(_ context.Context, _ *corev1.Pod, _ string, command []string, stdin io.Reader, stdout, stderr io.Writer, _ bool) error { | ||
| fmt.Println("TEST", command) |
There was a problem hiding this comment.
Debug print statement should be removed before merging to production. This appears to be leftover debugging code.
| fmt.Println("TEST", command) | |
| if err := cl.Get(ctx, client.ObjectKeyFromObject(obj), obj); err != nil { | ||
| return errors.Wrap(err, "get statefulset") | ||
| } |
There was a problem hiding this comment.
Trying to understand why we performed this get before. Do we still have the context of that decision?
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err != nil { | ||
| return errors.Wrap(err, "get MySQL status") | ||
| } | ||
| mysqlStatus.ImageID = status.MySQL.ImageID |
There was a problem hiding this comment.
The ImageID field is being manually preserved from the existing status, but this is fragile. If other fields need to be preserved in the future, they must also be manually copied. Consider having appStatus accept the existing status and preserve fields that shouldn't be recomputed, or document clearly why ImageID needs special handling.
commit: 32b9210 |
https://perconadev.atlassian.net/browse/K8SPS-498
DESCRIPTION
This PR fixes 2 issues:
...get statefulset: ...error during cluster deployment..status.mysql.imageIDconstantly appearing and disappearing.It also reduces the risk of writing an outdated status by introducing
updateStatusfunction intowriteStatus. This waywriteStatuswill update.statusbased on the latest.statusstate.CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
Config/Logging/Testability