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
rbd-nbd: create admin socket only for map command #12433
Conversation
|
@trociny , please help take a look. Thanks. |
I don't think it is a good idea to hardcode asok and log paths. Note, currently your problem may be solved by configuration, adding to the ceph.conf:
i.e. separating the files by rbd-nbd process id. If you want nbd client settings differ from those for other clients you can add them to
Note1: you also need to add client.nbd auth key to ceph for this to work. Also, you can specify these parameters directly in the command line:
(again, currently it does not work for I agree that current defaults are not very nice for rbd-nbd processes. It would be very desirable if we were able to change these to something more reasonable. Using the nbd device index to separate log and asok files also looks appealing to me, as it makes easier to find logs and asok files for a particular nbd device. Still I think that preserving a possibility for a user to set log and asok files locations via config file (and command args) is a must. I would agree with your overrides if they applied only if log_file or admin_socket are not specified in the config. Not sure everyone would be happy in this case too: the current behavior is that empty log_file means no logs and after this change it looks like no way to achieve this. With admin_socket it is simpler, because it is always set internally, even if it is empty in the config file. So I would definitely want to fix the default asok path for nbd processes to avoid 'File exists' errors. Using nbd device index for separation would be very nice, but pid would be good enough too. I would like we also fix the issue with We might want to change the default id for rbd-nbd from 'admin' to 'nbd', so we could use '[client.nbd]' config section for nbd, though it complicates a little testing and deploy as we need additionally to set auth key for client.nbd. Also (while here) currently we explicitly disable pid file for rbd-nbd processes. I am not sure it is a very good idea -- I would like to have at least a possibility for a user to specify pid file if she relays on this daemon behavior. |
+1 on @trociny's suggestion for a clean way to pass extra options to |
@liupan1111 So I would suggest limit this PR to the admin socket changes only (basically, your first patch) and change it to set admin socket path (using nbd index) only if it is not set in the config or command args. Please, also add 'Fixes: http://tracker.ceph.com/issues/17951' to your commit log message. If you are interested to work on fixing As for log file path, I would leave it as it is currently, still if you want to change this somehow, please create a separate PR. |
I would actually propose the following: right now, the config supports multiple metavariables (i.e. id, cctid, etc). These are currently hardcoded [1] but it would be nice if they could support a string -> functor mapping where the functor, when invoked, would return the translated metavariable. That would allow rbd-nbd to register a "device" metavariable that can be added to "log_file", "admin_socket", etc in a generic way in your ceph.conf instead of hard-coding things. [1] https://github.com/ceph/ceph/blob/master/src/common/config.cc#L1069 |
@dillaman extendable config metavariables is a very nice thing to have. Still I think we need some hard code to fix [1]. The problem is that the default value for |
@trociny That error isn't unique to rbd-nbd (it happens all over the place in the test harness, for example). I'd be fine w/ just blanking out the default asok path if it's not specified in the config file. If it is specified in the config file and the user gave a generic path that would result in a collision, they will get the warning and will need to fix it. |
@dillaman , I agree this error isn't unique to rbd-nbd. It is amazing to resolve this by extendable config metavariables. But for this issue http://tracker.ceph.com/issues/17951, I prefer we resolve it first by using my first patch(after modified as trociny's request). And I like to do more investigation later. Thanks. |
@trociny , I've updated, please help take a look. |
Actually, what Jason proposes (if a path is not specified in config, asok is not created) will also resolve this issue. Though I am not very happy with this solution. Having asok file for long running processes looks useful enough to have this by default: you can use this to control rbd-nbd (monitor, gather stats, invalidate cache, etc). We could have some wrapper similar to Also, currently I am not satisfied with [1] https://tycho.ws/blog/2015/02/setproctitle.html The current code looks a little strange: we call global_init with CODE_ENVIRONMENT_DAEMON, to set some desirable default behavior but then disable this behavior (unset pid_file, now we want to unset admin_socket). It looks the only useful thing from CODE_ENVIRONMENT_DAEMON that remains is daemonize. And actually, the current code not very correct: it should call global_init with CODE_ENVIRONMENT_DAEMON only for And returning to
E.g. it would be easy to set up process monitoring in this case. Currently |
src/tools/rbd_nbd/rbd-nbd.cc
Outdated
g_conf->get_my_sections(my_sections); | ||
std::string admin_socket_str; | ||
return ((g_conf->get_val_from_conf_file(my_sections, key, | ||
admin_socket_str, false) == 0) ? true : false); |
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.
Will this work if I specify admin_socket via command line param?
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.
er... I didn't find any option for admin_socket. Do you need me to add one?
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.
You don't need to add. rbd-nbd parses command line options using global_init(). That means that any config option can be also specified via command line. Just try:
rbd-nbd --admin-socket=/tmp/test.asok ${image}
and check that after your modifications it still uses /tmp/test.asok in this case.
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.
I tried, and it doesn't work ... Is this needed to support? The user cannot find --admin-sock option for rbd-nbd --help ....
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.
It is --admin-socket
(not --admin-sock
).
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.
yes yes... sorry for the typo in my comment, I really tried:
rbd-nbd --admin-socket=/tmp/test.asok map rbd/test
But there was no test.asok file created.
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.
Actually, trying your version, it looks like it works like expected: when admin-socket is specified either in config or via command line, this setting is used, otherwise it is set to {run_dir}/ceph-client.{nbd_dev}.asok.
I have many comments to your code though, but at first I think we need to reach a consensus on the default behavior (Jason wants asok is not set at all by default, and this is actually the safest behavior, though may be not the most useful).
@dillaman What do you think?
Looking at how asok could be used to improve nbd list
command, I tend to agree with the idea to hard code it to something like {run_dir}/rbd-nbd.{nbd_dev}.asok (so user settings are ignored). Then ndb list
could go through the list of rbd-nbd.*.asok files, and query through asok commands. Also, this could also allow to unmap using image name, not nbd device.
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.
I am still not sure about the defaulted asok. In terms of improving rbd list
, you can extract the associated pid from "/sys/block/nbdX/pid" and then extract the pool/image from "/proc/PID/cmdline" -- no need for "magic" hard-coded asok paths.
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.
Ah, "/sys/block/nbdX/pid" really makes the life much easier! I thought there should be something like this and looked for it some time ago, but failed to find then.
I agree that there is no need in asok for list-mapped
in this case, and now I don't have a strong opinion about the default for admin-socket (both empty and {run_dir}/rbd-nbd.{nbd_dev}.asok look good to me).
src/tools/rbd_nbd/rbd-nbd.cc
Outdated
#define RBD_NBD_SOCK_PREFIX "ceph-client" | ||
#define RBD_NBD_SOCK_EXT "asok" | ||
#define RBD_NBD_CONF_ADMIN_SOCKET "admin_socket" | ||
#define RBD_NBD_CONF_DIR "run_dir" |
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.
I don't see much values in these defines. I would use option names directly.
src/tools/rbd_nbd/rbd-nbd.cc
Outdated
@@ -85,6 +85,11 @@ static bool exclusive = false; | |||
#endif | |||
#define htonll(a) ntohll(a) | |||
|
|||
#define RBD_NBD_SOCK_PREFIX "ceph-client" |
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.
Actually this should be $cluster-$name. See config_opts.h how the default is defined:
OPTION(admin_socket, OPT_STR, "$run_dir/$cluster-$name.asok") // default changed by common_preinit()
[Pan] In deed, I've already have a fix in my local. I will create a new PR after this one closed.
[Pan] This part of code really makes me puzzled at first. We could enhance this later.
[Pan] I totally agree. For list and unmap, it is a one-time-run command. New PR later for this part. |
@liupan1111 ping |
There are many great comments and discussions, and we should first reach a consensus on what I should do in this PR. Let me summarize:
Future:
@trociny @dillaman, please correct me if I have anything wrong. |
The plan LGTM |
@liupan1111 sounds good |
@trociny @dillaman , I committed a new version to disable unmap and list-mapped commands to create asok files. But for default asok path, I tried log file, if I don't specify it in ceph.conf, it will still output a log file, named ceph-client.admin.log(default name defined in config_opt.h).
So how about we asok file consistent with log file: create a asok file with default name and path? |
@liupan1111 I'm probably not understanding the question, but there is a default asok path ( |
src/tools/rbd_nbd/rbd-nbd.cc
Outdated
@@ -803,8 +803,10 @@ static int rbd_nbd(int argc, const char *argv[]) | |||
cmd = Connect; | |||
} else if (strcmp(*args.begin(), "unmap") == 0) { | |||
cmd = Disconnect; | |||
g_ceph_context->_conf->set_val_or_die("admin_socket", ""); |
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.
@dillaman just from line 801, it begins to check the command type: map, unmap or list-mapped. But global_init is called in line 761, which is before 801... I cannot just move this global_init after 801 line. So I just set admin_socket to empty here for unmap and list-mapped.
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.
@liupan1111 I don't really think there is any pressing requirement for invoking "global_init" before processing the command line. Let me know if that assumption is incorrect.
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.
@dillaman for example, if i call:
rbd-nbd --admin-socket=/temp/test.sock map rbd/image1,
there will have three elems in args:
1) --admin-socket=/temp/test.sock
2) map
3) rbd/image1
After global_init, --admin-socket=/temp/test.sock(elem1, also, args.begin), will be removed from args, and only map and rbd/image1 are left. In line 802, it compare args.begin() to check cmd type. So If we move global_init after processing the command line, some thing will be different ...
Please correct me.
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.
First argument that doesn't start with "-" is the command. You could even just quickly pre-scan the argument vector and if the first argument that doesn't start w/ a "-" is "map", then you know to use DAEMON, otherwise use UTILITY.
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.
cool 👍
@liupan1111 LGTM |
src/tools/rbd_nbd/rbd-nbd.cc
Outdated
} | ||
} | ||
|
||
auto cct = global_init(NULL, args, CEPH_ENTITY_TYPE_CLIENT, code_env, env_flag); |
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.
@liupan1111 Unfortunately, this does not work:
% sudo ./bin/rbd-nbd list-mapped
2017-02-02 16:50:01.947681 7fdb1cd93a00 -1 asok(0x560b55cc07d0) AdminSocketConfigObs::init: failed: AdminSocket::bind_and_listen: failed to bind the UNIX domain socket to '/home/mgolub/ceph.ci/build/out/client.admin.asok': (17) File exists/dev/nbd1
In global_init(), admin_socket is properly reset by common_preinit(), but then global_init() calls parse_config_files and it is set again.
This common init things make me mad.
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, our rbd
util is also affected: it creates admin socket file when is running.
Also interesting effect found during testing: if a socket is created by root but in a user directory, and then a rbd commad is run by the user, it removes the root's asok, because before binding, it checks if the socket is alive (and this fails when testing a root socket by a non-root), and it removes this socket as dead.
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.
@trociny, I tried rbd-nbd --admin-socket=/path/test.sock map rbd/test_image, and rbd-nbd list-mapped, and no error. I believe you tried the case which add admin-socket by config file. That is not hard to fix, I could add one line after global_init for list-mapped and unmap commands:
g_ceph_context->_conf->set_val_or_die("pid_file", "");
I believe this will resolve the issue.
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, our rbd util is also affected: it creates admin socket file when is running. Also interesting effect found during testing: if a socket is created by root but in a user directory, and then a rbd commad is run by the user, it removes the root's asok, because before binding, it checks if the socket is alive (and this fails when testing a root socket by a non-root), and it removes this socket as dead.
Yes! I also found this issue before, the only way to resolve it I can imagine is to make the asok file name with nbd index, or process id... What is your opinion?
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.
I would prefer we fixed global_init() instead of introducing additional hacks. I have no idea how easy it would be though. At least if would be good to report this to tracker.
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.
I think it would be ok to me if we left your current version: it fixes [1] for the default case (when admin socket
is not specified in ceph.conf). And if a user wanted to specify admin socket
it would still need to make it unique (e.g. by adding $pid) to resolve conflict between rbd-nbd map
processes. So with your current patch the behaviour is good enough, similar to what we have for rbd
util. But if you add additional safeguard to reset admin_socket
for not map commands, this will be ok for me to.
May be @dillaman has opinion
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.
@trociny Are you only seeing this because you have a vstart cluster and the "[client]" section has entries for "log file" and "admin socket"? If that is the case and it works as expected when there are no config settings, I'm fine w/ the behavior as it relates to this tracker ticket. I'd still love to see a pluggable metavariable system where apps could define custom metavariables like "$device", but that is for another PR.
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.
@dillaman one question, what is the mean of "vstart" cluster?
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.
@liupan1111 A cluster that was started via "vstart.sh": https://github.com/ceph/ceph/blob/master/src/vstart.sh#L549
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.
@dillaman Yes, I observe the issue when modifying vstart ceph.conf, changing the admin_socket variable not to contain $pid. I agree this is not a bug, though looks weird.
src/tools/rbd_nbd/rbd-nbd.cc
Outdated
code_env = CODE_ENVIRONMENT_DAEMON; | ||
env_flag = CINIT_FLAG_UNPRIVILEGED_DAEMON_DEFAULTS; | ||
} | ||
break; |
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.
@liupan1111 It does not work for command like below:
rbd-nbd --device /dev/nbd0 map rbd/testrbdnbd18270
You can test this with qa/workunits/rbd/rbd-nbd.sh
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.
@trociny a new version, and the issue is fixed by local test |
src/tools/rbd_nbd/rbd-nbd.cc
Outdated
code_environment_t code_env = CODE_ENVIRONMENT_UTILITY; | ||
int env_flag = 0; | ||
for (auto arg : args) { | ||
if (strcmp(arg, "map") == 0) { |
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.
Theoretically, "map" can be an image name, or a cluster name, when it is specified as --cluster map
. Right now this is still only useful for map command, so the program should behave properly, still it is not very nice.
Actually I don't mind if Jason agrees, but what if we do this way instead:
- Don't call global_init here, and filter out common ceph options from args using md_config_t::parse_argv():
md_config_t().parse_argv(args);
- Put argc and argv further to do_map(), and call global_init() there.
So global context initialization (and socket creation) would be only for "map" command.
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.
@trociny it is a good way to separate map and other commands to handle parameters differently. But at this moment, I prefer we leave the change and fix this bug, and modify this common global_init later.
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.
@liupan1111 I don't propose to modify global_init(). I propose to move it's call to do_map(). Right now we do this call so early just to filter out ceph common options, and it has turned out it does not work well for our case: we still need to do preprocessing manually introducing potential bugs.
Consider a situation, when a user has a cluster named "map". And to make her life easier, she just added --cluster map
to CEPH_ARGS
env variable. Your current patch does not work as expected for this case.
My approach looks a little nicer to me, still if Jason likes your patch, I will not have objection to merge 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.
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.
I prefer @trociny's approach as it seems less fragile
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.
@trociny , global_init will handle common args, and remove the argument start with"-" from args, so that the code behind global_init could just use args.begin():
if (strcmp(*args.begin(), "map") == 0)
If we just move global_init to do_map, how do we check args.begin()? maybe many changes...
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.
@liupan1111 Make a temporary copy of the argument vector, run md_config_t().parse_argv(args);
against the temporary copy to strip out common command-line optionals, and continue the existing processing in rbd_nbd
function using the (pruned) temporary copy. When you call do_map
, pass along the original (unmodified) argument vector so that global_init can re-process the command-line optionals.
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.
mm... let me try.
src/tools/rbd_nbd/rbd-nbd.cc
Outdated
g_ceph_context->_conf->set_val_or_die("pid_file", ""); | ||
|
||
vector<const char*> temp_args(args); | ||
md_config_t().parse_argv(temp_args); | ||
|
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.
nitpicking: I would prefer if you passed argc, argv to do_map() and removed env_to_vec(args)
in rbd_nbd(). So we wouldn't parse unnecessary the environment here and and there would not be unnecessary code modifications (s/args/temp_args/) below.
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.
@trociny two questions:
-
do you mean we would better move argc and argv into do_map instead of args, and we call argv_to_vec again in do_map?
-
is this env_to_vec unnecessary? I want to get more info about this. Do you mean Just remove it, or move it to do_map? If Just remove it, I prefer to add a seperate commit for this removal in this pr.
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.
- Yes
- I mean to move env_to_vec() to do_map.
env_to_vec is necessary but only for global context initialization. When parsing specific rbd-nbd command line arguments we should not use arguments provided via environment.
So, in rbd_nbd() you create and fill args
using only argv_to_vec() and then filter it out with mg_config_t::parse_argv(). In do_map() you create and fill args
using both arv_to_vec and env_to_vec.
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.
agree, thanks.
Fixes: http://tracker.ceph.com/issues/17951 Signed-off-by: Pan Liu <liupan1111@gmail.com>
@trociny , updated, please take a look. |
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.
lgtm (but haven't tested)
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.
👍
Fixes: http://tracker.ceph.com/issues/17951