search

gluster / gluster-block

A framework for gluster block storage
GNU General Public License v2.0
74 stars 32 forks source link

create/delete: fix the return check logs #247

Closed lxbsz closed 5 years ago

lxbsz commented 5 years ago

What does this PR achieve? Why do we need it?

This will fix the issue when the create/delete are actually failed but the logs will still say create/delete cli return success.

lxbsz commented 5 years ago

@pkalever Rebased to the latest. Thanks.

lxbsz commented 5 years ago

@pkalever Please review, Thanks. BRs

pkalever commented 5 years ago

@pkalever Please review, Thanks.

@lxbsz cool, this is exactly what I wanted . Let me apply this patch and see if there are any gaps. Thanks!

lxbsz commented 5 years ago

@pkalever Addressed them all, please check it again. Thanks.

pkalever commented 5 years ago

@lxbsz one observation:

[2019-07-24 11:32:03.998721] INFO: delete cli request, volume=sample blockname=block6 [at block_delete.c+339 :<block_d
elete_cli_1_svc_st>]
[2019-07-24 11:32:04.019858] INFO: delete request, blockname=block6 filename=082acf0e-7e39-4d89-ad67-54ad8dfd4734 [at 
block_delete.c+462 :<block_delete_1_svc_st>]
[2019-07-24 11:32:04.913942] INFO: command exit code, 0 [at block_delete.c+494 :<block_delete_1_svc_st>]
[2019-07-24 11:32:04.933222] WARNING: glusterBlockDeleteRemoteAsync: return -1 failed in remote async delete for block
 block6 on volume sample [at block_delete.c+285 :<glusterBlockCleanUp>]
[2019-07-24 11:32:04.933286] WARNING: glusterBlockCleanUp: return -1 on block block6 for volume sample [at block_delet
e.c+416 :<block_delete_cli_1_svc_st>]
[2019-07-24 11:32:04.933654] INFO: delete cli return failure, volume=sample blockname=block6 [at block_delete.c+428 :<
block_delete_cli_1_svc_st>]

not sure if this PR introduced the issue, but lets fix it here ?

pkalever commented 5 years ago

@lxbsz In block_modify.c see line 834 In rpc/block_create.c line 1054 needs some attention.

rest all looks good to me. Just take of my earlier comment https://github.com/gluster/gluster-block/pull/247#issuecomment-514596184

Please send this as rspin-2 as a separate patch, just like before.

Thanks!

lxbsz commented 5 years ago

@lxbsz one observation:

[2019-07-24 11:32:03.998721] INFO: delete cli request, volume=sample blockname=block6 [at block_delete.c+339 :<block_d
elete_cli_1_svc_st>]
[2019-07-24 11:32:04.019858] INFO: delete request, blockname=block6 filename=082acf0e-7e39-4d89-ad67-54ad8dfd4734 [at 
block_delete.c+462 :<block_delete_1_svc_st>]
[2019-07-24 11:32:04.913942] INFO: command exit code, 0 [at block_delete.c+494 :<block_delete_1_svc_st>]
[2019-07-24 11:32:04.933222] WARNING: glusterBlockDeleteRemoteAsync: return -1 failed in remote async delete for block
 block6 on volume sample [at block_delete.c+285 :<glusterBlockCleanUp>]
[2019-07-24 11:32:04.933286] WARNING: glusterBlockCleanUp: return -1 on block block6 for volume sample [at block_delet
e.c+416 :<block_delete_cli_1_svc_st>]
[2019-07-24 11:32:04.933654] INFO: delete cli return failure, volume=sample blockname=block6 [at block_delete.c+428 :<
block_delete_cli_1_svc_st>]

not sure if this PR introduced the issue, but lets fix it here ?

@pkalever

BTW, do you mean the WARNING --> ERROR ?

Thanks.

pkalever commented 5 years ago

not sure if this PR introduced the issue, but lets fix it here ?

@pkalever

BTW, do you mean the WARNING --> ERROR ?

No No. This part is fine. If you observe. although "INFO: command exit code, 0" exit code is 0 glusterBlockDeleteRemoteAsync is returning -1.

Also, one comment as you pointed it right.

Should the log-levels be ERROR instead of INFO when cli returns failure ?

Thanks!

Thanks.

pkalever commented 5 years ago

not sure if this PR introduced the issue, but lets fix it here ? @pkalever BTW, do you mean the WARNING --> ERROR ?

No No. This part is fine. If you observe. although "INFO: command exit code, 0" exit code is 0 glusterBlockDeleteRemoteAsync is returning -1.

Ignore this, I see there was one issue in my setup. It works as expected.

Also, one comment as you pointed it right.

Should the log-levels be ERROR instead of INFO when cli returns failure ?

can we work on this in this PR ?

lxbsz commented 5 years ago

not sure if this PR introduced the issue, but lets fix it here ?

@pkalever BTW, do you mean the WARNING --> ERROR ?

No No. This part is fine. If you observe. although "INFO: command exit code, 0" exit code is 0 glusterBlockDeleteRemoteAsync is returning -1.

Also, one comment as you pointed it right.

Should the log-levels be ERROR instead of INFO when cli returns failure ?

Yeah, will fix it.

lxbsz commented 5 years ago

Over all looks good to me.

just left one comment on the new change. And do we want to print 'cli returned failure' log in the ERROR level ?

I will leave this to you.

Reviewed-by: Prasanna Kumar Kalever [prasanna.kalever@redhat.com](mailto:prasanna.kalever@redhat.com)

@pkalever Please check it again, if no any problem I will squash them and add the tag. Thanks.

pkalever commented 5 years ago

Thanks @lxbsz , Merged now.