diff --git a/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go b/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go index ba54337ff7..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,13 +187,22 @@ 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) + cleanupRemovedStaticDisks(kvvm, specDiskNames, hotpluggableVolumes, vmbdaDiskNames, 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 +224,24 @@ func applyBlockDeviceRefs( return nil } -func cleanupRemovedStaticDisks(kvvm *KVVM, specDiskNames, hotpluggableVolumes map[string]struct{}) { +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 + } 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..b16aedbfc5 --- /dev/null +++ b/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils_test.go @@ -0,0 +1,143 @@ +/* +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, nil, 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)) + }) + + 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)) + }) + }) + + 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, nil, 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/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 } 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())