Feat/ram reservation by DK101010 · Pull Request #4662 · apache/cloudstack

DK101010 · 2021-02-08T08:40:43Z

Description

WIth this PR user can manuelly modify the ram reservation for a specific vm in vmware context. User set it via vm settings as a float value between 0 and 1

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
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
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

tested in own vmware environment

ram and cpu reservation have not relation to ram and cpu hot add

DaanHoogland · 2021-02-08T11:14:21Z

vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java

-        if (cpuSpeedMHz != cpuReservedMhz) {
-            vmConfig.setCpuHotAddEnabled(true);
-        }
-        if (memoryMB != memoryReserveMB) {
-            vmConfig.setMemoryHotAddEnabled(true);
-        }


thsi removal does not seem relevant to your feature description. can you explain @DK101010 ?

@DaanHoogland sorry for my late answer. In my opinion I think yes and no.
Hot add memory and cpu will be enabled when reserved memory and cpu is in use. But this is not correct. Both features are independent, therefore I thought it is good to remove it in this PR. But if you want I can create a separate PR for this commit.

not sure if that is needed and/or suffices; How about when people want to hot-add and don't care about reservation?

@DaanHoogland hot add and reservation are two different features and they are independent of each other. I talked with my team and test it in my environment to get sure and in my env it works. In my point of view this code lines makes no sense.

ok @DK101010 , if that's verified I'm fine with the code.

I have not much experience with the codebase for VMware to comment on this piece of code. Good that it has been verified. I will rely on the contributor tests.

I've my doubts on these changes, pinged vmware experts @harikrishna-patnala @nvazquez @sureshanaparti

@DK101010 I think removing these lines will cause a regression on cpu and memory overcommit, have you tested those cases?

Hi @nvazquez not sure what do you mean with regression.
Reservation and hot add are two different things. I can use reservation in it doesn't matter if hot add enabled/disbaled and it makes for me not sense to enable this when I'm reserve memory .
Also you can't more reserve as you have for the current vm.

I may be missing something, but currently I don't see a problem here.

DaanHoogland · 2021-02-12T10:41:46Z

@blueorangutan package

blueorangutan · 2021-02-12T10:42:14Z

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

blueorangutan · 2021-02-12T12:09:58Z

Packaging result: ✖centos7 ✔centos8 ✖debian. JID-2687

DaanHoogland · 2021-02-13T10:03:46Z

@blueorangutan package

blueorangutan · 2021-02-13T10:04:19Z

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

blueorangutan · 2021-02-13T11:22:44Z

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

DaanHoogland · 2021-02-13T11:34:25Z

@blueorangutan test centos7 vmware-67u3

blueorangutan · 2021-02-13T11:35:19Z

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

blueorangutan · 2021-02-13T23:47:27Z

Trillian test result (tid-3537)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 42405 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4662-t3537-vmware-67u3.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 84 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_03_deploy_and_upgrade_kubernetes_cluster	`Failure`	779.98	test_kubernetes_clusters.py
test_02_redundant_VPC_default_routes	`Failure`	262.17	test_vpc_redundant.py

...ns/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java

…vmware/resource/VmwareResource.java Co-authored-by: dahn <daan.hoogland@gmail.com>

blueorangutan · 2021-05-10T10:21:24Z

Packaging result: ✔️ centos7 ✔️ centos8 ✔️ debian. SL-JID 550

blueorangutan · 2021-05-11T00:03:10Z

Trillian test result (tid-638)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 44049 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4662-t638-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Smoke tests completed. 88 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

DaanHoogland · 2021-05-11T06:44:03Z

@GabrielBrascher @sureshanaparti @nvazquez (not sure who else to bother), can you guys look at this?
tnx

GabrielBrascher

Added a small suggestion.

GabrielBrascher · 2021-05-13T15:05:17Z

vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java

