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: introduce v2 format for rbd export/import. #10487

Merged
merged 25 commits into from Feb 19, 2017

Conversation

Projects
None yet
5 participants
@yangdongsheng
Member

yangdongsheng commented Jul 29, 2016

@yangdongsheng

This comment has been minimized.

Member

yangdongsheng commented Jul 29, 2016

@dillaman
This PR added this feature http://tracker.ceph.com/issues/13186 , please help to take a look. thanx.

@yangdongsheng

This comment has been minimized.

Member

yangdongsheng commented Jul 29, 2016

@dillaman I found there is still some bug in this PR, so it's not ready for code-review.

@yangdongsheng yangdongsheng changed the title from rbd: add --with-snaps to allow user to import or export snapshots at the same time with image to [RFC] rbd: add --with-snaps to allow user to import or export snapshots at the same time with image Jul 29, 2016

@yangdongsheng yangdongsheng changed the title from [RFC] rbd: add --with-snaps to allow user to import or export snapshots at the same time with image to rbd: add --with-snaps to allow user to import or export snapshots at the same time with image Aug 1, 2016

@yangdongsheng

This comment has been minimized.

Member

yangdongsheng commented Aug 1, 2016

@dillaman it passed my local manual testing now, please help to take a look. thanx

@@ -196,7 +196,7 @@ static int do_merge_diff(const char *first, const char *second,
}
}
//We just handle the case like 'banner, [ftag], [ttag], stag, [wztag]*,etag',
//We just handle the case like 'banner, [ftag], [ttag], [stag], [wztag]*, [etag]',

This comment has been minimized.

@dillaman

dillaman Aug 5, 2016

Contributor

I think this was an attempt to say that the "s" and "e" tags are not optional and the ones in the brackets are optional.

This comment has been minimized.

@yangdongsheng

yangdongsheng Aug 8, 2016

Member

right. 👍

@@ -762,5 +762,93 @@ std::string timestr(time_t t) {
return buf;
}
int export_diff(librbd::Image& image, const char *fromsnapname,

This comment has been minimized.

@dillaman

dillaman Aug 5, 2016

Contributor

I'd rather see a new file for the main export/import diff logic instead of putting it into Utils (for lack of a better place).

This comment has been minimized.

@yangdongsheng

yangdongsheng Aug 8, 2016

Member

I need to refer these functions in Export/Import and ExportDiff/ImportDiff. Then I put them in Utils. a new file? Such as src/tools/rbd/Diff.cc?

}
}
r = image.close();
err:
if (r < 0)
rbd.remove(io_ctx, imgname);

This comment has been minimized.

@dillaman

dillaman Aug 5, 2016

Contributor

I think the image would still be open in the goto case -- so this remove would fail.

This comment has been minimized.

@yangdongsheng

yangdongsheng Aug 8, 2016

Member

will close it first.

r = import_diff_fd(image, fd, no_progress);
close(fd);

This comment has been minimized.

@dillaman

dillaman Aug 5, 2016

Contributor

Don't close stdin (not that it really matters)

This comment has been minimized.

@yangdongsheng
@@ -86,12 +86,13 @@ class C_Export : public Context
SimpleThrottle &m_throttle;
librbd::Image &m_image;
bufferlist m_bufferlist;
uint64_t f_offset;

This comment has been minimized.

@dillaman

dillaman Aug 5, 2016

Contributor

The "m_" prefix is for instance variables -- if you want to distinguish this as the destination offset, name it "m_dest_offset"

This comment has been minimized.

