Have HyperV behave in 4.4 and return null instead of false

[remibergsma]

Commit 6a4927f made the HyperV investigator return false instead of null.

Returning false means the VM is NOT running, returning null means "I don't know". In 4.4 I experienced corruption because of HyperV returning false, instead of null.

Tonight I experienced corruption when one of our management servers went down (out-of-memory, not root caused yet). While all hypervisors that were connected to this management server were connecting the other, HA work started as well with investigators. HyperV happily reported everything as down (while it was still running), causing a mess.

In 4.5 and master this was already fixed. If you know a better way to fix this, please let me know!
This may cause another FindBugs alert, not sure how to resolve that. I just want this out ASAP. Maybe @DaanHoogland@wilderrodrigues or @miguelaferreira can advise on how to fix this properly.

[asfbot]

cloudstack-pull-rats #444 FAILURE
Looks like there's a problem with this pull request

[asfbot]

cloudstack-pull-analysis #377 SUCCESS
This pull request looks good

[miguelaferreira]

@remi: passing null to a method or returning null from a method is always a bad choice. There are better ways to express "I don't know" (e.g. Using Optional in this case).

I understand the intention of your change, but it addresses the symptom, not the problem. The problem is most likely in how the return value is handled elseweare. Please reconsider this change. If it's a blocker we can work on fixing structurally ASAP.

[remibergsma]

@miguelaferreira Thanks. I guess returning null is done in many of the Investigators right now, but would be great to improve it. If we at least could make it the same in 4.4 like in master that'd be great because that works (https://github.com/apache/cloudstack/blob/master/plugins/hypervisors/hyperv/src/com/cloud/ha/HypervInvestigator.java) and that throws throw new UnknownVM();

The OVM3 Investigator also seems smarter (https://github.com/apache/cloudstack/blob/master/plugins/hypervisors/ovm3/src/main/java/com/cloud/ha/Ovm3Investigator.java).

I could live with a quick hotfix for 4.4 that I can deploy soon, and then improving this whole thing in master. Not sure what is possible right now. We'll discuss tomorrow :-)

[koushik-das]

Changes LGTM for 4.4.
@miguelaferreira@remibergsma These can be fixed properly in a future release. isVmAlive() can return some enum (like yes, no, unknown) similar to isAgentAlive().

[miguelaferreira]

I didn't realise this was a hotfix for 4.4.
I wouldn't want this fix for master, but for a previous release that has a critical bug, I can live with it.

[miguelaferreira]

The rats build failed because some files (mostly travis related) do not have license headers.
These files have not been changed in this PR.

 .travis.yml
engine/schema/src/org/apache/cloudstack/resourcedetail/dao/SnapshotPolicyDetailsDaoImpl.java
engine/schema/src/org/apache/cloudstack/resourcedetail/dao/SnapshotPolicyDetailsDao.java
engine/schema/src/org/apache/cloudstack/resourcedetail/SnapshotPolicyDetailVO.java
api/src/org/apache/cloudstack/api/command/admin/iso/CopyIsoCmdByAdmin.java
debian/source/format
tools/travis/after_success.sh
tools/travis/before_install.sh
tools/travis/install.sh
tools/travis/script.sh
tools/travis/after_failure.sh
tools/travis/before_script.sh
tools/travis/xunit-reader.py
tools/travis/after_script.sh

[remibergsma]

Travis error seems not related. I can force push to see if it works this time. At least the Apache pull-analysis build succeeded.

[asfbot]

cloudstack-pull-rats #450 FAILURE
Looks like there's a problem with this pull request

[remibergsma]

Seems Travis is broken for 4.4 :-s

[wilderrodrigues]

It has been partially fixed on 4.6, but still the code can be improved since it might return null. Below an snippet of the 4.6 code:

@OverRide
public boolean isVmAlive(com.cloud.vm.VirtualMachine vm, Host host) throws UnknownVM {
Status status = isAgentAlive(host);
if (status == null) {
throw new UnknownVM();
}
return status == Status.Up ? true : null;
}

As you can see, they now throw an Exception in canse the status is null. However, in the return it might return null. In addition, the return type is no longer a wrapper class, but a native boolean. Returning null would be very bad.

Could you perhaps make an amalgama of the fix, @remibergsma ? You can then push it towards 4.4 and 4.6

Cheers,
Wilder

[wilderrodrigues]

Just discussed with @remibergsma and went over the old (4.4) code, the change made by @rajesh-battala - which broke the behaviour - and how 4.6 now is. The change LGTM concerning the hotfix on 4.4.

In terms of future proof fix, would be nice to have a massive tests on all the investigators and get rid of those things returning null instead of a boolean.

Cheers,
Wilder

[asfbot]

cloudstack-pull-analysis #383 SUCCESS
This pull request looks good

[wilderrodrigues]

@remibergsma

I merged the PR following the new merge style you and @miguelaferreira did. I dunno why it's still not updated. Let's wait a bit more.

commit b0a4593
Merge: 3b3d2fc03929d2
Author: Wilder Rodrigues wilder.rodrigues@ekholabs.nl
Date: Tue Sep 1 11:25:20 2015 +0000

Merge pull request #761 from remibergsma/fix-hyperv-returns

Cheers,
Wilder

[yadvr]

Anyone github bug? why is @hubot here?

[remibergsma]

Created issue for it: CLOUDSTACK-8811