-        if (cpuSpeedMHz != cpuReservedMhz) {
-            vmConfig.setCpuHotAddEnabled(true);
-        }
-        if (memoryMB != memoryReserveMB) {
-            vmConfig.setMemoryHotAddEnabled(true);
-        }


I have not much experience with the codebase for VMware to comment on this piece of code. Good that it has been verified. I will rely on the contributor tests.

GabrielBrascher · 2021-05-13T15:06:27Z

plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VmwareVmImplementer.java

                }
            }
+            if(StringUtils.isEmpty(details.get(VmDetailConstants.RAM_RESERVATION))){
+                details.put(VmDetailConstants.RAM_RESERVATION, "0.0");


There are two occurrences of the "0.0" String.

What do you think of replacing these "0.0" with NumberUtils.DOUBLE_ZERO? The NumberUtils will return a Double object of 0.0d and, therefore, it has a toString() that returns the String "0.0".

Other option would be to create a constant with a self-describing name.

Hi @GabrielBrascher, sorry for my late answer, I must have missed the notification. I think there are no benefits regards reusable of this code, but I agree it's better to use parameter instead of strings. I'm prefer your first suggestion and will adapt it.

blueorangutan · 2021-06-11T13:30:10Z

Packaging result: ✔️ centos7 ✔️ centos8 ✔️ debian. SL-JID 214

blueorangutan · 2021-06-14T22:31:13Z

Trillian test result (tid-937)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 38713 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4662-t937-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_diagnostics.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 87 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_disable_oobm_ha_state_ineligible	`Error`	1513.49	test_hostha_kvm.py

DaanHoogland

clgtm, test failure is does not seem related

nvazquez · 2021-07-29T13:38:16Z

@blueorangutan test centos7 vmware-67u3

blueorangutan · 2021-07-29T13:39:03Z

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

...ns/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java

blueorangutan · 2021-07-30T08:15:34Z

Trillian test result (tid-1417)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 65614 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4662-t1417-vmware-67u3.zip
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_internal_lb.py
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_password_server.py
Intermittent failure detected: /marvin/tests/smoke/test_primary_storage.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_service_offerings.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_host_maintenance.py
Smoke tests completed. 81 look OK, 8 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_deployvm_userconcentrated	`Error`	93.84	test_deploy_vms_with_varied_deploymentplanners.py
test_deployvm_userdispersing	`Error`	6.26	test_deploy_vms_with_varied_deploymentplanners.py
test_01_primary_storage_nfs	`Error`	0.10	test_primary_storage.py
ContextSuite context=TestStorageTags>:setup	`Error`	0.18	test_primary_storage.py
test_02_internallb_roundrobin_1RVPC_3VM_HTTP_port80	`Failure`	708.23	test_internal_lb.py
test_02_internallb_roundrobin_1RVPC_3VM_HTTP_port80	`Error`	708.25	test_internal_lb.py
test_01_invalid_upgrade_kubernetes_cluster	`Failure`	3606.42	test_kubernetes_clusters.py
test_02_deploy_and_upgrade_kubernetes_cluster	`Failure`	3603.31	test_kubernetes_clusters.py
test_03_deploy_and_scale_kubernetes_cluster	`Failure`	0.05	test_kubernetes_clusters.py
test_04_basic_lifecycle_kubernetes_cluster	`Failure`	0.04	test_kubernetes_clusters.py
test_05_delete_kubernetes_cluster	`Failure`	0.04	test_kubernetes_clusters.py
test_07_deploy_kubernetes_ha_cluster	`Failure`	0.05	test_kubernetes_clusters.py
test_08_deploy_and_upgrade_kubernetes_ha_cluster	`Failure`	0.05	test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster	`Failure`	0.04	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown	`Error`	47.16	test_kubernetes_clusters.py
test_04_rvpc_privategw_static_routes	`Failure`	916.43	test_privategw_acl.py
test_01_RVR_Network_FW_PF_SSH_default_routes_egress_true	`Failure`	409.76	test_routers_network_ops.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL	`Failure`	835.79	test_vpc_redundant.py
test_02_redundant_VPC_default_routes	`Failure`	399.26	test_vpc_redundant.py
test_02_redundant_VPC_default_routes	`Error`	399.27	test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers	`Failure`	628.53	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Failure`	361.98	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Error`	361.99	test_vpc_redundant.py
test_02_cancel_host_maintenace_with_migration_jobs	`Error`	20.75	test_host_maintenance.py

…vmware/resource/VmwareResource.java Co-authored-by: davidjumani <dj.davidjumani1994@gmail.com>

nvazquez · 2021-07-30T11:40:28Z

@blueorangutan package

blueorangutan · 2021-07-30T11:41:02Z

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

blueorangutan · 2021-07-30T12:11:23Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 711

nvazquez · 2021-07-30T12:33:11Z

@blueorangutan test

blueorangutan · 2021-07-30T12:34:03Z

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

blueorangutan · 2021-07-31T00:59:04Z

Trillian test result (tid-1432)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 42843 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4662-t1432-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_primary_storage.py
Intermittent failure detected: /marvin/tests/smoke/test_resource_accounting.py
Intermittent failure detected: /marvin/tests/smoke/test_snapshots.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 85 look OK, 4 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_add_primary_storage_disabled_host	`Error`	0.54	test_primary_storage.py
test_01_primary_storage_nfs	`Error`	0.11	test_primary_storage.py
ContextSuite context=TestStorageTags>:setup	`Error`	0.19	test_primary_storage.py
test_02_list_snapshots_with_removed_data_store	`Error`	1.17	test_snapshots.py
test_01_secure_vm_migration	`Error`	156.12	test_vm_life_cycle.py
test_02_unsecure_vm_migration	`Error`	278.68	test_vm_life_cycle.py
test_03_secured_to_nonsecured_vm_migration	`Error`	145.71	test_vm_life_cycle.py
test_08_migrate_vm	`Error`	43.80	test_vm_life_cycle.py
test_hostha_kvm_host_degraded	`Error`	705.74	test_hostha_kvm.py
test_hostha_kvm_host_fencing	`Error`	681.02	test_hostha_kvm.py

nvazquez

Only one concern, besides it it looks good

nvazquez · 2021-08-02T15:39:22Z

vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java


        cpuInfo.setReservation((long)cpuReservedMhz);
        vmConfig.setCpuAllocation(cpuInfo);
-        if (cpuSpeedMHz != cpuReservedMhz) {


@DK101010 I think removing these lines will cause a regression on cpu and memory overcommit, have you tested those cases?

Hi @nvazquez not sure what do you mean with regression.
Reservation and hot add are two different things. I can use reservation in it doesn't matter if hot add enabled/disbaled and it makes for me not sense to enable this when I'm reserve memory .
Also you can't more reserve as you have for the current vm.

I may be missing something, but currently I don't see a problem here.

@DK101010 I have examined the setBasicVmConfig(..) method is used on:

VmwareResource.execute(StartCommand cmd) -> the values set on these removed lines are not relevant, are overridden later (https://github.com/apache/cloudstack/blob/main/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java#L1995-L2005)

HypervisorHostHelper.createBlankVm(..) -> possible regression at this point: blank VMs are always created with cpuHotAdd = false and memHotAdd = false. I'm not sure how/if this would affect deployments, any thoughts?

cc. @sureshanaparti @rhtyd @DaanHoogland

@nvazquez Hot-adding cpu & memory are set / overridden based on guest OS support. Also, the reservation and hot add are different things. So, I think, the hot add config here seems not relevant now.

Thanks @sureshanaparti

nvazquez · 2021-08-11T13:18:35Z

@blueorangutan package

blueorangutan · 2021-08-11T13:19:04Z

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

blueorangutan · 2021-08-11T13:44:55Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 843

nvazquez · 2021-08-11T13:53:58Z

@blueorangutan test centos7 vmware-67u3

blueorangutan · 2021-08-11T13:55:03Z

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

nvazquez · 2021-08-16T03:11:52Z

@blueorangutan test centos7 vmware-67u3

nvazquez · 2021-08-19T04:50:01Z

@blueorangutan test centos7 vmware-67u3

blueorangutan · 2021-08-19T04:51:03Z

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

nvazquez · 2021-08-24T04:45:49Z

@blueorangutan package

blueorangutan · 2021-08-24T04:46:03Z

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

blueorangutan · 2021-08-24T05:21:34Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 987

nvazquez · 2021-08-24T05:23:57Z

@blueorangutan test centos7 vmware-67u3

blueorangutan · 2021-08-24T05:25:04Z

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

blueorangutan · 2021-08-24T15:48:26Z

Trillian test result (tid-1759)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36206 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4662-t1759-vmware-67u3.zip
Smoke tests completed. 89 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

DK101010 added 2 commits February 8, 2021 07:57

remove hot enable cpu und memory in case of reservation

d7bed55

ram and cpu reservation have not relation to ram and cpu hot add

add custom ram_reservation and it to vm details

ee04678

DaanHoogland added this to the 4.16.0.0 milestone Feb 8, 2021

DaanHoogland added component:vmware Severity:Minor type:enhancement labels Feb 8, 2021

DaanHoogland reviewed Feb 8, 2021

View reviewed changes

DaanHoogland added the complexity:simple a few days to a couple of weeks label Feb 8, 2021

system vms haven't this property, for this reason add additional check

acf5218

DaanHoogland reviewed May 10, 2021

View reviewed changes

...ns/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java Outdated Show resolved Hide resolved

Update plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/…

57bd3c5

…vmware/resource/VmwareResource.java Co-authored-by: dahn <daan.hoogland@gmail.com>

GabrielBrascher reviewed May 13, 2021

View reviewed changes

replace 0.0 with NumberUtils

0baad4a

DaanHoogland approved these changes Jun 18, 2021

View reviewed changes

yadvr requested review from harikrishna-patnala and nvazquez June 29, 2021 07:16

davidjumani reviewed Jul 30, 2021

View reviewed changes

...ns/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java Show resolved Hide resolved

Update plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/…

9ec6d71

…vmware/resource/VmwareResource.java Co-authored-by: davidjumani <dj.davidjumani1994@gmail.com>

nvazquez reviewed Aug 2, 2021

View reviewed changes

nvazquez closed this Aug 24, 2021

nvazquez reopened this Aug 24, 2021

nvazquez approved these changes Aug 24, 2021

View reviewed changes

nvazquez merged commit 9163013 into apache:main Aug 24, 2021

Conversation

DK101010 commented Feb 8, 2021

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Feb 12, 2021

Uh oh!

blueorangutan commented Feb 12, 2021

Uh oh!

blueorangutan commented Feb 12, 2021

Uh oh!

DaanHoogland commented Feb 13, 2021

Uh oh!

blueorangutan commented Feb 13, 2021

Uh oh!

blueorangutan commented Feb 13, 2021

Uh oh!

DaanHoogland commented Feb 13, 2021

Uh oh!

blueorangutan commented Feb 13, 2021

Uh oh!

blueorangutan commented Feb 13, 2021

Uh oh!

Uh oh!

blueorangutan commented May 10, 2021

Uh oh!

blueorangutan commented May 11, 2021

Uh oh!

DaanHoogland commented May 11, 2021

Uh oh!

GabrielBrascher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jun 11, 2021

Uh oh!

blueorangutan commented Jun 14, 2021

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

nvazquez commented Jul 29, 2021

Uh oh!

blueorangutan commented Jul 29, 2021

Uh oh!

Uh oh!

blueorangutan commented Jul 30, 2021

Uh oh!

nvazquez commented Jul 30, 2021

Uh oh!

blueorangutan commented Jul 30, 2021

Uh oh!

nvazquez Aug 11, 2021 •

edited

Loading