@yangdongsheng
const char *last_snap = NULL;
for (size_t i = 0; i < snaps.size(); ++i) {
utils::snap_set(image, snaps[i].name.c_str());
r = utils::export_diff_fd(image, last_snap, snaps[i].name.c_str(), false, fd, true);

This comment has been minimized.

@dillaman

dillaman Aug 5, 2016

Contributor

I'd rather see export_diff_fd take a ProgressContext -- in this case, you would pass a derived ProgressContext that subdivides by the number of snapshots.

This comment has been minimized.

@yangdongsheng

yangdongsheng Aug 8, 2016

Member

There is already a parameter of no_progress in this function and this progress is subdivides by the number of extent in the diff from src_snap to dest_snap. And when we export-diff, it's more important how many extent different than how many snapshots between the two snapshots.

This comment has been minimized.

@dillaman

dillaman Aug 8, 2016

Contributor

Understood -- but my rationale is that if I have 10 snapshots on a 100G image, instead of having the progress bar smoothly progress, it will jump as each snapshot is completed. It would be nice to have the granular feedback.

@dillaman

This comment has been minimized.

Contributor

dillaman commented Aug 5, 2016

@yangdongsheng I'd recommend combining Export/ExportDiff and Import/ImportDiff into the same file now that they are going to be interdependent.

@dillaman

This comment has been minimized.

Contributor

dillaman commented Aug 5, 2016

@yangdongsheng It might be cleaner to just 100% re-use the existing export/import diff format for the export/import with snaps. Just create a new "v2" header to distinguish the new behavior that it might contain >1 snapshot segment. Then on import(diff), if it's a v2, it should keep looping when it gets to the end snapshot marker, then the next field should be a from snapshot marker, data, etc, end snapshot, etc.

goto done;
}
// seek back to the beginning.
lseek(fd, 0, SEEK_SET);

This comment has been minimized.

@dillaman

dillaman Aug 5, 2016

Contributor

Couldn't you just set "import_snaps" to false here and eliminate the need for new_format?

This comment has been minimized.

@yangdongsheng

yangdongsheng Aug 8, 2016

Member

Sorry, I am not sure what's your suggestion here, but I can explain the logic here more clear.

(1).import_snaps == true means --with-snaps is specified.
(2).(from_stdin == false )means importing something from file.

Then there are 3 possibility we will into this if statement.
(1). (import_snaps == true) and (from_stdin == true), in this case, when we found it's old format, because it's from stdin, we can't seek back, so return -EINVAL. That means, you have to make sure the you are accepting a new format importing stdin when you specify --with-snaps manually.

(2). (import_snaps == true) and (from_stdin == false), in this case, when we found it's a old format. we will lseek to the beginning, because it's from a fd. and the all data in file should be imported into image.

(3). (import_snaps == false) and (from_stdin == false), in this case, --with-snaps are not specified, and we are importing data from a file. then we try to check the banner at first, if we found it's old format. lseek to 0 to import it in old manner.

In this way, we can keep the backforwad compatability for the old format of export/exportdiff

@@ -132,6 +138,30 @@ static int do_import(librbd::RBD &rbd, librados::IoCtx& io_ctx,
#endif
}
if (import_snaps || !from_stdin) {
r = safe_read_exact(fd, image_buf, utils::RBD_IMAGE_BANNER.size());
if (r < 0)

This comment has been minimized.

@dillaman

dillaman Aug 8, 2016

Contributor

Not that anyone should have a very tiny image, but this would return -EDOM if the legacy format and import size < the image banner length.

This comment has been minimized.

@yangdongsheng

yangdongsheng Aug 9, 2016

Member

Aha, yes, good catch, actually I noticed and fixed this problem in my local tree yesterday. :) thanx.

// seek back to the beginning.
lseek(fd, 0, SEEK_SET);
} else {
r = safe_read_exact(fd, &size, 8);

This comment has been minimized.

@dillaman

dillaman Aug 8, 2016

Contributor

I'd hate to automatically switch to the new format based on the header -- I'd rather see a warning message displayed saying something along the lines of "image appears to contain snapshots, import with --with-snaps".

This comment has been minimized.

@yangdongsheng

yangdongsheng Aug 9, 2016

Member

Okey, sound much safer. after some consideration, I agree with you.

goto done;
image_buf[utils::RBD_IMAGE_BANNER.size()] = '\0';
if (strcmp(image_buf, utils::RBD_IMAGE_BANNER.c_str())) {
// Old format

This comment has been minimized.

@dillaman

dillaman Aug 8, 2016

Contributor

I'd prefer to see an error if "--with-snaps" specified on a legacy image.

This comment has been minimized.

@yangdongsheng

yangdongsheng Aug 9, 2016

Member

okey, similar with above comment, agree with you. We can make it working in a much simpler way.

@yangdongsheng

This comment has been minimized.

Member

yangdongsheng commented Sep 9, 2016

Hi @dillaman sorry for the late update. Please help to take a look again. thanx

@yangdongsheng

This comment has been minimized.

Member

yangdongsheng commented Sep 18, 2016

@dillaman do you have a time to take a look at this PR?

rbd info testimg_import
rbd info testimg_import@snap
rm ${TMPDIR}/img
rbd snap rm testimg_import@snap

This comment has been minimized.

@dillaman

dillaman Sep 18, 2016

Contributor

Compare the contents between snaps and image HEAD

This comment has been minimized.

@yangdongsheng
@@ -711,8 +711,11 @@ void ExclusiveLock<I>::handle_shutdown(int r) {
m_image_ctx.aio_work_queue->clear_require_lock_on_read();
m_image_ctx.aio_work_queue->unblock_writes();
m_image_ctx.image_watcher->flush(util::create_context_callback<
ExclusiveLock<I>, &ExclusiveLock<I>::complete_shutdown>(this));
if (m_image_ctx.image_watcher == nullptr)

This comment has been minimized.

@dillaman

dillaman Sep 18, 2016

Contributor

Why do you need to make any changes here?

This comment has been minimized.

@yangdongsheng

yangdongsheng Oct 10, 2016

Member

I got a segmentation fault without this change in "rbd export --with-snaps"

@@ -154,6 +213,8 @@ void get_arguments(po::options_description *positional,
at::add_path_options(positional, options,
"export file (or '-' for stdout)");
at::add_no_progress_option(options);
options->add_options()
("with-snaps", po::bool_switch(), "export snapshots at the same time");

This comment has been minimized.

@dillaman

dillaman Sep 18, 2016

Contributor

Minor: for some reason, "with-snaps" doesn't feel clear to me. Perhaps "--export-snapshots" or "--include-snapshots"

This comment has been minimized.

@yangdongsheng
@@ -857,5 +857,265 @@ std::string timestr(time_t t) {
return buf;
}
int export_diff_fd(librbd::Image& image, const char *fromsnapname,

This comment has been minimized.

@dillaman

dillaman Sep 18, 2016

Contributor

Minor: I would recommend combining Export.cc and ExportDiff.cc into the same file and then keeping the export_diff and export_diff_fd within the new combined Export.cc file. Same logic for Import.cc and ImportDiff.cc

This comment has been minimized.

@yangdongsheng
#include <string>
#include <boost/program_options.hpp>
namespace rbd {
namespace utils {
static const std::string RBD_IMAGE_BANNER ("rbd image v1\n");
static const std::string RBD_IMAGE_SNAPS_BANNER ("rbd image snaps v1\n");

This comment has been minimized.

@dillaman

dillaman Sep 18, 2016

Contributor

Minor: I can foresee wanted to eventually export other image attributes in addition to snapshots (e.g. features, metadata, etc). Therefore, perhaps it should be a v2 banner and then the export format should have a set of flag at the start that indicate what optional attributes have been included in the export (e.g. snapshots, features, etc).

This comment has been minimized.

@yangdongsheng
if (with_snaps) {
// header
bufferlist bl;
bl.append(utils::RBD_IMAGE_BANNER);

This comment has been minimized.

@dillaman

dillaman Sep 18, 2016

Contributor

Minor: new RBD_IMAGE_BANNER_V2

This comment has been minimized.

@yangdongsheng
}
if (r < 0) {
if (with_snaps) {

This comment has been minimized.

@dillaman

dillaman Sep 18, 2016

Contributor

I think you would first want to export the image starting at the first available snapshot via export_diff (i.e. only record the data that actually exists in the image), then iterating through all the available snapshots to generate diffs for each successive snapshot until you get to the specified head location. This would create a nice sparse export.

This comment has been minimized.

@yangdongsheng

yangdongsheng Oct 10, 2016

Member

sounds good.

This comment has been minimized.

@yangdongsheng

yangdongsheng Oct 10, 2016

Member

But then we can't import a image without snaps in this case.

rbd export --with-snaps test test.export
rbd import test.import test.export

I mean, sometimes, we can get a exported image with snapshots, but we only want to import a full image from it.

This comment has been minimized.

@yangdongsheng

yangdongsheng Oct 10, 2016

Member

So I name it as "with snaps" rather than "snaps mode". But yes, --include-snapshots sounds better. :)

if (from_stdin && (image_pos + (size_t)blklen) > size) {
size *= 2;
r = image.resize(size);
lseek(fd, size, SEEK_CUR);

This comment has been minimized.

@dillaman

dillaman Sep 18, 2016

Contributor

Seems like exported image has the full (non-sparse) image export which isn't used.

This comment has been minimized.

@yangdongsheng

yangdongsheng Oct 10, 2016

Member

Because this will be used when we import a image without --with-snaps. Sometimes, maybe we only want to import a full image even the snaps were exported already.

@dillaman

This comment has been minimized.

Contributor

dillaman commented Sep 18, 2016

@yangdongsheng Sorry -- the comments were sitting in "pending" state for days since I forgot to publish them.

@dillaman dillaman added the needs-doc label Sep 23, 2016

@yangdongsheng

This comment has been minimized.

Member

yangdongsheng commented Oct 11, 2016

@dillaman When I came back to refine this pr, I found maybe we should introduce a --format rather than --with-snaps. The v2 exporting can export not only content of image, but also snapshots, features, and some other priorities if necessary. When we are going to import an image with --format v2, we should check the BANNER at first, if fail, exit with an EINVAL.

Then there would be a big change on this branch, please wait for another review. Thanx.

@yangdongsheng yangdongsheng changed the title from rbd: add --with-snaps to allow user to import or export snapshots at the same time with image to rbd: introduce v2 format for rbd export/import. Oct 12, 2016

@yangdongsheng

This comment has been minimized.

Member

yangdongsheng commented Oct 13, 2016

@dillaman PR updated. Please help to take a look. changelog as below.
(1) squash ExportDiff.cc into Export.cc, same to Import.
(2) instead of --with-snaps, I introduce --import/export-format. The format v2 will export/import snapshots, image_order, image_format, features and so on.
(3) Currently only implement image_order and image_format. will continue for other priorities later.

@dillaman

This comment has been minimized.

Contributor

dillaman commented Oct 18, 2016

@yangdongsheng sorry for the delay, I will try to review this tomorrow. In the meantime, can you give your "fixup" commit a better commit title/message (or rebase and place the changes in the originating commit).

@@ -714,8 +714,11 @@ void ExclusiveLock<I>::handle_shutdown(int r) {
}
m_image_ctx.aio_work_queue->unblock_writes();
m_image_ctx.image_watcher->flush(util::create_context_callback<
ExclusiveLock<I>, &ExclusiveLock<I>::complete_shutdown>(this));
if (m_image_ctx.image_watcher == nullptr)

This comment has been minimized.

@dillaman

dillaman Oct 18, 2016

Contributor

What is this change? If you don't have an image watcher, you shouldn't have an ExclusiveLock either.

This comment has been minimized.

@yangdongsheng

yangdongsheng Oct 18, 2016

Member

But when I use the utils::snap_set(image, ""), it will create an ExclusiveLock for me. This case, I don't have a watcher but I need to shutdown it.

This comment has been minimized.

@yangdongsheng

yangdongsheng Oct 18, 2016

Member

@dillaman I found there are lots of assumption in ExclusiveLock about the image_watcher is not null. seems like the problem is in snap_set(). TBH, I am not clear about the relationship between exclusive_lock and watcher, is that 100% there is at list one watcher if we get an exclusive_lock? Could you help to explain it a bit. thanx a lot.

This comment has been minimized.

@yangdongsheng

yangdongsheng Oct 18, 2016

Member

Okey, If we open an image in readonly mode, then we will not create a watcher on it. In this case, if we snap_set(""), SetSnapRequest will send_init_exclusive_lock(). but we have no watcher on this image now. When we shutdown this exclusive-lock, we will get an null dereference error. Could you explain about the exclusive-lock design? should we prevent creating exclusive-lock for a readonly ictx?

This comment has been minimized.

@dillaman

dillaman Oct 19, 2016

Contributor

That is definitely a bug and I'll open a tracker ticket against it. The SetSnapRequest state machine should not initialize the exclusive lock when in read-only mode.

@yangdongsheng

This comment has been minimized.

Member

yangdongsheng commented Oct 18, 2016

@dillaman No problem, I will squash the fixup commit into the previous ones. And figure out why I need the wather related change. Please wait for tomorrow. Thanx a lot.

yangdongsheng added some commits Jul 28, 2016

rbd: remove image in error handling of importing
Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
rbd: finish the pc in importing even if from_stdin.
Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
rbd: introduce --import-format to import command
Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
rbd: introduce --export-format to export command
Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
qa/workunit/rbd: add test case for --import/export-format in import_e…
…xport.sh

Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
rbd: squash ExportDiff.cc into Export.cc
Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
rbd: squash ImportDiff.cc into Import.cc.
Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
rbd: move do_export_diff out from namespace of export_diff.
Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
rbd: move do_import_diff out from namespace of import_diff
Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
rbd: refactor do_export_diff to accept fd
Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
rbd: refactor do_import_diff to accept fd.
Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
rbd: import/export image_order.
Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
rbd: import/export features of image
Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
rbd: import/export image stripe_unit and stripe_count.
Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
qa/workunit/rbd: import_export.sh: add image priorities exporting and…
… importing test cases.

Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
rbd: use the unified --export-format option for rbd export and rbd im…
…port

Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
rbd: refactor export/import to v1 and v2 methods.
Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
rbd: import_diff/export_diff: encode length for each tag.
Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
rbd: combine the namespaces of export_diff and export_full, import_di…
…ff and import.

Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
rbd: add docs for new format of rbd export.
Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
rbd: import: use read() instead of lseek() when from_stdin is true
Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
rbd: import/export/mergediff: use STD[IN|OUT]_FILENO instead of 0 or 1
Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
rbd: import: decode file data in le64 for compatability on different …
…platform

Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>

@dillaman dillaman merged commit b2b1899 into ceph:master Feb 19, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Mar 30, 2017

@yangdongsheng and @dillaman

the workunit change breaks the jewel-x-singleton test, see http://pulpito.ceph.com/kchai-2017-03-30_14:32:23-rados-wip-13999-kefu---basic-mira/966061/

2017-03-30T14:41:21.411 INFO:tasks.workunit.client.0.mira071.stderr:+ rbd export --export-format 2 testimg /tmp/rbd_import_export_13580/img_v2
2017-03-30T14:41:21.434 INFO:tasks.workunit.client.0.mira071.stderr:rbd: unrecognised option '--export-format'
@dillaman

This comment has been minimized.

Contributor

dillaman commented Mar 30, 2017

@tchaikov Please open a tracker ticket -- this PR is already merged

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Mar 30, 2017

@dillaman sure, will do tomorrow. just wanted to give you guys a quick heads-up in case you run into this.

@dillaman

This comment has been minimized.

Contributor

dillaman commented Mar 30, 2017

@tchaikov It's a teuthology issue where the test case specifies that the "jewel" branch of the workunit should be used, but instead it downloads the "head" version (d463f52) of the workunit. Can't help but notice that particular commit changes the behavior of tasks/workunit.py so potentially this issue was just introduced?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment