fix(vm): avoid restartAwaitingChanges flicker for hotplug block devices

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

@@ -159,8 +159,11 @@ func (h *SyncKvvmHandler) Handle(ctx context.Context, s state.VirtualMachineStat
 		changes = h.detectSpecChanges(ctx, kvvm, &current.Spec, lastAppliedSpec)
 		if !changes.IsEmpty() {
 			kvvmi, kvvmiErr := s.KVVMI(ctx)
-			if kvvmiErr == nil && hasNonHotpluggableVolumes(kvvmi) {
-				changes.UpgradeBlockDeviceChangesToRestart()
+			if kvvmiErr == nil {
+				nonHotpluggableVolumes := nonHotpluggableVolumeRefs(kvvmi)
+				changes.UpgradeBlockDeviceChangesToRestartIf(func(change vmchange.FieldChange) bool {
+					return blockDeviceChangeTouchesRefs(change, nonHotpluggableVolumes)
+				})
 			}
 			// Require restart for CPU and memory changes if VM is non migratable.
 			if h.isVMNonMigratable(current) {
@@ -193,7 +196,7 @@ func (h *SyncKvvmHandler) Handle(ctx context.Context, s state.VirtualMachineStat
 	if kvvm == nil || changes.IsEmpty() {
 		changed.Status.RestartAwaitingChanges = nil
 	} else {
-		changed.Status.RestartAwaitingChanges, err = changes.ConvertPendingChanges()
+		changed.Status.RestartAwaitingChanges, err = changes.ConvertPendingRestartChanges()
 		if err != nil {
 			err = fmt.Errorf("failed to generate pending configuration changes: %w", err)
 			cbConfApplied.
@@ -202,6 +205,9 @@ func (h *SyncKvvmHandler) Handle(ctx context.Context, s state.VirtualMachineStat
 				Message(service.CapitalizeFirstLetter(err.Error()) + ".")
 			return reconcile.Result{}, err
 		}
+		if len(changed.Status.RestartAwaitingChanges) == 0 {
+			changed.Status.RestartAwaitingChanges = nil
+		}
 	}
 
 	// 2. Wait if dependent resources are not ready yet.
@@ -1193,20 +1199,56 @@ func (h *SyncKvvmHandler) patchPodNetworkAnnotation(ctx context.Context, s state
 	return desired, nil
 }
 
-// isPlacementPolicyChanged returns true if any of the Affinity, NodePlacement, or Toleration rules have changed.
-func hasNonHotpluggableVolumes(kvvmi *virtv1.VirtualMachineInstance) bool {
+func nonHotpluggableVolumeRefs(kvvmi *virtv1.VirtualMachineInstance) map[nameKindKey]struct{} {
+	refs := make(map[nameKindKey]struct{})
 	if kvvmi == nil {
-		return false
+		return refs
 	}
+
 	for _, v := range kvvmi.Spec.Volumes {
 		if v.PersistentVolumeClaim != nil && !v.PersistentVolumeClaim.Hotpluggable ||
 			v.ContainerDisk != nil && !v.ContainerDisk.Hotpluggable {
+			name, kind := kvbuilder.GetOriginalDiskName(v.Name)
+			if kind == "" {
+				continue
+			}
+			refs[nameKindKey{kind: kind, name: name}] = struct{}{}
+		}
+	}
+
+	return refs
+}
+
+func blockDeviceChangeTouchesRefs(change vmchange.FieldChange, refs map[nameKindKey]struct{}) bool {
+	if len(refs) == 0 {
+		return false
+	}
+
+	for _, ref := range blockDeviceRefsFromValue(change.CurrentValue) {
+		if _, ok := refs[nameKindKey{kind: ref.Kind, name: ref.Name}]; ok {
 			return true
 		}
 	}
+	for _, ref := range blockDeviceRefsFromValue(change.DesiredValue) {
+		if _, ok := refs[nameKindKey{kind: ref.Kind, name: ref.Name}]; ok {
+			return true
+		}
+	}
+
 	return false
 }
 
+func blockDeviceRefsFromValue(value interface{}) []v1alpha2.BlockDeviceSpecRef {
+	switch v := value.(type) {
+	case v1alpha2.BlockDeviceSpecRef:
+		return []v1alpha2.BlockDeviceSpecRef{v}
+	case []v1alpha2.BlockDeviceSpecRef:
+		return v
+	default:
+		return nil
+	}
+}
+
 func (h *SyncKvvmHandler) isPlacementPolicyChanged(allChanges vmchange.SpecChanges) bool {
 	for _, c := range allChanges.GetAll() {
 		switch c.Path {

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

@@ -542,6 +542,56 @@ var _ = Describe("SyncKvvmHandler", func() {
 		),
 	)
 
+	It("does not expose apply-immediate block device changes in RestartAwaitingChanges while dependencies are not ready", func() {
+		ip := makeVMIP()
+		vmClass := makeVMClass()
+
+		rootRef := v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "root"}
+		blankRef := v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "blank-disk"}
+
+		vm := makeVM(v1alpha2.MachineRunning)
+		vm.Spec.EnableParavirtualization = ptr.To(true)
+		vm.Spec.BlockDeviceRefs = []v1alpha2.BlockDeviceSpecRef{rootRef, blankRef}
+		vm.Status.Conditions = append(vm.Status.Conditions, metav1.Condition{
+			Type:   vmcondition.TypeBlockDevicesReady.String(),
+			Status: metav1.ConditionFalse,
+			Reason: "BlockDevicesNotReady",
+		})
+
+		kvvm := makeKVVM(vm)
+		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,
+				VirtualMachineClassName:       vm.Spec.VirtualMachineClassName,
+				EnableParavirtualization:      ptr.To(true),
+				BlockDeviceRefs:               []v1alpha2.BlockDeviceSpecRef{rootRef},
+				TerminationGracePeriodSeconds: vm.Spec.TerminationGracePeriodSeconds,
+				Disruptions: &v1alpha2.Disruptions{
+					RestartApprovalMode: vm.Spec.Disruptions.RestartApprovalMode,
+				},
+			},
+		})).To(Succeed())
+
+		kvvmi := makeKVVMI()
+		fakeClient, reconcileObj, vmState = setupEnvironment(vm, kvvm, kvvmi, ip, vmClass)
+
+		reconcile()
+
+		updatedVM := &v1alpha2.VirtualMachine{}
+		Expect(fakeClient.Get(ctx, client.ObjectKeyFromObject(vm), updatedVM)).To(Succeed())
+		Expect(updatedVM.Status.RestartAwaitingChanges).To(BeNil())
+		awaitCond, awaitExists := conditions.GetCondition(vmcondition.TypeAwaitingRestartToApplyConfiguration, updatedVM.Status.Conditions)
+		Expect(awaitExists).To(BeFalse(), "ApplyImmediate block device changes should not set %s, got %+v", vmcondition.TypeAwaitingRestartToApplyConfiguration, awaitCond)
+	})
+
 	It("keeps ConfigurationApplied False and requeues while SDN is not ready", func() {
 		ip := &v1alpha2.VirtualMachineIPAddress{
 			ObjectMeta: metav1.ObjectMeta{Name: "test-ip", Namespace: namespace},
@@ -578,6 +628,46 @@ var _ = Describe("SyncKvvmHandler", func() {
 		Expect(cond.Reason).To(Equal(vmcondition.ReasonConfigurationNotApplied.String()))
 	})
 
+	It("upgrades only block device changes touching non-hotpluggable volumes", func() {
+		rootRef := v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "root"}
+		dataRef := v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "data"}
+		extraRef := v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "extra"}
+
+		kvvmi := newEmptyKVVMI(name, namespace)
+		kvvmi.Spec.Volumes = []virtv1.Volume{
+			{
+				Name: kvbuilder.GenerateDiskName(rootRef.Kind, rootRef.Name),
+				VolumeSource: virtv1.VolumeSource{
+					PersistentVolumeClaim: &virtv1.PersistentVolumeClaimVolumeSource{Hotpluggable: false},
+				},
+			},
+			{
+				Name: kvbuilder.GenerateDiskName(dataRef.Kind, dataRef.Name),
+				VolumeSource: virtv1.VolumeSource{
+					PersistentVolumeClaim: &virtv1.PersistentVolumeClaimVolumeSource{Hotpluggable: true},
+				},
+			},
+		}
+
+		nonHotpluggableRefs := nonHotpluggableVolumeRefs(kvvmi)
+		changes := vmchange.SpecChanges{}
+		changes.Add(
+			vmchange.FieldChange{Path: "blockDeviceRefs.0", Operation: vmchange.ChangeRemove, CurrentValue: dataRef, ActionRequired: vmchange.ActionApplyImmediate},
+			vmchange.FieldChange{Path: "blockDeviceRefs.1", Operation: vmchange.ChangeAdd, DesiredValue: extraRef, ActionRequired: vmchange.ActionApplyImmediate},
+			vmchange.FieldChange{Path: "blockDeviceRefs.2", Operation: vmchange.ChangeReplace, CurrentValue: rootRef, DesiredValue: dataRef, ActionRequired: vmchange.ActionApplyImmediate},
+			vmchange.FieldChange{Path: "cpu.cores", Operation: vmchange.ChangeReplace, ActionRequired: vmchange.ActionApplyImmediate},
+		)
+
+		changes.UpgradeBlockDeviceChangesToRestartIf(func(change vmchange.FieldChange) bool {
+			return blockDeviceChangeTouchesRefs(change, nonHotpluggableRefs)
+		})
+
+		Expect(changes.GetAll()[0].ActionRequired).To(Equal(vmchange.ActionApplyImmediate))
+		Expect(changes.GetAll()[1].ActionRequired).To(Equal(vmchange.ActionApplyImmediate))
+		Expect(changes.GetAll()[2].ActionRequired).To(Equal(vmchange.ActionRestart))
+		Expect(changes.GetAll()[3].ActionRequired).To(Equal(vmchange.ActionApplyImmediate))
+	})
+
 	DescribeTable("isPlacementPolicyChanged",
 		func(path string, expected bool) {
 			h := &SyncKvvmHandler{}

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

@@ -230,8 +230,22 @@ func (s *SpecChanges) GetRestartMessages() []string {
 }
 
 func (s *SpecChanges) ConvertPendingChanges() ([]apiextensionsv1.JSON, error) {
+	return s.convertPendingChanges(func(FieldChange) bool { return true })
+}
+
+func (s *SpecChanges) ConvertPendingRestartChanges() ([]apiextensionsv1.JSON, error) {
+	return s.convertPendingChanges(func(change FieldChange) bool {
+		return change.ActionRequired == ActionRestart
+	})
+}
+
+func (s *SpecChanges) convertPendingChanges(include func(FieldChange) bool) ([]apiextensionsv1.JSON, error) {
 	res := make([]apiextensionsv1.JSON, 0, len(s.changes))
 	for i := range s.changes {
+		if !include(s.changes[i]) {
+			continue
+		}
+
 		b, err := json.Marshal(s.changes[i])
 		if err != nil {
 			return nil, fmt.Errorf("change[%d]: %w", i, err)
@@ -243,9 +257,13 @@ func (s *SpecChanges) ConvertPendingChanges() ([]apiextensionsv1.JSON, error) {
 }
 
 func (s *SpecChanges) UpgradeBlockDeviceChangesToRestart() {
+	s.UpgradeBlockDeviceChangesToRestartIf(nil)
+}
+
+func (s *SpecChanges) UpgradeBlockDeviceChangesToRestartIf(shouldUpgrade func(FieldChange) bool) {
 	for i := range s.changes {
 		isBlockDeviceChange := s.changes[i].Path == blockDevicesPath || strings.HasPrefix(s.changes[i].Path, blockDevicesPath+".")
-		if isBlockDeviceChange && s.changes[i].ActionRequired == ActionApplyImmediate {
+		if isBlockDeviceChange && s.changes[i].ActionRequired == ActionApplyImmediate && (shouldUpgrade == nil || shouldUpgrade(s.changes[i])) {
 			s.changes[i].ActionRequired = ActionRestart
 		}
 	}