Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exit code fix #93

Merged
merged 2 commits into from Jul 11, 2018
Merged

Exit code fix #93

merged 2 commits into from Jul 11, 2018

Conversation

pkalever
Copy link
Contributor

@pkalever pkalever commented Jul 6, 2018

No description provided.

@pkalever pkalever requested a review from pranithk July 6, 2018 12:23
@pranithk
Copy link
Member

pranithk commented Jul 9, 2018

@pkalever Could you add a bit more details about what is the problem and how it addresses the problem?

@@ -3783,6 +3782,7 @@ blockCreateCliFormatResponse(struct glfs *glfs, blockCreateCli *blk,
blockFormatErrorResponse(CREATE_SRV, blk->json_resp, errCode,
GB_DEFAULT_ERRMSG, reply);
}
reply->exit = errCode;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the BUG raised, this line fixes the problem. Create exit code is not collected before, so fixing the code to collect the right exit code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pkalever If you look at the pull request/commit message it doesn't explain the problem what you encountered and how it is solving it. People generally do that when a patch is sent, otherwise the context will be forgotten after a few months. I just want you to capture that in the pull request.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pkalever Thanks for the commit message. I am seeing similar issue in modify, resize, info. I think even in Delete we can move it to the end like you did here. Did you get a chance to check those as well? Am I missing some other detail?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks!

@pkalever
Copy link
Contributor Author

pkalever commented Jul 10, 2018

@pranithk I have updated the commit msg.

@pkalever
Copy link
Contributor Author

@pranithk after fixing your review comments. I have tested create/info/list/modify (auth/size)/delete operations with and without json flag and validated the exit code for success paths. In all the cases I got exit code 0.

Thanks!

Copy link
Member

@pranithk pranithk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look okay, but the commit message still says create, instead of the wide range of cli operations for which it works now.

Prasanna Kumar Kalever added 2 commits July 11, 2018 21:03
also tuned few logs to look more meaningful

Reviewed-by: Pranith Kumar K <pkarampu@redhat.com>
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
In version check failure paths, we are not updating the reply->exit variable.

This patch will makesure about updating reply->exit value in all
block*CliFormatResponse() functions, so that we never miss to update
the right exitCode.

Also improved some code for better readability.

Reviewed-by: Pranith Kumar K <pkarampu@redhat.com>
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
@pkalever
Copy link
Contributor Author

@pranithk after fixing your review comments. I have tested create/info/list/modify (auth/size)/delete operations with and without json flag and validated the exit code for success paths. In all the cases I got exit code 0.

As discussed, In addition to this for -ve testing, have run create//modify (auth/size)/delete ops while "for i in {1..100000}; do pkill -9 targetcli; done" is running on one of the HA node with and without json flag, In all the cases I got exit code 255.

Will paste the output next to this comment.

Thanks!

@pkalever
Copy link
Contributor Author

pkalever commented Jul 11, 2018

@pranithk here is the output:

[root@localhost gluster-block]# gluster-block create sample/block1 ha 3 192.168.124.6,192.168.124.163,192.168.124.217 1000000000 
failed to configure on 192.168.124.217 configure failed
RESULT:FAIL
[root@localhost gluster-block]# echo $?
255


[root@localhost gluster-block]# gluster-block create sample/block1 ha 3 192.168.124.6,192.168.124.163,192.168.124.217 1000000000  --json-pretty
{
  "RESULT":"FAIL",
  "errCode":255,
  "errMsg":"failed to configure on 192.168.124.217 configure failed"
}
[root@localhost gluster-block]# echo $?
255


[root@localhost gluster-block]# gluster-block modify sample/block1 auth enable                                                                                                                                     
IQN: iqn.2016-12.org.gluster-block:6cbd9abe-30a6-451a-acf4-6e9d50a2ae8c                                  
FAILED ON:  192.168.124.217                         
SUCCESSFUL ON:  192.168.124.6 192.168.124.163       
ROLLBACK SUCCESS ON:  192.168.124.6 192.168.124.163 
RESULT: FAIL                                        
[root@localhost gluster-block]# echo $?             
255                                                 


[root@localhost gluster-block]# gluster-block modify sample/block1 auth enable --json-pretty             
{                                                   
  "IQN":"iqn.2016-12.org.gluster-block:6cbd9abe-30a6-451a-acf4-6e9d50a2ae8c",                            
  "FAILED ON":[                                     
    "192.168.124.217"                               
  ],                                                
  "SUCCESSFUL ON":[                                 
    "192.168.124.6",                                
    "192.168.124.163"                               
  ],                                                
  "ROLLBACK SUCCESS ON":[                           
    "192.168.124.6",                                
    "192.168.124.163"                               
  ],                                                
  "RESULT":"FAIL"                                   
}                                                   
[root@localhost gluster-block]# echo $?             
255                                                 




[root@localhost gluster-block]# gluster-block modify sample/block1 size 2GiB
IQN: iqn.2016-12.org.gluster-block:6cbd9abe-30a6-451a-acf4-6e9d50a2ae8c
FAILED ON:  192.168.124.217
SUCCESSFUL ON:  192.168.124.6 192.168.124.163
RESULT: FAIL
[root@localhost gluster-block]# echo $?
255
[root@localhost gluster-block]# gluster-block modify sample/block1 size 2GiB --json-pretty
{
  "IQN":"iqn.2016-12.org.gluster-block:6cbd9abe-30a6-451a-acf4-6e9d50a2ae8c",
  "FAILED ON":[
    "192.168.124.217"
  ],
  "SUCCESSFUL ON":[
    "192.168.124.6",
    "192.168.124.163"
  ],
  "RESULT":"FAIL"
}
[root@localhost gluster-block]# echo $?
255
[root@localhost gluster-block]# 



[root@localhost gluster-block]# gluster-block delete sample/block1
FAILED ON:   192.168.124.217
SUCCESSFUL ON:   192.168.124.6 192.168.124.163
RESULT: FAIL
[root@localhost gluster-block]# echo $?
255
[root@localhost gluster-block]# gluster-block delete sample/block1 --json-pretty
{
  "FAILED ON":[
    "192.168.124.217"
  ],
  "RESULT":"FAIL"
}
[root@localhost gluster-block]# echo $?
255

@pranithk pranithk merged commit dc0c037 into gluster:master Jul 11, 2018
@pkalever pkalever deleted the exitCode_fix branch May 13, 2020 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants