You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2021/01/29 08:40:21 UTC

[GitHub] [cloudstack] DK101010 opened a new pull request #4630: disable hot add memory and cpu via vm settings

DK101010 opened a new pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630


   ### Description
   
   Currently hot add memory and cpu is always enabled when it supported. In some situation it is necessary to disable this to features for a specific vm.
   
   With this PR User can disable hot add memory and cpu via vm settings in the ui or via api call. the default is still enabled, therefore it should not break existing behavior.
   
   <!-- Fixes: # -->
   
   <!--- ********************************************************************************* -->
   <!--- NOTE: AUTOMATATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
   <!--- PLEASE PUT AN 'X' in only **ONE** box -->
   <!--- ********************************************************************************* -->
   
   ### Types of changes
   
   - [ ] Breaking change (fix or feature that would cause existing functionality to change)
   - [x ] New feature (non-breaking change which adds functionality)
   - [ ] Bug fix (non-breaking change which fixes an issue)
   - [ ] Enhancement (improves an existing feature and functionality)
   - [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
   
   ### Feature/Enhancement Scale or Bug Severity
   
   #### Feature/Enhancement Scale
   
   - [ ] Major
   - [x ] Minor
   
   #### Bug Severity
   
   - [ ] BLOCKER
   - [ ] Critical
   - [ ] Major
   - [ ] Minor
   - [ ] Trivial
   
   
   ### Screenshots (if appropriate):
   
   
   ### How Has This Been Tested?
   Manuelly tested in vmware env.
   
   
   <!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md) document -->
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-769877142


   Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2621


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-769844275


   @blueorangutan package


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-897714565


   Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian. SL-JID 857


-- 
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-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-894742846


   <b>Trillian test result (tid-1528)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 57877 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4630-t1528-kvm-centos7.zip
   Intermittent failure detected: /marvin/tests/smoke/test_diagnostics.py
   Intermittent failure detected: /marvin/tests/smoke/test_iso.py
   Intermittent failure detected: /marvin/tests/smoke/test_outofbandmanagement.py
   Intermittent failure detected: /marvin/tests/smoke/test_volumes.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
   Smoke tests completed. 87 look OK, 2 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_05_ping_in_cpvm_success | `Failure` | 14.35 | test_diagnostics.py
   test_06_download_detached_volume | `Failure` | 3990.75 | test_volumes.py
   


-- 
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-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DK101010 commented on a change in pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
DK101010 commented on a change in pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#discussion_r569365918



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VmwareVmImplementer.java
##########
@@ -139,6 +140,10 @@ VirtualMachineTO implement(VirtualMachineProfile vm, VirtualMachineTO to, long c
                     details.put(VmDetailConstants.NIC_ADAPTER, VirtualEthernetCardType.E1000.toString());
                 }
             }
+            if(vm.getVirtualMachine() instanceof VMInstanceVO){
+                VMInstanceVO vmInstanceVO =(VMInstanceVO) vm.getVirtualMachine();
+                to.setEnableDynamicallyScaleVm(vmInstanceVO.isDynamicallyScalable());

Review comment:
       > HypervisorGuruBase is already setting this paramter in toVirtualMachineTO(), can you please double check if this is necessary or redundant in VmwareVMImplementer.
   > to.setEnableDynamicallyScaleVm(isDynamicallyScalable);
   
   @harikrishna-patnala Hmm ... during my test I could enable/disable the flag in the fronend but in backend it keeps of false. That is the reason for my implementation in VmwareVmImplementer.java 
   
   I have checked the HypervisorGuru and found follow line 
   `Boolean isDynamicallyScalable = vmInstance.isDynamicallyScalable() && UserVmManager.EnableDynamicallyScaleVm.valueIn(vm.getDataCenterId());`
   
   I think I understood now what do you mean with zone settings. ;) But I ask me why we need two flags for the same thing. In my opinion it is confusing and not handy for a user to enable two flags to use this feature.   
   
   
   
   




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] harikrishna-patnala commented on a change in pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on a change in pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#discussion_r577360515



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VmwareVmImplementer.java
##########
@@ -139,6 +140,10 @@ VirtualMachineTO implement(VirtualMachineProfile vm, VirtualMachineTO to, long c
                     details.put(VmDetailConstants.NIC_ADAPTER, VirtualEthernetCardType.E1000.toString());
                 }
             }
