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: fix rbd children listing when child is in trash and add rbd_list_children2 method #18483

Merged
merged 3 commits into from Nov 14, 2017

Conversation

Projects
None yet
3 participants
@Songweibin
Copy link
Contributor

commented Oct 23, 2017

Add rbd_list_children2 and librbd::RBD::list_children2 methods as @dillaman suggested in #14865 (comment)
and after this fix rbd children listing as follows:

root@~:/ceph-dev/build# ./bin/rbd children i1@s1 
rbd/i3
root@~:/ceph-dev/build# ./bin/rbd children i1@s1 -a
rbd/i2 (trash 103c3d1b58ba)
rbd/i3
root@~:/ceph-dev/build#

http://tracker.ceph.com/issues/21893
Signed-off-by: songweibin song.weibin@zte.com.cn

@xiexingguo

This comment has been minimized.

Copy link
Member

commented Oct 24, 2017

need to fix the cli test first..

@Songweibin Songweibin force-pushed the Songweibin:wip-list-children2 branch from 1e74946 to 6332433 Oct 24, 2017

@xiexingguo xiexingguo requested a review from dillaman Oct 24, 2017

@Songweibin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2017

@dillaman please help to have a review, thanks in advance.

@dillaman
Copy link
Contributor

left a comment

Thanks -- can you also ensure the test cases verify that images in the trash are properly returned?

std::string image_name;
std::string image_id;
} children_info_t;

This comment has been minimized.

Copy link
@dillaman

dillaman Oct 25, 2017

Contributor

Nit: child_info_t (since children implies more than one)

* Returns a structure of poolname, imagename, imageid for
* each clone of this image at the currently set snapshot.
*/
int list_children2(std::vector<librbd::children_info_t>& children2);

This comment has been minimized.

Copy link
@dillaman

dillaman Oct 25, 2017

Contributor

Nit: no need for 2 at the end of the parameter name

@@ -105,6 +105,8 @@ namespace librbd {
int list(librados::IoCtx& io_ctx, std::vector<std::string>& names);
int list_children(ImageCtx *ictx,
std::set<std::pair<std::string, std::string> > & names);
int list_children2(ImageCtx *ictx,

This comment has been minimized.

Copy link
@dillaman

dillaman Oct 25, 2017

Contributor

Let's not add a duplicate function within the internal API -- instead, update the existing list_children method to take std::vector<child_info_t> and update the public API methods to convert between the new type and the old pair.

@@ -1272,6 +1272,20 @@ namespace librbd {
return r;
}

int Image::list_children2(vector<librbd::children_info_t>& children2)

This comment has been minimized.

Copy link
@dillaman

dillaman Oct 25, 2017

Contributor

Nit: no need for 2 suffix on param name

lderr(cct) << "Error looking up name for image id " << id_it
<< " in pool " << info.first.second << dendl;
return r;
} else if (r == -ENOENT) {

This comment has been minimized.

Copy link
@dillaman

dillaman Oct 25, 2017

Contributor

Nit: execute the -ENOENT handling first. If it fails w/ -EOENT, you should allow it to fall through to the generic handling so that the error message is "error looking up name for image id XYZ in pool" instead of "in rbd trash".

} else if (r == -ENOENT) {
cls::rbd::TrashImageSpec trash_spec;
r = cls_client::trash_get(&ioctx, id_it, &trash_spec);
if (r < 0) {

This comment has been minimized.

Copy link
@dillaman

dillaman Oct 25, 2017

Contributor

Need to support older OSDs to filter out -EOPNOTSUPP errors and change them to -ENOENT so that it falls down into the moved generic error handler clause (from above).

f->close_section();
} else {
std::cout << child_it.first << "/" << child_it.second << std::endl;
if (all_flag) {

This comment has been minimized.

Copy link
@dillaman

dillaman Oct 25, 2017

Contributor

Nit: instead of duplicating all this logic, perhaps always use the new list_children2 API and if --all wasn't specified, don't print any children that are in the trash.

std::string pool_name;
std::string image_name;
std::string image_id;
} children_info_t;

This comment has been minimized.

Copy link
@dillaman

dillaman Oct 25, 2017

Contributor

Perhaps add a bool trash member so that users don't need to re-execute a trash_get (since it was already performed within the API).

@@ -2232,6 +2235,40 @@ written." % (self.name, ret, length))
free(c_pools)
free(c_images)

def list_children2(self):

This comment has been minimized.

Copy link
@dillaman

dillaman Oct 25, 2017

Contributor

Might be nicer to switch to an iterator style and return a dict for each child.

@Songweibin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2017

@dillaman Thanks for your nicer suggestions, will update later.

@Songweibin Songweibin force-pushed the Songweibin:wip-list-children2 branch 7 times, most recently from e8399a8 to f541e87 Oct 27, 2017

@Songweibin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2017

@dillaman Updated, please take a look, thanks.
But I'm not sure I follow your idea about this:

Let's not add a duplicate function within the internal API -- instead, update the existing list_children method to take std::vector<child_info_t> and update the public API methods to convert between the new type and the old pair.

@dillaman
Copy link
Contributor

left a comment

@Songweibin The function list_children(ImageCtx *ictx, set<pair<string, string> >& names) should be removed from internal.h/cc and all "list children" public API methods should use list_children(ImageCtx *ictx, vector<child_info_t>& names)

lderr(cct) << "Error looking up name for image id " << id_it
<< " in pool " << info.first.second << dendl;
return r;
} else if (r == -ENOENT) {

This comment has been minimized.

Copy link
@dillaman

dillaman Oct 30, 2017

Contributor

Comment still needs to be addressed

CEPH_RBD_API int rbd_list_children2(rbd_image_t image,
rbd_child_info_t *children,
int *max_children);
CEPH_RBD_API void rbd_list_child_end(rbd_child_info_t *children);

This comment has been minimized.

Copy link
@dillaman

dillaman Oct 30, 2017

Contributor

Nit: rename to rbd_list_child_cleanup and add a size_t num_children parameter so that all the initialized rbd_child_info_t structs can be cleaned

names.insert(make_pair(info.first.second, name));
}
}

return 0;
}

