fix(vm): keep the volume migration set atomic and finalized

images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go

@@ -119,13 +119,35 @@ 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 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.")
+			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
 	}
 
+	// 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) {
+		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
@@ -149,23 +171,16 @@ 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
 		}
 		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.")
@@ -177,6 +192,22 @@ func (s MigrationVolumesService) SyncVolumes(ctx context.Context, vmState state.
 	}
 
 	if migrationRequested {
+		// 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: 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
+		}
+
 		// 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]
@@ -224,10 +255,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 {
@@ -283,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
 	}
@@ -554,6 +589,68 @@ func (s MigrationVolumesService) makeKVVMFromVirtualMachineSpec(ctx context.Cont
 	return kvvm, kvvmWithMigrationVolumes, nil
 }
 
+// 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) {
+			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 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 {
+		if v.PersistentVolumeClaim != nil {
+			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 {
+			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
+		}
+		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
+}
+
 // 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 {