api, vmware: Allow VM setting/detail for disk controller (root/data) to override template details #6276

Pearl1594 · 2022-04-18T07:20:15Z

Description

This PR allows VM details wrt root & data disk controller passed via details parameter of deployVMCmd to take precedence over template settings, i.e., the order of precedence for determining VM disk controller is : VM details passed during deployment > template settings > global setting vmware.root.disk.controller .

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?

Pearl1594 · 2022-04-18T07:25:35Z

@blueorangutan package

blueorangutan · 2022-04-18T07:26:03Z

@Pearl1594 a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2022-04-18T08:02:23Z

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

Pearl1594 · 2022-04-18T09:09:39Z

@blueorangutan test matrix

blueorangutan · 2022-04-18T09:10:03Z

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

blueorangutan · 2022-04-18T18:06:41Z

Trillian test result (tid-3926)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 30821 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6276-t3926-kvm-centos7.zip
Smoke tests completed. 93 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

blueorangutan · 2022-04-18T18:55:15Z

Trillian test result (tid-3927)
Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33915 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6276-t3927-vmware-65u2.zip
Smoke tests completed. 92 look OK, 1 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_deploy_vm_from_iso	`Error`	7.77	test_deploy_vm_iso.py

blueorangutan · 2022-04-18T19:11:29Z

Trillian test result (tid-3925)
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 34828 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6276-t3925-xenserver-71.zip
Smoke tests completed. 91 look OK, 2 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestListIdsParams>:teardown	`Error`	1.15	test_list_ids_parameter.py
test_02_cancel_host_maintenace_with_migration_jobs	`Error`	1990.88	test_host_maintenance.py
test_02_cancel_host_maintenace_with_migration_jobs	`Error`	1990.94	test_host_maintenance.py

nvazquez · 2022-04-20T02:37:50Z

@Pearl1594 is this ready for review?

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

harikrishna-patnala

LGTM

Pearl1594 · 2022-04-22T05:43:51Z

@blueorangutan package

blueorangutan · 2022-04-22T05:45:03Z

@Pearl1594 a Jenkins job has been kicked to build packages. It will be bundled with SystemVM template(s). I'll keep you posted as I make progress.

blueorangutan · 2022-04-22T06:32:28Z

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

Pearl1594 · 2022-04-22T06:36:30Z

@blueorangutan package

blueorangutan · 2022-04-22T06:37:02Z

@Pearl1594 a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2022-04-22T07:21:00Z

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

GutoVeronezi · 2022-04-22T22:30:50Z

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

+            if (vm.getDetail(VmDetailConstants.ROOT_DISK_CONTROLLER) == null) {
+                vm.setDetail(VmDetailConstants.ROOT_DISK_CONTROLLER, controllerSetting);
+            }


In UserVmVO we could create a method like setDetailIfNull. This way we can centralize the validation and reduce a bit of code.

I am not sure if it's worth doing this at this point, as it's not merely checking if it's null and setting it to some value.

rohityadavcloud · 2022-04-26T06:25:56Z

Is this essential for 4.17 @nvazquez @Pearl1594 ?

Pearl1594 · 2022-04-26T07:13:48Z

@rohityadavcloud this isn't essential for 4.17.0 as this has always been the behavior and there also exists a workaround, where users can change the template setting for root disk controller. So IMO this can go into 4.17.1

DaanHoogland · 2022-08-04T12:59:33Z

@blueorangutan test centos7 vmware-70u3

blueorangutan · 2022-08-04T13:00:03Z

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

blueorangutan · 2022-08-05T00:26:38Z

Trillian test result (tid-4621)
Environment: vmware-70u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 39638 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6276-t4621-vmware-70u3.zip
Smoke tests completed. 100 look OK, 1 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_deploy_vm_from_iso	`Error`	7.84	test_deploy_vm_iso.py

DaanHoogland · 2022-08-05T10:17:26Z

Test Result Time (s) Test File
test_deploy_vm_from_iso Error 7.84 test_deploy_vm_iso.py

@Pearl1594 , this remaining error seems to be consistent. Can you look at it?

shwstppr

@Pearl1594 failing vm from iso test is due to validateRootDiskResize should we call it only for templates and not iso?

… template details

shwstppr · 2022-08-18T08:38:34Z

@blueorangutan package

blueorangutan · 2022-08-18T08:39:02Z

@shwstppr a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2022-08-18T09:16:22Z

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

Pearl1594 · 2022-08-18T09:17:13Z

@blueorangutan test centos7 vmware-70u3

blueorangutan · 2022-08-18T09:18:03Z

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

sonarcloud · 2022-08-18T09:18:47Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

codecov · 2022-08-18T10:39:02Z

Codecov Report

Merging #6276 (59c5d0f) into 4.17 (010b47f) will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff            @@
##               4.17    #6276   +/-   ##
=========================================
  Coverage      5.86%    5.86%           
- Complexity     3918     3920    +2     
=========================================
  Files          2451     2451           
  Lines        242268   242277    +9     
  Branches      37910    37912    +2     
=========================================
+ Hits          14207    14211    +4     
- Misses       226491   226496    +5     
  Partials       1570     1570

