rbd-nbd: fix generic option issue #17375

liupan1111 · 2017-08-30T13:41:12Z

This issue is introduced by #12433

I also changed the name of asok by adding nbd name, so that the user could easily get perf information.

Fixes: http://tracker.ceph.com/issues/20426
Signed-off-by: Pan Liu wanjun.lp@alibaba-inc.com

Signed-off-by: Pan Liu <wanjun.lp@alibaba-inc.com>

liupan1111 · 2017-08-30T13:44:52Z

@trociny please help take a look. I tried qa/workunits/rbd/rbd_nbd.sh and it works fine.

liupan1111 · 2017-08-30T15:56:53Z

retest This please

trociny · 2017-08-30T16:41:10Z

src/tools/rbd_nbd/rbd-nbd.cc

+  }
+
+  auto cct = global_init(NULL, args, CEPH_ENTITY_TYPE_CLIENT, code_env, env_flag);
+  g_ceph_context->_conf->set_val_or_die("pid_file", "");


@liupan1111 I am not very happy you returned global_init back to rbd_nbd, that is run for all commands. We had some reasons to call it for "map" command only: it made the code cleaner avoiding e.g. manual command line parsing as you added above to check if it is "map" command.

Actually, the problem is that parse_args, which has to filter out all ceph common options before parsing nbd specific options, does not filter out "-c" like options. These can be filtered out by ceph_argparse_early_args. So if you just add something like below at the beginning of parse_args:

std::string conf_file_list; std::string cluster; CephInitParameters iparams = ceph_argparse_early_args( args, CEPH_ENTITY_TYPE_CLIENT, &cluster, &conf_file_list);

I think it should fix the issue.

The only potential problem with not calling global_init for non "map" commands is that if someone specifies "-c" like option for such a command it will be just ignored. This may have undesirable effect e.g. if the user has different default pool specified in config, and then "list" command will output mapped with wrong default pool. Still I prefer this solution.

yes! This method is much better. Thanks.

trociny · 2017-08-30T16:44:40Z

src/tools/rbd_nbd/rbd-nbd.cc

+  env_to_vec(args);
+
+  for (auto &temp_args : args) {
+	if (strcmp(temp_args, "map") == 0) {


We discussed issues with such manual parsing earlier. Consider a case when a user has a pool named "map" and specified --rbd-default-pool map in the command line.

It thought about it, but I didn't think it is a bug:
For map command, the user must input "rbd-nbd map". Even for the case "--rbd-default-pool map", it should be "rbd-nbd map --rbd-default-pool map", so I don't think it will break anything.

for other command, such as unmap, list-mapped, no pool/image should be input.

trociny · 2017-08-30T16:55:43Z

src/tools/rbd_nbd/rbd-nbd.cc

@@ -655,6 +658,11 @@ static int do_map(int argc, const char *argv[], Config *cfg)
    read_only = 1;
  }

+  change_admin_socket(index);


We discussed this earlier too. After this change the users lost a possibility to change admin socket path via config. I like the idea to have "nbdX" in the socket name so actually I don't mind. But as far as I recall Jason did not like this idea and suggested to extend config implementation to support user defined (client specific) meta variables so we could use e.g. {nbd} meta variable in admin_socket config option.

@dillaman what do you think?

Indeed -- I don't want to take a step backwards and have rbd-nbd automagically override configuration settings. I think a plugable set of metavariables would be helpful in other scenarios as well (but that shouldn't be part of this PR).

Actually, I think allowing users to override admin_socket is very important. Because now they can map images with a command like below:

rbd-nbd --admin-socket=/tmp/${image_name}.asok map ${image_name}

And would lose this possibility after your change. You could change your code to change admin socket only if it is not specified, then I would not mind, but I would like to see what Jason thinks.

@trociny I think it is hard for the user to specify it:

As you said, ${image_name}, but for now, we allow the user to map an image more than once in same client, and it is useful in our apps. So ${image_name} could not be used. I also consider pid, but also not workable, because in "do_map", main process will be forked.

I used "nbdX" in our product, and the feedback from our customer is good.

I can understand your concern, you may want to give flexibility. I could remove this commit from this PR.

liupan1111 · 2017-09-03T04:17:27Z

@trociny updated, thanks.

liupan1111 · 2017-09-03T09:32:38Z

retest This please

amitkumar50 · 2017-09-04T10:17:46Z

