[GitHub] cloudstack pull request: server: Introduce Unknown Status to be us...
Github user koushik-das commented on the pull request: https://github.com/apache/cloudstack/pull/222#issuecomment-98589348 I think there is agreement that this is a partial fix (with the right intent). And that this PR doesn't add any value from the end-users perspective. Personally, I would have liked this to be done in a separate branch with all the changes needed to replace null with Unknown in all the investigators. But since it is already committed, would wait for the next PR which will handle 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: server: Introduce Unknown Status to be us...
Github user DaanHoogland commented on the pull request: https://github.com/apache/cloudstack/pull/222#issuecomment-98592118 This does add value to us. It is used to page engineers on duty. Op ma 4 mei 2015 om 07:06 schreef koushik-das notificati...@github.com: I think there is agreement that this is a partial fix (with the right intent). And that this PR doesn't add any value from the end-users perspective. Personally, I would have liked this to be done in a separate branch with all the changes needed to replace null with Unknown in all the investigators. But since it is already committed, would wait for the next PR which will handle this. â Reply to this email directly or view it on GitHub https://github.com/apache/cloudstack/pull/222#issuecomment-98589348. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: server: Introduce Unknown Status to be us...
Github user remibergsma commented on the pull request: https://github.com/apache/cloudstack/pull/222#issuecomment-98471048 @abhinandanprateek Nice write up! +1 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: server: Introduce Unknown Status to be us...
Github user remibergsma commented on the pull request: https://github.com/apache/cloudstack/pull/222#issuecomment-98476949 @bhaisaab I've built 4.5.1 from your branch HA-abstractinvestigatorimpl-nullstate, built everything from scratch. When I shut down all nodes in a cluster, they enter Alert state in ~4min time in my setup (as it did initially). I'm OK with this, although I had expected to see something else than Alert since you set it to Unknown state. From an user perspective, the behaviour in 4.5 and HA-abstractinvestigatorimpl-nullstate seems the same. Am I doing something wrong or is this what we expect? I'll have some more time to work on this tonight. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: server: Introduce Unknown Status to be us...
Github user bhaisaab closed the pull request at: https://github.com/apache/cloudstack/pull/222 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: server: Introduce Unknown Status to be us...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/222#issuecomment-98479757 @remibergsma Hi Remi, thanks for testing. Yes, we should expect the same states - as Abhi shared no change should be seen from the users' side. The changes I did are mostly semantic changes and don't change any input/outputs for the outside world (outside of the black box). For master/4.6; we should change the states to reflect Unknown etc. I guess no testing is required since you've done the regression testing already. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: server: Introduce Unknown Status to be us...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/222#issuecomment-98471837 @abhinandanprateek thanks abhi, you're right on the spot. Merging the change soon (though as you said it does not change anything functionally, but only semantically). --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: server: Introduce Unknown Status to be us...
Github user DaanHoogland commented on the pull request: https://github.com/apache/cloudstack/pull/222#issuecomment-98464228 thanks for summarizing Abhi, +1 from me On Sun, May 3, 2015 at 4:40 AM, Abhinandan Prateek notificati...@github.com wrote: The issue has got entangled, let me try to summarise what each of us has been trying to do: The initial fix itself takes a step into right direction. Use of a proper state instead of null is a good idea. Right now that change is localised in only one of the investigator method. On the whole there is no change to code flow and the change makes it even more readable. If you look at the HA code holistically then this breaks a pattern for the good, but then the whole code should be made consistent, which means when any investigator i.e. a host investigator or a vm investigator does not know the state of the resource for sure it passes 'UNKNOWN' and not 'null'. I think that will be bit disruptive for a maintenance release, but if can be done that is fine too. So if majority agrees that they will continue to work on making this code consistent eventually then that is best way forward . As of now it does not change code flow but only effects the semantic consistency. â Reply to this email directly or view it on GitHub https://github.com/apache/cloudstack/pull/222#issuecomment-98430113. -- Daan --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: server: Introduce Unknown Status to be us...
Github user remibergsma commented on the pull request: https://github.com/apache/cloudstack/pull/222#issuecomment-98482158 Cool, thanks! Sent from my iPhone On 03 May 2015, at 15:14, Rohit Yadav notificati...@github.com wrote: @remibergsma Hi Remi, thanks for testing. Yes, we should expect the same states - as Abhi shared no change should be seen from the users' side. The changes I did are mostly semantic changes and don't change any input/outputs for the outside world (outside of the black box). For master/4.6; we should change the states to reflect Unknown etc. I guess no testing is required since you've done the regression testing already. â Reply to this email directly or view it on GitHub. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: server: Introduce Unknown Status to be us...
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/222#issuecomment-98372480
@koushik-das not needed. The null check is needed to prevent npe. The
unknown status will fall through.
On Sat, 2 May 2015 17:45 koushik-das notificati...@github.com wrote:
I thought the intent of introducing 'Unkonwn' host status was to replace
the entire null semantics that was thought to be confusing in the
investigators. By doing it partially aren't we introducing more confusion
-
now there is 'null' as well as 'Unknown' semantics.
Also specific to the change I see the below code removed
if (vmState == null) {continue;}
Shouldn't it be
if (vmState == Status.Unknown) {continue;}
â
Reply to this email directly or view it on GitHub
https://github.com/apache/cloudstack/pull/222#issuecomment-98372263.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: server: Introduce Unknown Status to be us...
Github user koushik-das commented on the pull request: https://github.com/apache/cloudstack/pull/222#issuecomment-98374391 @DaanHoogland It is not only to prevent NPE but also to go to the next item in the list when the status cannot be determined. Think of it like this - status cannot be determined using the current host, so move to the next one in the list immediately without any further processing. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: server: Introduce Unknown Status to be us...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/222#issuecomment-98360696 @remibergsma thanks man! I'm too working this weekend in order to test ACS and cut a 4.5.1 RC candidate on monday. @koushik-das I've not touched any other code, other than testIpAddress and its consumers. #211 and this PR only try to introduce changes for better debugging. If we want to change the consumers of investigate() or isAlive it could be done separately and if you look at the code they are not returning Unknown at all, their return types remain same. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: server: Introduce Unknown Status to be us...
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/222#issuecomment-98372263
I thought the intent of introducing 'Unkonwn' host status was to replace
the entire null semantics that was thought to be confusing in the
investigators. By doing it partially aren't we introducing more confusion - now
there is 'null' as well as 'Unknown' semantics.
Also specific to the change I see the below code removed
if (vmState == null) {continue;}
Shouldn't it be
if (vmState == Status.Unknown) {continue;}
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: server: Introduce Unknown Status to be us...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/222#issuecomment-98408236 @koushik-das the new fix ensures that testIpAddress does not return null; and there is no need to check for null and then continue --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: server: Introduce Unknown Status to be us...
Github user abhinandanprateek commented on the pull request: https://github.com/apache/cloudstack/pull/222#issuecomment-98430113 The issue has got entangled, let me try to summarise what each of us has been trying to do: The initial fix itself takes a step into right direction. Use of a proper state instead of null is a good idea. Right now that change is localised in only one of the investigator method. On the whole there is no change to code flow and the change makes it even more readable. If you look at the HA code holistically then this breaks a pattern for the good, but then the whole code should be made consistent, which means when any investigator i.e. a host investigator or a vm investigator does not know the state of the resource for sure it passes 'UNKNOWN' and not 'null'. I think that will be bit disruptive for a maintenance release, but if can be done that is fine too. So if majority agrees that they will continue to work on making this code consistent eventually then that is best way forward . As of now it does not change code flow but only effects the semantic consistency. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: server: Introduce Unknown Status to be us...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/222#issuecomment-98146018 @DaanHoogland thanks, I've fixed the issues now. Removed dead code where null check is not required as the method only return Unknown/Up/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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: server: Introduce Unknown Status to be us...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/222#issuecomment-98142160 @dahn @snuf @remibergsma @koushik-das @abhinandanprateek can anyone of you help review this patch? It simply replaces the null semantics with the Status.Unknown state. I checked #211 and I think there is no functional changes that would introduce any bugs, though it adds better return values and debug messages. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: server: Introduce Unknown Status to be us...
GitHub user bhaisaab opened a pull request: https://github.com/apache/cloudstack/pull/222 server: Introduce Unknown Status to be used in AbstractInvestigatorImpl The PR #211 introduced changes where the abstract investigator testIpAddress() would return other Status, which previously only returned null, Up or Down. In this patch we introduce a new Status Unknown that replaces null's semantics. The important changes #211 introduced was the debugging statements as semantically the changes would work same as the consumers of testIpAddress() method only used if returned values were Up or Down and in other cases (null, Alert etc) it would simply continue to loop through the resources being investigated. Keeping the debug logs, this commit only replaces the previously returned null values with Status.Unknown and fixed the debug statements to reflect the same. In case of trapped exceptions too, we return Unknown status but log the exception we trapped. Signed-off-by: Rohit Yadav rohit.ya...@shapeblue.com You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/cloudstack HA-abstractinvestigatorimpl-nullstate Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/222.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 #222 commit fee74106aabbd97693b829200fc4d8f30d965b88 Author: Rohit Yadav rohit.ya...@shapeblue.com Date: 2015-05-01T14:03:51Z server: Introduce Unknown Status to be used in AbstractInvestigatorImpl The PR #211 introduced changes where the abstract investigator testIpAddress() would return other Status, which previously only returned null, Up or Down. In this patch we introduce a new Status Unknown that replaces null's semantics. The important changes #211 introduced was the debugging statements as semantically the changes would work same as the consumers of testIpAddress() method only used if returned values were Up or Down and in other cases (null, Alert etc) it would simply continue to loop through the resources being investigated. Keeping the debug logs, this commit only replaces the previously returned null values with Status.Unknown and fixed the debug statements to reflect the same. In case of trapped exceptions too, we return Unknown status but log the exception we trapped. Signed-off-by: Rohit Yadav rohit.ya...@shapeblue.com --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: server: Introduce Unknown Status to be us...
Github user DaanHoogland commented on the pull request: https://github.com/apache/cloudstack/pull/222#issuecomment-98143398 @bhaisaab Remi told me he will test. LGTM except that the null check is redundant. We could remove it --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: server: Introduce Unknown Status to be us...
Github user koushik-das commented on the pull request: https://github.com/apache/cloudstack/pull/222#issuecomment-98181315 @bhaisaab The 'null' semantics of all investigators needs to change to 'Unknown'. Just look at all classes that implements Investigator interface. (XS, KVM, Simulator, CheckOnAgent etc.) Also check the noredist ones. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: server: Introduce Unknown Status to be us...
Github user koushik-das commented on the pull request: https://github.com/apache/cloudstack/pull/222#issuecomment-98183123 Also callers for investigate()/isAgentAlive() methods needs to be fixed to handle Unknown instead of 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: server: Introduce Unknown Status to be us...
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/222#discussion_r29503805
--- Diff: server/src/com/cloud/ha/ManagementIPSystemVMInvestigator.java ---
@@ -78,7 +78,7 @@ public Boolean isVmAlive(VirtualMachine vm, Host host) {
ListLong otherHosts = findHostByPod(vmHost.getPodId(),
vm.getHostId());
for (Long otherHost : otherHosts) {
Status vmState = testIpAddress(otherHost,
nic.getIp4Address());
-if (vmState == null) {
+if (vmState == null || vmState == Status.Unknown) {
--- End diff --
this check should not be needed (neither null or 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: server: Introduce Unknown Status to be us...
Github user remibergsma commented on the pull request: https://github.com/apache/cloudstack/pull/222#issuecomment-98247992 @bhaisaab I'm preparing my lab to test your branch. Will do that in the weekend and let you know! --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: server: Introduce Unknown Status to be us...
Github user DaanHoogland commented on the pull request: https://github.com/apache/cloudstack/pull/222#issuecomment-98250942 @koushik-das You sure this is needed for this PR to work? it seems to me this can be future work. Nothing will be broken more then it is now. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