+            if(vm.getVirtualMachine() instanceof VMInstanceVO){
+                VMInstanceVO vmInstanceVO =(VMInstanceVO) vm.getVirtualMachine();
+                to.setEnableDynamicallyScaleVm(vmInstanceVO.isDynamicallyScalable());

Review comment:
       Thanks @DaanHoogland for explaining that.
   Here in this case setting isDynamicallyScalable on VM has no dependency on hypervisor type that is why HypervisorGuruBase sets the dynamic scaling flag on VM while preparing TO (transfer object). Setting this flag in either VMwareGuru or VMwareVMImplementer is not required.
   
   @DK101010 I would suggest you to please revert the change HypervisorGuruBase in which you ignored the global/zone setting. This is required because global/zone level setting actually decides whether dynamic scaling can be enabled in that management setup or not. vmInstance.isDynamicallyScalable() is not sufficient. There is another PR#4643 which fixes and all these settings. For this PR you can keep the VMwareResource changes. 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DK101010 commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
DK101010 commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-771496778


   > @DK101010 we already have a flag per VM to enable or disable dynamic scalability of resources on the VM.
   > ![image](https://user-images.githubusercontent.com/3348673/106410976-16659880-646a-11eb-9bca-4d68c7724e55.png)
   > May I know why we need these settings again ?
   > 
   > Also keeping two different flags for CPU and memory may conflict during actual VM deployment (with different combinations of true and false) since we are controlling the dynamic scalability of VM with only one flag at global/zone setting.
   
   Hi @harikrishna-patnala, until now I don't know this feature. I had checked this, but how I can disable/enable hot add ? I can set dynamic scalability true/false but hot add cpu and memory keeps enabled.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-836501783


   Packaging result: :heavy_multiplication_x: centos7 :heavy_check_mark: centos8 :heavy_check_mark: debian. SL-JID 548


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] harikrishna-patnala commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-770536631


   @DK101010 we already have a flag per VM to enable or disable dynamic scalability of resources on the VM.
   ![image](https://user-images.githubusercontent.com/3348673/106410976-16659880-646a-11eb-9bca-4d68c7724e55.png)
   May I know why we need these settings again ?
   
   Also keeping two different flags for CPU and memory may conflict during actual VM deployment (with different combinations of true and false) since we are controlling the dynamic scalability of VM with only one flag at global/zone setting.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DK101010 commented on a change in pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
DK101010 commented on a change in pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#discussion_r578441412



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VmwareVmImplementer.java
##########
@@ -139,6 +140,10 @@ VirtualMachineTO implement(VirtualMachineProfile vm, VirtualMachineTO to, long c
                     details.put(VmDetailConstants.NIC_ADAPTER, VirtualEthernetCardType.E1000.toString());
                 }
             }
+            if(vm.getVirtualMachine() instanceof VMInstanceVO){
+                VMInstanceVO vmInstanceVO =(VMInstanceVO) vm.getVirtualMachine();
+                to.setEnableDynamicallyScaleVm(vmInstanceVO.isDynamicallyScalable());

Review comment:
       @DaanHoogland Thanks for input. Good to know for the future.
   
   @harikrishna-patnala sure, i can revert this, but I still don't find it practical to enable 2 flags to turn a feature on ;).




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-769844693


   @DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-894322065


   Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian. SL-JID 786


-- 
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-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-897693568


   @nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


-- 
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-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-894646738


   @DaanHoogland a Trillian-Jenkins matrix job (centos7 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests


-- 
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-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] rhtyd commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-895094417


   @blueorangutan centos7 vmware-67u3
   
   


-- 
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-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-898244690


   <b>Trillian test result (tid-1622)</b>
   Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 57150 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4630-t1622-vmware-67u3.zip
   Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
   Intermittent failure detected: /marvin/tests/smoke/test_list_ids_parameter.py
   Intermittent failure detected: /marvin/tests/smoke/test_network.py
   Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
   Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
   Smoke tests completed. 87 look OK, 2 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_04_rvpc_privategw_static_routes | `Failure` | 1477.14 | test_privategw_acl.py
   test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | `Failure` | 666.45 | test_vpc_redundant.py
   test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | `Failure` | 809.77 | test_vpc_redundant.py
   test_05_rvpc_multi_tiers | `Failure` | 607.88 | test_vpc_redundant.py
   


-- 
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-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DK101010 commented on a change in pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
DK101010 commented on a change in pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#discussion_r566875797



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -1959,15 +1959,15 @@ protected StartAnswer execute(StartCommand cmd) {
             }
 
             // Check for hotadd settings
-            vmConfigSpec.setMemoryHotAddEnabled(vmMo.isMemoryHotAddSupported(guestOsId));
+            vmConfigSpec.setMemoryHotAddEnabled(vmMo.isMemoryHotAddSupported(guestOsId) && Boolean.parseBoolean(vmSpec.getDetails().get(VmDetailConstants.HOT_ADD_MEMORY)));

Review comment:
       Nope, you can switch on/off like you want. 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] harikrishna-patnala commented on a change in pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on a change in pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#discussion_r616349611



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -1959,15 +1959,18 @@ protected StartAnswer execute(StartCommand cmd) {
             }
 
             // Check for hotadd settings
-            vmConfigSpec.setMemoryHotAddEnabled(vmMo.isMemoryHotAddSupported(guestOsId));
-
+            vmConfigSpec.setMemoryHotAddEnabled(vmMo.isMemoryHotAddSupported(guestOsId) && vmSpec.isEnableDynamicallyScaleVm());
             String hostApiVersion = ((HostMO) hyperHost).getHostAboutInfo().getApiVersion();
             if (numCoresPerSocket > 1 && hostApiVersion.compareTo("5.0") < 0) {
                 s_logger.warn("Dynamic scaling of CPU is not supported for Virtual Machines with multi-core vCPUs in case of ESXi hosts 4.1 and prior. Hence CpuHotAdd will not be"
                         + " enabled for Virtual Machine: " + vmInternalCSName);
                 vmConfigSpec.setCpuHotAddEnabled(false);
             } else {
-                vmConfigSpec.setCpuHotAddEnabled(vmMo.isCpuHotAddSupported(guestOsId));
+                vmConfigSpec.setCpuHotAddEnabled(vmMo.isCpuHotAddSupported(guestOsId) && vmSpec.isEnableDynamicallyScaleVm());
+            }
+
+            if(!vmMo.isMemoryHotAddSupported(guestOsId) && vmSpec.isEnableDynamicallyScaleVm()){
+                s_logger.warn("hotadd is not supported, dynamic scaling feature can not be applied " + vmInternalCSName);

Review comment:
       Thanks @DK101010 for making the changes and my apologies for coming on this PR this late. can you please add the reason being guest OS does not support hot add, something like "hotadd of memory is not supported by the guest OS, dynamic scaling feature can not be applied " + vmInternalCSName)".
   
   and please add another log for CPU hot add.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-894304635


   @nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


-- 
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-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-894646630


   @blueorangutan test matrix


-- 
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-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] rhtyd commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-895097218


   @blueorangutan help


