-
Notifications
You must be signed in to change notification settings - Fork 33
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
gluster-block: adopt to targetclid usage #203
Conversation
Sending this PR, just to enable others to test targetcli-fb#132 PR with gluster-block. |
c95876b
to
4e91f32
Compare
4e91f32
to
3518bff
Compare
@lxbsz Please take a look. Thanks! |
@pkalever Thanks. |
@lxbsz IMHO, having targetclid is not mandatory, hence lets just check if its exist. In case its available start it and use the daemon approach, else fall back to old approach only. Does that explain you, why I didn't wrote up code in the capabilities[.c/.h]/versioning part aswell ? |
3518bff
to
2605b43
Compare
There is one problem, if the targetclid service died for some reason, then the create will fail: The logs are:
And currently we only checking the targetclid liveness when gluster-blockd daemon's starting. After the targetclid dead, the targetcli command will stuck even after manually start the targetclid service. Thanks. |
For this I was using the old setups which is not clean: [root@rhel3 gluster-block]# ls /usr/lib/python2.7/site-packages/rtslib By cleaning the old packages in the old setup and also test it again in a new setup, all works as expected, no stuck hit any more. Thanks. |
@lxbsz updated this PR based on latest improvements to targetcli PR#153, please help review. Thanks! |
daemon/gluster-blockd.c
Outdated
ret = gbRunner("ps aux ww | grep -w '[t]argetclid' > /dev/null"); | ||
if (ret) { | ||
LOG("mgmt", GB_LOG_WARNING, "targetclid not running, using targetcli"); | ||
if (GB_ASPRINTF(&global_opts, "targetcli --skip-daemon; %s", tmp) == -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, should we also set the auto_use_daemon=false at the same time here since the targetclid is not running ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lxbsz '# targetcli --skip-daemon' will internally set auto_use_daemon=false, as there is no daemon running, we cannot send any commands via daemon, hence ''# targetcli --skip-daemon' will internally bypass call to daemon although 'auto_use_daemon=true' is in action and set 'auto_use_daemon=false' via the cli route.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pkalever
Ah, yes, you are right. I misread that part of code.
Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lxbsz could you please test this and approve ? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pkalever
Before I had test it and it works. I thought we need to wait the targetcli_fb patches, which were not totally finished yet and the '--skip-daemon' option may change ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lxbsz
That is right, we need to wait for targetcli pathes for sure, just wanted to make sure we keep this patch ready till then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pkalever Sure, once that PR is done I will test it again and approve it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lxbsz the targetcli improvements PR is merged now, could you please test and review this patch. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested it and this looks good to me.
Thanks.
Requires: open-iscsi/targetcli-fb#132 Signed-off-by: Prasanna Kumar Kalever <[email protected]>
Tested this and it works for me as well. Merging now. |
What does this PR achieve? Why do we need it?
Improve over all creation time at scale and give a consistent create time for users.
Does this PR fix issues?
Fixes:
http://bit.ly/targetcli-create-delay
Notes for the reviewer
TODO: add targetcli version check and fall-back mechanism in absence of
required targetcli version supporting daemonize targetclid
Requires: open-iscsi/targetcli-fb#132
Signed-off-by: Prasanna Kumar Kalever [email protected]