From 3726c402501af9a26d61bca2b88eb04e9953f11d Mon Sep 17 00:00:00 2001 From: calvix <7136358+calvix@users.noreply.github.com> Date: Tue, 13 Oct 2026 09:36:24 +0200 Subject: [PATCH] kvm: add a disk.discard VM detail and drop the virtio-blk disk controller Discard on KVM disks could only be had implicitly: SCSI and Linstor disks get discard='unmap', every other disk keeps libvirt's default 'ignore', so a guest's TRIM never reaches thin storage such as Ceph RBD. The virtio-blk controller option was meant to provide it for virtio disks, but libvirt has no such bus ('virtio' already is a virtio-blk device), so a VM using it could not start. - New KVM VM detail disk.discard (unmap|ignore). It is applied to every disk when the VM starts and when a disk is attached; without it the disks keep their current defaults. 'unmap' is not set on IDE disks with a QEMU that has the IDE discard bug. - virtio-blk is no longer offered as a disk controller. A VM or template that still has it set gets a virtio disk, as before the option existed. --- .../java/com/cloud/vm/VmDetailConstants.java | 3 + .../resource/LibvirtComputingResource.java | 36 ++++++++-- .../hypervisor/kvm/resource/LibvirtVMDef.java | 16 ++++- .../kvm/storage/KVMStorageProcessor.java | 5 +- .../LibvirtComputingResourceTest.java | 68 ++++++++++++++++++- .../kvm/resource/LibvirtVMDefTest.java | 15 ++++ .../kvm/storage/KVMStorageProcessorTest.java | 8 +++ .../com/cloud/api/query/QueryManagerImpl.java | 5 +- .../cloud/storage/VolumeApiServiceImpl.java | 12 ++++ .../java/com/cloud/vm/UserVmManagerImpl.java | 4 ++ ui/src/views/image/UpdateTemplate.vue | 4 -- 11 files changed, 159 insertions(+), 17 deletions(-) diff --git a/api/src/main/java/com/cloud/vm/VmDetailConstants.java b/api/src/main/java/com/cloud/vm/VmDetailConstants.java index 3d0152b0e438..ed26cb6db01d 100644 --- a/api/src/main/java/com/cloud/vm/VmDetailConstants.java +++ b/api/src/main/java/com/cloud/vm/VmDetailConstants.java @@ -50,6 +50,9 @@ public interface VmDetailConstants { String IOTHREADS = "iothreads"; String IO_POLICY = "io.policy"; + // KVM specific, discard mode of the VM's disks: unmap or ignore + String DISK_DISCARD = "disk.discard"; + // KVM specific, the number of queues for multiqueue NICs String NIC_MULTIQUEUE_NUMBER = "nic.multiqueue.number"; String NIC_PACKED_VIRTQUEUES_ENABLED = "nic.packed.virtqueues.enabled"; diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java index 7b7a38b2f5bc..ab503f1b3516 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java @@ -3229,7 +3229,7 @@ public int compare(final DiskTO arg0, final DiskTO arg1) { disk.setLogicalBlockIOSize(pool.getSupportedLogicalBlockSize()); disk.setPhysicalBlockIOSize(pool.getSupportedPhysicalBlockSize()); - if (diskBusType == DiskDef.DiskBus.SCSI || diskBusType == DiskDef.DiskBus.VIRTIOBLK) { + if (diskBusType == DiskDef.DiskBus.SCSI) { disk.setQemuDriver(true); disk.setDiscard(DiscardType.UNMAP); } @@ -3283,6 +3283,7 @@ public int compare(final DiskTO arg0, final DiskTO arg1) { defineDiskForDefaultPoolType(disk, volume, isWindowsTemplate, isUefiEnabled, isSecureBoot, physicalDisk, devId, diskBusType, diskBusTypeData, details); } + applyDiskDiscardDetail(disk, details); pool.customizeLibvirtDiskDef(disk); } @@ -3426,6 +3427,30 @@ public boolean isQemuDiscardBugFree(DiskDef.DiskBus diskBus) { return diskBus != DiskDef.DiskBus.IDE || getHypervisorQemuVersion() >= HYPERVISOR_QEMU_VERSION_IDE_DISCARD_FIXED; } + /** + * Sets the disk's discard mode from the {@link VmDetailConstants#DISK_DISCARD} VM detail, overriding the + * mode the disk got from its bus or storage pool. Without the detail the disk is left as it is. + * @param disk the disk being defined, with its bus already set + * @param details the VM details + */ + public void applyDiskDiscardDetail(DiskDef disk, Map details) { + String discard = MapUtils.getString(details, VmDetailConstants.DISK_DISCARD); + if (StringUtils.isBlank(discard)) { + return; + } + DiscardType discardType = DiscardType.fromValue(discard); + if (discardType == null) { + LOGGER.warn("Ignoring unknown {} value '{}', expected 'unmap' or 'ignore'", VmDetailConstants.DISK_DISCARD, discard); + return; + } + if (discardType == DiscardType.UNMAP && !isQemuDiscardBugFree(disk.getBusType())) { + LOGGER.warn("Not enabling discard on disk {}: QEMU {} has a discard bug on the {} bus", disk.getDiskLabel(), + getHypervisorQemuVersion(), disk.getBusType()); + return; + } + disk.setDiscard(discardType); + } + public boolean isUbuntuOrDebianHost() { Map versionString = getVersionStrings(); String hostKey = "Host.OS"; @@ -4352,11 +4377,10 @@ public DiskDef.DiskBus getDiskModelFromVMDetail(final VirtualMachineTO vmTO) { String rootDiskController = details.get(VmDetailConstants.ROOT_DISK_CONTROLLER); if (StringUtils.isNotBlank(rootDiskController)) { LOGGER.debug("Passed custom disk controller for ROOT disk " + rootDiskController); - for (DiskDef.DiskBus bus : DiskDef.DiskBus.values()) { - if (bus.toString().equalsIgnoreCase(rootDiskController)) { - LOGGER.debug("Found matching enum for disk controller for ROOT disk " + rootDiskController); - return bus; - } + DiskDef.DiskBus bus = DiskDef.DiskBus.fromValue(rootDiskController); + if (bus != null) { + LOGGER.debug("Found matching enum for disk controller for ROOT disk " + rootDiskController); + return bus; } } return null; diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java index 696e71bea80c..e1c5e0147528 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java @@ -679,7 +679,7 @@ public String toString() { } public enum DiskBus { - IDE("ide"), SCSI("scsi"), VIRTIO("virtio"), XEN("xen"), USB("usb"), UML("uml"), FDC("fdc"), SATA("sata"), VIRTIOBLK("virtio-blk"); + IDE("ide"), SCSI("scsi"), VIRTIO("virtio"), XEN("xen"), USB("usb"), UML("uml"), FDC("fdc"), SATA("sata"); String _bus; DiskBus(String bus) { @@ -687,6 +687,11 @@ public enum DiskBus { } public static DiskBus fromValue(String bus) { + if ("virtio-blk".equalsIgnoreCase(bus)) { + // A disk controller name CloudStack used to offer: the virtio bus already is a + // virtio-blk device. Kept so a VM or template that still has it set uses virtio. + return VIRTIO; + } for (DiskBus b : DiskBus.values()) { if (b.toString().equalsIgnoreCase(bus)) { return b; @@ -739,6 +744,15 @@ public enum DiscardType { _discardType = discardType; } + public static DiscardType fromValue(String discardType) { + for (DiscardType d : DiscardType.values()) { + if (d.toString().equalsIgnoreCase(discardType)) { + return d; + } + } + return null; + } + @Override public String toString() { if (_discardType == null) { diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java index 337504ac4f5c..a554a2c3fd9a 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java @@ -1426,7 +1426,7 @@ protected synchronized void attachOrDetachDisk(final Connect conn, final boolean } else { DiskDef.DiskBus busT = getAttachDiskBusType(devId, disks, controllerInfo); diskdef = new DiskDef(); - if (busT == DiskDef.DiskBus.SCSI || busT == DiskDef.DiskBus.VIRTIOBLK) { + if (busT == DiskDef.DiskBus.SCSI) { diskdef.setQemuDriver(true); diskdef.setDiscard(DiscardType.UNMAP); } @@ -1518,6 +1518,7 @@ protected synchronized void attachOrDetachDisk(final Connect conn, final boolean } diskdef.setPhysicalBlockIOSize(attachingPool.getSupportedPhysicalBlockSize()); diskdef.setLogicalBlockIOSize(attachingPool.getSupportedLogicalBlockSize()); + resource.applyDiskDiscardDetail(diskdef, details); attachingPool.customizeLibvirtDiskDef(diskdef); } @@ -1544,8 +1545,6 @@ protected DiskDef.DiskBus getAttachDiskBusType(int deviceId, List disks } if (disk.getBusType() == DiskDef.DiskBus.SCSI) { return DiskDef.DiskBus.SCSI; - } else if (disk.getBusType() == DiskDef.DiskBus.VIRTIOBLK) { - return DiskDef.DiskBus.VIRTIOBLK; } } return DiskDef.DiskBus.VIRTIO; diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java index b7d66e66542d..cf8d74021f02 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java @@ -6554,6 +6554,32 @@ public void createLinstorVdb() throws LibvirtException, InternalErrorException, assertEquals(DiskDef.DiskBus.VIRTIO, rootDisk.getBusType()); assertEquals(DiskDef.DiscardType.UNMAP, rootDisk.getDiscard()); } + + // 5. test Bus: VIRTIO and disk.discard=ignore -> the detail overrides the Linstor default + vmToDetails.put(VmDetailConstants.DISK_DISCARD, "ignore"); + { + LibvirtVMDef vm = new LibvirtVMDef(); + vm.addComp(new DevicesDef()); + libvirtComputingResourceSpy.createVbd(connect, to, name, vm); + + DiskDef rootDisk = vm.getDevices().getDisks().get(0); + assertEquals(DiskDef.DiskBus.VIRTIO, rootDisk.getBusType()); + assertEquals(DiskDef.DiscardType.IGNORE, rootDisk.getDiscard()); + } + + // 6. test Bus: IDE, broken qemu version and disk.discard=unmap -> NO discard + when(libvirtComputingResourceSpy.getHypervisorQemuVersion()).thenReturn(6000000L); + vmToDetails.put(VmDetailConstants.ROOT_DISK_CONTROLLER, DiskDef.DiskBus.IDE.name()); + vmToDetails.put(VmDetailConstants.DISK_DISCARD, "unmap"); + { + LibvirtVMDef vm = new LibvirtVMDef(); + vm.addComp(new DevicesDef()); + libvirtComputingResourceSpy.createVbd(connect, to, name, vm); + + DiskDef rootDisk = vm.getDevices().getDisks().get(0); + assertEquals(DiskDef.DiskBus.IDE, rootDisk.getBusType()); + assertEquals(DiskDef.DiscardType.IGNORE, rootDisk.getDiscard()); + } } @Test @@ -6563,7 +6589,47 @@ public void testGetDiskModelFromVMDetailVirtioBlk() { details.put(VmDetailConstants.ROOT_DISK_CONTROLLER, "virtio-blk"); Mockito.when(virtualMachineTO.getDetails()).thenReturn(details); DiskDef.DiskBus diskBus = libvirtComputingResourceSpy.getDiskModelFromVMDetail(virtualMachineTO); - assertEquals(DiskDef.DiskBus.VIRTIOBLK, diskBus); + assertEquals(DiskDef.DiskBus.VIRTIO, diskBus); + } + + private DiskDef virtioDisk() { + DiskDef disk = new DiskDef(); + disk.defNetworkBasedDisk("pool/volume", "mon", 6789, "admin", "secret", 0, DiskDef.DiskBus.VIRTIO, + DiskDef.DiskProtocol.RBD, DiskDef.DiskFmtType.RAW); + return disk; + } + + @Test + public void applyDiskDiscardDetailUnmap() { + DiskDef disk = virtioDisk(); + libvirtComputingResourceSpy.applyDiskDiscardDetail(disk, Map.of(VmDetailConstants.DISK_DISCARD, "unmap")); + assertEquals(DiskDef.DiscardType.UNMAP, disk.getDiscard()); + assertTrue(disk.toString().contains("discard='unmap'")); + } + + @Test + public void applyDiskDiscardDetailIgnoreOverridesUnmap() { + DiskDef disk = virtioDisk(); + disk.setDiscard(DiskDef.DiscardType.UNMAP); + libvirtComputingResourceSpy.applyDiskDiscardDetail(disk, Map.of(VmDetailConstants.DISK_DISCARD, "IGNORE")); + assertEquals(DiskDef.DiscardType.IGNORE, disk.getDiscard()); + assertFalse(disk.toString().contains("discard=")); + } + + @Test + public void applyDiskDiscardDetailWithoutDetailKeepsDiscard() { + DiskDef disk = virtioDisk(); + disk.setDiscard(DiskDef.DiscardType.UNMAP); + libvirtComputingResourceSpy.applyDiskDiscardDetail(disk, new HashMap<>()); + libvirtComputingResourceSpy.applyDiskDiscardDetail(disk, null); + assertEquals(DiskDef.DiscardType.UNMAP, disk.getDiscard()); + } + + @Test + public void applyDiskDiscardDetailUnknownValueKeepsDiscard() { + DiskDef disk = virtioDisk(); + libvirtComputingResourceSpy.applyDiskDiscardDetail(disk, Map.of(VmDetailConstants.DISK_DISCARD, "on")); + assertEquals(DiskDef.DiscardType.IGNORE, disk.getDiscard()); } @Test diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java index 856dc0be9dcf..77eb72383885 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java @@ -48,6 +48,21 @@ public class LibvirtVMDefTest extends TestCase { "Active: 4260808 kB\n" + "Inactive: 949392 kB\n"; + @Test + public void testDiskBusFromValueReadsVirtioBlkAsVirtio() { + assertEquals(DiskDef.DiskBus.VIRTIO, DiskDef.DiskBus.fromValue("virtio-blk")); + assertEquals(DiskDef.DiskBus.SCSI, DiskDef.DiskBus.fromValue("SCSI")); + assertNull(DiskDef.DiskBus.fromValue("osdefault")); + } + + @Test + public void testDiscardTypeFromValue() { + assertEquals(DiskDef.DiscardType.UNMAP, DiskDef.DiscardType.fromValue("unmap")); + assertEquals(DiskDef.DiscardType.IGNORE, DiskDef.DiscardType.fromValue("Ignore")); + assertNull(DiskDef.DiscardType.fromValue("on")); + assertNull(DiskDef.DiscardType.fromValue(null)); + } + @Test public void testInterfaceTypeUserWithNetwork() { LibvirtVMDef.InterfaceDef interfaceDef = new LibvirtVMDef.InterfaceDef(); diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessorTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessorTest.java index 0159deda3476..76e0a71f0b4a 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessorTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessorTest.java @@ -27,6 +27,7 @@ import com.cloud.utils.Pair; import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.utils.script.Script; +import com.cloud.vm.VmDetailConstants; import org.apache.cloudstack.storage.to.SnapshotObjectTO; import org.apache.cloudstack.storage.to.VolumeObjectTO; import org.apache.cloudstack.utils.qemu.QemuImageOptions; @@ -487,4 +488,11 @@ public void attachOrDetachDeviceTestDetachThrowInternalErrorException() throws L attachOrDetachDeviceTest( false, "vmName", diskDefMock); Mockito.verify(domainMock, Mockito.times(1)).detachDevice(Mockito.anyString()); } + + @Test + public void getAttachDiskBusTypeReadsVirtioBlkControllerAsVirtio() { + Map controllerInfo = Map.of(VmDetailConstants.DATA_DISK_CONTROLLER, "virtio-blk"); + Assert.assertEquals(LibvirtVMDef.DiskDef.DiskBus.VIRTIO, + storageProcessor.getAttachDiskBusType(1, new ArrayList<>(), controllerInfo)); + } } diff --git a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java index 4fab347869b0..da27b075c1c7 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -5125,12 +5125,13 @@ private void fillVMOrTemplateDetailOptions(final Map> optio if (HypervisorType.KVM.equals(hypervisorType)) { options.put(VmDetailConstants.CPU_THREAD_PER_CORE, Collections.emptyList()); options.put(VmDetailConstants.NIC_ADAPTER, Arrays.asList("e1000", "virtio", "rtl8139", "vmxnet3", "ne2k_pci")); - options.put(VmDetailConstants.ROOT_DISK_CONTROLLER, Arrays.asList("osdefault", "ide", "scsi", "virtio", "virtio-blk")); - options.put(VmDetailConstants.DATA_DISK_CONTROLLER, Arrays.asList("osdefault", "ide", "scsi", "virtio", "virtio-blk")); + options.put(VmDetailConstants.ROOT_DISK_CONTROLLER, Arrays.asList("osdefault", "ide", "scsi", "virtio")); + options.put(VmDetailConstants.DATA_DISK_CONTROLLER, Arrays.asList("osdefault", "ide", "scsi", "virtio")); options.put(VmDetailConstants.VIDEO_HARDWARE, Arrays.asList("cirrus", "vga", "qxl", "virtio")); options.put(VmDetailConstants.VIDEO_RAM, Collections.emptyList()); options.put(VmDetailConstants.IO_POLICY, Arrays.asList("threads", "native", "io_uring", "storage_specific")); options.put(VmDetailConstants.IOTHREADS, List.of("enabled")); + options.put(VmDetailConstants.DISK_DISCARD, Arrays.asList("unmap", "ignore")); options.put(VmDetailConstants.NIC_MULTIQUEUE_NUMBER, Collections.emptyList()); options.put(VmDetailConstants.NIC_PACKED_VIRTQUEUES_ENABLED, Arrays.asList("true", "false")); options.put(VmDetailConstants.VIRTUAL_TPM_MODEL, Arrays.asList("tpm-tis", "tpm-crb")); diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 2fc43d13b46a..f54dc98f4db3 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -4688,6 +4688,11 @@ private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, L details.put(VmDetailConstants.IO_POLICY, ioPolicy); } + String diskDiscard = getDiskDiscard(vm); + if (diskDiscard != null) { + details.put(VmDetailConstants.DISK_DISCARD, diskDiscard); + } + if (chapInfo != null) { details.put(DiskTO.CHAP_INITIATOR_USERNAME, chapInfo.getInitiatorUsername()); details.put(DiskTO.CHAP_INITIATOR_SECRET, chapInfo.getInitiatorSecret()); @@ -4805,6 +4810,13 @@ private boolean isIothreadsSupported(UserVmVO vm) { && vm.getDetail(VmDetailConstants.IOTHREADS) != null; } + private String getDiskDiscard(UserVmVO vm) { + if (vm.getHypervisorType() != HypervisorType.KVM || vm.getDetails() == null) { + return null; + } + return vm.getDetail(VmDetailConstants.DISK_DISCARD); + } + private String getIoPolicy(UserVmVO vm, long poolId) { String ioPolicy = null; if (vm.getHypervisorType() == HypervisorType.KVM && vm.getDetails() != null && vm.getDetail(VmDetailConstants.IO_POLICY) != null) { diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 54ba6f0e872b..6e4d0e6998b5 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -4724,6 +4724,10 @@ private UserVmVO commitUserVm(final boolean isImport, final DataCenter zone, fin vm.details.remove(VmDetailConstants.IO_POLICY); continue; } + if (key.equalsIgnoreCase(VmDetailConstants.DISK_DISCARD)) { + vm.details.remove(VmDetailConstants.DISK_DISCARD); + continue; + } } if (key.equalsIgnoreCase(VmDetailConstants.CPU_NUMBER) || diff --git a/ui/src/views/image/UpdateTemplate.vue b/ui/src/views/image/UpdateTemplate.vue index 590e8c233c8d..01befdecc854 100644 --- a/ui/src/views/image/UpdateTemplate.vue +++ b/ui/src/views/image/UpdateTemplate.vue @@ -356,10 +356,6 @@ export default { id: 'virtio', description: 'virtio' }) - controller.push({ - id: 'virtio-blk', - description: 'virtio-blk' - }) } else if (hyperVisor === 'VMware') { controller.push({ id: '',