kvm: add a disk.discard VM detail and drop the virtio-blk disk controller

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";

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<String, String> 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<String, String> 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;

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java

@@ -679,14 +679,19 @@ 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) {
                 _bus = bus;
             }
 
             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) {

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<DiskDef> 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;

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

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();

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<String, String> controllerInfo = Map.of(VmDetailConstants.DATA_DISK_CONTROLLER, "virtio-blk");
+        Assert.assertEquals(LibvirtVMDef.DiskDef.DiskBus.VIRTIO,
+                storageProcessor.getAttachDiskBusType(1, new ArrayList<>(), controllerInfo));
+    }
 }

server/src/main/java/com/cloud/api/query/QueryManagerImpl.java

@@ -5125,12 +5125,13 @@ private void fillVMOrTemplateDetailOptions(final Map<String, List<String>> 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"));

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) {

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) ||

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: '',