From f8372d0be4b4c5d1618641cfc4076e4dc57238c6 Mon Sep 17 00:00:00 2001 From: Valeriy Khorunzhin Date: Tue, 16 Jun 2026 18:48:36 +0300 Subject: [PATCH 1/3] fix restart changes Signed-off-by: Valeriy Khorunzhin --- .../pkg/controller/vmchange/comparator_block_devices.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/images/virtualization-artifact/pkg/controller/vmchange/comparator_block_devices.go b/images/virtualization-artifact/pkg/controller/vmchange/comparator_block_devices.go index cab020114d..baa4e2475f 100644 --- a/images/virtualization-artifact/pkg/controller/vmchange/comparator_block_devices.go +++ b/images/virtualization-artifact/pkg/controller/vmchange/comparator_block_devices.go @@ -27,6 +27,10 @@ const blockDevicesPath = "blockDeviceRefs" // compareBlockDevices returns changes between current and desired blockDevices lists. func compareBlockDevices(current, desired *v1alpha2.VirtualMachineSpec) []FieldChange { + if current.EnableParavirtualization && desired.EnableParavirtualization { + return nil + } + if len(current.BlockDeviceRefs) == 0 && len(desired.BlockDeviceRefs) == 0 { return nil } From 51d33a930e30d39b54adfa1fb9137b0a8c125f6a Mon Sep 17 00:00:00 2001 From: Valeriy Khorunzhin Date: Tue, 16 Jun 2026 19:03:17 +0300 Subject: [PATCH 2/3] fix test Signed-off-by: Valeriy Khorunzhin --- .../pkg/controller/vmchange/compare_test.go | 38 ++++++++----------- 1 file changed, 15 insertions(+), 23 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/vmchange/compare_test.go b/images/virtualization-artifact/pkg/controller/vmchange/compare_test.go index 8377ac10fa..e190205987 100644 --- a/images/virtualization-artifact/pkg/controller/vmchange/compare_test.go +++ b/images/virtualization-artifact/pkg/controller/vmchange/compare_test.go @@ -281,7 +281,7 @@ blockDeviceRefs: ), }, { - "apply immediate on blockDeviceRefs add disk", + "no changes on blockDeviceRefs add disk with paravirt enabled", ` enableParavirtualization: true blockDeviceRefs: @@ -297,13 +297,10 @@ blockDeviceRefs: name: linux `, nil, - assertChanges( - actionRequired(ActionApplyImmediate), - requirePathOperation("blockDeviceRefs.0", ChangeAdd), - ), + assertNoChanges(), }, { - "apply immediate on blockDeviceRefs remove disk", + "no changes on blockDeviceRefs remove disk with paravirt enabled", ` enableParavirtualization: true blockDeviceRefs: @@ -319,10 +316,7 @@ blockDeviceRefs: name: linux `, nil, - assertChanges( - actionRequired(ActionApplyImmediate), - requirePathOperation("blockDeviceRefs.0", ChangeRemove), - ), + assertNoChanges(), }, { "restart on blockDeviceRefs remove disk with paravirt disabled", @@ -347,7 +341,7 @@ blockDeviceRefs: ), }, { - "apply immediate on blockDeviceRefs change order", + "no changes on blockDeviceRefs change order with paravirt enabled", ` enableParavirtualization: true blockDeviceRefs: @@ -365,14 +359,10 @@ blockDeviceRefs: name: linux `, nil, - assertChanges( - actionRequired(ActionApplyImmediate), - requirePathOperation("blockDeviceRefs.0", ChangeReplace), - requirePathOperation("blockDeviceRefs.1", ChangeReplace), - ), + assertNoChanges(), }, { - "apply immediate on blockDeviceRefs change order :: bigger", + "no changes on blockDeviceRefs change order with paravirt enabled :: bigger", ` enableParavirtualization: true blockDeviceRefs: @@ -403,12 +393,7 @@ blockDeviceRefs: name: linux `, nil, - assertChanges( - actionRequired(ActionApplyImmediate), - requirePathOperation("blockDeviceRefs.0", ChangeReplace), - requirePathOperation("blockDeviceRefs.1", ChangeReplace), - requirePathOperation("blockDeviceRefs.4", ChangeReplace), - ), + assertNoChanges(), }, { "restart on provisioning add", @@ -769,6 +754,13 @@ func assertChanges(asserts ...func(t *testing.T, changes SpecChanges)) func(t *t } } +func assertNoChanges() func(t *testing.T, changes SpecChanges) { + return func(t *testing.T, changes SpecChanges) { + t.Helper() + require.True(t, changes.IsEmpty(), "changes should be empty, got %+v", changes) + } +} + func actionRequired(actionType ActionType) func(t *testing.T, changes SpecChanges) { return func(t *testing.T, changes SpecChanges) { t.Helper() From 06f5d07cf8d628622d4393fc8aaf70abb178c99c Mon Sep 17 00:00:00 2001 From: Valeriy Khorunzhin Date: Thu, 25 Jun 2026 12:12:31 +0300 Subject: [PATCH 3/3] no early check Signed-off-by: Valeriy Khorunzhin --- .../pkg/controller/vm/internal/sync_kvvm.go | 54 +++++++++-- .../controller/vm/internal/sync_kvvm_test.go | 90 +++++++++++++++++++ .../vmchange/comparator_block_devices.go | 4 - .../pkg/controller/vmchange/compare_test.go | 38 ++++---- .../pkg/controller/vmchange/spec_changes.go | 20 ++++- 5 files changed, 180 insertions(+), 26 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm.go b/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm.go index a2de001ae5..07b6ef6a06 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm.go +++ b/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, ¤t.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 { diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm_test.go index ea6c9693a5..33e432550b 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm_test.go +++ b/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{} diff --git a/images/virtualization-artifact/pkg/controller/vmchange/comparator_block_devices.go b/images/virtualization-artifact/pkg/controller/vmchange/comparator_block_devices.go index baa4e2475f..cab020114d 100644 --- a/images/virtualization-artifact/pkg/controller/vmchange/comparator_block_devices.go +++ b/images/virtualization-artifact/pkg/controller/vmchange/comparator_block_devices.go @@ -27,10 +27,6 @@ const blockDevicesPath = "blockDeviceRefs" // compareBlockDevices returns changes between current and desired blockDevices lists. func compareBlockDevices(current, desired *v1alpha2.VirtualMachineSpec) []FieldChange { - if current.EnableParavirtualization && desired.EnableParavirtualization { - return nil - } - if len(current.BlockDeviceRefs) == 0 && len(desired.BlockDeviceRefs) == 0 { return nil } diff --git a/images/virtualization-artifact/pkg/controller/vmchange/compare_test.go b/images/virtualization-artifact/pkg/controller/vmchange/compare_test.go index e190205987..8377ac10fa 100644 --- a/images/virtualization-artifact/pkg/controller/vmchange/compare_test.go +++ b/images/virtualization-artifact/pkg/controller/vmchange/compare_test.go @@ -281,7 +281,7 @@ blockDeviceRefs: ), }, { - "no changes on blockDeviceRefs add disk with paravirt enabled", + "apply immediate on blockDeviceRefs add disk", ` enableParavirtualization: true blockDeviceRefs: @@ -297,10 +297,13 @@ blockDeviceRefs: name: linux `, nil, - assertNoChanges(), + assertChanges( + actionRequired(ActionApplyImmediate), + requirePathOperation("blockDeviceRefs.0", ChangeAdd), + ), }, { - "no changes on blockDeviceRefs remove disk with paravirt enabled", + "apply immediate on blockDeviceRefs remove disk", ` enableParavirtualization: true blockDeviceRefs: @@ -316,7 +319,10 @@ blockDeviceRefs: name: linux `, nil, - assertNoChanges(), + assertChanges( + actionRequired(ActionApplyImmediate), + requirePathOperation("blockDeviceRefs.0", ChangeRemove), + ), }, { "restart on blockDeviceRefs remove disk with paravirt disabled", @@ -341,7 +347,7 @@ blockDeviceRefs: ), }, { - "no changes on blockDeviceRefs change order with paravirt enabled", + "apply immediate on blockDeviceRefs change order", ` enableParavirtualization: true blockDeviceRefs: @@ -359,10 +365,14 @@ blockDeviceRefs: name: linux `, nil, - assertNoChanges(), + assertChanges( + actionRequired(ActionApplyImmediate), + requirePathOperation("blockDeviceRefs.0", ChangeReplace), + requirePathOperation("blockDeviceRefs.1", ChangeReplace), + ), }, { - "no changes on blockDeviceRefs change order with paravirt enabled :: bigger", + "apply immediate on blockDeviceRefs change order :: bigger", ` enableParavirtualization: true blockDeviceRefs: @@ -393,7 +403,12 @@ blockDeviceRefs: name: linux `, nil, - assertNoChanges(), + assertChanges( + actionRequired(ActionApplyImmediate), + requirePathOperation("blockDeviceRefs.0", ChangeReplace), + requirePathOperation("blockDeviceRefs.1", ChangeReplace), + requirePathOperation("blockDeviceRefs.4", ChangeReplace), + ), }, { "restart on provisioning add", @@ -754,13 +769,6 @@ func assertChanges(asserts ...func(t *testing.T, changes SpecChanges)) func(t *t } } -func assertNoChanges() func(t *testing.T, changes SpecChanges) { - return func(t *testing.T, changes SpecChanges) { - t.Helper() - require.True(t, changes.IsEmpty(), "changes should be empty, got %+v", changes) - } -} - func actionRequired(actionType ActionType) func(t *testing.T, changes SpecChanges) { return func(t *testing.T, changes SpecChanges) { t.Helper() diff --git a/images/virtualization-artifact/pkg/controller/vmchange/spec_changes.go b/images/virtualization-artifact/pkg/controller/vmchange/spec_changes.go index e400f72a4a..0580338e9e 100644 --- a/images/virtualization-artifact/pkg/controller/vmchange/spec_changes.go +++ b/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 } }