fix(backups): create no driver object for a job that already finished - #4673

…eclaim policy (#4671)

## 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 -->