Fix hardcoded max data volumes when VM has been created but not started before by nvazquez · Pull Request #3494 · apache/cloudstack

nvazquez · 2019-07-15T18:38:54Z

Description

When VM is created but not started on any host, attaching a volume on it causes the maximum number of allowed volumes to be 6 (hardcoded)

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?

Deploy VM with startvm=false parameter
Attach more than 6 volumes to VM (in case of KVM and VMware)

nvazquez · 2019-07-15T22:12:38Z

Ping @anuragaw @shwstppr can you please review?

yadvr · 2019-07-16T07:05:15Z

server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java

+                    HypervisorType.KVM, HypervisorType.Ovm, HypervisorType.LXC);
+            HypervisorType hypervisorType = vm.getHypervisorType();
+            if (hypervisorType != null && supportingDefaultHV.contains(hypervisorType)) {
+                maxDataVolumesSupported = _hypervisorCapabilitiesDao.getMaxDataVolumesLimit(hypervisorType, "default");


Could version specific values cause regression or bug?

if these are lower the "default", yes @rhtyd. But that should never be the case, should it?

Good point, I've verified any version contains max limit lower than the default for each hypervisor

DaanHoogland · 2019-07-16T07:41:05Z

server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java

+                    HypervisorType.KVM, HypervisorType.Ovm, HypervisorType.LXC);
+            HypervisorType hypervisorType = vm.getHypervisorType();
+            if (hypervisorType != null && supportingDefaultHV.contains(hypervisorType)) {
+                maxDataVolumesSupported = _hypervisorCapabilitiesDao.getMaxDataVolumesLimit(hypervisorType, "default");


if these are lower the "default", yes @rhtyd. But that should never be the case, should it?

DaanHoogland · 2019-07-16T07:42:52Z

hudson couldn't reach github :(

DaanHoogland · 2019-07-16T07:43:14Z

try again

shwstppr

LGTM. Tested locally with KVM. Able to attach upto 32 volumes

nvazquez · 2019-07-16T13:51:48Z

@blueorangutan package

blueorangutan · 2019-07-16T13:52:37Z

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

blueorangutan · 2019-07-16T14:00:25Z

Packaging result: ✖centos6 ✖centos7 ✖debian. JID-137

nvazquez · 2019-07-16T14:20:09Z

@blueorangutan package

blueorangutan · 2019-07-16T14:20:37Z

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

blueorangutan · 2019-07-16T14:40:31Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-138

anuragaw

LGTM in general but a minor questions.

anuragaw · 2019-07-16T14:45:18Z

engine/schema/src/main/java/com/cloud/hypervisor/dao/HypervisorCapabilitiesDaoImpl.java

    }