-- 
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-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] nvazquez merged pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
nvazquez merged pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630


   


-- 
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-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-839000422


   <b>Trillian test result (tid-640)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 102056 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4630-t640-kvm-centos7.zip
   Intermittent failure detected: /marvin/tests/smoke/test_accounts.py
   Intermittent failure detected: /marvin/tests/smoke/test_affinity_groups_projects.py
   Intermittent failure detected: /marvin/tests/smoke/test_async_job.py
   Intermittent failure detected: /marvin/tests/smoke/test_deploy_virtio_scsi_vm.py
   Intermittent failure detected: /marvin/tests/smoke/test_deploy_vm_iso.py
   Intermittent failure detected: /marvin/tests/smoke/test_deploy_vms_with_varied_deploymentplanners.py
   Intermittent failure detected: /marvin/tests/smoke/test_deploy_vm_with_userdata.py
   Intermittent failure detected: /marvin/tests/smoke/test_diagnostics.py
   Intermittent failure detected: /marvin/tests/smoke/test_domain_service_offerings.py
   Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
   Intermittent failure detected: /marvin/tests/smoke/test_iso.py
   Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
   Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_supported_versions.py
   Intermittent failure detected: /marvin/tests/smoke/test_list_ids_parameter.py
   Intermittent failure detected: /marvin/tests/smoke/test_loadbalance.py
   Intermittent failure detected: /marvin/tests/smoke/test_metrics_api.py
   Intermittent failure detected: /marvin/tests/smoke/test_multipleips_per_nic.py
   Intermittent failure detected: /marvin/tests/smoke/test_nested_virtualization.py
   Intermittent failure detected: /marvin/tests/smoke/test_network_acl.py
   Intermittent failure detected: /marvin/tests/smoke/test_network.py
   Intermittent failure detected: /marvin/tests/smoke/test_nic_adapter_type.py
   Intermittent failure detected: /marvin/tests/smoke/test_password_server.py
   Intermittent failure detected: /marvin/tests/smoke/test_portforwardingrules.py
   Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
   Intermittent failure detected: /marvin/tests/smoke/test_projects.py
   Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_router_nics.py
   Intermittent failure detected: /marvin/tests/smoke/test_host_maintenance.py
   Smoke tests completed. 64 look OK, 24 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   ContextSuite context=TestAccounts>:setup | `Error` | 0.00 | test_accounts.py
   ContextSuite context=TestAddVmToSubDomain>:setup | `Error` | 0.00 | test_accounts.py
   test_DeleteDomain | `Error` | 4.01 | test_accounts.py
   test_forceDeleteDomain | `Failure` | 4.04 | test_accounts.py
   ContextSuite context=TestRemoveUserFromAccount>:setup | `Error` | 5.06 | test_accounts.py
   ContextSuite context=TestDomainsServiceOfferings>:setup | `Error` | 1514.62 | test_domain_service_offerings.py
   ContextSuite context=TestInternalLb>:setup | `Error` | 0.00 | test_internal_lb.py
   test_01_create_iso_with_checksum_sha1 | `Error` | 66.39 | test_iso.py
   test_02_create_iso_with_checksum_sha256 | `Error` | 66.40 | test_iso.py
   test_03_create_iso_with_checksum_md5 | `Error` | 66.40 | test_iso.py
   test_04_create_iso_with_no_checksum | `Error` | 66.40 | test_iso.py
   test_01_create_iso | `Failure` | 1511.95 | test_iso.py
   ContextSuite context=TestISO>:setup | `Error` | 3022.82 | test_iso.py
   ContextSuite context=TestAsyncJob>:setup | `Error` | 0.00 | test_async_job.py
   ContextSuite context=TestLoadBalance>:setup | `Error` | 0.00 | test_loadbalance.py
   ContextSuite context=TestDeployVirtioSCSIVM>:setup | `Error` | 0.00 | test_deploy_virtio_scsi_vm.py
   test_list_clusters_metrics | `Error` | 1511.66 | test_metrics_api.py
   test_list_vms_metrics | `Error` | 0.15 | test_metrics_api.py
   ContextSuite context=TestDeployVMFromISO>:setup | `Error` | 0.00 | test_deploy_vm_iso.py
   ContextSuite context=TestDeployVmWithVariedPlanners>:setup | `Error` | 0.00 | test_deploy_vms_with_varied_deploymentplanners.py
   ContextSuite context=TestNetworkACL>:setup | `Error` | 0.00 | test_network_acl.py
   ContextSuite context=TestDeployVmWithUserData>:setup | `Error` | 0.00 | test_deploy_vm_with_userdata.py
   ContextSuite context=TestRemoteDiagnostics>:setup | `Error` | 0.00 | test_diagnostics.py
   test_delete_account | `Error` | 1511.59 | test_network.py
   test_delete_network_while_vm_on_it | `Error` | 1.11 | test_network.py
   test_deploy_vm_l2network | `Error` | 1.10 | test_network.py
   test_l2network_restart | `Error` | 2.17 | test_network.py
   ContextSuite context=TestPortForwarding>:setup | `Error` | 3.32 | test_network.py
   ContextSuite context=TestPublicIP>:setup | `Error` | 1.04 | test_network.py
   test_reboot_router | `Failure` | 0.04 | test_network.py
   test_releaseIP | `Error` | 0.44 | test_network.py
   ContextSuite context=TestRouterRules>:setup | `Error` | 0.49 | test_network.py
   ContextSuite context=TestAdapterTypeForNic>:setup | `Error` | 0.00 | test_nic_adapter_type.py
   test_01_invalid_upgrade_kubernetes_cluster | `Failure` | 0.00 | test_kubernetes_clusters.py
   test_02_deploy_and_upgrade_kubernetes_cluster | `Failure` | 0.00 | test_kubernetes_clusters.py
   test_03_deploy_and_scale_kubernetes_cluster | `Failure` | 0.00 | test_kubernetes_clusters.py
   test_04_basic_lifecycle_kubernetes_cluster | `Failure` | 0.00 | test_kubernetes_clusters.py
   test_05_delete_kubernetes_cluster | `Failure` | 0.00 | test_kubernetes_clusters.py
   test_06_deploy_invalid_kubernetes_ha_cluster | `Failure` | 0.00 | test_kubernetes_clusters.py
   test_07_deploy_kubernetes_ha_cluster | `Failure` | 0.00 | test_kubernetes_clusters.py
   test_08_deploy_and_upgrade_kubernetes_ha_cluster | `Failure` | 0.00 | test_kubernetes_clusters.py
   test_09_delete_kubernetes_ha_cluster | `Failure` | 0.00 | test_kubernetes_clusters.py
   test_01_add_delete_kubernetes_supported_version | `Error` | 1801.99 | test_kubernetes_supported_versions.py
   ContextSuite context=TestListIdsParams>:setup | `Error` | 0.00 | test_list_ids_parameter.py
   test_nic_secondaryip_add_remove | `Error` | 1511.60 | test_multipleips_per_nic.py
   ContextSuite context=TestNestedVirtualization>:setup | `Error` | 0.00 | test_nested_virtualization.py
   ContextSuite context=TestIsolatedNetworksPasswdServer>:setup | `Error` | 0.00 | test_password_server.py
   ContextSuite context=TestPortForwardingRules>:setup | `Error` | 0.00 | test_portforwardingrules.py
   ContextSuite context=TestPrivateGwACL>:setup | `Error` | 0.00 | test_privategw_acl.py
   ContextSuite context=TestProjectSuspendActivate>:setup | `Error` | 1517.46 | test_projects.py
   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] weizhouapache commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-769669465


   code lgtm


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#discussion_r626523666



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -1959,15 +1959,22 @@ protected StartAnswer execute(StartCommand cmd) {
             }
 
             // Check for hotadd settings
