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: export/import image-meta when we export/import an image #17134

Merged
merged 1 commit into from Oct 17, 2017

Conversation

Projects
None yet
4 participants
@PCzhangPC
Copy link

commented Aug 22, 2017

when we do exporting/importing an image, we should export or import the image-meta of this image at the same time

Signed-off-by: PCzhangPC pengcheng.zhang@easystack.cn

@yangdongsheng yangdongsheng added the rbd label Aug 22, 2017

@PCzhangPC

This comment has been minimized.

Copy link
Author

commented Aug 22, 2017

@yangdongsheng please help to review this, thx

@@ -79,10 +79,18 @@ if rbd help export | grep -q export-format; then
dd if=/bin/dd of=${TMPDIR}/img bs=1k count=10 seek=100
rbd import $RBD_CREATE_ARGS ${TMPDIR}/img testimg
rbd snap create testimg@snap
rbd image-meta set testimg key1 value1
KEY=`rbd image-meta list testimg|grep key1`
VALUE=`rbd image-meta list testimg|grep value1`

This comment has been minimized.

Copy link
@yangdongsheng

yangdongsheng Aug 22, 2017

Member

The $KEY and $VALUE seems refer to the same line. right? because the key1 and value1 should be outputted in the same line.

r = read_tag(fd, RBD_EXPORT_IMAGEMETA_END, import_format, &tag, &length);

if (r < 0 || tag == RBD_EXPORT_IMAGEMETA_END) {
break;

This comment has been minimized.

Copy link
@yangdongsheng

yangdongsheng Aug 22, 2017

Member

if r < 0, this break will lead to return 0 in line 620.

@PCzhangPC PCzhangPC force-pushed the PCzhangPC:metaexin branch from ac84857 to 77cc6a1 Aug 22, 2017

rbd export --export-format 2 testimg ${TMPDIR}/img_v2
rbd import --export-format 2 ${TMPDIR}/img_v2 testimg_import
rbd info testimg_import
rbd info testimg_import@snap
IMP_KEY=`rbd image-meta list testimg_import|grep key1`

This comment has been minimized.

Copy link
@yangdongsheng

yangdongsheng Aug 22, 2017

Member

I prefer to check the whole output of image-meta list rather than check a specified one.

@PCzhangPC PCzhangPC force-pushed the PCzhangPC:metaexin branch from 77cc6a1 to 920cd3e Aug 22, 2017

@PCzhangPC

This comment has been minimized.

Copy link
Author

commented Aug 22, 2017

update already,please help to review @yangdongsheng @dillaman @joscollin

@PCzhangPC PCzhangPC force-pushed the PCzhangPC:metaexin branch 3 times, most recently from 53f0c9a to a1efca1 Aug 22, 2017

@yangdongsheng

This comment has been minimized.

@PCzhangPC PCzhangPC force-pushed the PCzhangPC:metaexin branch 2 times, most recently from 1bc7eab to 5238d9d Aug 23, 2017

@PCzhangPC

This comment has been minimized.

Copy link
Author

commented Aug 23, 2017

@yangdongsheng update already

@PCzhangPC PCzhangPC force-pushed the PCzhangPC:metaexin branch from 5238d9d to f5b965c Aug 23, 2017

#define RBD_EXPORT_IMAGEMETA_KEY 'K'
#define RBD_EXPORT_IMAGEMETA_VALUE 'V'
#define RBD_EXPORT_IMAGEMETA_END 'D'

This comment has been minimized.

Copy link
@yangdongsheng

yangdongsheng Aug 23, 2017

Member

I prefer to put the image-meta related into header. That means we have only two sections in the exported file, HEADER and DIFFs. So we can put the metadatas into HEADER section.

This comment has been minimized.

Copy link
@PCzhangPC

PCzhangPC Aug 23, 2017

Author

sounds good. and update already

@PCzhangPC PCzhangPC force-pushed the PCzhangPC:metaexin branch 2 times, most recently from 72f30ea to a03da79 Aug 23, 2017

@@ -555,7 +555,45 @@ static int decode_and_set_image_option(int fd, uint64_t imageopt, librbd::ImageO
return 0;
}

static int do_import_header(int fd, int import_format, uint64_t &size, librbd::ImageOptions& opts)
static int read_imagemeta_key_or_value(int fd, string &val, uint64_t length)

This comment has been minimized.

Copy link
@yangdongsheng

yangdongsheng Aug 23, 2017

Member

can we reuse util::read_string() for it?

This comment has been minimized.

Copy link
@PCzhangPC

PCzhangPC Aug 23, 2017

Author

update already, please review this again

return r;
}