+
+    @Override
+    public List<HypervisorType> getHypervisorsWithDefaultEntries() {


Is there a possibility of high frequency calls to this method? If so we can perhaps optimize query calls with some caching? If not LGTM

Thanks @anuragaw, have refactored it to invoke this only once

nvazquez · 2019-07-16T18:10:27Z

@blueorangutan package

blueorangutan · 2019-07-16T18:10:38Z

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

blueorangutan · 2019-07-16T18:30:38Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-139

nvazquez · 2019-07-16T19:02:22Z

@blueorangutan test matrix

blueorangutan · 2019-07-16T19:02:37Z

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

anuragaw

LGTM based on code and manual testing.

blueorangutan · 2019-07-17T04:49:21Z

Trillian test result (tid-167)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33160 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3494-t167-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 76 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_04_rvpc_network_garbage_collector_nics	`Failure`	297.88	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Failure`	422.28	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Error`	448.88	test_vpc_redundant.py

borisstoyanov

LGTM

blueorangutan · 2019-07-17T06:36:09Z

Trillian test result (tid-168)
Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
Total time taken: 39476 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3494-t168-vmware-65u2.zip
Intermittent failure detected: /marvin/tests/smoke/test_network.py
Intermittent failure detected: /marvin/tests/smoke/test_routers.py
Intermittent failure detected: /marvin/tests/smoke/test_snapshots.py
Intermittent failure detected: /marvin/tests/smoke/test_usage.py
Smoke tests completed. 73 look OK, 4 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_reboot_router	`Error`	325.67	test_network.py
test_08_start_router	`Error`	75.22	test_routers.py
test_09_reboot_router	`Error`	1.15	test_routers.py
test_02_list_snapshots_with_removed_data_store	`Error`	1.21	test_snapshots.py
test_01_volume_usage	`Error`	10.51	test_usage.py

yadvr · 2019-07-22T12:02:17Z

Tests LGTM, they are failing irrespective of this PR.

nvazquez added 2 commits July 15, 2019 15:32

Prevent NullPointerException when vm has not run before

c327286

Add hypervisor default max volumes if vm has not run before

5e2c301

nvazquez requested review from DaanHoogland, borisstoyanov and yadvr July 15, 2019 21:57

nvazquez self-assigned this Jul 15, 2019

nvazquez changed the title ~~Fix hardcoded max data volumes when VM has been created but not started before~~ [WIP DO NOT MERGE] Fix hardcoded max data volumes when VM has been created but not started before Jul 15, 2019

yadvr approved these changes Jul 16, 2019

View reviewed changes

DaanHoogland approved these changes Jul 16, 2019

View reviewed changes

DaanHoogland closed this Jul 16, 2019

DaanHoogland reopened this Jul 16, 2019

shwstppr approved these changes Jul 16, 2019

View reviewed changes

anuragaw approved these changes Jul 16, 2019

View reviewed changes

svenvogel added type:enhancement component:api labels Jul 16, 2019

nvazquez changed the title ~~[WIP DO NOT MERGE] Fix hardcoded max data volumes when VM has been created but not started before~~ Fix hardcoded max data volumes when VM has been created but not started before Jul 16, 2019

nvazquez added this to the 4.13.0.0 milestone Jul 16, 2019

nvazquez force-pushed the fixmaxdatavolumes branch from 07b38f0 to 435ad5f Compare July 16, 2019 13:49

Check database for default hypervisor entries instead of hardcoded

5fa201c

nvazquez force-pushed the fixmaxdatavolumes branch from 435ad5f to 5fa201c Compare July 16, 2019 13:51

Fix failing test

6a33826

anuragaw approved these changes Jul 16, 2019

View reviewed changes

Reduce number of invokations

dd1b028

anuragaw approved these changes Jul 17, 2019

View reviewed changes

borisstoyanov approved these changes Jul 17, 2019

View reviewed changes

nvazquez closed this Jul 17, 2019

nvazquez reopened this Jul 17, 2019

yadvr merged commit 38f97c6 into apache:master Jul 22, 2019

nvazquez deleted the fixmaxdatavolumes branch April 6, 2020 14:36

Conversation

nvazquez commented Jul 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

nvazquez commented Jul 15, 2019

Uh oh!

yadvr Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

nvazquez Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Jul 16, 2019

Uh oh!

DaanHoogland commented Jul 16, 2019

Uh oh!

shwstppr left a comment

Choose a reason for hiding this comment

Uh oh!

nvazquez commented Jul 16, 2019

Uh oh!

blueorangutan commented Jul 16, 2019

Uh oh!

blueorangutan commented Jul 16, 2019

Uh oh!

nvazquez commented Jul 16, 2019

Uh oh!

blueorangutan commented Jul 16, 2019

Uh oh!

blueorangutan commented Jul 16, 2019

Uh oh!

anuragaw left a comment

Choose a reason for hiding this comment

Uh oh!

anuragaw Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

nvazquez Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

nvazquez commented Jul 16, 2019

Uh oh!

blueorangutan commented Jul 16, 2019

Uh oh!

blueorangutan commented Jul 16, 2019

Uh oh!

nvazquez commented Jul 16, 2019

Uh oh!

blueorangutan commented Jul 16, 2019

Uh oh!

anuragaw left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jul 17, 2019

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jul 17, 2019

Uh oh!

yadvr commented Jul 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

nvazquez commented Jul 15, 2019 •

edited

Loading