mirror of
https://github.com/cozystack/cozystack.git
synced 2026-10-30 16:47:20 +09:00
Aleksei Sviridkin
ff3c409881
## What this PR does Two defects in `renamePVC` (`internal/backupcontroller/velerostrategy_controller.go`), the step of an in-place VMInstance restore that keeps each disk as `<dv>-orig-<shortHash(RestoreJob name)>` when `keepOriginalPVC` is true, the default. ### A taken `-orig` name skipped the rename, and the restore went on `renamePVC` returned `nil` as soon as a PVC with the `-orig` name existed (`main:1263-1266`), whatever that PVC was. `prepareForRestore` then reached Step 4 (`main:1172-1183`), which deletes the DataVolume of every disk regardless of what Step 3 did, because a Step 3 error was only ever turned into a Warning event (`main:1165-1168`). The restore therefore ran exactly as `keepOriginalPVC: false` does and reported `Succeeded`: the live disk was not kept, and the `-orig` name held a different, older disk that the user takes for the pre-restore copy. When that name can already exist: - A RestoreJob created under the name of a deleted one. The hash covers only the RestoreJob name, and nothing in the controller ever deletes an `-orig` PVC, so the predecessor's PVC is still there. This is the realistic case, and it is what a scripted restore with a fixed RestoreJob name runs into on its second run unless the `-orig` PVC was removed in between. - Two RestoreJobs with different names whose 4-hex-digit hashes collide (16 bits) while restoring the same disk in the same namespace. Rare, but nothing guards it. - A reconcile of the same RestoreJob after a controller restart in the middle of a rename. The name is ours, but the early return treated a half-done rename as complete: the original PVC stayed in place and the PV stayed bound to it. - Truncation does not apply: the name is not truncated, and one longer than 253 characters makes the `Create` fail, an error that was swallowed in the same way. Each `-orig` PVC now carries the UID of the RestoreJob that created it, in `backups.cozystack.io/restore-job-uid`. `prepareForRestore` checks every `-orig` name before it suspends or halts anything, and fails the RestoreJob, naming the PVC, when any of them is held by a PVC another RestoreJob created. When the PVC is ours, `renamePVC` resumes: it deletes the original PVC only while it still claims the kept PV, and repoints the PV unless it already points at the `-orig` PVC, so a rerun is a no-op. Any other rename error now holds the restore back before Step 4 deletes a DataVolume, and the restore is retried, instead of the error becoming an event (see the next section). An `-orig` PVC created by an earlier controller has no UID annotation and is refused like any foreign PVC. The only RestoreJob that loses by this is one whose controller was upgraded after its rename and before its Velero Restore was created; it fails with the message above rather than going on. ### The rename read its own write back through the cache `renamePVC` created the `-orig` PVC, then read it back through the cached client to learn its UID for the PV's claimRef (`main:1314-1316`). The informer often has not seen a PVC created a moment earlier. On our lab, running Cozystack v1.6.3, an in-place restore of a two-disk VM with `keepOriginalPVC: true` renamed and restored the system disk, while the data disk lost the race, with this event on the RestoreJob: `Failed to keep old PVC vm-disk-e2e-dc-fix-data: failed to get -orig PVC vm-disk-e2e-dc-fix-data-orig-a574: PersistentVolumeClaim "vm-disk-e2e-dc-fix-data-orig-a574" not found`. As the error was only an event, the data disk's DataVolume was deleted and its original PVC stayed in place. Velero skipped that PVC and created no DataDownload for it, so its content was not restored, yet the restore finished `Succeeded`. The `-orig` PVC was left `Pending` on the same PV, which was by then `Retain`. `Create` already fills in the UID, so the read-back is gone. The other reads the rename decides on (the `-orig` PVC, the original PVC and the PV, before and after its own writes) go through the manager's uncached reader, wired as `APIReader`. The PV rebind retries on conflict, since the PV controller also writes the PV once the original PVC is deleted. A rename that fails for any reason other than a foreign `-orig` name requeues the restore rather than failing it. The DataVolume is still there, and the retry finds its own `-orig` PVC by the RestoreJob UID and resumes where it stopped. A cause that never clears keeps the RestoreJob `Running`, with a Warning event on each attempt. #4673 adds the same `APIReader` field and `apiReader()` helper to `RestoreJobReconciler`, for its finished-job guard. Whichever of the two lands second drops its own copy when it rebases. ### The PV's reclaim policy was overwritten without being recorded `renamePVC` switches the PV to `Retain` so it survives the deletion of the original PVC (`main:1289-1293`), and the policy it had was lost. I looked for a path in the controller that releases or cleans up `-orig` PVCs, and there is none: `cleanupOnDelete` on a Velero RestoreJob removes only the Velero `Restore` and the resource-modifier ConfigMaps, and no code deletes an `-orig` PVC or touches its PV again. A PV that was `Delete` therefore stays `Retain` for good, and its volume leaks when an operator deletes the `-orig` PVC. The original policy is now recorded in `backups.cozystack.io/original-reclaim-policy`, on the PV and on the `-orig` PVC, following the `backups.cozystack.io/` prefix the controller already uses for `underlying-resources`. A value already present on the PV is kept, as it predates any rename. Since there is no release path to restore it from, `examples/backups/vmi/scenario-user-restore.md` gains a section for the operator who deletes an `-orig` PVC: set the recorded policy back on the PV first, with a snippet that leaves a PV without the annotation on `Retain`. Restoring the policy from the controller itself, for instance once the Velero Restore has completed and the PV is `Bound` to the `-orig` PVC, would close the leak without the operator. I did not do it, because it changes what deleting an `-orig` PVC destroys, from nothing to the volume; that is a maintainer call, and the annotation is what it would read. ### How this fits with #3427 #3427 is complementary, and the two do not touch the same code: it changes `collectUnderlyingResources`, this PR changes `prepareForRestore` and `renamePVC`. Both append tests to `velerostrategy_controller_test.go`, so whichever lands second may need a trivial rebase. - #3427 makes a standalone VMDisk report its own disk, which sends VMDisk in-place restores through this same `renamePVC`. Once both land, the VMDisk path gets the same guard and the same annotation. - Its `examples/backups/{vmi,vmdisk}/cleanup.sh` and the Chainsaw `finally` reclaim each `-orig` PVC by patching its PV to `Delete` before deleting the PVC. That works around, in the e2e teardown, the leak described above. It is correct on the LINSTOR classes, which are `Delete`, but it would destroy a volume whose class was `Retain`. With this PR the scripts can read `backups.cozystack.io/original-reclaim-policy` instead of hard-coding `Delete`. I have not changed them here, since they only exist on that branch. - Its round-trips use fixed RestoreJob names and assert that a `<pvc>-orig-*` PVC exists after the restore. If a failed teardown leaves one behind, today the next run skips the rename and the assertion still passes on the stale PVC; with this PR the RestoreJob fails and names it. - Its cleanup comments describe the `-orig` PVC as a bare ObjectMeta with no labels or owner. That stays true: this PR adds annotations only, so its name-based lookup is unaffected. ### Tests New tests in `internal/backupcontroller/velerostrategy_controller_test.go`, run through `prepareForRestore` with the DataVolume in the dynamic fake client: - `TestPrepareForRestore_KeepOriginalPVC_RefusesForeignOrigPVC`: an `-orig` PVC without the annotation, and one carrying another RestoreJob's UID, both fail the restore with the PVC named, and leave the DataVolume, the live PVC, the PV and the foreign PVC untouched. - `TestPrepareForRestore_KeepOriginalPVC_ResumesOwnRename`: a rename interrupted after the `-orig` PVC was created completes on the next pass, and a second pass changes nothing. - `TestPrepareForRestore_KeepOriginalPVC_FailedRenameKeepsDataVolume`: a rename that fails for another reason (missing PV) requeues the restore without failing it and keeps the DataVolume, and once the PV exists the retry completes the rename. - `TestPrepareForRestore_KeepOriginalPVC_CacheLagsBehindWrites`: the reconciler's cached client serves every read from the objects as they were seeded, through an `interceptor` over the fake client, as an informer that has not caught up with this reconcile's writes would, while `APIReader` sees them. The rename must complete. - `TestRenamePVC_RecordsOriginalReclaimPolicy`: a `Delete` PV and a `Retain` PV both end up `Retain`, with their original policy recorded on the PV and on the `-orig` PVC. With the controller reverted to `main` (only the two annotation constants kept so the tests compile), all four fail: `prepareForRestore() = ready true, nil error` with `DataVolume was deleted although the disk could not be kept` for both foreign cases and for the failed rename, `original PVC still exists` and the PV still bound to it for the interrupted rename, and an empty `original-reclaim-policy` on the PV and on the PVC. For the cache race, the first commit alone fails `CacheLagsBehindWrites` with the lab's error, `failed to keep PVC vm-disk-ubuntu-source: failed to get -orig PVC vm-disk-ubuntu-source-orig-de3e: persistentvolumeclaims "vm-disk-ubuntu-source-orig-de3e" not found`, and `FailedRenameKeepsDataVolume` with a terminal error instead of a requeue. On `main` the race test reproduces the lab outcome: `original PVC still exists`, and the PV still claimed by it. With both commits, `go test -count=1 ./internal/backupcontroller/...` passes. No API type, CRD, chart or values file changes, so there is nothing to regenerate. ### Screenshots N/A, no UI change. ### Downstream repositories Walked the trigger map against the diff: the restore preparation in `internal/backupcontroller/velerostrategy_controller.go`, its tests, and `examples/backups/vmi/scenario-user-restore.md`. No CRD, ApplicationDefinition, values, schema, enum, namespace, label, metric or tooling change, so terraform-provider-cozystack, ansible-cozystack, ccp, talm, cozyhr, cozy-proxy, cozystack-telemetry-server, external-apps-example, examples and community are not reached. `RestoreJob.spec.options` stays opaque to the provider. cozystack/website is reached: `content/en/docs/next/virtualization/backup-and-recovery.md` (and each released copy) tells users to delete the `*-orig-*` PVCs "to free up storage". A PV left on `Retain` frees nothing, and the page should explain the recorded policy and the new failure for a reused RestoreJob name. No follow-up exists there yet, so the box stays empty until one is filed and linked. - [ ] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(backups): an in-place VM restore with keepOriginalPVC now fails, before the VM is halted, when the <disk>-orig-<hash> PVC it would keep the disk under already exists and was not created by this RestoreJob (for instance one left by a deleted RestoreJob of the same name), instead of skipping the rename and restoring without keeping the current disk. Any other failure to keep a disk, including a race in which the rename read its just-created -orig PVC back through the cache and failed, now holds the restore back and retries it before its DataVolume is deleted. Previously such a restore reported success without restoring the disk. The PV's reclaim policy before the switch to Retain is recorded in the backups.cozystack.io/original-reclaim-policy annotation on the PV and the -orig PVC; set it back on the PV before deleting the -orig PVC, or its volume is not reclaimed. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * In-place restores that preserve original PVCs can now resume safely after interruptions, including when reads lag behind recent changes. * Restores stop before suspending the VM if a conflicting `-orig` PVC already exists. PVC rename failures can be retried, while conflicting names remain fatal. * Original PV reclaim policies are recorded and restored before preserved PVCs are deleted. * **Documentation** * Updated the restore guide with disk-reclamation steps and guidance for resolving leftover PVC name conflicts. <!-- end of auto-generated comment: release notes by coderabbit.ai -->