mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-09 14:02:56 +09:00
Aleksei Sviridkin
f4bde9e50d
<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `kind/backport` if it's a bugfix that needs to be
backported to a previous version.
-->
## What this PR does
An in-place VMInstance restore with `keepOriginalPVC: false` reports
`Succeeded` without restoring the disks, and leaves every disk it
touched unable to come back. `prepareForRestore` deletes each DataVolume
with Orphan propagation and leaves the PVC in place, on the assumption,
stated in the `deleteDataVolume` comment, that Velero overwrites it.
Velero does not overwrite a PVC. Its CSI restore action returns early
for a claim that already exists, so no DataDownload is ever created, and
with `existingResourcePolicy: update` the merge patch it attempts
instead is rejected because a bound claim's spec is immutable. All that
reaches the claim is the labels-only fallback patch, which also drops
the `cdi.kubevirt.io/allowClaimAdoption` annotation the resource
modifier added.
The claim left behind then blocks the disk for good. Orphan propagation
has removed its owner reference to the DataVolume, and it lacks the
`cdi.kubevirt.io/storage.populatedFor` annotation that
kubevirt-velero-plugin writes on the backed-up copy, so when the
HelmRelease resumes, the CDI webhook refuses the DataVolume it creates
with `Destination PVC <ns>/<name> already exists`, on every reconcile.
Deleting the disk through the API leaves the PVC behind, and a new disk
of the same name fails the same way.
Observed on a v1.6.3 cluster, for a VMInstance restore with
`keepOriginalPVC: false` whose disk was attached to the VM. The Velero
log for the restore, trimmed:
```text
level=info msg="Retrieved per-restore resource modifiers from configmap cozy-velero/restore-modifiers-..."
level=warning msg="PVC already exists and ExistingVolumeDataPolicy is not in-place restore. Skip restore this PVC." Action=PVCCSIRestoreItemAction PVC=tenant-datalab/vm-disk-e2edisk
level=info msg="Applying resource modifier patch on tenant-datalab/vm-disk-e2edisk"
level=warning msg="patch attempt failed for /v1, Kind=PersistentVolumeClaim tenant-datalab/vm-disk-e2edisk: PersistentVolumeClaim \"vm-disk-e2edisk\" is invalid: spec: Forbidden: spec is immutable after creation except resources.requests and volumeAttributesClassName for bound claims"
level=info msg="backup/restore labels successfully updated for PersistentVolumeClaim tenant-datalab/vm-disk-e2edisk"
```
`velero restore describe --details` lists the PVC as `(failed)` and the
Restore is `Completed` with two warnings. Afterwards the claim had its
original creation timestamp and UID, no owner reference, no
`allowClaimAdoption`, no `populatedFor`, and the disk's HelmRelease was
`Ready=False` with the webhook error above. That cluster runs a Velero
build from `main`, whose wording is quoted above; the released v1.17.1
and v1.18.3 return early at the same point with `PVC already exists.
Skip restore this PVC.` (`pkg/restore/actions/csi/pvc_action.go`).
The fix deletes the PVC when `keepOriginalPVC` is false, only once its
DataVolume is confirmed gone so that CDI cannot recreate it, and holds
the Velero Restore until the claim has actually disappeared. A claim
still mounted stays `Terminating` behind `kubernetes.io/pvc-protection`,
and creating the Restore then would hit the same existing-claim path.
Velero then creates the PVC from the backup, the data mover fills it,
and the recreated DataVolume adopts it through `populatedFor`. A failed
deletion now fails the RestoreJob instead of letting it report
`Succeeded` over the old data. Before 2234824fe the DataVolume was
deleted with background propagation, so garbage collection removed the
claim, but nothing waited for it.
Deleting the claim is only safe when Velero can put the disk back, so
before any preparation the reconciler now gets the Velero Backup and
fails the RestoreJob unless it exists and is `Completed`. The Backup
artifact outlives the Velero Backup, which Velero garbage-collects at
its TTL, and without the check a restore of an expired backup would halt
the VM and delete its disks before failing. The check runs for every
Velero restore, whatever `keepOriginalPVC` says. A DataVolume that
cannot be deleted now fails the RestoreJob too, and the preparation
waits for every DataVolume to be gone before it goes further: a
surviving DataVolume makes CDI recreate the claim, which Velero then
skips. Reading the DataVolume back needs `get` on `datavolumes`, which
the backupstrategy-controller ClusterRole now grants next to `delete`.
`TestPrepareForRestore_KeepOriginalPVCFalse_SkipsRename` asserted the
old behaviour, that the original PVC survives, and is replaced by
`TestPrepareForRestore_KeepOriginalPVCFalse_DeletesPVC`.
`TestPrepareForRestore_KeepOriginalPVCFalse_WaitsForTerminatingPVC`
covers the wait. Both fail on `upstream/main` without the controller
change. `TestReconcileVeleroRestore_UnusableVeleroBackupTouchesNothing`
covers an expired Velero Backup and one being deleted,
`TestPrepareForRestore_KeepOriginalPVCFalse_KeepsPVCWhileDataVolumeRemains`
a failed and a pending DataVolume deletion, and
`TestPrepareForRestore_KeepOriginalPVCTrue_FailedRenameKeepsPVC` a
failed rename under `keepOriginalPVC: true`, which must leave the claim
in place. A helm-unittest suite, `tests/restore_rbac_test.yaml`, checks
the new verb. Each test fails with its fix reverted, with one exception:
`TestPrepareForRestore_KeepOriginalPVCTrue_FailedRenameKeepsPVC` still
passes with the Step 5 condition cut to `vmRes != nil`, because a failed
rename holds the restore back before Step 5 is reached. That guard is
pinned by `TestPrepareForRestore_KeepOriginalPVC_CacheLagsBehindWrites`,
which fails with the same cut. `helm unittest
packages/system/backupstrategy-controller` passes. `go test
./internal/backupcontroller/...` passes.
Related: #4601 reports the same Velero mechanism for a standalone
VMDisk, where `prepareForRestore` does nothing at all because
`collectUnderlyingResources` records no disk. #3427 makes a VMDisk
report its own disk; with `keepOriginalPVC: false` that path then runs
into this defect too, and this change covers it once both are in. This
PR does not touch the VMDisk path itself.
### Screenshots
N/A — no UI changes.
### Downstream repositories
Walked the trigger map against the diff, which is the restore
preparation in `internal/backupcontroller/velerostrategy_controller.go`
and its unit tests, plus one verb in the backupstrategy-controller
ClusterRole and its helm-unittest: no package added or renamed, no
`values.yaml` or `values.schema.json` change, no CRD,
ApplicationDefinition, label, annotation, namespace or metric change.
`keepOriginalPVC` lives in the opaque `RestoreJob.spec.options`, and a
search of the organisation finds it only in this repository, so neither
the website nor the Terraform provider documents or models it.
- [x] 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 VMInstance RestoreJob with keepOriginalPVC=false now deletes each original disk PVC, and waits for it to be gone, before Velero restores it. Previously the PVC was left in place, Velero skipped restoring its data, the RestoreJob still reported Succeeded, and the disk's HelmRelease could never recreate its DataVolume ("Destination PVC already exists"). A Velero RestoreJob now also fails before touching the application when its Velero Backup is gone or not Completed, for example after the Backup expired at its TTL.
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* When a restore is configured not to keep the original disk claim, the
claim is deleted before Velero restores it, allowing Velero to recreate
it.
* Restores wait while the original disk claim or its DataVolume is being
deleted. Deletion or status-check errors stop restore preparation.
* Restores configured to keep the original disk claim continue to
preserve it.
* Restores stop before changing resources if the referenced Velero
backup is missing or has not completed.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->