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

osd/OSD: auto class on osd start up #16014

Merged
merged 10 commits into from Jul 13, 2017

Conversation

Projects
None yet
5 participants
@xiexingguo
Copy link
Member

xiexingguo commented Jun 29, 2017

No description provided.

@leseb leseb referenced this pull request Jun 29, 2017

Closed

Device classes on OSDs #1631

@leseb leseb added the build/ops label Jun 29, 2017

@xiexingguo xiexingguo requested a review from liewegas Jun 29, 2017

@liewegas
Copy link
Member

liewegas left a comment

Thanks!

return 0;
}

if (store->get_type() != "bluestore") {

This comment has been minimized.

Copy link
@liewegas

liewegas Jun 29, 2017

Member

Instead of making this bluestore specific, how about

string device_class = store->is_rotational() ? "hdd" : "ssd";
@xiexingguo

This comment has been minimized.

Copy link
Member Author

xiexingguo commented Jun 30, 2017

Instead of making this bluestore specific, how about

string device_class = store->is_rotational() ? "hdd" : "ssd";

I'd prefer to keep current logic for bluestore and pull the changes you suggested in for other oss.
As bluestore is able to support more device types, such as nvme (and perhaps others in the near future)

@xiexingguo xiexingguo force-pushed the xiexingguo:wip-autoclass branch 2 times, most recently from 1c45e6a to 3dedf40 Jun 30, 2017

@liewegas

This comment has been minimized.

Copy link
Member

liewegas commented Jun 30, 2017

That's reasonable. Can we either implement the filestore hdd/ssd here too, or maybe add an string ObjectStore::get_default_device_class() method?

@xiexingguo xiexingguo force-pushed the xiexingguo:wip-autoclass branch from 3dedf40 to f0e1e15 Jun 30, 2017

@xiexingguo

This comment has been minimized.

Copy link
Member Author

xiexingguo commented Jun 30, 2017

maybe add an string ObjectStore::get_default_device_class() method?

Sounds reasonable. On it.

@xiexingguo xiexingguo force-pushed the xiexingguo:wip-autoclass branch 6 times, most recently from 79548b4 to fd67bf1 Jun 30, 2017

@@ -72,8 +72,7 @@ function TEST_classes() {
ceph osd getcrushmap > $dir/map || return 1
crushtool -d $dir/map -o $dir/map.txt || return 1
${SED} -i \
-e '/device 0 osd.0/s/$/ class ssd/' \
-e '/step take default/s/$/ class ssd/' \
-e '/step take default/s/$/ class hdd/' \

This comment has been minimized.

Copy link
@liewegas

liewegas Jun 30, 2017

Member

This will break if the test cluster happens to have an ssd, right?

This comment has been minimized.

Copy link
@xiexingguo

xiexingguo Jun 30, 2017

Author Member

yeah, still working on it. thanks for the feedback.

@xiexingguo xiexingguo force-pushed the xiexingguo:wip-autoclass branch from fd67bf1 to f1b2abb Jun 30, 2017

'--crush-device-class',
help='crush device class to assign this disk to',
)
parser.add_argument(

This comment has been minimized.

Copy link
@liewegas

liewegas Jun 30, 2017

Member

I don't think we shoudl deprecate this.. we can still let the admin manually specify a different class of their choice. Not everyone will be happy with the ssd/hdd default behavior!

@xiexingguo xiexingguo force-pushed the xiexingguo:wip-autoclass branch 5 times, most recently from 4310901 to 7d69fb4 Jun 30, 2017

@xiexingguo xiexingguo added the bug fix label Jul 1, 2017

@liewegas liewegas added the needs-qa label Jul 1, 2017

xiexingguo added some commits Jun 27, 2017

crush: fix potential class id collision
Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
crush: use auto for type safety
class-id, by definition, is of type int32_t, not int.

Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
crush: allow "osd crush set-device-class" to create class automatically
Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
mon/OSDMonitor: extend "set-device-class" for multiple osds
This is useful for applying changes to entire subtrees.
For example:

./bin/ceph osd crush set-device-class hdd `./bin/ceph osd ls-tree default`
set osd(s) 0,1,2 to class 'hdd'

Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
osd/OSD: automatically set device class on start
Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
crush: fix error handling
get_bucket() does not return a nullptr on error,
it returns a negative error code instead.

Use the IS_ERR() macro to handle it properly.

Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
mon/OSDMonitor: "osd crush ls-osd" support
Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
mon/OSDMonitor: tidy up class_map too on crush removal
Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
test: fix ut and release-notes
To keep pace with the newly merged #16027

Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>

@xiexingguo xiexingguo force-pushed the xiexingguo:wip-autoclass branch from bf2bfe1 to c5b99af Jul 10, 2017

@xiexingguo

This comment has been minimized.

Copy link
Member Author

xiexingguo commented Jul 11, 2017

#16188 put a guard for the "set-device-class" command and make it available for Luminous osds only, hence will prevent pre-Luminous osds from booting when auto-class-on-startup feature is enabled.
Fix this by appending ff7ea58.

Note that we don't fix the above problem by making osds conditionally sending "set-device-class" instead, since it will turn off the auto-class feature for newly created luminous clusters.

osd/OSD: don't fault on osd's "set-device-class" request
So pre-luminous osds can still boot normally.

Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>

@xiexingguo xiexingguo force-pushed the xiexingguo:wip-autoclass branch from ff7ea58 to d7ca4bc Jul 11, 2017

@xiexingguo

This comment has been minimized.

Copy link
Member Author

xiexingguo commented Jul 11, 2017

Instead of this, let's just make the OSD tolerate and ignore -EPERM. (Also, it might be a good time to make the error path not seg fault. You can vstart a clsuter without luminous with -o 'mon debug no require luminous = true'.)

@liewegas Updated. I'll push another PR for the seg fault fix since it might take some time and has no obvious relationship with this patch set.

"name=class,type=CephString " \
"name=ids,type=CephString,n=N", \
"set the <class> of the osd(s) <id> [<id>...]," \
"or use <all|any|*> to set all.", \

This comment has been minimized.

Copy link
@liewegas

liewegas Jul 13, 2017

Member

This breaks:

2017-07-12T22:14:25.296 INFO:tasks.workunit.client.0.smithi172.stderr:+ ceph osd crush set-device-class osd.0 ssd
2017-07-12T22:14:25.447 INFO:tasks.workunit.client.0.smithi172.stderr:Error EINVAL: Expected option value to be integer, got 'ssd', unable to parse osd id:"ssd".

/a/sage-2017-07-12_19:30:01-rados-wip-sage-testing-distro-basic-smithi/1392480

..and other tests in this run. let's drop this commit?

This comment has been minimized.

Copy link
@xiexingguo

xiexingguo Jul 13, 2017

Author Member

let's drop this commit?

@liewegas The failure is expected. And I have told you in #16283.
Can we fix the test script instead? It is annoying if we have to manually set the class of a large host/rack or something alike.
With this change we can do something like ceph osd crush set-device-class hdd 'ceph osd ls-tree host'

This comment has been minimized.

Copy link
@xiexingguo

xiexingguo Jul 13, 2017

Author Member

e4fe498
03fe1a6

The above two commits need to be updated if you test them together with this change, otherwise this one is good to merge. (And we can fix and retest the others later)

This comment has been minimized.

Copy link
@liewegas

liewegas Jul 13, 2017

Member

ah i see, sorry! i'm getting confused with these racing changes. :)

@liewegas liewegas merged commit 95c07fb into ceph:master Jul 13, 2017

4 checks passed

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

@xiexingguo xiexingguo deleted the xiexingguo:wip-autoclass branch Jul 13, 2017

@tchaikov

This comment has been minimized.

Copy link
Contributor

tchaikov commented Jul 30, 2017

@xiexingguo this breaks qa/workunits/mon/crush_ops.sh, see http://pulpito.ceph.com/kchai-2017-07-30_00:01:51-rados-wip-kefu-testing-distro-basic-smithi/1460903/

2017-07-30T00:56:53.953 INFO:tasks.workunit.client.0.smithi057.stderr:+ ceph osd crush set-device-class hdd osd.1
2017-07-30T00:56:54.013 INFO:tasks.workunit.client.0.smithi057.stderr:2017-07-30 00:56:54.012649 7f82357d2700 -1 WARNING: all dangerous and experimental features are enabled.
2017-07-30T00:56:54.025 INFO:tasks.workunit.client.0.smithi057.stderr:2017-07-30 00:56:54.025780 7f82357d2700 -1 WARNING: all dangerous and experimental features are enabled.
2017-07-30T00:56:54.029 INFO:tasks.workunit.client.0.smithi057.stderr:2017-07-30 00:56:54.029138 7f822c7f8700  0 -- 172.21.15.57:0/4055027923 >> 172.21.15.37:6792/0 pipe(0x7f8228089bc0 sd=7 :0 s=1 pgs=0 cs=0 l=1 c=0x7f8228057c30).fault
2017-07-30T00:56:54.123 INFO:tasks.workunit.client.0.smithi057.stderr:Error EBUSY: osd.1 has already bound to class 'ssd', can not reset class to 'hdd'; use 'ceph osd crush rm-device-class <osd>' to remove old class first
@tchaikov

This comment has been minimized.

Copy link
Contributor

tchaikov commented Jul 30, 2017

#16680 should address this.

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.