CLOUDSTACK-9428: Fix for CLOUDSTACK-9211 - Improve performance of 3D GPU support in cloud-plugin-hypervisor-vmware #1605
Conversation
|
In general, a good idea. I've not tested it though. @nvazquez can you re-kick Jenkins by pushing -f on the PR |
| */ | ||
| protected void postVideoCardMemoryConfigBeforeStart(VirtualMachineMO vmMo, VirtualMachineTO vmSpec) { | ||
| protected void videoCardMemoryConfig(VirtualMachineMO vmMo, VirtualMachineTO vmSpec, VirtualMachineConfigSpec vmConfigSpec) { | ||
| String paramVRamSize = "svga.vramSize"; |
There was a problem hiding this comment.
👍 Thanks @nvazquez for this PR. It indeed reduces additional API calls.
I'd suggest few minor changes though.
Please move this string constant "svga.vramSize" to VmDetailConstants which is place holder for VM detail constants.
|
Thanks @rhtyd @sateesh-chodapuneedi for your reviews! I pushed new chages based on @sateesh-chodapuneedi comments, now Jenkins passed. |
|
@nvazquez Thanks. |
|
Ping for review -- @sateesh-chodapuneedi, @rhtyd, @koushik-das |
nvazquez
force-pushed
the
fixVram
branch
2 times, most recently
from
e78a69a
to
d21c485
Compare
|
@blueorangutan package |
|
@rhtyd a Trillian-Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✖centos6 ✔centos7 ✔debian repo: http://packages.shapeblue.com/cloudstack/pr/1605 |
|
@rafaelweingartner @swill Will you be able to review this one? |
|
LGTM 👍 |
|
Thanks @sateesh-chodapuneedi! |
| VirtualMachineConfigSpec changeVideoCardSpecs = new VirtualMachineConfigSpec(); | ||
| changeVideoCardSpecs.getDeviceChange().add(arrayVideoCardConfigSpecs); | ||
| return changeVideoCardSpecs; | ||
| vmConfigSpec.getDeviceChange().add(arrayVideoCardConfigSpecs); |
There was a problem hiding this comment.
Is it possible for getDeviceChange to return null?
There was a problem hiding this comment.
I don't think so. Device change done in the structure locally and exception occurs during the VM reconfiguration on the hypervisor if anything in the config is wrong.
|
@nvazquez could you please create Marvin test case to verify the use case in the PR description? |
|
@jburwell The support for passing vGPU parameters is already implemented via PR1310 which has a test case. This PR is only performance optimization by reducing an extra reconfgtask which save about 5 sec during VM startup. |
|
@nvazquez,
@nvazquez great work, my suggestions are mostly aesthetics, but I think they can help improve this PR. |
|
@rafaelweingartner Thanks for your review! I'll work on your suggestions! |
|
Thanks @rafaelweingartner for your review! I've been working on it and pushed changes based on your comments! |
|
@nvazquez, I am sorry, my bad; last time I read the code, I overlooked the method com.cloud.hypervisor.vmware.mo.VirtualMachineMO.getAllDeviceList(). Having said that, how do you feel about extracting the try/catch block to “getAllDeviceList”, then this method would not need to throw a checked exception. It could re-throw a runtime exception such as the CloudRuntime. From my understanding the test method “testStartVm3dgpuEnabled” is used to test the method “configureVideoCard”, right? What about changing that method name to “testConfigureVideoCard”? Moreover, do not you think that the method “configureVideoCard” needs at least two test cases? One that is already written, and the other to test the condition in which the call “vmSpec.getDetails().containsKey(VmDetailConstants.SVGA_VRAM_SIZE)” returns false, and then the method “setNewVRamSizeVmVideoCard” is never called. The same happens for the method “modifyVmVideoCardVRamSize”, I think it has another condition “videoCard.getVideoRamSizeInKB().longValue() == svgaVmramSize” to be tested. And finally, for the test method “testConfigureSpecVideoCardNewVRamSize”, what about using the “Mockito.InOrder” to assure that the methods are being called in the proper order. If someone changes that order of call in the future, that can affect the method behavior. I also have one small comment about the title of the PR and the commit message. “Improve performance” seems a little too vague for a title. The idea is very well explained on the PR, but the title does not reflect much. What about, “improve the performance of cloud-plugin-hypervisor-vmware” or something like that. |
|
LGTM for testing. Vmware ESX 5.5 and 6.0 hypervisors, advanced networking, RHEL 6 management servers [root@ussarlabcsmgt41 ~]# cat /tmp//MarvinLogs/test_volumes_930LZ3/results.txt|grep -v ok Ran 104 tests in 11634.840sOK (SKIP=21) |
nvazquez
changed the title
|
Thanks @rafaelweingartner! I pushed new changes |
|
@nvazquez great. LGTM for the code, giving my reviews. |
|
Done, thanks @rafaelweingartner! I'll start working on adding Marvin tests for this PR as @jburwell suggested. |
|
@jburwell I added marvin test, this are results on VMware env: |
|
@rhtyd @jburwell @swill @koushik-das @rafaelweingartner @wido This PR has enough of everything. Can one of the committers merge it? |
CLOUDSTACK-9428: Fix for CLOUDSTACK-9211 - Improve performance of 3D GPU support in cloud-plugin-hypervisor-vmwareJIRA TICKET: https://issues.apache.org/jira/browse/CLOUDSTACK-9428 ### Introduction On #1310 passing vRAM size to support 3D GPU problem was addressed on VMware. It was found out that it could be improved to increase performance by reducing extra API calls, as we'll describe later ### Improvement On WMware, `VmwareResource` manages execution of `StartCommand.` Before sending power on command to ESXi hypervisor, vm is configured by calling `reconfigVMTask` web method on vSphere's client `VimPortType` web service. It was found out that we were using this method 2 times when passing vRAM size, as it implied creating a new vm config spec only editing video card specs and making an extra call to `reconfigVMTask.` We propose reducing the extra web service call by adjusting vm's config spec. This way video card gets properly configured (when passing vRAM size) in the same configure call, increasing performance. ### Use case (passing vRAM size) * Deploy a new VM, let its id be X * Stop VM * Execute SQL, where X is vm's id and Z is vRAM size (in kB): ```` INSERT INTO cloud.user_vm_details (vm_id, name, value) VALUES (X, 'mks.enable3d', 'true'); INSERT INTO cloud.user_vm_details (vm_id, name, value) VALUES (X, 'mks.use3dRenderer', 'automatic'); INSERT INTO cloud.user_vm_details (vm_id, name, value) VALUES (X, 'svga.autodetect', 'false'); INSERT INTO cloud.user_vm_details (vm_id, name, value) VALUES (X, 'svga.vramSize', Z); ```` * Start VM * pr/1605: CLOUDSTACK-9428: Add marvin test CLOUDSTACK-9428: Fix for CLOUDSTACK-9211 - Improve performance Signed-off-by: Rafael Weingärtner <rafael@apache.org>
|
@serg38 merged based on reviews, and tests (marvin and unit ones). |
|
Thanks @rafaelweingartner! |
JIRA TICKET: https://issues.apache.org/jira/browse/CLOUDSTACK-9428
Introduction
On #1310 passing vRAM size to support 3D GPU problem was addressed on VMware. It was found out that it could be improved to increase performance by reducing extra API calls, as we'll describe later
Improvement
On WMware,
VmwareResourcemanages execution ofStartCommand.Before sending power on command to ESXi hypervisor, vm is configured by callingreconfigVMTaskweb method on vSphere's clientVimPortTypeweb service.It was found out that we were using this method 2 times when passing vRAM size, as it implied creating a new vm config spec only editing video card specs and making an extra call to
reconfigVMTask.We propose reducing the extra web service call by adjusting vm's config spec. This way video card gets properly configured (when passing vRAM size) in the same configure call, increasing performance.
Use case (passing vRAM size)