CLOUDSTACK-9473: storage pool capacity check when volume is resized or migrated by yadvr · Pull Request #2829 · apache/cloudstack

yadvr · 2018-08-30T08:22:24Z

This was previously opened as PR but prematurely closed:
#1931
#1932

Issue: https://issues.apache.org/jira/browse/CLOUDSTACK-9473

Storage pool checker is not being called on resize and migrate volume.
This may lead to allocated percentage of storage above 100%.

Setup:
1 VMware cluster with 2 Hosts.

Executed Steps:

Applied the following global settings:
storage.overprovisioning.factor = 1
pool.storage.allocated.capacity.disablethreshold = 1
pool.storage.capacity.disablethreshold = 1
Restarted management server
Executed Resize and migrate pool and Observed that Storage pool checker is not performed on resizeVolume and migrateVolume.
Result:
Root cause analysis shows storage pool checker is not called when doing migration and resizing.

Signed-off-by: Rohit Yadav rohit.yadav@shapeblue.com

Description

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)

GitHub Issue/PRs

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

I have read the CONTRIBUTING document.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Testing
I have added tests to cover my changes.
All relevant new and existing integration tests have passed.
A full integration testsuite with all test that can run on my environment has passed.

…r migrated Storage pool checker is not being called on resize and migrate volume. This may lead to allocated percentage of storage above 100%. Setup: 1 VMware cluster with 2 Hosts. Executed Steps: Applied the following global settings: storage.overprovisioning.factor = 1 pool.storage.allocated.capacity.disablethreshold = 1 pool.storage.capacity.disablethreshold = 1 Restarted management server Executed Resize and migrate pool and Observed that Storage pool checker is not performed on resizeVolume and migrateVolume. Result: Root cause analysis shows storage pool checker is not called when doing migration and resizing. Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

yadvr · 2018-08-30T08:36:13Z

@blueorangutan package

blueorangutan · 2018-08-30T08:36:39Z

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

blueorangutan · 2018-08-30T09:23:25Z

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

yadvr · 2018-08-30T09:23:51Z

@blueorangutan test matrix

blueorangutan · 2018-08-30T09:24:40Z

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

wido · 2018-08-30T09:44:51Z