-            vmConfigSpec.setMemoryHotAddEnabled(vmMo.isMemoryHotAddSupported(guestOsId));
-
+            vmConfigSpec.setMemoryHotAddEnabled(vmMo.isMemoryHotAddSupported(guestOsId) && vmSpec.isEnableDynamicallyScaleVm());
             String hostApiVersion = ((HostMO) hyperHost).getHostAboutInfo().getApiVersion();
             if (numCoresPerSocket > 1 && hostApiVersion.compareTo("5.0") < 0) {
                 s_logger.warn("Dynamic scaling of CPU is not supported for Virtual Machines with multi-core vCPUs in case of ESXi hosts 4.1 and prior. Hence CpuHotAdd will not be"
                         + " enabled for Virtual Machine: " + vmInternalCSName);
                 vmConfigSpec.setCpuHotAddEnabled(false);
             } else {
-                vmConfigSpec.setCpuHotAddEnabled(vmMo.isCpuHotAddSupported(guestOsId));
+                vmConfigSpec.setCpuHotAddEnabled(vmMo.isCpuHotAddSupported(guestOsId) && vmSpec.isEnableDynamicallyScaleVm());
+            }
+
+            if(!vmMo.isMemoryHotAddSupported(guestOsId) && vmSpec.isEnableDynamicallyScaleVm()){
+                s_logger.warn("hotadd of memory is not supported, dynamic scaling feature can not be applied " + vmInternalCSName);
+            }
+
+            if(!vmMo.isCpuHotAddSupported(guestOsId) && vmSpec.isEnableDynamicallyScaleVm()){
+                s_logger.warn("hotadd of cpu is not supported, dynamic scaling feature can not be applied " + vmInternalCSName);

Review comment:
       ```suggestion
                   s_logger.warn("hotadd of cpu is not supported, dynamic scaling feature can not be applied to vm: " + vmInternalCSName);
   ```




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] nvazquez commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-894304066


   @blueorangutan package


-- 
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-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DK101010 commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
DK101010 commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-772425809


   @harikrishna-patnala @DaanHoogland Here is a alternative implementation to use the dynamic scalability flag. What do you think?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-894713483


   <b>Trillian test result (tid-1527)</b>
   Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 35318 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4630-t1527-xenserver-71.zip
   Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
   Smoke tests completed. 89 look OK, 0 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   


-- 
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-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DK101010 commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
DK101010 commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-771496778


   > @DK101010 we already have a flag per VM to enable or disable dynamic scalability of resources on the VM.
   > ![image](https://user-images.githubusercontent.com/3348673/106410976-16659880-646a-11eb-9bca-4d68c7724e55.png)
   > May I know why we need these settings again ?
   > 
   > Also keeping two different flags for CPU and memory may conflict during actual VM deployment (with different combinations of true and false) since we are controlling the dynamic scalability of VM with only one flag at global/zone setting.
   
   Hi @harikrishna-patnala, until now I don't know this feature. I had checked this, but how I can disable/enable hot add ? I can set dynamic scalability true/false but hot add cpu and memory keeps enabled.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DK101010 commented on a change in pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
DK101010 commented on a change in pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#discussion_r620199452



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -1959,15 +1959,18 @@ protected StartAnswer execute(StartCommand cmd) {
             }
 
             // Check for hotadd settings
-            vmConfigSpec.setMemoryHotAddEnabled(vmMo.isMemoryHotAddSupported(guestOsId));
-
+            vmConfigSpec.setMemoryHotAddEnabled(vmMo.isMemoryHotAddSupported(guestOsId) && vmSpec.isEnableDynamicallyScaleVm());
             String hostApiVersion = ((HostMO) hyperHost).getHostAboutInfo().getApiVersion();
             if (numCoresPerSocket > 1 && hostApiVersion.compareTo("5.0") < 0) {
                 s_logger.warn("Dynamic scaling of CPU is not supported for Virtual Machines with multi-core vCPUs in case of ESXi hosts 4.1 and prior. Hence CpuHotAdd will not be"
                         + " enabled for Virtual Machine: " + vmInternalCSName);
                 vmConfigSpec.setCpuHotAddEnabled(false);
             } else {
-                vmConfigSpec.setCpuHotAddEnabled(vmMo.isCpuHotAddSupported(guestOsId));
+                vmConfigSpec.setCpuHotAddEnabled(vmMo.isCpuHotAddSupported(guestOsId) && vmSpec.isEnableDynamicallyScaleVm());
+            }
+
+            if(!vmMo.isMemoryHotAddSupported(guestOsId) && vmSpec.isEnableDynamicallyScaleVm()){
+                s_logger.warn("hotadd is not supported, dynamic scaling feature can not be applied " + vmInternalCSName);

Review comment:
       @harikrishna-patnala no problem, I will adapt it.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-895098987


   @rhtyd a Trillian-Jenkins test job (centos7 mgmt + vmware-67u3) has been kicked to run smoke tests


-- 
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-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan removed a comment on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-836501783






-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] harikrishna-patnala commented on a change in pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on a change in pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#discussion_r569340333



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -1959,15 +1959,14 @@ protected StartAnswer execute(StartCommand cmd) {
             }
 
             // Check for hotadd settings
-            vmConfigSpec.setMemoryHotAddEnabled(vmMo.isMemoryHotAddSupported(guestOsId));
-
+            vmConfigSpec.setMemoryHotAddEnabled(vmMo.isMemoryHotAddSupported(guestOsId) && vmSpec.isEnableDynamicallyScaleVm());

Review comment:
       Thanks @DK101010, this seems to be good.
   I'm also thinking, if memory hot add memory and CPU are not supported by OS and vmSpec wants the VM to be dynamically scalable, may be we should atleast log it saying this is not supported by OS. Can you please do that here.

##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VmwareVmImplementer.java
##########
@@ -139,6 +140,10 @@ VirtualMachineTO implement(VirtualMachineProfile vm, VirtualMachineTO to, long c
                     details.put(VmDetailConstants.NIC_ADAPTER, VirtualEthernetCardType.E1000.toString());
                 }
             }
