[VMware] apply IOPS in resize/migrate #7226
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7226 +/- ##
============================================
+ Coverage 16.57% 16.60% +0.02%
- Complexity 13870 13924 +54
============================================
Files 5719 5730 +11
Lines 507200 508166 +966
Branches 61574 61783 +209
============================================
+ Hits 84093 84384 +291
- Misses 413688 414345 +657
- Partials 9419 9437 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| String attachedVmName; | ||
| Volume.Type volumeType; | ||
| String hostGuidInTargetCluster; | ||
| Long newIops; |
There was a problem hiding this comment.
Can you explain why newIops is added , not newIopsRead/newIopsWrite ?
There was a problem hiding this comment.
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.
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.
@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).
...ns/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
Outdated
Show resolved
Hide resolved
|
SonarCloud Quality Gate failed. |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
...ns/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
Outdated
Show resolved
Hide resolved
|
@SadiJr can you please check the review comments |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7542 |
|
@SadiJr can you answer any questions/address any comments, please? |
|
ping @SadiJr |
|
@DaanHoogland Sorry for the delay, I will review the comments and work on this PR. |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
@blueorangutan package |
|
@hsato03 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 13640 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-13484)
|
|
@hsato03 @BryanMLima , apart from the test_network failure, what is the status of this PR? |
|
@DaanHoogland The PR is ready to be reviewed and tested. Also, I have reproduced the tests from the description and it's working as expected. Regarding the test failure, it seems to be unrelated to the PR. FAIL: test_01_deployVMInSharedNetwork (tests.smoke.test_network.TestSharedNetworkWithConfigDrive)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/marvin/tests/smoke/test_network.py", line 2395, in test_01_deployVMInSharedNetwork
self._umount_config_drive(ssh, mount_path)
File "/marvin/tests/smoke/test_network.py", line 2319, in _umount_config_drive
"but contains: %s" % result)
AssertionError: False is not true : After umount directory should be empty but contains: ['sudo: unable to resolve host VM-a1746bba-97eb-47b9-9d6a-70ac9c03262b: Temporary failure in name resolution'] |
|
@blueorangutan package |
|
@hsato03 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 13772 |
|
@blueorangutan test ol8 vmware-70u3 |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + vmware-70u3) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-13524)
|
|
@DaanHoogland I think the test error is not related to the PR. Could you run the tests again, please? ERROR: test_01_prepare_and_cancel_maintenance (tests.smoke.test_ms_maintenance_and_safe_shutdown.TestMSMaintenanceAndSafeShutdown)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/marvin/tests/smoke/test_ms_maintenance_and_safe_shutdown.py", line 138, in test_01_prepare_and_cancel_maintenance
self.apiclient.cancelMaintenance(cancel_maintenance_cmd)
File "/usr/local/lib/python3.6/site-packages/marvin/cloudstackAPI/cloudstackAPIClient.py", line 1936, in cancelMaintenance
response = self.connection.marvinRequest(command, response_type=response, method=method)
File "/usr/local/lib/python3.6/site-packages/marvin/cloudstackConnection.py", line 381, in marvinRequest
raise e
File "/usr/local/lib/python3.6/site-packages/marvin/cloudstackConnection.py", line 376, in marvinRequest
raise self.__lastError
File "/usr/local/lib/python3.6/site-packages/marvin/cloudstackConnection.py", line 310, in __parseAndGetResponse
response_cls)
File "/usr/local/lib/python3.6/site-packages/marvin/jsonHelper.py", line 155, in getResultObj
raise cloudstackException.CloudstackAPIException(respname, errMsg)
marvin.cloudstackException.CloudstackAPIException: Execute cmd: cancelmaintenance failed, due to: errorCode: 530, errorText:Management server is not in the right state to cancel maintenance |
|
@blueorangutan test ol8 vmware-70u3 |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + vmware-70u3) has been kicked to run smoke tests |
I am running it again, but the root cause may well be in the test itself, or the order in which tests are executed. I agree it does not look like related to your changes. |
|
[SF] Trillian test result (tid-13714)
|
| String attachedVmName; | ||
| Volume.Type volumeType; | ||
| String hostGuidInTargetCluster; | ||
| Long newIops; |
There was a problem hiding this 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).
| private Long newMaxIops; | ||
| private Long newMinIops; |
There was a problem hiding this comment.
Why do we have max and min IOPs here but on the migrate volume command only IOPS?
| * 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 { |
There was a problem hiding this comment.
Are we sure that every time the volume is migrated we inform the IOPS limitation?
| newMinIops = newDiskOffering.getMinIops() != null ? newDiskOffering.getMinIops() : newDiskOffering.getIopsReadRate(); | ||
| newMaxIops = newDiskOffering.getMaxIops() != null ? newDiskOffering.getMaxIops() : newDiskOffering.getIopsWriteRate(); |
There was a problem hiding this comment.
We should extract this to a method and add a javadoc to it explaining why we are doing it like this.
| boolean volumeResizeRequired = currentSize != newSize || !compareEqualsIncludingNullOrZero(newMaxIops, volume.getMaxIops()) || !compareEqualsIncludingNullOrZero(newMinIops, volume.getMinIops()) | ||
| || !compareEqualsIncludingNullOrZero(newMaxIops, diskOffering.getIopsWriteRate()) || !compareEqualsIncludingNullOrZero(newMinIops, diskOffering.getIopsReadRate()); |
There was a problem hiding this 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.
| 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 + "."; | ||
| } |
There was a problem hiding this comment.
could you explain this change?
|
@JoaoJandre @hsato03 are you guys still looking at this? |
Description
Using the VMware hypervisor, when migrating/resizing one volume, with or without IOPS limitation, and changing the disk offering, this volume keeps the configurations of IOPS of the original offering, only applying the new configurations when detaching and attaching the volume. This PR aims to fix this behavior, to apply the new IOPS configuration when migration/resizing a volume changing the disk offering.
Types of changes
Feature/Enhancement Scale or Bug Severity
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
It was tested in a local lab: