chore(vm): more messages about cpu and memory hotplug fails

- Do not start live migration to apply cpu or memory changes with hotplug if VM is NonMigratable.
- Fix sync_kvvm_test: fill PrintableStatus for kvvm as it is required to detect if disruptive changes may be applied.
- Add test for Upgrade*ChangesToRestart mutators.

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

Author: diafour

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

@@ -155,6 +155,10 @@ func (h *SyncKvvmHandler) Handle(ctx context.Context, s state.VirtualMachineStat
 			if kvvmiErr == nil && hasNonHotpluggableVolumes(kvvmi) {
 				changes.UpgradeBlockDeviceChangesToRestart()
 			}
+			// Require restart for CPU and memory changes if VM is non migratable.
+			if h.isVMNonMigratable(current) {
+				changes.UpgradeHotplugComputeChangesToRestart()
+			}
 			allChanges.Add(changes.GetAll()...)
 		}
 		if class != nil {
@@ -771,6 +775,13 @@ func (h *SyncKvvmHandler) isVMUnschedulable(
 	return false
 }
 
+func (h *SyncKvvmHandler) isVMNonMigratable(
+	vm *v1alpha2.VirtualMachine,
+) bool {
+	vmMigratable, has := conditions.GetCondition(vmcondition.TypeMigratable, vm.Status.Conditions)
+	return has && vmMigratable.Status == metav1.ConditionFalse
+}
+
 // isPlacementPolicyChanged returns true if any of the Affinity, NodePlacement, or Toleration rules have changed.
 func hasNonHotpluggableVolumes(kvvmi *virtv1.VirtualMachineInstance) bool {
 	if kvvmi == nil {

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

@@ -22,6 +22,7 @@ import (
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/ptr"
 	virtv1 "kubevirt.io/api/core/v1"
@@ -41,24 +42,24 @@ import (
 	"github.com/deckhouse/virtualization/api/core/v1alpha2/vmcondition"
 )
 
-var _ = Describe("SyncKvvmHandler", func() {
+var _ = FDescribe("SyncKvvmHandler", func() {
 	const (
 		name      = "vm-sync"
 		namespace = "default"
 	)
 
 	var (
-		ctx        context.Context
-		fakeClient client.WithWatch
-		resource   *reconciler.Resource[*v1alpha2.VirtualMachine, v1alpha2.VirtualMachineStatus]
-		vmState    state.VirtualMachineState
-		recorder   *eventrecord.EventRecorderLoggerMock
+		ctx          context.Context
+		fakeClient   client.WithWatch
+		reconcileObj *reconciler.Resource[*v1alpha2.VirtualMachine, v1alpha2.VirtualMachineStatus]
+		vmState      state.VirtualMachineState
+		recorder     *eventrecord.EventRecorderLoggerMock
 	)
 
 	BeforeEach(func() {
 		ctx = testutil.ContextBackgroundWithNoOpLogger()
 		fakeClient = nil
-		resource = nil
+		reconcileObj = nil
 		vmState = nil
 		recorder = &eventrecord.EventRecorderLoggerMock{
 			EventFunc:       func(_ client.Object, _, _, _ string) {},
@@ -69,27 +70,49 @@ var _ = Describe("SyncKvvmHandler", func() {
 
 	AfterEach(func() {
 		fakeClient = nil
-		resource = nil
+		reconcileObj = nil
 		vmState = nil
 		recorder = nil
 	})
 
-	newVM := func(phase v1alpha2.MachinePhase) *v1alpha2.VirtualMachine {
+	makeVM := func(phase v1alpha2.MachinePhase) *v1alpha2.VirtualMachine {
 		vm := vmbuilder.NewEmpty(name, namespace)
-		vm.Status.Phase = phase
+
 		vm.Spec.VirtualMachineClassName = "vmclass"
 		vm.Spec.CPU.Cores = 2
+		vm.Spec.Memory.Size = resource.MustParse("2Gi")
 		vm.Spec.RunPolicy = v1alpha2.ManualPolicy
 		vm.Spec.VirtualMachineIPAddress = "test-ip"
 		vm.Spec.OsType = v1alpha2.GenericOs
 		vm.Spec.Disruptions = &v1alpha2.Disruptions{
 			RestartApprovalMode: v1alpha2.Manual,
 		}
 
+		vm.Status.Phase = phase
+
 		return vm
 	}
 
-	newKVVM := func(vm *v1alpha2.VirtualMachine) *virtv1.VirtualMachine {
+	// It is like mapPhases in vm/internal/util.go but reversed.
+	mapVMPhaseToKVVMPrintableStatus := func(phase v1alpha2.MachinePhase) virtv1.VirtualMachinePrintableStatus {
+		switch phase {
+		case v1alpha2.MachineRunning:
+			return virtv1.VirtualMachineStatusRunning
+		case v1alpha2.MachineMigrating:
+			return virtv1.VirtualMachineStatusMigrating
+		case v1alpha2.MachineStopping:
+			return virtv1.VirtualMachineStatusStopping
+		case v1alpha2.MachineStopped:
+			return virtv1.VirtualMachineStatusStopped
+		case v1alpha2.MachineStarting:
+			return virtv1.VirtualMachineStatusProvisioning
+		case v1alpha2.MachinePending:
+			return virtv1.VirtualMachineStatusUnknown
+		}
+		return ""
+	}
+
+	makeKVVM := func(vm *v1alpha2.VirtualMachine) *virtv1.VirtualMachine {
 		kvvm := &virtv1.VirtualMachine{
 			ObjectMeta: metav1.ObjectMeta{
 				Name:      name,
@@ -101,11 +124,17 @@ var _ = Describe("SyncKvvmHandler", func() {
 		}
 		kvvm.Spec.RunStrategy = ptr.To(virtv1.RunStrategyAlways)
 
+		// Printable status is required for proper detection if changes are disruptive.
+		kvvm.Status.PrintableStatus = mapVMPhaseToKVVMPrintableStatus(vm.Status.Phase)
+
 		Expect(kvbuilder.SetLastAppliedSpec(kvvm, &v1alpha2.VirtualMachine{
 			Spec: v1alpha2.VirtualMachineSpec{
 				CPU: v1alpha2.CPUSpec{
 					Cores: vm.Spec.CPU.Cores,
 				},
+				Memory: v1alpha2.MemorySpec{
+					Size: vm.Spec.Memory.Size,
+				},
 				VirtualMachineIPAddress: vm.Spec.VirtualMachineIPAddress,
 				RunPolicy:               vm.Spec.RunPolicy,
 				OsType:                  vm.Spec.OsType,
@@ -132,16 +161,50 @@ var _ = Describe("SyncKvvmHandler", func() {
 		return kvvm
 	}
 
-	newKVVMI := func() *virtv1.VirtualMachineInstance {
+	makeKVVMI := func() *virtv1.VirtualMachineInstance {
 		kvvmi := newEmptyKVVMI(name, namespace)
 		return kvvmi
 	}
 
+	makeVMIP := func() *v1alpha2.VirtualMachineIPAddress {
+		return &v1alpha2.VirtualMachineIPAddress{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "test-ip",
+				Namespace: namespace,
+			},
+			Spec: v1alpha2.VirtualMachineIPAddressSpec{
+				Type:     v1alpha2.VirtualMachineIPAddressTypeStatic,
+				StaticIP: "192.168.1.10",
+			},
+			Status: v1alpha2.VirtualMachineIPAddressStatus{
+				Address: "192.168.1.10",
+				Phase:   v1alpha2.VirtualMachineIPAddressPhaseAttached,
+			},
+		}
+	}
+
+	makeVMClass := func() *v1alpha2.VirtualMachineClass {
+		return &v1alpha2.VirtualMachineClass{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "vmclass",
+			}, Spec: v1alpha2.VirtualMachineClassSpec{
+				CPU: v1alpha2.CPU{
+					Type: v1alpha2.CPUTypeHost,
+				},
+				NodeSelector: v1alpha2.NodeSelector{
+					MatchLabels: map[string]string{
+						"node1": "node1",
+					},
+				},
+			},
+		}
+	}
+
 	reconcile := func() {
 		h := NewSyncKvvmHandler(nil, fakeClient, recorder, featuregates.Default(), vmservice.NewMigrationVolumesService(fakeClient, MakeKVVMFromVMSpec, 10*time.Second))
 		_, err := h.Handle(ctx, vmState)
 		Expect(err).NotTo(HaveOccurred())
-		err = resource.Update(context.Background())
+		err = reconcileObj.Update(context.Background())
 		Expect(err).NotTo(HaveOccurred())
 	}
 
@@ -175,47 +238,33 @@ var _ = Describe("SyncKvvmHandler", func() {
 		})).To(Succeed())
 	}
 
+	mutateCPUCores := func(cores int) func(fakeClient client.WithWatch, vm *v1alpha2.VirtualMachine, kvvm *virtv1.VirtualMachine) {
+		return func(fakeClient client.WithWatch, vm *v1alpha2.VirtualMachine, kvvm *virtv1.VirtualMachine) {
+			vm.Spec.CPU.Cores = cores
+		}
+	}
+
+	mutateMemorySize := func(size string) func(fakeClient client.WithWatch, vm *v1alpha2.VirtualMachine, kvvm *virtv1.VirtualMachine) {
+		memSize := resource.MustParse(size)
+		return func(fakeClient client.WithWatch, vm *v1alpha2.VirtualMachine, kvvm *virtv1.VirtualMachine) {
+			vm.Spec.Memory.Size = memSize
+		}
+	}
+
 	DescribeTable("AwaitingRestart Condition Tests",
 		func(phase v1alpha2.MachinePhase, needChange bool, expectedStatus metav1.ConditionStatus, expectedExistence bool) {
-			ip := &v1alpha2.VirtualMachineIPAddress{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "test-ip",
-					Namespace: namespace,
-				},
-				Spec: v1alpha2.VirtualMachineIPAddressSpec{
-					Type:     v1alpha2.VirtualMachineIPAddressTypeStatic,
-					StaticIP: "192.168.1.10",
-				},
-				Status: v1alpha2.VirtualMachineIPAddressStatus{
-					Address: "192.168.1.10",
-					Phase:   v1alpha2.VirtualMachineIPAddressPhaseAttached,
-				},
-			}
+			ip := makeVMIP()
+			vmClass := makeVMClass()
 
-			vmClass := &v1alpha2.VirtualMachineClass{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "vmclass",
-				}, Spec: v1alpha2.VirtualMachineClassSpec{
-					CPU: v1alpha2.CPU{
-						Type: v1alpha2.CPUTypeHost,
-					},
-					NodeSelector: v1alpha2.NodeSelector{
-						MatchLabels: map[string]string{
-							"node1": "node1",
-						},
-					},
-				},
-			}
-
-			vm := newVM(phase)
-			kvvm := newKVVM(vm)
-			kvvmi := newKVVMI()
+			vm := makeVM(phase)
+			kvvm := makeKVVM(vm)
+			kvvmi := makeKVVMI()
 
 			if needChange {
 				mutateKVVM(vm, kvvm)
 			}
 
-			fakeClient, resource, vmState = setupEnvironment(vm, kvvm, kvvmi, ip, vmClass)
+			fakeClient, reconcileObj, vmState = setupEnvironment(vm, kvvm, kvvmi, ip, vmClass)
 
 			reconcile()
 
@@ -248,49 +297,59 @@ var _ = Describe("SyncKvvmHandler", func() {
 		Entry("Pending phase without changes, shouldn't have condition", v1alpha2.MachinePending, false, metav1.ConditionUnknown, false),
 	)
 
-	DescribeTable("ConfigurationApplied Condition Tests",
-		func(phase v1alpha2.MachinePhase, notReady bool, expectedStatus metav1.ConditionStatus, expectedExistence bool) {
-			ip := &v1alpha2.VirtualMachineIPAddress{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "test-ip",
-					Namespace: namespace,
-				},
-				Spec: v1alpha2.VirtualMachineIPAddressSpec{
-					Type:     v1alpha2.VirtualMachineIPAddressTypeStatic,
-					StaticIP: "192.168.1.10",
-				},
-				Status: v1alpha2.VirtualMachineIPAddressStatus{
-					Address: "192.168.1.10",
-					Phase:   v1alpha2.VirtualMachineIPAddressPhaseAttached,
-				},
+	DescribeTable("AwaitingRestart Condition for NonMigratable VM",
+		func(phase v1alpha2.MachinePhase, mutateFn func(fakeClient client.WithWatch, vm *v1alpha2.VirtualMachine, kvvm *virtv1.VirtualMachine), expectedStatus metav1.ConditionStatus, expectedExistence bool) {
+			ip := makeVMIP()
+			vmClass := makeVMClass()
+
+			vm := makeVM(phase)
+			vm.Status.Conditions = append(vm.Status.Conditions, metav1.Condition{
+				Type:   vmcondition.TypeMigratable.String(),
+				Status: metav1.ConditionFalse,
+				Reason: string(vmcondition.ReasonHostDevicesNotMigratable),
+			})
+			kvvm := makeKVVM(vm)
+			kvvmi := makeKVVMI()
+
+			if mutateFn != nil {
+				mutateFn(fakeClient, vm, kvvm)
 			}
 
-			vmClass := &v1alpha2.VirtualMachineClass{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "vmclass",
-				}, Spec: v1alpha2.VirtualMachineClassSpec{
-					CPU: v1alpha2.CPU{
-						Type: v1alpha2.CPUTypeHost,
-					},
-					NodeSelector: v1alpha2.NodeSelector{
-						MatchLabels: map[string]string{
-							"node1": "node1",
-						},
-					},
-				},
+			fakeClient, reconcileObj, vmState = setupEnvironment(vm, kvvm, kvvmi, ip, vmClass)
+
+			reconcile()
+
+			newVM := &v1alpha2.VirtualMachine{}
+			err := fakeClient.Get(ctx, client.ObjectKeyFromObject(vm), newVM)
+			Expect(err).NotTo(HaveOccurred())
+
+			awaitCond, awaitExists := conditions.GetCondition(vmcondition.TypeAwaitingRestartToApplyConfiguration, newVM.Status.Conditions)
+			Expect(awaitExists).To(Equal(expectedExistence))
+			if awaitExists {
+				Expect(awaitCond.Status).To(Equal(expectedStatus))
 			}
+		},
+		Entry("should present on hotplugging cpu.cores", v1alpha2.MachineRunning, mutateCPUCores(3), metav1.ConditionTrue, true),
+		Entry("should present on hotplugging memory.size", v1alpha2.MachineRunning, mutateMemorySize("4Gi"), metav1.ConditionTrue, true),
+	)
+
+	DescribeTable("ConfigurationApplied Condition Tests",
+		func(phase v1alpha2.MachinePhase, notReady bool, expectedStatus metav1.ConditionStatus, expectedExistence bool) {
+			ip := makeVMIP()
+			vmClass := makeVMClass()
 
-			vm := newVM(phase)
+			vm := makeVM(phase)
 			if notReady {
 				vm.Status.Conditions = append(vm.Status.Conditions, metav1.Condition{
 					Type:   vmcondition.TypeBlockDevicesReady.String(),
 					Status: metav1.ConditionFalse,
 					Reason: "BlockDevicesNotReady",
 				})
 			}
-			kvvm := newKVVM(vm)
+			kvvm := makeKVVM(vm)
+			kvvmi := makeKVVMI()
 
-			fakeClient, resource, vmState = setupEnvironment(vm, kvvm, ip, vmClass)
+			fakeClient, reconcileObj, vmState = setupEnvironment(vm, kvvm, kvvmi, ip, vmClass)
 			reconcile()
 
 			newVM := &v1alpha2.VirtualMachine{}

images/virtualization-artifact/pkg/controller/vmchange/comparator_cpu.go

@@ -28,6 +28,8 @@ type comparatorCPU struct {
 	featureGate featuregate.FeatureGate
 }
 
+const CPUPath = "cpu"
+
 func NewComparatorCPU(featureGate featuregate.FeatureGate) VMSpecFieldComparator {
 	return &comparatorCPU{
 		featureGate: featureGate,

images/virtualization-artifact/pkg/controller/vmchange/comparator_memory.go

@@ -25,6 +25,8 @@ import (
 	"github.com/deckhouse/virtualization/api/core/v1alpha2"
 )
 
+const MemoryPath = "memory"
+
 type comparatorMemory struct {
 	featureGate featuregate.FeatureGate
 }

images/virtualization-artifact/pkg/controller/vmchange/compare_test.go

@@ -561,6 +561,26 @@ networks:
 	}
 }
 
+// Test ApplyImmediate to Restart upgraders.
+func TestUpgradeHotplugComputeChangesToRestart(t *testing.T) {
+	const cpuReason = "cpu restart reason"
+
+	changes := SpecChanges{}
+	changes.Add(
+		FieldChange{Path: "cpu.cores", ActionRequired: ActionApplyImmediate, RestartMessage: cpuReason},
+		FieldChange{Path: "memory.size", ActionRequired: ActionApplyImmediate},
+		FieldChange{Path: "blockDeviceRefs.0", ActionRequired: ActionApplyImmediate},
+	)
+
+	changes.UpgradeHotplugComputeChangesToRestart()
+	changes.UpgradeBlockDeviceChangesToRestart()
+
+	require.Equal(t, ActionRestart, changes.GetAll()[0].ActionRequired)
+	require.Equal(t, cpuReason, changes.GetAll()[0].RestartMessage)
+	require.Equal(t, ActionRestart, changes.GetAll()[1].ActionRequired)
+	require.Equal(t, ActionRestart, changes.GetAll()[2].ActionRequired)
+}
+
 func loadVMSpec(t *testing.T, inYAML string) *v1alpha2.VirtualMachineSpec {
 	t.Helper()
 	var spec v1alpha2.VirtualMachineSpec