You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by remibergsma <gi...@git.apache.org> on 2015/04/29 14:41:52 UTC
[GitHub] cloudstack pull request: return a state instead of null in Abstrac...
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 is usually
Up. When it cannot reach a host and cannot
reach another host in the same cluster either,
it returns null and says "I don't know". This
prevents it from reporting the problem. Now,
we return an Alert or Disconnected state so
proper action can be taken.
Also logging was added, so we know what part
of the code put it to Alert or Disconnected.
When the host is available again, it goes
from Alert state back to Up and CloudStack
starts HA work to recover the VMs. I tested
it on 4.6/master and it works fine now.
As this is a nasty bug, we might want to fix
this also in 4.5 and 4.4.
Thanks to @dahn and @snuf for their
help solving this issue.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/remibergsma/cloudstack investigator_null_state_fix
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/211.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #211
----
commit 78e095e64b2344a49e96a7939ca6edd3b36d93dd
Author: Remi Bergsma <gi...@remi.nl>
Date: 2015-04-29T18:14:14Z
return a state instead of null
When a full cluster is down or unreachable,
CloudStack currently reports everything the
same as the last known state, which is usually
Up. When it cannot reach a host and cannot
reach another host in the same cluster either,
it returns null and says "I don't know". This
prevents it from reporting the problem. Now,
we return an Alert or Disconnected state so
proper action can be taken.
Also logging was added, so we know what part
of the code put it to Alert or Disconnected.
----
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: return a state instead of null in Abstrac...
Posted by koushik-das <gi...@git.apache.org>.
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, Down and null. So I am not able to follow the use of this fix.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: return a state instead of null in Abstrac...
Posted by abhinandanprateek <gi...@git.apache.org>.
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 returned some pings is in an indeterministic state, HA recognises it as "null" and not as Alert.
That apart there are there places that the method changed gets called. If you look at the code the Alert state does not have any affect on the code flow. I hope I did not miss out some.
I will say check again, as introduction of Alert state may confuse people looking at the code.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: return a state instead of null in Abstrac...
Posted by bhaisaab <gi...@git.apache.org>.
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 regressions in the release. I'm not personally sure of any side effects since the method only returned Up, Down or null, looks like whatever you return which is no Up or Down, the "for loop" would simply continue (so same behaviour as in null).
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: return a state instead of null in Abstrac...
Posted by remibergsma <gi...@git.apache.org>.
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 the hypervisors.
@koushik-das It already takes a few minutes before it determines it cannot reach the hypersvisor (after a few retries). I didn't change anything on this. It is not configurable right now as far as I know.
I will have a look at the test you mentioned to see what you mean.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: return a state instead of null in Abstrac...
Posted by koushik-das <gi...@git.apache.org>.
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 the test. I see that HighAvailabilityManagerImpl.investigate() method returns status as Alert if none of the investigators are able to determine the status. Is it possible that the test is passing due to this?
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: return a state instead of null in Abstrac...
Posted by DaanHoogland <gi...@git.apache.org>.
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 state of an object to null, which should never happen. especially with
a state enum of
Creating(true, false, false),
Connecting(true, false, false),
Up(true, false, false),
Down(true, true, true),
Disconnected(true, true, true),
Alert(true, true, true),
Removed(true, false, true),
Error(true, false, true),
Rebalancing(true, false, true);
The page you mention has been last edited Sep 13, 2013. By capable people
but can still be considered outdated. I think if we need to we should add
UNKNOWN to the states but we shouldn't return null.
Op vr 1 mei 2015 om 11:55 schreef Abhinandan Prateek <
notifications@github.com>:
> The way HA is implemented, “null” is a valid return value from an
> investigator. The valid values are Up, Down or null.
>
> https://cwiki.apache.org/confluence/display/CLOUDSTACK/High+Availability+Developer's+Guide
> The other values may not be handled by HA impl., please check.
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/cloudstack/pull/211#issuecomment-98087158>.
>
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: return a state instead of null in Abstrac...
Posted by remibergsma <gi...@git.apache.org>.
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 cluster
- pull the plug on all hypervisors at the same time, so an unclean shutdown
Before this fix, CloudStack would not notice the hypervisors were down and keep them Up.
After this fix, it puts the hypervisors to Alert state and notifies admin.
If you mean something else, please let me know. All tests pass so I'm not aware of anything failing.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: return a state instead of null in Abstrac...
Posted by bhaisaab <gi...@git.apache.org>.
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 code that are so tribal or fragile (that must be fixed), changes in them may terrify people.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: return a state instead of null in Abstrac...
Posted by mlsorensen <gi...@git.apache.org>.
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 hypervisors to alert state to warn the admin is fine, but we don't want to assume that anything actually went down. In the scenario given, it is probably more likely that there was a break in the network between the hypervisors and mgmt server, and VMs are running fine, so as long as we are accounting for that scenario during recovery from alert state and not assuming the VMs are down we should be good.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: return a state instead of null in Abstrac...
Posted by abhinandanprateek <gi...@git.apache.org>.
Github user abhinandanprateek commented on the pull request:
https://github.com/apache/cloudstack/pull/211#issuecomment-98087158
The way HA is implemented, “null” is a valid return value from an investigator. The valid values are Up, Down or null.
https://cwiki.apache.org/confluence/display/CLOUDSTACK/High+Availability+Developer's+Guide
The other values may not be handled by HA impl., please check.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: return a state instead of null in Abstrac...
Posted by DaanHoogland <gi...@git.apache.org>.
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
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
Re: [GitHub] cloudstack pull request: return a state instead of null
in Abstrac...
Posted by Daan Hoogland <da...@gmail.com>.
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 return is ignoring and as
such same as the new returns at the moment.
effectively the result (and objective) of the commit is only the clearer
log messages. This enables alerting via 3rd party alerting (pager duty)
systems.
Hope this convinces you all.
Op vr 1 mei 2015 om 14:53 schreef DaanHoogland <gi...@git.apache.org>:
> 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 I'll fix forward.
>
>
> ---
> 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 enabled but not working, please
> contact infrastructure at infrastructure@apache.org or file a JIRA ticket
> with INFRA.
> ---
>
[GitHub] cloudstack pull request: return a state instead of null in Abstrac...
Posted by DaanHoogland <gi...@git.apache.org>.
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 I'll fix forward.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: return a state instead of null in Abstrac...
Posted by mlsorensen <gi...@git.apache.org>.
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 as-is, and is reconciled when the Agent is restarted.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: return a state instead of null in Abstrac...
Posted by remibergsma <gi...@git.apache.org>.
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 behaviour, but that's another story. I can imagine this is why it does not align with ping.timeout anymore?
Was your test done with XenHA enabled? If you share your test details I will try to replay it and see what happens.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: return a state instead of null in Abstrac...
Posted by koushik-das <gi...@git.apache.org>.
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 not have the desired effect all the time.
Recently I came across a scenario where if XS pool master is shutdown and it takes more than "ping timeout" for the new master to get elected then the host always stays in Alert state. Earlier Cloudstack was able to determine the state as Down.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: return a state instead of null in Abstrac...
Posted by DaanHoogland <gi...@git.apache.org>.
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 appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: return a state instead of null in Abstrac...
Posted by DaanHoogland <gi...@git.apache.org>.
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 state to alert.
Op vr 1 mei 2015 om 13:03 schreef koushik-das <no...@github.com>:
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 that the status of host cannot be determined then put it to
> Alert rather than putting it to Alert immediately.
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/cloudstack/pull/211#issuecomment-98100569>.
>
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: return a state instead of null in Abstrac...
Posted by abhinandanprateek <gi...@git.apache.org>.
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 trying to convey. The HA related timeouts are very delicately balanced. On further looking at the code around the commit, my guess is it actually does not make difference to code-flow. But we should replace the "null" with say a new state like "UNKNOWN" and make sure that it is handled in the call hierarchy.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: return a state instead of null in Abstrac...
Posted by asfgit <gi...@git.apache.org>.
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: return a state instead of null in Abstrac...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/211#issuecomment-97512774
Also about the scenario that is being addressed - all hosts in a XS cluster are shutdown by pulling the plug. Instead of immediately putting the hosts in Alert state, it may be better to wait for some configurable amount of time and after that if it is not possible to determine status of hosts in the cluster then put them in Alert.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: return a state instead of null in Abstrac...
Posted by abhinandanprateek <gi...@git.apache.org>.
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 existing documentation around it that also needs to be revamped.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: return a state instead of null in Abstrac...
Posted by koushik-das <gi...@git.apache.org>.
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 that the status of host cannot be determined then put it to Alert rather than putting it to Alert immediately.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: return a state instead of null in Abstrac...
Posted by koushik-das <gi...@git.apache.org>.
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 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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: return a state instead of null in Abstrac...
Posted by koushik-das <gi...@git.apache.org>.
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 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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---