Files
cozystack/internal/migrationcontroller/vmimporttask_controller_test.go
Aleksei Sviridkin d9ed566326 fix(migration): restore the class reclaim policy on imported volumes (#4480)
<!-- 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

The VM import handoff sets the transferred `PersistentVolume` to
`Retain` so the data survives the swap from the Forklift-populated claim
to the VMDisk's claim, and then never gives the policy back. Every
imported disk therefore ends up with `persistentVolumeReclaimPolicy:
Retain`, while its StorageClass says `Delete` and every VMDisk created
directly carries `Delete`. When the tenant later deletes an imported
VMDisk through the normal path, its DataVolume and claim go away, but
the volume stays `Released` with its backing storage: an orphaned,
billed volume that no tenant can see or remove.

#### How it was measured

On a lab cluster with a LINSTOR-backed `replicated` class
(`reclaimPolicy: Delete`): a 16 GiB VM imported from vSphere produced a
VMDisk whose PV was `Retain`, while every non-imported VMDisk's PV in
the cluster was `Delete`. Deleting the VMInstance and then the VMDisk
removed the DataVolume and the claim and left the PV `Released`, still
`Retain`, with its LINSTOR resource in place. Patching that PV back to
`Delete` made the CSI provisioner delete both the PV and the LINSTOR
resource, which confirms the reclaim policy is the only thing keeping
it.

#### The fix

`Retain` is now held only for the window it protects. A new step,
`settleReclaimPolicy`, restores the policy the volume's StorageClass
declares (`Delete` when the class names none or no longer exists) once
the handoff is provably over, and until then reports "not yet" and
touches nothing:

- the replacement claim is `Bound` and the PV is `Bound` with a
`claimRef` naming that claim by UID, as the PV controller reports it
rather than as the controller wrote it, so no release is in flight;
- the transferred claim is gone;
- the claim's one and only owner is the VMDisk's DataVolume, as a
controller reference whose UID matches the live DataVolume (CDI's
adoption), so the only deletion that can reach the volume is the
VMDisk's own;
- the VMDisk exists and carries this task's output markers.

`fulfill` runs this step after adopting every disk and before creating
the VMInstance and deleting the scaffolding VM, so a VM is reported
`Succeeded` only once its volumes carry their class's policy, and an
import that fails after this point still leaves disks that clean up like
any other. The step is a compare-and-set, so repeated passes and
restarts are no-ops once settled. It also runs on the path that recovers
outputs whose status was never written, for tasks caught mid-handoff by
an upgrade. The comment that called permanent `Retain` "part of the
contract" and the matching paragraph in
`packages/system/migration-controller/README.md` are updated; the README
also says how to fix volumes left on `Retain` by imports completed
before this change, since those tasks are terminal and are not
reconciled again.

In practice a VM now stays in `Creating` for one more requeue (about 20
s) after the swap, while the PV controller rebinds and CDI adopts the
claim.

#### Tests

- `TestSettleRestoresTheClassReclaimPolicy`: a finished handoff gets the
class's policy back: `Delete`, `Retain` when the class asks for it, and
`Delete` when the class names none or is gone.
- `TestSettleIsIdempotent`: a second pass over a settled volume reports
settled and does not rewrite the PV.
- `TestSettleKeepsRetainWhileTheHandoffIsOpen`: thirteen cases, each
breaking one condition (claim not created by this import, claim not
bound, PV still `Released`, `claimRef` still on the old claim,
`claimRef` on the new claim with a stale UID, transferred claim still
present, claim not yet adopted by CDI, an extra owner, a DataVolume
owner that is not the controller, an owner reference to a previous
DataVolume, DataVolume missing, VMDisk missing, VMDisk not created by
this import). Every one keeps `Retain`.
- `TestFulfillRestoresTheReclaimPolicyBeforeFinishing`: behavioural
regression over `fulfill`. The first pass swaps the volume, keeps
`Retain`, creates no VMInstance and keeps the scaffolding VM. After
simulating the PV controller and CDI, the second pass restores `Delete`,
creates the VMInstance and removes the scaffolding VM. With the new step
turned into a no-op, this test fails.
- `TestFulfillSettlesReclaimedOutputs`: the recovery path for outputs
whose status was never written. With the handoff still open the volume
keeps `Retain`; once it is finished the policy goes back to `Delete`.
- The existing `adoptVolume` tests still assert `Retain` during the
swap.

`go test ./internal/migrationcontroller/...` and `make test-controllers`
pass, `go vet` is clean, and `make generate` leaves the tree unchanged.


### Screenshots

<!-- REQUIRED for UI changes: attach screenshots or screen recordings
demonstrating
the visual impact of your changes. PRs with UI changes without
screenshots will not be merged. -->

### Downstream repositories

<!--
Cozystack is upstream for repositories that are NOT kept in sync with it
automatically, and nothing in CI compares the two sides, so a change
here can break them silently.

Which change forces what, and which file to touch, is the trigger map in
docs/agents/contributing.md under "Downstream Repositories". Walk it
before you tick anything.

IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM
TO FILL IN:

1. Walk the trigger map against the actual diff of this PR, file by
file. Do not decide from the PR title or from your memory of what you
changed.
2. For every repository you tick, open the follow-up PR there and link
it on the same line. A ticked box with no link claims work that does not
exist, which is worse than an empty box. Search that repository for an
open PR or issue covering it first, and link that instead of filing a
duplicate.
3. When the follow-up is out of scope, or needs a decision that is not
yours to make, open an issue in that repository instead, link that, and
say so in the PR body. Do not push a speculative PR into another
repository just to fill a line here.
4. Do not tick "No downstream repository is affected" to make the
checklist go away. If you are unsure, leave every box empty and say so
in the PR body, so a human decides.
-->

- [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:

The diff touches only `internal/migrationcontroller/`,
`packages/system/migration-controller/README.md` and a comment in
`packages/system/migration-controller/templates/rbac.yaml`, which match
no entry of the trigger map; the website's VMware migration page does
not describe the reclaim policy, so it needs no change.

### Release note

<!--  Write a release note:
- Explain what has changed internally and for users.
- Start with the same `type(scope):` prefix as in the PR title
- Follow the guidelines at
https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md.
-->

```release-note
fix(migration): disks imported from VMware no longer keep a Retain reclaim policy after the import. The volume gets its StorageClass policy back once it is bound to its VMDisk, so deleting an imported VMDisk deletes its volume instead of leaving an orphaned Released PersistentVolume. Volumes imported before this fix keep Retain and have to be patched back by hand.
```


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Bug Fixes**
* Imported volumes are protected during claim transfers, then returned
to their StorageClass reclaim policy after the replacement claim and
volume are safely adopted. If the class or policy is unavailable, the
policy defaults to `Delete`.
* VM imports wait for volume adoption and reclaim-policy settlement
before completing, helping prevent premature volume deletion and
incomplete imports. Previously unrecorded handoff outputs are also
settled before being returned.
* **Documentation**
* Clarified reclaim-policy behavior during imports and how to restore
cleanup behavior for volumes from older imports.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
2026-09-29 14:41:04 +03:00

886 lines
36 KiB
Go

// SPDX-License-Identifier: Apache-2.0
package migrationcontroller
import (
"context"
"encoding/json"
"reflect"
"strings"
"testing"
corev1 "k8s.io/api/core/v1"
storagev1 "k8s.io/api/storage/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/tools/record"
clientfake "sigs.k8s.io/controller-runtime/pkg/client/fake"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
migrationv1alpha1 "github.com/cozystack/cozystack/api/migration/v1alpha1"
)
func storageClass(name string, mode storagev1.VolumeBindingMode, isDefault bool) *storagev1.StorageClass {
sc := &storagev1.StorageClass{
ObjectMeta: metav1.ObjectMeta{Name: name},
VolumeBindingMode: &mode,
}
if isDefault {
sc.Annotations = map[string]string{defaultStorageClassAnnotation: "true"}
}
return sc
}
func readySource(name, namespace string) *migrationv1alpha1.VMImportSource {
src := vsphereSource(name, namespace)
src.Status.Conditions = []metav1.Condition{{
Type: migrationv1alpha1.SourceReadyCondition,
Status: metav1.ConditionTrue,
Reason: migrationv1alpha1.ReasonConnected,
Message: "ok",
LastTransitionTime: metav1.Now(),
}}
return src
}
func task(name, namespace, source, class string, vms ...migrationv1alpha1.VMImportRequest) *migrationv1alpha1.VMImportTask {
return &migrationv1alpha1.VMImportTask{
ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace, UID: types.UID(name + "-uid")},
Spec: migrationv1alpha1.VMImportTaskSpec{
SourceRef: corev1.LocalObjectReference{Name: source},
VMs: vms,
StorageClass: class,
},
}
}
// TestTaskRejectsWaitForFirstConsumer is the check that turns a hang into a
// message. A WaitForFirstConsumer class does not fail an import, it deadlocks
// it: nothing consumes the claim while CDI populates it. Failing at validation
// costs one edit; not failing costs a support ticket and a stuck transfer.
func TestTaskRejectsWaitForFirstConsumer(t *testing.T) {
s := testScheme(t)
src := readySource("vcenter", "tenant-foo")
tk := task("import", "tenant-foo", "vcenter", "local",
migrationv1alpha1.VMImportRequest{ID: "vm-1", Name: "web-01"})
c := clientfake.NewClientBuilder().WithScheme(s).
WithObjects(src, tk, storageClass("local", storagev1.VolumeBindingWaitForFirstConsumer, true)).
WithStatusSubresource(src, tk).Build()
r := &VMImportTaskReconciler{Client: c, Scheme: s, Recorder: record.NewFakeRecorder(10)}
if _, err := r.Reconcile(context.Background(), reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: "tenant-foo", Name: "import"},
}); err != nil {
t.Fatalf("reconcile: %v", err)
}
got := &migrationv1alpha1.VMImportTask{}
if err := c.Get(context.Background(), types.NamespacedName{Namespace: "tenant-foo", Name: "import"}, got); err != nil {
t.Fatalf("get task: %v", err)
}
if got.Status.Phase != migrationv1alpha1.VMImportTaskPhaseFailed {
t.Errorf("phase = %q, want Failed", got.Status.Phase)
}
if !strings.Contains(got.Status.Message, "WaitForFirstConsumer") {
t.Errorf("message does not name the binding mode: %q", got.Status.Message)
}
// Nothing may have been created: a task that cannot succeed must not leave
// Forklift objects behind for a tenant to wonder about.
plan := newObject(planGVK)
if err := c.Get(context.Background(), types.NamespacedName{
Namespace: "tenant-foo", Name: planName("import", "vm-1"),
}, plan); err == nil {
t.Error("a Plan was created for a task that failed validation")
}
}
// TestTaskWaitsForUnreadySource asserts a task does not start work while its
// connection is unusable, and says which connection and why.
func TestTaskWaitsForUnreadySource(t *testing.T) {
s := testScheme(t)
src := vsphereSource("vcenter", "tenant-foo") // no Ready condition
tk := task("import", "tenant-foo", "vcenter", "replicated",
migrationv1alpha1.VMImportRequest{ID: "vm-1", Name: "web-01"})
c := clientfake.NewClientBuilder().WithScheme(s).
WithObjects(src, tk).WithStatusSubresource(src, tk).Build()
r := &VMImportTaskReconciler{Client: c, Scheme: s, Recorder: record.NewFakeRecorder(10)}
if _, err := r.Reconcile(context.Background(), reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: "tenant-foo", Name: "import"},
}); err != nil {
t.Fatalf("reconcile: %v", err)
}
got := &migrationv1alpha1.VMImportTask{}
if err := c.Get(context.Background(), types.NamespacedName{Namespace: "tenant-foo", Name: "import"}, got); err != nil {
t.Fatalf("get task: %v", err)
}
if got.Status.Phase != migrationv1alpha1.VMImportTaskPhasePending {
t.Errorf("phase = %q, want Pending", got.Status.Phase)
}
if !strings.Contains(got.Status.Message, "vcenter") {
t.Errorf("message does not name the source: %q", got.Status.Message)
}
}
// TestTaskRefusesToOverwriteExistingInstance locks in that the controller never
// takes over a tenant's object. An import that would clobber a running VM must
// stop and say so.
func TestTaskRefusesToOverwriteExistingInstance(t *testing.T) {
s := testScheme(t)
src := readySource("vcenter", "tenant-foo")
tk := task("import", "tenant-foo", "vcenter", "replicated",
migrationv1alpha1.VMImportRequest{ID: "vm-1", Name: "web-01"})
occupied := newObject(vmInstanceGVK)
occupied.SetName("web-01")
occupied.SetNamespace("tenant-foo")
c := clientfake.NewClientBuilder().WithScheme(s).
WithObjects(src, tk, occupied, storageClass("replicated", storagev1.VolumeBindingImmediate, false)).
WithStatusSubresource(src, tk).Build()
r := &VMImportTaskReconciler{Client: c, Scheme: s, Recorder: record.NewFakeRecorder(10)}
if _, err := r.Reconcile(context.Background(), reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: "tenant-foo", Name: "import"},
}); err != nil {
t.Fatalf("reconcile: %v", err)
}
got := &migrationv1alpha1.VMImportTask{}
if err := c.Get(context.Background(), types.NamespacedName{Namespace: "tenant-foo", Name: "import"}, got); err != nil {
t.Fatalf("get task: %v", err)
}
if got.Status.Phase != migrationv1alpha1.VMImportTaskPhaseFailed {
t.Errorf("phase = %q, want Failed", got.Status.Phase)
}
if !strings.Contains(got.Status.Message, "already exists") {
t.Errorf("message does not explain the collision: %q", got.Status.Message)
}
}
// TestAggregatePhase covers the fold from per-VM phases to the task's own. The
// property that matters: one failed VM must not stop its siblings, and the task
// only reaches a terminal phase when nothing can still progress.
func TestAggregatePhase(t *testing.T) {
p := func(phases ...migrationv1alpha1.VMImportTaskPhase) []migrationv1alpha1.VMImportStatus {
out := make([]migrationv1alpha1.VMImportStatus, 0, len(phases))
for i, ph := range phases {
out = append(out, migrationv1alpha1.VMImportStatus{ID: string(rune('a' + i)), Phase: ph})
}
return out
}
cases := []struct {
name string
in []migrationv1alpha1.VMImportStatus
want migrationv1alpha1.VMImportTaskPhase
}{
{"empty", nil, migrationv1alpha1.VMImportTaskPhasePending},
{"all succeeded", p(migrationv1alpha1.VMImportTaskPhaseSucceeded, migrationv1alpha1.VMImportTaskPhaseSucceeded), migrationv1alpha1.VMImportTaskPhaseSucceeded},
{"all failed", p(migrationv1alpha1.VMImportTaskPhaseFailed), migrationv1alpha1.VMImportTaskPhaseFailed},
{"mixed terminal is failed", p(migrationv1alpha1.VMImportTaskPhaseSucceeded, migrationv1alpha1.VMImportTaskPhaseFailed), migrationv1alpha1.VMImportTaskPhaseFailed},
{
"one failed while another transfers keeps going",
p(migrationv1alpha1.VMImportTaskPhaseFailed, migrationv1alpha1.VMImportTaskPhaseTransferring),
migrationv1alpha1.VMImportTaskPhaseTransferring,
},
{
"furthest-along non-terminal wins",
p(migrationv1alpha1.VMImportTaskPhaseValidating, migrationv1alpha1.VMImportTaskPhaseCreating),
migrationv1alpha1.VMImportTaskPhaseCreating,
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
if got := aggregatePhase(tc.in); got != tc.want {
t.Errorf("aggregatePhase = %q, want %q", got, tc.want)
}
})
}
}
// TestAdoptVolumeReplicatesVolumeModeAndAccessModes locks in a constraint that
// only a live cluster revealed. Pre-binding through volumeName bypasses the
// volumeMode match check, so a claim that silently defaults to Filesystem
// against a Block volume *binds anyway* and fails much later at attach time
// with a message pointing nowhere near the cause. The replacement claim must
// therefore copy the volume's own modes rather than assume a default.
func TestAdoptVolumeReplicatesVolumeModeAndAccessModes(t *testing.T) {
block := corev1.PersistentVolumeBlock
s := testScheme(t)
tk := task("import", "tenant-foo", "vcenter", "replicated")
pv := &corev1.PersistentVolume{
ObjectMeta: metav1.ObjectMeta{Name: "pv-1"},
Spec: corev1.PersistentVolumeSpec{
AccessModes: []corev1.PersistentVolumeAccessMode{corev1.ReadWriteMany},
VolumeMode: &block,
StorageClassName: "replicated",
PersistentVolumeReclaimPolicy: corev1.PersistentVolumeReclaimDelete,
Capacity: corev1.ResourceList{corev1.ResourceStorage: resource.MustParse("20Gi")},
},
}
transferred := &corev1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{Name: "forklift-claim", Namespace: "tenant-foo"},
Spec: corev1.PersistentVolumeClaimSpec{
VolumeName: "pv-1",
Resources: corev1.VolumeResourceRequirements{
Requests: corev1.ResourceList{corev1.ResourceStorage: resource.MustParse("20Gi")},
},
},
}
c := clientfake.NewClientBuilder().WithScheme(s).WithObjects(pv, transferred, tk).Build()
r := &VMImportTaskReconciler{Client: c, Scheme: s, Recorder: record.NewFakeRecorder(10)}
if err := r.adoptVolume(context.Background(), tk, "vm-1", "forklift-claim", "web-01-disk-0"); err != nil {
t.Fatalf("adoptVolume: %v", err)
}
adopted := &corev1.PersistentVolumeClaim{}
if err := c.Get(context.Background(), types.NamespacedName{
Namespace: "tenant-foo", Name: "vm-disk-web-01-disk-0",
}, adopted); err != nil {
t.Fatalf("replacement claim was not created: %v", err)
}
if adopted.Spec.VolumeMode == nil || *adopted.Spec.VolumeMode != corev1.PersistentVolumeBlock {
t.Errorf("volumeMode = %v, want Block copied from the volume", adopted.Spec.VolumeMode)
}
if len(adopted.Spec.AccessModes) != 1 || adopted.Spec.AccessModes[0] != corev1.ReadWriteMany {
t.Errorf("accessModes = %v, want ReadWriteMany copied from the volume", adopted.Spec.AccessModes)
}
if adopted.Spec.VolumeName != "pv-1" {
t.Errorf("volumeName = %q, want the claim pre-bound to the retained volume", adopted.Spec.VolumeName)
}
if adopted.Annotations[populatedForAnnotation] != "vm-disk-web-01-disk-0" {
t.Errorf("populatedFor = %q, want it to equal the DataVolume name",
adopted.Annotations[populatedForAnnotation])
}
// Retain is not advice during the swap: between deleting the transferred
// claim and re-pointing claimRef the volume is Released, and under Delete
// the provisioner would reclaim it. settleReclaimPolicy lifts it later.
gotPV := &corev1.PersistentVolume{}
if err := c.Get(context.Background(), types.NamespacedName{Name: "pv-1"}, gotPV); err != nil {
t.Fatalf("get pv: %v", err)
}
if gotPV.Spec.PersistentVolumeReclaimPolicy != corev1.PersistentVolumeReclaimRetain {
t.Errorf("reclaim policy = %q, want Retain", gotPV.Spec.PersistentVolumeReclaimPolicy)
}
if gotPV.Spec.ClaimRef == nil || gotPV.Spec.ClaimRef.Name != "vm-disk-web-01-disk-0" {
t.Fatalf("claimRef was not re-pointed: %+v", gotPV.Spec.ClaimRef)
}
if gotPV.Spec.ClaimRef.UID != adopted.UID {
t.Error("claimRef carries no UID, leaving a window where another claim could bind the volume")
}
// The DataVolume the VMDisk release will adopt must exist and must not
// re-fetch anything: a blank source is what makes the handoff free.
dv := newObject(dataVolumeGVK)
if err := c.Get(context.Background(), types.NamespacedName{
Namespace: "tenant-foo", Name: "vm-disk-web-01-disk-0",
}, dv); err != nil {
t.Fatalf("DataVolume was not created: %v", err)
}
if _, found, _ := unstructured.NestedMap(dv.Object, "spec", "source", "blank"); !found {
t.Error("DataVolume source is not blank; the handoff would trigger a second copy")
}
if dv.GetAnnotations()[helmReleaseNameAnnotation] != "vm-disk-web-01-disk-0" {
t.Error("DataVolume lacks Helm ownership metadata, so the VMDisk release would collide with it")
}
// And the tenant-facing object, with no owner reference to the task: that
// is what lets a finished task be deleted without losing the disk.
disk := newObject(vmDiskGVK)
if err := c.Get(context.Background(), types.NamespacedName{
Namespace: "tenant-foo", Name: "web-01-disk-0",
}, disk); err != nil {
t.Fatalf("VMDisk was not created: %v", err)
}
if len(disk.GetOwnerReferences()) != 0 {
t.Error("VMDisk carries an owner reference; deleting the task would destroy the imported disk")
}
}
// TestAdoptVolumeRemovesOwningDataVolumeFirst locks in the second live-only
// finding. Deleting a claim while its DataVolume still exists makes CDI
// recreate the claim and provision a second volume, re-running the whole
// import — the exact duplicate copy this design exists to remove.
func TestAdoptVolumeRemovesOwningDataVolumeFirst(t *testing.T) {
s := testScheme(t)
tk := task("import", "tenant-foo", "vcenter", "replicated")
pv := &corev1.PersistentVolume{
ObjectMeta: metav1.ObjectMeta{Name: "pv-1"},
Spec: corev1.PersistentVolumeSpec{
StorageClassName: "replicated",
Capacity: corev1.ResourceList{corev1.ResourceStorage: resource.MustParse("10Gi")},
},
}
// The transferred claim as CDI leaves it: owned by its DataVolume.
controller := true
transferred := &corev1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Name: "forklift-claim",
Namespace: "tenant-foo",
OwnerReferences: []metav1.OwnerReference{{
APIVersion: "cdi.kubevirt.io/v1beta1",
Kind: "DataVolume",
Name: "forklift-claim",
UID: "dv-uid",
Controller: &controller,
}},
},
Spec: corev1.PersistentVolumeClaimSpec{
VolumeName: "pv-1",
Resources: corev1.VolumeResourceRequirements{
Requests: corev1.ResourceList{corev1.ResourceStorage: resource.MustParse("10Gi")},
},
},
}
forkliftDV := newObject(dataVolumeGVK)
forkliftDV.SetName("forklift-claim")
forkliftDV.SetNamespace("tenant-foo")
c := clientfake.NewClientBuilder().WithScheme(s).
WithObjects(pv, transferred, forkliftDV, tk).Build()
r := &VMImportTaskReconciler{Client: c, Scheme: s, Recorder: record.NewFakeRecorder(10)}
if err := r.adoptVolume(context.Background(), tk, "vm-1", "forklift-claim", "web-01-disk-0"); err != nil {
t.Fatalf("adoptVolume: %v", err)
}
gone := newObject(dataVolumeGVK)
err := c.Get(context.Background(), types.NamespacedName{Namespace: "tenant-foo", Name: "forklift-claim"}, gone)
if err == nil {
t.Fatal("the transferring DataVolume still exists; CDI would recreate the claim and run a second full copy")
}
oldClaim := &corev1.PersistentVolumeClaim{}
if err := c.Get(context.Background(), types.NamespacedName{
Namespace: "tenant-foo", Name: "forklift-claim",
}, oldClaim); err == nil {
t.Error("the original claim was left behind")
}
}
// TestAdoptVolumeIsIdempotent asserts a second pass over an already-adopted
// disk is a no-op rather than a second swap: the controller is re-entered on
// every requeue and after every restart.
func TestAdoptVolumeIsIdempotent(t *testing.T) {
s := testScheme(t)
tk := task("import", "tenant-foo", "vcenter", "replicated")
already := &corev1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Name: "vm-disk-web-01-disk-0",
Namespace: "tenant-foo",
// The markers are what make this a resume rather than a collision:
// an unmarked claim of the same name belongs to the tenant.
Labels: map[string]string{
migrationv1alpha1.OutputTaskUIDLabel: "import-uid",
migrationv1alpha1.OutputVMIDLabel: "vm-1",
},
},
Spec: corev1.PersistentVolumeClaimSpec{
VolumeName: "pv-1",
Resources: corev1.VolumeResourceRequirements{
Requests: corev1.ResourceList{corev1.ResourceStorage: resource.MustParse("20Gi")},
},
},
}
c := clientfake.NewClientBuilder().WithScheme(s).WithObjects(already, tk).Build()
r := &VMImportTaskReconciler{Client: c, Scheme: s, Recorder: record.NewFakeRecorder(10)}
// The source claim is deliberately absent: a second pass must not need it.
if err := r.adoptVolume(context.Background(), tk, "vm-1", "forklift-claim", "web-01-disk-0"); err != nil {
t.Fatalf("second pass over an adopted disk failed: %v", err)
}
disk := newObject(vmDiskGVK)
if err := c.Get(context.Background(), types.NamespacedName{
Namespace: "tenant-foo", Name: "web-01-disk-0",
}, disk); err != nil {
t.Fatalf("VMDisk was not created on the second pass: %v", err)
}
}
// TestSourceVMResources asserts the source VM's shape is read off the object
// Forklift already built, which is why no inventory client is needed.
func TestSourceVMResources(t *testing.T) {
vm := newObject(virtualMachineGVK)
if err := unstructured.SetNestedMap(vm.Object, map[string]any{
"cores": int64(2),
"sockets": int64(2),
}, "spec", "template", "spec", "domain", "cpu"); err != nil {
t.Fatalf("set cpu: %v", err)
}
if err := unstructured.SetNestedField(vm.Object, "8Gi",
"spec", "template", "spec", "domain", "memory", "guest"); err != nil {
t.Fatalf("set memory: %v", err)
}
// Cores and sockets stay separate. vm-instance maps them to
// domain.cpu.cores and domain.cpu.sockets and refuses to render unless both
// are present, so multiplying them into one number both loses the topology
// and fails the chart — observed live: "Either instanceType or resources
// (cpu, sockets, memory) must be specified".
cores, sockets, memory := sourceVMResources(vm)
if cores != "2" {
t.Errorf("cores = %q, want 2", cores)
}
if sockets != "2" {
t.Errorf("sockets = %q, want 2", sockets)
}
if memory != "8Gi" {
t.Errorf("memory = %q, want 8Gi", memory)
}
}
// A source that reports cores but no socket count is single-socket. Saying so
// explicitly is what lets the chart render at all.
func TestSourceVMResourcesDefaultsSingleSocket(t *testing.T) {
vm := newObject(virtualMachineGVK)
if err := unstructured.SetNestedField(vm.Object, int64(4),
"spec", "template", "spec", "domain", "cpu", "cores"); err != nil {
t.Fatalf("set cores: %v", err)
}
cores, sockets, _ := sourceVMResources(vm)
if cores != "4" || sockets != "1" {
t.Errorf("cores/sockets = %q/%q, want 4/1", cores, sockets)
}
}
// TestMigrationProgressReadsTheTransferStep asserts progress reflects the disk
// transfer rather than an average over every pipeline step, which would report
// a four-step pipeline as 25% done before any data moved.
func TestMigrationProgressReadsTheTransferStep(t *testing.T) {
m := newObject(migrationGVK)
if err := unstructured.SetNestedSlice(m.Object, []any{
map[string]any{
"id": "vm-1",
"pipeline": []any{
map[string]any{
"name": "Initialize",
"progress": map[string]any{"completed": int64(1), "total": int64(1)},
},
map[string]any{
"name": "DiskTransfer",
"progress": map[string]any{"completed": int64(3), "total": int64(10)},
},
},
},
}, "status", "vms"); err != nil {
t.Fatalf("set vms: %v", err)
}
done, progress, failure := migrationProgress(m)
if done {
t.Error("migration reported done with no completion timestamp")
}
if failure != "" {
t.Errorf("unexpected failure: %q", failure)
}
if progress != 30 {
t.Errorf("progress = %d, want 30", progress)
}
}
// TestMigrationProgressFallsBackWhenStepsAreUnnamed guards against pegging
// progress at zero for a whole transfer. The pipeline step names are not part
// of any contract, and a release that renames them would silently break a
// name-only match — so an unrecognised pipeline still has to report something.
func TestMigrationProgressFallsBackWhenStepsAreUnnamed(t *testing.T) {
m := newObject(migrationGVK)
if err := unstructured.SetNestedSlice(m.Object, []any{
map[string]any{
"id": "vm-1",
"pipeline": []any{
map[string]any{
"name": "SomeFutureStepName",
"progress": map[string]any{"completed": int64(4096), "total": int64(16384)},
},
},
},
}, "status", "vms"); err != nil {
t.Fatalf("set vms: %v", err)
}
_, progress, _ := migrationProgress(m)
if progress != 25 {
t.Errorf("progress = %d, want 25 from the unnamed step", progress)
}
}
// TestMigrationProgressAcceptsCompletedPhase asserts the phase the engine
// settles on is treated as terminal. Waiting only on a completion timestamp
// would leave a finished transfer stuck in Transferring.
func TestMigrationProgressAcceptsCompletedPhase(t *testing.T) {
m := newObject(migrationGVK)
if err := unstructured.SetNestedSlice(m.Object, []any{
map[string]any{"id": "vm-1", "phase": "Completed"},
}, "status", "vms"); err != nil {
t.Fatalf("set vms: %v", err)
}
done, progress, failure := migrationProgress(m)
if !done {
t.Error("a Completed phase must count as done")
}
if progress != 100 || failure != "" {
t.Errorf("progress = %d, failure = %q", progress, failure)
}
}
// TestAdoptVolumeOrphansAClaimWithNoDataVolume is the populator path, which is
// the path Forklift actually takes: the transferred disk arrives as a populated
// claim with no DataVolume, carrying an ownerReference to the VirtualMachine
// Forklift built. Anything that later deletes that VM takes the disk with it,
// and garbage collection wins any race against a copy — so the reference has to
// be cleared even though there is no DataVolume to trigger the cleanup.
func TestAdoptVolumeOrphansAClaimWithNoDataVolume(t *testing.T) {
s := testScheme(t)
tk := task("import", "tenant-foo", "vcenter", "replicated")
pv := &corev1.PersistentVolume{
ObjectMeta: metav1.ObjectMeta{Name: "pv-1"},
Spec: corev1.PersistentVolumeSpec{
StorageClassName: "replicated",
Capacity: corev1.ResourceList{corev1.ResourceStorage: resource.MustParse("16Gi")},
},
}
blockOwnerDeletion := true
notController := false
populated := &corev1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
// Forklift's own naming: <plan>-vm-<id>-<random>.
Name: "import-vm-1234-pdbdm",
Namespace: "tenant-foo",
OwnerReferences: []metav1.OwnerReference{{
APIVersion: "kubevirt.io/v1",
Kind: "VirtualMachine",
Name: "test-vm",
UID: "vm-uid",
Controller: &notController,
BlockOwnerDeletion: &blockOwnerDeletion,
}},
},
Spec: corev1.PersistentVolumeClaimSpec{
VolumeName: "pv-1",
Resources: corev1.VolumeResourceRequirements{
Requests: corev1.ResourceList{corev1.ResourceStorage: resource.MustParse("16Gi")},
},
},
}
// Deliberately no DataVolume: that is what the populator path looks like.
c := clientfake.NewClientBuilder().WithScheme(s).WithObjects(pv, populated, tk).Build()
r := &VMImportTaskReconciler{Client: c, Scheme: s, Recorder: record.NewFakeRecorder(10)}
if err := r.adoptVolume(context.Background(), tk, "vm-1", "import-vm-1234-pdbdm", "web-01-disk-0"); err != nil {
t.Fatalf("adoptVolume on the populator path: %v", err)
}
adopted := &corev1.PersistentVolumeClaim{}
if err := c.Get(context.Background(), types.NamespacedName{
Namespace: "tenant-foo", Name: "vm-disk-web-01-disk-0",
}, adopted); err != nil {
t.Fatalf("replacement claim was not created: %v", err)
}
if len(adopted.OwnerReferences) != 0 {
t.Error("the adopted claim carries an owner reference; deleting the Forklift VM would destroy the disk")
}
gotPV := &corev1.PersistentVolume{}
if err := c.Get(context.Background(), types.NamespacedName{Name: "pv-1"}, gotPV); err != nil {
t.Fatalf("get pv: %v", err)
}
if gotPV.Spec.PersistentVolumeReclaimPolicy != corev1.PersistentVolumeReclaimRetain {
t.Error("the volume was not retained for the swap")
}
}
// TestMigrationProgressSurfacesForkliftErrors asserts a failed transfer carries
// Forklift's own words rather than a paraphrase.
func TestMigrationProgressSurfacesForkliftErrors(t *testing.T) {
m := newObject(migrationGVK)
if err := unstructured.SetNestedSlice(m.Object, []any{
map[string]any{
"id": "vm-1",
"error": map[string]any{
"reasons": []any{"vddk: failed to open disk"},
},
},
}, "status", "vms"); err != nil {
t.Fatalf("set vms: %v", err)
}
done, _, failure := migrationProgress(m)
if done {
t.Error("a failed migration must not report done")
}
if !strings.Contains(failure, "failed to open disk") {
t.Errorf("failure = %q, want Forklift's own reason", failure)
}
}
// The topology comes out of an inventory record, and this is the shape the
// inventory actually returns — verified against a live Forklift v2.11.5
// inventory service on 2026-08-31, where a vSphere VM reported
// networks:[{kind:Network,id:"HaNetwork-..."}] and
// disks:[{...,datastore:{kind:Datastore,id:"61f94272-..."}}].
//
// An earlier revision read these from the Plan's VMNetworksNotMapped /
// VMStorageNotMapped conditions instead. That cannot work: those conditions
// carry the VM's own reference, not the network or datastore, so the map was
// built from parsed VM names and never resolved. The test that covered it
// asserted bare IDs Forklift never emits and so locked in the wrong contract.
func TestInventoryVMYieldsNetworksAndDatastores(t *testing.T) {
const record = `{
"id": "86",
"name": "sample-vm",
"firmware": "efi",
"powerState": "poweredOff",
"networks": [{"kind": "Network", "id": "HaNetwork-Cluster Example"}],
"disks": [
{"datastore": {"kind": "Datastore", "id": "61f94272-8f3a3584-b45f-00005e005301"}},
{"datastore": {"kind": "Datastore", "id": "61f94272-8f3a3584-b45f-00005e005301"}}
]
}`
vm := &inventoryVM{}
if err := json.Unmarshal([]byte(record), vm); err != nil {
t.Fatalf("decoding the inventory record: %v", err)
}
if vm.Firmware != "efi" || vm.PowerState != "poweredOff" {
t.Errorf("firmware/power = %q/%q", vm.Firmware, vm.PowerState)
}
if got := vm.networkIDs(); len(got) != 1 || got[0] != "HaNetwork-Cluster Example" {
t.Errorf("networkIDs = %v", got)
}
// Two disks on one datastore yield one map entry, not two.
if got := vm.datastoreIDs(); len(got) != 1 || got[0] != "61f94272-8f3a3584-b45f-00005e005301" {
t.Errorf("datastoreIDs = %v", got)
}
}
// A Plan that does not pin the target power state lets Forklift match the
// source's: a production cutover migrates running machines, so the imported VM
// would boot the instant the transfer finished — a duplicate of a live machine
// on the network, started on the very volume the handoff is about to re-point.
func TestPlanKeepsTheImportedVMPoweredOff(t *testing.T) {
s := testScheme(t)
tk := task("import", "tenant-foo", "vcenter", "replicated")
src := readySource("vcenter", "tenant-foo")
c := clientfake.NewClientBuilder().WithScheme(s).WithObjects(tk, src).Build()
r := &VMImportTaskReconciler{Client: c, Scheme: s, Recorder: record.NewFakeRecorder(10)}
req := &migrationv1alpha1.VMImportRequest{ID: "vm-1234"}
if err := r.createPlan(context.Background(), tk, src, req); err != nil {
t.Fatalf("createPlan: %v", err)
}
plan := newObject(planGVK)
if err := c.Get(context.Background(), types.NamespacedName{
Namespace: "tenant-foo", Name: planName("import", "vm-1234"),
}, plan); err != nil {
t.Fatalf("Plan was not created: %v", err)
}
vms, _, _ := unstructured.NestedSlice(plan.Object, "spec", "vms")
if len(vms) != 1 {
t.Fatalf("spec.vms = %d entries, want 1", len(vms))
}
entry, _ := vms[0].(map[string]any)
got, _, _ := unstructured.NestedString(entry, "targetPowerState")
if got != "off" {
t.Errorf("targetPowerState = %q, want \"off\" — Forklift would boot the migrated guest before the handoff", got)
}
}
// The source's boot mode has to reach the VMInstance. A UEFI guest rendered
// with a BIOS bootloader imports "successfully" and then does not boot, which
// is the whole reason the vm-instance firmware field is a merge-order
// dependency of this controller.
func TestSourceFirmwareIsCarriedAcross(t *testing.T) {
cases := []struct {
name string
set func(vm *unstructured.Unstructured)
want map[string]any
}{
{"efi", func(vm *unstructured.Unstructured) {
_ = unstructured.SetNestedMap(vm.Object, map[string]any{},
"spec", "template", "spec", "domain", "firmware", "bootloader", "efi")
}, map[string]any{"bootloader": "uefi"}},
{"efi with secureBoot", func(vm *unstructured.Unstructured) {
_ = unstructured.SetNestedField(vm.Object, true,
"spec", "template", "spec", "domain", "firmware", "bootloader", "efi", "secureBoot")
}, map[string]any{"bootloader": "uefi", "secureBoot": true}},
{"bios", func(vm *unstructured.Unstructured) {
_ = unstructured.SetNestedMap(vm.Object, map[string]any{},
"spec", "template", "spec", "domain", "firmware", "bootloader", "bios")
}, map[string]any{"bootloader": "bios"}},
// Nothing on the source means nothing written: the instance profile's
// own default stands rather than being overridden with a guess.
{"no firmware at all", func(vm *unstructured.Unstructured) {}, nil},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
vm := newObject(virtualMachineGVK)
tc.set(vm)
got := sourceFirmware(vm)
if !reflect.DeepEqual(got, tc.want) {
t.Errorf("sourceFirmware = %v, want %v", got, tc.want)
}
})
}
}
// The boot disk of a guest copied verbatim out of VMware has to sit on a bus
// that guest can already read, or the import "succeeds" into a VM that stalls
// in its initramfs with no root device.
func TestImportedDisksPreferTheInboxBus(t *testing.T) {
for i := range ahciPorts {
if got := importedDiskBus(i); got != "sata" {
t.Errorf("disk %d: bus = %q, want sata", i, got)
}
}
// Past the controller's port count SATA has nowhere to put the disk, and a
// data disk may fall back: by then the guest is running off its own root
// and can load the driver itself.
if got := importedDiskBus(ahciPorts); got != "virtio" {
t.Errorf("disk %d: bus = %q, want virtio", ahciPorts, got)
}
}
// The failure this marker exists to stop: a first import half-succeeds, the
// tenant deletes the VMInstance but keeps the disks, and retries under the same
// name. Without an identity check the leftover claim reads as this task's own
// finished work, adoptVolume early-returns, and the freshly transferred claim
// keeps its ownerRef to the scaffolding VM — which fulfill then deletes, taking
// the new transfer with it. The import reports success over stale data.
func TestAdoptVolumeRefusesAClaimItDidNotCreate(t *testing.T) {
s := testScheme(t)
tk := task("import", "tenant-foo", "vcenter", "replicated")
foreign := &corev1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{Name: "vm-disk-web-01-disk-0", Namespace: "tenant-foo"},
Spec: corev1.PersistentVolumeClaimSpec{
VolumeName: "pv-1",
Resources: corev1.VolumeResourceRequirements{
Requests: corev1.ResourceList{corev1.ResourceStorage: resource.MustParse("20Gi")},
},
},
}
c := clientfake.NewClientBuilder().WithScheme(s).WithObjects(foreign, tk).Build()
r := &VMImportTaskReconciler{Client: c, Scheme: s, Recorder: record.NewFakeRecorder(10)}
err := r.adoptVolume(context.Background(), tk, "vm-1", "forklift-claim", "web-01-disk-0")
if err == nil {
t.Fatal("adoptVolume accepted a claim this task never created; the transfer would be garbage-collected")
}
var verr *validationError
if !asValidationError(err, &verr) {
t.Fatalf("error = %v, want a tenant-visible validationError", err)
}
if verr.reason != migrationv1alpha1.ReasonOutputExists {
t.Errorf("reason = %q, want %q", verr.reason, migrationv1alpha1.ReasonOutputExists)
}
}
// A claim carrying another task's marker is just as foreign as an unmarked one:
// names are reused, so the UID is what decides.
func TestAdoptVolumeRefusesAnotherTasksOutput(t *testing.T) {
s := testScheme(t)
tk := task("import", "tenant-foo", "vcenter", "replicated")
other := &corev1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Name: "vm-disk-web-01-disk-0",
Namespace: "tenant-foo",
Labels: map[string]string{
migrationv1alpha1.OutputTaskUIDLabel: "some-older-task-uid",
migrationv1alpha1.OutputVMIDLabel: "vm-1",
},
},
Spec: corev1.PersistentVolumeClaimSpec{VolumeName: "pv-1"},
}
c := clientfake.NewClientBuilder().WithScheme(s).WithObjects(other, tk).Build()
r := &VMImportTaskReconciler{Client: c, Scheme: s, Recorder: record.NewFakeRecorder(10)}
if err := r.adoptVolume(context.Background(), tk, "vm-1", "forklift-claim", "web-01-disk-0"); err == nil {
t.Fatal("adoptVolume adopted an output belonging to a different task")
}
}
// A deleted task must take the engine's transfer volumes with it. They carry no
// owner reference — the engine tracks them by label — so deleting the task
// removes the Plan and leaves the volume behind, still retrying the transfer
// against the source with nothing left to collect it. Observed live: 37 minutes
// and 8 restarts after its task was gone.
//
// The outputs must survive the same sweep, which is the other half of the test:
// they are what the tenant keeps.
func TestFinalizerRemovesTransferVolumesButNotOutputs(t *testing.T) {
s := testScheme(t)
tk := task("import", "tenant-foo", "vcenter", "replicated",
migrationv1alpha1.VMImportRequest{ID: "vm-1", Name: "web-01"})
tk.Finalizers = []string{migrationv1alpha1.TaskFinalizer}
now := metav1.Now()
tk.DeletionTimestamp = &now
plan := newObject(planGVK)
plan.SetName(planName("import", "vm-1"))
plan.SetNamespace("tenant-foo")
plan.SetUID(types.UID("plan-uid-1"))
transfer := newObject(dataVolumeGVK)
transfer.SetName("import-vm-1-abcde")
transfer.SetNamespace("tenant-foo")
transfer.SetLabels(map[string]string{"plan": "plan-uid-1", "vmID": "vm-1"})
// The imported disk: created by this controller, carries Helm metadata and
// no engine label, and must be left alone.
output := newObject(dataVolumeGVK)
output.SetName("vm-disk-web-01-disk-0")
output.SetNamespace("tenant-foo")
output.SetLabels(map[string]string{"app.kubernetes.io/instance": "vm-disk-web-01-disk-0"})
c := clientfake.NewClientBuilder().WithScheme(s).
WithObjects(tk, plan, transfer, output).Build()
r := &VMImportTaskReconciler{Client: c, Scheme: s, Recorder: record.NewFakeRecorder(10)}
if _, err := r.finalize(context.Background(), tk); err != nil {
t.Fatalf("finalize: %v", err)
}
got := newObject(dataVolumeGVK)
err := c.Get(context.Background(), types.NamespacedName{
Namespace: "tenant-foo", Name: "import-vm-1-abcde",
}, got)
if !apierrors.IsNotFound(err) {
t.Errorf("the engine's transfer volume survived the task, got err %v — it would keep retrying forever", err)
}
if err := c.Get(context.Background(), types.NamespacedName{
Namespace: "tenant-foo", Name: "vm-disk-web-01-disk-0",
}, newObject(dataVolumeGVK)); err != nil {
t.Errorf("the imported disk was removed with the task: %v", err)
}
if controllerutil.ContainsFinalizer(tk, migrationv1alpha1.TaskFinalizer) {
t.Error("the finalizer was not released; the task would never delete")
}
}
// Outputs must carry the marker, or the checks above can never recognise them
// on a later pass and every resume becomes a collision.
func TestAdoptedOutputsCarryTheTaskMarker(t *testing.T) {
tk := task("import", "tenant-foo", "vcenter", "replicated")
obj := newObject(vmDiskGVK)
stampOutput(obj, tk, "vm-1")
if !isOwnOutput(obj, tk, "vm-1") {
t.Error("a freshly stamped output is not recognised as this task's own")
}
if isOwnOutput(obj, tk, "vm-2") {
t.Error("an output for one source VM is recognised as another's")
}
other := task("import", "tenant-foo", "vcenter", "replicated")
other.UID = "different-uid"
if isOwnOutput(obj, other, "vm-1") {
t.Error("an output is recognised as belonging to a different task")
}
}