if (!pairs.empty()) {

This comment has been minimized.

Copy link
@vshankar

vshankar Aug 23, 2017

Contributor

nit: unneeded check

This comment has been minimized.

Copy link
@PCzhangPC

PCzhangPC Aug 23, 2017

Author

ok,will update soon,thx

return r;
}

if (!pairs.empty()) {

This comment has been minimized.

Copy link
@vshankar

vshankar Aug 23, 2017

Contributor

same nit as above

@PCzhangPC PCzhangPC force-pushed the PCzhangPC:metaexin branch 4 times, most recently from 11069f4 to 02824d3 Aug 23, 2017

} else if (tag == RBD_EXPORT_IMAGE_META_KEY) {
r = utils::read_string(fd, 4096, &key);
} else if (tag == RBD_EXPORT_IMAGE_META_VALUE) {
r = utils::read_string(fd, 4096, &value);

This comment has been minimized.

Copy link
@yangdongsheng

yangdongsheng Aug 23, 2017

Member

4096? or length?

@PCzhangPC PCzhangPC force-pushed the PCzhangPC:metaexin branch from 02824d3 to 32a718a Aug 23, 2017

@PCzhangPC

This comment has been minimized.

Copy link
Author

commented Aug 23, 2017

@ceph-jenkins retest this please

@yangdongsheng

This comment has been minimized.

Copy link
Member

commented Aug 24, 2017

Hi @dillaman This looks good to me. what's your opinion?

@PCzhangPC

This comment has been minimized.

Copy link
Author

commented Aug 29, 2017

@dillaman please help to review this pr too, thx

@dillaman dillaman added the feature label Aug 29, 2017

@@ -65,6 +65,20 @@ Image Stripe count
- le64: length of appending data (8)
- le64: image striping count

ImageMeta Key

This comment has been minimized.

Copy link
@dillaman

dillaman Aug 29, 2017

Contributor

Image-meta should be combined into a single tag -- so perhaps the 'M' tag (for image-meta), le64 for tag length as usual, then the string key followed by the string value.

This comment has been minimized.

Copy link
@PCzhangPC

PCzhangPC Aug 31, 2017

Author

what you suggest is that we should combined all key-value paires into one tag? or one tag for one pair which contains key and value together?

//encode imageMeta key and value
std::map<std::string, bufferlist> imagemetas;

r = image.metadata_list("", 0, &imagemetas);

This comment has been minimized.

Copy link
@dillaman

dillaman Aug 29, 2017

Contributor

This needs to be a loop -- read X key/value pairs at a time and continue the loop if the number of retrieved pairs equals the request count.

This comment has been minimized.

Copy link
@PCzhangPC

PCzhangPC Aug 30, 2017

Author

no need to be a loop in fact. image.metadata_list("", 0, &imagemetas) will fetch all key-value pairs into the map imagemetas if we set the second parameter as 0. Or am i missing sth ?

This comment has been minimized.

Copy link
@dillaman

dillaman Aug 30, 2017

Contributor

It works for a large number of keys but not all possible (worse-case): https://github.com/ceph/ceph/blob/master/src/cls/rbd/cls_rbd.cc#L2654

This comment has been minimized.

Copy link
@yangdongsheng

yangdongsheng Aug 30, 2017

Member

Hi @dillaman , if we specify the max_return to 0. I think the code you pointed is trying to read all metadatas in a loop until there is no more.

This comment has been minimized.

Copy link
@dillaman

dillaman Aug 30, 2017

Contributor

It returns a maximum of 64 keys if zero is provided

This comment has been minimized.

Copy link
@yangdongsheng

yangdongsheng Aug 30, 2017

Member

Thanx @dillaman , will open a PR for this.

This comment has been minimized.

Copy link
@PCzhangPC

PCzhangPC Aug 31, 2017

Author

@dillaman so, what should be done are as follow:

  1. fix the bug you mention, make more indicate a partial set being retrieved.
  2. do some changes wherever the cls_cxx_map_get_vals function is being used in cls_rbd.cc : realize a new logic to fetch all metadates instead of using more as a loop flag

This comment has been minimized.

Copy link
@dillaman

dillaman Sep 1, 2017

Contributor

I'm not sure I am following your last commit:
(1) the tracker ticket is to just update the rbd CLI to iterate over partial sets -- the more out parameter on cls_cxx_map_get_XYZ already indicates that the OSD had to truncate the result but the requested entry limit has not been reached. See the osd_max_omap_bytes_per_request configuration value that can cause the truncation of the results.
(2) The more boolean should already be handled everywhere. The issue in this PR is that you need to iterate over image.metadata_list until the returned set is less than the request max entries.

This comment has been minimized.

Copy link
@PCzhangPC

PCzhangPC Sep 4, 2017

Author

hi @dillaman ,there are sth that make me confused.

  1. image.metadata_list() is not broken currently. I have tried 300 metadates and succeed to get all. (i think it is the 'bug' that makes it work)
  2. if we fix the bug you mentioned , which is that more should be false if we tell cls_cxx_map_get_vals to retrieve 5 entries and it returns 5 (am i right ?). But, please consider about this: we have 100 metadates and want to get all. The first time cls_cxx_map_get_vals succeeds to fetch 64 pairs(RBD_MAX_KEYS_READ) and more is set as false, so it will not go to fetch the remaining pairs, which means image.imagemeta_list() would be broken after fix. And that's why i said "realize a new logic to fetch all metadates instead of using more as a loop flag" in my last comment. And so, there are some other logic that use cls_cxx_map_get_vals to fetch things need to be changed.
  3. about "iterate over image.metadata_list": no matter how many time image.metadata_list() we call, the result of each time will be repeated. image.metadata_list() will start from the ‘first‘ pair but not from the end of last function call to get metadates.

This comment has been minimized.

Copy link
@dillaman

dillaman Sep 5, 2017

Contributor

OK -- looks like that was recently broken and is a totally separate issue. I'll push a fix for the broken handling of more [1].

[1] http://tracker.ceph.com/issues/21247

@@ -555,7 +555,28 @@ static int decode_and_set_image_option(int fd, uint64_t imageopt, librbd::ImageO
return 0;
}

static int do_import_header(int fd, int import_format, uint64_t &size, librbd::ImageOptions& opts)
static int do_import_imagemeta(int import_format, librbd::Image& image,

This comment has been minimized.

Copy link
@dillaman

dillaman Aug 29, 2017

Contributor

Nit: imagemeta -> metadata throughout

@PCzhangPC PCzhangPC force-pushed the PCzhangPC:metaexin branch 3 times, most recently from 81a3f6a to 2eb6242 Sep 8, 2017

@PCzhangPC

This comment has been minimized.

Copy link
Author

commented Sep 8, 2017

@dillaman updated

rbd export --export-format 2 testimg ${TMPDIR}/img_v2
rbd import --export-format 2 ${TMPDIR}/img_v2 testimg_import
rbd info testimg_import
rbd info testimg_import@snap
IMAGEMETA_AFTER=`rbd image-meta list testimg_import`
if [ "$IMAGEMETA_BEFORE" != "$IMAGEMETA_AFTER" ]; then

This comment has been minimized.

Copy link
@dillaman

dillaman Sep 8, 2017

Contributor

Nit: [ "$IMAGEMETA_BEFORE" = "$IMAGEMETA_AFTER" ]

@@ -594,6 +618,16 @@ static int do_import_header(int fd, int import_format, uint64_t &size, librbd::I
r = decode_and_set_image_option(fd, RBD_IMAGE_OPTION_STRIPE_UNIT, opts);
} else if (tag == RBD_EXPORT_IMAGE_STRIPE_COUNT) {
r = decode_and_set_image_option(fd, RBD_IMAGE_OPTION_STRIPE_COUNT, opts);
} else if (tag == RBD_EXPORT_IMAGE_META) {
r = utils::read_string(fd, length, &key);

This comment has been minimized.

Copy link
@dillaman

dillaman Sep 8, 2017

Contributor

Nit: why not follow the existing pattern of just calling a helper function to read the associated values of the tag struct and update the image (avoiding the intermediate list)?

This comment has been minimized.

Copy link
@PCzhangPC

PCzhangPC Sep 14, 2017

Author

we havn't created an import_image yet. so what we could do is just to collect them all here but set those image-meta later.

@PCzhangPC PCzhangPC force-pushed the PCzhangPC:metaexin branch 3 times, most recently from 31ce8ec to 84f19a7 Sep 14, 2017

@PCzhangPC

This comment has been minimized.

Copy link
Author

commented Sep 14, 2017

@dillaman updated

return r;
}

static int decode_imagemeta(int fd, uint64_t length, std::map<std::string,

This comment has been minimized.

Copy link
@dillaman

dillaman Sep 14, 2017

Contributor

Nit: don't break type between multiple lines and out params should be passed by pointer

This comment has been minimized.

Copy link
@PCzhangPC

PCzhangPC Sep 15, 2017

Author

my bad. updated already

return r;

imagemetas[key] = value;
return r;

This comment has been minimized.

Copy link
@dillaman

dillaman Sep 14, 2017

Contributor

Nit: for clarity, return 0

@PCzhangPC PCzhangPC force-pushed the PCzhangPC:metaexin branch 2 times, most recently from a95cb9b to 6ef8cc4 Sep 15, 2017

return r;
}

static int decode_imagemeta(int fd, uint64_t length, std::map<std::string, std::string> &imagemetas)

This comment has been minimized.

Copy link
@dillaman

dillaman Sep 15, 2017

Contributor

Nit: imagemetas should be passed by pointer

This comment has been minimized.

Copy link
@PCzhangPC

PCzhangPC Sep 21, 2017

Author

i'am not very sure. any bad effect if passed by reference ?

This comment has been minimized.

Copy link
@vshankar

vshankar Sep 21, 2017

Contributor

@PCzhangPC its just a convention -- if an argument is modified by a routine then pass it as a pointer.

This comment has been minimized.

Copy link
@PCzhangPC

PCzhangPC Sep 21, 2017

Author

ok. thank you

@@ -555,7 +555,46 @@ static int decode_and_set_image_option(int fd, uint64_t imageopt, librbd::ImageO
return 0;
}

static int do_import_header(int fd, int import_format, uint64_t &size, librbd::ImageOptions& opts)
static int do_import_metadata(int import_format, librbd::Image& image,
std::map<std::string, std::string> &imagemetas)

This comment has been minimized.

Copy link
@dillaman

dillaman Sep 15, 2017

Contributor

Nit: imagemetas should be passed by const reference

@@ -578,7 +617,7 @@ static int do_import_header(int fd, int import_format, uint64_t &size, librbd::I
opts.set(RBD_IMAGE_OPTION_FORMAT, image_format);
}

while (r == 0) {
while (r >= 0) {

This comment has been minimized.

Copy link
@dillaman

dillaman Sep 15, 2017

Contributor

Nit: was this change required?

@PCzhangPC PCzhangPC force-pushed the PCzhangPC:metaexin branch from 6ef8cc4 to 5d6dadf Sep 21, 2017

@PCzhangPC

This comment has been minimized.

Copy link
Author

commented Sep 25, 2017

@dillaman need review,thx

}

static int do_import_header(int fd, int import_format, uint64_t &size, librbd::ImageOptions& opts,
std::map<std::string, std::string> &imagemetas)

This comment has been minimized.

Copy link
@dillaman

dillaman Sep 28, 2017

Contributor

Nit: pass out-param as pointer instead of by reference

PCzhangPC
rbd:export/import image-meta when we export/import an image
when we do exporting/importing an image, we should export or import the image-meta of this image at the same time

Signed-off-by: PCzhangPC <pengcheng.zhang@easystack.cn>

@PCzhangPC PCzhangPC force-pushed the PCzhangPC:metaexin branch from 5d6dadf to 2f0b233 Sep 29, 2017

@PCzhangPC

This comment has been minimized.

Copy link
Author

commented Oct 4, 2017

@dillaman need review,THX

@dillaman
Copy link
Contributor

left a comment

lgtm

@dillaman dillaman added the needs-qa label Oct 4, 2017

@dillaman dillaman changed the title rbd:export/import image-meta when we export/import an image rbd: export/import image-meta when we export/import an image Oct 17, 2017

@dillaman dillaman merged commit ecec321 into ceph:master Oct 17, 2017

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