src/tools/rbd_nbd/rbd-nbd.cc

@@ -917,6 +917,11 @@ static int do_list_mapped_devices()

 static int parse_args(vector<const char*>& args, std::ostream *err_msg, Config *cfg)
 {
+  std::string conf_file_list;
+  std::string cluster;
+  CephInitParameters iparams = ceph_argparse_early_args(


How we are using this local variable in this function?

@amitkumar50 thanks for comment. As @trociny said, I will use it later.

trociny · 2017-09-04T10:55:53Z

src/tools/rbd_nbd/rbd-nbd.cc

+  std::string conf_file_list;
+  std::string cluster;
+  CephInitParameters iparams = ceph_argparse_early_args(
+          args, CEPH_ENTITY_TYPE_CLIENT, &cluster, &conf_file_list);


@liupan1111 It would be nice to pass the results of ceph_argparse_early_args to config, i.e:

set config.name = iparams.name

set config.cluster = cluster

if conf_file_list is not empty, pass it to config.parse_config_files instead of nullptr.

This should make "rbd_default_pool" config option to be used properly when it is changed in config.

liupan1111 · 2017-09-04T11:58:58Z

@trociny updated, please help take a look.

trociny

otherwise LGTM

trociny · 2017-09-04T13:03:54Z

src/tools/rbd_nbd/rbd-nbd.cc

+  std::string cluster;
+  CephInitParameters iparams = ceph_argparse_early_args(
+          args, CEPH_ENTITY_TYPE_CLIENT, &cluster, &conf_file_list);
+


nit: unnecessary empty line

Fixes: http://tracker.ceph.com/issues/20426 Signed-off-by: Pan Liu <wanjun.lp@alibaba-inc.com>

liupan1111 · 2017-09-04T14:18:30Z

@trociny done.

liupan1111 · 2017-09-05T13:28:21Z

@trociny could this pr be merged? thanks.

rbd-nbd: lower down the log level when return -EINVAL in aio_callback.

dcd7e5b

Signed-off-by: Pan Liu <wanjun.lp@alibaba-inc.com>

liupan1111 added rbd bug-fix labels Aug 30, 2017

trociny reviewed Aug 30, 2017

View reviewed changes

amitkumar50 reviewed Sep 4, 2017

View reviewed changes

trociny reviewed Sep 4, 2017

View reviewed changes

rbd-nbd: fix generic option issue

16f64a4

Fixes: http://tracker.ceph.com/issues/20426 Signed-off-by: Pan Liu <wanjun.lp@alibaba-inc.com>

trociny approved these changes Sep 6, 2017

View reviewed changes

trociny merged commit b1e9cab into ceph:master Sep 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rbd-nbd: fix generic option issue #17375

rbd-nbd: fix generic option issue #17375

liupan1111 commented Aug 30, 2017 •

edited

liupan1111 commented Aug 30, 2017

liupan1111 commented Aug 30, 2017

trociny Aug 30, 2017

liupan1111 Sep 3, 2017

trociny Aug 30, 2017

liupan1111 Sep 2, 2017

liupan1111 Sep 2, 2017

trociny Aug 30, 2017

dillaman Aug 30, 2017

trociny Aug 30, 2017

liupan1111 Sep 2, 2017 •

edited

liupan1111 commented Sep 3, 2017

liupan1111 commented Sep 3, 2017

amitkumar50 Sep 4, 2017

liupan1111 Sep 4, 2017

trociny Sep 4, 2017

liupan1111 Sep 4, 2017

liupan1111 commented Sep 4, 2017

trociny left a comment

trociny Sep 4, 2017

liupan1111 Sep 4, 2017

liupan1111 commented Sep 4, 2017

liupan1111 commented Sep 5, 2017

rbd-nbd: fix generic option issue #17375

rbd-nbd: fix generic option issue #17375

Conversation

liupan1111 commented Aug 30, 2017 • edited

liupan1111 commented Aug 30, 2017

liupan1111 commented Aug 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liupan1111 Sep 2, 2017 • edited

Choose a reason for hiding this comment

liupan1111 commented Sep 3, 2017

liupan1111 commented Sep 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liupan1111 commented Sep 4, 2017

trociny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liupan1111 commented Sep 4, 2017

liupan1111 commented Sep 5, 2017

liupan1111 commented Aug 30, 2017 •

edited

liupan1111 Sep 2, 2017 •

edited