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

rbd: unified way to map images using different drivers #19711

Merged
merged 1 commit into from Feb 2, 2018

Conversation

Projects
None yet
6 participants
@trociny
Copy link
Contributor

commented Dec 28, 2017

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

@wjwithagen

This comment has been minimized.

Copy link
Contributor

commented Dec 28, 2017

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

@wjwithagen
Copy link
Contributor

left a comment

@liewegas liewegas added the needs-qa label Dec 29, 2017

@trociny trociny force-pushed the trociny:wip-20762 branch from 8f1950b to 572e35a Dec 29, 2017

@trociny

This comment has been minimized.

Copy link
Contributor Author

commented Dec 29, 2017

rebased after #19679 was merged

@trociny trociny requested a review from dillaman Dec 29, 2017


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

This comment has been minimized.

Copy link
@liewegas

liewegas Dec 29, 2017

Member

nit: deprecated

This comment has been minimized.

Copy link
@trociny

trociny Dec 30, 2017

Author Contributor

Updated, thanks.

Also, the man page is updated.

@trociny trociny force-pushed the trociny:wip-20762 branch from 572e35a to 76a63f8 Dec 30, 2017

@dillaman
Copy link
Contributor

left a comment

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

("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>(),

This comment has been minimized.

Copy link
@dillaman

dillaman Jan 2, 2018

Contributor

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.

This comment has been minimized.

Copy link
@trociny

trociny Jan 3, 2018

Author Contributor

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   ...

This comment has been minimized.

Copy link
@trociny

trociny Jan 3, 2018

Author Contributor

Sorry, for nbd I meant something like:

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

This comment has been minimized.

Copy link
@dillaman

dillaman Jan 3, 2018

Contributor

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

This comment has been minimized.

Copy link
@trociny

trociny Jan 3, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@dillaman

dillaman Jan 3, 2018

Contributor

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?

This comment has been minimized.

Copy link
@idryomov

idryomov Jan 8, 2018

Contributor

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.

object-map check Verify the object map is correct.
object-map rebuild Rebuild an invalid object map.
pool init Initialize pool for use by RBD.
remove (rm) Delete an image.
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.

This comment has been minimized.

Copy link
@dillaman

dillaman Jan 2, 2018

Contributor

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(?)

This comment has been minimized.

Copy link
@trociny

trociny Jan 2, 2018

Author Contributor

@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.

This comment has been minimized.

Copy link
@dillaman

dillaman Jan 2, 2018

Contributor

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


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

This comment has been minimized.

Copy link
@dillaman

dillaman Jan 2, 2018

Contributor

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

@dillaman dillaman removed the needs-qa label Jan 3, 2018

@trociny trociny force-pushed the trociny:wip-20762 branch from 76a63f8 to 3c34bc8 Jan 9, 2018

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

@trociny

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2018

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.


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

This comment has been minimized.

Copy link
@Songweibin

Songweibin Jan 10, 2018

Contributor

Commands map, unmap, showmapped should be dropped also.

This comment has been minimized.

Copy link
@idryomov

idryomov Jan 10, 2018

Contributor

@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.

This comment has been minimized.

Copy link
@trociny

trociny Jan 10, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@idryomov

idryomov Jan 10, 2018

Contributor

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.

This comment has been minimized.

Copy link
@idryomov

idryomov Jan 10, 2018

Contributor

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.

This comment has been minimized.

Copy link
@dillaman

dillaman Jan 10, 2018

Contributor

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 trociny force-pushed the trociny:wip-20762 branch 2 times, most recently from 792cf02 to 690776c Jan 27, 2018

rbd: unified way to map images using different drivers
Instead of "rbd map|unmap|showmapped", "rbd ndb map|unmap|list",
"rbd ggate map|unmap|list" commands, provide:

 rbd device -t krbd|nbd|ggate|... map|unmap|list

which gives interface consistent between drivers.

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

@trociny trociny force-pushed the trociny:wip-20762 branch from 690776c to 6a57358 Jan 29, 2018

@trociny

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2018

@dillaman
Copy link
Contributor

left a comment

lgtm

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

5 checks passed

Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details

@trociny trociny deleted the trociny:wip-20762 branch Feb 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.