kvm: honour data disk controller for vm during attach volume

Fixes #12090

Some years back #4569 allowed honouring dataDiskController for KVM VMs
during start but the same behviour is not seen while attaching a
volume to a running VM.
This PR first checks in the AttachCommand if a valid controller is
present. If a valid controller/bus type is not present than existing
logic is used.

Signed-off-by: Abhishek Kumar <[email protected]>

Author: shwstppr

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

@@ -4371,12 +4371,11 @@ public DiskDef.DiskBus getDataDiskModelFromVMDetail(final VirtualMachineTO vmTO)
 
         String dataDiskController = details.get(VmDetailConstants.DATA_DISK_CONTROLLER);
         if (StringUtils.isNotBlank(dataDiskController)) {
-            LOGGER.debug("Passed custom disk controller for DATA disk " + dataDiskController);
-            for (DiskDef.DiskBus bus : DiskDef.DiskBus.values()) {
-                if (bus.toString().equalsIgnoreCase(dataDiskController)) {
-                    LOGGER.debug("Found matching enum for disk controller for DATA disk " + dataDiskController);
-                    return bus;
-                }
+            LOGGER.debug("Passed custom disk controller for DATA disk {}", dataDiskController);
+            DiskDef.DiskBus bus = DiskDef.DiskBus.fromValue(dataDiskController);
+            if (bus != null) {
+                LOGGER.debug("Found matching enum for disk controller for DATA disk {}", dataDiskController);
+                return bus;
             }
         }
         return null;

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

@@ -686,6 +686,15 @@ public enum DiskBus {
                 _bus = bus;
             }
 
+            public static DiskBus fromValue(String bus) {
+                for (DiskBus b : DiskBus.values()) {
+                    if (b.toString().equalsIgnoreCase(bus)) {
+                        return b;
+                    }
+                }
+                return null;
+            }
+
             @Override
             public String toString() {
                 return _bus;

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java

@@ -1320,64 +1320,68 @@ protected boolean checkDetachSuccess(String diskPath, Domain dm) throws LibvirtE
 
     /**
      * Attaches or detaches a disk to an instance.
-     * @param conn libvirt connection
-     * @param attach boolean that determines whether the device will be attached or detached
-     * @param vmName instance name
-     * @param attachingDisk kvm physical disk
-     * @param devId device id in instance
+     *
+     * @param conn                    libvirt connection
+     * @param attach                  boolean that determines whether the device will be attached or detached
+     * @param vmName                  instance name
+     * @param attachingDisk           kvm physical disk
+     * @param devId                   device id in instance
      * @param serial
-     * @param bytesReadRate bytes read rate
-     * @param bytesReadRateMax bytes read rate max
-     * @param bytesReadRateMaxLength bytes read rate max length
-     * @param bytesWriteRate bytes write rate
-     * @param bytesWriteRateMax bytes write rate amx
+     * @param bytesReadRate           bytes read rate
+     * @param bytesReadRateMax        bytes read rate max
+     * @param bytesReadRateMaxLength  bytes read rate max length
+     * @param bytesWriteRate          bytes write rate
+     * @param bytesWriteRateMax       bytes write rate amx
      * @param bytesWriteRateMaxLength bytes write rate max length
-     * @param iopsReadRate iops read rate
-     * @param iopsReadRateMax iops read rate max
-     * @param iopsReadRateMaxLength iops read rate max length
-     * @param iopsWriteRate iops write rate
-     * @param iopsWriteRateMax iops write rate max
-     * @param iopsWriteRateMaxLength iops write rate max length
-     * @param cacheMode cache mode
-     * @param encryptDetails encrypt details
+     * @param iopsReadRate            iops read rate
+     * @param iopsReadRateMax         iops read rate max
+     * @param iopsReadRateMaxLength   iops read rate max length
+     * @param iopsWriteRate           iops write rate
+     * @param iopsWriteRateMax        iops write rate max
+     * @param iopsWriteRateMaxLength  iops write rate max length
+     * @param cacheMode               cache mode
+     * @param encryptDetails          encrypt details
+     * @param controllerInfo
      * @throws LibvirtException
      * @throws InternalErrorException
      */
     protected synchronized void attachOrDetachDisk(final Connect conn, final boolean attach, final String vmName, final KVMPhysicalDisk attachingDisk, final int devId,
                                                    final String serial, final Long bytesReadRate, final Long bytesReadRateMax, final Long bytesReadRateMaxLength,
                                                    final Long bytesWriteRate, final Long bytesWriteRateMax, final Long bytesWriteRateMaxLength, final Long iopsReadRate,
                                                    final Long iopsReadRateMax, final Long iopsReadRateMaxLength, final Long iopsWriteRate, final Long iopsWriteRateMax,
-                                                   final Long iopsWriteRateMaxLength, final String cacheMode, final DiskDef.LibvirtDiskEncryptDetails encryptDetails, Map<String, String> details)
+                                                   final Long iopsWriteRateMaxLength, final String cacheMode, final DiskDef.LibvirtDiskEncryptDetails encryptDetails, Map<String, String> details, Map<String, String> controllerInfo)
             throws LibvirtException, InternalErrorException {
         attachOrDetachDisk(conn, attach, vmName, attachingDisk, devId, serial, bytesReadRate, bytesReadRateMax, bytesReadRateMaxLength,
                 bytesWriteRate, bytesWriteRateMax, bytesWriteRateMaxLength, iopsReadRate, iopsReadRateMax, iopsReadRateMaxLength, iopsWriteRate,
-                iopsWriteRateMax, iopsWriteRateMaxLength, cacheMode, encryptDetails, 0l, details);
+                iopsWriteRateMax, iopsWriteRateMaxLength, cacheMode, encryptDetails, 0l, details, controllerInfo);
     }
 
     /**
      *
      * Attaches or detaches a disk to an instance.
-     * @param conn libvirt connection
-     * @param attach boolean that determines whether the device will be attached or detached
-     * @param vmName instance name
-     * @param attachingDisk kvm physical disk
-     * @param devId device id in instance
+     *
+     * @param conn                    libvirt connection
+     * @param attach                  boolean that determines whether the device will be attached or detached
+     * @param vmName                  instance name
+     * @param attachingDisk           kvm physical disk
+     * @param devId                   device id in instance
      * @param serial
-     * @param bytesReadRate bytes read rate
-     * @param bytesReadRateMax bytes read rate max
-     * @param bytesReadRateMaxLength bytes read rate max length
-     * @param bytesWriteRate bytes write rate
-     * @param bytesWriteRateMax bytes write rate amx
+     * @param bytesReadRate           bytes read rate
+     * @param bytesReadRateMax        bytes read rate max
+     * @param bytesReadRateMaxLength  bytes read rate max length
+     * @param bytesWriteRate          bytes write rate
+     * @param bytesWriteRateMax       bytes write rate amx
      * @param bytesWriteRateMaxLength bytes write rate max length
-     * @param iopsReadRate iops read rate
-     * @param iopsReadRateMax iops read rate max
-     * @param iopsReadRateMaxLength iops read rate max length
-     * @param iopsWriteRate iops write rate
-     * @param iopsWriteRateMax iops write rate max
-     * @param iopsWriteRateMaxLength iops write rate max length
-     * @param cacheMode cache mode
-     * @param encryptDetails encrypt details
-     * @param waitDetachDevice value set in milliseconds to wait before assuming device removal failed
+     * @param iopsReadRate            iops read rate
+     * @param iopsReadRateMax         iops read rate max
+     * @param iopsReadRateMaxLength   iops read rate max length
+     * @param iopsWriteRate           iops write rate
+     * @param iopsWriteRateMax        iops write rate max
+     * @param iopsWriteRateMaxLength  iops write rate max length
+     * @param cacheMode               cache mode
+     * @param encryptDetails          encrypt details
+     * @param waitDetachDevice        value set in milliseconds to wait before assuming device removal failed
+     * @param controllerInfo
      * @throws LibvirtException
      * @throws InternalErrorException
      */
