[VMware] Support to attach more than 15 data disks in VMware VM by sureshanaparti · Pull Request #4172 · apache/cloudstack

sureshanaparti · 2020-06-24T13:39:48Z

Description

Support to attach more than 15 data disks in VMware VM

Fixes: #4102

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)

Screenshots (if appropriate):

How Has This Been Tested?

Create a new VM and attached 25 data disks to the VM.

[cloud]> select vm.instance_name as vm_name, count(*) as volumes_count from vm_instance vm join volumes vol on vm.id = vol.instance_id where vm.id = 4 group by vol.instance_id;
+----------+---------------+
| vm_name | volumes_count |
+----------+---------------+
| i-2-4-VM | 26 |
+----------+---------------+
1 row in set (0.00 sec)

nvazquez

Looking good, just minor comments

nvazquez · 2020-06-24T23:13:18Z

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

                    }
                } else {
-                    controllerKey = vmMo.getScsiDiskControllerKeyNoException(diskController);
+                    if (VmwareHelper.isReservedScsiDeviceNumber(scsiUnitNumber))


Minor one: can you please add braces to this if statement?

Minor one: can you please add braces to this if statement?

updated this

nvazquez · 2020-06-24T23:19:47Z

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

+
+                    int deviceNumber = -1;
+                    if (controllerKey == vmMo.getIDEControllerKey(ideUnitNumber)) {
+                        deviceNumber = (ideUnitNumber % VmwareHelper.MAX_ALLOWED_DEVICES_IDE_CONTROLLER);


Also minor: unnecessary parenthesis

Also minor: unnecessary parenthesis

updated

nvazquez · 2020-06-24T23:22:21Z

vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java

+        }
+
+        List<Integer> scsiDiskDevicesOnController = scsiDiskController.getDevice();
+        if (scsiDiskDevicesOnController != null && scsiDiskDevicesOnController.size() >= (VmwareHelper.MAX_SUPPORTED_DEVICES_SCSI_CONTROLLER)) {


Maybe also a null check for scsiDiskDevicesOnController?

Maybe also a null check for scsiDiskDevicesOnController?

updated

yadvr · 2020-06-25T11:57:00Z

@sureshanaparti can you kick packaging and tests yourself for PR?

sureshanaparti · 2020-07-01T04:56:17Z

@blueorangutan package

blueorangutan · 2020-07-01T04:56:47Z

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

blueorangutan · 2020-07-01T05:21:12Z

Packaging result: ✔centos7 ✔debian. JID-1495

sureshanaparti · 2020-07-01T05:41:32Z

@blueorangutan test centos7 vmware-67u3

blueorangutan · 2020-07-01T05:42:59Z

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

nvazquez

Thanks @sureshanaparti LGTM

borisstoyanov · 2020-07-03T11:03:29Z

@blueorangutan test centos7 vmware-67u3

blueorangutan · 2020-07-03T11:04:53Z

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

borisstoyanov

LGTM

blueorangutan · 2020-07-03T22:52:17Z

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

Test	Result	Time (s)	Test File
test_07_reboot_ssvm	`Failure`	42.31	test_ssvm.py

blueorangutan · 2020-07-05T04:35:31Z

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

Test	Result	Time (s)	Test File
test_07_reboot_ssvm	`Failure`	25.34	test_ssvm.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL	`Error`	98.53	test_vpc_redundant.py
test_02_redundant_VPC_default_routes	`Error`	0.01	test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers	`Error`	0.00	test_vpc_redundant.py
test_04_rvpc_network_garbage_collector_nics	`Error`	0.00	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Error`	0.00	test_vpc_redundant.py
ContextSuite context=TestVPCRedundancy>:teardown	`Error`	0.01	test_vpc_redundant.py

borisstoyanov · 2020-07-06T06:43:05Z

@sureshanaparti I guess we can merge this, the marvin failures does not seem related

sureshanaparti · 2020-07-06T13:10:25Z

@sureshanaparti I guess we can merge this, the marvin failures does not seem related

@borisstoyanov yes, these marvin failures are not related to the changes in this PR.

yadvr · 2020-07-08T05:53:15Z

@sureshanaparti this test is consistently failing, please check:

test_07_reboot_ssvm | Failure | 42.31 | test_ssvm.py

@blueorangutan package

blueorangutan · 2020-07-08T05:54:17Z

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

blueorangutan · 2020-07-08T06:29:43Z

Packaging result: ✔centos7 ✔debian. JID-1553

yadvr · 2020-07-08T08:27:15Z

@blueorangutan test centos7 vmware-67u3

blueorangutan · 2020-07-08T08:28:18Z

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

GabrielBrascher

Code LGTM

blueorangutan · 2020-07-09T20:45:39Z

Trillian test result (tid-2041)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 43378 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4172-t2041-vmware-67u3.zip
Intermittent failure detected: /marvin/tests/smoke/test_ssvm.py
Smoke tests completed. 82 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_07_reboot_ssvm	`Failure`	41.97	test_ssvm.py

yadvr · 2020-07-10T00:19:29Z

The test failure is pretty consistent, needs investigation.

yadvr · 2020-07-15T10:20:19Z

I reviewed and found test_07_reboot_ssvm has been failing due to other PRs merged before this PR, needs fixing nonetheless.

yadvr added this to the 4.15.0.0 milestone Jun 24, 2020

nvazquez reviewed Jun 24, 2020

View reviewed changes

[VMware] Support to attach more than 15 data disks in VMware VM

2e73ca0

sureshanaparti force-pushed the vmware-pick-available-scsi-controller-to-attach-disks branch from 0aad24a to 2e73ca0 Compare July 1, 2020 04:54

sureshanaparti requested a review from nvazquez July 1, 2020 04:55

nvazquez approved these changes Jul 2, 2020

View reviewed changes

sureshanaparti marked this pull request as draft July 2, 2020 12:57

borisstoyanov approved these changes Jul 3, 2020

View reviewed changes

yadvr marked this pull request as ready for review July 8, 2020 05:52

GabrielBrascher approved these changes Jul 8, 2020

View reviewed changes

yadvr changed the base branch from master to 4.14 July 15, 2020 10:20

yadvr changed the base branch from 4.14 to master July 15, 2020 10:21

yadvr merged commit f0a67cc into apache:master Jul 15, 2020

DaanHoogland added the type:enhancement label Oct 21, 2020

Conversation

sureshanaparti commented Jun 24, 2020

Description

Types of changes

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

nvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

nvazquez Jun 24, 2020

Choose a reason for hiding this comment

Uh oh!

sureshanaparti Jul 1, 2020

Choose a reason for hiding this comment

Uh oh!

nvazquez Jun 24, 2020

Choose a reason for hiding this comment

Uh oh!

sureshanaparti Jul 1, 2020

Choose a reason for hiding this comment

Uh oh!

nvazquez Jun 24, 2020

Choose a reason for hiding this comment

Uh oh!

sureshanaparti Jul 1, 2020

Choose a reason for hiding this comment

Uh oh!

yadvr commented Jun 25, 2020

Uh oh!

sureshanaparti commented Jul 1, 2020

Uh oh!

blueorangutan commented Jul 1, 2020

Uh oh!

blueorangutan commented Jul 1, 2020

Uh oh!

sureshanaparti commented Jul 1, 2020

Uh oh!

blueorangutan commented Jul 1, 2020

Uh oh!

nvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

borisstoyanov commented Jul 3, 2020

Uh oh!

blueorangutan commented Jul 3, 2020

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jul 3, 2020

Uh oh!

blueorangutan commented Jul 5, 2020

Uh oh!

borisstoyanov commented Jul 6, 2020

Uh oh!

sureshanaparti commented Jul 6, 2020

Uh oh!

yadvr commented Jul 8, 2020

Uh oh!

blueorangutan commented Jul 8, 2020

Uh oh!

blueorangutan commented Jul 8, 2020

Uh oh!

yadvr commented Jul 8, 2020

Uh oh!

blueorangutan commented Jul 8, 2020

Uh oh!

GabrielBrascher left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jul 9, 2020

Uh oh!

yadvr commented Jul 10, 2020

Uh oh!

yadvr commented Jul 15, 2020

Uh oh!

Reviewers

Assignees

Labels