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

ceph_context: re-expand admin_socket metavariables in child process #18393

Merged
merged 1 commit into from Oct 24, 2017

Conversation

david-z
Copy link
Member

@david-z david-z commented Oct 19, 2017

Reset admin_socket raw value if it is defined in conf file. Just in case it used metavarirables (e.g, $pid) which could be expanded again to the correct value in child process.

Fixes: #http://tracker.ceph.com/issues/21848

Signed-off-by: Zhi Zhang zhangz.david@outlook.com

@david-z david-z requested a review from ukernel October 19, 2017 07:53
@tchaikov tchaikov added the cephfs Ceph File System label Oct 19, 2017
@david-z
Copy link
Member Author

david-z commented Oct 20, 2017

retest this please.

@ukernel
Copy link
Contributor

ukernel commented Oct 20, 2017

it's a general problem for all ceph daemons. is it possible to fix problem completely?

Reset admin_socket raw value if it is defined in conffile.
Just in case it used metavarirables (e.g, $pid) which could be expanded
again to the correct value in child process.

Signed-off-by: Zhi Zhang <zhangz.david@outlook.com>
@david-z david-z changed the title ceph-fuse: re-expand admin_socket metavariables in child process ceph_context: re-expand admin_socket metavariables in child process Oct 23, 2017
@david-z
Copy link
Member Author

david-z commented Oct 23, 2017

@ukernel I checked again on ceph-mon, ceph-osd, ceph-mds, ceph-mgr and ceph-fuse daemons. Except ceph-fuse, other daemons don't have this problem right now. But if a daemon is started in a child process and the value of its "daemonize" option is true, this problem would happen. So once for all, I moved the fix into ceph_context just before creating admin_socket.

Please review the new changes, thanks.

@batrick batrick merged commit 1630f4b into ceph:master Oct 24, 2017
batrick added a commit that referenced this pull request Oct 24, 2017
* refs/pull/18393/head:
	ceph_context: re-expand admin_socket metavariables in child process

Reviewed-by: Patrick Donnelly <pdonnell@redhat.com>
@trociny
Copy link
Contributor

trociny commented Oct 25, 2017

This change introduces a regression: admin-socket, specified via command line or CEPH_ARGS env does not work:

rbd --admin-socket=/tmp/2.asok watch test_image

E.g. this test fails now: qa/workunits/rbd/test_admin_socket.sh

@trociny
Copy link
Contributor

trociny commented Oct 25, 2017

I mean, if you have admin-socket specified both in ceph.conf and via command line (or env), it will use ceph.conf setting instead of command line override.

@trociny
Copy link
Contributor

trociny commented Oct 25, 2017

Another thing that I don't like very much about this solution is that it tries to fix only "admin-socket" config option, while $pid can be useful in other options too (e.g. log-file).

@batrick
Copy link
Member

batrick commented Oct 25, 2017

Hrm, that's a good point @trociny. I'll revert this and let @david-z revisit the solution in a new PR.

@batrick
Copy link
Member

batrick commented Oct 25, 2017

#18545

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants