rbd: unified way to map images using different drivers by trociny · Pull Request #19711 · ceph/ceph

trociny · 2017-12-28T20:19:50Z

Signed-off-by: Mykola Golub mgolub@suse.com

wjwithagen · 2017-12-28T22:32:21Z

Hi mykola,
Looks like a nice cleanup for more consistency
will run it thru FreeBSD-jenkins

wjwithagen

FreeBSD-jenkins thinks it is oke.
http://cephdev.digiware.nl:8180/jenkins/job/ceph-master/1605/console

trociny · 2017-12-29T20:42:46Z

rebased after #19679 was merged

liewegas · 2017-12-29T22:09:25Z

src/tools/rbd/action/Nbd.cc

 Shell::Action action_unmap(
  {"nbd", "unmap"}, {}, "Unmap a nbd device.", "",
-  &get_unmap_arguments, &execute_unmap);
+  &get_unmap_arguments, &execute_unmap_depricated, false);


nit: deprecated

Updated, thanks.

Also, the man page is updated.

dillaman

Perhaps also swap the workunit tests to use the new commands vs the newly deprecated commands

dillaman · 2018-01-02T15:47:47Z

src/tools/rbd/action/Nbd.cc

+     ("specify nbd device" + help_suffix).c_str())
+    ("nbds_max", po::value<std::string>(),
+     ("override module param nbds_max" + help_suffix).c_str())
+    ("max_part", po::value<std::string>(),


Nit: when invoked from Kernel.cc, it might be nice to change the optional to --nbd_(device|max_part|timeout) and only keep it --XYZ for the deprecated action. That way it's immediately clear which options are only valid for NBD maps.

May be reuse --options, which we already use for krbd? I.e.:

rbd device map -t krbd -o rw,nocrc,mount_timeout=10 ... rbd device map -t nbd -o nbd_device=256,timeout=10 ...

Sorry, for nbd I meant something like:

rbd device map -t nbd -o device=/dev/nbd1,max_part=256,timeout=10

@trociny That works for me -- just need to document the possible options somehow, though

Or we could make vice versa: for krbd, instead of:

rbd device map -t krbd -o rw,nocrc,mount_timeout=10 ...

have:

rbd device map -t krbd --krbd_rw --krbd_nocrc --krbd_mount_timeout=10 ...

I just would like we have this consistent between different device types.

Something about nearly a dozen --krbd_XYZ-prefixed optionals makes me prefer the older style instead even though the new style is self-documenting. As a third possibility, perhaps all the driver-specific options can be added to the help only after you specify the type so that they are only accepted as valid optionals when applicable to the current device type:

rbd help device map --device-type nbd ... snip ... snip ... snip ... Optional arguments --device arg specify nbd device + --nbds_max arg override module param nbds_max + --max_part arg override module param max_part + --timeout arg set nbd request timeout (seconds) +

@idryomov thoughts?

rbd help device map --device-type nbd looks too cumbersome to me. I'd vote for reusing -o/--options. It's not self-documenting, but we can make it so -- although personally I'd rather we improve rbd(8) man page.

dillaman · 2018-01-02T15:54:42Z

src/test/cli/rbd/help.t

      rename (mv)                       Rename image within pool.
      resize                            Resize (expand or shrink) image.
-      showmapped                        Show the rbd images mapped by the kernel.
+      showmapped                        Show mapped rbd images.


The showmapped command has always seemed oddly named to me. Perhaps since we are changing/merging, we can just create new "device map", "device list", and "device unmap" commands and deprecate all the older CLI actions(?)

@dillaman I like "device map|list|unmap" too. I was reluctant to change (deprecate) krbd commands though as in contrast to nbd/ggate they have long history, mentioned in many docs, and are very likely used in many scripts.

But if you and other think it is ok I will change this.

... I think if we keep it deprecated for two releases, that will leave plenty of time to adjust.

dillaman · 2018-01-02T15:55:34Z

src/tools/rbd/action/Map.cc

+
+Shell::Action action_map(
+  {"map"}, {}, "Map an image to a block device.",
+  "* kernel device specific option\n"


Nit: krbd instead of kernel since the other methods use the kernel as well

trociny · 2018-01-09T14:36:10Z

Updated:

rbd device map|unmap|list|options -t devtype commands added
device specific options passed as -o opt1,opt2=val,... (-o can be specified multiple types and the result is concatenated)
rbd device options is used to display device specific option.

I am using boost::program_options to parse and self document device options, though I am not very happy with the result:

Compatibility: Previously for krbd map the option string like this "share,noshare,share" would result in "share=share" passed to the kernel (share and noshare treated as the same option with different value and the last specified won). Now the boost parser will complain about "share" option specified twice (probably easy to fix if needed) and if you specify "noshare,share" the result passed to the kernel "share=noshare" ("noshare" always win).
rbd device options output looks like below:

Map options:
  --fsid arg                    cluster fsid (uuid)
  --ip arg                      ip and optionally port to bind (a.b.c.d[:p])
  --share                       enable sharing of client instances with other 
                                mappings (default)
  --noshare                     disable sharing of client instances with other 
                                mappings
  --crc                         enable CRC32C checksumming for data writes 
                                (default)
  --nocrc                       disable CRC32C checksumming for data writes 
                                (default)
  --cephx_require_signatures    require cephx message signing (since 3.19, 
                                default)
  --nocephx_require_signatures  do not require cephx message signing (since 
                                3.19)
  --tcp_nodelay                 disable Nagle's algorithm on client sockets 
                                (since 4.0, default)
  --notcp_nodelay               enable Nagle's algorithm on client sockets 
                                (since 4.0)
  --cephx_sign_messages         enable message signing (since 4.4, default)
  --nocephx_sign_messages       disable message signing (since 4.4)
  --mount_timeout arg           mount timeout (default is 60 seconds)
  --osdkeepalive arg            OSD keepalive timeout (default is 5 seconds)
  --osd_idle_ttl arg            OSD idle TTL (default is 60 seconds)
  --rw                          map the image read-write (default)
  --ro                          map the image read-only
  --queue_depth arg             queue depth (since 4.2, default is 128 
                                requests)
  --lock_on_read                acquire exclusive lock on reads (since 4.9)
  --exclusive                   disable automatic exclusive lock transitions 
                                (since 4.12)

Unmap options:
  --force               force unmapping of opened block device (since 4.9)

The things I don't like are two dashes prefix and a space instead of "=" as key-value separator. This might be confusing for users. It looks like it is hardcoded in boost::program_options option descriptions print function, and I would not like to write my own for this task.

So, what do you think? Is it a good idea to use boost::program_options for options parsing or should I implement my own simplified parser? Does rbd device options look useful and do I need to write my own options printer?

idryomov · 2018-01-09T15:22:34Z

The two dashes thing is going to be highly confusing. I think having map options documented in the man page is sufficient and wouldn't worry about rbd device options.

As for the parser, I don't have a strong opinion. The existing parse_map_options is simple, less verbose and be cut-and-pasted for the few options that nbd and ggate have with very little effort. If we agree to punt on rbd device options I don't see a good reason for the rewrite.

dillaman · 2018-01-09T15:32:31Z

I agree w/ Ilya. The valid options for all block types can just be dumped out as a raw text via long argument help (see get_long_features_help) since it just dumps it out at the end of the generated help.

Songweibin · 2018-01-10T00:57:47Z

doc/man/8/rbd.rst

-
-:command:`nbd unmap` *device-path*
-  Unmap the block device that was mapped via the rbd-nbd tool.
-


Commands map, unmap, showmapped should be dropped also.

@dillaman On that note, I don't really understand the need to deprecate rbd map, rbd unmap and rbd showmapped. Even if rbd showmapped is weirdly named, they are short, sweet and to the point ;) They also have been around forever, so if the new rbd device commands are added, I'd like to keep the old ones as synonyms.

One of the need to deprecate rbd showmapped was discussed in #19704: rbd showmapped produces invalid xml, so rbd device list produces a different output (see "krbd: add krbd_list and depricate krbd_showmapped function" commit in this PR).

And I like "device" prefix in the command as it groups these commands in one section when rbd commands are listed alphabetically. Probably we could keep rbd map|unmap|showmapped forever (without a deprecation warning, as aliases, still showmapped producing old format output) but to me in long term it might be more confusing to users.

Hmm, that's probably not the only place where an XML element name ends up starting with (or being) a number. I wonder if we should teach XMLFormatter to escape these as _xHHHH_ per https://msdn.microsoft.com/en-us/library/system.xml.xmlconvert.encodename(v=vs.110).aspx or similar.

A quick grep just within rbd revealed do_lock_list, which opens with a lock cookie, which is a free-form string. I'm not sure that's enough to deprecate a command in my book.

Honestly, the fact that is has been broken for so long probably tells us that no one is using the XML output from rbd showmapped. Encoding the id in hex would be backwards incompatible to any users out there. We've always stated that the output from the CLI is not guarenteed to be stable, so perhaps we just change it, release note it, and move on.

trociny · 2018-01-29T07:50:34Z

@dillaman @idryomov updated

dillaman

lgtm

trociny added feature rbd labels Dec 28, 2017

wjwithagen mentioned this pull request Dec 28, 2017

rbd-ggate: small fixes and improvements #19679

Merged

wjwithagen self-requested a review December 29, 2017 00:37

wjwithagen approved these changes Dec 29, 2017

View reviewed changes

liewegas added the needs-qa label Dec 29, 2017

trociny force-pushed the wip-20762 branch from 8f1950b to 572e35a Compare December 29, 2017 20:14

trociny requested a review from dillaman December 29, 2017 20:43

liewegas reviewed Dec 29, 2017

View reviewed changes

trociny force-pushed the wip-20762 branch from 572e35a to 76a63f8 Compare December 30, 2017 16:01

dillaman reviewed Jan 2, 2018

View reviewed changes

dillaman removed the needs-qa label Jan 3, 2018

trociny force-pushed the wip-20762 branch from 76a63f8 to 3c34bc8 Compare January 9, 2018 13:59

trociny changed the title ~~rbd: map should support other block devices~~ rbd: unified way to map images using different drivers Jan 9, 2018

Songweibin reviewed Jan 10, 2018

View reviewed changes

trociny mentioned this pull request Jan 12, 2018

rbd: adjusted "showmapped" JSON and XML formatted output #19937

Merged

trociny force-pushed the wip-20762 branch 2 times, most recently from 792cf02 to 690776c Compare January 28, 2018 08:27

trociny force-pushed the wip-20762 branch from 690776c to 6a57358 Compare January 29, 2018 07:35

dillaman approved these changes Jan 31, 2018

View reviewed changes

dillaman added needs-qa wip-jason-testing labels Jan 31, 2018

dillaman merged commit 3ac57a7 into ceph:master Feb 2, 2018

trociny deleted the wip-20762 branch February 2, 2018 19:17


		:command:`nbd unmap` device-path
		Unmap the block device that was mapped via the rbd-nbd tool.

Conversation

trociny commented Dec 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wjwithagen commented Dec 28, 2017

Uh oh!

wjwithagen left a comment

Choose a reason for hiding this comment

Uh oh!

trociny commented Dec 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dillaman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trociny Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trociny Jan 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trociny commented Jan 9, 2018

Uh oh!

idryomov commented Jan 9, 2018

Uh oh!

dillaman commented Jan 9, 2018

Uh oh!

Songweibin Jan 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trociny commented Jan 29, 2018

Uh oh!

dillaman left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

trociny commented Dec 28, 2017 •

edited

Loading

trociny Jan 3, 2018 •

edited

Loading

trociny Jan 2, 2018 •

edited

Loading

Songweibin Jan 10, 2018 •

edited

Loading