Backport: fix(vm): fix some volume migration bugs (#1505)

fix(vm): fix some volume migration bugs (#1498)

This PR fixes several critical bugs related to volume migration in VM operations:

1. Fixed Migrating Condition and Generation Sync
Problem: Incorrect handling of the Migrating condition and lack of proper synchronization.
Solution: Added proper synchronization logic and correct setting of observedGeneration to ensure state consistency.
2. Blocked RWO Volume Migration with Immediate Failure
Problem: Attempting to migrate VM with RWO (ReadWriteOnce) volumes was causing undefined behavior.
Solution: VMOP now immediately fails the migration operation when RWO volumes are detected, as this configuration is not supported. Added e2e test.
3. Fixed Pending State Hang and Volume Sync on KVVM
Problem: VM operations could hang indefinitely in Pending state due to volume synchronization issues on KVVM.
Solution: Implemented proper volume synchronization logic to prevent hanging and ensure smooth state transitions.
4. Prevented Unplanned Restarts from Unexpected State
Problem: Presence of unexpected state could trigger unplanned VM restarts.
Solution: Added validation and handling logic to prevent restarts caused by unexpected state conditions.

Signed-off-by: Yaroslav Borbat <[email protected]>
Co-authored-by: Yaroslav Borbat <[email protected]>

Author: deckhouse-BOaTswain

images/virtualization-artifact/pkg/builder/vmbda/option.go

@@ -0,0 +1,50 @@
+/*
+Copyright 2025 Flant JSC
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+     http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package vmbda
+
+import (
+	"github.com/deckhouse/virtualization-controller/pkg/builder/meta"
+	"github.com/deckhouse/virtualization/api/core/v1alpha2"
+)
+
+type Option func(vd *v1alpha2.VirtualMachineBlockDeviceAttachment)
+
+var (
+	WithName         = meta.WithName[*v1alpha2.VirtualMachineBlockDeviceAttachment]
+	WithNamespace    = meta.WithNamespace[*v1alpha2.VirtualMachineBlockDeviceAttachment]
+	WithGenerateName = meta.WithGenerateName[*v1alpha2.VirtualMachineBlockDeviceAttachment]
+	WithLabel        = meta.WithLabel[*v1alpha2.VirtualMachineBlockDeviceAttachment]
+	WithLabels       = meta.WithLabels[*v1alpha2.VirtualMachineBlockDeviceAttachment]
+	WithAnnotation   = meta.WithAnnotation[*v1alpha2.VirtualMachineBlockDeviceAttachment]
+	WithAnnotations  = meta.WithAnnotations[*v1alpha2.VirtualMachineBlockDeviceAttachment]
+	WithFinalizer    = meta.WithFinalizer[*v1alpha2.VirtualMachineBlockDeviceAttachment]
+)
+
+func WithVirtualMachineName(name string) func(vmbda *v1alpha2.VirtualMachineBlockDeviceAttachment) {
+	return func(vmbda *v1alpha2.VirtualMachineBlockDeviceAttachment) {
+		vmbda.Spec.VirtualMachineName = name
+	}
+}
+
+func WithBlockDeviceRef(kind v1alpha2.VMBDAObjectRefKind, name string) func(vmbda *v1alpha2.VirtualMachineBlockDeviceAttachment) {
+	return func(vmbda *v1alpha2.VirtualMachineBlockDeviceAttachment) {
+		vmbda.Spec.BlockDeviceRef = v1alpha2.VMBDAObjectRef{
+			Kind: kind,
+			Name: name,
+		}
+	}
+}

images/virtualization-artifact/pkg/builder/vmbda/vd.go

@@ -0,0 +1,51 @@
+/*
+Copyright 2025 Flant JSC
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+     http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package vmbda
+
+import (
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	"github.com/deckhouse/virtualization/api/core/v1alpha2"
+)
+
+func New(options ...Option) *v1alpha2.VirtualMachineBlockDeviceAttachment {
+	vmbda := NewEmpty("", "")
+	ApplyOptions(vmbda, options...)
+	return vmbda
+}
+
+func ApplyOptions(vmbda *v1alpha2.VirtualMachineBlockDeviceAttachment, opts ...Option) {
+	if vmbda == nil {
+		return
+	}
+	for _, opt := range opts {
+		opt(vmbda)
+	}
+}
+
+func NewEmpty(name, namespace string) *v1alpha2.VirtualMachineBlockDeviceAttachment {
+	return &v1alpha2.VirtualMachineBlockDeviceAttachment{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: v1alpha2.SchemeGroupVersion.String(),
+			Kind:       v1alpha2.VirtualMachineBlockDeviceAttachmentKind,
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name,
+			Namespace: namespace,
+		},
+	}
+}

images/virtualization-artifact/pkg/common/vmop/vmop.go

@@ -17,7 +17,11 @@ limitations under the License.
 package vmop
 
 import (
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	"github.com/deckhouse/virtualization-controller/pkg/controller/conditions"
 	"github.com/deckhouse/virtualization/api/core/v1alpha2"
+	"github.com/deckhouse/virtualization/api/core/v1alpha2/vmopcondition"
 )
 
 func IsInProgressOrPending(vmop *v1alpha2.VirtualMachineOperation) bool {
@@ -44,3 +48,8 @@ func InProgressOrPendingExists(vmops []v1alpha2.VirtualMachineOperation) bool {
 	}
 	return false
 }
+
+func IsOperationInProgress(vmop *v1alpha2.VirtualMachineOperation) bool {
+	sent, _ := conditions.GetCondition(vmopcondition.TypeSignalSent, vmop.Status.Conditions)
+	return sent.Status == metav1.ConditionTrue && !IsFinished(vmop)
+}

images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go

@@ -259,7 +259,7 @@ func ApplyVirtualMachineSpec(
 
 func ApplyMigrationVolumes(kvvm *KVVM, vm *virtv2.VirtualMachine, vdsByName map[string]*virtv2.VirtualDisk) error {
 	bootOrder := uint(1)
-	var updateVolumesStrategy *virtv1.UpdateVolumesStrategy
+	var updateVolumesStrategy *virtv1.UpdateVolumesStrategy = nil
 
 	for _, bd := range vm.Spec.BlockDeviceRefs {
 		if bd.Kind != virtv2.DiskDevice {

images/virtualization-artifact/pkg/controller/vd/internal/life_cycle.go

@@ -19,7 +19,6 @@ package internal
 import (
 	"context"
 	"fmt"
-	"time"
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -153,10 +152,5 @@ func (h LifeCycleHandler) Handle(ctx context.Context, vd *virtv2.VirtualDisk) (r
 		return reconcile.Result{}, fmt.Errorf("failed to sync virtual disk data source %s: %w", ds.Name(), err)
 	}
 
-	readyConditionAfterSync, _ := conditions.GetCondition(vdcondition.ReadyType, vd.Status.Conditions)
-	if readyConditionAfterSync.Status == metav1.ConditionTrue && conditions.IsLastUpdated(readyConditionAfterSync, vd) {
-		return reconcile.Result{RequeueAfter: 1 * time.Second}, nil
-	}
-
 	return result, nil
 }

images/virtualization-artifact/pkg/controller/vd/internal/migration.go

@@ -410,7 +410,7 @@ func (h MigrationHandler) handleMigratePrepareTarget(ctx context.Context, vd *v1
 		StartTimestamp: metav1.Now(),
 	}
 
-	cb.Status(metav1.ConditionTrue).
+	cb.Status(metav1.ConditionFalse).
 		Reason(vdcondition.MigratingWaitForTargetReadyReason).
 		Message("Migration started.")
 	conditions.SetCondition(cb, &vd.Status.Conditions)
@@ -426,19 +426,17 @@ func (h MigrationHandler) handleMigrateSync(ctx context.Context, vd *v1alpha2.Vi
 
 	cb := conditions.NewConditionBuilder(vdcondition.MigratingType).
 		Generation(vd.Generation).
-		Status(metav1.ConditionTrue).
+		Status(metav1.ConditionFalse).
 		Reason(vdcondition.MigratingWaitForTargetReadyReason)
 
 	if pvc == nil {
-		cb.Status(metav1.ConditionFalse).
-			Reason(vdcondition.MigratingWaitForTargetReadyReason).
-			Message("Target persistent volume claim is not found.")
+		cb.Message("Target persistent volume claim is not found.")
 		conditions.SetCondition(cb, &vd.Status.Conditions)
 		return nil
 	}
 
 	if pvc.Status.Phase == corev1.ClaimBound {
-		cb.Reason(vdcondition.InProgress).Message("Target persistent volume claim is bound.")
+		cb.Status(metav1.ConditionTrue).Reason(vdcondition.InProgress).Message("Target persistent volume claim is bound.")
 		conditions.SetCondition(cb, &vd.Status.Conditions)
 		return nil
 	}
@@ -467,15 +465,13 @@ func (h MigrationHandler) handleMigrateSync(ctx context.Context, vd *v1alpha2.Vi
 
 		isWaitForFistConsumer := sc.VolumeBindingMode == nil || *sc.VolumeBindingMode == storev1.VolumeBindingWaitForFirstConsumer
 		if isWaitForFistConsumer {
-			cb.Reason(vdcondition.InProgress).Message("Target persistent volume claim is waiting for first consumer.")
+			cb.Status(metav1.ConditionTrue).Reason(vdcondition.InProgress).Message("Target persistent volume claim is waiting for first consumer.")
 			conditions.SetCondition(cb, &vd.Status.Conditions)
 			return nil
 		}
 	}
 
-	cb.Status(metav1.ConditionFalse).
-		Reason(vdcondition.MigratingWaitForTargetReadyReason).
-		Message("Target persistent volume claim is not bound or not waiting for first consumer.")
+	cb.Message("Target persistent volume claim is not bound or not waiting for first consumer.")
 	conditions.SetCondition(cb, &vd.Status.Conditions)
 	return nil
 }

images/virtualization-artifact/pkg/controller/vm/internal/migrating.go

@@ -17,8 +17,10 @@ limitations under the License.
 package internal
 
 import (
+	"cmp"
 	"context"
 	"fmt"
+	"slices"
 	"strings"
 
 	corev1 "k8s.io/api/core/v1"
@@ -285,21 +287,30 @@ func (h *MigratingHandler) getVMOPCandidate(ctx context.Context, s state.Virtual
 		return nil, err
 	}
 
-	var candidate *virtv2.VirtualMachineOperation
-	if len(vmops) > 0 {
-		candidate = vmops[0]
+	if len(vmops) == 0 {
+		return nil, nil
+	}
 
-		for _, vmop := range vmops {
-			if !commonvmop.IsMigration(vmop) {
-				continue
-			}
-			if vmop.GetCreationTimestamp().Time.After(candidate.GetCreationTimestamp().Time) {
-				candidate = vmop
-			}
+	// sort vmops from the oldest to the newest
+	slices.SortFunc(vmops, func(a, b *virtv2.VirtualMachineOperation) int {
+		return cmp.Compare(a.GetCreationTimestamp().UnixNano(), b.GetCreationTimestamp().UnixNano())
+	})
+
+	migrations := slices.DeleteFunc(vmops, func(vmop *virtv2.VirtualMachineOperation) bool {
+		return !commonvmop.IsMigration(vmop)
+	})
+
+	for _, migration := range migrations {
+		if commonvmop.IsInProgressOrPending(migration) {
+			return migration, nil
 		}
 	}
 
-	return candidate, nil
+	if len(migrations) > 0 {
+		return migrations[len(migrations)-1], nil
+	}
+
+	return nil, nil
 }
 
 func (h *MigratingHandler) syncMigratable(ctx context.Context, s state.VirtualMachineState, vm *virtv2.VirtualMachine, kvvm *virtv1.VirtualMachine) error {