You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by nvazquez <gi...@git.apache.org> on 2016/01/05 19:53:15 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-9211: Support passing vRAM siz...
GitHub user nvazquez opened a pull request:
https://github.com/apache/cloudstack/pull/1310
CLOUDSTACK-9211: Support passing vRAM size to support 3D GPU on Vmware
JIRA TICKET:
https://issues.apache.org/jira/browse/CLOUDSTACK-9211
CS support passing hypervisor options by creating entries in <code>vm_template_details</code> or <code>user_vm_details</code>
To enable software 3D GPU 4 options needs to be added:
| name| value |
|:-------------:|:-------------:|
|mks.enable3d|true|
|mks.use3dRenderer|automatic|
|svga.autodetect|false|
|svga.vramSize|(size in KB) e.g. 131072|
Currently all options except <code>svga.vramSize</code> works, VMs always get configured with default 64Mb videoRAM instead of the one provided on the option.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nvazquez/cloudstack 3dgpu
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1310.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1310
----
commit 656ae109377e7d4400d16f05cbb4096e20a8c05b
Author: nvazquez <ni...@gmail.com>
Date: 2016-01-05T18:45:01Z
CLOUDSTACK-9211: Support passing vRAM size to support 3D GPU on Vmware
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9211: Support passing vRAM siz...
Posted by cristofolini <gi...@git.apache.org>.
Github user cristofolini commented on the pull request:
https://github.com/apache/cloudstack/pull/1310#issuecomment-171798749
Thanks for taking the time to add the descriptions and refactoring, @nvazquez. It has helped readability quite a bit. :)
Now, in case you're interested in improving this code *even further*, may I suggest breaking down the `postVideoCardMemoryConfigBeforeStart` method a bit more? I like what you did separating it into two, but there are still quite a few conditionals within each other and that makes the whole thing look convoluted, not to mention adding up to cyclomatic complexity. Bonus points for adding javadoc style descriptions to the new methods you create like you did to the existing ones.
My last suggestion would be to write some test cases (unit tests) for the methods you've created. After that, as far as I can assess, this PR will be perfect. :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9211: Support passing vRAM siz...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1310#issuecomment-187969486
Merged based on integration tests that were executed and code reviews; hope I did not do any mistake (my first merge for ACS).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9211: Support passing vRAM siz...
Posted by cristofolini <gi...@git.apache.org>.
Github user cristofolini commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1310#discussion_r49279282
--- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
@@ -1895,6 +1896,9 @@ protected StartAnswer execute(StartCommand cmd) {
postDiskConfigBeforeStart(vmMo, vmSpec, sortedDisks, ideControllerKey, scsiControllerKey, iqnToPath, hyperHost, context);
+ //Sets video card memory to the one provided in detail svga.vramSize (if provided), 64MB was always set before
--- End diff --
I think a comment like this would be better located in a Javadoc style comment above the `postVideoCardMemoryConfigBeforeStart` method itself, along with brief descriptions of the parameters involved. This could make it easier to read/understand going forward. :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9211: Support passing vRAM siz...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1310
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9211: Support passing vRAM siz...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1310#issuecomment-175689785
LGTM, can we have regression test results against VMware then we would be able to merge this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9211: Support passing vRAM siz...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1310#issuecomment-176351414
Hi @bhaisaab ,
Which test files would you like me to run? Is this ok with this command on each file?
<code>nosetests --with-marvin --marvin-config=setup/dev/advanced.cfg test/integration/...file.py</code>
Or is it a command to run multiple files?
Thanks
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9211: Support passing vRAM siz...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1310#issuecomment-173439752
Thanks for your help @cristofolini!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9211: Support passing vRAM siz...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1310#issuecomment-180363651
Hi @bhaisaab can you help me testing this?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9211: Support passing vRAM siz...
Posted by bvbharatk <gi...@git.apache.org>.
Github user bvbharatk commented on the pull request:
https://github.com/apache/cloudstack/pull/1310#issuecomment-201959648
### ACS CI BVT Run
**Sumarry:**
Build Number 133
Hypervisor xenserver
NetworkType Advanced
Passed=104
Failed=1
Skipped=4
_Link to logs Folder (search by build_no):_ https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0
**Failed tests:**
* integration.smoke.test_reset_vm_on_reboot.TestResetVmOnReboot
* test_01_reset_vm_on_reboot Failed
**Skipped tests:**
test_vm_nic_adapter_vmxnet3
test_deploy_vgpu_enabled_vm
test_06_copy_template
test_06_copy_iso
**Passed test suits:**
integration.smoke.test_deploy_vm_with_userdata.TestDeployVmWithUserData
integration.smoke.test_affinity_groups_projects.TestDeployVmWithAffinityGroup
integration.smoke.test_portable_publicip.TestPortablePublicIPAcquire
integration.smoke.test_over_provisioning.TestUpdateOverProvision
integration.smoke.test_global_settings.TestUpdateConfigWithScope
integration.smoke.test_guest_vlan_range.TestDedicateGuestVlanRange
integration.smoke.test_scale_vm.TestScaleVm
integration.smoke.test_service_offerings.TestCreateServiceOffering
integration.smoke.test_loadbalance.TestLoadBalance
integration.smoke.test_routers.TestRouterServices
integration.smoke.test_snapshots.TestSnapshotRootDisk
integration.smoke.test_deploy_vms_with_varied_deploymentplanners.TestDeployVmWithVariedPlanners
integration.smoke.test_network.TestDeleteAccount
integration.smoke.test_non_contigiousvlan.TestUpdatePhysicalNetwork
integration.smoke.test_deploy_vm_iso.TestDeployVMFromISO
integration.smoke.test_public_ip_range.TestDedicatePublicIPRange
integration.smoke.test_multipleips_per_nic.TestDeployVM
integration.smoke.test_regions.TestRegions
integration.smoke.test_affinity_groups.TestDeployVmWithAffinityGroup
integration.smoke.test_network_acl.TestNetworkACL
integration.smoke.test_pvlan.TestPVLAN
integration.smoke.test_ssvm.TestSSVMs
integration.smoke.test_nic.TestNic
integration.smoke.test_deploy_vm_root_resize.TestDeployVM
integration.smoke.test_resource_detail.TestResourceDetail
integration.smoke.test_secondary_storage.TestSecStorageServices
integration.smoke.test_vm_life_cycle.TestDeployVM
integration.smoke.test_disk_offerings.TestCreateDiskOffering
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9211: Support passing vRAM siz...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1310#issuecomment-180368839
@nvazquez Yes, please run all the smoke tests against real hardware, the tests are in test/integration/smoke. You can run them one by one or multiple. The idea is to ensure we're not breaking anything with the change. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9211: Support passing vRAM siz...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1310#issuecomment-188129352
Thanks @nvazquez @rafaelweingartner - I could not reply in time, but LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9211: Support passing vRAM siz...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1310#issuecomment-186444960
Hi @bhaisaab sorry I couldn't post this earlier but I've been working on another pull request and couldn't be able to test this one before.
As my chages affects vm deployments, I only ran <code>test_deploy_vms_with_varied_deploymentplanners.py</code>, <code>test_reset_vm_on_reboot.py</code> and <code>test_vm_life_cycle.py</code> but I can run all the test if you want
```
# nosetests --with-marvin --marvin-config=setup/dev/advanced.cfg.new test/integration/smoke/test_deploy_vms_with_varied_deploymentplanners.py test/integration/smoke/test_reset_vm_on_reboot.py
==== Marvin Init Started ====
=== Marvin Parse Config Successful ===
=== Marvin Setting TestData Successful===
==== Log Folder Path: /tmp//MarvinLogs//Feb_19_2016_14_55_40_UF5PE3. All logs will be available here ====
=== Marvin Init Logging Successful===
==== Marvin Init Successful ====
===final results are now copied to: /tmp//MarvinLogs/test_reset_vm_on_reboot_S6H4PW===
```
```
# cat /tmp//MarvinLogs/test_reset_vm_on_reboot_S6H4PW/results.txt
Test to deploy vm with a first fit offering ... === TestName: test_deployvm_firstfit | Status : SUCCESS ===
ok
Test deploy VMs using user concentrated planner ... === TestName: test_deployvm_userconcentrated | Status : SUCCESS ===
ok
Test deploy VMs using user dispersion planner ... === TestName: test_deployvm_userdispersing | Status : SUCCESS ===
ok
Test reset virtual machine on reboot ... === TestName: test_01_reset_vm_on_reboot | Status : SUCCESS ===
ok
----------------------------------------------------------------------
Ran 4 tests in 571.499s
OK
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9211: Support passing vRAM siz...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1310#issuecomment-171752709
Hi @cristofolini
Thanks for your comments. I refactored the method to make it more readable into two methods, and added description to both, like javadoc.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9211: Support passing vRAM siz...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1310#issuecomment-173350754
Thanks @cristofolini
I've tryied writing a marvin test for testing this feature which would be:
* Select (or create) a vm (id=XXXX) which is already running and stop it.
* For vm with id=XXXX do the following:
<code>INSERT INTO cloud.user_vm_details (vm_id, name, value) VALUES ('XXXX', 'mks.enable3d', 'true');</code>
<code>INSERT INTO cloud.user_vm_details (vm_id, name, value) VALUES ('XXXX', 'mks.use3dRenderer', 'automatic');</code>
<code>INSERT INTO cloud.user_vm_details (vm_id, name, value) VALUES ('XXXX', 'svga.autodetect', 'false');</code>
<code>INSERT INTO cloud.user_vm_details (vm_id, name, value) VALUES ('XXXX', 'svga.vramSize', 'ZZZZ');</code>
</code>
Where <code>ZZZZ</code> is the ram size in KB
* Start vm XXXX again
In previous behaviour, it all worked except vram size, which was always set to 64MB, ignoring size <code>ZZZZ</code> provided. With this fix, vram size is set to <code>ZZZZ</code> when provided.
I couldn't be able to write a marvin test which follows these steps, however I wrote a unit test for this new method, which is only accessed through <code>execute(StartCommand)</code> method.
Nicolas
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9211: Support passing vRAM siz...
Posted by cristofolini <gi...@git.apache.org>.
Github user cristofolini commented on the pull request:
https://github.com/apache/cloudstack/pull/1310#issuecomment-173364669
Thanks again, @nvazquez. The unit test you wrote is a nice improvement and I'm fine with it. :)
Code LGTM.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---