rbd: import with option --export-format fails to protect snapshot #20613
Conversation
There was a problem hiding this comment.
Can you also update [1]?
[1] https://github.com/ceph/ceph/blob/master/doc/dev/rbd-export.rst
| tag = RBD_SNAP_PROTECTION_STATUS; | ||
| encode(tag, bl); | ||
| bool is_protected = false; | ||
| r = image.snap_is_protected(endsnapname, &is_protected); |
src/tools/rbd/action/Export.cc
Outdated
| auto protect = boost::lexical_cast<std::string>(is_protected); | ||
| len = protect.length() + 4; | ||
| encode(len, bl); | ||
| encode(protect, bl); |
There was a problem hiding this comment.
Nit: just encode the is_protected boolean instead of a string of true / false
Songweibin
force-pushed
the
wip-23038
branch
2 times, most recently
from
c27f872
to
af0029a
Compare
|
@dillaman Updated, thanks. |
doc/dev/rbd-export.rst
Outdated
| Record the snapshot's protection status if `--export-format=2`. | ||
| - u8: 'p' | ||
| - le64: length of appending data (8) | ||
| - le64: snap protection status |
There was a problem hiding this comment.
Nit: bools are encoded as u8 integer (0 for false, 1 for true)
src/tools/rbd/action/Import.cc
Outdated
| static int get_snap_protection_status(ImportDiffContext *idiffctx, bool *is_protected) | ||
| { | ||
| int r; | ||
| char buf[sizeof(bool)]; |
src/tools/rbd/action/Import.cc
Outdated
| return r; | ||
| } | ||
|
|
||
| bufferlist bl; |
There was a problem hiding this comment.
Probably a bit overkill for a single bit -- perhaps just *is_protected = (buf[0] != 0)
There was a problem hiding this comment.
@dillaman Updated, thank you.
src/tools/rbd/action/Import.cc
Outdated
| @@ -400,6 +418,8 @@ int do_import_diff_fd(librados::Rados &rados, librbd::Image &image, int fd, | |||
| r = (r < 0) ? r : temp_r; // preserve original error | |||
| if (r == 0 && tosnap.length()) { | |||
| idiffctx.image->snap_create(tosnap.c_str()); | |||
| if (is_protected) | |||
There was a problem hiding this comment.
Nit: Sorry just noticed that the error code isn't propagated. Can you modify the snap_create line above to store the return value in r, change this to if (r == 0 && is_protected), and change the line below to r = ...snap_protect?
Fixes: http://tracker.ceph.com/issues/23038 Signed-off-by: songweibin <song.weibin@zte.com.cn>
|
@dillaman Done, please have a look, thanks. |
src/tools/rbd/action/Import.cc
Outdated
| r = idiffctx.image->snap_protect(tosnap.c_str()); | ||
| if (r < 0) | ||
| return r; | ||
| } else if (r < 0) { |
There was a problem hiding this comment.
Nit: no need since it finishes the context and returns the result below.
src/tools/rbd/action/Import.cc
Outdated
| if (r == 0 && is_protected) { | ||
| r = idiffctx.image->snap_protect(tosnap.c_str()); | ||
| if (r < 0) | ||
| return r; |
There was a problem hiding this comment.
Yeah, already dropped. Thanks.
|
Hi @Songweibin , sorry for the late, but please consider to add a case in https://github.com/ceph/ceph/blob/master/qa/workunits/rbd/import_export.sh for this. |
|
@yangdongsheng Fine, will add later. Thanks. |
|
rbd/test: add snap protection test for ex/import: #20689 |
Fixes: http://tracker.ceph.com/issues/23038
@dillaman I'm not sure if this is the most correct way to fix it, please help to have a look.
Thank you in advance.
Signed-off-by: songweibin song.weibin@zte.com.cn