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.

Sign up for GitHub

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

Jump to bottom

rados: implement rm --force option to force remove when full #6202

Merged
merged 2 commits into from Dec 17, 2015
Merged

rados: implement rm --force option to force remove when full #6202

merged 2 commits into from Dec 17, 2015

Conversation

cxwshawn
Copy link

@cxwshawn cxwshawn commented Oct 8, 2015

[librados] extend remove interface, add flags parameter
rados tool use the extended librados interface to implement force remove.

test:
when cluster full, ceph -s show: 3 full osd(s)(only 3 osds in my cluster),
use:
./rados -p poolname rm objname --force (delete ok)
but ./rados -p poolname rm objname will stuck.

Signed-off-by: Xiaowei Chen chen.xiaowei@h3c.com

@loic-bot
Copy link

loic-bot commented Oct 8, 2015

SUCCESS: http://jenkins.ceph.dachary.org/job/ceph/8241/

@cxwshawn
Copy link
Author

cxwshawn commented Nov 5, 2015

@athanatos @liewegas please help check!

liewegas
liewegas reviewed Nov 6, 2015
View reviewed changes
src/tools/rados/rados.cc
@@ -2101,7 +2106,11 @@ static int rados_tool_common(const std::map < std::string, std::string > &opts,
if (use_striper) {
ret = striper.remove(oid);
} else {
ret = io_ctx.remove(oid);
if ( force == "true" ){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls fix whitespace

@liewegas
Copy link
Member

liewegas commented Nov 6, 2015

2 small issues, otherwise looks good!

@liewegas liewegas added the core label Nov 6, 2015
@liewegas liewegas self-assigned this Nov 6, 2015
@cxwshawn cxwshawn force-pushed the wip-ful-fix branch 2 times, most recently from 836dd69 to b6ac5da Compare November 7, 2015 03:26
@loic-bot
Copy link

loic-bot commented Nov 7, 2015

SUCCESS: http://jenkins.ceph.dachary.org/job/ceph/8906/

@cxwshawn
Copy link
Author

cxwshawn commented Nov 7, 2015

@liewegas fixed, please recheck !

@liewegas
Copy link
Member

liewegas commented Nov 8, 2015

lgtm. Do you mind adding a test to qa/workunits/rados/... that creates a pool, sets a small quota, fills it up, and shows that remove fails, but remove --force-full succeeds?

@sponce
Copy link

sponce commented Nov 10, 2015

Ideally, this should also be done in radosstriper, by patching in the same way libradosstriper/RadosStriperImpl so that it calls the new IoCtl entry point.

@cxwshawn cxwshawn force-pushed the wip-ful-fix branch 2 times, most recently from 0992b23 to 5246a9b Compare November 11, 2015 09:52
@loic-bot
Copy link

FAILURE: http://jenkins.ceph.dachary.org/job/ceph/9178/

@loic-bot
Copy link

FAILURE: http://jenkins.ceph.dachary.org/job/ceph/9179/

@cxwshawn
Copy link
Author

@liewegas @sponce implemented stripper force-full-remove support , please review .

@sponce
Copy link

sponce commented Nov 13, 2015

I've put comments in the code directly. I have basically only one : why duplicate all calls and not add an extra flags argument with default value 0 ?

@cxwshawn
Copy link
Author

@sponce @liewegas till now, the logic seems to have much duplication, but if only extend logic in remove(xxx,flags) interfaces, maybe separation is better, @liewegas what's your advice ?

@liewegas
Copy link
Member

@sponce We can't change the function signatures without breaking the .so's ABI.

@sponce
Copy link

sponce commented Nov 13, 2015

ok, that was my fear. Thanks for the confirmation.
Then I would duplicate everything but the remove of the striper library, as it's the only long one.
I would rather implement the one with extra arg and call it from the original one.

@liewegas
Copy link
Member

@cxwshawn Yep, agree with @sponce ... please remove the duplicate of the remove call (change it to a private _remove(...) that is called with by both remove() and remove2(), where remove() passes flags = 0).

@liewegas liewegas removed the needs-qa label Nov 16, 2015
@liewegas
Copy link
Member

@cxwshawn ping

@cxwshawn cxwshawn force-pushed the wip-ful-fix branch 2 times, most recently from 037d21b to b04d75b Compare November 25, 2015 02:19
@loic-bot
Copy link

FAILURE: http://jenkins.ceph.dachary.org/job/ceph/9533/

@cxwshawn
Copy link
Author

@dachary compile failed ? can @liewegas review now ?

@ghost
Copy link

ghost commented Nov 25, 2015

@cxwshawn the test/cephtool-test-rados.sh test failed. You can run it locally from the src directory to reproduce the problem.

@loic-bot
Copy link

SUCCESS: http://jenkins.ceph.dachary.org/job/ceph/9549/

liewegas
liewegas reviewed Nov 25, 2015
View reviewed changes
qa/workunits/rados/test_rados_tool.sh
ceph osd pool set-quota $p max_objects 1
V1=`mktemp fooattrXXXXXXX`
rados put $OBJ $V1 -p $p
sleep 30
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind replacing this with a loop that looks for the full flag on teh pool in the 'ceph osd dump' output? Otherwise this will slow down the make check execution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, looks good. Almost there!

@loic-bot
Copy link

SUCCESS: http://jenkins.ceph.dachary.org/job/ceph/9585/

@cxwshawn
Copy link
Author

@liewegas fixed, please review again.

tchaikov
tchaikov reviewed Nov 27, 2015
View reviewed changes
src/tools/rados/rados.cc
@@ -1239,7 +1239,7 @@ static int rados_tool_common(const std::map < std::string, std::string > &opts,

std::string run_name;
std::string prefix;

std::string forcefull("");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not make it a bool?

Xiaowei Chen added 2 commits December 11, 2015 03:26
rados: implement rm --force option to force remove when full.
db525ef 
           librados extend remove interface, add flags parameter, and use
            this extended interface to implement force remove when cluster full.
Signed-off-by: Xiaowei Chen <chen.xiaowei@h3c.com>
rados: implement radosstripper force remove when full.
54a6ba8 
                    extend libradosstripper remove interface and rados tool
                    rm force-full when use_stripper.

Signed-off-by: Xiaowei Chen <chen.xiaowei@h3c.com>
@cxwshawn cxwshawn force-pushed the wip-ful-fix branch 2 times, most recently from 4751199 to 54a6ba8 Compare December 11, 2015 08:53
@loic-bot
Copy link

SUCCESS: http://jenkins.ceph.dachary.org/job/ceph/10078/

@loic-bot
Copy link

SUCCESS: http://jenkins.ceph.dachary.org/job/ceph/10079/

@cxwshawn
Copy link
Author

@liewegas @tchaikov sorry for the latency, please help review!

@tchaikov
Copy link
Contributor

lgtm after the qa run passed.

liewegas added a commit that referenced this pull request Dec 17, 2015
@liewegas
Merge pull request #6202 from cxwshawn/wip-ful-fix
4060d21 
rados: implement rm --force option to force remove when full

Reviewed-by: Kefu Chai <kchai@redhat.com>
@liewegas liewegas merged commit 4060d21 into ceph:master Dec 17, 2015
@ghost ghost changed the title rados: implement rm --force option to force remove when full. rados: implement rm --force option to force remove when full Feb 10, 2016
@cxwshawn cxwshawn deleted the wip-ful-fix branch February 17, 2016 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@liewegas liewegas

Labels
core feature needs-qa wip-sage-testing
Projects
None yet
Milestone
No milestone
6 participants
@cxwshawn @loic-bot @liewegas @sponce @tchaikov @smithfarm