-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[VMware] apply IOPS in resize/migrate #7226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 4.22
Are you sure you want to change the base?
Changes from all commits
6c73a29
fd32974
936951c
c3f41a9
47f0cd1
acb645c
fcdeaa2
19deded
8a1ae6a
1684347
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,8 @@ | |
| private boolean shrinkOk; | ||
| private String vmInstance; | ||
| private String chainInfo; | ||
| private Long newMaxIops; | ||
| private Long newMinIops; | ||
|
Comment on lines
+36
to
+37
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we have max and min IOPs here but on the migrate volume command only IOPS? |
||
|
|
||
| /* For managed storage */ | ||
| private boolean managed; | ||
|
|
@@ -70,7 +72,6 @@ | |
| public ResizeVolumeCommand(String path, StorageFilerTO pool, Long currentSize, Long newSize, boolean shrinkOk, String vmInstance, | ||
| boolean isManaged, String iScsiName) { | ||
| this(path, pool, currentSize, newSize, shrinkOk, vmInstance); | ||
|
|
||
| this.iScsiName = iScsiName; | ||
| this.managed = isManaged; | ||
| } | ||
|
|
@@ -120,4 +121,20 @@ | |
| public boolean executeInSequence() { | ||
| return false; | ||
| } | ||
|
|
||
| public Long getNewMaxIops() { | ||
| return newMaxIops; | ||
| } | ||
|
|
||
| public void setNewMaxIops(Long newMaxIops) { | ||
| this.newMaxIops = newMaxIops; | ||
| } | ||
|
|
||
| public Long getNewMinIops() { | ||
| return newMinIops; | ||
| } | ||
|
|
||
| public void setNewMinIops(Long newMinIops) { | ||
| this.newMinIops = newMinIops; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -321,7 +321,7 @@ | |
| throw new CloudRuntimeException(String.format("Failed to find a storage pool with enough capacity to move the volume [%s] to.", volumeToString)); | ||
| } | ||
|
|
||
| Volume newVol = migrateVolume(volumeInfo, destPool); | ||
| Volume newVol = migrateVolume(volumeInfo, destPool, diskOffering); | ||
|
Check warning on line 324 in engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java
|
||
| return volFactory.getVolume(newVol.getId()); | ||
| } | ||
|
|
||
|
|
@@ -1359,13 +1359,14 @@ | |
|
|
||
| @Override | ||
| @DB | ||
| public Volume migrateVolume(Volume volume, StoragePool destPool) throws StorageUnavailableException { | ||
| public Volume migrateVolume(Volume volume, StoragePool destPool, DiskOffering newDiskOffering) throws StorageUnavailableException { | ||
|
Check warning on line 1362 in engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @SadiJr Is this new offering updated on the volume record, when the migrate is successful.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sureshanaparti, I did not get what you mean by this, we set the payload with this disk offering, for setting the IOPS later. |
||
| String volumeToString = getVolumeIdentificationInfos(volume); | ||
|
|
||
| VolumeInfo vol = volFactory.getVolume(volume.getId()); | ||
| if (vol == null){ | ||
| throw new CloudRuntimeException(String.format("Volume migration failed because volume [%s] is null.", volumeToString)); | ||
| } | ||
| vol.addPayload(newDiskOffering); | ||
|
Check warning on line 1369 in engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java
|
||
| if (destPool == null) { | ||
| throw new CloudRuntimeException("Volume migration failed because the destination storage pool is not available."); | ||
| } | ||
|
|
@@ -1494,7 +1495,7 @@ | |
| } | ||
| logger.debug("Offline VM migration was not done up the stack in VirtualMachineManager. Trying to migrate the VM here."); | ||
| for (Map.Entry<Volume, StoragePool> entry : volumeStoragePoolMap.entrySet()) { | ||
| Volume result = migrateVolume(entry.getKey(), entry.getValue()); | ||
| Volume result = migrateVolume(entry.getKey(), entry.getValue(), null); | ||
|
Check warning on line 1498 in engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java
|
||
| if (result == null) { | ||
| return false; | ||
| } | ||
|
|
@@ -1955,7 +1956,7 @@ | |
| } else if (task.type == VolumeTaskType.MIGRATE) { | ||
| store = (PrimaryDataStore) dataStoreMgr.getDataStore(task.pool.getId(), DataStoreRole.Primary); | ||
| updateVolumeSize(store, task.volume); | ||
| vol = migrateVolume(task.volume, store); | ||
| vol = migrateVolume(task.volume, store, null); | ||
|
Check warning on line 1959 in engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java
|
||
| } else if (task.type == VolumeTaskType.RECREATE) { | ||
| Pair<VolumeVO, DataStore> result = recreateVolume(task.volume, vm, dest); | ||
| store = (PrimaryDataStore) dataStoreMgr.getDataStore(result.second().getId(), DataStoreRole.Primary); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,6 +45,7 @@ | |
| import java.util.UUID; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| import com.vmware.vim25.StorageIOAllocationInfo; | ||
| import com.cloud.agent.api.CleanupVMCommand; | ||
| import javax.naming.ConfigurationException; | ||
| import javax.xml.datatype.XMLGregorianCalendar; | ||
|
|
@@ -78,6 +79,7 @@ | |
| import org.apache.commons.lang.ArrayUtils; | ||
| import org.apache.commons.lang.math.NumberUtils; | ||
| import org.apache.logging.log4j.ThreadContext; | ||
| import org.apache.commons.lang3.ObjectUtils; | ||
| import org.joda.time.Duration; | ||
|
|
||
| import com.cloud.agent.IAgentControl; | ||
|
|
@@ -873,6 +875,8 @@ | |
| boolean managed = cmd.isManaged(); | ||
| String poolUUID = cmd.getPoolUuid(); | ||
| String chainInfo = cmd.getChainInfo(); | ||
| Long newMinIops = cmd.getNewMinIops(); | ||
| Long newMaxIops = cmd.getNewMaxIops(); | ||
|
Check warning on line 879 in plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
|
||
| boolean useWorkerVm = false; | ||
|
|
||
| VmwareContext context = getServiceContext(); | ||
|
|
@@ -887,8 +891,6 @@ | |
| oldSize / (float) ResourceType.bytesToMiB, newSize / (float) ResourceType.bytesToMiB, vmName); | ||
| logger.error(errorMsg); | ||
| throw new Exception(errorMsg); | ||
| } else if (newSize == oldSize) { | ||
| return new ResizeVolumeAnswer(cmd, true, "success", newSize * ResourceType.bytesToKiB); | ||
| } | ||
|
|
||
| if (vmName.equalsIgnoreCase("none")) { | ||
|
|
@@ -982,6 +984,8 @@ | |
| VirtualDisk disk = getDiskAfterResizeDiskValidations(vmMo, path); | ||
| String vmdkAbsFile = VmwareHelper.getAbsoluteVmdkFile(disk); | ||
|
|
||
| setDiskIops(disk, newMinIops, newMaxIops); | ||
|
Check warning on line 987 in plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
|
||
|
|
||
| if (vmdkAbsFile != null && !vmdkAbsFile.isEmpty()) { | ||
| vmMo.updateAdapterTypeIfRequired(vmdkAbsFile); | ||
| } | ||
|
|
@@ -1033,6 +1037,22 @@ | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Sets the disk IOPS which is the sum of min IOPS and max IOPS; if they are null, the IOPS limit is set to -1 (unlimited). | ||
| */ | ||
| private void setDiskIops(VirtualDisk disk, Long newMinIops, Long newMaxIops) { | ||
| StorageIOAllocationInfo storageIOAllocation = new StorageIOAllocationInfo(); | ||
| Long iops = -1L; | ||
|
Check warning on line 1045 in plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
|
||
|
|
||
| if (ObjectUtils.allNotNull(newMinIops, newMaxIops) && newMinIops > 0 && newMaxIops > 0) { | ||
| iops = newMinIops + newMaxIops; | ||
|
Check warning on line 1048 in plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
|
||
| } | ||
|
|
||
| storageIOAllocation.setLimit(iops); | ||
|
Check warning on line 1051 in plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
|
||
| logger.debug(LogUtils.logGsonWithoutException("Setting [%s] as the IOPS limit of disk [%s].", iops == -1L ? "unlimited" : iops, disk)); | ||
| disk.setStorageIOAllocation(storageIOAllocation); | ||
| } | ||
|
Check warning on line 1054 in plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
|
||
|
|
||
| private VirtualDisk getDiskAfterResizeDiskValidations(VirtualMachineMO vmMo, String volumePath) throws Exception { | ||
| Pair<VirtualDisk, String> vdisk = vmMo.getDiskDevice(volumePath); | ||
| if (vdisk == null) { | ||
|
|
@@ -4425,7 +4445,7 @@ | |
| for (String diskPath : disks) { | ||
| DatastoreFile file = new DatastoreFile(diskPath); | ||
| VirtualMachineMO vmMo = dcMo.findVm(file.getDir()); | ||
| Pair<VirtualDisk, String> vds = vmMo.getDiskDevice(file.getFileName(), true); | ||
| Pair<VirtualDisk, String> vds = vmMo.getDiskDevice(file.getFileName(), true, false); | ||
|
Check warning on line 4448 in plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
|
||
| long virtualsize = vds.first().getCapacityInKB() * 1024; | ||
| long physicalsize = primaryStorageDatastoreMo.fileDiskSize(file.getPath()); | ||
| if (statEntry.containsKey(chainInfo)) { | ||
|
|
@@ -5178,6 +5198,8 @@ | |
| volumePath = vmMo.getVmdkFileBaseName(disk); | ||
| } | ||
| } | ||
|
|
||
| setDiskIops(cmd, vmMo, volumePath); | ||
|
Check warning on line 5202 in plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
|
||
| VirtualMachineDiskInfoBuilder diskInfoBuilder = vmMo.getDiskInfoBuilder(); | ||
| chainInfo = _gson.toJson(diskInfoBuilder.getDiskInfoByBackingFileBaseName(volumePath, targetDsMo.getName())); | ||
| MigrateVolumeAnswer answer = new MigrateVolumeAnswer(cmd, true, null, volumePath); | ||
|
|
@@ -5190,6 +5212,33 @@ | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Sets the disk IOPS limitation, if the {@link MigrateVolumeCommand} did not specify this limitation, then it is set to -1 (unlimited). | ||
| */ | ||
| private void setDiskIops(MigrateVolumeCommand cmd, VirtualMachineMO vmMo, String volumePath) throws Exception { | ||
|
Check warning on line 5218 in plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
|
||
|
Comment on lines
+5216
to
+5218
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we sure that every time the volume is migrated we inform the IOPS limitation? |
||
| Long newIops = cmd.getNewIops() == null ? -1L : cmd.getNewIops(); | ||
| VirtualDisk disk = vmMo.getDiskDevice(volumePath, true, true).first(); | ||
|
Check warning on line 5220 in plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
|
||
|
|
||
| try { | ||
| logger.debug(LogUtils.logGsonWithoutException("Trying to change disk [%s] IOPS to [%s].", disk, newIops)); | ||
| VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec(); | ||
| VirtualDeviceConfigSpec deviceConfigSpec = new VirtualDeviceConfigSpec(); | ||
|
Check warning on line 5225 in plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
|
||
|
|
||
| StorageIOAllocationInfo storageIOAllocation = new StorageIOAllocationInfo(); | ||
| storageIOAllocation.setLimit(newIops); | ||
| disk.setStorageIOAllocation(storageIOAllocation); | ||
|
Check warning on line 5229 in plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
|
||
|
|
||
| deviceConfigSpec.setDevice(disk); | ||
| deviceConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.EDIT); | ||
| vmConfigSpec.getDeviceChange().add(deviceConfigSpec); | ||
| vmMo.configureVm(vmConfigSpec); | ||
| } catch (Exception e) { | ||
| String vmwareDocumentation = "https://kb.vmware.com/s/article/68164"; | ||
| logger.error(LogUtils.logGsonWithoutException("Failed to change disk [%s] IOPS to [%s] due to [%s]. This happens when the disk controller is IDE." + | ||
| " Please read this documentation for more information: [%s]. ", disk, newIops, e.getMessage(), vmwareDocumentation), e); | ||
| } | ||
| } | ||
|
Check warning on line 5240 in plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
|
||
|
|
||
| private Pair<VirtualDisk, String> getVirtualDiskInfo(VirtualMachineMO vmMo, String srcDiskName) throws Exception { | ||
| Pair<VirtualDisk, String> deviceInfo = vmMo.getDiskDevice(srcDiskName); | ||
| if (deviceInfo == null) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1253,8 +1253,8 @@ | |
|
|
||
| validateIops(newMinIops, newMaxIops, volume.getPoolType()); | ||
| } else { | ||
| newMinIops = newDiskOffering.getMinIops(); | ||
| newMaxIops = newDiskOffering.getMaxIops(); | ||
| newMinIops = newDiskOffering.getMinIops() != null ? newDiskOffering.getMinIops() : newDiskOffering.getIopsReadRate(); | ||
| newMaxIops = newDiskOffering.getMaxIops() != null ? newDiskOffering.getMaxIops() : newDiskOffering.getIopsWriteRate(); | ||
JoaoJandre marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+1256
to
+1257
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should extract this to a method and add a javadoc to it explaining why we are doing it like this. |
||
| } | ||
|
|
||
| // if the hypervisor snapshot reserve value is null, it must remain null (currently only KVM uses null and null is all KVM uses for a value here) | ||
|
|
@@ -1322,10 +1322,8 @@ | |
| volumeMigrateRequired = true; | ||
| } | ||
|
|
||
| boolean volumeResizeRequired = false; | ||
| if (currentSize != newSize || !compareEqualsIncludingNullOrZero(newMaxIops, volume.getMaxIops()) || !compareEqualsIncludingNullOrZero(newMinIops, volume.getMinIops())) { | ||
| volumeResizeRequired = true; | ||
| } | ||
| boolean volumeResizeRequired = currentSize != newSize || !compareEqualsIncludingNullOrZero(newMaxIops, volume.getMaxIops()) || !compareEqualsIncludingNullOrZero(newMinIops, volume.getMinIops()) | ||
| || !compareEqualsIncludingNullOrZero(newMaxIops, diskOffering.getIopsWriteRate()) || !compareEqualsIncludingNullOrZero(newMinIops, diskOffering.getIopsReadRate()); | ||
|
Comment on lines
+1325
to
+1326
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really dislike assuming that newMaxIops has to equal write and newMinIops has to equal read. We should change this logic to avoid future problems, it is very easy for some other dev to mix these in the future. |
||
| if (!volumeMigrateRequired && !volumeResizeRequired && newDiskOffering != null) { | ||
| _volsDao.updateDiskOffering(volume.getId(), newDiskOffering.getId()); | ||
| volume = _volsDao.findById(volume.getId()); | ||
|
|
@@ -1390,7 +1388,11 @@ | |
| } else if (jobResult instanceof Throwable) { | ||
| throw new RuntimeException("Unexpected exception", (Throwable) jobResult); | ||
| } else if (jobResult instanceof Long) { | ||
| return _volsDao.findById((Long) jobResult); | ||
| Long volumeId = (Long) jobResult; | ||
| if (newDiskOffering != null) { | ||
| _volsDao.updateDiskOffering(volumeId, newDiskOffering.getId()); | ||
| } | ||
| return _volsDao.findById(volumeId); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -3730,9 +3732,9 @@ | |
| Volume newVol = null; | ||
| try { | ||
| if (liveMigrateVolume) { | ||
| newVol = liveMigrateVolume(volume, destPool); | ||
| newVol = liveMigrateVolume(volume, destPool, newDiskOffering); | ||
| } else { | ||
| newVol = _volumeMgr.migrateVolume(volume, destPool); | ||
| newVol = _volumeMgr.migrateVolume(volume, destPool, newDiskOffering); | ||
| } | ||
| if (newDiskOffering != null) { | ||
| _volsDao.updateDiskOffering(newVol.getId(), newDiskOffering.getId()); | ||
|
|
@@ -3748,9 +3750,9 @@ | |
| } | ||
|
|
||
| @DB | ||
| protected Volume liveMigrateVolume(Volume volume, StoragePool destPool) throws StorageUnavailableException { | ||
| protected Volume liveMigrateVolume(Volume volume, StoragePool destPool, DiskOfferingVO newDiskOffering) throws StorageUnavailableException { | ||
| VolumeInfo vol = volFactory.getVolume(volume.getId()); | ||
|
|
||
| vol.addPayload(newDiskOffering); | ||
| DataStore dataStoreTarget = dataStoreMgr.getDataStore(destPool.getId(), DataStoreRole.Primary); | ||
| AsyncCallFuture<VolumeApiResult> future = volService.migrateVolume(vol, dataStoreTarget); | ||
| try { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why newIops is added , not newIopsRead/newIopsWrite ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VMware hypervisor does not allow specifying the IOPS for read and write operations. Instead, you can only specify the IOPS for the disk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @SadiJr
MigrateVolumeCommand is a class in core module. It might be used if there are similar issues with kvm and/or xenserver which support IOPS read/write.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hsato03 Do you think that it makes sense to separate it into read and write IOPS? As @weizhouapache said, this is an agnostic command, so it should be treated like that (meaning separate read and write IOPS).