From d2550a116ca23cbfe76a7c3e7f5e302d3439d76d Mon Sep 17 00:00:00 2001 From: Daniil Antoshin Date: Tue, 14 Jul 2026 16:06:18 +0200 Subject: [PATCH 01/21] fix(vm): keep the volume migration set atomic and serialized MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live volume migration of a multi-disk VM built the migration volume set non-atomically, so KubeVirt could be handed an inconsistent set: some disks on this round's target while others still point at a previous round's PVC, or a local disk left out of the set entirely. KubeVirt then rejects the transition ("the volume can only be reverted to the previous version during the update") and the migration hangs, or completes a migration that never copied a local disk, leaving the domain broken on the target node — reported as success but requiring a restart. SyncVolumes now: - waits until all ReadWriteOnce local disks migrate together, never patching a set that mixes disks from different rounds (completeness); - refuses to start a new migration while a volume migration is already in progress (VMI condition VolumesChange=True) that targets different destinations, since KubeVirt only accepts continuing the current round or a clean revert to source (serialization). Signed-off-by: Daniil Antoshin --- .../vm/internal/service/migration_volumes.go | 67 +++++++++++++++ .../service/migration_volumes_test.go | 86 +++++++++++++++++++ 2 files changed, 153 insertions(+) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go index 513ab3dbda..67bf571859 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go @@ -177,6 +177,32 @@ func (s MigrationVolumesService) SyncVolumes(ctx context.Context, vmState state. } if migrationRequested { + // Completeness: all ReadWriteOnce local disks must migrate together in a + // single round. If only some are migrating, the built set mixes this round's + // targets with a previous round's PVCs, which KubeVirt rejects ("the volume + // can only be reverted to the previous version during the update") or, worse, + // leaves an RWO volume out of the migration and breaks the domain on the + // target node. Wait until the whole set is migrating before patching. + // ponytail: this only blocks the bad patch; preparing all disks together is + // the vd-controller's job, so if a round never completes we keep waiting here. + if !allDisksMigrating(readWriteOnceDisks) { + log.Info("not all ReadWriteOnce disks are migrating in this round yet, wait for a complete volume set.") + return reconcile.Result{RequeueAfter: 10 * time.Second}, nil + } + + // Serialization: do not start a new migration round while a volume migration + // is already in progress (VMI condition VolumesChange=True) that targets + // different destinations. KubeVirt only accepts continuing to the current + // destinations or a clean revert to the source during an update; a new, + // different set is rejected ("the volume can only be reverted to the previous + // version during the update"). Revert-to-source is handled by the builtKVVM + // branch above, so here we only allow continuing the same round; otherwise + // wait for the in-flight migration to finalize. + if isVolumeMigrating(kvvmiInCluster) && !destinationsMatch(kvvmiInCluster, builtKVVMWithMigrationVolumes) { + log.Info("a volume migration is already in progress with different targets, wait for it to finalize.") + return reconcile.Result{RequeueAfter: 5 * time.Second}, nil + } + // We should wait delayDuration seconds. This delay allows user to change storage class on other volumes if len(storageClassChangedDisks) > 0 { delay, exists := s.delay[vm.UID] @@ -554,6 +580,47 @@ func (s MigrationVolumesService) makeKVVMFromVirtualMachineSpec(ctx context.Cont return kvvm, kvvmWithMigrationVolumes, nil } +// allDisksMigrating reports whether every disk in the set is migrating in the +// current round (MigrationState started and not ended). Used to avoid patching a +// volume set that mixes disks from different migration rounds. +func allDisksMigrating(disks map[string]*v1alpha2.VirtualDisk) bool { + for _, d := range disks { + if !commonvd.IsMigrating(d) { + return false + } + } + return true +} + +// isVolumeMigrating reports whether KubeVirt is currently running a volume +// migration for the VMI (condition VolumesChange=True). +func isVolumeMigrating(kvvmi *virtv1.VirtualMachineInstance) bool { + cond, _ := conditions.GetKVVMICondition(virtv1.VirtualMachineInstanceVolumesChange, kvvmi.Status.Conditions) + return cond.Status == corev1.ConditionTrue +} + +// destinationsMatch reports whether the volume update KubeVirt currently records +// (kvvmi.status.migratedVolumes) targets the same destinations as the set we are +// about to patch. If it does, patching continues the same update; if not, a new, +// conflicting update would be issued over an in-flight one. +func destinationsMatch(kvvmi *virtv1.VirtualMachineInstance, built *virtv1.VirtualMachine) bool { + want := make(map[string]string, len(built.Spec.Template.Spec.Volumes)) + for _, v := range built.Spec.Template.Spec.Volumes { + if v.PersistentVolumeClaim != nil { + want[v.Name] = v.PersistentVolumeClaim.ClaimName + } + } + for _, mv := range kvvmi.Status.MigratedVolumes { + if mv.DestinationPVCInfo == nil { + continue + } + if want[mv.VolumeName] != mv.DestinationPVCInfo.ClaimName { + return false + } + } + return true +} + // areDisksSynced checks whether all disks are synchronized with their corresponding PVCs in kvvm // All TargetPVCs on disks must be present in kvvm func (s MigrationVolumesService) areDisksSynced(kvvm *virtv1.VirtualMachine, disks map[string]*v1alpha2.VirtualDisk) bool { diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go index 496a52f724..50d6a53a60 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go @@ -302,3 +302,89 @@ var _ = Describe("isStructuralVolumeChange", func() { Entry("disk renamed (same count, different name)", map[string]string{"root": "a"}, map[string]string{"data": "a"}, true), ) }) + +var _ = Describe("allDisksMigrating", func() { + mk := func(started, ended bool) *v1alpha2.VirtualDisk { + vd := &v1alpha2.VirtualDisk{} + if started { + vd.Status.MigrationState.StartTimestamp = metav1.Now() + } + if ended { + vd.Status.MigrationState.EndTimestamp = metav1.Now() + } + return vd + } + + It("is true for an empty set", func() { + Expect(allDisksMigrating(map[string]*v1alpha2.VirtualDisk{})).To(BeTrue()) + }) + It("is true when every disk is migrating this round", func() { + Expect(allDisksMigrating(map[string]*v1alpha2.VirtualDisk{"a": mk(true, false), "b": mk(true, false)})).To(BeTrue()) + }) + It("is false when a disk has not started migrating", func() { + Expect(allDisksMigrating(map[string]*v1alpha2.VirtualDisk{"a": mk(true, false), "b": mk(false, false)})).To(BeFalse()) + }) + It("is false when a disk already completed a previous round", func() { + Expect(allDisksMigrating(map[string]*v1alpha2.VirtualDisk{"a": mk(true, false), "b": mk(true, true)})).To(BeFalse()) + }) +}) + +var _ = Describe("isVolumeMigrating", func() { + withVolumesChange := func(status corev1.ConditionStatus, set bool) *virtv1.VirtualMachineInstance { + vmi := &virtv1.VirtualMachineInstance{} + if set { + vmi.Status.Conditions = []virtv1.VirtualMachineInstanceCondition{ + {Type: virtv1.VirtualMachineInstanceVolumesChange, Status: status}, + } + } + return vmi + } + + It("is true when VolumesChange condition is True", func() { + Expect(isVolumeMigrating(withVolumesChange(corev1.ConditionTrue, true))).To(BeTrue()) + }) + It("is false when VolumesChange condition is False", func() { + Expect(isVolumeMigrating(withVolumesChange(corev1.ConditionFalse, true))).To(BeFalse()) + }) + It("is false when VolumesChange condition is absent", func() { + Expect(isVolumeMigrating(withVolumesChange(corev1.ConditionTrue, false))).To(BeFalse()) + }) +}) + +var _ = Describe("destinationsMatch", func() { + built := func(nameToClaim map[string]string) *virtv1.VirtualMachine { + vols := make([]virtv1.Volume, 0, len(nameToClaim)) + for name, claim := range nameToClaim { + vols = append(vols, virtv1.Volume{ + Name: name, + VolumeSource: virtv1.VolumeSource{PersistentVolumeClaim: &virtv1.PersistentVolumeClaimVolumeSource{PersistentVolumeClaimVolumeSource: corev1.PersistentVolumeClaimVolumeSource{ClaimName: claim}}}, + }) + } + return &virtv1.VirtualMachine{Spec: virtv1.VirtualMachineSpec{Template: &virtv1.VirtualMachineInstanceTemplateSpec{Spec: virtv1.VirtualMachineInstanceSpec{Volumes: vols}}}} + } + kvvmi := func(volToDest map[string]string) *virtv1.VirtualMachineInstance { + vmi := &virtv1.VirtualMachineInstance{} + for vol, dest := range volToDest { + vmi.Status.MigratedVolumes = append(vmi.Status.MigratedVolumes, virtv1.StorageMigratedVolumeInfo{ + VolumeName: vol, + DestinationPVCInfo: &virtv1.PersistentVolumeClaimInfo{ClaimName: dest}, + }) + } + return vmi + } + + It("is true when there is no recorded migration", func() { + Expect(destinationsMatch(kvvmi(nil), built(map[string]string{"root": "new"}))).To(BeTrue()) + }) + It("is true when the recorded destination matches the target being patched", func() { + Expect(destinationsMatch(kvvmi(map[string]string{"root": "tgt"}), built(map[string]string{"root": "tgt"}))).To(BeTrue()) + }) + It("is false when the recorded destination differs from the new target", func() { + Expect(destinationsMatch(kvvmi(map[string]string{"root": "old-tgt"}), built(map[string]string{"root": "new-tgt"}))).To(BeFalse()) + }) + It("ignores recorded entries without destination info", func() { + vmi := &virtv1.VirtualMachineInstance{} + vmi.Status.MigratedVolumes = []virtv1.StorageMigratedVolumeInfo{{VolumeName: "root", DestinationPVCInfo: nil}} + Expect(destinationsMatch(vmi, built(map[string]string{"root": "whatever"}))).To(BeTrue()) + }) +}) From d50df707bb386aaa110ec818462486e4ae631f67 Mon Sep 17 00:00:00 2001 From: Daniil Antoshin Date: Tue, 14 Jul 2026 16:06:18 +0200 Subject: [PATCH 02/21] test(e2e): migrate a multi-disk VM repeatedly and keep the volume set consistent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add an e2e case to RWOVirtualDiskMigration that migrates a VM with a root disk plus three ReadWriteOnce local disks several times in a row, asserting each migration succeeds, all disks stay Ready on their targets, and the VM is never left restart-required — the symptom of a non-atomic volume set. Signed-off-by: Daniil Antoshin --- test/e2e/vm/util.go | 18 ++++++++ test/e2e/vm/volume_migration_local_disks.go | 49 +++++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/test/e2e/vm/util.go b/test/e2e/vm/util.go index ccd9613527..6d37b6222a 100644 --- a/test/e2e/vm/util.go +++ b/test/e2e/vm/util.go @@ -105,6 +105,24 @@ func rootAndAdditionalBuild(f *framework.Framework, vi *v1alpha2.VirtualImage, r return vm, vds } +// rootAndManyAdditionalBuild builds a VM with a root disk plus count additional +// ReadWriteOnce disks on the given storage class — a multi-disk VM whose block +// migration must move the whole volume set atomically. +func rootAndManyAdditionalBuild(f *framework.Framework, vi *v1alpha2.VirtualImage, root buildOption, storageClass *string, count int) (*v1alpha2.VirtualMachine, []*v1alpha2.VirtualDisk) { + refs := []v1alpha2.BlockDeviceSpecRef{{Kind: v1alpha2.VirtualDiskKind, Name: root.name}} + vds := []*v1alpha2.VirtualDisk{newRootVD(f, root, vi)} + for i := range count { + name := fmt.Sprintf("vd-alpine-additional-disk-%d", i) + refs = append(refs, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.VirtualDiskKind, Name: name}) + vds = append(vds, newBlankVD(f, buildOption{name: name, storageClass: storageClass, rwo: true})) + } + vm := object.NewMinimalVM("volume-migration-many-disks-", f.Namespace().Name, + vmbuilder.WithBlockDeviceRefs(refs...), + vmbuilder.WithCPU(1, ptr.To("100%")), + ) + return vm, vds +} + func onlyAdditionalBuild(f *framework.Framework, vi *v1alpha2.VirtualImage, root, additional buildOption) (*v1alpha2.VirtualMachine, []*v1alpha2.VirtualDisk) { vm := object.NewMinimalVM( "volume-migration-only-additional-disk-", diff --git a/test/e2e/vm/volume_migration_local_disks.go b/test/e2e/vm/volume_migration_local_disks.go index fb931fec7a..0dc6b9c6f0 100644 --- a/test/e2e/vm/volume_migration_local_disks.go +++ b/test/e2e/vm/volume_migration_local_disks.go @@ -39,6 +39,7 @@ import ( "github.com/deckhouse/virtualization-controller/pkg/common/patch" "github.com/deckhouse/virtualization-controller/pkg/controller/conditions" "github.com/deckhouse/virtualization/api/core/v1alpha2" + "github.com/deckhouse/virtualization/api/core/v1alpha2/vmcondition" "github.com/deckhouse/virtualization/test/e2e/internal/framework" "github.com/deckhouse/virtualization/test/e2e/internal/object" "github.com/deckhouse/virtualization/test/e2e/internal/precheck" @@ -104,6 +105,10 @@ var _ = Describe("RWOVirtualDiskMigration", decoratorsForVolumeMigrations(), Lab ) } + localMigrationManyDisksBuild := func() (*v1alpha2.VirtualMachine, []*v1alpha2.VirtualDisk) { + return rootAndManyAdditionalBuild(f, vi, buildOption{name: vdRootName, storageClass: &storageClass.Name, rwo: true}, &storageClass.Name, 3) + } + DescribeTable("should be successful", func(build func() (vm *v1alpha2.VirtualMachine, vds []*v1alpha2.VirtualDisk)) { ns := f.Namespace().Name @@ -213,6 +218,50 @@ var _ = Describe("RWOVirtualDiskMigration", decoratorsForVolumeMigrations(), Lab } }) + It("keeps a multi-disk volume set consistent across repeated migrations", func() { + ns := f.Namespace().Name + + vm, vds := localMigrationManyDisksBuild() + + vm, err := f.VirtClient().VirtualMachines(ns).Create(ctx, vm, metav1.CreateOptions{}) + Expect(err).NotTo(HaveOccurred()) + f.DeferDelete(vm) + + for _, vd := range vds { + _, err := f.VirtClient().VirtualDisks(ns).Create(ctx, vd, metav1.CreateOptions{}) + Expect(err).NotTo(HaveOccurred()) + f.DeferDelete(vd) + } + + By("Wait until VM agent is ready") + util.UntilVMAgentReady(ctx, crclient.ObjectKeyFromObject(vm), framework.LongTimeout) + + // Repeated migrations of a multi-disk VM: each round must move the whole + // volume set atomically and finalize before the next starts. A non-atomic or + // unfinalized set makes KubeVirt reject the transition (migration hangs) or + // drops an RWO volume (VM ends up restart-required with UnexpectedState). + for i := range 3 { + vmopName := "many-disks-migration-" + strconv.Itoa(i) + + By("Starting migration round " + strconv.Itoa(i)) + vmop := util.MigrateVirtualMachine(f, vm, vmopbuilder.WithName(vmopName)) + + util.UntilVMOPMigrationSucceeded(ctx, vmop, framework.MaxTimeout) + + vm, err = f.VirtClient().VirtualMachines(ns).Get(ctx, vm.GetName(), metav1.GetOptions{}) + Expect(err).NotTo(HaveOccurred()) + Expect(vm.Status.MigrationState).ShouldNot(BeNil()) + Expect(vm.Status.MigrationState.Result).To(Equal(v1alpha2.MigrationResultSucceeded)) + + untilVirtualDisksMigrationsSucceeded(f) + + By("Verifying the migration did not leave the VM restart-required") + awaitRestart, _ := conditions.GetCondition(vmcondition.TypeAwaitingRestartToApplyConfiguration, vm.Status.Conditions) + Expect(awaitRestart.Status).NotTo(Equal(metav1.ConditionTrue), + "volume set left inconsistent after migration: %s", awaitRestart.Message) + } + }) + It("should be successful when a restart is pending", func() { ns := f.Namespace().Name From 588971092cb9558e1ffcad0124d13ee410faf7cc Mon Sep 17 00:00:00 2001 From: Daniil Antoshin Date: Tue, 14 Jul 2026 17:17:35 +0200 Subject: [PATCH 03/21] docs(vm): trim verbose volume-migration comments Signed-off-by: Daniil Antoshin --- .../vm/internal/service/migration_volumes.go | 49 ++++++------------- test/e2e/vm/util.go | 3 +- test/e2e/vm/volume_migration_local_disks.go | 6 +-- 3 files changed, 17 insertions(+), 41 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go index 67bf571859..b273bdd222 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go @@ -149,14 +149,9 @@ func (s MigrationVolumesService) SyncVolumes(ctx context.Context, vmState state. } if !equality.Semantic.DeepEqual(builtKVVM.Spec.Template.Spec.Volumes, kvvmiInCluster.Spec.Volumes) { - // A difference here (ignoring migration target PVCs, which live in - // builtKVVMWithMigrationVolumes) means the desired volume set differs from - // the running one. Only a structural change (a disk added or removed) may - // require a restart, so it must not be propagated to KVVM while the VM - // awaits restart. A difference that is only a PVC swap on the same disks is - // a volume migration or a revert of one: it keeps the structure intact and - // must proceed regardless of restart, otherwise a KVVM left pointing at a - // dead migration target can never be reverted back to the source. + // Defer only structural changes (disk added/removed) under restart. A PVC swap + // on the same disks is a migration or its revert and must proceed regardless, + // else a KVVM pointing at a dead migration target can never be reverted. if restartRequired && isStructuralVolumeChange(builtKVVM, kvvmiInCluster) { log.Info("Virtualmachine is restart required, delay structural volume changes to KVVM.") return reconcile.Result{}, nil @@ -177,27 +172,17 @@ func (s MigrationVolumesService) SyncVolumes(ctx context.Context, vmState state. } if migrationRequested { - // Completeness: all ReadWriteOnce local disks must migrate together in a - // single round. If only some are migrating, the built set mixes this round's - // targets with a previous round's PVCs, which KubeVirt rejects ("the volume - // can only be reverted to the previous version during the update") or, worse, - // leaves an RWO volume out of the migration and breaks the domain on the - // target node. Wait until the whole set is migrating before patching. - // ponytail: this only blocks the bad patch; preparing all disks together is - // the vd-controller's job, so if a round never completes we keep waiting here. + // Completeness: patch the whole RWO set at once. A partial set mixes this + // round's targets with a previous round's PVCs, which KubeVirt rejects or, + // worse, drops an RWO volume and breaks the domain on the target node. if !allDisksMigrating(readWriteOnceDisks) { log.Info("not all ReadWriteOnce disks are migrating in this round yet, wait for a complete volume set.") return reconcile.Result{RequeueAfter: 10 * time.Second}, nil } - // Serialization: do not start a new migration round while a volume migration - // is already in progress (VMI condition VolumesChange=True) that targets - // different destinations. KubeVirt only accepts continuing to the current - // destinations or a clean revert to the source during an update; a new, - // different set is rejected ("the volume can only be reverted to the previous - // version during the update"). Revert-to-source is handled by the builtKVVM - // branch above, so here we only allow continuing the same round; otherwise - // wait for the in-flight migration to finalize. + // Serialization: while a migration is in flight, KubeVirt only accepts + // continuing to the same targets or reverting to the source (handled above); + // a new, different target set is rejected. Wait for the current one to finalize. if isVolumeMigrating(kvvmiInCluster) && !destinationsMatch(kvvmiInCluster, builtKVVMWithMigrationVolumes) { log.Info("a volume migration is already in progress with different targets, wait for it to finalize.") return reconcile.Result{RequeueAfter: 5 * time.Second}, nil @@ -250,10 +235,8 @@ func (s MigrationVolumesService) SyncVolumes(ctx context.Context, vmState state. return reconcile.Result{}, nil } -// isStructuralVolumeChange reports whether the desired and running volume sets -// differ structurally, i.e. a volume was added or removed. A difference that is -// only a PersistentVolumeClaim swap on the same set of volume names is a volume -// migration or its revert, not a structural change. +// isStructuralVolumeChange reports whether a volume was added or removed. A PVC +// swap on the same volume names is a migration or its revert, not structural. func isStructuralVolumeChange(builtKVVM *virtv1.VirtualMachine, kvvmi *virtv1.VirtualMachineInstance) bool { desired := make(map[string]struct{}, len(builtKVVM.Spec.Template.Spec.Volumes)) for _, v := range builtKVVM.Spec.Template.Spec.Volumes { @@ -580,9 +563,7 @@ func (s MigrationVolumesService) makeKVVMFromVirtualMachineSpec(ctx context.Cont return kvvm, kvvmWithMigrationVolumes, nil } -// allDisksMigrating reports whether every disk in the set is migrating in the -// current round (MigrationState started and not ended). Used to avoid patching a -// volume set that mixes disks from different migration rounds. +// allDisksMigrating reports whether every disk is migrating in the current round. func allDisksMigrating(disks map[string]*v1alpha2.VirtualDisk) bool { for _, d := range disks { if !commonvd.IsMigrating(d) { @@ -599,10 +580,8 @@ func isVolumeMigrating(kvvmi *virtv1.VirtualMachineInstance) bool { return cond.Status == corev1.ConditionTrue } -// destinationsMatch reports whether the volume update KubeVirt currently records -// (kvvmi.status.migratedVolumes) targets the same destinations as the set we are -// about to patch. If it does, patching continues the same update; if not, a new, -// conflicting update would be issued over an in-flight one. +// destinationsMatch reports whether the in-flight migration (kvvmi.status.migratedVolumes) +// targets the same destinations as the set we are about to patch. func destinationsMatch(kvvmi *virtv1.VirtualMachineInstance, built *virtv1.VirtualMachine) bool { want := make(map[string]string, len(built.Spec.Template.Spec.Volumes)) for _, v := range built.Spec.Template.Spec.Volumes { diff --git a/test/e2e/vm/util.go b/test/e2e/vm/util.go index 6d37b6222a..e7c784864b 100644 --- a/test/e2e/vm/util.go +++ b/test/e2e/vm/util.go @@ -106,8 +106,7 @@ func rootAndAdditionalBuild(f *framework.Framework, vi *v1alpha2.VirtualImage, r } // rootAndManyAdditionalBuild builds a VM with a root disk plus count additional -// ReadWriteOnce disks on the given storage class — a multi-disk VM whose block -// migration must move the whole volume set atomically. +// ReadWriteOnce disks on the given storage class. func rootAndManyAdditionalBuild(f *framework.Framework, vi *v1alpha2.VirtualImage, root buildOption, storageClass *string, count int) (*v1alpha2.VirtualMachine, []*v1alpha2.VirtualDisk) { refs := []v1alpha2.BlockDeviceSpecRef{{Kind: v1alpha2.VirtualDiskKind, Name: root.name}} vds := []*v1alpha2.VirtualDisk{newRootVD(f, root, vi)} diff --git a/test/e2e/vm/volume_migration_local_disks.go b/test/e2e/vm/volume_migration_local_disks.go index 0dc6b9c6f0..e01e995cbd 100644 --- a/test/e2e/vm/volume_migration_local_disks.go +++ b/test/e2e/vm/volume_migration_local_disks.go @@ -236,10 +236,8 @@ var _ = Describe("RWOVirtualDiskMigration", decoratorsForVolumeMigrations(), Lab By("Wait until VM agent is ready") util.UntilVMAgentReady(ctx, crclient.ObjectKeyFromObject(vm), framework.LongTimeout) - // Repeated migrations of a multi-disk VM: each round must move the whole - // volume set atomically and finalize before the next starts. A non-atomic or - // unfinalized set makes KubeVirt reject the transition (migration hangs) or - // drops an RWO volume (VM ends up restart-required with UnexpectedState). + // Each round must move the whole volume set atomically and finalize before the + // next: a partial or unfinalized set makes KubeVirt hang or drop an RWO volume. for i := range 3 { vmopName := "many-disks-migration-" + strconv.Itoa(i) From 0c2ab5cd560f02cdc34c4321eac07bf121a221cc Mon Sep 17 00:00:00 2001 From: Daniil Antoshin Date: Tue, 14 Jul 2026 17:51:12 +0200 Subject: [PATCH 04/21] test(e2e): fail volume migration on VolumesUpdateError and revert instead of skipping Drop the skip-on-known-failure for VolumesUpdateError and reverted VD migrations so these specs surface the failure they are meant to guard. The client-socket-closed KubeVirt flake stays skipped for now. Signed-off-by: Daniil Antoshin --- test/e2e/internal/util/vm.go | 64 ++-------------------------------- test/e2e/internal/util/vmop.go | 4 --- test/e2e/vm/util.go | 4 --- 3 files changed, 3 insertions(+), 69 deletions(-) diff --git a/test/e2e/internal/util/vm.go b/test/e2e/internal/util/vm.go index d1778bffc9..fe84a47fa7 100644 --- a/test/e2e/internal/util/vm.go +++ b/test/e2e/internal/util/vm.go @@ -41,8 +41,7 @@ import ( ) const ( - VmopE2ePrefix = "vmop-e2e" - knownVolumeUpdateFailureReason = "VolumesUpdateError" + VmopE2ePrefix = "vmop-e2e" ) var knownKubeVirtClientSocketClosedRe = regexp.MustCompile(`(?is)virError\(Code=1,.*internal error:\s*client\s+socket\s+is\s+closed`) @@ -77,69 +76,16 @@ func SkipIfKnownKubeVirtClientSocketClosedMigrationFailureWithContext(ctx contex } } -func IsKnownVolumesUpdateFailureReason(reason string) bool { - return reason == knownVolumeUpdateFailureReason -} - -// TODO: remove temporary migration skip logic when known issue "VolumesUpdateError" is fixed: -func SkipIfKnownVolumesUpdateMigrationFailure(vm *v1alpha2.VirtualMachine) { - SkipIfKnownVolumesUpdateMigrationFailureWithContext(context.Background(), vm) -} - -// TODO: remove temporary migration skip logic when known issue "VolumesUpdateError" is fixed: -func SkipIfKnownVolumesUpdateMigrationFailureWithContext(ctx context.Context, vm *v1alpha2.VirtualMachine) { - GinkgoHelper() - - if vm == nil { - return - } - - intvirtvmi, err := GetInternalVirtualMachineInstance(ctx, vm) - Expect(err).NotTo(HaveOccurred()) - if intvirtvmi == nil { - return - } - - // Prefer checking the concrete migratable condition, where volume update issues are expected. - migratableCondition, exists := conditions.GetKVVMICondition(virtv1.VirtualMachineInstanceIsMigratable, intvirtvmi.Status.Conditions) - if exists && IsKnownVolumesUpdateFailureReason(migratableCondition.Reason) { - Skip(fmt.Sprintf("skip due to known volume update migration issue for vm %s/%s: condition=%s, reason=%s, message=%s", - vm.Namespace, vm.Name, migratableCondition.Type, migratableCondition.Reason, migratableCondition.Message)) - } -} - -// TODO: remove temporary migration skip logic when both known issues are fixed: -// kubevirt "client socket is closed" and VolumesUpdateError. +// TODO: remove temporary migration skip logic when issue "client socket is closed" is fixed: func SkipIfKnownMigrationFailure(vm *v1alpha2.VirtualMachine) { SkipIfKnownMigrationFailureWithContext(context.Background(), vm) } -// TODO: remove temporary migration skip logic when both known issues are fixed: -// kubevirt "client socket is closed" and VolumesUpdateError. +// TODO: remove temporary migration skip logic when issue "client socket is closed" is fixed: func SkipIfKnownMigrationFailureWithContext(ctx context.Context, vm *v1alpha2.VirtualMachine) { GinkgoHelper() SkipIfKnownKubeVirtClientSocketClosedMigrationFailureWithContext(ctx, vm) - SkipIfKnownVolumesUpdateMigrationFailureWithContext(ctx, vm) -} - -// TODO: remove temporary migration skip logic when VD Migration Controller revert issue is fixed: -// controller may revert volume migration (VM not running, VM not migrating, etc.). -func SkipIfVDMigrationReverted(namespace string) { - GinkgoHelper() - - vds, err := framework.GetClients().VirtClient().VirtualDisks(namespace).List(context.Background(), metav1.ListOptions{}) - if err != nil { - GinkgoWriter.Printf("Failed to list VirtualDisks in namespace %q for revert check: %v\n", namespace, err) - return - } - - for _, vd := range vds.Items { - if vd.Status.MigrationState.Result == v1alpha2.VirtualDiskMigrationResultFailed && - vd.Status.MigrationState.Message == "Migration reverted." { - Skip(fmt.Sprintf("skip: VD %s/%s migration was reverted", namespace, vd.Name)) - } - } } func GetInternalVirtualMachineInstance(ctx context.Context, vm *v1alpha2.VirtualMachine) (*virtv1.VirtualMachineInstance, error) { @@ -274,10 +220,6 @@ func UntilVMMigrationSucceeded(key client.ObjectKey, timeout time.Duration) { // The VM object mirrors the migration state of the completed VMOP with a small lag; keep // asserting the same final state as before. Eventually(func() error { - // TODO: remove temporary migration skip logic when VD Migration Controller revert issue is fixed: - // controller may revert volume migration (VM not running, VM not migrating, etc.). - SkipIfVDMigrationReverted(key.Namespace) - vm, err := framework.GetClients().VirtClient().VirtualMachines(key.Namespace).Get(context.Background(), key.Name, metav1.GetOptions{}) if err != nil { return err diff --git a/test/e2e/internal/util/vmop.go b/test/e2e/internal/util/vmop.go index ef68694a60..0802a08f6c 100644 --- a/test/e2e/internal/util/vmop.go +++ b/test/e2e/internal/util/vmop.go @@ -70,10 +70,6 @@ func failVMOPMigration(vmop *v1alpha2.VirtualMachineOperation, err error) { func skipIfKnownMigrationIssue(vmop *v1alpha2.VirtualMachineOperation) { GinkgoHelper() - // TODO: remove temporary migration skip logic when VD Migration Controller revert issue is fixed: - // controller may revert volume migration (VM not running, VM not migrating, etc.). - SkipIfVDMigrationReverted(vmop.Namespace) - // The context is intentionally fresh: the caller's context may already be expired on the // timeout path, while the skip checks must still be able to inspect the cluster. ctx := context.Background() diff --git a/test/e2e/vm/util.go b/test/e2e/vm/util.go index e7c784864b..a908210093 100644 --- a/test/e2e/vm/util.go +++ b/test/e2e/vm/util.go @@ -150,10 +150,6 @@ func untilVirtualDisksMigrationsSucceeded(f *framework.Framework) { By("Wait until VirtualDisks migrations succeeded") Eventually(func(g Gomega) { - // TODO: remove temporary migration skip logic when VD Migration Controller revert issue is fixed: - // controller may revert volume migration (VM not running, VM not migrating, etc.). - e2eutil.SkipIfVDMigrationReverted(f.Namespace().Name) - vms, err := f.VirtClient().VirtualMachines(f.Namespace().Name).List(context.Background(), metav1.ListOptions{}) g.Expect(err).NotTo(HaveOccurred()) for _, vm := range vms.Items { From 3c29f79e3243f5cb9dae76b358a660cf93508228 Mon Sep 17 00:00:00 2001 From: Daniil Antoshin Date: Tue, 14 Jul 2026 18:21:04 +0200 Subject: [PATCH 05/21] test(e2e): migrate a multi-disk VM repeatedly under a pending restart The volume-update inconsistency ("the volume can only be reverted to the previous version during the update") only reproduces when the whole RWO set is migrated repeatedly while a restart is pending. Induce the restart before the migration rounds so the spec exercises that path. Signed-off-by: Daniil Antoshin --- test/e2e/vm/volume_migration_local_disks.go | 22 +++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/test/e2e/vm/volume_migration_local_disks.go b/test/e2e/vm/volume_migration_local_disks.go index e01e995cbd..5d12fd1719 100644 --- a/test/e2e/vm/volume_migration_local_disks.go +++ b/test/e2e/vm/volume_migration_local_disks.go @@ -39,7 +39,6 @@ import ( "github.com/deckhouse/virtualization-controller/pkg/common/patch" "github.com/deckhouse/virtualization-controller/pkg/controller/conditions" "github.com/deckhouse/virtualization/api/core/v1alpha2" - "github.com/deckhouse/virtualization/api/core/v1alpha2/vmcondition" "github.com/deckhouse/virtualization/test/e2e/internal/framework" "github.com/deckhouse/virtualization/test/e2e/internal/object" "github.com/deckhouse/virtualization/test/e2e/internal/precheck" @@ -218,7 +217,7 @@ var _ = Describe("RWOVirtualDiskMigration", decoratorsForVolumeMigrations(), Lab } }) - It("keeps a multi-disk volume set consistent across repeated migrations", func() { + It("keeps a multi-disk volume set consistent across repeated migrations while a restart is pending", func() { ns := f.Namespace().Name vm, vds := localMigrationManyDisksBuild() @@ -236,8 +235,17 @@ var _ = Describe("RWOVirtualDiskMigration", decoratorsForVolumeMigrations(), Lab By("Wait until VM agent is ready") util.UntilVMAgentReady(ctx, crclient.ObjectKeyFromObject(vm), framework.LongTimeout) - // Each round must move the whole volume set atomically and finalize before the - // next: a partial or unfinalized set makes KubeVirt hang or drop an RWO volume. + By("Applying a change that requires a restart") + patchBytes, err := patch.NewJSONPatch(patch.WithAdd("/spec/terminationGracePeriodSeconds", int64(11))).Bytes() + Expect(err).NotTo(HaveOccurred()) + vm, err = f.VirtClient().VirtualMachines(ns).Patch(ctx, vm.GetName(), types.JSONPatchType, patchBytes, metav1.PatchOptions{}) + Expect(err).NotTo(HaveOccurred()) + Expect(util.IsRestartRequired(vm, framework.ShortTimeout)).To(BeTrue()) + + // Repeated migrations of a multi-disk VM while a restart is pending: each round + // must move the whole RWO set atomically and finalize before the next starts. + // A partial or unfinalized set makes KubeVirt reject the next round ("the volume + // can only be reverted to the previous version during the update"). for i := range 3 { vmopName := "many-disks-migration-" + strconv.Itoa(i) @@ -253,10 +261,8 @@ var _ = Describe("RWOVirtualDiskMigration", decoratorsForVolumeMigrations(), Lab untilVirtualDisksMigrationsSucceeded(f) - By("Verifying the migration did not leave the VM restart-required") - awaitRestart, _ := conditions.GetCondition(vmcondition.TypeAwaitingRestartToApplyConfiguration, vm.Status.Conditions) - Expect(awaitRestart.Status).NotTo(Equal(metav1.ConditionTrue), - "volume set left inconsistent after migration: %s", awaitRestart.Message) + By("Restart stays pending: the change was neither lost nor applied without a restart") + Expect(util.IsRestartRequired(vm, framework.ShortTimeout)).To(BeTrue()) } }) From 9229d20cf06a1574ffa2bd4be4548b4ffb3c94ac Mon Sep 17 00:00:00 2001 From: Daniil Antoshin Date: Tue, 14 Jul 2026 19:16:23 +0200 Subject: [PATCH 06/21] test(e2e): request a restart mid-migration on a multi-disk VM Migrate the whole RWO set once so the disks leave their base PVCs, start a second migration, and request a restart while it is still in flight. The restart reconcile must not push a conflicting volume update over the unfinalized set. Combined with failing (not skipping) on VolumesUpdateError, this guards the volume-consistency path; the race itself is exercised aggressively by the manual repro. Signed-off-by: Daniil Antoshin --- test/e2e/vm/volume_migration_local_disks.go | 61 +++++++++++++-------- 1 file changed, 37 insertions(+), 24 deletions(-) diff --git a/test/e2e/vm/volume_migration_local_disks.go b/test/e2e/vm/volume_migration_local_disks.go index 5d12fd1719..4e5353be4b 100644 --- a/test/e2e/vm/volume_migration_local_disks.go +++ b/test/e2e/vm/volume_migration_local_disks.go @@ -217,7 +217,7 @@ var _ = Describe("RWOVirtualDiskMigration", decoratorsForVolumeMigrations(), Lab } }) - It("keeps a multi-disk volume set consistent across repeated migrations while a restart is pending", func() { + It("keeps a multi-disk volume set consistent when a restart is requested mid-migration", func() { ns := f.Namespace().Name vm, vds := localMigrationManyDisksBuild() @@ -235,35 +235,48 @@ var _ = Describe("RWOVirtualDiskMigration", decoratorsForVolumeMigrations(), Lab By("Wait until VM agent is ready") util.UntilVMAgentReady(ctx, crclient.ObjectKeyFromObject(vm), framework.LongTimeout) - By("Applying a change that requires a restart") - patchBytes, err := patch.NewJSONPatch(patch.WithAdd("/spec/terminationGracePeriodSeconds", int64(11))).Bytes() - Expect(err).NotTo(HaveOccurred()) - vm, err = f.VirtClient().VirtualMachines(ns).Patch(ctx, vm.GetName(), types.JSONPatchType, patchBytes, metav1.PatchOptions{}) - Expect(err).NotTo(HaveOccurred()) - Expect(util.IsRestartRequired(vm, framework.ShortTimeout)).To(BeTrue()) + By("Migrating the whole set once so the disks move off their base PVCs") + firstVMOP := util.MigrateVirtualMachine(f, vm, vmopbuilder.WithName("many-disks-migration-1")) + util.UntilVMOPMigrationSucceeded(ctx, firstVMOP, framework.MaxTimeout) + untilVirtualDisksMigrationsSucceeded(f) - // Repeated migrations of a multi-disk VM while a restart is pending: each round - // must move the whole RWO set atomically and finalize before the next starts. - // A partial or unfinalized set makes KubeVirt reject the next round ("the volume - // can only be reverted to the previous version during the update"). - for i := range 3 { - vmopName := "many-disks-migration-" + strconv.Itoa(i) + By("Starting a second migration of the whole volume set") + vmop := util.MigrateVirtualMachine(f, vm, vmopbuilder.WithName("many-disks-migration-2")) - By("Starting migration round " + strconv.Itoa(i)) - vmop := util.MigrateVirtualMachine(f, vm, vmopbuilder.WithName(vmopName)) + // Request a restart while the second volume migration is still in flight. The + // restart reconcile must not issue a conflicting volume update over the unfinalized + // set, otherwise KubeVirt rejects it ("the volume can only be reverted to the + // previous version during the update") and leaves the volume set inconsistent. + By("Requesting a restart while the migration is in flight") + Eventually(func() error { + vm, err = f.VirtClient().VirtualMachines(ns).Get(ctx, vm.GetName(), metav1.GetOptions{}) + if err != nil { + return err + } + state := vm.Status.MigrationState + if state == nil || state.StartTimestamp.IsZero() || !state.EndTimestamp.IsZero() { + return fmt.Errorf("migration is not in flight") + } + patchBytes, err := patch.NewJSONPatch(patch.WithAdd("/spec/terminationGracePeriodSeconds", int64(11))).Bytes() + if err != nil { + return err + } + _, err = f.VirtClient().VirtualMachines(ns).Patch(ctx, vm.GetName(), types.JSONPatchType, patchBytes, metav1.PatchOptions{}) + return err + }).WithTimeout(framework.ShortTimeout).WithPolling(time.Second).Should(Succeed()) - util.UntilVMOPMigrationSucceeded(ctx, vmop, framework.MaxTimeout) + By("The in-flight migration still finalizes cleanly") + util.UntilVMOPMigrationSucceeded(ctx, vmop, framework.MaxTimeout) - vm, err = f.VirtClient().VirtualMachines(ns).Get(ctx, vm.GetName(), metav1.GetOptions{}) - Expect(err).NotTo(HaveOccurred()) - Expect(vm.Status.MigrationState).ShouldNot(BeNil()) - Expect(vm.Status.MigrationState.Result).To(Equal(v1alpha2.MigrationResultSucceeded)) + vm, err = f.VirtClient().VirtualMachines(ns).Get(ctx, vm.GetName(), metav1.GetOptions{}) + Expect(err).NotTo(HaveOccurred()) + Expect(vm.Status.MigrationState).ShouldNot(BeNil()) + Expect(vm.Status.MigrationState.Result).To(Equal(v1alpha2.MigrationResultSucceeded)) - untilVirtualDisksMigrationsSucceeded(f) + untilVirtualDisksMigrationsSucceeded(f) - By("Restart stays pending: the change was neither lost nor applied without a restart") - Expect(util.IsRestartRequired(vm, framework.ShortTimeout)).To(BeTrue()) - } + By("Restart stays pending: the change was neither lost nor applied without a restart") + Expect(util.IsRestartRequired(vm, framework.ShortTimeout)).To(BeTrue()) }) It("should be successful when a restart is pending", func() { From 142cc54b1e4667b3f8bfcabe02b4e54cbeb4e2be Mon Sep 17 00:00:00 2001 From: Daniil Antoshin Date: Tue, 14 Jul 2026 20:47:45 +0200 Subject: [PATCH 07/21] test(e2e): make the mid-migration restart spec storage-class portable Dropping the fragile in-flight catch: on instant (replicated) storage the migration finalizes before it can be observed mid-flight, which timed out. Just request the restart right after the migration starts so it races the unfinalized set on copy-based storage and lands right after on instant storage; both must finalize cleanly. Signed-off-by: Daniil Antoshin --- test/e2e/vm/volume_migration_local_disks.go | 34 ++++++++------------- 1 file changed, 12 insertions(+), 22 deletions(-) diff --git a/test/e2e/vm/volume_migration_local_disks.go b/test/e2e/vm/volume_migration_local_disks.go index 4e5353be4b..4797743b23 100644 --- a/test/e2e/vm/volume_migration_local_disks.go +++ b/test/e2e/vm/volume_migration_local_disks.go @@ -243,29 +243,19 @@ var _ = Describe("RWOVirtualDiskMigration", decoratorsForVolumeMigrations(), Lab By("Starting a second migration of the whole volume set") vmop := util.MigrateVirtualMachine(f, vm, vmopbuilder.WithName("many-disks-migration-2")) - // Request a restart while the second volume migration is still in flight. The - // restart reconcile must not issue a conflicting volume update over the unfinalized - // set, otherwise KubeVirt rejects it ("the volume can only be reverted to the - // previous version during the update") and leaves the volume set inconsistent. - By("Requesting a restart while the migration is in flight") - Eventually(func() error { - vm, err = f.VirtClient().VirtualMachines(ns).Get(ctx, vm.GetName(), metav1.GetOptions{}) - if err != nil { - return err - } - state := vm.Status.MigrationState - if state == nil || state.StartTimestamp.IsZero() || !state.EndTimestamp.IsZero() { - return fmt.Errorf("migration is not in flight") - } - patchBytes, err := patch.NewJSONPatch(patch.WithAdd("/spec/terminationGracePeriodSeconds", int64(11))).Bytes() - if err != nil { - return err - } - _, err = f.VirtClient().VirtualMachines(ns).Patch(ctx, vm.GetName(), types.JSONPatchType, patchBytes, metav1.PatchOptions{}) - return err - }).WithTimeout(framework.ShortTimeout).WithPolling(time.Second).Should(Succeed()) + // Request a restart right after the migration starts, so the restart reconcile + // races with the still-unfinalized volume set. It must not issue a conflicting + // volume update over that set, otherwise KubeVirt rejects it ("the volume can only + // be reverted to the previous version during the update") and the set is left + // inconsistent. On copy-based storage the patch lands mid-migration; on instant + // (replicated) storage it lands right after — both must finalize cleanly. + By("Requesting a restart around the migration") + patchBytes, err := patch.NewJSONPatch(patch.WithAdd("/spec/terminationGracePeriodSeconds", int64(11))).Bytes() + Expect(err).NotTo(HaveOccurred()) + _, err = f.VirtClient().VirtualMachines(ns).Patch(ctx, vm.GetName(), types.JSONPatchType, patchBytes, metav1.PatchOptions{}) + Expect(err).NotTo(HaveOccurred()) - By("The in-flight migration still finalizes cleanly") + By("The migration still finalizes cleanly") util.UntilVMOPMigrationSucceeded(ctx, vmop, framework.MaxTimeout) vm, err = f.VirtClient().VirtualMachines(ns).Get(ctx, vm.GetName(), metav1.GetOptions{}) From 9a511922dc90a1dd9d85b2bc79e244acaf041afa Mon Sep 17 00:00:00 2001 From: Daniil Antoshin Date: Wed, 15 Jul 2026 15:17:17 +0200 Subject: [PATCH 08/21] fix(vmop): time out migrations waiting for the VM to be ready A volume migration whose target disks never synchronize (e.g. the target PVCs stay Pending or a previous round left the state uncleaned) keeps the VMOP Pending with reason WaitingForVirtualMachineToBeReadyToMigrate forever, with no time bound. Bound the wait and fail the operation so the disk migration revert path can recover the VM, and requeue while pending so the deadline fires without further watch events. Signed-off-by: Daniil Antoshin --- .../migration/internal/handler/lifecycle.go | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/images/virtualization-artifact/pkg/controller/vmop/migration/internal/handler/lifecycle.go b/images/virtualization-artifact/pkg/controller/vmop/migration/internal/handler/lifecycle.go index 0abae12f95..f3fd5cd9a8 100644 --- a/images/virtualization-artifact/pkg/controller/vmop/migration/internal/handler/lifecycle.go +++ b/images/virtualization-artifact/pkg/controller/vmop/migration/internal/handler/lifecycle.go @@ -49,6 +49,12 @@ const lifecycleHandlerName = "LifecycleHandler" const timeElapsedUpdateInterval = 10 * time.Second +// waitForVMReadyToMigrateTimeout bounds how long a migration operation may wait +// for the VM to become ready to migrate. A volume migration that never reaches +// this state (e.g. its target disks never sync) would otherwise keep the VMOP +// Pending forever; failing it lets the disk-migration revert path recover the VM. +const waitForVMReadyToMigrateTimeout = 10 * time.Minute + const ( progressMigrationPending int32 = 0 progressTargetScheduling int32 = 2 @@ -259,6 +265,12 @@ func (h LifecycleHandler) Handle(ctx context.Context, vmop *v1alpha2.VirtualMach // 7. Check if the vm is migratable. if !h.canExecute(vmop, vm) { + // Drive the deadline in canExecute while the operation waits for the VM to + // become ready to migrate, so a wedged migration is failed even without + // further watch events. + if vmop.Status.Phase == v1alpha2.VMOPPhasePending { + return reconcile.Result{RequeueAfter: timeElapsedUpdateInterval}, nil + } return reconcile.Result{}, nil } // 7.1 The Operation is valid, and can be executed. @@ -440,6 +452,22 @@ func (h LifecycleHandler) canExecute(vmop *v1alpha2.VirtualMachineOperation, vm migratable, _ := conditions.GetCondition(vmcondition.TypeMigratable, vm.Status.Conditions) if migratable.Status == metav1.ConditionTrue { + completed, _ := conditions.GetCondition(vmopcondition.TypeCompleted, vmop.Status.Conditions) + waitingForReady := completed.Reason == vmopcondition.ReasonWaitingForVirtualMachineToBeReadyToMigrate.String() + if waitingForReady && !completed.LastTransitionTime.IsZero() && + time.Since(completed.LastTransitionTime.Time) > waitForVMReadyToMigrateTimeout { + vmop.Status.Phase = v1alpha2.VMOPPhaseFailed + h.recorder.Event(vmop, corev1.EventTypeWarning, v1alpha2.ReasonErrVMOPFailed, "Timed out waiting for the VirtualMachine to become ready to migrate") + conditions.SetCondition( + conditions.NewConditionBuilder(vmopcondition.TypeCompleted). + Generation(vmop.GetGeneration()). + Reason(vmopcondition.ReasonOperationFailed). + Status(metav1.ConditionFalse). + Message("Timed out waiting for the VirtualMachine to become ready to migrate."), + &vmop.Status.Conditions) + return false + } + vmop.Status.Phase = v1alpha2.VMOPPhasePending conditions.SetCondition( conditions.NewConditionBuilder(vmopcondition.TypeCompleted). From 6626c1680c0b052b17f9142201d9df0ede065a6e Mon Sep 17 00:00:00 2001 From: Daniil Antoshin Date: Wed, 15 Jul 2026 15:18:56 +0200 Subject: [PATCH 09/21] fix(vm): force-revert kvvm to source when a dead migration wedges it When a volume migration cannot proceed and no operation is in progress, KVVM may be left pointing at a dead migration target (e.g. the target PVC was removed) while KVVMI stays on the source. kubevirt keeps requeuing on the missing target and the kvvmiSynced barrier never lets us revert, so the VM wedges forever. Force-revert KVVM to the source volumes in that case. Signed-off-by: Daniil Antoshin --- .../controller/vm/internal/service/migration_volumes.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go index b273bdd222..e4bbbc9f62 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go @@ -121,6 +121,14 @@ func (s MigrationVolumesService) SyncVolumes(ctx context.Context, vmState state. kvvmiSynced := equality.Semantic.DeepEqual(kvvmInClusterCopy.Spec.Template.Spec.Volumes, kvvmiInCluster.Spec.Volumes) if !kvvmiSynced { + // KVVM and KVVMI disagree. With no migration in progress this is a wedge: + // KVVM still carries a dead migration volume set that kubevirt will never + // sync (e.g. the migration target PVC was removed). Force-revert KVVM to the + // source volumes instead of waiting for a sync that will never come. + if vmop == nil && s.shouldPatchVolumes(kvvmInCluster, builtKVVM) { + log.Info("No in-progress migration but kvvm/kvvmi diverged, force revert kvvm to source volumes.") + return reconcile.Result{}, s.patchVolumes(ctx, builtKVVM) + } // kubevirt does not sync volumes with kvvmi yet log.Info("kvvmi volumes are not synced yet, skip volume migration.") return reconcile.Result{}, nil From 0966d7c09479b3ea05e1f4609f534a5765c69508 Mon Sep 17 00:00:00 2001 From: Daniil Antoshin Date: Wed, 15 Jul 2026 15:22:14 +0200 Subject: [PATCH 10/21] test(vm): cover migration ready-to-migrate timeout and diverged-kvvm revert Signed-off-by: Daniil Antoshin --- .../service/migration_volumes_test.go | 32 +++++++++++++++++++ .../internal/handler/lifecycle_test.go | 32 +++++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go index 50d6a53a60..624a10841d 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go @@ -250,6 +250,38 @@ var _ = Describe("MigrationVolumesService", func() { Expect(updatedKVVM.Spec.Template.Spec.Volumes[0].PersistentVolumeClaim.ClaimName).To(Equal(sourcePVC)) Expect(updatedKVVM.Spec.Template.Spec.Affinity).To(Equal(desiredKVVM.Spec.Template.Spec.Affinity)) }) + + It("force-reverts kvvm to source when kvvm/kvvmi diverged and no migration is in progress", func() { + ctx := testutil.ContextBackgroundWithNoOpLogger() + migrationStrategy := virtv1.UpdateVolumesStrategyMigration + + vm := newVM() + // KVVM is stuck on a dead migration target (with the migration strategy), + // while KVVMI never synced and still points at the source. With no in-progress + // migration this must be force-reverted instead of waiting on the kvvmiSynced + // barrier forever. + kvvmInCluster := newKVVMWithVolume(targetPVC, &migrationStrategy, "target-node") + kvvmi := newKVVMIWithVolume(sourcePVC) + desiredKVVM := newKVVMWithVolume(sourcePVC, nil, "source-node") + vmState := setupState(vm, kvvmInCluster, kvvmi) + + service := NewMigrationVolumesService( + vmState.Client(), + func(context.Context, state.VirtualMachineState) (*virtv1.VirtualMachine, error) { + return desiredKVVM.DeepCopy(), nil + }, + 10*time.Second, + ) + + _, err := service.SyncVolumes(ctx, vmState, false) + Expect(err).NotTo(HaveOccurred()) + + updatedKVVM := &virtv1.VirtualMachine{} + Expect(vmState.Client().Get(ctx, types.NamespacedName{Name: vmName, Namespace: namespace}, updatedKVVM)).To(Succeed()) + Expect(updatedKVVM.Spec.UpdateVolumesStrategy).To(BeNil()) + Expect(updatedKVVM.Spec.Template.Spec.Volumes).To(HaveLen(1)) + Expect(updatedKVVM.Spec.Template.Spec.Volumes[0].PersistentVolumeClaim.ClaimName).To(Equal(sourcePVC)) + }) }) var _ = Describe("isStructuralVolumeChange", func() { diff --git a/images/virtualization-artifact/pkg/controller/vmop/migration/internal/handler/lifecycle_test.go b/images/virtualization-artifact/pkg/controller/vmop/migration/internal/handler/lifecycle_test.go index 23fbd0a170..6a37ff1f5c 100644 --- a/images/virtualization-artifact/pkg/controller/vmop/migration/internal/handler/lifecycle_test.go +++ b/images/virtualization-artifact/pkg/controller/vmop/migration/internal/handler/lifecycle_test.go @@ -247,6 +247,38 @@ var _ = Describe("LifecycleHandler", func() { Expect(completed.Reason).To(Equal(vmopcondition.ReasonTargetScheduling.String())) }) + waitingVMOP := func(age time.Duration) *v1alpha2.VirtualMachineOperation { + vmop := newVMOPMigrate() + vmop.Status.Phase = v1alpha2.VMOPPhasePending + vmop.Status.Conditions = []metav1.Condition{{ + Type: vmopcondition.TypeCompleted.String(), + Status: metav1.ConditionFalse, + Reason: vmopcondition.ReasonWaitingForVirtualMachineToBeReadyToMigrate.String(), + LastTransitionTime: metav1.NewTime(time.Now().Add(-age)), + }} + return vmop + } + + It("fails a migration that waited past the timeout for the VM to be ready", func() { + h := LifecycleHandler{recorder: recorderMock} + vm := newVM(v1alpha2.AlwaysSafeMigrationPolicy) + vmop := waitingVMOP(waitForVMReadyToMigrateTimeout + time.Minute) + + Expect(h.canExecute(vmop, vm)).To(BeFalse()) + Expect(vmop.Status.Phase).To(Equal(v1alpha2.VMOPPhaseFailed)) + completed, _ := conditions.GetCondition(vmopcondition.TypeCompleted, vmop.Status.Conditions) + Expect(completed.Reason).To(Equal(vmopcondition.ReasonOperationFailed.String())) + }) + + It("keeps a migration pending while within the ready-to-migrate timeout", func() { + h := LifecycleHandler{recorder: recorderMock} + vm := newVM(v1alpha2.AlwaysSafeMigrationPolicy) + vmop := waitingVMOP(time.Minute) + + Expect(h.canExecute(vmop, vm)).To(BeFalse()) + Expect(vmop.Status.Phase).To(Equal(v1alpha2.VMOPPhasePending)) + }) + DescribeTable("TargetMigration", func(vmPolicy v1alpha2.LiveMigrationPolicy, nodeSelector map[string]string, targetMigrationEnabled bool) { vm := newVM(vmPolicy) vm.Status.Conditions = []metav1.Condition{ From 91326f309f3f1a14494142b91566f6e410381513 Mon Sep 17 00:00:00 2001 From: Daniil Antoshin Date: Wed, 15 Jul 2026 16:47:13 +0200 Subject: [PATCH 11/21] fix(vm): log restart-required reflection at info with the real reason Reflecting the KVVM RestartRequired condition logged a generic sentence at error level with an empty err, polluting error logs and alerts for an expected VM state while hiding why kubevirt asked for a restart. Log it at info and attach the underlying KVVM condition reason and message. Signed-off-by: Daniil Antoshin --- .../pkg/controller/vm/internal/sync_kvvm.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm.go b/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm.go index b8a72e409d..34c8000de7 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm.go @@ -297,7 +297,7 @@ func (h *SyncKvvmHandler) Handle(ctx context.Context, s state.VirtualMachineStat cond, _ := conditions.GetKVVMCondition(virtv1.VirtualMachineRestartRequired, kvvm.Status.Conditions) if cond.Status == corev1.ConditionTrue && len(kvvm.Status.StateChangeRequests) == 0 { msg := "Please restart the virtual machine to synchronize its configuration." - log.Error(msg) + log.Info(msg, "kvvmRestartRequiredReason", cond.Reason, "kvvmRestartRequiredMessage", cond.Message) cbAwaitingRestart. Status(metav1.ConditionTrue). Reason(vmcondition.ReasonUnexpectedState). From 2ca1a7883654930d67558f51956ee58bc3dadd24 Mon Sep 17 00:00:00 2001 From: Daniil Antoshin Date: Wed, 15 Jul 2026 17:59:56 +0200 Subject: [PATCH 12/21] fix(vm): clear a stale migration strategy left on kvvm A finished migration could leave updateVolumesStrategy=Migration on KVVM while KVVM and KVVMI already agree on the volumes. KubeVirt then keeps treating the VM as mid-migration. When no migration is in progress and only the strategy is stale (volumes already match the desired set), clear it. Signed-off-by: Daniil Antoshin --- .../vm/internal/service/migration_volumes.go | 12 ++++++++ .../service/migration_volumes_test.go | 30 +++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go index e4bbbc9f62..1f1607d8f9 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go @@ -134,6 +134,18 @@ func (s MigrationVolumesService) SyncVolumes(ctx context.Context, vmState state. return reconcile.Result{}, nil } + // A finished migration can leave updateVolumesStrategy=Migration on KVVM while the + // volumes already match the desired set. KubeVirt then keeps treating the VM as + // mid-migration. If no migration is in progress and only the strategy is stale, + // clear it. Guarded on equal volumes so a mid-completion window (volumes still + // differ) is never reverted here. + if vmop == nil && + equality.Semantic.DeepEqual(builtKVVM.Spec.Template.Spec.Volumes, kvvmInCluster.Spec.Template.Spec.Volumes) && + !equality.Semantic.DeepEqual(builtKVVM.Spec.UpdateVolumesStrategy, kvvmInCluster.Spec.UpdateVolumesStrategy) { + log.Info("clearing stale updateVolumesStrategy on kvvm after migration finished.") + return reconcile.Result{}, s.patchVolumes(ctx, builtKVVM) + } + readWriteOnceDisks, storageClassChangedDisks, err := s.getDisks(ctx, vmState) if err != nil { return reconcile.Result{}, err diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go index 624a10841d..eccc446320 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go @@ -251,6 +251,36 @@ var _ = Describe("MigrationVolumesService", func() { Expect(updatedKVVM.Spec.Template.Spec.Affinity).To(Equal(desiredKVVM.Spec.Template.Spec.Affinity)) }) + It("clears a stale migration strategy left on kvvm when volumes already match and no migration is in progress", func() { + ctx := testutil.ContextBackgroundWithNoOpLogger() + migrationStrategy := virtv1.UpdateVolumesStrategyMigration + + vm := newVM() + // A finished migration left updateVolumesStrategy=Migration on KVVM while KVVM + // and KVVMI already agree on the volumes. The stale strategy must be cleared. + kvvmInCluster := newKVVMWithVolume(targetPVC, &migrationStrategy, "node") + kvvmi := newKVVMIWithVolume(targetPVC) + desiredKVVM := newKVVMWithVolume(targetPVC, nil, "node") + vmState := setupState(vm, kvvmInCluster, kvvmi) + + service := NewMigrationVolumesService( + vmState.Client(), + func(context.Context, state.VirtualMachineState) (*virtv1.VirtualMachine, error) { + return desiredKVVM.DeepCopy(), nil + }, + 10*time.Second, + ) + + _, err := service.SyncVolumes(ctx, vmState, false) + Expect(err).NotTo(HaveOccurred()) + + updatedKVVM := &virtv1.VirtualMachine{} + Expect(vmState.Client().Get(ctx, types.NamespacedName{Name: vmName, Namespace: namespace}, updatedKVVM)).To(Succeed()) + Expect(updatedKVVM.Spec.UpdateVolumesStrategy).To(BeNil()) + Expect(updatedKVVM.Spec.Template.Spec.Volumes).To(HaveLen(1)) + Expect(updatedKVVM.Spec.Template.Spec.Volumes[0].PersistentVolumeClaim.ClaimName).To(Equal(targetPVC)) + }) + It("force-reverts kvvm to source when kvvm/kvvmi diverged and no migration is in progress", func() { ctx := testutil.ContextBackgroundWithNoOpLogger() migrationStrategy := virtv1.UpdateVolumesStrategyMigration From ce91f82ee5326c1973b2ee4095b777417c57cfd1 Mon Sep 17 00:00:00 2001 From: Daniil Antoshin Date: Wed, 15 Jul 2026 17:59:57 +0200 Subject: [PATCH 13/21] fix(vmop): shorten ready-to-migrate timeout to 5m A healthy migration reaches ReadyToMigrate within about a minute, so 5 minutes is a wide margin over the happy path while recovering a wedge sooner. Signed-off-by: Daniil Antoshin --- .../controller/vmop/migration/internal/handler/lifecycle.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/images/virtualization-artifact/pkg/controller/vmop/migration/internal/handler/lifecycle.go b/images/virtualization-artifact/pkg/controller/vmop/migration/internal/handler/lifecycle.go index f3fd5cd9a8..753446cbf4 100644 --- a/images/virtualization-artifact/pkg/controller/vmop/migration/internal/handler/lifecycle.go +++ b/images/virtualization-artifact/pkg/controller/vmop/migration/internal/handler/lifecycle.go @@ -53,7 +53,9 @@ const timeElapsedUpdateInterval = 10 * time.Second // for the VM to become ready to migrate. A volume migration that never reaches // this state (e.g. its target disks never sync) would otherwise keep the VMOP // Pending forever; failing it lets the disk-migration revert path recover the VM. -const waitForVMReadyToMigrateTimeout = 10 * time.Minute +// A healthy migration reaches ReadyToMigrate within roughly a minute, so this bound +// is a wide margin over the happy path while still recovering a wedge promptly. +const waitForVMReadyToMigrateTimeout = 5 * time.Minute const ( progressMigrationPending int32 = 0 From e2f1111fc0ebe8a34387dd4e16448e699e7c1b4b Mon Sep 17 00:00:00 2001 From: Daniil Antoshin Date: Wed, 15 Jul 2026 17:59:58 +0200 Subject: [PATCH 14/21] fix(vm): log restart-required reflection only on transition The KVVM RestartRequired condition is latched by kubevirt until an actual restart, so reflecting it logged the same line every reconcile. Log only when the VM newly transitions into the awaiting-restart state. Signed-off-by: Daniil Antoshin --- .../pkg/controller/vm/internal/sync_kvvm.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm.go b/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm.go index 34c8000de7..635eb5eee2 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm.go @@ -297,7 +297,12 @@ func (h *SyncKvvmHandler) Handle(ctx context.Context, s state.VirtualMachineStat cond, _ := conditions.GetKVVMCondition(virtv1.VirtualMachineRestartRequired, kvvm.Status.Conditions) if cond.Status == corev1.ConditionTrue && len(kvvm.Status.StateChangeRequests) == 0 { msg := "Please restart the virtual machine to synchronize its configuration." - log.Info(msg, "kvvmRestartRequiredReason", cond.Reason, "kvvmRestartRequiredMessage", cond.Message) + // Log only on transition into this state; the condition is latched by + // kubevirt until an actual restart, so logging every reconcile would spam. + prev, _ := conditions.GetCondition(vmcondition.TypeAwaitingRestartToApplyConfiguration, current.Status.Conditions) + if prev.Status != metav1.ConditionTrue || prev.Reason != vmcondition.ReasonUnexpectedState.String() { + log.Info(msg, "kvvmRestartRequiredReason", cond.Reason, "kvvmRestartRequiredMessage", cond.Message) + } cbAwaitingRestart. Status(metav1.ConditionTrue). Reason(vmcondition.ReasonUnexpectedState). From 444770277b86d4273b635bdf53f145890cfb6bce Mon Sep 17 00:00:00 2001 From: Daniil Antoshin Date: Wed, 15 Jul 2026 18:06:04 +0200 Subject: [PATCH 15/21] docs(vm): trim volume-migration recovery comments Signed-off-by: Daniil Antoshin --- .../vm/internal/service/migration_volumes.go | 14 +++++--------- .../pkg/controller/vm/internal/sync_kvvm.go | 4 ++-- .../vmop/migration/internal/handler/lifecycle.go | 8 ++------ 3 files changed, 9 insertions(+), 17 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go index 1f1607d8f9..e0893957d0 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go @@ -121,10 +121,8 @@ func (s MigrationVolumesService) SyncVolumes(ctx context.Context, vmState state. kvvmiSynced := equality.Semantic.DeepEqual(kvvmInClusterCopy.Spec.Template.Spec.Volumes, kvvmiInCluster.Spec.Volumes) if !kvvmiSynced { - // KVVM and KVVMI disagree. With no migration in progress this is a wedge: - // KVVM still carries a dead migration volume set that kubevirt will never - // sync (e.g. the migration target PVC was removed). Force-revert KVVM to the - // source volumes instead of waiting for a sync that will never come. + // KVVM carries a dead migration volume set kubevirt will never sync (e.g. the + // target PVC was removed) and no migration is running: revert to source. if vmop == nil && s.shouldPatchVolumes(kvvmInCluster, builtKVVM) { log.Info("No in-progress migration but kvvm/kvvmi diverged, force revert kvvm to source volumes.") return reconcile.Result{}, s.patchVolumes(ctx, builtKVVM) @@ -134,11 +132,9 @@ func (s MigrationVolumesService) SyncVolumes(ctx context.Context, vmState state. return reconcile.Result{}, nil } - // A finished migration can leave updateVolumesStrategy=Migration on KVVM while the - // volumes already match the desired set. KubeVirt then keeps treating the VM as - // mid-migration. If no migration is in progress and only the strategy is stale, - // clear it. Guarded on equal volumes so a mid-completion window (volumes still - // differ) is never reverted here. + // Clear a stale updateVolumesStrategy left by a finished migration; kubevirt + // otherwise keeps treating the VM as mid-migration. Equal-volumes guard avoids + // touching a mid-completion window where volumes still differ. if vmop == nil && equality.Semantic.DeepEqual(builtKVVM.Spec.Template.Spec.Volumes, kvvmInCluster.Spec.Template.Spec.Volumes) && !equality.Semantic.DeepEqual(builtKVVM.Spec.UpdateVolumesStrategy, kvvmInCluster.Spec.UpdateVolumesStrategy) { diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm.go b/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm.go index 635eb5eee2..0d9089fb6e 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm.go @@ -297,8 +297,8 @@ func (h *SyncKvvmHandler) Handle(ctx context.Context, s state.VirtualMachineStat cond, _ := conditions.GetKVVMCondition(virtv1.VirtualMachineRestartRequired, kvvm.Status.Conditions) if cond.Status == corev1.ConditionTrue && len(kvvm.Status.StateChangeRequests) == 0 { msg := "Please restart the virtual machine to synchronize its configuration." - // Log only on transition into this state; the condition is latched by - // kubevirt until an actual restart, so logging every reconcile would spam. + // Log only on transition: the condition is latched until restart, so + // logging every reconcile would spam. prev, _ := conditions.GetCondition(vmcondition.TypeAwaitingRestartToApplyConfiguration, current.Status.Conditions) if prev.Status != metav1.ConditionTrue || prev.Reason != vmcondition.ReasonUnexpectedState.String() { log.Info(msg, "kvvmRestartRequiredReason", cond.Reason, "kvvmRestartRequiredMessage", cond.Message) diff --git a/images/virtualization-artifact/pkg/controller/vmop/migration/internal/handler/lifecycle.go b/images/virtualization-artifact/pkg/controller/vmop/migration/internal/handler/lifecycle.go index 753446cbf4..809bd9edd5 100644 --- a/images/virtualization-artifact/pkg/controller/vmop/migration/internal/handler/lifecycle.go +++ b/images/virtualization-artifact/pkg/controller/vmop/migration/internal/handler/lifecycle.go @@ -49,12 +49,8 @@ const lifecycleHandlerName = "LifecycleHandler" const timeElapsedUpdateInterval = 10 * time.Second -// waitForVMReadyToMigrateTimeout bounds how long a migration operation may wait -// for the VM to become ready to migrate. A volume migration that never reaches -// this state (e.g. its target disks never sync) would otherwise keep the VMOP -// Pending forever; failing it lets the disk-migration revert path recover the VM. -// A healthy migration reaches ReadyToMigrate within roughly a minute, so this bound -// is a wide margin over the happy path while still recovering a wedge promptly. +// waitForVMReadyToMigrateTimeout fails a migration whose disks never sync instead +// of waiting for ReadyToMigrate forever. Healthy migrations reach it within ~1m. const waitForVMReadyToMigrateTimeout = 5 * time.Minute const ( From da1bf98b13b66e50bf1782e9f60e666ff206b692 Mon Sep 17 00:00:00 2001 From: Daniil Antoshin Date: Wed, 15 Jul 2026 19:08:39 +0200 Subject: [PATCH 16/21] fix(vm): reject any diverging volume set while a migration is in flight The in-flight destination check only verified that recorded migrated volumes keep their destinations, so a set that migrates an extra volume on top of the running round, keeps a volume at source mid-migration, or drops an in-flight (hotplugged) volume slipped through and was rejected by KubeVirt, leaving the controller retrying noisily until the round finalized. Compare both directions: a patch is allowed mid-migration only if it purely continues the current round. Signed-off-by: Daniil Antoshin --- .../vm/internal/service/migration_volumes.go | 40 ++++++++++++++--- .../service/migration_volumes_test.go | 43 +++++++++++++++---- 2 files changed, 67 insertions(+), 16 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go index e0893957d0..0f057a86dc 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go @@ -596,23 +596,49 @@ func isVolumeMigrating(kvvmi *virtv1.VirtualMachineInstance) bool { return cond.Status == corev1.ConditionTrue } -// destinationsMatch reports whether the in-flight migration (kvvmi.status.migratedVolumes) -// targets the same destinations as the set we are about to patch. +// destinationsMatch reports whether the set we are about to patch merely continues +// the in-flight migration: every volume either keeps its currently running claim or +// goes to the destination already recorded in kvvmi.status.migratedVolumes, and no +// in-flight volume is left out of the set. Anything else is a different target set, +// which KubeVirt rejects mid-migration. func destinationsMatch(kvvmi *virtv1.VirtualMachineInstance, built *virtv1.VirtualMachine) bool { - want := make(map[string]string, len(built.Spec.Template.Spec.Volumes)) - for _, v := range built.Spec.Template.Spec.Volumes { + current := make(map[string]string, len(kvvmi.Spec.Volumes)) + for _, v := range kvvmi.Spec.Volumes { if v.PersistentVolumeClaim != nil { - want[v.Name] = v.PersistentVolumeClaim.ClaimName + current[v.Name] = v.PersistentVolumeClaim.ClaimName } } + + dest := make(map[string]string, len(kvvmi.Status.MigratedVolumes)) for _, mv := range kvvmi.Status.MigratedVolumes { - if mv.DestinationPVCInfo == nil { + if mv.DestinationPVCInfo != nil { + dest[mv.VolumeName] = mv.DestinationPVCInfo.ClaimName + } + } + + seen := make(map[string]struct{}, len(built.Spec.Template.Spec.Volumes)) + for _, v := range built.Spec.Template.Spec.Volumes { + if v.PersistentVolumeClaim == nil { continue } - if want[mv.VolumeName] != mv.DestinationPVCInfo.ClaimName { + seen[v.Name] = struct{}{} + if d, ok := dest[v.Name]; ok { + if v.PersistentVolumeClaim.ClaimName != d { + return false + } + continue + } + if v.PersistentVolumeClaim.ClaimName != current[v.Name] { return false } } + + for name := range dest { + if _, ok := seen[name]; !ok { + return false + } + } + return true } diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go index eccc446320..1151da530d 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go @@ -414,7 +414,7 @@ var _ = Describe("isVolumeMigrating", func() { }) var _ = Describe("destinationsMatch", func() { - built := func(nameToClaim map[string]string) *virtv1.VirtualMachine { + pvcVolumes := func(nameToClaim map[string]string) []virtv1.Volume { vols := make([]virtv1.Volume, 0, len(nameToClaim)) for name, claim := range nameToClaim { vols = append(vols, virtv1.Volume{ @@ -422,10 +422,14 @@ var _ = Describe("destinationsMatch", func() { VolumeSource: virtv1.VolumeSource{PersistentVolumeClaim: &virtv1.PersistentVolumeClaimVolumeSource{PersistentVolumeClaimVolumeSource: corev1.PersistentVolumeClaimVolumeSource{ClaimName: claim}}}, }) } - return &virtv1.VirtualMachine{Spec: virtv1.VirtualMachineSpec{Template: &virtv1.VirtualMachineInstanceTemplateSpec{Spec: virtv1.VirtualMachineInstanceSpec{Volumes: vols}}}} + return vols } - kvvmi := func(volToDest map[string]string) *virtv1.VirtualMachineInstance { + built := func(nameToClaim map[string]string) *virtv1.VirtualMachine { + return &virtv1.VirtualMachine{Spec: virtv1.VirtualMachineSpec{Template: &virtv1.VirtualMachineInstanceTemplateSpec{Spec: virtv1.VirtualMachineInstanceSpec{Volumes: pvcVolumes(nameToClaim)}}}} + } + kvvmi := func(running, volToDest map[string]string) *virtv1.VirtualMachineInstance { vmi := &virtv1.VirtualMachineInstance{} + vmi.Spec.Volumes = pvcVolumes(running) for vol, dest := range volToDest { vmi.Status.MigratedVolumes = append(vmi.Status.MigratedVolumes, virtv1.StorageMigratedVolumeInfo{ VolumeName: vol, @@ -435,18 +439,39 @@ var _ = Describe("destinationsMatch", func() { return vmi } - It("is true when there is no recorded migration", func() { - Expect(destinationsMatch(kvvmi(nil), built(map[string]string{"root": "new"}))).To(BeTrue()) + It("is true when the set keeps the running claims and there is no recorded migration", func() { + Expect(destinationsMatch(kvvmi(map[string]string{"root": "src"}, nil), built(map[string]string{"root": "src"}))).To(BeTrue()) + }) + It("is false when a claim changes without a recorded migration for it", func() { + Expect(destinationsMatch(kvvmi(map[string]string{"root": "src"}, nil), built(map[string]string{"root": "new"}))).To(BeFalse()) }) It("is true when the recorded destination matches the target being patched", func() { - Expect(destinationsMatch(kvvmi(map[string]string{"root": "tgt"}), built(map[string]string{"root": "tgt"}))).To(BeTrue()) + Expect(destinationsMatch(kvvmi(map[string]string{"root": "src"}, map[string]string{"root": "tgt"}), built(map[string]string{"root": "tgt"}))).To(BeTrue()) }) It("is false when the recorded destination differs from the new target", func() { - Expect(destinationsMatch(kvvmi(map[string]string{"root": "old-tgt"}), built(map[string]string{"root": "new-tgt"}))).To(BeFalse()) + Expect(destinationsMatch(kvvmi(map[string]string{"root": "src"}, map[string]string{"root": "old-tgt"}), built(map[string]string{"root": "new-tgt"}))).To(BeFalse()) + }) + It("is false when the set migrates an extra volume on top of the in-flight round", func() { + Expect(destinationsMatch( + kvvmi(map[string]string{"root": "src", "data": "data-src"}, map[string]string{"root": "tgt"}), + built(map[string]string{"root": "tgt", "data": "data-tgt"}), + )).To(BeFalse()) + }) + It("is false when the set keeps a volume at source while it is migrating", func() { + Expect(destinationsMatch( + kvvmi(map[string]string{"root": "src", "data": "data-src"}, map[string]string{"root": "tgt", "data": "data-tgt"}), + built(map[string]string{"root": "tgt", "data": "data-src"}), + )).To(BeFalse()) + }) + It("is false when an in-flight volume is absent from the set being patched", func() { + Expect(destinationsMatch( + kvvmi(map[string]string{"root": "src"}, map[string]string{"hotplug": "hp-tgt"}), + built(map[string]string{"root": "src"}), + )).To(BeFalse()) }) It("ignores recorded entries without destination info", func() { - vmi := &virtv1.VirtualMachineInstance{} + vmi := kvvmi(map[string]string{"root": "src"}, nil) vmi.Status.MigratedVolumes = []virtv1.StorageMigratedVolumeInfo{{VolumeName: "root", DestinationPVCInfo: nil}} - Expect(destinationsMatch(vmi, built(map[string]string{"root": "whatever"}))).To(BeTrue()) + Expect(destinationsMatch(vmi, built(map[string]string{"root": "src"}))).To(BeTrue()) }) }) From f57401b24ebfbf0a771a8db24ee5a30d1865358e Mon Sep 17 00:00:00 2001 From: Daniil Antoshin Date: Wed, 15 Jul 2026 19:18:18 +0200 Subject: [PATCH 17/21] fix(vm): compare pull-policy normalized volumes in migration recovery guards The stale-strategy clearing and force-revert guards compared built volumes (normalized with the VMI's containerdisk pull policies) against the raw in-cluster KVVM, which never carries them. For a VM with a containerdisk volume the equality never held, so a stale updateVolumesStrategy was never cleared and the no-op check of the force revert never suppressed a patch. Compare against the normalized copy used by the kvvmi sync check instead. Signed-off-by: Daniil Antoshin --- .../vm/internal/service/migration_volumes.go | 9 +++-- .../service/migration_volumes_test.go | 40 +++++++++++++++++++ 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go index 0f057a86dc..b2ce89cb2e 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go @@ -123,7 +123,7 @@ func (s MigrationVolumesService) SyncVolumes(ctx context.Context, vmState state. if !kvvmiSynced { // KVVM carries a dead migration volume set kubevirt will never sync (e.g. the // target PVC was removed) and no migration is running: revert to source. - if vmop == nil && s.shouldPatchVolumes(kvvmInCluster, builtKVVM) { + if vmop == nil && s.shouldPatchVolumes(kvvmInClusterCopy, builtKVVM) { log.Info("No in-progress migration but kvvm/kvvmi diverged, force revert kvvm to source volumes.") return reconcile.Result{}, s.patchVolumes(ctx, builtKVVM) } @@ -134,10 +134,11 @@ func (s MigrationVolumesService) SyncVolumes(ctx context.Context, vmState state. // Clear a stale updateVolumesStrategy left by a finished migration; kubevirt // otherwise keeps treating the VM as mid-migration. Equal-volumes guard avoids - // touching a mid-completion window where volumes still differ. + // touching a mid-completion window where volumes still differ. The pull-policy + // normalized copy is compared, or containerdisk volumes never match. if vmop == nil && - equality.Semantic.DeepEqual(builtKVVM.Spec.Template.Spec.Volumes, kvvmInCluster.Spec.Template.Spec.Volumes) && - !equality.Semantic.DeepEqual(builtKVVM.Spec.UpdateVolumesStrategy, kvvmInCluster.Spec.UpdateVolumesStrategy) { + equality.Semantic.DeepEqual(builtKVVM.Spec.Template.Spec.Volumes, kvvmInClusterCopy.Spec.Template.Spec.Volumes) && + !equality.Semantic.DeepEqual(builtKVVM.Spec.UpdateVolumesStrategy, kvvmInClusterCopy.Spec.UpdateVolumesStrategy) { log.Info("clearing stale updateVolumesStrategy on kvvm after migration finished.") return reconcile.Result{}, s.patchVolumes(ctx, builtKVVM) } diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go index 1151da530d..9fb20c6d4e 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go @@ -281,6 +281,46 @@ var _ = Describe("MigrationVolumesService", func() { Expect(updatedKVVM.Spec.Template.Spec.Volumes[0].PersistentVolumeClaim.ClaimName).To(Equal(targetPVC)) }) + It("clears a stale migration strategy on a VM with a containerdisk volume despite the pull-policy drift", func() { + ctx := testutil.ContextBackgroundWithNoOpLogger() + migrationStrategy := virtv1.UpdateVolumesStrategyMigration + + containerDisk := func(policy corev1.PullPolicy) virtv1.Volume { + return virtv1.Volume{ + Name: "cdrom", + VolumeSource: virtv1.VolumeSource{ + ContainerDisk: &virtv1.ContainerDiskSource{Image: "registry.example.com/image:tag", ImagePullPolicy: policy}, + }, + } + } + + vm := newVM() + // The pull policy is defaulted by kubevirt only on the VMI: KVVM and the + // desired spec carry an empty one. The stale strategy must still be cleared. + kvvmInCluster := newKVVMWithVolume(targetPVC, &migrationStrategy, "node") + kvvmInCluster.Spec.Template.Spec.Volumes = append(kvvmInCluster.Spec.Template.Spec.Volumes, containerDisk("")) + kvvmi := newKVVMIWithVolume(targetPVC) + kvvmi.Spec.Volumes = append(kvvmi.Spec.Volumes, containerDisk(corev1.PullIfNotPresent)) + desiredKVVM := newKVVMWithVolume(targetPVC, nil, "node") + desiredKVVM.Spec.Template.Spec.Volumes = append(desiredKVVM.Spec.Template.Spec.Volumes, containerDisk("")) + vmState := setupState(vm, kvvmInCluster, kvvmi) + + service := NewMigrationVolumesService( + vmState.Client(), + func(context.Context, state.VirtualMachineState) (*virtv1.VirtualMachine, error) { + return desiredKVVM.DeepCopy(), nil + }, + 10*time.Second, + ) + + _, err := service.SyncVolumes(ctx, vmState, false) + Expect(err).NotTo(HaveOccurred()) + + updatedKVVM := &virtv1.VirtualMachine{} + Expect(vmState.Client().Get(ctx, types.NamespacedName{Name: vmName, Namespace: namespace}, updatedKVVM)).To(Succeed()) + Expect(updatedKVVM.Spec.UpdateVolumesStrategy).To(BeNil()) + }) + It("force-reverts kvvm to source when kvvm/kvvmi diverged and no migration is in progress", func() { ctx := testutil.ContextBackgroundWithNoOpLogger() migrationStrategy := virtv1.UpdateVolumesStrategyMigration From 553715c26d54a302448542609a8dae7e7ddab85b Mon Sep 17 00:00:00 2001 From: Daniil Antoshin Date: Wed, 15 Jul 2026 20:54:10 +0200 Subject: [PATCH 18/21] fix(vm): only force-revert a diverged kvvm for a stuck migration The recovery force-revert fired for any kvvm/kvvmi volume divergence with no running migration, including a benign hotplug volume mid-attach, tearing the attachment down (VMBDA stuck InProgress). Gate it on updateVolumesStrategy= Migration so only a genuinely stuck migration is reverted; other divergence is left to sync as before. Signed-off-by: Daniil Antoshin --- .../vm/internal/service/migration_volumes.go | 6 +++- .../service/migration_volumes_test.go | 28 +++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go index b2ce89cb2e..51071d1798 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go @@ -123,7 +123,11 @@ func (s MigrationVolumesService) SyncVolumes(ctx context.Context, vmState state. if !kvvmiSynced { // KVVM carries a dead migration volume set kubevirt will never sync (e.g. the // target PVC was removed) and no migration is running: revert to source. - if vmop == nil && s.shouldPatchVolumes(kvvmInClusterCopy, builtKVVM) { + // Gate on the migration strategy so benign divergence (e.g. a hotplug volume + // mid-attach) is left to sync normally instead of being reverted. + migrationStuck := kvvmInCluster.Spec.UpdateVolumesStrategy != nil && + *kvvmInCluster.Spec.UpdateVolumesStrategy == virtv1.UpdateVolumesStrategyMigration + if vmop == nil && migrationStuck && s.shouldPatchVolumes(kvvmInClusterCopy, builtKVVM) { log.Info("No in-progress migration but kvvm/kvvmi diverged, force revert kvvm to source volumes.") return reconcile.Result{}, s.patchVolumes(ctx, builtKVVM) } diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go index 9fb20c6d4e..1c91c6173d 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go @@ -352,6 +352,34 @@ var _ = Describe("MigrationVolumesService", func() { Expect(updatedKVVM.Spec.Template.Spec.Volumes).To(HaveLen(1)) Expect(updatedKVVM.Spec.Template.Spec.Volumes[0].PersistentVolumeClaim.ClaimName).To(Equal(sourcePVC)) }) + + It("does not revert diverged volumes when no migration strategy is set (e.g. hotplug mid-attach)", func() { + ctx := testutil.ContextBackgroundWithNoOpLogger() + + vm := newVM() + // KVVM and KVVMI diverge but there is no migration strategy: a benign + // transient such as a hotplug volume being attached. It must be left to sync, + // not force-reverted (that would tear down the in-flight attachment). + kvvmInCluster := newKVVMWithVolume(targetPVC, nil, "node") + kvvmi := newKVVMIWithVolume(sourcePVC) + desiredKVVM := newKVVMWithVolume(sourcePVC, nil, "node") + vmState := setupState(vm, kvvmInCluster, kvvmi) + + service := NewMigrationVolumesService( + vmState.Client(), + func(context.Context, state.VirtualMachineState) (*virtv1.VirtualMachine, error) { + return desiredKVVM.DeepCopy(), nil + }, + 10*time.Second, + ) + + _, err := service.SyncVolumes(ctx, vmState, false) + Expect(err).NotTo(HaveOccurred()) + + updatedKVVM := &virtv1.VirtualMachine{} + Expect(vmState.Client().Get(ctx, types.NamespacedName{Name: vmName, Namespace: namespace}, updatedKVVM)).To(Succeed()) + Expect(updatedKVVM.Spec.Template.Spec.Volumes[0].PersistentVolumeClaim.ClaimName).To(Equal(targetPVC)) + }) }) var _ = Describe("isStructuralVolumeChange", func() { From 37d476484819348f9d1d4d0d129132197227fa2a Mon Sep 17 00:00:00 2001 From: Daniil Antoshin Date: Wed, 15 Jul 2026 21:16:53 +0200 Subject: [PATCH 19/21] fix(vm): do not force-revert kvvm while a no-vmop migration round is active A storage class change round runs without any VMOP: the migration itself is driven by KubeVirt's workload-updater. In the window between patching KVVM with the round's targets and KubeVirt syncing them to the KVVMI, the recovery force-revert saw a diverged KVVM with no in-progress VMOP and rolled the just-started round back to the source, churning the round start. Skip the revert while the disks still demand a migration round. Signed-off-by: Daniil Antoshin --- .../vm/internal/service/migration_volumes.go | 14 ++++---- .../service/migration_volumes_test.go | 36 +++++++++++++++++++ 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go index 51071d1798..8c1fb2edfc 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go @@ -119,15 +119,19 @@ func (s MigrationVolumesService) SyncVolumes(ctx context.Context, vmState state. s.fillContainerDiskImagePullPolicies(kvvmInClusterCopy, kvvmiInCluster) s.fillContainerDiskImagePullPolicies(builtKVVM, kvvmiInCluster) + migrationRequested := builtKVVMWithMigrationVolumes.Spec.UpdateVolumesStrategy != nil && *builtKVVMWithMigrationVolumes.Spec.UpdateVolumesStrategy == virtv1.UpdateVolumesStrategyMigration + kvvmiSynced := equality.Semantic.DeepEqual(kvvmInClusterCopy.Spec.Template.Spec.Volumes, kvvmiInCluster.Spec.Volumes) if !kvvmiSynced { // KVVM carries a dead migration volume set kubevirt will never sync (e.g. the // target PVC was removed) and no migration is running: revert to source. // Gate on the migration strategy so benign divergence (e.g. a hotplug volume - // mid-attach) is left to sync normally instead of being reverted. - migrationStuck := kvvmInCluster.Spec.UpdateVolumesStrategy != nil && - *kvvmInCluster.Spec.UpdateVolumesStrategy == virtv1.UpdateVolumesStrategyMigration - if vmop == nil && migrationStuck && s.shouldPatchVolumes(kvvmInClusterCopy, builtKVVM) { + // mid-attach) is left to sync normally instead of being reverted. A round can + // also run without any VMOP (storage class change, driven by kubevirt's + // workload-updater): while the disks still demand it (migrationRequested), + // the diverged KVVM is that round starting, not a leftover. + migrationStuck := kvvmInCluster.Spec.UpdateVolumesStrategy != nil && *kvvmInCluster.Spec.UpdateVolumesStrategy == virtv1.UpdateVolumesStrategyMigration + if vmop == nil && migrationStuck && !migrationRequested && s.shouldPatchVolumes(kvvmInClusterCopy, builtKVVM) { log.Info("No in-progress migration but kvvm/kvvmi diverged, force revert kvvm to source volumes.") return reconcile.Result{}, s.patchVolumes(ctx, builtKVVM) } @@ -180,8 +184,6 @@ func (s MigrationVolumesService) SyncVolumes(ctx context.Context, vmState state. return reconcile.Result{}, s.patchVolumes(ctx, builtKVVM) } - migrationRequested := builtKVVMWithMigrationVolumes.Spec.UpdateVolumesStrategy != nil && *builtKVVMWithMigrationVolumes.Spec.UpdateVolumesStrategy == virtv1.UpdateVolumesStrategyMigration - // Check disks in generated KVVM before running kvvmSynced check: detect non-migratable disks and disks with changed storage class. if !readWriteOnceDisksSynced { log.Info("ReadWriteOnce disks are not synced yet, skip volume migration.") diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go index 1c91c6173d..4284b35be7 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go @@ -380,6 +380,42 @@ var _ = Describe("MigrationVolumesService", func() { Expect(vmState.Client().Get(ctx, types.NamespacedName{Name: vmName, Namespace: namespace}, updatedKVVM)).To(Succeed()) Expect(updatedKVVM.Spec.Template.Spec.Volumes[0].PersistentVolumeClaim.ClaimName).To(Equal(targetPVC)) }) + + It("does not force-revert kvvm while a migration round without a vmop is active", func() { + ctx := testutil.ContextBackgroundWithNoOpLogger() + migrationStrategy := virtv1.UpdateVolumesStrategyMigration + + // A storage class change round runs without any VMOP: the disk is migrating, + // KVVM is already patched with the round's target, KVVMI is not synced yet. + // The diverged KVVM is the round starting, not a leftover to revert. + vm := newVM() + vm.Status.BlockDeviceRefs = []v1alpha2.BlockDeviceStatusRef{{Kind: v1alpha2.DiskDevice, Name: "root"}} + vd := &v1alpha2.VirtualDisk{ + ObjectMeta: metav1.ObjectMeta{Name: "root", Namespace: namespace}, + } + vd.Status.MigrationState.StartTimestamp = metav1.Now() + vd.Status.MigrationState.TargetPVC = targetPVC + kvvmInCluster := newKVVMWithVolume(targetPVC, &migrationStrategy, "node") + kvvmi := newKVVMIWithVolume(sourcePVC) + desiredKVVM := newKVVMWithVolume(sourcePVC, nil, "node") + vmState := setupState(vm, kvvmInCluster, kvvmi, vd) + + service := NewMigrationVolumesService( + vmState.Client(), + func(context.Context, state.VirtualMachineState) (*virtv1.VirtualMachine, error) { + return desiredKVVM.DeepCopy(), nil + }, + 10*time.Second, + ) + + _, err := service.SyncVolumes(ctx, vmState, false) + Expect(err).NotTo(HaveOccurred()) + + updatedKVVM := &virtv1.VirtualMachine{} + Expect(vmState.Client().Get(ctx, types.NamespacedName{Name: vmName, Namespace: namespace}, updatedKVVM)).To(Succeed()) + Expect(updatedKVVM.Spec.UpdateVolumesStrategy).To(HaveValue(Equal(migrationStrategy))) + Expect(updatedKVVM.Spec.Template.Spec.Volumes[0].PersistentVolumeClaim.ClaimName).To(Equal(targetPVC)) + }) }) var _ = Describe("isStructuralVolumeChange", func() { From 0f3dc0fbd1d26d5c9b84ec1e34a991f0c971bd01 Mon Sep 17 00:00:00 2001 From: Daniil Antoshin Date: Wed, 15 Jul 2026 21:20:11 +0200 Subject: [PATCH 20/21] docs(vm): trim volume-migration guard comments Signed-off-by: Daniil Antoshin --- .../vm/internal/service/migration_volumes.go | 28 ++++++++----------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go index 8c1fb2edfc..9f778080db 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go @@ -123,13 +123,11 @@ func (s MigrationVolumesService) SyncVolumes(ctx context.Context, vmState state. kvvmiSynced := equality.Semantic.DeepEqual(kvvmInClusterCopy.Spec.Template.Spec.Volumes, kvvmiInCluster.Spec.Volumes) if !kvvmiSynced { - // KVVM carries a dead migration volume set kubevirt will never sync (e.g. the - // target PVC was removed) and no migration is running: revert to source. - // Gate on the migration strategy so benign divergence (e.g. a hotplug volume - // mid-attach) is left to sync normally instead of being reverted. A round can - // also run without any VMOP (storage class change, driven by kubevirt's - // workload-updater): while the disks still demand it (migrationRequested), - // the diverged KVVM is that round starting, not a leftover. + // KVVM holds a dead migration set kubevirt will never sync (e.g. the target + // PVC was removed): revert to source. Only when the strategy is still set + // (plain divergence like a hotplug mid-attach must sync, not revert) and no + // round is wanted (a storage class round runs without a VMOP and must not + // be reverted at its start). migrationStuck := kvvmInCluster.Spec.UpdateVolumesStrategy != nil && *kvvmInCluster.Spec.UpdateVolumesStrategy == virtv1.UpdateVolumesStrategyMigration if vmop == nil && migrationStuck && !migrationRequested && s.shouldPatchVolumes(kvvmInClusterCopy, builtKVVM) { log.Info("No in-progress migration but kvvm/kvvmi diverged, force revert kvvm to source volumes.") @@ -140,10 +138,9 @@ func (s MigrationVolumesService) SyncVolumes(ctx context.Context, vmState state. return reconcile.Result{}, nil } - // Clear a stale updateVolumesStrategy left by a finished migration; kubevirt - // otherwise keeps treating the VM as mid-migration. Equal-volumes guard avoids - // touching a mid-completion window where volumes still differ. The pull-policy - // normalized copy is compared, or containerdisk volumes never match. + // Clear a stale updateVolumesStrategy after a finished migration; kubevirt never + // clears it and keeps treating the VM as mid-migration. Volumes-equal guard skips + // the mid-completion window; the normalized copy is required for containerdisks. if vmop == nil && equality.Semantic.DeepEqual(builtKVVM.Spec.Template.Spec.Volumes, kvvmInClusterCopy.Spec.Template.Spec.Volumes) && !equality.Semantic.DeepEqual(builtKVVM.Spec.UpdateVolumesStrategy, kvvmInClusterCopy.Spec.UpdateVolumesStrategy) { @@ -603,11 +600,10 @@ func isVolumeMigrating(kvvmi *virtv1.VirtualMachineInstance) bool { return cond.Status == corev1.ConditionTrue } -// destinationsMatch reports whether the set we are about to patch merely continues -// the in-flight migration: every volume either keeps its currently running claim or -// goes to the destination already recorded in kvvmi.status.migratedVolumes, and no -// in-flight volume is left out of the set. Anything else is a different target set, -// which KubeVirt rejects mid-migration. +// destinationsMatch reports whether the patched set merely continues the in-flight +// migration: every volume keeps its running claim or goes to its recorded destination +// (kvvmi.status.migratedVolumes), and no in-flight volume is left out. Anything else +// KubeVirt rejects mid-migration. func destinationsMatch(kvvmi *virtv1.VirtualMachineInstance, built *virtv1.VirtualMachine) bool { current := make(map[string]string, len(kvvmi.Spec.Volumes)) for _, v := range kvvmi.Spec.Volumes { From 5684d2bdfda22f8b5c68229302b1b9bbdb7c535b Mon Sep 17 00:00:00 2001 From: Daniil Antoshin Date: Wed, 15 Jul 2026 22:18:31 +0200 Subject: [PATCH 21/21] fix(vm): guard volume patches with an optimistic lock on kvvm The volume patch replaces the whole volumes array from a read snapshot. KubeVirt concurrently persists hotplugged (addvolume) volumes into the same array, so a patch built from a stale read silently dropped the just-persisted hotplug volume and the attachment never completed: with local (WFFC) disks the node affinity legitimately evolves while PVCs bind, each shift re-patched the volumes, and a VMBDA could stay InProgress indefinitely. Assert the kvvm resourceVersion in the patch so a concurrent change fails it and the next reconcile rebuilds from a fresh read. Signed-off-by: Daniil Antoshin --- .../vm/internal/service/migration_volumes.go | 10 +++++-- .../service/migration_volumes_test.go | 30 +++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go index 9f778080db..b8f2afd537 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go @@ -312,13 +312,19 @@ func (s MigrationVolumesService) shouldRevert(kvvmi *virtv1.VirtualMachineInstan } func (s MigrationVolumesService) patchVolumes(ctx context.Context, kvvm *virtv1.VirtualMachine) error { - patchBytes, err := patch.NewJSONPatch( + ops := []patch.JSONPatchOperation{ patch.WithReplace("/spec/updateVolumesStrategy", kvvm.Spec.UpdateVolumesStrategy), patch.WithReplace("/spec/template/spec/volumes", kvvm.Spec.Template.Spec.Volumes), // Affinity is patched together with volumes because the migration target PVCs // can resolve to a different node than the source. patch.WithReplace("/spec/template/spec/affinity", kvvm.Spec.Template.Spec.Affinity), - ).Bytes() + } + // Optimistic lock: kubevirt persists hotplug (addvolume) volumes into the same + // array concurrently; replacing it from a stale read silently drops them. + if kvvm.ResourceVersion != "" { + ops = append([]patch.JSONPatchOperation{patch.WithTest("/metadata/resourceVersion", kvvm.ResourceVersion)}, ops...) + } + patchBytes, err := patch.NewJSONPatch(ops...).Bytes() if err != nil { return err } diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go index 4284b35be7..542f835433 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go @@ -381,6 +381,36 @@ var _ = Describe("MigrationVolumesService", func() { Expect(updatedKVVM.Spec.Template.Spec.Volumes[0].PersistentVolumeClaim.ClaimName).To(Equal(targetPVC)) }) + It("rejects a volume patch built from a stale kvvm read", func() { + ctx := testutil.ContextBackgroundWithNoOpLogger() + migrationStrategy := virtv1.UpdateVolumesStrategyMigration + + vm := newVM() + kvvmInCluster := newKVVMWithVolume(targetPVC, &migrationStrategy, "target-node") + kvvmi := newKVVMIWithVolume(sourcePVC) + // The desired spec carries the resourceVersion of the kvvm it was built from; + // if the kvvm changed since (e.g. kubevirt persisted a hotplug volume), the + // patch must fail instead of overwriting the volumes from the stale read. + desiredKVVM := newKVVMWithVolume(sourcePVC, nil, "source-node") + desiredKVVM.ResourceVersion = "stale" + vmState := setupState(vm, kvvmInCluster, kvvmi) + + service := NewMigrationVolumesService( + vmState.Client(), + func(context.Context, state.VirtualMachineState) (*virtv1.VirtualMachine, error) { + return desiredKVVM.DeepCopy(), nil + }, + 10*time.Second, + ) + + _, err := service.SyncVolumes(ctx, vmState, false) + Expect(err).To(HaveOccurred()) + + updatedKVVM := &virtv1.VirtualMachine{} + Expect(vmState.Client().Get(ctx, types.NamespacedName{Name: vmName, Namespace: namespace}, updatedKVVM)).To(Succeed()) + Expect(updatedKVVM.Spec.Template.Spec.Volumes[0].PersistentVolumeClaim.ClaimName).To(Equal(targetPVC)) + }) + It("does not force-revert kvvm while a migration round without a vmop is active", func() { ctx := testutil.ContextBackgroundWithNoOpLogger() migrationStrategy := virtv1.UpdateVolumesStrategyMigration