CLOUDSTACK-9199: Fixed deployVirtualMachine API does not throw an error when cpunumber is specified for static compute offering

[anshul1886]

https://issues.apache.org/jira/browse/CLOUDSTACK-9199

To test:

Deploy VM by providing cpuNumber, cpuSpeed or memory for static/ non dynamic service offering
Deployment should fail.

API example:

http:https://10.220.135.6/client/api?command=deployVirtualMachine&name=olotwo&response=&zoneid=ab6e4154-62a3-42a8-9627-3cbdc66bcbb6&templateid=3aaaace6-91b4-11e5-b6fc-e26c2aa1d1d0&hypervisor=XenServer&serviceofferingid=39643075-4b45-489d-afac-88f09d536bdd&details[0].cpuNumber=1&details[0].cpuSpeed=1000&details[0].memory=1000&securitygroupids=60844698-91b4-11e5-b6fc-e26c2aa1d1d0&_=1448277187743

[pedro-martins]

Nice :)
Can you extract the code to a method with a javadoc explaining the code?
If you can do a testcase for the method too, it will be apreciated :)
Thx :)

[bvbharatk]

ACS CI BVT Run

Sumarry:
Build Number 141
Hypervisor xenserver
NetworkType Advanced
Passed=101
Failed=5
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_loadbalance.TestLoadBalance
  • test_01_create_lb_rule_src_nat Failed
  • test_02_create_lb_rule_non_nat Failed
  • test_assign_and_removal_lb Failed
• integration.smoke.test_volumes.TestCreateVolume
  • test_01_create_volume Failed
  • test_06_download_detached_volume 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_routers.TestRouterServices
integration.smoke.test_reset_vm_on_reboot.TestResetVmOnReboot
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

[koushik-das]

LGTM. Verified on a simulator setup. Also the test failures from CI run are unrelated to this change.

[swill]

Looking for one more LGTM for this one... Thx...

[rohityadavcloud]

@anshul1886 please rebase against latest master, can you explain if this can cause backward compatiblity issue

tag:easypr

[swill]

@anshul1886 can you answer @rhtyd's question so we can get his code review and get this moving forward. Thanks...

[anshul1886]

@rhtyd @swill There will not be backward compatibility issues as with static offering those parameters were not taken into consideration. They were wrongly giving the impression that they are being used.

[alexandrelimassantana]

as @pedro-martins stated, it seems to be fitting that this method is extracted to a class to be documented/tested. The code looks good but if the exception is to be launched I see no room for backward compatibility. If that's an issue, this needs to be rethinked as the raised exception would have to be treated and at that point we would have to think if raising the exception was really necessary. Overall the code looks good, simple and justified.

[swill]

@anshul1886 can you please review and address the comments in this PR? Thanks...

[anshul1886]

I don't find much value here in adding documentation as code seems to be self explanatory. Regrading backward compatibility I have already answered in my previous comment.

[swill]

@anshul1886 I think the ask is that it get extracted as a method and a test be written for it. @pedro-martins and @alexandrelimassantana, I feel that this might be a bit overkill. Can you explain what the test would be validating?

As for the backwards compatibility issue, I am not entirely sure I understand the concern. Can someone help me understand what that concern is? Thx...

[rohityadavcloud]

LGTM (just code review), based on what @anshul1886 says there should not be backward compatibility issue though I've not verified this by performing manual tests

[swill]

I think we are ok on this one. thanks @rhtyd. I will merge this one...