Repository navigation
fix(backups): fail Velero RestoreJob rename for non-VMInstance kinds - #4426
Conversation
A RestoreJob that set targetApplicationRef.name to a new name and options.targetNamespace to another namespace reported Succeeded for any Velero-backed application, but only a VMInstance was actually renamed. postRestoreRename looks up the HelmRelease under the vm-instance- prefix, gets NotFound for every other kind and returns nil, so a Postgres, MariaDB, ClickHouse, MongoDB or standalone VMDisk copy came up under the source name with no error surfaced. Renaming those kinds through the Velero strategy cannot work: DataUpload restores volumes under their original names, and only a VMInstance keeps using them after its HelmRelease is renamed, because its disks keep their names and CDI adopts the restored PVCs. A renamed database Cluster or VMDisk renders fresh volumes and comes up empty. Reject the rename up front for every kind other than VMInstance, before the RestoreJob enters Running, with a message that points at omitting targetApplicationRef.name or using the engine-native strategy. The existing same-namespace guard keeps precedence, so its message is unchanged. Closes #3537 Assisted-By: LLM Signed-off-by: Andrey Kolkov <[email protected]>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughVelero restore reconciliation now rejects rename requests for application kinds other than VMInstance. For jobs that have started, it also cleans up resource-modifier ConfigMaps and reports that resources may already have been restored under the source name. ChangesVelero Restore Rename Guard
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~15 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Unsupported Velero renames now fail explicitly without an identified merge-blocking regression. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
github-actions
Bot
added
area/storage
The guard's regression cases only exercised renames, so dropping the IsRenamed conjunct (failing every non-VMInstance restore) or keying the check on targetApplicationRef.kind instead of the backup's kind left the package green. Add the inputs the guard must let through, a VMDisk copy that keeps its name and a VMDisk in-place restore without targetApplicationRef, and a VMDisk backup restored with a VMInstance target kind, which must still be rejected by the source kind. Assisted-By: LLM Signed-off-by: Andrey Kolkov <[email protected]>
…t jobs The refusal pointed every kind at "the application's engine-native strategy", but VMDisk, the only non-VMInstance kind the default BackupClass routes to Velero, has none. Offer that remedy only to kinds that can have one. The check also runs on every reconcile, so a rename RestoreJob that was already Running when the controller picked up the check is failed after its Velero Restore has completed. That job now says the application may already sit in the target namespace under its source name (a retry that drops the rename would otherwise trip failIfTargetExists on that copy without explanation), and its resource-modifier ConfigMap is released right away instead of at the job's deletion. Assisted-By: LLM Signed-off-by: Andrey Kolkov <[email protected]>
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
The guard rejects an unsupported Velero rename before the job goes Running (fresh job), or on the next reconcile for a job already in flight, with an explicit Ready=False message. Build, vet and test are green and the new tests are non-vacuous. One non-blocking note on the in-flight cleanup below.
Findings
- [MINOR]
internal/backupcontroller/velerostrategy_controller.go:623, In-flight cleanup releases the resource-modifier ConfigMap without gating on the Velero Restore phase
Caveats
- Verified statically and by unit test only (hermetic review, no live cluster). The terminal-phase early return at
internal/backupcontroller/restorejob_controller.go:96-100means an alreadySucceeded/Failedjob never re-enters this guard, so the in-flight branch fires only on a non-terminal job, which is what the PR body claims. The in-flight branch (velerostrategy_controller.go:620-626) leaves the VeleroRestoreCR to be reaped onRestoreJobdeletion, so it adds no new leak. Its one asymmetry with the existing cleanup paths, the missing Velero-phase gate, is the MINOR note above. - Upgrade path (existing customer): the only affected state is a rename
RestoreJobleftRunningby an older controller. It now fails with a message that names the possible source-name copy. Losing the priorSucceededis not a regression, because that success was the bug:postRestoreRenamereturns nil on theNotFoundfor thevm-instance-prefixed HelmRelease, so non-VM kinds came up under the source name. Fresh-install, chart and migration surfaces are N/A on a Go-only diff. - The guard keys on
backup.Spec.ApplicationRef.Kind(the source kind), not the user-suppliedtargetApplicationRef.kind. That is correct: Velero restores the source application's volumes, so only the source kind matters. AVMDiskbackup restored with aVMInstancetarget kind is still rejected, and a test covers it. golangci-lintreports issues in the package, all in files this PR does not touch (other strategy controllers,backupjob_controller_test.go), none in the two changed files. Pre-existing, out of scope.
Recommended follow-ups
- Non-VM rename-on-restore via engine-native recovery, and the standalone
VMDiskrebind (#4428), stay out of scope as stated. Not blocking.
| if restoreJob.Status.StartedAt != nil { | ||
| // The job predates this check: its Velero Restore may already have | ||
| // restored the application under the source name. | ||
| r.cleanupResourceModifierConfigMaps(ctx, restoreJob) |
There was a problem hiding this comment.
[MINOR] In-flight cleanup releases the resource-modifier ConfigMap without gating on the Velero Restore phase
The three existing calls to cleanupResourceModifierConfigMaps (:742, :748, :763) each run only after the Velero Restore reaches a terminal phase; the non-terminal branch at :733 requeues without cleanup. This new call site deletes the ConfigMap unconditionally. If the controller carrying this guard starts while an old-code rename job has a Velero Restore still in its pre-validation window (phase empty/New), the delete removes the ConfigMap before Velero reads it in validateAndComplete ([email protected] pkg/controller/restore_controller.go:407-426, a single kbClient.Get whose failure appends a ValidationError), so that Restore fails validation instead of completing. No data loss and the RestoreJob fails either way (a one-time migration race, and failing that stray Restore may even be preferable), so this is not blocking and may be intentional. Worth one line either way: TestReconcileVeleroRestore_RenameGuard_InFlight only exercises the Completed phase, so the non-terminal window is untested; a case with RestorePhaseNew (or empty) would document the intended behaviour.
2 participants
What this PR does
Fixes #3537.
A
RestoreJobwithspec.targetApplicationRef.nameset to a new name andoptions.targetNamespaceset to another namespace reportedphase: Succeededfor every Velero-backed application, but only aVMInstancewas actually renamed.postRestoreRenamelooks up the restored HelmRelease under thevm-instance-prefix, getsNotFoundfor any other kind and returnsnil, so a Postgres, MariaDB, ClickHouse, MongoDB or standalone VMDisk copy came up under the source name and nothing was surfaced.Renaming those kinds through the Velero strategy cannot work by construction: Velero DataUpload restores volumes under their original names, and only a
VMInstancekeeps using them after its HelmRelease is renamed (its disks keep their names and CDI adopts the restored PVCs). A renamed databaseClusterorVMDiskrenders fresh volumes for the new name and comes up empty next to orphaned source volumes.This PR takes the short-term, correctness half of the issue: the rename is rejected up front for every kind other than
VMInstance, before a new RestoreJob entersRunningor a VeleroRestoreis created, with aReady=Falsemessage that names the kind and points at omittingtargetApplicationRef.nameor, for kinds that have one, using the application's engine-native strategy (for Postgres that is the CNPG strategy, which supports restore-to-copy). The existing same-namespace guard keeps precedence, so its message is unchanged.A rename RestoreJob that is already
Runningwhen a controller with this check starts is failed on its next reconcile, after its VeleroRestoremay have completed. Such a job says in its message that the application may already sit in the target namespace under its source name (a retry without the rename would otherwise tripfailIfTargetExistson that copy), and its resource-modifier ConfigMap is released immediately rather than at the job's deletion. Jobs alreadySucceededorFailedare never revisited.Rename-on-restore for non-VM applications through engine-native recovery (the longer-term feature in the issue) is out of scope here. A standalone
VMDiskis rejected as well: its rename needs a post-restore PVC rebind and DataVolume replacement that do not exist yet, tracked in #4428.Tests:
TestVeleroRenameSupportedandTestReconcileVeleroRestore_RenameGuard. Rejected inputs: Postgres and VMDisk cross-namespace rename, and a VMDisk backup restored with aVMInstancetarget kind (the guard keys on the backup's kind, since that is whatpostRestoreRenameacts on) →Failedwith the new message; Postgres same-namespace rename → the pre-existing DataUpload message. Accepted inputs: VMInstance cross-namespace rename, a VMDisk copy that keeps its name, and a VMDisk in-place restore withouttargetApplicationRef→Running. The rejected cases fail onmain(phaseRunninginstead ofFailed); the accepted cases fail if theIsRenamedconjunct is dropped.TestReconcileVeleroRestore_RenameGuard_InFlightcovers the already-running case for VMDisk (Failed, message names the source-name copy, ConfigMap gone) and VMInstance (Succeeded).Screenshots
N/A — no UI changes.
Downstream repositories
Walked the trigger map in
docs/agents/contributing.mdagainst the diff: two Go files underinternal/backupcontroller/, no API, CRD, chart, values, schema or docs change.Release note
Summary by CodeRabbit