[GitHub] cloudstack pull request: return a state instead of null in Abstrac...

2015-05-01 Thread bhaisaab
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/211#issuecomment-98143582 @dahn you're right, I looked at the code and made some semantic changes around debugging/logging and return value type. I guess there are some parts in the source

Re: [GitHub] cloudstack pull request: return a state instead of null in Abstrac...

2015-05-01 Thread Daan Hoogland
I finally got around a further code analysis and must conclude that returning ALERT is no problem. Status testIpAddress(Long hostId, String testHostIp), is called twice and in both cases null is checked for avoiding NPE and next only the cases UP and DOWN are handled. So the behaviour on a null

[GitHub] cloudstack pull request: return a state instead of null in Abstrac...

2015-05-01 Thread abhinandanprateek
Github user abhinandanprateek commented on the pull request: https://github.com/apache/cloudstack/pull/211#issuecomment-98148840 Another thing to note is that while changing some of this legacy understanding, it is not just the code that needs improvement/change, it is all the

[GitHub] cloudstack pull request: return a state instead of null in Abstrac...

2015-05-01 Thread abhinandanprateek
Github user abhinandanprateek commented on the pull request: https://github.com/apache/cloudstack/pull/211#issuecomment-98091401 I agree with Koushik here. The purpose of an HA investigator is to either say that host is UP or Down or it does not know for sure. A host that has not

[GitHub] cloudstack pull request: return a state instead of null in Abstrac...

2015-05-01 Thread abhinandanprateek
Github user abhinandanprateek commented on the pull request: https://github.com/apache/cloudstack/pull/211#issuecomment-98093700 Daan, Yes, I agree. We can define a proper state and introduce it. I guess there is some tribal knowledge involved here that Koushik and myself were

[GitHub] cloudstack pull request: return a state instead of null in Abstrac...

2015-05-01 Thread DaanHoogland
Github user DaanHoogland commented on the pull request: https://github.com/apache/cloudstack/pull/211#issuecomment-98091566 Abhinandan, I think that is a design flow and we should revisit. I doubt that it is relevant to this fix however. The stjatemachine was resetting the

[GitHub] cloudstack pull request: return a state instead of null in Abstrac...

2015-05-01 Thread koushik-das
Github user koushik-das commented on the pull request: https://github.com/apache/cloudstack/pull/211#issuecomment-98100569 One possible solution I can think of the original problem for this PR is to wait for sometime (say based on the config parameter alert.wait) and if even after

[GitHub] cloudstack pull request: return a state instead of null in Abstrac...

2015-05-01 Thread remibergsma
Github user remibergsma commented on the pull request: https://github.com/apache/cloudstack/pull/211#issuecomment-98104693 @koushik-das Since CloudStack 4.4 XenHA is the one to elect the new pool master. Before, CloudStack would force an election. Personally I do not like this new

[GitHub] cloudstack pull request: return a state instead of null in Abstrac...

2015-05-01 Thread koushik-das
Github user koushik-das commented on the pull request: https://github.com/apache/cloudstack/pull/211#issuecomment-98100363 I looked at the code and saw that if investigate() is not able to determine the state of host then by default Alert is returned, earlier it was null. This may

[GitHub] cloudstack pull request: return a state instead of null in Abstrac...

2015-05-01 Thread DaanHoogland
Github user DaanHoogland commented on the pull request: https://github.com/apache/cloudstack/pull/211#issuecomment-98104904 Koushik, I'd rather have the state machine never return null and create a state UNKNOWN. The alert.wait parameter can check for this before setting the

[GitHub] cloudstack pull request: return a state instead of null in Abstrac...

2015-05-01 Thread bhaisaab
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/211#issuecomment-98126407 Guys - do we have any consensus on this issue? I'm planning to cut a 4.5.1 RC on Monday, if we are not very clear I would like to revert this on 4.5 to avoid any

[GitHub] cloudstack pull request: return a state instead of null in Abstrac...

2015-05-01 Thread DaanHoogland
Github user DaanHoogland commented on the pull request: https://github.com/apache/cloudstack/pull/211#issuecomment-98127447 Please don't revert. It is on the 4.4 branch as well. It passes all tests and we have yet to create a test that show this hypothetical regression. If we have

