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..e7844801d647 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 @@ -3209,7 +3209,7 @@ public int compare(final DiskTO arg0, final DiskTO arg1) { DiskDef.DiskBus diskBusTypeData = getDataDiskModelFromVMDetail(vmSpec); if (diskBusTypeData == null) { - diskBusTypeData = (diskBusType == DiskDef.DiskBus.SCSI) ? diskBusType : DiskDef.DiskBus.VIRTIO; + diskBusTypeData = (diskBusType == DiskDef.DiskBus.SCSI || diskBusType == DiskDef.DiskBus.VIRTIOBLK) ? diskBusType : DiskDef.DiskBus.VIRTIO; } final DiskDef disk = new DiskDef(); @@ -3613,13 +3613,15 @@ public synchronized String attachOrDetachDisk(final Connect conn, if (disk.getDeviceType() == DeviceType.DISK) { if (disk.getBusType() == DiskDef.DiskBus.SCSI) { busT = DiskDef.DiskBus.SCSI; + } else if (disk.getBusType() == DiskDef.DiskBus.VIRTIOBLK) { + busT = DiskDef.DiskBus.VIRTIOBLK; } break; } } diskdef = new DiskDef(); - if (busT == DiskDef.DiskBus.SCSI) { + if (busT == DiskDef.DiskBus.SCSI || busT == DiskDef.DiskBus.VIRTIOBLK) { diskdef.setQemuDriver(true); diskdef.setDiscard(DiscardType.UNMAP); } 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..0dda85e6efbe 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 @@ -686,6 +686,17 @@ public enum DiskBus { _bus = bus; } + /** + * The name libvirt knows this bus by. It is the enum's own value everywhere except + * VIRTIOBLK: 'virtio-blk' is a CloudStack disk controller name, not one of libvirt's + * target buses (ide, scsi, virtio, xen, usb, sata, sd, fdc, uml), and a virtio-blk + * disk is a virtio one. Rendering the enum value instead makes libvirt reject the + * domain XML, so the VM cannot start. + */ + public String libvirtBus() { + return this == VIRTIOBLK ? VIRTIO._bus : _bus; + } + public static DiskBus fromValue(String bus) { for (DiskBus b : DiskBus.values()) { if (b.toString().equalsIgnoreCase(bus)) { @@ -845,7 +856,7 @@ private String getDevLabel(int devId, DiskBus bus, boolean forIso) { if (bus == DiskBus.SCSI) { return "sd" + getDevLabelSuffix(devId); - } else if (bus == DiskBus.VIRTIO) { + } else if (bus == DiskBus.VIRTIO || bus == DiskBus.VIRTIOBLK) { return "vd" + getDevLabelSuffix(devId); } else if (bus == DiskBus.SATA){ return "sd" + getDevLabelSuffix(devId); @@ -1192,7 +1203,7 @@ public String toString() { diskBuilder.append(String.format("io='%s' ", ioDriver)); } - if (isIothreadsEnabled && _bus == DiskBus.VIRTIO) { + if (isIothreadsEnabled && (_bus == DiskBus.VIRTIO || _bus == DiskBus.VIRTIOBLK)) { diskBuilder.append(String.format("iothread='%s' ", NUMBER_OF_IOTHREADS)); } diskBuilder.append("/>\n"); @@ -1235,7 +1246,7 @@ public String toString() { } diskBuilder.append("\n"); 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..d2618c9da992 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 @@ -1548,6 +1548,17 @@ protected DiskDef.DiskBus getAttachDiskBusType(int deviceId, List disks return DiskDef.DiskBus.VIRTIOBLK; } } + if (deviceId != 0 && DiskDef.DiskBus.VIRTIOBLK == DiskDef.DiskBus.fromValue( + MapUtils.getString(controllerInfo, VmDetailConstants.ROOT_DISK_CONTROLLER))) { + // A data disk with no controller of its own follows the root, as it already does when the + // VM starts. The scan above cannot see this case: a virtio-blk disk is rendered with + // libvirt's 'virtio' bus, so it is indistinguishable from a plain virtio disk in the domain + // XML, and the disk would silently lose discard until the next stop/start. The scan still + // wins when the running VM is on SCSI, so a detail changed after the VM started cannot + // put a virtio disk next to its SCSI ones. + logger.debug("Attaching disk (deviceId={}) with the root controller virtio-blk", deviceId); + return DiskDef.DiskBus.VIRTIOBLK; + } return DiskDef.DiskBus.VIRTIO; } 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..6cb5c245fb8c 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 @@ -19,6 +19,9 @@ package com.cloud.hypervisor.kvm.resource; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import java.io.File; import java.util.Arrays; import java.util.List; @@ -582,4 +585,71 @@ public void testTpmModel() { "\n" + "\n", tpmDef.toString()); } + + @Test + public void testDiskDefVirtioBlkUsesVirtioBusAndVdLabel() { + DiskDef disk = new DiskDef(); + disk.defFileBasedDisk("/var/lib/libvirt/images/disk.qcow2", 1, DiskDef.DiskBus.VIRTIOBLK, DiskDef.DiskFmtType.QCOW2); + + String xmlDef = disk.toString(); + // 'virtio-blk' is a CloudStack controller name; libvirt only accepts the bus 'virtio' and + // rejects the whole domain XML otherwise, which is what stopped these VMs from starting. + assertTrue(xmlDef, xmlDef.contains("")); + assertFalse(xmlDef, xmlDef.contains("virtio-blk")); + assertEquals(DiskDef.DiskBus.VIRTIOBLK, disk.getBusType()); + } + + @Test + public void testDiskDefVirtioBlkLabelsMatchVirtio() { + for (int devId = 0; devId < 4; devId++) { + DiskDef virtio = new DiskDef(); + virtio.defFileBasedDisk("/var/lib/libvirt/images/disk.qcow2", devId, DiskDef.DiskBus.VIRTIO, DiskDef.DiskFmtType.QCOW2); + DiskDef virtioBlk = new DiskDef(); + virtioBlk.defFileBasedDisk("/var/lib/libvirt/images/disk.qcow2", devId, DiskDef.DiskBus.VIRTIOBLK, DiskDef.DiskFmtType.QCOW2); + assertEquals(virtio.getDiskLabel(), virtioBlk.getDiskLabel()); + } + } + + @Test + public void testDiskDefVirtioBlkWithDiscardUnmap() { + DiskDef disk = new DiskDef(); + disk.defFileBasedDisk("/var/lib/libvirt/images/disk.qcow2", 0, DiskDef.DiskBus.VIRTIOBLK, DiskDef.DiskFmtType.QCOW2); + disk.setQemuDriver(true); + disk.setDiscard(DiskDef.DiscardType.UNMAP); + + String xmlDef = disk.toString(); + // the point of the virtio-blk controller: a virtio disk that passes the guest's discard through + assertTrue(xmlDef, xmlDef.contains("discard='unmap'")); + assertTrue(xmlDef, xmlDef.contains("bus='virtio'")); + assertFalse(xmlDef, xmlDef.contains("virtio-blk")); + } + + @Test + public void testDiskDefVirtioIsUnchanged() { + DiskDef disk = new DiskDef(); + disk.defFileBasedDisk("/var/lib/libvirt/images/disk.qcow2", 0, DiskDef.DiskBus.VIRTIO, DiskDef.DiskFmtType.QCOW2); + + String xmlDef = disk.toString(); + assertTrue(xmlDef, xmlDef.contains("")); + assertFalse(xmlDef, xmlDef.contains("discard=")); + } + + @Test + public void testDiskDefScsiIsUnchanged() { + DiskDef disk = new DiskDef(); + disk.defFileBasedDisk("/var/lib/libvirt/images/disk.qcow2", 0, DiskDef.DiskBus.SCSI, DiskDef.DiskFmtType.QCOW2); + + String xmlDef = disk.toString(); + assertTrue(xmlDef, xmlDef.contains("")); + } + + @Test + public void testDiskDefVirtioBlkGetsAnIothreadLikeVirtio() { + DiskDef disk = new DiskDef(); + disk.defFileBasedDisk("/var/lib/libvirt/images/disk.qcow2", 0, DiskDef.DiskBus.VIRTIOBLK, DiskDef.DiskFmtType.QCOW2); + disk.isIothreadsEnabled(true); + + String xmlDef = disk.toString(); + assertTrue(xmlDef, xmlDef.contains("iothread=")); + } } 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..e6b4d7be7a3f 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 @@ -18,6 +18,7 @@ */ package com.cloud.hypervisor.kvm.storage; +import com.cloud.vm.VmDetailConstants; import com.cloud.exception.InternalErrorException; import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; import com.cloud.hypervisor.kvm.resource.LibvirtDomainXMLParser; @@ -487,4 +488,48 @@ public void attachOrDetachDeviceTestDetachThrowInternalErrorException() throws L attachOrDetachDeviceTest( false, "vmName", diskDefMock); Mockito.verify(domainMock, Mockito.times(1)).detachDevice(Mockito.anyString()); } + + private LibvirtVMDef.DiskDef diskWithBus(LibvirtVMDef.DiskDef.DiskBus bus) { + LibvirtVMDef.DiskDef disk = new LibvirtVMDef.DiskDef(); + disk.defFileBasedDisk("/var/lib/libvirt/images/disk.qcow2", 0, bus, LibvirtVMDef.DiskDef.DiskFmtType.QCOW2); + return disk; + } + + @Test + public void getAttachDiskBusTypeUsesTheDataDiskControllerWhenItIsSet() { + Map controllerInfo = Map.of(VmDetailConstants.DATA_DISK_CONTROLLER, "virtio-blk"); + Assert.assertEquals(LibvirtVMDef.DiskDef.DiskBus.VIRTIOBLK, + storageProcessor.getAttachDiskBusType(1, List.of(diskWithBus(LibvirtVMDef.DiskDef.DiskBus.VIRTIO)), controllerInfo)); + } + + @Test + public void getAttachDiskBusTypeFollowsAVirtioBlkRootForDataDisks() { + // a virtio-blk disk renders with libvirt's 'virtio' bus, so scanning the domain XML cannot + // tell the two apart; without this the hot-plugged disk loses discard until the next start + Map controllerInfo = Map.of(VmDetailConstants.ROOT_DISK_CONTROLLER, "virtio-blk"); + Assert.assertEquals(LibvirtVMDef.DiskDef.DiskBus.VIRTIOBLK, + storageProcessor.getAttachDiskBusType(1, List.of(diskWithBus(LibvirtVMDef.DiskDef.DiskBus.VIRTIO)), controllerInfo)); + } + + @Test + public void getAttachDiskBusTypeKeepsVirtioForAVirtioRoot() { + Map controllerInfo = Map.of(VmDetailConstants.ROOT_DISK_CONTROLLER, "virtio"); + Assert.assertEquals(LibvirtVMDef.DiskDef.DiskBus.VIRTIO, + storageProcessor.getAttachDiskBusType(1, List.of(diskWithBus(LibvirtVMDef.DiskDef.DiskBus.VIRTIO)), controllerInfo)); + } + + @Test + public void getAttachDiskBusTypeStillDetectsScsiFromTheRunningDisks() { + Assert.assertEquals(LibvirtVMDef.DiskDef.DiskBus.SCSI, + storageProcessor.getAttachDiskBusType(1, List.of(diskWithBus(LibvirtVMDef.DiskDef.DiskBus.SCSI)), Map.of())); + } + + @Test + public void getAttachDiskBusTypePrefersTheRunningScsiDisksOverAVirtioBlkRootDetail() { + // the detail can be changed on a running VM; the disks it actually runs on win, so a SCSI VM + // does not get a virtio disk hot-plugged next to its sd* ones + Map controllerInfo = Map.of(VmDetailConstants.ROOT_DISK_CONTROLLER, "virtio-blk"); + Assert.assertEquals(LibvirtVMDef.DiskDef.DiskBus.SCSI, + storageProcessor.getAttachDiskBusType(1, List.of(diskWithBus(LibvirtVMDef.DiskDef.DiskBus.SCSI)), controllerInfo)); + } }