Persist Sandbox status on reconcile errors#366
Conversation
|
[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 |
|
i have recreated the pr for the sandbox status feature, earlier pr had some merge conflicts which i have completely resolved |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #366 +/- ##
==========================================
+ Coverage 75.90% 75.92% +0.01%
==========================================
Files 145 145
Lines 10626 10626
==========================================
+ Hits 8066 8068 +2
+ Misses 2212 2211 -1
+ Partials 348 347 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR updates the Sandbox controller reconcile flow to persist computed Sandbox.status updates even when the phase-specific control logic returns an error, improving status observability during failed operations.
Changes:
- Persist
newStatusviaupdateSandboxStatuson any control error (not just the Upgrading phase), while still returning the original reconcile error. - Log status persistence failures without masking the original control error.
- Add a controller test that verifies status mutations made by control logic are persisted even when that control logic returns an error.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/controller/sandbox/sandbox_controller.go | Persists computed Sandbox status on control errors across all phases, logging persistence failures while returning the original error. |
| pkg/controller/sandbox/sandbox_controller_test.go | Adds a focused reconcile test ensuring status updates are persisted even when control logic fails. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Ⅰ. Describe what this PR does
This PR ensures the Sandbox controller persists computed status updates even when phase-specific reconcile logic returns an error.
Previously, most control-error paths returned immediately without calling
updateSandboxStatus. Only theUpgradingphase had special-case status persistence. This meant useful status changes could be lost when reconcile failed, making the KubernetesSandbox.statusless accurate during failed pause/resume/update/upgrade operations.Changes
updateSandboxStatuson all control errors, not onlySandboxUpgrading.newStatusand then returns an error.Why
Users should be able to inspect
Sandbox.statusand understand what happened, even if the controller hit an error and will retry.This improves visibility for fields such as:
status.phasestatus.messagestatus.conditionsⅡ. Does this pull request fix one issue?
"NONE" but resolves the TODO at sandbox_controller.go (line 245)
Ⅲ. Describe how to verify it
Ⅳ. Special notes for reviews
I also did manual kind verification
Deployed the locally built controller image into kind:
Created a demo Sandbox and confirmed it reached Running:
Triggered a failing recreate upgrade with a failing preUpgrade hook:
Confirmed status was persisted with the failed upgrade state: