librbd: warning message for mirroring pool option #12319
Conversation
gaurav36
force-pushed
the
bug-18125-rbd-mirror-enable-disable
branch
from
e899603
to
00c1b81
Compare
| @@ -2862,10 +2864,16 @@ int mirror_image_disable_internal(ImageCtx *ictx, bool force, | |||
| } | |||
|
|
|||
| if (next_mirror_mode == cls::rbd::MIRROR_MODE_IMAGE) { | |||
| if (current_mirror_mode == cls::rbd::MIRROR_MODE_POOL) { | |||
There was a problem hiding this comment.
I would actually put this and the next message within the rbd CLI prefixed with "note: " instead of "warning: " (since it isn't technically wrong).
There was a problem hiding this comment.
Thanks, will prefix with note in next PR.
I can put this within CLI but its seems to be costly operation. because if i put this within execute_enable and execute_disable function than i need to initialize io_ctx for getting current_mirror_mode (cls_client::mirror_mode_get(&io_ctx, ¤t_mirror_mode)). if i put this within execute_enable_disable than i need to compare it with two type of enum one is rbd_mirror_mode_t and second one is cls::rbd::MirrorMode for checking current and next mirror mode, which doesn't seems to be good because if in future if we change any one of the enum sequence than it might create problem. Any how it can be done at cli side
@dillaman should i stick with this method or do change within cli, thoughts ?
There was a problem hiding this comment.
These are already part of the public librbd API: https://github.com/ceph/ceph/blob/master/src/include/rbd/librbd.hpp#L149
There was a problem hiding this comment.
@dillaman ahhh right, thanks. Will send updated PR with change in CLI.
| @@ -2833,6 +2833,8 @@ int mirror_image_disable_internal(ImageCtx *ictx, bool force, | |||
| } | |||
|
|
|||
| if (current_mirror_mode == next_mirror_mode) { | |||
| lderr(cct)<<"warning: mirroring on pool is already set to " | |||
There was a problem hiding this comment.
Thanks, without warning it would be better. will remove it in next PR.
gaurav36
force-pushed
the
bug-18125-rbd-mirror-enable-disable
branch
from
00c1b81
to
9e578bd
Compare
| @@ -259,16 +259,48 @@ void get_enable_arguments(po::options_description *positional, | |||
| } | |||
|
|
|||
| int execute_enable_disable(const std::string &pool_name, | |||
| rbd_mirror_mode_t mirror_mode) { | |||
| const std::string mode) { | |||
There was a problem hiding this comment.
right, thanks, it should be by reference. Will do change in next PR
|
|
||
| if (current_mirror_mode == next_mirror_mode) { | ||
| std::cout << "mirroring on pool is already set to " | ||
| << mode << std::endl; |
There was a problem hiding this comment.
Nit 1) when (re)disabling mirroring, this sentence won't be grammatically correct ("mirroring on pool is already set to disable") -- it would need to be "... is already disabled".
Nit 2) if no update, you can return here and skip the update call.
There was a problem hiding this comment.
@dillaman Thank you, will do change in Next PR
gaurav36
force-pushed
the
bug-18125-rbd-mirror-enable-disable
branch
from
9e578bd
to
9a09b24
Compare
| rbd_mirror_mode_t next_mirror_mode; | ||
| rbd_mirror_mode_t current_mirror_mode; | ||
|
|
||
| if (mode == "disable") { |
There was a problem hiding this comment.
Minor: perhaps just pass in both the string and the enum so that you don't need to move this logic here (and so that "rbd mirror pool enable disable" isn't a valid selection).
There was a problem hiding this comment.
@dillaman yeah, i will pass it in next PR. I was having same thing in my mind previously but i thought we can save passing extra argument as a string so moved logic here form execute_enable function. rbd mirror pool enable disable is taking care by L+275
| } | ||
|
|
||
| if (current_mirror_mode == next_mirror_mode) { | ||
| std::cout << "mirroring mode on pool is already " << mode << std::endl; |
There was a problem hiding this comment.
This is still lightly off grammar-wise. If disabling, the text should be something like "mirroring is already disabled", otherwise it can be something like "mirroring is already configured for mode".
There was a problem hiding this comment.
will do it in next PR.
gaurav36
force-pushed
the
bug-18125-rbd-mirror-enable-disable
branch
from
9a09b24
to
60a1ccd
Compare
|
|
||
| if (current_mirror_mode == next_mirror_mode) { | ||
| if (mode == "disabled") { | ||
| std::cout << "mirroring is already "<< mode << std::endl; |
There was a problem hiding this comment.
Nit: space between already " and << mode
| if (mode == "disabled") { | ||
| std::cout << "mirroring is already "<< mode << std::endl; | ||
| } else { | ||
| std::cout << "mirroring is already configured for mode " |
There was a problem hiding this comment.
Nit: "mirroring is already configured for " << mode << " mode"
When user enable/disable mirroring option on a pool, than re-enabling/disabling mirroring option should give warning message if it is already enable/disable. It should also give warning when changing the mode from pool to image or vice versa. Fixes: http://tracker.ceph.com/issues/18125 Reported-by: Jason Dillaman <dillaman@redhat.com> Signed-off-by: Gaurav Kumar Garg <garg.gaurav52@gmail.com>
gaurav36
force-pushed
the
bug-18125-rbd-mirror-enable-disable
branch
from
60a1ccd
to
a0379f0
Compare
When user enable/disable mirroring option on a pool, than
re-enabling/disabling mirroring option should give warning
message if it is already enable/disable.
It should also give warning when changing the mode from pool
to image or vice versa.
Fixes: http://tracker.ceph.com/issues/18125
Reported-by: Jason Dillaman dillaman@redhat.com
Signed-off-by: Gaurav Kumar Garg garg.gaurav52@gmail.com