+            if(vm.getVirtualMachine() instanceof VMInstanceVO){
+                VMInstanceVO vmInstanceVO =(VMInstanceVO) vm.getVirtualMachine();
+                to.setEnableDynamicallyScaleVm(vmInstanceVO.isDynamicallyScalable());

Review comment:
       HypervisorGuruBase is already setting this paramter in toVirtualMachineTO(), can you please double check if this is necessary or redundant in VmwareVMImplementer.
           to.setEnableDynamicallyScaleVm(isDynamicallyScalable);




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-895097543


   @rhtyd I understand these words: "help", "hello", "thanks", "package", "test"
   Test command usage: test [mgmt os] [hypervisor] [keepEnv]
   Mgmt OS options: ['centos7', 'centos6', 'alma8', 'ubuntu18', 'suse15', 'ubuntu20', 'rocky8', 'centos8']
   Hypervisor options: ['kvm-centos6', 'kvm-centos7', 'kvm-centos8', 'kvm-rocky8', 'kvm-alma8', 'kvm-ubuntu18', 'kvm-ubuntu20', 'kvm-suse15', 'vmware-55u3', 'vmware-60u2', 'vmware-65u2', 'vmware-67u3', 'vmware-70u1', 'xenserver-65sp1', 'xenserver-71', 'xenserver-74', 'xcpng74', 'xcpng76', 'xcpng80', 'xcpng81']
   	Note: when keepEnv is passed, you need to specify mgmt server os and hypervisor or use the `matrix` command.
   
   Blessed contributors for kicking Trillian test jobs: ['rhtyd', 'nvazquez', 'PaulAngus', 'borisstoyanov', 'DaanHoogland', 'shwstppr', 'andrijapanicsb', 'Spaceman1984', 'Pearl1594', 'davidjumani', 'harikrishna-patnala', 'vladimirpetrov', 'sureshanaparti', 'weizhouapache']


-- 
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-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DK101010 commented on a change in pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
DK101010 commented on a change in pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#discussion_r568435815



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -1959,15 +1959,15 @@ protected StartAnswer execute(StartCommand cmd) {
             }
 
             // Check for hotadd settings
-            vmConfigSpec.setMemoryHotAddEnabled(vmMo.isMemoryHotAddSupported(guestOsId));
+            vmConfigSpec.setMemoryHotAddEnabled(vmMo.isMemoryHotAddSupported(guestOsId) && Boolean.parseBoolean(vmSpec.getDetails().get(VmDetailConstants.HOT_ADD_MEMORY)));

Review comment:
       Hi @DaanHoogland, At first I thought it also. But this method will be called each time when you start a vm and each time will be update the vm config spec. Btw. you can find in VmwareHelper in method setBasicVmConfig similar logic to enable/disable hot add but it will be override from this method. I find it also a little bit strange and perhaps it have refactoring potential. But currently I have not so much time and knowledge to do it. Independent from this, I tested the code to enable and disable hot add for cpu and memory and it works like expected. I checked it also in VCenter if it enabled/disabled.  




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#discussion_r566868813



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -1959,15 +1959,15 @@ protected StartAnswer execute(StartCommand cmd) {
             }
 
             // Check for hotadd settings
-            vmConfigSpec.setMemoryHotAddEnabled(vmMo.isMemoryHotAddSupported(guestOsId));
+            vmConfigSpec.setMemoryHotAddEnabled(vmMo.isMemoryHotAddSupported(guestOsId) && Boolean.parseBoolean(vmSpec.getDetails().get(VmDetailConstants.HOT_ADD_MEMORY)));

Review comment:
       wouldn't this mean you can turn it off but never back on again?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-854950713


   <b>Trillian test result (tid-857)</b>
   Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 37551 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4630-t857-vmware-67u3.zip
   Smoke tests completed. 88 look OK, 0 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] rhtyd commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-895098245


   @blueorangutan test centos7 vmware-67u3


-- 
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-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-895683370


   <b>Trillian test result (tid-1546)</b>
   Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 57475 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4630-t1546-vmware-67u3.zip
   Intermittent failure detected: /marvin/tests/smoke/test_accounts.py
   Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
   Intermittent failure detected: /marvin/tests/smoke/test_nested_virtualization.py
   Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
   Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
   Intermittent failure detected: /marvin/tests/smoke/test_host_maintenance.py
   Smoke tests completed. 85 look OK, 4 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_03_vpc_privategw_restart_vpc_cleanup | `Failure` | 442.28 | test_privategw_acl.py
   test_04_rvpc_privategw_static_routes | `Failure` | 753.07 | test_privategw_acl.py
   test_01_isolate_network_FW_PF_default_routes_egress_true | `Error` | 155.63 | test_routers_network_ops.py
   test_02_isolate_network_FW_PF_default_routes_egress_false | `Failure` | 147.06 | test_routers_network_ops.py
   test_01_RVR_Network_FW_PF_SSH_default_routes_egress_true | `Failure` | 436.94 | test_routers_network_ops.py
   test_02_RVR_Network_FW_PF_SSH_default_routes_egress_false | `Failure` | 449.87 | test_routers_network_ops.py
   test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | `Failure` | 686.77 | test_vpc_redundant.py
   test_05_rvpc_multi_tiers | `Failure` | 635.23 | test_vpc_redundant.py
   test_02_cancel_host_maintenace_with_migration_jobs | `Error` | 165.73 | test_host_maintenance.py
   test_03_cancel_host_maintenace_with_migration_jobs_failure | `Error` | 21.98 | test_host_maintenance.py
   


-- 
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-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-858676532


   @borisstoyanov @vladimirpetrov any extra tests required? cc @sureshanaparti @nvazquez 


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] rhtyd commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-895053228


   @blueorangutan centos7 vmware-67u3


-- 
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-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#discussion_r575648160



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VmwareVmImplementer.java
##########
@@ -139,6 +140,10 @@ VirtualMachineTO implement(VirtualMachineProfile vm, VirtualMachineTO to, long c
                     details.put(VmDetailConstants.NIC_ADAPTER, VirtualEthernetCardType.E1000.toString());
                 }
             }
+            if(vm.getVirtualMachine() instanceof VMInstanceVO){
+                VMInstanceVO vmInstanceVO =(VMInstanceVO) vm.getVirtualMachine();
+                to.setEnableDynamicallyScaleVm(vmInstanceVO.isDynamicallyScalable());

Review comment:
       bit of background guys: The VmwareVMImplementer is a worker class that is part of the VmwareGuru, and is meant to reduce the complexity by extracting the deploy and start code. It would be good to put shared code in a VmwareGuruUtilities class to prevent duplication. I can imagine that setting flags can be done on implement as well as on restart/migrate or other methods. I might be guilty of this redundancy, but take care that setting might happen twice in one scenario but twice in another ...




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-836576043


   Packaging result: :heavy_check_mark: centos7 :heavy_check_mark: centos8 :heavy_check_mark: debian. SL-JID 553


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DK101010 commented on a change in pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
DK101010 commented on a change in pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#discussion_r569367766



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -1959,15 +1959,14 @@ protected StartAnswer execute(StartCommand cmd) {
             }
 
             // Check for hotadd settings
-            vmConfigSpec.setMemoryHotAddEnabled(vmMo.isMemoryHotAddSupported(guestOsId));
-
+            vmConfigSpec.setMemoryHotAddEnabled(vmMo.isMemoryHotAddSupported(guestOsId) && vmSpec.isEnableDynamicallyScaleVm());

Review comment:
       @harikrishna-patnala sure, shouldn't be a problem. I will add this




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#discussion_r626523410



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -1959,15 +1959,22 @@ protected StartAnswer execute(StartCommand cmd) {
             }
 
             // Check for hotadd settings
-            vmConfigSpec.setMemoryHotAddEnabled(vmMo.isMemoryHotAddSupported(guestOsId));
-
+            vmConfigSpec.setMemoryHotAddEnabled(vmMo.isMemoryHotAddSupported(guestOsId) && vmSpec.isEnableDynamicallyScaleVm());
             String hostApiVersion = ((HostMO) hyperHost).getHostAboutInfo().getApiVersion();
             if (numCoresPerSocket > 1 && hostApiVersion.compareTo("5.0") < 0) {
                 s_logger.warn("Dynamic scaling of CPU is not supported for Virtual Machines with multi-core vCPUs in case of ESXi hosts 4.1 and prior. Hence CpuHotAdd will not be"
                         + " enabled for Virtual Machine: " + vmInternalCSName);
                 vmConfigSpec.setCpuHotAddEnabled(false);
             } else {
-                vmConfigSpec.setCpuHotAddEnabled(vmMo.isCpuHotAddSupported(guestOsId));
+                vmConfigSpec.setCpuHotAddEnabled(vmMo.isCpuHotAddSupported(guestOsId) && vmSpec.isEnableDynamicallyScaleVm());
+            }
+
+            if(!vmMo.isMemoryHotAddSupported(guestOsId) && vmSpec.isEnableDynamicallyScaleVm()){
+                s_logger.warn("hotadd of memory is not supported, dynamic scaling feature can not be applied " + vmInternalCSName);

Review comment:
       ```suggestion
                   s_logger.warn("hotadd of memory is not supported, dynamic scaling feature can not be applied to vm: " + vmInternalCSName);
   ```




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] nvazquez commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-897715102


   @blueorangutan test centos7 vmware-67u3


-- 
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-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-897716087


   @nvazquez a Trillian-Jenkins test job (centos7 mgmt + vmware-67u3) has been kicked to run smoke tests


-- 
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-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-894646882


   <b>Trillian Build Failed (tid-1529)<b/>


-- 
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-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#discussion_r567678690



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -1959,15 +1959,15 @@ protected StartAnswer execute(StartCommand cmd) {
             }
 
             // Check for hotadd settings
-            vmConfigSpec.setMemoryHotAddEnabled(vmMo.isMemoryHotAddSupported(guestOsId));
+            vmConfigSpec.setMemoryHotAddEnabled(vmMo.isMemoryHotAddSupported(guestOsId) && Boolean.parseBoolean(vmSpec.getDetails().get(VmDetailConstants.HOT_ADD_MEMORY)));

Review comment:
       @DK101010, I probably don't understand but the `vmMo` is found on the hypervisor with `vmMo = hyperHost.findVmOnHyperHost(vmInternalCSName);`, if it was already created. And if on creation `memoryHotAddEnabled` was set to false, the above will always be false on the next start, and hence all subsequent. Maybe this is not in the functional intent of this PR but it seems when you turn it off for a VM it will not be turned on again if the user switches it on again.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-854476980


   Packaging result: :heavy_check_mark: centos7 :heavy_check_mark: centos8 :heavy_check_mark: debian. SL-JID 163


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] rhtyd removed a comment on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
rhtyd removed a comment on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-895053228


   @blueorangutan centos7 vmware-67u3