[GitHub] cloudstack pull request: return a state instead of null in Abstrac...

2015-05-01 Thread koushik-das
Github user koushik-das commented on the pull request: https://github.com/apache/cloudstack/pull/211#issuecomment-98120238 @remibergsma Yes XS (6.5) pool HA was enabled. @DaanHoogland Agree on the new state UNKNOWN --- If your project is set up for it, you can reply to this

[GitHub] cloudstack pull request: return a state instead of null in Abstrac...

2015-04-29 Thread asfgit
Github user asfgit closed the pull request at: https://github.com/apache/cloudstack/pull/211 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is

[GitHub] cloudstack pull request: return a state instead of null in Abstrac...

2015-04-29 Thread remibergsma
GitHub user remibergsma opened a pull request: https://github.com/apache/cloudstack/pull/211 return a state instead of null in AbstractInvestigatorImpl When a full cluster is down or unreachable, CloudStack currently reports everything the same as the last known state, which

[GitHub] cloudstack pull request: return a state instead of null in Abstrac...

2015-04-29 Thread DaanHoogland
Github user DaanHoogland commented on the pull request: https://github.com/apache/cloudstack/pull/211#issuecomment-97416661 LGTM --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature

[GitHub] cloudstack pull request: return a state instead of null in Abstrac...

2015-04-29 Thread mlsorensen
Github user mlsorensen commented on the pull request: https://github.com/apache/cloudstack/pull/211#issuecomment-97475041 I guess for KVM at least, the behavior should be similar to stopping/starting the Agent. It makes the hypervisor go into disconnected, but leaves the VM states

[GitHub] cloudstack pull request: return a state instead of null in Abstrac...

2015-04-29 Thread remibergsma
Github user remibergsma commented on the pull request: https://github.com/apache/cloudstack/pull/211#issuecomment-97471962 @koushik-das To reproduce the issue: - create a cluster of 1 or more hypervisors. We observed this issue using xenserver. - spin up some vms on this

[GitHub] cloudstack pull request: return a state instead of null in Abstrac...

2015-04-29 Thread mlsorensen
Github user mlsorensen commented on the pull request: https://github.com/apache/cloudstack/pull/211#issuecomment-97474185 I haven't researched this fix extensively. However, I just want to make sure that the alert state is not accompanied by any undesirable behavior. Moving the

[GitHub] cloudstack pull request: return a state instead of null in Abstrac...

2015-04-29 Thread koushik-das
Github user koushik-das commented on the pull request: https://github.com/apache/cloudstack/pull/211#issuecomment-97503915 @remibergsma My comments are based on the latest master code. Based on my reading of the code, the changes you made shouldn't have any impact on the outcome of

[GitHub] cloudstack pull request: return a state instead of null in Abstrac...

2015-04-29 Thread remibergsma
Github user remibergsma commented on the pull request: https://github.com/apache/cloudstack/pull/211#issuecomment-97542905 @mlsorensen Agree, no state of VMs should change since we cannot tell because the hypervisors are unreachable. This does not change the state of the VMs, only

[GitHub] cloudstack pull request: return a state instead of null in Abstrac...

2015-04-29 Thread DaanHoogland
Github user DaanHoogland commented on the pull request: https://github.com/apache/cloudstack/pull/211#issuecomment-97453854 @koushik-das what is your point? I think I miss context for your remark. --- If your project is set up for it, you can reply to this email and have your reply

[GitHub] cloudstack pull request: return a state instead of null in Abstrac...

2015-04-29 Thread koushik-das
Github user koushik-das commented on the pull request: https://github.com/apache/cloudstack/pull/211#issuecomment-97454863 @DaanHoogland If you look at the code from where testIpAddress() is called (for e.g. UserVmDomRInvestigator.isAgentAlive()), it only deals with return of Up,

[GitHub] cloudstack pull request: return a state instead of null in Abstrac...

2015-04-29 Thread koushik-das
Github user koushik-das commented on the pull request: https://github.com/apache/cloudstack/pull/211#issuecomment-97452036 Reading the code, if testIpAddress() returns anything other than Up, Down or null then returned status is ignored. --- If your project is set up for it, you