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

mon,mgr: fix "ceph osd df", add some tools to find untested commands #15675

Merged
merged 8 commits into from Jun 15, 2017

Conversation

Projects
None yet
5 participants
@gregsfortytwo
Copy link
Member

gregsfortytwo commented Jun 13, 2017

Fixes http://tracker.ceph.com/issues/20256

This PR moves the "osd df" command handling from the monitor (where it currently crashes out!) to the manager. Along the way, I move the PGStatService into PGMap and the OSDUtilizationDumper code from OSDMonitor into OSDMap.

There's also a little script which greps for COMMANDs and does some basic grepping to see if they are unused in the qa and src/test directories, so we can identify them in future.

@gregsfortytwo

This comment has been minimized.

Copy link
Member Author

gregsfortytwo commented Jun 13, 2017

There are several commands which fail the check_commands.sh checks right now; I will create tickets for them in the appropriate projects.
I also didn't actually add one for "osd df"; I'll push one in a moment. Are we interested in trying to make check_commands.sh part of the "make check" tests?

@dmick

This comment has been minimized.

Copy link
Member

dmick commented Jun 14, 2017

it kind of is, with cephtool-test-X.sh ? Edit: sorry, I misunderstood. If it's sufficiently robust wrt identifying commands and their tests, I'm all for it, but that seems like a hard job to make sure the patterns are complete and correct

@gregsfortytwo

This comment has been minimized.

Copy link
Member Author

gregsfortytwo commented Jun 14, 2017

@dmick, I notice an arm build failure here which seems to be a build infrastructure program rather than a code issue.

@tchaikov
Copy link
Contributor

tchaikov left a comment

i think we can leave check_commands.sh out of make check.

i will need another morning to understand these services and the relation between them: PGStatService, MonPGStatService, PGMonStatService, MgrPGStatService and PGMapStatService.

but functionality wise, the change looks good to me.

@@ -18,7 +18,7 @@
#include "MgrMap.h"
#include "PaxosService.h"

class PGStatService;
class MonPGStatService;

This comment has been minimized.

Copy link
@tchaikov

tchaikov Jun 14, 2017

Contributor

this forward declaration is not necessary, and can be removed.

This comment has been minimized.

Copy link
@gregsfortytwo

gregsfortytwo Jun 14, 2017

Author Member

Done.

@@ -4166,7 +3849,8 @@ bool OSDMonitor::preprocess_command(MonOpRequestRef op)
} else if (prefix == "osd df") {
string method;
cmd_getval(g_ceph_context, cmdmap, "output_method", method);
print_utilization(ds, f ? f.get() : NULL, method == "tree");
print_osd_utilization(osdmap, mon->pgservice, ds,

This comment has been minimized.

Copy link
@tchaikov

tchaikov Jun 14, 2017

Contributor

could take this opportunity to replace

f ? f.get() : NULL

with

f.get()

because scoped_ptr<>::get() returns nullptr if the stored pointer is nullptr.

This comment has been minimized.

Copy link
@gregsfortytwo

gregsfortytwo Jun 14, 2017

Author Member

Yeah, I was a bit confused about this and just assumed the scoped_ptr semantics required it somehow (though that was also nonsensical). Clearly it's not the case, changing.

@gregsfortytwo

This comment has been minimized.

Copy link
Member Author

gregsfortytwo commented Jun 14, 2017

Added a ticket for check_commands.sh once it's cleaned up.
http://tracker.ceph.com/issues/20300

@gregsfortytwo

This comment has been minimized.

Copy link
Member Author

gregsfortytwo commented Jun 14, 2017

And yeah, sorry about the obtuse naming of all the StatService derivatives. :( I couldn't come up with anything better and figure it won't matter very often.

@gregsfortytwo gregsfortytwo requested a review from liewegas Jun 14, 2017

@liewegas

This comment has been minimized.

Copy link
Member

liewegas commented Jun 14, 2017

I don't really follow what the *Services (there are more than one now?) are for or why they vary, but they'll presumably all get removed right after luminous branches off so I don't have any strong feelings about it! Otherwise this looks good to me!

@liewegas
Copy link
Member

liewegas left a comment

don't forget to squash

@gregsfortytwo

This comment has been minimized.

Copy link
Member Author

gregsfortytwo commented Jun 14, 2017

I don't really follow what the *Services (there are more than one now?) are for or why they vary

The osd dumping stuff takes a PGStatService, so I moved the base class into PGMap and have an implementation running on the manager. Then the others inherit from that in various ways (and there's a monitor-specific base class adding its necessary functionality). It's tedious but whatever.

@gregsfortytwo

This comment has been minimized.

Copy link
Member Author

gregsfortytwo commented Jun 14, 2017

I'll squash this once we are happy with testing? :)

@yuriw

This comment has been minimized.

Copy link
Contributor

yuriw commented Jun 14, 2017

@gregsfortytwo seems having conflict

Auto-merging src/osd/OSDMap.h
Auto-merging src/osd/OSDMap.cc
CONFLICT (content): Merge conflict in src/osd/OSDMap.cc
Auto-merging src/mon/OSDMonitor.cc
Auto-merging src/mon/MonCommands.h
Auto-merging qa/workunits/cephtool/test.sh
Automatic merge failed; fix conflicts and then commit the result.

gregsfortytwo added some commits Jun 9, 2017

mon: move creating_pgs and reweight_by_utilization into new MonPGStat…
…Service

Use that throughout the monitor code, so that we can use the
PGStatService elsewhere.

Signed-off-by: Greg Farnum <gfarnum@redhat.com>
mon: mgr: move 'osd df' handling to manager
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
mon: move most PGMapStatService into PGMap; rename PGMon's to PGMonSt…
…atService

Most of this is independent of the PGMonitor, so move it into
PGMap and strip out those few bits. It'll come in handy shortly
when I move "ceph osd df" into the mgr.

Signed-off-by: Greg Farnum <gfarnum@redhat.com>
mon: inherit PGMonStatService from the PGMapStatService
Adventures in multiple inheritance!

Signed-off-by: Greg Farnum <gfarnum@redhat.com>
move the OSDUtilizationDumper code into OSDMap
Signed-off-by: Greg Farnum <gfarnum@redhat.com>
mon: mgr: enable "osd df" on the manager
Fixes: http://tracker.ceph.com/issues/20256

Signed-off-by: Greg Farnum <gfarnum@redhat.com>
qa: add a check_commands.sh script which looks for commands with no t…
…ests

This isn't run automatically by anything yet. Note that it's also a best-effort
thing; passing doesn't guarantee there are tests. It can be pretty easily fooled
if the command is a common word which shows up in specifying other things,
for instance.

Signed-off-by: Greg Farnum <gfarnum@redhat.com>
qa: test 'osd df' in cephtool/test.sh
Signed-off-by: Greg Farnum <gfarnum@redhat.com>

@gregsfortytwo gregsfortytwo force-pushed the gregsfortytwo:wip-bad-command branch from 66f82b6 to 010e45d Jun 15, 2017

@gregsfortytwo

This comment has been minimized.

Copy link
Member Author

gregsfortytwo commented Jun 15, 2017

Rebased and squashed @yuriw

@liewegas liewegas changed the title Fix "ceph osd df", add some tools to find untested commands mon,mgr: fix "ceph osd df", add some tools to find untested commands Jun 15, 2017

@liewegas liewegas merged commit 078e910 into ceph:master Jun 15, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
arm64 make check arm64 make check succeeded
Details
make check make check succeeded
Details
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.