Impacted Files	Coverage Δ
.../src/main/java/com/cloud/vm/UserVmManagerImpl.java	`0.00% <0.00%> (ø)`
...apache/cloudstack/syslog/AlertsSyslogAppender.java	`58.75% <0.00%> (+2.25%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

blueorangutan · 2022-08-18T20:51:28Z

Trillian test result (tid-4725)
Environment: vmware-70u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 40107 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6276-t4725-vmware-70u3.zip
Smoke tests completed. 101 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

shwstppr · 2022-08-19T05:00:14Z

@blueorangutan test matrix

blueorangutan · 2022-08-19T05:01:03Z

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

blueorangutan · 2022-08-19T15:57:04Z

Trillian test result (tid-4737)
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 38043 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6276-t4737-xenserver-71.zip
Smoke tests completed. 100 look OK, 1 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_08_upgrade_kubernetes_ha_cluster	`Failure`	623.84	test_kubernetes_clusters.py

blueorangutan · 2022-08-19T16:34:42Z

Trillian test result (tid-4739)
Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
Total time taken: 40362 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6276-t4739-vmware-65u2.zip
Smoke tests completed. 101 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

blueorangutan · 2022-08-19T16:53:20Z

Trillian test result (tid-4738)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 41359 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6276-t4738-kvm-centos7.zip
Smoke tests completed. 101 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

rohityadavcloud

LGTM, but didn't test it

harikrishna-patnala

Tested the VM root disk controller settings at different places.

With only global setting value
With global setting and template setting
With global, template and VM deployment
Stopped the VM, changed VM setting in the above cases and started the VM

I've checked the VM settings on vCenter, in all the above cases I observed the precedence in the order of
VM setting > Template setting > Global setting

…to override template details (apache#6276) This PR allows VM details wrt root & data disk controller passed via details parameter of deployVMCmd to take precedence over template settings, i.e., the order of precedence for determining VM disk controller is : VM details passed during deployment > template settings > global setting vmware.root.disk.controller.

Pearl1594 changed the title ~~api, vmware: Allow VM setting/detail for disk controller (root/data) override template details~~ api, vmware: Allow VM setting/detail for disk controller (root/data) to override template details Apr 18, 2022

Pearl1594 marked this pull request as ready for review April 20, 2022 02:41

Pearl1594 requested review from nvazquez and weizhouapache April 20, 2022 08:39

weizhouapache reviewed Apr 20, 2022

View reviewed changes

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java Show resolved Hide resolved

weizhouapache approved these changes Apr 20, 2022

View reviewed changes

harikrishna-patnala approved these changes Apr 20, 2022

View reviewed changes

Pearl1594 modified the milestone: 4.17.1.0 Apr 22, 2022

GutoVeronezi reviewed Apr 22, 2022

View reviewed changes

nvazquez added this to the 4.17.1.0 milestone Apr 23, 2022

nvazquez added component:vmware component:api labels Apr 23, 2022

shwstppr reviewed Aug 12, 2022

View reviewed changes

Pearl1594 added 2 commits August 18, 2022 12:42

api: Allow VM setting/detail for disk controller (root/data) override…

a5c4394

… template details

add iso check

59c5d0f

Pearl1594 force-pushed the consider-vm-setting-controller branch from a5d5379 to 59c5d0f Compare August 18, 2022 08:26

shwstppr approved these changes Aug 19, 2022

View reviewed changes

shwstppr requested review from harikrishna-patnala, sureshanaparti, weizhouapache and rohityadavcloud August 22, 2022 05:33

rohityadavcloud approved these changes Aug 22, 2022

View reviewed changes

harikrishna-patnala approved these changes Aug 22, 2022

View reviewed changes

shwstppr merged commit 9847918 into apache:4.17 Aug 22, 2022

api, vmware: Allow VM setting/detail for disk controller (root/data) to override template details #6276

api, vmware: Allow VM setting/detail for disk controller (root/data) to override template details #6276

Conversation

Pearl1594 commented Apr 18, 2022 • edited by shwstppr

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Pearl1594 commented Apr 18, 2022

blueorangutan commented Apr 18, 2022

blueorangutan commented Apr 18, 2022

Pearl1594 commented Apr 18, 2022

blueorangutan commented Apr 18, 2022

blueorangutan commented Apr 18, 2022

blueorangutan commented Apr 18, 2022

blueorangutan commented Apr 18, 2022

nvazquez commented Apr 20, 2022

harikrishna-patnala left a comment

Choose a reason for hiding this comment

Pearl1594 commented Apr 22, 2022

blueorangutan commented Apr 22, 2022

blueorangutan commented Apr 22, 2022

Pearl1594 commented Apr 22, 2022

blueorangutan commented Apr 22, 2022

blueorangutan commented Apr 22, 2022

GutoVeronezi Apr 22, 2022

Choose a reason for hiding this comment

Pearl1594 Aug 1, 2022

Choose a reason for hiding this comment

rohityadavcloud commented Apr 26, 2022

Pearl1594 commented Apr 26, 2022

DaanHoogland commented Aug 4, 2022

blueorangutan commented Aug 4, 2022

blueorangutan commented Aug 5, 2022

DaanHoogland commented Aug 5, 2022

shwstppr left a comment

Choose a reason for hiding this comment

shwstppr commented Aug 18, 2022

blueorangutan commented Aug 18, 2022

blueorangutan commented Aug 18, 2022

Pearl1594 commented Aug 18, 2022

blueorangutan commented Aug 18, 2022

sonarcloud bot commented Aug 18, 2022

codecov bot commented Aug 18, 2022 • edited

Codecov Report

blueorangutan commented Aug 18, 2022

shwstppr commented Aug 19, 2022

blueorangutan commented Aug 19, 2022

blueorangutan commented Aug 19, 2022

blueorangutan commented Aug 19, 2022

blueorangutan commented Aug 19, 2022

rohityadavcloud left a comment

Choose a reason for hiding this comment

harikrishna-patnala left a comment • edited

Choose a reason for hiding this comment

Pearl1594 commented Apr 18, 2022 •

edited by shwstppr

codecov bot commented Aug 18, 2022 •

edited

harikrishna-patnala left a comment •

edited