@@ -1386,7 +1390,7 @@ protected synchronized void attachOrDetachDisk(final Connect conn, final boolean
                                                    final Long bytesWriteRate, final Long bytesWriteRateMax, final Long bytesWriteRateMaxLength, final Long iopsReadRate,
                                                    final Long iopsReadRateMax, final Long iopsReadRateMaxLength, final Long iopsWriteRate, final Long iopsWriteRateMax,
                                                    final Long iopsWriteRateMaxLength, final String cacheMode, final DiskDef.LibvirtDiskEncryptDetails encryptDetails,
-                                                   long waitDetachDevice, Map<String, String> details)
+                                                   long waitDetachDevice, Map<String, String> details, Map<String, String> controllerInfo)
             throws LibvirtException, InternalErrorException {
 
         List<DiskDef> disks = null;
@@ -1423,17 +1427,7 @@ protected synchronized void attachOrDetachDisk(final Connect conn, final boolean
                     return;
                 }
             } else {
-                DiskDef.DiskBus busT = DiskDef.DiskBus.VIRTIO;
-                for (final DiskDef disk : disks) {
-                    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.DiskBus busT = getAttachDiskBusType(devId, disks, controllerInfo);
                 diskdef = new DiskDef();
                 if (busT == DiskDef.DiskBus.SCSI || busT == DiskDef.DiskBus.VIRTIOBLK) {
                     diskdef.setQemuDriver(true);
@@ -1538,6 +1532,28 @@ protected synchronized void attachOrDetachDisk(final Connect conn, final boolean
         }
     }
 
+    protected DiskDef.DiskBus getAttachDiskBusType(int deviceId, List<DiskDef> disks, Map<String, String> controllerInfo) {
+        String controllerKey = deviceId == 0 ? VmDetailConstants.ROOT_DISK_CONTROLLER : VmDetailConstants.DATA_DISK_CONTROLLER;
+        String diskController = MapUtils.getString(controllerInfo, controllerKey);
+        DiskDef.DiskBus busType = DiskDef.DiskBus.fromValue(diskController);
+        if (diskController != null) {
+            logger.debug("Using controller '{}' from command specified as {} while attaching disk (deviceId={})",
+                    diskController, controllerKey, deviceId);
+            return busType;
+        }
+        for (final DiskDef disk : disks) {
+            if (disk.getDeviceType() != DeviceType.DISK) {
+                continue;
+            }
+            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;
+    }
+
     @Override
     public Answer attachVolume(final AttachCommand cmd) {
         final DiskTO disk = cmd.getDisk();
@@ -1565,7 +1581,8 @@ public Answer attachVolume(final AttachCommand cmd) {
                     vol.getBytesReadRate(), vol.getBytesReadRateMax(), vol.getBytesReadRateMaxLength(),
                     vol.getBytesWriteRate(), vol.getBytesWriteRateMax(), vol.getBytesWriteRateMaxLength(),
                     vol.getIopsReadRate(), vol.getIopsReadRateMax(), vol.getIopsReadRateMaxLength(),
-                    vol.getIopsWriteRate(), vol.getIopsWriteRateMax(), vol.getIopsWriteRateMaxLength(), volCacheMode, encryptDetails, disk.getDetails());
+                    vol.getIopsWriteRate(), vol.getIopsWriteRateMax(), vol.getIopsWriteRateMaxLength(), volCacheMode,
+                    encryptDetails, disk.getDetails(), cmd.getControllerInfo());
 
             return new AttachAnswer(disk);
         } catch (final LibvirtException e) {
@@ -1602,7 +1619,7 @@ public Answer dettachVolume(final DettachCommand cmd) {
                     vol.getBytesReadRate(), vol.getBytesReadRateMax(), vol.getBytesReadRateMaxLength(),
                     vol.getBytesWriteRate(), vol.getBytesWriteRateMax(), vol.getBytesWriteRateMaxLength(),
                     vol.getIopsReadRate(), vol.getIopsReadRateMax(), vol.getIopsReadRateMaxLength(),
-                    vol.getIopsWriteRate(), vol.getIopsWriteRateMax(), vol.getIopsWriteRateMaxLength(), volCacheMode, null, waitDetachDevice, null);
+                    vol.getIopsWriteRate(), vol.getIopsWriteRateMax(), vol.getIopsWriteRateMaxLength(), volCacheMode, null, waitDetachDevice, null, null);
 
             storagePoolMgr.disconnectPhysicalDisk(primaryStore.getPoolType(), primaryStore.getUuid(), vol.getPath());