osd: revamp {noup,nodown,noin,noout} related commands #27735

xiexingguo · 2019-04-24T07:55:39Z

This works as a good supplement of #27563.

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

References tracker ticket
Updates documentation if necessary
Includes tests for new functionality or reproducer for bug

xiexingguo · 2019-04-24T07:59:19Z

@liewegas I end up writing yet another patch which I think works as a good supplement of #27563 as we used to employ device-classes to separate different pools...

xiexingguo · 2019-04-24T09:27:26Z

retest this please

xiexingguo · 2019-04-24T12:28:46Z

src/mon/OSDMonitor.cc

@@ -2442,25 +2442,12 @@ bool OSDMonitor::prepare_mark_me_down(MonOpRequestRef op)

 bool OSDMonitor::can_mark_down(int i)
 {
-  if (osdmap.test_flag(CEPH_OSDMAP_NODOWN)) {


FIXME: hrm, this is CEPH_OSDMAP_NODOWN, not CEPH_OSD_NODOWN

src/osd/OSDMap.h

liewegas · 2019-04-24T13:54:26Z

qa/workunits/cephtool/test.sh

+  ceph osd add-noup $osd_0_device_class
+  ceph osd add-nodown $osd_0_device_class
+  ceph osd add-noin $osd_0_device_class
+  ceph osd add-noout $osd_0_device_class


It may be a bit late, but I don't really like this syntax. I wonder if we should take the opportunity to switch to something more like:

ceph osd set-group <flag> <name> ceph osd unset-group <flag> <name>

I don't love the 'group' part much either, tbh. But we usually do 'set' and 'unset' so it's weird to have 'add' and 'rm' here. And also weird to put the flag name as part of the command instead of as an argument.

xiexingguo · 2019-04-26T10:13:31Z

@liewegas Ready for re-review.

xiexingguo · 2019-04-27T00:47:10Z

retest this please

xiexingguo · 2019-04-29T12:10:06Z

@liewegas Can you look?

xiexingguo · 2019-05-06T12:15:09Z

@liewegas Ping :-)

xiexingguo · 2019-05-13T12:39:42Z

@liewegas Are there any other comments that I should address first? Also please let me know if you are reluctant to accept these changes...

liewegas · 2019-05-28T15:01:49Z

@jdurgin @athanatos

athanatos · 2019-05-28T22:34:20Z

The usage seems good to me. LGTM!

(What follows is pure bike-shedding, feel free to ignore it) Perhaps set-flags and unset-flags rather than set-group and unset-group might be clearer?

liewegas · 2019-05-29T15:55:04Z

The usage seems good to me. LGTM!

(What follows is pure bike-shedding, feel free to ignore it) Perhaps set-flags and unset-flags rather than set-group and unset-group might be clearer?

By itself, ceph osd set-flags flagname osd-or-crush-node seems clearer. The -group came from trying to make it make sense in the context of ceph osd [un]set flagname, which sets cluster-wide flags. I can't say I like -group that much but set-flags seems like it would confuse the distinction between set vs set-flags.

liewegas · 2019-05-29T16:30:49Z

retest this please

athanatos · 2019-05-29T17:54:32Z

Makes sense.

xiexingguo · 2019-05-30T00:31:29Z

retest this please

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

This works as a good supplement of ceph#27563. Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>

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

…nd to new implementation Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>

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

It does not make much sense to add this kind of restrictions as long as user is aware of what is going on. Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>

xiexingguo · 2019-05-30T11:49:19Z

@liewegas make check passed now. Can we move forward with this now? I have some other bugs and improvements that depend on this.

liewegas · 2019-06-03T20:27:30Z

@xiexingguo @neha-ojha @jdurgin We have a decision to make here.

If this targets octopus only, then we should change the encoding for OSDMap so that the new device flags are only encoded when the octopus feature is present. If we plan to backport this to nautilus, then that can stay the same.

But... if we backport, we should make the old command format still work (e.g., add-flagname and rm-flagname) in addition to the new names. In fact, we should probably make the old way continue to work for a release or two anyway?

neha-ojha · 2019-06-03T22:40:08Z

@xiexingguo @neha-ojha @jdurgin We have a decision to make here.

If this targets octopus only, then we should change the encoding for OSDMap so that the new device flags are only encoded when the octopus feature is present. If we plan to backport this to nautilus, then that can stay the same.

But... if we backport, we should make the old command format still work (e.g., add-flagname and rm-flagname) in addition to the new names. In fact, we should probably make the old way continue to work for a release or two anyway?

Yes, I agree. In my opinion, we should probably backport it to nautilus and make sure the old commands still work. I'd be interested to know what others think, though.

xiexingguo · 2019-06-03T23:22:24Z

If this targets octopus only, then we should change the encoding for OSDMap so that the new device flags are only encoded when the octopus feature is present. If we plan to backport this to nautilus, then that can stay the same.

I'd love to prepare the nautilus backports:-)

we should make the old command format still work (e.g., add-flagname and rm-flagname) in addition to the new names. In fact, we should probably make the old way continue to work for a release or two anyway?

They should (I just mark them as stale (deprecated) and there are still related tests, as you can see https://github.com/ceph/ceph/pull/27735/files#diff-9d634c21d02bca80a9ff0df5fd35acb9L1554 )

xiexingguo · 2019-06-05T06:16:30Z

http://pulpito.ceph.com/xxg-2019-06-05_05:22:43-rados-wip-device-class-noout-distro-basic-smithi/

xiexingguo · 2019-06-05T06:32:07Z

Backports for nautilus: #28400

xiexingguo added feature core labels Apr 24, 2019

xiexingguo requested a review from liewegas April 24, 2019 07:55

xiexingguo commented Apr 24, 2019

View reviewed changes

liewegas reviewed Apr 24, 2019

View reviewed changes

src/osd/OSDMap.h Show resolved Hide resolved

liewegas reviewed Apr 24, 2019

View reviewed changes

xiexingguo force-pushed the wip-device-class-noout branch from 21e1faa to 05bd54f Compare April 26, 2019 08:35

xiexingguo changed the title ~~osd: add no{out,down,in,out} flags by device class~~ osd: revamp {noup,nodown,noin,noout} related commands Apr 26, 2019

liewegas requested review from neha-ojha and jdurgin May 13, 2019 14:00

xiexingguo added 6 commits May 30, 2019 09:46

osd,mon: add 'ceph osd {set,unset}-group <flags> <who>' command

9a05a06

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

osd: add no{out,down,in,out} flags by device class

5d69526

This works as a good supplement of ceph#27563. Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>

mon/MonCommands: mark 'osd {add,rm}-{noup,nodown,noin,noout}' as depr…

06e2255

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

mon/OSDMonitor: migrate 'osd {add,rm}-{noup,nodown,noin,noout}' comma…

20d05a1

…nd to new implementation Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>

doc: refresh {noup,nodown,noin,noout} changes

a3b0dc2

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

qa: tolerate wildcard position

199d92a

It does not make much sense to add this kind of restrictions as long as user is aware of what is going on. Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>

xiexingguo force-pushed the wip-device-class-noout branch from c58f207 to 199d92a Compare May 30, 2019 02:57

liewegas added the wip-sage-testing label May 30, 2019

liewegas approved these changes May 30, 2019

View reviewed changes

xiexingguo merged commit 302d7bc into ceph:master Jun 5, 2019

xiexingguo deleted the wip-device-class-noout branch June 5, 2019 06:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

osd: revamp {noup,nodown,noin,noout} related commands #27735

osd: revamp {noup,nodown,noin,noout} related commands #27735

xiexingguo commented Apr 24, 2019

xiexingguo commented Apr 24, 2019

xiexingguo commented Apr 24, 2019

xiexingguo Apr 24, 2019

liewegas Apr 24, 2019

xiexingguo commented Apr 26, 2019

xiexingguo commented Apr 27, 2019

xiexingguo commented Apr 29, 2019

xiexingguo commented May 6, 2019

xiexingguo commented May 13, 2019

liewegas commented May 28, 2019

athanatos commented May 28, 2019

liewegas commented May 29, 2019

liewegas commented May 29, 2019

athanatos commented May 29, 2019

xiexingguo commented May 30, 2019

xiexingguo commented May 30, 2019

liewegas commented Jun 3, 2019

neha-ojha commented Jun 3, 2019

xiexingguo commented Jun 3, 2019 •

edited

xiexingguo commented Jun 5, 2019

xiexingguo commented Jun 5, 2019

osd: revamp {noup,nodown,noin,noout} related commands #27735

osd: revamp {noup,nodown,noin,noout} related commands #27735

Conversation

xiexingguo commented Apr 24, 2019

xiexingguo commented Apr 24, 2019

xiexingguo commented Apr 24, 2019

xiexingguo Apr 24, 2019

Choose a reason for hiding this comment

liewegas Apr 24, 2019

Choose a reason for hiding this comment

xiexingguo commented Apr 26, 2019

xiexingguo commented Apr 27, 2019

xiexingguo commented Apr 29, 2019

xiexingguo commented May 6, 2019

xiexingguo commented May 13, 2019

liewegas commented May 28, 2019

athanatos commented May 28, 2019

liewegas commented May 29, 2019

liewegas commented May 29, 2019

athanatos commented May 29, 2019

xiexingguo commented May 30, 2019

xiexingguo commented May 30, 2019

liewegas commented Jun 3, 2019

neha-ojha commented Jun 3, 2019

xiexingguo commented Jun 3, 2019 • edited

xiexingguo commented Jun 5, 2019

xiexingguo commented Jun 5, 2019

xiexingguo commented Jun 3, 2019 •

edited