You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2022/04/13 07:13:59 UTC
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #6012: Add volume group handling
DaanHoogland commented on code in PR #6012:
URL: https://github.com/apache/cloudstack/pull/6012#discussion_r849144950
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -883,7 +886,7 @@ public VolumeVO createVolume(CreateVolumeCmd cmd) {
// if VM Id is provided, attach the volume to the VM
if (cmd.getVirtualMachineId() != null) {
try {
- attachVolumeToVM(cmd.getVirtualMachineId(), volume.getId(), volume.getDeviceId());
+ attachVolumeToVM(cmd.getVirtualMachineId(), volume.getId(), volume.getDeviceId(),-1);
Review Comment:
```suggestion
attachVolumeToVM(cmd.getVirtualMachineId(), volume.getId(), volume.getDeviceId(), -1);
```
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -224,6 +225,8 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
@Inject
private VolumeDetailsDao _volsDetailsDao;
@Inject
+ private VolumeGroupDao _volsGroupDao;
Review Comment:
I don't like or see the use of this `_` naming convention, let's break with it! (no :-1: on this PR in any way!)
##########
vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java:
##########
@@ -2340,23 +2340,23 @@ public boolean isPvScsiSupported() throws Exception {
}
// Would be useful if there exists multiple sub types of SCSI controllers per VM are supported in CloudStack f
- public int getScsiDiskControllerKey(String diskController) throws Exception {
+ public int getScsiDiskControllerKey(String diskController, int groupNumber) throws Exception {
List<VirtualDevice> devices = (List<VirtualDevice>)_context.getVimClient().getDynamicProperty(_mor, "config.hardware.device");
if (CollectionUtils.isNotEmpty(devices)) {
DiskControllerType diskControllerType = DiskControllerType.getType(diskController);
for (VirtualDevice device : devices) {
if ((diskControllerType == DiskControllerType.lsilogic || diskControllerType == DiskControllerType.scsi)
- && device instanceof VirtualLsiLogicController && isValidScsiDiskController((VirtualLsiLogicController)device)) {
+ && device instanceof VirtualLsiLogicController && isValidScsiDiskController((VirtualLsiLogicController)device,groupNumber)) {
Review Comment:
```suggestion
&& device instanceof VirtualLsiLogicController && isValidScsiDiskController((VirtualLsiLogicController)device, groupNumber)) {
```
##########
vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java:
##########
@@ -2375,35 +2375,31 @@ public int getScsiDiskControllerKeyNoException(String diskController, int scsiUn
DiskControllerType diskControllerType = DiskControllerType.getType(diskController);
for (VirtualDevice device : devices) {
if ((diskControllerType == DiskControllerType.lsilogic || diskControllerType == DiskControllerType.scsi) && device instanceof VirtualLsiLogicController) {
- if (scsiControllerDeviceCount == requiredScsiController) {
- if (isValidScsiDiskController((VirtualLsiLogicController)device)) {
+ if (scsiControllerDeviceCount == requiredScsiController || groupNumber > -1) {
+ if (isValidScsiDiskController((VirtualLsiLogicController)device,groupNumber)) {
return ((VirtualLsiLogicController)device).getKey();
}
- break;
}
scsiControllerDeviceCount++;
} else if ((diskControllerType == DiskControllerType.lsisas1068 || diskControllerType == DiskControllerType.scsi) && device instanceof VirtualLsiLogicSASController) {
- if (scsiControllerDeviceCount == requiredScsiController) {
- if (isValidScsiDiskController((VirtualLsiLogicSASController)device)) {
+ if (scsiControllerDeviceCount == requiredScsiController || groupNumber > -1) {
+ if (isValidScsiDiskController((VirtualLsiLogicSASController)device,groupNumber)) {
Review Comment:
```suggestion
if (isValidScsiDiskController((VirtualLsiLogicSASController)device, groupNumber)) {
```
##########
vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java:
##########
@@ -2413,18 +2409,18 @@ public int getScsiDiskControllerKeyNoException(String diskController, int scsiUn
}
public int getNextScsiDiskDeviceNumber() throws Exception {
- int scsiControllerKey = getScsiDeviceControllerKey();
+ int scsiControllerKey = getScsiDeviceControllerKey(-1);
int deviceNumber = getNextDeviceNumber(scsiControllerKey);
return deviceNumber;
}
- public int getScsiDeviceControllerKey() throws Exception {
+ public int getScsiDeviceControllerKey(int groupNumber) throws Exception {
List<VirtualDevice> devices = _context.getVimClient().getDynamicProperty(_mor, "config.hardware.device");
if (devices != null && devices.size() > 0) {
for (VirtualDevice device : devices) {
- if (device instanceof VirtualSCSIController && isValidScsiDiskController((VirtualSCSIController)device)) {
+ if (device instanceof VirtualSCSIController && isValidScsiDiskController((VirtualSCSIController)device,groupNumber)) {
Review Comment:
```suggestion
if (device instanceof VirtualSCSIController && isValidScsiDiskController((VirtualSCSIController)device, groupNumber)) {
```
##########
server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java:
##########
@@ -935,9 +940,9 @@ private void publishVMUsageUpdateResourceCount(final UserVm userVm, ServiceOffer
private UserVm importVirtualMachineInternal(final UnmanagedInstanceTO unmanagedInstance, final String instanceName, final DataCenter zone, final Cluster cluster, final HostVO host,
final VirtualMachineTemplate template, final String displayName, final String hostName, final Account caller, final Account owner, final Long userId,
- final ServiceOfferingVO serviceOffering, final Map<String, Long> dataDiskOfferingMap,
+ final ServiceOfferingVO serviceOffering, final Map<String, Long> dataDiskOfferingMap, final Map<String,Integer> diskVolumeGroupMap,
Review Comment:
```suggestion
final ServiceOfferingVO serviceOffering, final Map<String, Long> dataDiskOfferingMap, final Map<String, Integer> diskVolumeGroupMap,
```
##########
vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java:
##########
@@ -2340,23 +2340,23 @@ public boolean isPvScsiSupported() throws Exception {
}
// Would be useful if there exists multiple sub types of SCSI controllers per VM are supported in CloudStack f
- public int getScsiDiskControllerKey(String diskController) throws Exception {
+ public int getScsiDiskControllerKey(String diskController, int groupNumber) throws Exception {
List<VirtualDevice> devices = (List<VirtualDevice>)_context.getVimClient().getDynamicProperty(_mor, "config.hardware.device");
if (CollectionUtils.isNotEmpty(devices)) {
DiskControllerType diskControllerType = DiskControllerType.getType(diskController);
for (VirtualDevice device : devices) {
if ((diskControllerType == DiskControllerType.lsilogic || diskControllerType == DiskControllerType.scsi)
- && device instanceof VirtualLsiLogicController && isValidScsiDiskController((VirtualLsiLogicController)device)) {
+ && device instanceof VirtualLsiLogicController && isValidScsiDiskController((VirtualLsiLogicController)device,groupNumber)) {
return ((VirtualLsiLogicController)device).getKey();
} else if ((diskControllerType == DiskControllerType.lsisas1068 || diskControllerType == DiskControllerType.scsi)
- && device instanceof VirtualLsiLogicSASController && isValidScsiDiskController((VirtualLsiLogicSASController)device)) {
+ && device instanceof VirtualLsiLogicSASController && isValidScsiDiskController((VirtualLsiLogicSASController)device,groupNumber)) {
return ((VirtualLsiLogicSASController)device).getKey();
} else if ((diskControllerType == DiskControllerType.pvscsi || diskControllerType == DiskControllerType.scsi)
- && device instanceof ParaVirtualSCSIController && isValidScsiDiskController((ParaVirtualSCSIController)device)) {
+ && device instanceof ParaVirtualSCSIController && isValidScsiDiskController((ParaVirtualSCSIController)device,groupNumber)) {
Review Comment:
```suggestion
&& device instanceof ParaVirtualSCSIController && isValidScsiDiskController((ParaVirtualSCSIController)device, groupNumber)) {
```
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -3372,6 +3378,7 @@ private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, L
DiskTO disk = answer.getDisk();
_volsDao.attachVolume(volumeToAttach.getId(), vm.getId(), disk.getDiskSeq());
+ _volsGroupDao.addVolumeToGroup(vm.getId(),volumeToAttach.getId(),deviceId,groupNumber);
Review Comment:
```suggestion
_volsGroupDao.addVolumeToGroup(vm.getId(), volumeToAttach.getId(), deviceId,groupNumber);
```
##########
server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java:
##########
@@ -1024,15 +1029,19 @@ private UserVm importVirtualMachineInternal(final UnmanagedInstanceTO unmanagedI
}
diskProfileStoragePoolList.add(importDisk(rootDisk, userVm, cluster, serviceOffering, Volume.Type.ROOT, String.format("ROOT-%d", userVm.getId()),
(rootDisk.getCapacity() / Resource.ResourceType.bytesToGiB), minIops, maxIops,
- template, owner, null));
+ template, owner, null, null, false));
for (UnmanagedInstanceTO.Disk disk : dataDisks) {
+ Integer volumeGroup = -1;
+ if(diskVolumeGroupMap != null && !diskVolumeGroupMap.isEmpty()){
Review Comment:
```suggestion
if (diskVolumeGroupMap != null && !diskVolumeGroupMap.isEmpty()) {
```
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/vm/ImportUnmanagedInstanceCmd.java:
##########
@@ -158,6 +158,11 @@ public class ImportUnmanagedInstanceCmd extends BaseAsyncCmd {
description = "VM is imported despite some of its NIC's MAC addresses are already present")
private Boolean forced;
+ @Parameter(name = ApiConstants.USE_CONTROLLER_CONFIGURATION,
+ type = CommandType.BOOLEAN,
+ description = "volumes automatically get volume groups based on their current controller connections")
Review Comment:
```suggestion
@Parameter(name = ApiConstants.USE_CONTROLLER_CONFIGURATION,
type = CommandType.BOOLEAN,
description = "volumes automatically get volume groups based on their current controller connections",
since="4.17.0")
```
##########
vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java:
##########
@@ -2340,23 +2340,23 @@ public boolean isPvScsiSupported() throws Exception {
}
// Would be useful if there exists multiple sub types of SCSI controllers per VM are supported in CloudStack f
- public int getScsiDiskControllerKey(String diskController) throws Exception {
+ public int getScsiDiskControllerKey(String diskController, int groupNumber) throws Exception {
List<VirtualDevice> devices = (List<VirtualDevice>)_context.getVimClient().getDynamicProperty(_mor, "config.hardware.device");
if (CollectionUtils.isNotEmpty(devices)) {
DiskControllerType diskControllerType = DiskControllerType.getType(diskController);
for (VirtualDevice device : devices) {
if ((diskControllerType == DiskControllerType.lsilogic || diskControllerType == DiskControllerType.scsi)
- && device instanceof VirtualLsiLogicController && isValidScsiDiskController((VirtualLsiLogicController)device)) {
+ && device instanceof VirtualLsiLogicController && isValidScsiDiskController((VirtualLsiLogicController)device,groupNumber)) {
return ((VirtualLsiLogicController)device).getKey();
} else if ((diskControllerType == DiskControllerType.lsisas1068 || diskControllerType == DiskControllerType.scsi)
- && device instanceof VirtualLsiLogicSASController && isValidScsiDiskController((VirtualLsiLogicSASController)device)) {
+ && device instanceof VirtualLsiLogicSASController && isValidScsiDiskController((VirtualLsiLogicSASController)device,groupNumber)) {
Review Comment:
```suggestion
&& device instanceof VirtualLsiLogicSASController && isValidScsiDiskController((VirtualLsiLogicSASController)device, groupNumber)) {
```
##########
vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java:
##########
@@ -1421,9 +1421,9 @@ public void attachDisk(String[] vmdkDatastorePathChain, ManagedObjectReference m
unitNumber = getFreeUnitNumberOnIDEController(controllerKey);
} else {
if (StringUtils.isNotBlank(diskController)) {
- controllerKey = getScsiDiskControllerKey(diskController);
+ controllerKey = getScsiDiskControllerKey(diskController,groupNumber);
Review Comment:
```suggestion
controllerKey = getScsiDiskControllerKey(diskController, groupNumber);
```
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -3390,6 +3397,7 @@ private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, L
deviceId = getDeviceId(vm, deviceId);
_volsDao.attachVolume(volumeToAttach.getId(), vm.getId(), deviceId);
+ _volsGroupDao.addVolumeToGroup(vm.getId(),volumeToAttach.getId(),deviceId,groupNumber);
Review Comment:
```suggestion
_volsGroupDao.addVolumeToGroup(vm.getId(), volumeToAttach.getId(), deviceId,groupNumber);
```
##########
vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java:
##########
@@ -2340,23 +2340,23 @@ public boolean isPvScsiSupported() throws Exception {
}
// Would be useful if there exists multiple sub types of SCSI controllers per VM are supported in CloudStack f
- public int getScsiDiskControllerKey(String diskController) throws Exception {
+ public int getScsiDiskControllerKey(String diskController, int groupNumber) throws Exception {
List<VirtualDevice> devices = (List<VirtualDevice>)_context.getVimClient().getDynamicProperty(_mor, "config.hardware.device");
if (CollectionUtils.isNotEmpty(devices)) {
DiskControllerType diskControllerType = DiskControllerType.getType(diskController);
for (VirtualDevice device : devices) {
if ((diskControllerType == DiskControllerType.lsilogic || diskControllerType == DiskControllerType.scsi)
- && device instanceof VirtualLsiLogicController && isValidScsiDiskController((VirtualLsiLogicController)device)) {
+ && device instanceof VirtualLsiLogicController && isValidScsiDiskController((VirtualLsiLogicController)device,groupNumber)) {
return ((VirtualLsiLogicController)device).getKey();
} else if ((diskControllerType == DiskControllerType.lsisas1068 || diskControllerType == DiskControllerType.scsi)
- && device instanceof VirtualLsiLogicSASController && isValidScsiDiskController((VirtualLsiLogicSASController)device)) {
+ && device instanceof VirtualLsiLogicSASController && isValidScsiDiskController((VirtualLsiLogicSASController)device,groupNumber)) {
return ((VirtualLsiLogicSASController)device).getKey();
} else if ((diskControllerType == DiskControllerType.pvscsi || diskControllerType == DiskControllerType.scsi)
- && device instanceof ParaVirtualSCSIController && isValidScsiDiskController((ParaVirtualSCSIController)device)) {
+ && device instanceof ParaVirtualSCSIController && isValidScsiDiskController((ParaVirtualSCSIController)device,groupNumber)) {
return ((ParaVirtualSCSIController)device).getKey();
} else if ((diskControllerType == DiskControllerType.buslogic || diskControllerType == DiskControllerType.scsi)
- && device instanceof VirtualBusLogicController && isValidScsiDiskController((VirtualBusLogicController)device)) {
+ && device instanceof VirtualBusLogicController && isValidScsiDiskController((VirtualBusLogicController)device,groupNumber)) {
Review Comment:
```suggestion
&& device instanceof VirtualBusLogicController && isValidScsiDiskController((VirtualBusLogicController)device, groupNumber)) {
```
##########
vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java:
##########
@@ -2375,35 +2375,31 @@ public int getScsiDiskControllerKeyNoException(String diskController, int scsiUn
DiskControllerType diskControllerType = DiskControllerType.getType(diskController);
for (VirtualDevice device : devices) {
if ((diskControllerType == DiskControllerType.lsilogic || diskControllerType == DiskControllerType.scsi) && device instanceof VirtualLsiLogicController) {
- if (scsiControllerDeviceCount == requiredScsiController) {
- if (isValidScsiDiskController((VirtualLsiLogicController)device)) {
+ if (scsiControllerDeviceCount == requiredScsiController || groupNumber > -1) {
+ if (isValidScsiDiskController((VirtualLsiLogicController)device,groupNumber)) {
return ((VirtualLsiLogicController)device).getKey();
}
- break;
}
scsiControllerDeviceCount++;
} else if ((diskControllerType == DiskControllerType.lsisas1068 || diskControllerType == DiskControllerType.scsi) && device instanceof VirtualLsiLogicSASController) {
- if (scsiControllerDeviceCount == requiredScsiController) {
- if (isValidScsiDiskController((VirtualLsiLogicSASController)device)) {
+ if (scsiControllerDeviceCount == requiredScsiController || groupNumber > -1) {
+ if (isValidScsiDiskController((VirtualLsiLogicSASController)device,groupNumber)) {
return ((VirtualLsiLogicSASController)device).getKey();
}
- break;
}
scsiControllerDeviceCount++;
} else if ((diskControllerType == DiskControllerType.pvscsi || diskControllerType == DiskControllerType.scsi) && device instanceof ParaVirtualSCSIController) {
- if (scsiControllerDeviceCount == requiredScsiController) {
- if (isValidScsiDiskController((ParaVirtualSCSIController)device)) {
+ if (scsiControllerDeviceCount == requiredScsiController || groupNumber > -1) {
+ if (isValidScsiDiskController((ParaVirtualSCSIController)device,groupNumber)) {
return ((ParaVirtualSCSIController)device).getKey();
}
- break;
}
scsiControllerDeviceCount++;
} else if ((diskControllerType == DiskControllerType.buslogic || diskControllerType == DiskControllerType.scsi) && device instanceof VirtualBusLogicController) {
- if (scsiControllerDeviceCount == requiredScsiController) {
- if (isValidScsiDiskController((VirtualBusLogicController)device)) {
+ if (scsiControllerDeviceCount == requiredScsiController || groupNumber > -1) {
+ if (isValidScsiDiskController((VirtualBusLogicController)device,groupNumber)) {
Review Comment:
```suggestion
if (isValidScsiDiskController((VirtualBusLogicController)device, groupNumber)) {
```
##########
vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java:
##########
@@ -2375,35 +2375,31 @@ public int getScsiDiskControllerKeyNoException(String diskController, int scsiUn
DiskControllerType diskControllerType = DiskControllerType.getType(diskController);
for (VirtualDevice device : devices) {
if ((diskControllerType == DiskControllerType.lsilogic || diskControllerType == DiskControllerType.scsi) && device instanceof VirtualLsiLogicController) {
- if (scsiControllerDeviceCount == requiredScsiController) {
- if (isValidScsiDiskController((VirtualLsiLogicController)device)) {
+ if (scsiControllerDeviceCount == requiredScsiController || groupNumber > -1) {
+ if (isValidScsiDiskController((VirtualLsiLogicController)device,groupNumber)) {
return ((VirtualLsiLogicController)device).getKey();
}
- break;
}
scsiControllerDeviceCount++;
} else if ((diskControllerType == DiskControllerType.lsisas1068 || diskControllerType == DiskControllerType.scsi) && device instanceof VirtualLsiLogicSASController) {
- if (scsiControllerDeviceCount == requiredScsiController) {
- if (isValidScsiDiskController((VirtualLsiLogicSASController)device)) {
+ if (scsiControllerDeviceCount == requiredScsiController || groupNumber > -1) {
+ if (isValidScsiDiskController((VirtualLsiLogicSASController)device,groupNumber)) {
return ((VirtualLsiLogicSASController)device).getKey();
}
- break;
}
scsiControllerDeviceCount++;
} else if ((diskControllerType == DiskControllerType.pvscsi || diskControllerType == DiskControllerType.scsi) && device instanceof ParaVirtualSCSIController) {
- if (scsiControllerDeviceCount == requiredScsiController) {
- if (isValidScsiDiskController((ParaVirtualSCSIController)device)) {
+ if (scsiControllerDeviceCount == requiredScsiController || groupNumber > -1) {
+ if (isValidScsiDiskController((ParaVirtualSCSIController)device,groupNumber)) {
Review Comment:
```suggestion
if (isValidScsiDiskController((ParaVirtualSCSIController)device, groupNumber)) {
```
##########
vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java:
##########
@@ -2439,7 +2435,7 @@ public int getScsiDeviceControllerKeyNoException() throws Exception {
if (devices != null && devices.size() > 0) {
for (VirtualDevice device : devices) {
- if (device instanceof VirtualSCSIController && isValidScsiDiskController((VirtualSCSIController)device)) {
+ if (device instanceof VirtualSCSIController && isValidScsiDiskController((VirtualSCSIController)device,-1)) {
Review Comment:
```suggestion
if (device instanceof VirtualSCSIController && isValidScsiDiskController((VirtualSCSIController)device, -1)) {
```
##########
vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java:
##########
@@ -2375,35 +2375,31 @@ public int getScsiDiskControllerKeyNoException(String diskController, int scsiUn
DiskControllerType diskControllerType = DiskControllerType.getType(diskController);
for (VirtualDevice device : devices) {
if ((diskControllerType == DiskControllerType.lsilogic || diskControllerType == DiskControllerType.scsi) && device instanceof VirtualLsiLogicController) {
- if (scsiControllerDeviceCount == requiredScsiController) {
- if (isValidScsiDiskController((VirtualLsiLogicController)device)) {
+ if (scsiControllerDeviceCount == requiredScsiController || groupNumber > -1) {
+ if (isValidScsiDiskController((VirtualLsiLogicController)device,groupNumber)) {
Review Comment:
```suggestion
if (isValidScsiDiskController((VirtualLsiLogicController)device, groupNumber)) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org