bool compare_by_name(const child_info_t& c1, const child_info_t& c2) {

This comment has been minimized.

Copy link
@dillaman

dillaman Oct 30, 2017

Contributor

Nit: add to anonymous namespace near top of file

@@ -105,6 +105,9 @@ namespace librbd {
int list(librados::IoCtx& io_ctx, std::vector<std::string>& names);
int list_children(ImageCtx *ictx,
std::set<std::pair<std::string, std::string> > & names);
int list_children(ImageCtx *ictx,
std::vector<child_info_t>& names);

This comment has been minimized.

Copy link
@dillaman

dillaman Oct 30, 2017

Contributor

Pass out-values by pointer instead of reference

@Songweibin Songweibin force-pushed the Songweibin:wip-list-children2 branch 2 times, most recently from 88d6557 to 0d2c611 Oct 31, 2017

librbd: drop unused 'rbd_snap_t'
Signed-off-by: songweibin <song.weibin@zte.com.cn>

@Songweibin Songweibin changed the title rbd: add librbd::RBD::list_children2 and rbd_list_children2 methods rbd: fix rbd children listing when child is in trash and add rbd_list_children2 method Oct 31, 2017

@Songweibin Songweibin force-pushed the Songweibin:wip-list-children2 branch from 0d2c611 to 007d4d9 Oct 31, 2017

@Songweibin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2017

@dillaman Updated all, please take a review again, thanks.

*/
int list_children(std::set<std::pair<std::string, std::string> > *children);
int list_children(std::vector<librbd::child_info_t> *children);

This comment has been minimized.

Copy link
@dillaman

dillaman Nov 2, 2017

Contributor

You cannot change the public API -- you can only change the private API that is implemented in "src/librbd/internal.cc"

This comment has been minimized.

Copy link
@Songweibin

Songweibin Nov 2, 2017

Author Contributor

Oh, sorry, at first I thought list_children(std::set<std::pair<std::string, std::string> > *children) was unused, so i changed it directly. Actually there may be other external APPs used it. Already modified, thanks.

@@ -144,7 +144,7 @@ Skip test on FreeBSD as it generates different output there.
--io-type arg IO type (read , write, or readwrite(rw))

rbd help children
usage: rbd children [--pool <pool>] [--image <image>] [--snap <snap>]
usage: rbd children [--pool <pool>] [--image <image>] [--snap <snap>] [--all]

This comment has been minimized.

Copy link
@dillaman

dillaman Nov 2, 2017

Contributor

These (related) changes seem to be mixing and matching between commits. The title of this commit is to fix a bug re: listing children for trashed images so it should be limited to handling that fix. The next commit can add the new API methods and expand the rbd CLI to list trashed images.

@Songweibin Songweibin force-pushed the Songweibin:wip-list-children2 branch from 007d4d9 to 1d603b2 Nov 2, 2017

librbd: fix rbd children listing when child is in trash bin
Signed-off-by: songweibin <song.weibin@zte.com.cn>

@Songweibin Songweibin force-pushed the Songweibin:wip-list-children2 branch from 1d603b2 to e6332d0 Nov 2, 2017

@Songweibin

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2017

@dillaman Done, please help to take a look, thank you.

children[i].trash = cpp_children[i].trash;
if (!children[i].image_id) {
for (int j = 0; j < i; j++)
free((void *)children[j].image_id);

This comment has been minimized.

Copy link
@dillaman

dillaman Nov 6, 2017

Contributor

Nit: re-use rbd_list_children_cleanup

This comment has been minimized.

Copy link
@Songweibin

Songweibin Nov 7, 2017

Author Contributor

@dillaman fixed, thanks for your carefully review.

children[i].image_name = strdup(cpp_children[i].image_name.c_str());
children[i].image_id = strdup(cpp_children[i].image_id.c_str());
children[i].trash = cpp_children[i].trash;
if (!children[i].image_id) {

This comment has been minimized.

Copy link
@dillaman

dillaman Nov 6, 2017

Contributor

Nit: why only validate the result of image_id instead of also checking pool_name and image_name?

@Songweibin Songweibin force-pushed the Songweibin:wip-list-children2 branch 2 times, most recently from 59a20ad to 5856920 Nov 7, 2017

int r = librbd::list_children(ictx, &cpp_children);
if (r == -ENOENT) {
tracepoint(librbd, list_children_exit, *max_children);
return 0;

This comment has been minimized.

Copy link
@dillaman

dillaman Nov 7, 2017

Contributor

Nit: if r == -NOENT, just set r = 0 and continue so that max_return is properly populated and the array is properly null terminated.

children[i].image_name = strdup(cpp_children[i].image_name.c_str());
children[i].image_id = strdup(cpp_children[i].image_id.c_str());
children[i].trash = cpp_children[i].trash;
if (!children[i].pool_name && !children[i].image_name &&

This comment has been minimized.

Copy link
@dillaman

dillaman Nov 7, 2017

Contributor

Swap the && operators to || since it should fail if any memory allocation fails

This comment has been minimized.

Copy link
@Songweibin

Songweibin Nov 8, 2017

Author Contributor

@dillaman fixed, thanks.

@Songweibin Songweibin force-pushed the Songweibin:wip-list-children2 branch from 5856920 to e4ea1f1 Nov 8, 2017

@dillaman
Copy link
Contributor

left a comment

lgtm

@dillaman dillaman added needs-qa and removed needs-qa labels Nov 8, 2017

std::cout << child_it.first << "/" << child_it.second << std::endl;
if (all_flag) {
std::cout << it->pool_name << "/" << it->image_name << "/"
<< it->image_id;

This comment has been minimized.

Copy link
@dillaman

dillaman Nov 8, 2017

Contributor

Sorry -- I missed this during the review but saw it when play-testing the change. I don't like how adding '--all" changes the format to include the image id. The "/" is part of the image-spec (i.e. pool/image) but it's being overloaded here to include the image id. I would include the image id only for items in the trash like the following:

<pool name>/<image name> (trash <image id>)

This comment has been minimized.

Copy link
@Songweibin

Songweibin Nov 9, 2017

Author Contributor

Yeah, sounds more reasonable. Already fixed, thanks.

@Songweibin Songweibin force-pushed the Songweibin:wip-list-children2 branch from e4ea1f1 to ed3753c Nov 9, 2017

@dillaman

This comment has been minimized.

librbd: add new API methods and expand the rbd CLI to list trashed im…
…ages

implement librbd::RBD::list_children2 and rbd_list_children2
methods and expand the rbd CLI to list trashed images

Signed-off-by: songweibin <song.weibin@zte.com.cn>

@Songweibin Songweibin force-pushed the Songweibin:wip-list-children2 branch from ed3753c to 9d9a276 Nov 14, 2017

@Songweibin

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2017

@dillaman Sorry, since list_children2() returns an iterator style it shoud convert to list when comparing, and there also have a mistake when manipulating expected_children/expected_children2 in the trash cases.
All have fixed, thanks.

@dillaman dillaman merged commit 16715ec into ceph:master Nov 14, 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

@Songweibin Songweibin deleted the Songweibin:wip-list-children2 branch Nov 15, 2017

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.