mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-03 02:27:01 +09:00
myasnikovdaniil
a26c441d78
## What this PR does After a Velero restore of a VMDisk or of a VMInstance (`RestoreJob` with the Velero strategy), the disk's HelmRelease fails on its next upgrade and stays failed: ``` Helm upgrade failed for release <ns>/vm-disk-<name> with chart vm-disk@...: failed to create resource: admission webhook "datavolume-validate.cdi.kubevirt.io" denied the request: Destination PVC <ns>/vm-disk-<name> already exists ``` The VMInstance release that references the disk then fails in turn (`Specified disk not exists in cluster`). Seen on a v1.6.5 cluster with CDI v1.66.1, where each failed upgrade is retried and the release history of one such disk had grown past 46,000 revisions. ### Cause The restore controller deletes the disk's DataVolume (`prepareForRestore`) and Velero restores the PVC under the same name, with no owner. The restored release matches its chart and values, so Flux does not upgrade it and the DataVolume is not recreated right away; the next Helm upgrade (a platform update, an edit of the VMDisk) does recreate it, and CDI's webhook refuses it. In CDI v1.66.1 the webhook admits a DataVolume over an existing claim only when `ClaimMayExistBeforeDataVolume` is true (`pkg/controller/common/util.go`): either the PVC carries `cdi.kubevirt.io/storage.populatedFor: <dv name>`, or `AllowClaimAdoption` returns true, which reads `cdi.kubevirt.io/allowClaimAdoption` **from the DataVolume** and otherwise falls back to the `DataVolumeClaimAdoption` feature gate. The Velero strategy's resource modifiers (`createResourceModifiersConfigMap`) put `allowClaimAdoption=true` on the restored **PVC**, where CDI never reads it, and the `CDI` CR in `packages/system/kubevirt-cdi` enables neither the gate nor anything equivalent. Migration 29 creates its PVCs with the same PVC-side annotation. ### Fix The vm-disk chart sets `cdi.kubevirt.io/allowClaimAdoption: "true"` on the DataVolume it renders, on the creation path and on the existing-DataVolume path alike, so the annotation does not flap between upgrades. - A fresh disk is unaffected: no PVC exists when the DataVolume is admitted, and the PVC CDI then creates carries `cdi.kubevirt.io/createdForDataVolume: <dv uid>`, for which `AllowClaimAdoption` returns false, so CDI imports, clones or uploads as before. - An existing DataVolume already controls its PVC, so the annotation changes nothing there; a Helm upgrade only adds it to the DataVolume's metadata, which CDI allows (only the spec is immutable). - After a restore, the webhook admits the recreated DataVolume and the DataVolume controller adopts the claim: `pvcRequiresWork` is false, it takes controller ownership of the PVC and the DataVolume goes `Succeeded` without populating it again, so the restored data is kept. Checked against a v1.6.5 cluster carrying the broken state, by rendering the chart for an affected disk and running `kubectl create --dry-run=server`: the DataVolume without the annotation is refused with the error above, the same one with the annotation is admitted. On that cluster two affected disks were later recovered through the claim-adoption path (the same `AllowClaimAdoption` check): the PVC kept its uid, became controlled by the new DataVolume, and the DataVolume reached `Succeeded` with no import. ### Alternatives considered - **Enable the `DataVolumeClaimAdoption` feature gate in the `CDI` CR.** It works too, but for every DataVolume in the cluster (tenant Kubernetes worker disks, golden images, KubeVirt `dataVolumeTemplates`), each of which would then silently take over any unowned PVC of its name. The per-DataVolume annotation keeps the behaviour to the one chart whose DataVolumes are expected to meet a restored claim, and CDI has said it means to retire the gate (kubevirt/containerized-data-importer#3480) while the annotation is checked first and independently of it. - **Stamp `cdi.kubevirt.io/storage.populatedFor` on the restored PVC** from the resource modifiers. That would also pass the webhook, but only for disks restored by the Velero strategy, not for claims created by migration 29 or by hand, and it would need a controller image change where a chart change suffices. The PVC-side `allowClaimAdoption` rule in `createResourceModifiersConfigMap` (and the one in migration 29) is inert under CDI v1.66.1. This PR leaves the rule and its unit-test assertions in place, and corrects the comments that credited it with the adoption (the collector and `createResourceModifiersConfigMap` in `velerostrategy_controller.go`, and step 5 of `examples/backups/vmi/scenario-user-restore.md`): they now say CDI adopts the restored PVC through the annotation the vm-disk chart sets on the DataVolume. The backports carry only the template and the unittest suite, so these comment and doc edits do not reach them. One behaviour change to be aware of: a PVC named `vm-disk-<name>` left behind without an owner (a restored disk deleted before this fix, whose PVC Velero restored without an owner) is now adopted by a new VMDisk of the same name, with its old contents, instead of blocking that VMDisk forever. That PVC lives in the same tenant namespace. ### Tests - `packages/apps/vm-disk/tests/dv_claim_adoption_test.yaml`: the post-restore state (a restored PVC, no DataVolume) renders the DataVolume through the creation path with the annotation, and the existing-DataVolume path keeps it. Both fail with the template change reverted. `helm unittest packages/apps/vm-disk`: 116 passed. - `examples/backups/vmdisk/05-restore-in-place.sh` (run by the `vmdisk-2-backup-roundtrip` Chainsaw suite): after the restore, the HelmRelease was already reported Ready because no upgrade ran, which is why CI did not catch this. The script now forces a Helm upgrade (`reconcile.fluxcd.io/forceAt` + `requestedAt`), then requires the release Ready, the DataVolume `Succeeded`, and the PVC to keep its restored uid and be controlled by the DataVolume. ### Backports Opened by hand as #4956 (`release-1.6`) and #4957 (`release-1.7`) rather than through the `kind/backport` labels: `examples/backups/vmdisk/` does not exist on either branch, so the bot's cherry-pick would conflict. They carry the template change and the helm unittest suite. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff: a vm-disk template change (no `values.yaml`, schema, `README.md` or `Chart.yaml` change), a new helm unittest file, and a backup walkthrough script. No app, field, default, CRD, platform value or tooling changes, and no documented workaround on the website is made obsolete (the only mention of `allowClaimAdoption` there is in the archived v0 docs, about a different manual procedure). - [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(vm-disk): a VMDisk restored by Velero, alone or as part of a VMInstance, no longer fails its next upgrade with "Destination PVC ... already exists": the DataVolume now carries cdi.kubevirt.io/allowClaimAdoption, so CDI adopts the restored PVC and keeps its data. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Bug Fixes** * Improved in-place VM disk restores so restored storage is correctly adopted when the disk resource is recreated. * The restore process now waits for the disk to become ready and reports an error if it fails or the restored storage is not retained. * Clarified the adoption behavior in restore documentation and ensured recreated disks use the required adoption setting. <!-- end of auto-generated comment: release notes by coderabbit.ai -->