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

rgw: bucket index check in radosgw-admin removes valid index. #12851

Merged
merged 1 commit into from Jun 9, 2017

Conversation

Projects
None yet
5 participants
@zhangsw
Copy link
Contributor

zhangsw commented Jan 10, 2017

@zhangsw zhangsw force-pushed the zhangsw:fix-rgw-bucketcheck-bug branch from 6e3d5b7 to 50766ec Jan 10, 2017

@zhangsw

This comment has been minimized.

Copy link
Contributor Author

zhangsw commented Mar 3, 2017

@cbodley please help review this ,thanks~

@cbodley cbodley self-assigned this Mar 6, 2017

@cbodley

This comment has been minimized.

Copy link
Contributor

cbodley commented Mar 7, 2017

looks okay to me - @yehudasa can you double check?

@cbodley cbodley requested a review from yehudasa Mar 7, 2017

@theanalyst

This comment has been minimized.

Copy link
Member

theanalyst commented Mar 13, 2017

there is a merge commit in this pr which should be dropped

@zhangsw zhangsw force-pushed the zhangsw:fix-rgw-bucketcheck-bug branch from 4bf2fec to 3f6afbc Mar 13, 2017

@zhangsw

This comment has been minimized.

Copy link
Contributor Author

zhangsw commented Mar 13, 2017

@theanalyst I've rebased it and resolved the conflicts

@zhangsw zhangsw force-pushed the zhangsw:fix-rgw-bucketcheck-bug branch from 3f6afbc to 6cd4e3f Mar 13, 2017

@yehudasa

This comment has been minimized.

Copy link
Member

yehudasa commented Mar 23, 2017

@zhangsw how did you test it?

@zhangsw zhangsw force-pushed the zhangsw:fix-rgw-bucketcheck-bug branch 2 times, most recently from 606b165 to 1aa5bb0 Mar 27, 2017

@zhangsw

This comment has been minimized.

Copy link
Contributor Author

zhangsw commented Mar 27, 2017

@yehudasa @cbodley I'm sorry that I missed checking whether the bad index removed. I just checked those valid index were not removed. And there is an another bug when removing bad index. I've updated this pr to ensure those bad index will be removed. I've tested it by modifying complete multipart code to leave the keys those should be removed, and this pr could remove these keys.

@zhangsw

This comment has been minimized.

Copy link
Contributor Author

zhangsw commented Apr 12, 2017

string oid = key.name;
rgw_obj_index_key key = iter->key;
rgw_obj obj(bucket, key);
string oid = obj.key.name;

This comment has been minimized.

Copy link
@cbodley

cbodley Apr 12, 2017

Contributor

is there a reason this can't use rgw_obj::get_oid()?

This comment has been minimized.

Copy link
@zhangsw

zhangsw Apr 13, 2017

Author Contributor

@cbodley Yes, we can also use that. Maybe get_oid() is more readable and clear here. I've changed it

rgw: bucket index check in radosgw-admin removes valid index.
Fixes: http://tracker.ceph.com/issues/18470

Signed-off-by: Zhang Shaowen <zhangshaowen@cmss.chinamobile.com>

@zhangsw zhangsw force-pushed the zhangsw:fix-rgw-bucketcheck-bug branch from 1aa5bb0 to a786252 Apr 13, 2017

@cbodley

This comment has been minimized.

Copy link
Contributor

cbodley commented Apr 17, 2017

thanks @zhangsw, i was able to reproduce the issue by modifying the multipart complete as you suggested:

--- a/src/rgw/rgw_op.cc
+++ b/src/rgw/rgw_op.cc
@@ -4947,7 +4947,7 @@ void RGWCompleteMultipart::execute()
       rgw_obj_index_key remove_key;
       src_obj.key.get_index_key(&remove_key);
 
-      remove_objs.push_back(remove_key);
+      //remove_objs.push_back(remove_key);
 
       ofs += obj_part.size;
       accounted_size += obj_part.accounted_size;

after a multipart upload to BUCKET/big.iso, the _multipart_big.iso.2~rNBlqXA8BoV1OKRm7z5jY2dviJ_X-g4.XX objects were still listed in the output of radosgw-admin bi list --bucket=BUCKET. the radosgw-admin bucket check --bucket=BUCKET command was able to identify all of those objects, and the --fix command successfully removed them from the index. a final radosgw-admin bi list --bucket=BUCKET command confirmed that big.iso was the only object remaining

@cbodley

This comment has been minimized.

Copy link
Contributor

cbodley commented Apr 17, 2017

@yehudasa can you take another look, and merge if you're happy with it?

@zhangsw

This comment has been minimized.

Copy link
Contributor Author

zhangsw commented May 9, 2017

@yehudasa ping..

@yehudasa

This comment has been minimized.

Copy link
Member

yehudasa commented May 10, 2017

@zhangsw I will try to take a look at this one this week

@yehudasa

This comment has been minimized.

Copy link
Member

yehudasa commented Jun 9, 2017

@zhangsw congratulations on being the first person to send a PR to ragweed!

@yehudasa
Copy link
Member

yehudasa left a comment

@zhangsw overall ok.. I would have rather you used a different api to check the bucket index validity (the one you used is really obscure, I didn't remember it existed). E.g., something like bi_list() somehow: you could do a listomapkeys() using the librados bindings. That been said, I think it's ok for now.

@yehudasa

This comment has been minimized.

Copy link
Member

yehudasa commented Jun 9, 2017

@zhangsw for some reason I commented on the wrong PR, the comment above referred to the ragweed PR .

@yehudasa yehudasa merged commit 10a2281 into ceph:master Jun 9, 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
@zhangsw

This comment has been minimized.

Copy link
Contributor Author

zhangsw commented Jun 12, 2017

@yehudasa I also wanted to use bi_list,but I didn't find it in rest admin api. Maybe we should add it.

@zhangsw zhangsw deleted the zhangsw:fix-rgw-bucketcheck-bug branch Jun 12, 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.