++ code cleanups, add unit and e2e tests, fix nil pointer

Signed-off-by: Ivan Mikheykin <ivan.mikheykin@flant.com>

Author: diafour

images/virtualization-artifact/pkg/config/load_live_migration_settings.go

@@ -22,6 +22,8 @@ import (
 	"strconv"
 
 	"k8s.io/apimachinery/pkg/api/resource"
+
+	"github.com/deckhouse/virtualization/api/core/v1alpha2"
 )
 
 const (
@@ -43,6 +45,7 @@ const (
 	DefaultBandwidthPerNode     = "64Mi"
 	DefaultMaxMigrationsPerNode = 2
 	DefaultNetwork              = LiveMigrationNetworkShared
+	DefaultLiveMigrationPolicy  = v1alpha2.PreferSafeMigrationPolicy
 )
 
 type LiveMigrationSettings struct {

images/virtualization-artifact/pkg/controller/livemigration/internal/dynamic_settings_handler.go

@@ -27,7 +27,7 @@ import (
 	"github.com/deckhouse/virtualization-controller/pkg/config"
 	internalservice "github.com/deckhouse/virtualization-controller/pkg/controller/livemigration/internal/service"
 	"github.com/deckhouse/virtualization-controller/pkg/logger"
-	virtv2 "github.com/deckhouse/virtualization/api/core/v1alpha2"
+	"github.com/deckhouse/virtualization/api/core/v1alpha2"
 )
 
 const dynamicSettingsHandlerName = "DynamicSettingsHandler"
@@ -51,13 +51,7 @@ func (h *DynamicSettingsHandler) Handle(ctx context.Context, kvvmi *virtv1.Virtu
 		return reconcile.Result{}, nil
 	}
 
-	// Get vm
-	// Get vmop
-	// Merge live migration policies.
-	// Add global live migration settings.
-	// Patch vmi status with migrationConfiguration.
-
-	var vm virtv2.VirtualMachine
+	var vm v1alpha2.VirtualMachine
 	vmKey := types.NamespacedName{
 		Namespace: kvvmi.Namespace,
 		Name:      kvvmi.Name,
@@ -73,39 +67,17 @@ func (h *DynamicSettingsHandler) Handle(ctx context.Context, kvvmi *virtv1.Virtu
 		return reconcile.Result{}, err
 	}
 
-	// Merge
-	// Detect policy: use default from vmclass if no policy on vm or policy is not allowed.
-	// Use policy from vm if contains in allowed.
-	// Set initial autoconverge value depending on policy.
-	// Use force flag from vmop to override autoconverge flag for Manual, PreferSafe. Can't implement default force:true for PreferForced, do not override for now.
-
-	vmPolicy := virtv2.PreferSafeMigrationPolicy
-	if vm.Spec.LiveMigrationPolicy != "" {
-		vmPolicy = vm.Spec.LiveMigrationPolicy
-	}
-
-	// Calculate final autoConverge value.
-	autoConverge := false
-	switch vmPolicy {
-	case virtv2.PreferSafeMigrationPolicy:
-		// User may override autoConverge with vmop.
-		if vmopInProgress.Spec.Force != nil {
-			autoConverge = *vmopInProgress.Spec.Force
-		}
-	case virtv2.PreferForcedMigrationPolicy:
-		autoConverge = true
-		if vmopInProgress.Spec.Force != nil {
-			autoConverge = *vmopInProgress.Spec.Force
-		}
-	case virtv2.AlwaysForcedMigrationPolicy:
-		autoConverge = true
-	}
+	effectivePolicy, autoConverge := internalservice.CalculateEffectivePolicy(vm, vmopInProgress)
 
 	conf := internalservice.NewMigrationConfiguration(h.moduleSettings, autoConverge)
 
 	kvvmi.Status.MigrationState.MigrationConfiguration = conf
 
-	log.Info("Patch KVVMI with migration settings", "migration.configuration", internalservice.DumpKVVMIMigrationConfiguration(kvvmi), "conf", internalservice.DumpMigrationConfiguration(conf))
+	log.Debug("Set migrationConfiguration on KVVMI",
+		"migrationConfiguration", internalservice.DumpKVVMIMigrationConfiguration(kvvmi),
+		"policy", effectivePolicy,
+		"autoConverge", autoConverge,
+	)
 
 	return reconcile.Result{}, nil
 }
@@ -124,8 +96,8 @@ func (h *DynamicSettingsHandler) shouldUpdateMigrationConfiguration(kvvmi *virtv
 }
 
 // GetVMOPInProgressForVM check if there is at least one VMOP for the same VM in progress phase.
-func (h *DynamicSettingsHandler) GetVMOPInProgressForVM(ctx context.Context, vmKey client.ObjectKey) (*virtv2.VirtualMachineOperation, error) {
-	var vmopList virtv2.VirtualMachineOperationList
+func (h *DynamicSettingsHandler) GetVMOPInProgressForVM(ctx context.Context, vmKey client.ObjectKey) (*v1alpha2.VirtualMachineOperation, error) {
+	var vmopList v1alpha2.VirtualMachineOperationList
 	err := h.client.List(ctx, &vmopList, client.InNamespace(vmKey.Namespace))
 	if err != nil {
 		return nil, err
@@ -138,7 +110,7 @@ func (h *DynamicSettingsHandler) GetVMOPInProgressForVM(ctx context.Context, vmK
 		}
 
 		// Return if VMOP has phase InProgress.
-		if vmop.Status.Phase == virtv2.VMOPPhaseInProgress {
+		if vmop.Status.Phase == v1alpha2.VMOPPhaseInProgress {
 			return &vmop, nil
 		}
 	}

images/virtualization-artifact/pkg/controller/livemigration/internal/dynamic_settings_handler_test.go

@@ -0,0 +1,122 @@
+package internal
+
+import (
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/utils/ptr"
+	virtv1 "kubevirt.io/api/core/v1"
+
+	vmbuilder "github.com/deckhouse/virtualization-controller/pkg/builder/vm"
+	"github.com/deckhouse/virtualization-controller/pkg/common/testutil"
+	"github.com/deckhouse/virtualization-controller/pkg/config"
+	"github.com/deckhouse/virtualization/api/core/v1alpha2"
+)
+
+var _ = Describe("TestEvacuationHandler", func() {
+	const (
+		vmName      = "vm-migratable"
+		vmNamespace = "default"
+	)
+
+	ctx := testutil.ContextBackgroundWithNoOpLogger()
+
+	newVM := func() *v1alpha2.VirtualMachine {
+		vm := vmbuilder.NewEmpty(vmName, vmNamespace)
+		vm.Spec.LiveMigrationPolicy = v1alpha2.PreferSafeMigrationPolicy
+
+		return vm
+	}
+
+	newKVVMI := func() *virtv1.VirtualMachineInstance {
+		vmi := &virtv1.VirtualMachineInstance{
+			TypeMeta: metav1.TypeMeta{
+				APIVersion: virtv1.SchemeGroupVersion.String(),
+				Kind:       virtv1.VirtualMachineInstanceGroupVersionKind.Kind,
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      vmName,
+				Namespace: vmNamespace,
+			},
+		}
+		return vmi
+	}
+
+	newVMOPEvict := func(force *bool) *v1alpha2.VirtualMachineOperation {
+		vmop := &v1alpha2.VirtualMachineOperation{
+			TypeMeta: metav1.TypeMeta{
+				APIVersion: v1alpha2.SchemeGroupVersion.String(),
+				Kind:       v1alpha2.VirtualMachineOperationKind,
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "some-vmop-name",
+				Namespace: vmNamespace,
+			},
+			Spec: v1alpha2.VirtualMachineOperationSpec{
+				Type:           v1alpha2.VMOPTypeEvict,
+				VirtualMachine: vmName,
+				Force:          force,
+			},
+			Status: v1alpha2.VirtualMachineOperationStatus{
+				Phase: v1alpha2.VMOPPhaseInProgress,
+			},
+		}
+		return vmop
+	}
+
+	When("Observe KVVMI with migrateState", func() {
+		It("Should set migrationConfiguration", func() {
+			vm := newVM()
+			kvvmi := newKVVMI()
+
+			kvvmi.Status.MigrationState = &virtv1.VirtualMachineInstanceMigrationState{}
+
+			fakeClient := setupEnvironment(kvvmi, vm)
+			h := NewDynamicSettingsHandler(fakeClient, config.NewDefaultLiveMigrationSettings())
+			_, err := h.Handle(ctx, kvvmi)
+			Expect(err).NotTo(HaveOccurred())
+
+			Expect(kvvmi.Status.MigrationState.MigrationConfiguration).ShouldNot(BeNil(), "Should set migrationConfiguration")
+		})
+	})
+
+	When("Observe KVVMI with completed migration", func() {
+		It("Should not set migrationConfiguration", func() {
+			vm := newVM()
+			kvvmi := newKVVMI()
+
+			kvvmi.Status.MigrationState = &virtv1.VirtualMachineInstanceMigrationState{
+				Completed: true,
+			}
+
+			fakeClient := setupEnvironment(kvvmi, vm)
+			h := NewDynamicSettingsHandler(fakeClient, config.NewDefaultLiveMigrationSettings())
+			_, err := h.Handle(ctx, kvvmi)
+			Expect(err).NotTo(HaveOccurred())
+
+			Expect(kvvmi.Status.MigrationState.MigrationConfiguration).Should(BeNil(), "Should not set migrationConfiguration")
+		})
+	})
+
+	DescribeTable("When migration started with VMOP and force flag",
+		func(policy v1alpha2.LiveMigrationPolicy, force *bool) {
+			vm := newVM()
+			vm.Spec.LiveMigrationPolicy = policy
+
+			kvvmi := newKVVMI()
+			kvvmi.Status.MigrationState = &virtv1.VirtualMachineInstanceMigrationState{}
+
+			vmop := newVMOPEvict(force)
+
+			fakeClient := setupEnvironment(kvvmi, vm, vmop)
+			h := NewDynamicSettingsHandler(fakeClient, config.NewDefaultLiveMigrationSettings())
+			_, err := h.Handle(ctx, kvvmi)
+			Expect(err).NotTo(HaveOccurred())
+
+			Expect(kvvmi.Status.MigrationState.MigrationConfiguration).ShouldNot(BeNil(), "Should set migrationConfiguration")
+			Expect(kvvmi.Status.MigrationState.MigrationConfiguration.AllowAutoConverge).Should(Equal(force))
+		},
+		Entry("Should enable autoConverge for PreferSafe policy and force=true", v1alpha2.PreferSafeMigrationPolicy, ptr.To(true)),
+		Entry("Should disable autoConverge for PreferForced policy and force=false", v1alpha2.PreferForcedMigrationPolicy, ptr.To(false)),
+	)
+})

images/virtualization-artifact/pkg/controller/livemigration/internal/service/migration_configuration.go

@@ -63,14 +63,6 @@ func NewMigrationConfiguration(moduleSettings config.LiveMigrationSettings, allo
 	}
 }
 
-func DumpMigrationConfiguration(conf *virtv1.MigrationConfiguration) string {
-	out, err := json.Marshal(conf)
-	if err != nil {
-		return fmt.Sprintf("(json err %v, fmt dump)%#v", err, conf)
-	}
-	return string(out)
-}
-
 func DumpKVVMIMigrationConfiguration(kvvmi *virtv1.VirtualMachineInstance) string {
 	if kvvmi.Status.MigrationState == nil {
 		return "status.migrationState is null"
@@ -86,7 +78,8 @@ func DumpKVVMIMigrationConfiguration(kvvmi *virtv1.VirtualMachineInstance) strin
 	return string(out)
 }
 
-func IsMigrationConfigurationChanged(current *virtv1.VirtualMachineInstance, changed *virtv1.VirtualMachineInstance) bool {
+// IsMigrationConfigurationChanged detects if MigrationConfiguration was changed.
+func IsMigrationConfigurationChanged(current, changed *virtv1.VirtualMachineInstance) bool {
 	// true if migrationConfiguration was added.
 	if current.Status.MigrationState != nil && current.Status.MigrationState.MigrationConfiguration == nil &&
 		changed.Status.MigrationState != nil && changed.Status.MigrationState.MigrationConfiguration != nil {

images/virtualization-artifact/pkg/controller/livemigration/internal/service/policy.go

@@ -0,0 +1,50 @@
+package service
+
+import (
+	"k8s.io/utils/ptr"
+
+	"github.com/deckhouse/virtualization-controller/pkg/config"
+	"github.com/deckhouse/virtualization/api/core/v1alpha2"
+)
+
+func AutoConvergeForPolicy(policy v1alpha2.LiveMigrationPolicy) (autoConverge *bool) {
+	switch policy {
+	case v1alpha2.AlwaysSafeMigrationPolicy, v1alpha2.PreferSafeMigrationPolicy:
+		return ptr.To(false)
+	case v1alpha2.AlwaysForcedMigrationPolicy, v1alpha2.PreferForcedMigrationPolicy:
+		return ptr.To(true)
+	}
+	return nil
+}
+
+// CalculateEffectivePolicy merges live migration policy from default value and from VM.
+// Also, autoConverge value may be overridden from VMOP.
+func CalculateEffectivePolicy(vm v1alpha2.VirtualMachine, vmop *v1alpha2.VirtualMachineOperation) (effectivePolicy v1alpha2.LiveMigrationPolicy, autoConverge bool) {
+	effectivePolicy = config.DefaultLiveMigrationPolicy
+
+	if vm.Spec.LiveMigrationPolicy != "" {
+		effectivePolicy = vm.Spec.LiveMigrationPolicy
+	}
+
+	autoConvergePtr := AutoConvergeForPolicy(effectivePolicy)
+
+	// Override autoConverge value.
+	if vmop != nil {
+		switch effectivePolicy {
+		case v1alpha2.PreferSafeMigrationPolicy,
+			v1alpha2.PreferForcedMigrationPolicy:
+			if vmop.Spec.Force != nil {
+				autoConvergePtr = vmop.Spec.Force
+			}
+		}
+	}
+
+	if autoConvergePtr == nil {
+		// Should not be reached. Disable autoConverge is safe.
+		autoConverge = false
+	} else {
+		autoConverge = *autoConvergePtr
+	}
+
+	return
+}

images/virtualization-artifact/pkg/controller/livemigration/internal/suite_test.go

@@ -0,0 +1,29 @@
+package internal
+
+import (
+	"testing"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	virtv1 "kubevirt.io/api/core/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	"github.com/deckhouse/virtualization-controller/pkg/common/testutil"
+)
+
+func TestHandlers(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Live migration handlers suite")
+}
+
+func setupEnvironment(kvvmi *virtv1.VirtualMachineInstance, objs ...client.Object) client.WithWatch {
+	GinkgoHelper()
+	Expect(kvvmi).ToNot(BeNil(), "Should set KVVMI for setupEnvironment")
+	allObjects := []client.Object{kvvmi}
+	allObjects = append(allObjects, objs...)
+
+	fakeClient, err := testutil.NewFakeClientWithObjects(allObjects...)
+	Expect(err).NotTo(HaveOccurred())
+
+	return fakeClient
+}

images/virtualization-artifact/pkg/controller/livemigration/internal/watcher/kvvmi.go

@@ -0,0 +1,56 @@
+/*
+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 watcher
+
+import (
+	"fmt"
+	"reflect"
+
+	virtv1 "kubevirt.io/api/core/v1"
+	"sigs.k8s.io/controller-runtime/pkg/controller"
+	"sigs.k8s.io/controller-runtime/pkg/event"
+	"sigs.k8s.io/controller-runtime/pkg/handler"
+	"sigs.k8s.io/controller-runtime/pkg/manager"
+	"sigs.k8s.io/controller-runtime/pkg/predicate"
+	"sigs.k8s.io/controller-runtime/pkg/source"
+)
+
+func NewKVVMIWatcher() *KVVMIWatcher {
+	return &KVVMIWatcher{}
+}
+
+type KVVMIWatcher struct{}
+
+func (w *KVVMIWatcher) Watch(mgr manager.Manager, ctr controller.Controller) error {
+	// Subscribe to KVVMI status updates.
+	if err := ctr.Watch(
+		source.Kind(mgr.GetCache(), &virtv1.VirtualMachineInstance{}),
+		&handler.EnqueueRequestForObject{},
+		predicate.Funcs{
+			CreateFunc: func(e event.CreateEvent) bool { return false },
+			DeleteFunc: func(e event.DeleteEvent) bool { return false },
+			UpdateFunc: func(e event.UpdateEvent) bool {
+				oldVMI := e.ObjectOld.(*virtv1.VirtualMachineInstance)
+				newVMI := e.ObjectNew.(*virtv1.VirtualMachineInstance)
+				return !reflect.DeepEqual(oldVMI.Status, newVMI.Status)
+			},
+		},
+	); err != nil {
+		return fmt.Errorf("error setting watch on VirtualMachineInstance: %w", err)
+	}
+	return nil
+}