From a754a1153b8408e53a91bdce4406e467f1731b1d Mon Sep 17 00:00:00 2001 From: Roman Sysoev Date: Wed, 27 May 2026 02:00:48 +0300 Subject: [PATCH 1/2] fix(vm): allow disk updates when VM is stopped When a VM with paravirtualization is stopped and disks are changed in VM spec, the KVVM was not being updated. This caused the old disks to remain and new disks to not be added. Additionally, added wait for 'lsblk' command to be ready in the block_device_hotplug e2e test to prevent intermittent failures. Changes: - Add isVmRunning check to paravirtualization condition in applyBlockDeviceRefs to allow new disks when VM is stopped - Update cleanupRemovedStaticDisks to remove all disks unconditionally when VM is stopped - Add unit tests for cleanupRemovedStaticDisks with isVmRunning parameter - Add util.UntilGuestCommandsReady for lsblk in block_device_hotplug test Signed-off-by: Roman Sysoev --- .../pkg/controller/kvbuilder/kvvm_utils.go | 11 +- .../controller/kvbuilder/kvvm_utils_test.go | 122 ++++++++++++++++++ test/e2e/vm/block_device_hotplug.go | 3 + 3 files changed, 133 insertions(+), 3 deletions(-) create mode 100644 images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils_test.go diff --git a/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go b/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go index ba54337ff7..01c89109a2 100644 --- a/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go +++ b/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go @@ -186,12 +186,13 @@ func applyBlockDeviceRefs( } } // This is needed to support disk removal in old VMs with static disks - cleanupRemovedStaticDisks(kvvm, specDiskNames, hotpluggableVolumes) + cleanupRemovedStaticDisks(kvvm, specDiskNames, hotpluggableVolumes, isVmRunning) kvvmVolumes := kvvm.Resource.Spec.Template.Spec.Volumes for i, bd := range vm.Spec.BlockDeviceRefs { diskName := GenerateDiskName(bd.Kind, bd.Name) - if vm.Spec.EnableParavirtualization && len(kvvmVolumes) > 0 && !slices.ContainsFunc(kvvmVolumes, func(v virtv1.Volume) bool { return v.Name == diskName }) { + // When VM is stopped, update disks unconditionally. + if isVmRunning && vm.Spec.EnableParavirtualization && len(kvvmVolumes) > 0 && !slices.ContainsFunc(kvvmVolumes, func(v virtv1.Volume) bool { return v.Name == diskName }) { continue } @@ -213,11 +214,15 @@ func applyBlockDeviceRefs( return nil } -func cleanupRemovedStaticDisks(kvvm *KVVM, specDiskNames, hotpluggableVolumes map[string]struct{}) { +func cleanupRemovedStaticDisks(kvvm *KVVM, specDiskNames, hotpluggableVolumes map[string]struct{}, isVmRunning bool) { isRemovedStatic := func(name string) bool { _, kind := GetOriginalDiskName(name) _, inSpec := specDiskNames[name] _, hotpluggable := hotpluggableVolumes[name] + // When VM is stopped, remove all disks that are not in VM spec. + if !isVmRunning { + return kind != "" && !inSpec + } return kind != "" && !inSpec && !hotpluggable } diff --git a/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils_test.go b/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils_test.go new file mode 100644 index 0000000000..b36aa6f4cc --- /dev/null +++ b/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils_test.go @@ -0,0 +1,122 @@ +/* +Copyright 2026 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 kvbuilder + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" + virtv1 "kubevirt.io/api/core/v1" +) + +var _ = Describe("cleanupRemovedStaticDisks", func() { + const ( + vmName = "test-vm" + vmNamespace = "test-ns" + + oldDisk1Name = "vd-old-disk-1" + oldDisk2Name = "vd-old-disk-2" + oldPVC1Name = "pvc-old-disk-1" + oldPVC2Name = "pvc-old-disk-2" + + newDisk1Name = "vd-new-disk-1" + newDisk2Name = "vd-new-disk-2" + ) + + var kvvm *KVVM + + BeforeEach(func() { + kvvm = NewEmptyKVVM( + namespacedName(vmName, vmNamespace), + KVVMOptions{}, + ) + // Add initial volumes to KVVM + kvvm.Resource.Spec.Template.Spec.Volumes = []virtv1.Volume{ + { + Name: oldDisk1Name, + VolumeSource: virtv1.VolumeSource{ + PersistentVolumeClaim: &virtv1.PersistentVolumeClaimVolumeSource{ + PersistentVolumeClaimVolumeSource: corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: oldPVC1Name, + }, + Hotpluggable: true, + }, + }, + }, + { + Name: oldDisk2Name, + VolumeSource: virtv1.VolumeSource{ + PersistentVolumeClaim: &virtv1.PersistentVolumeClaimVolumeSource{ + PersistentVolumeClaimVolumeSource: corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: oldPVC2Name, + }, + Hotpluggable: false, + }, + }, + }, + } + kvvm.Resource.Spec.Template.Spec.Domain.Devices.Disks = []virtv1.Disk{ + {Name: oldDisk1Name}, + {Name: oldDisk2Name}, + } + }) + + Describe("when VM is stopped (isVmRunning=false)", func() { + It("should remove all disks that are not in VM spec, regardless of hotpluggable flag", func() { + specDiskNames := map[string]struct{}{ + newDisk1Name: {}, + newDisk2Name: {}, + } + hotpluggableVolumes := map[string]struct{}{ + oldDisk1Name: {}, // hotpluggable + } + + cleanupRemovedStaticDisks(kvvm, specDiskNames, hotpluggableVolumes, false) + + // Should remove old-disk-1 (hotpluggable) and old-disk-2 (non-hotpluggable) + // because VM is stopped + Expect(kvvm.Resource.Spec.Template.Spec.Volumes).To(HaveLen(0)) + Expect(kvvm.Resource.Spec.Template.Spec.Domain.Devices.Disks).To(HaveLen(0)) + }) + }) + + Describe("when VM is running (isVmRunning=true)", func() { + It("should only remove non-hotpluggable disks that are not in VM spec", func() { + specDiskNames := map[string]struct{}{ + newDisk1Name: {}, + newDisk2Name: {}, + } + hotpluggableVolumes := map[string]struct{}{ + oldDisk1Name: {}, // hotpluggable - should NOT be removed + } + + cleanupRemovedStaticDisks(kvvm, specDiskNames, hotpluggableVolumes, true) + + // Should only remove old-disk-2 (non-hotpluggable) + // old-disk-1 should stay because it's hotpluggable + Expect(kvvm.Resource.Spec.Template.Spec.Volumes).To(HaveLen(1)) + Expect(kvvm.Resource.Spec.Template.Spec.Volumes[0].Name).To(Equal(oldDisk1Name)) + Expect(kvvm.Resource.Spec.Template.Spec.Domain.Devices.Disks).To(HaveLen(1)) + Expect(kvvm.Resource.Spec.Template.Spec.Domain.Devices.Disks[0].Name).To(Equal(oldDisk1Name)) + }) + }) +}) + +func namespacedName(name, namespace string) types.NamespacedName { + return types.NamespacedName{Name: name, Namespace: namespace} +} diff --git a/test/e2e/vm/block_device_hotplug.go b/test/e2e/vm/block_device_hotplug.go index e7562489d2..c8d7abb853 100644 --- a/test/e2e/vm/block_device_hotplug.go +++ b/test/e2e/vm/block_device_hotplug.go @@ -197,6 +197,9 @@ func setupVM(f *framework.Framework, withBlank bool) ( By("Waiting for SSH to be ready") util.UntilSSHReady(f, vm, framework.LongTimeout) + By("Waiting for 'lsblk' to be ready for use") + util.UntilGuestCommandsReady(f, vm, []string{"lsblk"}, framework.MiddleTimeout) + By("Recording initial disk count") initialDiskCount, err = util.GetDiskCount(f, vm.Name, vm.Namespace) Expect(err).NotTo(HaveOccurred()) From 7993060cca4757c89c538e064c5cee85c190df7f Mon Sep 17 00:00:00 2001 From: Roman Sysoev Date: Wed, 27 May 2026 19:05:28 +0300 Subject: [PATCH 2/2] fix(vm): add VMBDA filtering to prevent removing hotplug disks from KVVM When VM is stopped and disks are being cleaned up from KVVM spec, disks that are attached via VMBDA (VirtualMachineBlockDeviceAttachment) should not be removed. These disks are visible in the stopped VM status and removing them would cause inconsistency. Changes: - Add vmbdaByBlockDeviceRef parameter to ApplyVirtualMachineSpec - Add VMBDAByBlockDeviceRef field to BlockDevicesState - Update cleanupRemovedStaticDisks to filter out VMBDA-attached disks - Add unit test for VMBDA disk preservation Signed-off-by: Roman Sysoev --- .../pkg/controller/kvbuilder/kvvm_utils.go | 25 ++++++++++++++++--- .../controller/kvbuilder/kvvm_utils_test.go | 25 +++++++++++++++++-- .../vm/internal/block_device_handler.go | 17 ++++++++++--- .../pkg/controller/vm/internal/sync_kvvm.go | 13 +++++++++- 4 files changed, 70 insertions(+), 10 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go b/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go index 01c89109a2..21d7eab66c 100644 --- a/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go +++ b/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go @@ -97,6 +97,7 @@ func ApplyVirtualMachineSpec( vdByName map[string]*v1alpha2.VirtualDisk, viByName map[string]*v1alpha2.VirtualImage, cviByName map[string]*v1alpha2.ClusterVirtualImage, + vmbdaByBlockDeviceRef map[v1alpha2.VMBDAObjectRef][]*v1alpha2.VirtualMachineBlockDeviceAttachment, class *v1alpha2.VirtualMachineClass, ipAddress string, networkSpec network.InterfaceSpecList, @@ -130,7 +131,7 @@ func ApplyVirtualMachineSpec( return err } - if err := applyBlockDeviceRefs(kvvm, vm, isVmRunning, vdByName, viByName, cviByName); err != nil { + if err := applyBlockDeviceRefs(kvvm, vm, isVmRunning, vdByName, viByName, cviByName, vmbdaByBlockDeviceRef); err != nil { return err } @@ -166,6 +167,7 @@ func applyBlockDeviceRefs( vdByName map[string]*v1alpha2.VirtualDisk, viByName map[string]*v1alpha2.VirtualImage, cviByName map[string]*v1alpha2.ClusterVirtualImage, + vmbdaByBlockDeviceRef map[v1alpha2.VMBDAObjectRef][]*v1alpha2.VirtualMachineBlockDeviceAttachment, ) error { hasExplicitBootOrder := false for _, bd := range vm.Spec.BlockDeviceRefs { @@ -185,8 +187,16 @@ func applyBlockDeviceRefs( hotpluggableVolumes[v.Name] = struct{}{} } } + vmbdaDiskNames := make(map[string]struct{}, len(vmbdaByBlockDeviceRef)) + for ref := range vmbdaByBlockDeviceRef { + diskName := GenerateDiskName(v1alpha2.BlockDeviceKind(ref.Kind), ref.Name) + if diskName != "" { + vmbdaDiskNames[diskName] = struct{}{} + } + } + // This is needed to support disk removal in old VMs with static disks - cleanupRemovedStaticDisks(kvvm, specDiskNames, hotpluggableVolumes, isVmRunning) + cleanupRemovedStaticDisks(kvvm, specDiskNames, hotpluggableVolumes, vmbdaDiskNames, isVmRunning) kvvmVolumes := kvvm.Resource.Spec.Template.Spec.Volumes for i, bd := range vm.Spec.BlockDeviceRefs { @@ -214,11 +224,20 @@ func applyBlockDeviceRefs( return nil } -func cleanupRemovedStaticDisks(kvvm *KVVM, specDiskNames, hotpluggableVolumes map[string]struct{}, isVmRunning bool) { +func cleanupRemovedStaticDisks(kvvm *KVVM, specDiskNames, hotpluggableVolumes, vmbdaDiskNames map[string]struct{}, isVmRunning bool) { + // Disks attached via VMBDA should not be removed from KVVM spec even when VM is stopped. + // If VMBDA exists, the disk is associated with this VM - VMBDA controller will + // handle cleanup when VMBDA is deleted. isRemovedStatic := func(name string) bool { _, kind := GetOriginalDiskName(name) _, inSpec := specDiskNames[name] _, hotpluggable := hotpluggableVolumes[name] + _, attachedViaVMBDA := vmbdaDiskNames[name] + + // Don't remove disks that are attached via VMBDA. + if attachedViaVMBDA { + return false + } // When VM is stopped, remove all disks that are not in VM spec. if !isVmRunning { return kind != "" && !inSpec diff --git a/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils_test.go b/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils_test.go index b36aa6f4cc..ab280b54ac 100644 --- a/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils_test.go +++ b/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils_test.go @@ -86,7 +86,7 @@ var _ = Describe("cleanupRemovedStaticDisks", func() { oldDisk1Name: {}, // hotpluggable } - cleanupRemovedStaticDisks(kvvm, specDiskNames, hotpluggableVolumes, false) + cleanupRemovedStaticDisks(kvvm, specDiskNames, hotpluggableVolumes, nil, false) // Should remove old-disk-1 (hotpluggable) and old-disk-2 (non-hotpluggable) // because VM is stopped @@ -105,7 +105,7 @@ var _ = Describe("cleanupRemovedStaticDisks", func() { oldDisk1Name: {}, // hotpluggable - should NOT be removed } - cleanupRemovedStaticDisks(kvvm, specDiskNames, hotpluggableVolumes, true) + cleanupRemovedStaticDisks(kvvm, specDiskNames, hotpluggableVolumes, nil, true) // Should only remove old-disk-2 (non-hotpluggable) // old-disk-1 should stay because it's hotpluggable @@ -114,6 +114,27 @@ var _ = Describe("cleanupRemovedStaticDisks", func() { Expect(kvvm.Resource.Spec.Template.Spec.Domain.Devices.Disks).To(HaveLen(1)) Expect(kvvm.Resource.Spec.Template.Spec.Domain.Devices.Disks[0].Name).To(Equal(oldDisk1Name)) }) + + It("should not remove disk attached via VMBDA when VM is stopped", func() { + specDiskNames := map[string]struct{}{ + newDisk1Name: {}, + } + hotpluggableVolumes := map[string]struct{}{} + + // Simulate disk attached via VMBDA + vmbdaDiskNames := map[string]struct{}{ + oldDisk1Name: {}, + } + + cleanupRemovedStaticDisks(kvvm, specDiskNames, hotpluggableVolumes, vmbdaDiskNames, false) + + // old-disk-1 should stay because it's attached via VMBDA + Expect(kvvm.Resource.Spec.Template.Spec.Volumes).To(HaveLen(1)) + Expect(kvvm.Resource.Spec.Template.Spec.Volumes[0].Name).To(Equal(oldDisk1Name)) + // old-disk-2 should be removed because it's not in spec and not attached via VMBDA + Expect(kvvm.Resource.Spec.Template.Spec.Domain.Devices.Disks).To(HaveLen(1)) + Expect(kvvm.Resource.Spec.Template.Spec.Domain.Devices.Disks[0].Name).To(Equal(oldDisk1Name)) + }) }) }) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/block_device_handler.go b/images/virtualization-artifact/pkg/controller/vm/internal/block_device_handler.go index de3166476d..2af6194427 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/block_device_handler.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/block_device_handler.go @@ -275,10 +275,11 @@ func (h *BlockDeviceHandler) updateFinalizers(ctx context.Context, vm *v1alpha2. func NewBlockDeviceState(s state.VirtualMachineState) BlockDevicesState { return BlockDevicesState{ - s: s, - VIByName: make(map[string]*v1alpha2.VirtualImage), - CVIByName: make(map[string]*v1alpha2.ClusterVirtualImage), - VDByName: make(map[string]*v1alpha2.VirtualDisk), + s: s, + VIByName: make(map[string]*v1alpha2.VirtualImage), + CVIByName: make(map[string]*v1alpha2.ClusterVirtualImage), + VDByName: make(map[string]*v1alpha2.VirtualDisk), + VMBDAByBlockDeviceRef: make(map[v1alpha2.VMBDAObjectRef][]*v1alpha2.VirtualMachineBlockDeviceAttachment), } } @@ -287,6 +288,9 @@ type BlockDevicesState struct { VIByName map[string]*v1alpha2.VirtualImage CVIByName map[string]*v1alpha2.ClusterVirtualImage VDByName map[string]*v1alpha2.VirtualDisk + // VMBDAByBlockDeviceRef contains VMBDA objects keyed by block device reference. + // Used to filter out hotplug disks that should not be removed from KVVM spec. + VMBDAByBlockDeviceRef map[v1alpha2.VMBDAObjectRef][]*v1alpha2.VirtualMachineBlockDeviceAttachment } func (s *BlockDevicesState) Reload(ctx context.Context) error { @@ -302,8 +306,13 @@ func (s *BlockDevicesState) Reload(ctx context.Context) error { if err != nil { return err } + vmbdaByRef, err := s.s.VirtualMachineBlockDeviceAttachments(ctx) + if err != nil { + return err + } s.VIByName = viByName s.CVIByName = ciByName s.VDByName = vdByName + s.VMBDAByBlockDeviceRef = vmbdaByRef return nil } 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 688b0f556d..7916bc5a66 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm.go @@ -503,7 +503,18 @@ func MakeKVVMFromVMSpec(ctx context.Context, s state.VirtualMachineState) (*virt } // Create kubevirt VirtualMachine resource from d8 VirtualMachine spec. - err = kvbuilder.ApplyVirtualMachineSpec(kvvmBuilder, current, bdState.VDByName, bdState.VIByName, bdState.CVIByName, class, ipAddress, networkSpec, kvvmi != nil && kvvmi.Status.Phase == virtv1.Running) + err = kvbuilder.ApplyVirtualMachineSpec( + kvvmBuilder, + current, + bdState.VDByName, + bdState.VIByName, + bdState.CVIByName, + bdState.VMBDAByBlockDeviceRef, + class, + ipAddress, + networkSpec, + kvvmi != nil && kvvmi.Status.Phase == virtv1.Running, + ) if err != nil { return nil, err }