+        if (!checkUsagedSpace(pool)) {
+            return false;
+        }
+        if (s_logger.isDebugEnabled()) {


Is this if really needed? Can't you just log to debug and let the filter of log4j handle it?

If log4j does not have debug enabled, then the string concat operation will still be performed by the JVM only when the string is passed to the debug() method it will be ignored. So, it depends if we can afford the CPU/time spent on the operation.

I think we can afford this tiny bit of extra CPU.

I think as a matter of principle we should alwas add the check. You never no for what reason someone will alter the message. I like using String.format to construct the message, btw, and then pass the single resulting string to the logger

What principle?

I also use String.format, and I do not see the problem.

"bla" + "blob" is a costly operation, basically to memcpy()s. String.format() as well in comparison. We don't want those operations performed if we don't end up using the result in a log statement. so the boolean check potentially saves us a lot of cpu cycles, especially in the debug() and trace() cases.

Yes, I do know that. However, we should ask ourselves, if the addition of a if conditions (it will clutter the code) is worth the CPU cycles. For me, I would not trade readability for this small CPU cost in a web system (ACS is a at the end of the day a Java web application).

rafaelweingartner · 2018-08-30T10:53:00Z

+        if (!checkUsagedSpace(pool)) {
+            return false;
+        }
+        if (s_logger.isDebugEnabled()) {


I think we can afford this tiny bit of extra CPU.

rafaelweingartner · 2018-08-30T10:54:33Z

-
-        s_logger.debug("Checking pool: " + pool.getId() + " for volume allocation " + volumes.toString() + ", maxSize : " + totalOverProvCapacity + ", totalAllocatedSize : "
-                + allocatedSizeWithTemplate + ", askingSize : " + totalAskingSize + ", allocated disable threshold: " + storageAllocatedThreshold);
+        if (s_logger.isDebugEnabled()) {


Why checking before logging?

We are removing this from other places. Let's remove conditionals from our code base.

please let's always check before logging any lower priority (below error)

I still see no real benefits of adding this log level checks in our context. On the other hand, I do see problems with readability of the code.

I cannot object though, if you want to proceed with this ;)

rafaelweingartner · 2018-08-30T10:55:51Z

        StoragePoolVO storagePool = _storagePoolDao.findById(volume.getPoolId());
        boolean isManaged = storagePool.isManaged();
+
+        List<Volume> volumes = new ArrayList<Volume>();


Are you using this variable?

don't think so and also these methods here are to huge for my puny brain

My brain only works with up to 10 lines of code methods. Anything else is just a blur.

I think the code is ok; we just need to check if this variable is used; if it is not, please remove it.

Removed, unused code.

DaanHoogland

I agree with the priciple and also the functional bits of code and the extra log-level checks look good. There are some bits (volumes list) that I don't understand/are not needed and a full integration test awaiting 👍

blueorangutan · 2018-08-30T18:52:19Z

Trillian test result (tid-2975)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 27836 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2829-t2975-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_affinity_groups_projects.py
Intermittent failure detected: /marvin/tests/smoke/test_affinity_groups.py
Intermittent failure detected: /marvin/tests/smoke/test_deploy_virtio_scsi_vm.py
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 64 look OK, 3 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_DeployVmAntiAffinityGroup_in_project	`Error`	38.33	test_affinity_groups_projects.py
test_DeployVmAntiAffinityGroup	`Error`	53.52	test_affinity_groups.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	1213.67	test_privategw_acl.py

blueorangutan · 2018-08-30T18:54:03Z

Trillian test result (tid-2970)
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31650 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2829-t2970-xenserver-71.zip
Intermittent failure detected: /marvin/tests/smoke/test_deploy_virtio_scsi_vm.py
Intermittent failure detected: /marvin/tests/smoke/test_scale_vm.py
Smoke tests completed. 65 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestDeployVirtioSCSIVM>:teardown	`Error`	0.00	test_deploy_virtio_scsi_vm.py
test_01_scale_vm	`Error`	19.43	test_scale_vm.py

rafaelweingartner · 2018-09-01T13:38:40Z

        StoragePoolVO storagePool = _storagePoolDao.findById(volume.getPoolId());
        boolean isManaged = storagePool.isManaged();
+
+        List<Volume> volumes = new ArrayList<Volume>();


My brain only works with up to 10 lines of code methods. Anything else is just a blur.

I think the code is ok; we just need to check if this variable is used; if it is not, please remove it.

borisstoyanov

LGTM

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

yadvr · 2018-09-04T08:21:28Z

Packaging failed due to backend error, rekicking.
@blueorangutan package

blueorangutan · 2018-09-04T08:22:03Z

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

blueorangutan · 2018-09-04T09:02:09Z

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

blueorangutan · 2018-09-04T09:33:35Z

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

blueorangutan · 2018-09-04T10:00:24Z

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-2286

yadvr · 2018-09-04T11:15:28Z

@blueorangutan test

blueorangutan · 2018-09-04T11:16:03Z

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

yadvr · 2018-09-05T07:54:24Z

@blueorangutan test

blueorangutan · 2018-09-05T07:55:08Z

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

blueorangutan · 2018-09-07T15:56:08Z

Trillian test result (tid-2987)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 23411 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2829-t2987-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 65 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	1134.77	test_privategw_acl.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	3.50	test_hostha_kvm.py

yadvr · 2018-09-07T16:31:25Z

Test lgtm

…louds' Normalização dos valores das configurações globais relacionadas à deleção em _batch_ de registros do banco de dados Closes apache#2829 See merge request scclouds/scclouds!1226

yadvr added this to the 4.11.2.0 milestone Aug 30, 2018

yadvr requested review from DaanHoogland and rafaelweingartner and removed request for rafaelweingartner August 30, 2018 08:22

yadvr force-pushed the capacity-calculate-for-migrate-resize branch from 4860e33 to c2a3410 Compare August 30, 2018 08:34

yadvr added the type:bug label Aug 30, 2018

apache deleted a comment from blueorangutan Aug 30, 2018

wido reviewed Aug 30, 2018

View reviewed changes

rafaelweingartner reviewed Aug 30, 2018

View reviewed changes

DaanHoogland approved these changes Aug 30, 2018

View reviewed changes

rafaelweingartner approved these changes Sep 1, 2018

View reviewed changes

borisstoyanov approved these changes Sep 3, 2018

View reviewed changes

fix review comments

3921420

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

apache deleted a comment from blueorangutan Sep 4, 2018

yadvr mentioned this pull request Sep 4, 2018

Fix SystemVMs running in Xen HVM mode are not configured (#2760) #2824

Merged

12 tasks

yadvr merged commit 2ab3976 into apache:4.11 Sep 7, 2018

Conversation

yadvr commented Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

GitHub Issue/PRs

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

Uh oh!

yadvr commented Aug 30, 2018

Uh oh!

blueorangutan commented Aug 30, 2018

Uh oh!

blueorangutan commented Aug 30, 2018

Uh oh!

yadvr commented Aug 30, 2018

Uh oh!

blueorangutan commented Aug 30, 2018

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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Aug 30, 2018

Uh oh!

blueorangutan commented Aug 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Sep 4, 2018

Uh oh!

blueorangutan commented Sep 4, 2018

Uh oh!

blueorangutan commented Sep 4, 2018

Uh oh!

blueorangutan commented Sep 4, 2018

Uh oh!

blueorangutan commented Sep 4, 2018

Uh oh!

yadvr commented Sep 4, 2018

Uh oh!

blueorangutan commented Sep 4, 2018

Uh oh!

yadvr commented Sep 5, 2018

Uh oh!

blueorangutan commented Sep 5, 2018

yadvr commented Aug 30, 2018 •

edited

Loading