JoaoJandre commented on code in PR #7226:
URL: https://github.com/apache/cloudstack/pull/7226#discussion_r2195766027
##########
plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java:
##########
@@ -5190,6 +5212,33 @@ private Answer execute(MigrateVolumeCommand cmd) {
}
}
+ /**
+ * 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 {
Review Comment:
Are we sure that every time the volume is migrated we inform the IOPS
limitation?
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -1322,10 +1322,8 @@ public VolumeVO resizeVolume(ResizeVolumeCmd cmd) throws
ResourceAllocationExcep
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());
Review Comment:
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.
##########
core/src/main/java/com/cloud/agent/api/storage/MigrateVolumeCommand.java:
##########
@@ -36,6 +36,7 @@ public class MigrateVolumeCommand extends Command {
String attachedVmName;
Volume.Type volumeType;
String hostGuidInTargetCluster;
+ Long newIops;
Review Comment:
@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).
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -1253,8 +1253,8 @@ public VolumeVO resizeVolume(ResizeVolumeCmd cmd) throws
ResourceAllocationExcep
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();
Review Comment:
We should extract this to a method and add a javadoc to it explaining why we
are doing it like this.
##########
core/src/main/java/com/cloud/agent/api/storage/ResizeVolumeCommand.java:
##########
@@ -33,6 +33,8 @@ public class ResizeVolumeCommand extends Command {
private boolean shrinkOk;
private String vmInstance;
private String chainInfo;
+ private Long newMaxIops;
+ private Long newMinIops;
Review Comment:
Why do we have max and min IOPs here but on the migrate volume command only
IOPS?
##########
vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java:
##########
@@ -2785,9 +2785,13 @@ public Pair<VirtualDisk, String> getDiskDevice(String
vmdkDatastorePath) throws
}
// return pair of VirtualDisk and disk device bus name(ide0:0, etc)
- public Pair<VirtualDisk, String> getDiskDevice(String vmdkDatastorePath,
boolean matchExactly) throws Exception {
+ public Pair<VirtualDisk, String> getDiskDevice(String vmdkDatastorePath,
boolean matchExactly, boolean ignoreDotOnPath) throws Exception {
List<VirtualDevice> devices =
_context.getVimClient().getDynamicProperty(_mor, "config.hardware.device");
+ if (ignoreDotOnPath) {
+ vmdkDatastorePath = vmdkDatastorePath + ".";
+ }
Review Comment:
could you explain this change?
--
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-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
