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

ceph-disk,osd: add support for crush device classes #14436

Merged
merged 3 commits into from Apr 18, 2017
Merged

ceph-disk,osd: add support for crush device classes #14436

merged 3 commits into from Apr 18, 2017

Conversation

ghost
Copy link

@ghost ghost commented Apr 10, 2017 •
edited by ghost
Loading

@ghost ghost added core feature labels Apr 10, 2017
@ghost
Copy link
Author

ghost commented Apr 10, 2017

@liewegas could you please take a quick look at ecf934e ? It's passing tests and I'll split it into two commits for clarity. Unless you see something obviously wrong with it at first glance ;-)

@liewegas
Copy link
Member

lgtm!

@liewegas
Copy link
Member

The fallback to 'osd create' for both commands is sort of weird at first blush but makes sense since the crush location update (and/or device class) might be disabled.

@ldachary
mon: set-device-class implicitly creates OSD
7c2fbca 
If the OSD does not exist in the crushmap, it is implicitly created.

Signed-off-by: Loic Dachary <loic@dachary.org>
@ghost ghost changed the title DNM: ceph-disk support for device class ceph-disk support for device class Apr 10, 2017
@ghost ghost changed the title ceph-disk support for device class ceph-disk,osd: add support for crush device classes Apr 10, 2017
@ghost ghost requested a review from liewegas April 10, 2017 21:00
@ghost
Copy link
Author

ghost commented Apr 10, 2017

@liewegas thanks for the quick review :-) I split the commit in three for clarity and rebased after @yuriw merged the pull request it depended on. It should be ready for your final review.

liewegas
liewegas requested changes Apr 10, 2017
View reviewed changes
src/ceph-disk/ceph_disk/main.py Outdated
@@ -1922,6 +1922,10 @@ def parser():
help='unique OSD uuid to assign this disk to',
)
parser.add_argument(
'--crush-class',
Copy link
Member

Choose a reason for hiding this comment

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

should this be --crush-device-class maybe?

Copy link
Author

Choose a reason for hiding this comment

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

We already have ceph osd crush class {ls,rm,create} and it looked vaguely consistent to have --crush-class. But if it feels weird I can change it.

Copy link
Author

@ghost ghost Apr 10, 2017 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

maybe we should change osd crush set-device-class to osd crush set-class so they all look the same ?

Copy link
Author

Choose a reason for hiding this comment

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

In the context CrushWrapper we use device but in the mon we use osd. In the context of ceph osd crush repeating device or even crush is redundant and we can just have set-class instead. I used set-device-class initially because the CrushWrapper function has the same name but in retrospect I think it was not a good idea.

Copy link
Member

Choose a reason for hiding this comment

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

Ok I buy that argument. Let's go with it!

src/osd/OSD.cc Outdated
@@ -3021,6 +3021,43 @@ int OSD::shutdown()
return r;
}

int OSD::mon_cmd_maybe_osd_create(string &cmd) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: newline before {

Copy link
Author

Choose a reason for hiding this comment

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

oops, fixed and repushed

@ldachary
osd: split mon_cmd_maybe_osd_create out of update_crush_location
83b695f 
Signed-off-by: Loic Dachary <loic@dachary.org>
@ghost
Copy link
Author

ghost commented Apr 10, 2017

@liewegas I added b17989e to s/set-device-class/set-class/ and have crush-class when the OSD is implied or in context and class when both the OSD and crush are implied. It feels consistent to me right now but ...

@liewegas
Copy link
Member

I think it's consistent, but I worry that we have both "type" and "class" and the user can be easily confused. If we refer to them as "bucket type" and "device class" throughout then they will be less likely to be mixed up...

@ghost
Copy link
Author

ghost commented Apr 10, 2017

that's sensible. Shall we change ceph osd crush class {rm,ls,create} to ceph osd crush device-class {rm,ls,create} then ?

@liewegas
Copy link
Member

liewegas commented Apr 10, 2017 via email

@ghost
Copy link
Author

ghost commented Apr 10, 2017 

/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/erasure-code/test-erasure-eio.sh:85return 1
/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/erasure-code/test-erasure-eio.sh:11return 1
/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/erasure-code/test-erasure-eio.sh:17return 1
/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/erasure-code/test-erasure-eio.sh:26return 1
/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/erasure-code/test-erasure-eio.sh:39return 1

jenkins test this please

@ghost
Copy link
Author

ghost commented Apr 11, 2017

@liewegas I prefer class and crush_class over device_class :-)

@liewegas
Copy link
Member

liewegas commented Apr 11, 2017 via email

@ghost ghost added needs-qa and removed needs-qa labels Apr 11, 2017
@ghost
Copy link
Author

ghost commented Apr 11, 2017

repushed after renaming crush_class into crush_device_class

@ghost ghost added the needs-qa label Apr 12, 2017
@ghost
Copy link
Author

ghost commented Apr 12, 2017

@liewegas I think I addressed your remarks. Did I forget something ?

@yuriw
Copy link
Contributor

yuriw commented Apr 18, 2017

http://pulpito.ceph.com/yuriw-2017-04-17_21:11:42-rados-wip-sage-testing2_2017_4_18-distro-basic-smithi

http://pulpito.ceph.com/yuriw-2017-04-18_14:45:29-rados-wip-sage-testing2_2017_4_18---basic-smithi/

@yuriw
Copy link
Contributor

yuriw commented Apr 18, 2017

@liewegas pls merge

@liewegas liewegas merged commit 8ded80f into ceph:master Apr 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
core feature needs-qa wip-sage2-testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@liewegas @yuriw @ldachary