-- 
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-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] harikrishna-patnala commented on a change in pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on a change in pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#discussion_r616349611



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -1959,15 +1959,18 @@ protected StartAnswer execute(StartCommand cmd) {
             }
 
             // Check for hotadd settings
-            vmConfigSpec.setMemoryHotAddEnabled(vmMo.isMemoryHotAddSupported(guestOsId));
-
+            vmConfigSpec.setMemoryHotAddEnabled(vmMo.isMemoryHotAddSupported(guestOsId) && vmSpec.isEnableDynamicallyScaleVm());
             String hostApiVersion = ((HostMO) hyperHost).getHostAboutInfo().getApiVersion();
             if (numCoresPerSocket > 1 && hostApiVersion.compareTo("5.0") < 0) {
                 s_logger.warn("Dynamic scaling of CPU is not supported for Virtual Machines with multi-core vCPUs in case of ESXi hosts 4.1 and prior. Hence CpuHotAdd will not be"
                         + " enabled for Virtual Machine: " + vmInternalCSName);
                 vmConfigSpec.setCpuHotAddEnabled(false);
             } else {
-                vmConfigSpec.setCpuHotAddEnabled(vmMo.isCpuHotAddSupported(guestOsId));
+                vmConfigSpec.setCpuHotAddEnabled(vmMo.isCpuHotAddSupported(guestOsId) && vmSpec.isEnableDynamicallyScaleVm());
+            }
+
+            if(!vmMo.isMemoryHotAddSupported(guestOsId) && vmSpec.isEnableDynamicallyScaleVm()){
+                s_logger.warn("hotadd is not supported, dynamic scaling feature can not be applied " + vmInternalCSName);

Review comment:
       Thanks @DK101010 for making the changes. can you please add the reason being guest OS does not support hot add, something like "hotadd of memory is not supported by the guest OS, dynamic scaling feature can not be applied " + vmInternalCSName)".
   
   and please add another log for CPU hot add.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] nvazquez commented on pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#issuecomment-897693240


   @blueorangutan package


-- 
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-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DK101010 commented on a change in pull request #4630: disable hot add memory and cpu via vm settings

Posted by GitBox <gi...@apache.org>.
DK101010 commented on a change in pull request #4630:
URL: https://github.com/apache/cloudstack/pull/4630#discussion_r568435815



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -1959,15 +1959,15 @@ protected StartAnswer execute(StartCommand cmd) {
             }
 
             // Check for hotadd settings
-            vmConfigSpec.setMemoryHotAddEnabled(vmMo.isMemoryHotAddSupported(guestOsId));
+            vmConfigSpec.setMemoryHotAddEnabled(vmMo.isMemoryHotAddSupported(guestOsId) && Boolean.parseBoolean(vmSpec.getDetails().get(VmDetailConstants.HOT_ADD_MEMORY)));

Review comment:
       Hi @DaanHoogland, At first I thought it also. But this method will be called each time when you start a vm and each time will be update the vm config spec. Btw. you can find in VmwareHelper in method setBasicVmConfig similar logic to enable/disable hot add but it will be override from this method. I find it also a little bit strange and perhaps it have refactoring potential. But currently I have not so much time and knowledge to do it. Independent from this, I tested the code to enable and disable hot add for cpu and memory and it works like expected. I checked it also in VCenter if it enabled/disabled.  




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org