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

tool: ceph-kvstore-tool doesn't umount BlueStore properly #18083

Merged
merged 1 commit into from Oct 4, 2017

Conversation

liuchang0812
Copy link
Contributor

Copy link
Member

@liewegas liewegas left a comment

Choose a reason for hiding this comment

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

otherwise looks good


~StoreTool() {
if (bluestore) {
assert(0 == bluestore->umount());
Copy link
Member

Choose a reason for hiding this comment

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

please move the call with side effects out of the assert, in case we compile out assert() in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you Sage. done, remove assert here.

@liewegas liewegas merged commit 1de1109 into ceph:master Oct 4, 2017
@liewegas
Copy link
Member

liewegas commented Oct 4, 2017

@liuchang0812 do you mind writing a test case that reproduces the original issue? can probably put it in qa/standalone.. maybe a separate dir for tool tests that don't need an actual cluster?

@liuchang0812
Copy link
Contributor Author

I will do it. we need a bluestore dir, so standalone seems is better?

@liewegas
Copy link
Member

liewegas commented Oct 5, 2017 via email

@liuchang0812
Copy link
Contributor Author

@liewegas I have wrote a test case for ceph-kvstore-tool, see 4752513

@liewegas
Copy link
Member

liewegas commented Oct 6, 2017

Looks good.. is that commit part of another PR? We also need to add a .yaml file in qa/suites/rados/standalone-nomgrs/... somewhere so that it gets run as part of the test suite.

@liuchang0812
Copy link
Contributor Author

Looks good.. is that commit part of another PR? 

yeah, PR: #16745

We also need to add a .yaml file in qa/suites/rados/standalone-nomgrs/... somewhere so that it gets run as part of the test suite.

I put it in qa/workunits since it doesn't need a standalone cluster. it seems that we will run it automatically. not sure....I'm learning how to write a yaml file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants