Only use the host if its Resource State is Enabled. by mike-tutkowski · Pull Request #2512 · apache/cloudstack

mike-tutkowski · 2018-03-26T20:33:08Z

Description

Update the StorageSystemSnapshotStrategy and StorageSystemDataMotionStrategy classes to only leverage a host when its Resource State is Enabled.

https://issues.apache.org/jira/browse/CLOUDSTACK-10345

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)

How Has This Been Tested?

Checked to see if a host in maintenance mode is avoided

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.
I have added tests to cover my changes.
All new and existing tests passed.

@blueorangutan package

blueorangutan · 2018-03-26T20:33:20Z

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

nitin-maharana · 2018-03-26T20:50:24Z

...shot/src/main/java/org/apache/cloudstack/storage/snapshot/StorageSystemSnapshotStrategy.java

                vmSnapshot = takeHypervisorSnapshot(volumeInfo);
            }
-            catch (ResourceAllocationException ex) {
+            catch (Exception ex) {


@mike-tutkowski, Thanks for the fix. I think it would be better to capture the required exceptions with multiple catches instead of the parent Exception class. We can always put this at the end.

nitin-maharana · 2018-03-26T21:04:03Z

...shot/src/main/java/org/apache/cloudstack/storage/snapshot/StorageSystemSnapshotStrategy.java


        SnapshotResult result = null;
-        SnapshotInfo snapshotOnPrimary = null;
+        SnapshotInfo snapshotOnPrimary;


I guess the removal of this may create NPE.

how is this, @nitin-maharana?

This is an IntelliJ-recommended change. The IDE pointed out (correctly) that we did not need to assign null on this line because snapshotOnPrimary is later assigned a value regardless of the path the code takes.

@DaanHoogland, My bad, wrong interpretation!! As @mike-tutkowski explained, that's redundant. Thanks!!

blueorangutan · 2018-03-26T21:07:59Z

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

yadvr

LGTM

yadvr · 2018-03-27T11:32:18Z

@mike-tutkowski bugfixes can be considered for 4.11 branch, if you think this is useful for 4.11/tls please change the base-branch to 4.11 (and rebase your branch against 4.11). Please also put this PR against a milestone, 4.11.1.0, or 4.12.0.0 as applicable.

mike-tutkowski · 2018-03-27T17:33:32Z

@rhtyd Sounds good, Rohit. I can change the base branch to 4.11 and rebase. I'll do that after I implement @nitin-maharana's suggestion around exception handling.

mike-tutkowski · 2018-03-27T17:49:06Z

@rhtyd @nitin-maharana @DaanHoogland I have rebased the code on top of 4.11 and changed the base branch of the PR. Also, @nitin-maharana: I was able to entirely remove that try/catch block.

DaanHoogland

lgtm

DaanHoogland · 2018-03-28T11:09:30Z

@blueorangutan package

blueorangutan · 2018-03-28T11:10:27Z

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

blueorangutan · 2018-03-28T11:38:50Z

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

DaanHoogland · 2018-03-28T11:56:09Z

@blueorangutan test

blueorangutan · 2018-03-28T11:56:28Z

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

blueorangutan · 2018-03-29T13:59:38Z

Trillian test result (tid-2435)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 92219 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2512-t2435-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_routers.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Smoke tests completed. 64 look OK, 3 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_02_vpc_privategw_static_routes	`Failure`	150.61	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	150.58	test_privategw_acl.py
test_04_rvpc_privategw_static_routes	`Failure`	272.80	test_privategw_acl.py
test_04_restart_network_wo_cleanup	`Failure`	2.79	test_routers.py
test_01_vpc_site2site_vpn	`Failure`	220.59	test_vpc_vpn.py

mike-tutkowski · 2018-03-29T17:41:28Z

Just a quick comment on the five test failures: It seems unlikely that this PR caused them. Those are all networking related and this PR is mainly concerned with managed storage.

blueorangutan · 2018-03-29T17:43:33Z

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

mike-tutkowski · 2018-03-29T17:46:09Z

@DaanHoogland Quick question: Is it part of our process to forward merge these PRs to master or do I need to open a new PR for that? Thanks!

rafaelweingartner · 2018-03-29T17:48:43Z

I was going to mention that just now. No new PR should be required. We should merge forward.
There is also another PR that @DaanHoogland merged yesterday that has not been merged forward.

DaanHoogland · 2018-03-29T17:50:03Z

I think we are at more than ten now :s , gents

DaanHoogland · 2018-03-29T17:51:14Z

but I think it doesn't hurt to gather a few before we merge forward.

rafaelweingartner · 2018-03-29T17:52:37Z

Really?! I normally follow PRs merge, but I only noticed the one yesterday and this one.
Well, I guest that as long as we merge them forward, it is ok to do the merges in batch.

blueorangutan · 2018-03-29T18:09:44Z

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

This reverts commit e68f5ce.

nitin-maharana reviewed Mar 26, 2018

View reviewed changes

yadvr approved these changes Mar 27, 2018

View reviewed changes

Only use the host if its Resource State is Enabled.

041a0eb

mike-tutkowski force-pushed the check-maint-mode branch from 78de206 to 041a0eb Compare March 27, 2018 17:45

mike-tutkowski changed the base branch from master to 4.11 March 27, 2018 17:46

yadvr added this to the 4.11.1.0 milestone Mar 28, 2018

DaanHoogland approved these changes Mar 28, 2018

View reviewed changes

DaanHoogland merged commit e68f5ce into apache:4.11 Mar 29, 2018

rafaelweingartner added a commit that referenced this pull request Mar 29, 2018

Revert "Only use the host if its Resource State is Enabled. (#2512)"

7cf95de

This reverts commit e68f5ce.

mike-tutkowski deleted the check-maint-mode branch April 12, 2018 20:50

Conversation

mike-tutkowski commented Mar 26, 2018

Description

Types of changes

How Has This Been Tested?

Checklist:

Uh oh!

blueorangutan commented Mar 26, 2018

Uh oh!

nitin-maharana Mar 26, 2018

Choose a reason for hiding this comment

Uh oh!

nitin-maharana Mar 26, 2018

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Mar 27, 2018

Choose a reason for hiding this comment

Uh oh!

mike-tutkowski Mar 27, 2018

Choose a reason for hiding this comment

Uh oh!

nitin-maharana Mar 28, 2018

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Mar 26, 2018

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Mar 27, 2018

Uh oh!

mike-tutkowski commented Mar 27, 2018

Uh oh!

mike-tutkowski commented Mar 27, 2018

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Mar 28, 2018

Uh oh!

blueorangutan commented Mar 28, 2018

Uh oh!

blueorangutan commented Mar 28, 2018

Uh oh!

DaanHoogland commented Mar 28, 2018

Uh oh!

blueorangutan commented Mar 28, 2018

Uh oh!

blueorangutan commented Mar 29, 2018

Uh oh!

mike-tutkowski commented Mar 29, 2018

Uh oh!

blueorangutan commented Mar 29, 2018

Uh oh!

mike-tutkowski commented Mar 29, 2018

Uh oh!

rafaelweingartner commented Mar 29, 2018

Uh oh!

DaanHoogland commented Mar 29, 2018

Uh oh!

DaanHoogland commented Mar 29, 2018

Uh oh!

rafaelweingartner commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blueorangutan commented Mar 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rafaelweingartner commented Mar 29, 2018 •

edited

Loading