mon: Add ceph osd get-require-min-compat-client command
#19015
Conversation
hansbogert
force-pushed
the
master
branch
2 times, most recently
from
1fe4d1e
to
b65da47
Compare
|
I'm not sure if test code is needed for this, and if so, where I would have to add it. |
src/mon/OSDMonitor.cc
Outdated
| @@ -8321,6 +8321,9 @@ bool OSDMonitor::prepare_command_impl(MonOpRequestRef op, | |||
| wait_for_finished_proposal(op, new Monitor::C_Command(mon, op, 0, rs, | |||
| get_last_committed() + 1)); | |||
| return true; | |||
| } else if (prefix == "osd get-require-min-compat-client") { | |||
| ss << "require_min_compat_client is set to " << ceph_release_name(osdmap.require_min_compat_client); | |||
There was a problem hiding this comment.
just ceph_release_name(..), no need to make it a complete sentence!
|
Can you add the test to .qa/workunits/cephtool/test.sh? Just call this after the set-... and grep for the result. Thanks! |
|
The errors in the ARM64 build seem unrelated. Is the ARM64 build still flaky? |
src/mon/MonCommands.h
Outdated
| @@ -704,6 +704,7 @@ COMMAND("osd set-nearfull-ratio " \ | |||
| "name=ratio,type=CephFloat,range=0.0|1.0", \ | |||
| "set usage ratio at which OSDs are marked near-full", | |||
| "osd", "rw", "cli,rest") | |||
| COMMAND("osd get-require-min-compat-client", "get the minimum client version we will maintain compatibility with", "osd", "r", "cli,rest") | |||
There was a problem hiding this comment.
I would appreciate if you could break this line to conform with the rest of the file, as well as the coding style guidelines.
There was a problem hiding this comment.
What's the idea behind the continuation slashes? I can't seem to find consistency when I look at other COMMANDs.
src/mon/OSDMonitor.cc
Outdated
| @@ -8321,6 +8321,9 @@ bool OSDMonitor::prepare_command_impl(MonOpRequestRef op, | |||
| wait_for_finished_proposal(op, new Monitor::C_Command(mon, op, 0, rs, | |||
| get_last_committed() + 1)); | |||
| return true; | |||
| } else if (prefix == "osd get-require-min-compat-client") { | |||
There was a problem hiding this comment.
This is a read-only command. It belongs in OSDMonitor::preprocess_command() instead.
There was a problem hiding this comment.
Well the responsibilities of the code in that file are a bit convoluted. (Functions with thousands of lines?!) so sorry, for not seeing that. But updated per your instructions.
hansbogert
force-pushed
the
master
branch
2 times, most recently
from
7ba430a
to
4ac43da
Compare
|
Does the "needs-test" label need to be removed before this will be picked up? |
src/mon/OSDMonitor.cc
Outdated
| @@ -4791,6 +4791,9 @@ bool OSDMonitor::preprocess_command(MonOpRequestRef op) | |||
| rdata.append(ss.str()); | |||
| ss.str(""); | |||
| } | |||
| } else if (prefix == "osd get-require-min-compat-client") { | |||
| ss << ceph_release_name(osdmap.require_min_compat_client); | |||
There was a problem hiding this comment.
use rdata.append(...)
ss goes to stderr, rdata goes to stdout.
There was a problem hiding this comment.
@hansbogert untagged untill @Sage 's comments implemented
There was a problem hiding this comment.
@liewegas then shouldn't the is QA/integration test the only place to check this? Would've helped a lot if make check would've shown this error. Thank you for patience so far, for such a trivial patch.grep in the test have failed, as that grep only checks stdout ?
There was a problem hiding this comment.
currently, yes. we move a lot of the make check tests out to teuthology because they took too long to run. we might consider putting the cli tests back in!
hansbogert
force-pushed
the
master
branch
2 times, most recently
from
4bc0d13
to
90e3b2c
Compare
As an antagonist to the already existing `set-require-min-compat-client` Signed-off-by: hansbogert <hansbogert@gmail.com>
|
@yuriw Can you or someone add the needs-review label? |
As an antagonist to the already existing
set-require-min-compat-client