[CLOUDSTACK-10128] Simplify the search for host to execute command

[rafaelweingartner]

@mike-tutkowski found a problem in #2315 when converting a snapshot to a template from a storage pool that has been deleted. Following his feedback in #2298 I am creating this PR to address the issue.

[rafaelweingartner]

@DaanHoogland, @rhtyd, and @mike-tutkowski can you review these changes?
I believe that the problem described by Mike in #2298 can be considered a blocker, and this PR is fixing that.

[yadvr]

@rafaelweingartner do you mean it's blocker for 4.11 or for #2298 , we're already past freeze now and we should not be accepting any feature PRs now.

[rafaelweingartner]

Hmm, that is a good question.
This affects only XenServers and the case that is broken is the following.

If you have a snapshot from a storage pool that has been deleted, you will not be able to create a volume or template from it.

It feels like a critical problem, but I am not sure now if it is a blocker or not.
Do we have a place where the concept of blocker is described?

[yadvr]

@rafaelweingartner I think you've answered yourself. Wrt a release, I would consider blocker to be something that would literally stop some part or the whole of CloudStack to be working (installation, upgrade failures included). We can always fix rest of the bugs as part of a minor (for example the next, 4.11.1.0) release.
@blueorangutan package

[blueorangutan]

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

[rafaelweingartner]

Ah ok. Thanks for the feedback.
I will leave the PR open, so we can work on it after 4.11

[blueorangutan]

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

[mike-tutkowski]

This should definitely at least go into a maintenance release for 4.11.

I can test this out soon.

Thanks, @rafaelweingartner!

[rafaelweingartner]

@mike-tutkowski Welcome!
I should thank you for the help ;)

Travis is failing, but I have not yet figured out why. It looks like a timeout thing.

[mike-tutkowski]

@rafaelweingartner I have the same problem with Travis on my PR.

[yadvr]

@rafaelweingartner can you fix build issues, I'll rekick anyway.
@blueorangutan package

[blueorangutan]

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

[rafaelweingartner]

@rhtyd the build issues were already fixed. You started your packing with my first commit here, and there was a build issue at that time, but not anymore

[blueorangutan]

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

[yadvr]

@rafaelweingartner okay, I'll kick tests.
@blueorangutan test centos7 xenserver-65sp1

[blueorangutan]

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

[yadvr]

@rafaelweingartner travis test LGTM, I've fixed one of the intermittent failing issues here - #2403

[blueorangutan]

Trillian test result (tid-2120)
Environment: xenserver-65sp1 (x2), Advanced Networking with Mgmt server 7
Total time taken: 26520 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2402-t2120-xenserver-65sp1.zip
Intermitten failure detected: /marvin/tests/smoke/test_templates.py
Intermitten failure detected: /marvin/tests/smoke/test_volumes.py
Smoke tests completed. 65 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_02_edit_template	Failure	90.12	test_templates.py
test_07_resize_fail	Failure	30.79	test_volumes.py

[yadvr]

Test LGTM.

[yadvr]

@rafaelweingartner@mike-tutkowski /cc @DaanHoogland and others - can you comment if this should be considered for 4.11.0.0, is this blocker/critical from your perspective if we merge/accept #2298 ?

[DaanHoogland]

is this fixing something that used to work before? Is the server not starting?
I think not, so I think we should not be considering this.

[rafaelweingartner]

Hmm..I am inclined to follow Daan's opinion on this one. It is a bug for sure, but it is a very specific one that we can easily fix later (if someone reports it). Anyways, it is going into 4.12... :)

[mike-tutkowski]

@rhtyd @rafaelweingartner@DaanHoogland I don't think it's really a blocker. Perhaps we can get it into the first maintenance release for 4.11.

It causes two problems:

1. For non-managed storage: If you create a volume snapshot from, say, storage pool 1 and later delete storage pool 1, then you can no longer use the volume snapshot (because we can't find an applicable host for the copy operation with this new logic).

2. For managed storage: The SAN-assisted volume snapshot use case fails. This only applies to XenServer and only when locationtype=secondary is passed into createSnapshot (that parameter is only used for managed storage and primary is the default value for that parameter). As far as I know, only one company currently uses this API this way. This company also is accustomed to patching CloudStack and using those modified versions in production, so I don't think we need to hold this particular release up because they can use the 4.11 release with this PR in their production environment (even before it "officially" comes out in a maintenance release).

[mike-tutkowski]

@rhtyd @rafaelweingartner@DaanHoogland If we are OK with waiting until the first 4.11 maintenance release for this PR, then I will hold off on testing this PR until we have 4.11 out the door.

[yadvr]

Okay @mike-tutkowski@rafaelweingartner@DaanHoogland I've moved this to 4.11.1 milestone.

[mike-tutkowski]

The managed-storage test for 4.11 RC1 that failed and led to the creation of the JIRA ticket that this PR addresses now passes:

test_04_create_volume_snapshot_using_sf_snapshot_and_archiving (TestSnapshots.TestSnapshots) ... === TestName: test_04_create_volume_snapshot_using_sf_snapshot_and_archiving | Status : SUCCESS ===
ok

---

Ran 1 test in 1931.664s

OK

[mike-tutkowski]

I have also tested the other failed use case that led to this PR: Take a volume snapshot of a volume. Delete the volume and the primary storage the volume was on. Create a new volume from the volume snapshot. The test is now successful.

[mike-tutkowski]

The code LGTM. Also, as noted above, I tested both failed use cases that led to this PR and they both work now.

[rafaelweingartner]

@rhtyd I noticed that you changed the base branch for this PR, from master to 4.11. Do you want me to open a new PR against master? Given that you might run into trouble when merging forward.

[DaanHoogland]

@rafaelweingartner no extra PR please, we will merge forward 4.11 after merge of any fixes

[rafaelweingartner]

reviewers (@DaanHoogland, @mike-tutkowski, @rhtyd ) what is your positions here? It seems that the tests passed and we have a consensus. If you are ok with this one, I would like to proceed and merge this to 4.11 and then forward to 4.12 (master branch)

[mike-tutkowski]

Yes, per my prior comments, the code LGTM and the two tests that previously failed now pass.

[rafaelweingartner]

@mike-tutkowski thanks!
I am getting used to look directly to the "reviweres" section on Github and checking the approved ones. I did not seen any for this PR, that is why I asked again.

[mike-tutkowski]

Sounds good - we should all get used to marking that field. On Feb 19, 2018, at 12:46 PM, Rafael Weingärtner <notifications@github.com<mailto:notifications@github.com>> wrote: @mike-tutkowski<https://github.com/mike-tutkowski> thanks! I am getting used to look directly to the "reviweres" section on Github and checking the approved ones. I did not seen any for this PR, that is why I asked again. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#2402 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AC4SHxDTguzYeKeTROs1yhRE0tmZFpSMks5tWd2pgaJpZM4RasNN>.

[rafaelweingartner]

Since everything is ok, I will merge this one.

Let's see if I still know how to do it :)