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: add "heap *" admin command #13073

Merged
merged 5 commits into from Apr 10, 2017

Conversation

Projects
None yet
7 participants
@chardan
Contributor

chardan commented Jan 23, 2017

Extends the OSD admin console with a heap profiling command.

@jecluis

This comment has been minimized.

Member

jecluis commented Jan 23, 2017

@chardan mentioned one of the commits (I'm guessing the rgw one) mistakenly made it into the branch. He will be rebasing the branch and repushing soon.

[[ $do_test -eq 0 ]] && return 0
admin_socket = "--admin-deamon out/osd.0.asok"

This comment has been minimized.

@jdurgin

jdurgin Jan 28, 2017

Member

typo: should be daemon

This comment has been minimized.

@chardan

chardan Jan 28, 2017

Contributor

Good eyes! Thanks.

This comment has been minimized.

@liewegas

liewegas Feb 8, 2017

Member

this still needs to be fixed it looks like?

This comment has been minimized.

@chardan

chardan Feb 8, 2017

Contributor

...if at first, you don't succeed... (should be corrected now, thank you).

@jdurgin

This comment has been minimized.

Member

jdurgin commented Jan 28, 2017

nice cleanup to share the command implementation! looks like it'll need a rebase for the command->admin_command rename

string cmd;
std::vector<std::string> cmd_vec;
if (false == cmd_getval(&cct, cmdmap, "heapcmd", cmd)) {

This comment has been minimized.

@jdurgin

jdurgin Jan 28, 2017

Member

usually we don't do yoda conditions - any reason to avoid !cmd_getval(...) here?

This comment has been minimized.

@chardan

chardan Jan 28, 2017

Contributor

lol @"yoda conditions". No reason here, and changing it makes it consistent with the rest of the function.

@jdurgin

This comment has been minimized.

Member

jdurgin commented Jan 30, 2017

The build error in jenkins looks real:

/home/jenkins-build/build/workspace/ceph-pull-requests/src/osd/OSD.cc:1951:14: error: ‘command’ was not declared in this scope
   } else if (command == "dump_objectstore_kv_stats") {
              ^

other than that lgtm.

@chardan

This comment has been minimized.

Contributor

chardan commented Jan 31, 2017

Not sure why I didn't see that build issue locally, but I just did a clean build with this patch.

@jdurgin

This comment has been minimized.

Member

jdurgin commented Feb 2, 2017

I'm getting the same error as jenkins when building locally - perhaps due to merging with the latest master

@jdurgin

jdurgin approved these changes Feb 3, 2017

@jdurgin jdurgin added the needs-qa label Feb 3, 2017

@tchaikov tchaikov changed the title from Jfw wip 15475 admin heap command to osd: add "heap *" admin command Feb 3, 2017

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Feb 3, 2017

@chardan could you squash the commits into a single one?

@chardan

This comment has been minimized.

Contributor

chardan commented Feb 3, 2017

@tchaikov Sure-- would you rather see all of them together, or do you mean that you want the "corrections" commits squashed? Thanks!

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Feb 3, 2017

@chardan the latter. =) sorry for the confusion!

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented Feb 3, 2017

Hi, it would be better to update commit message and PR' title, likes: osd: cleanup "heap *" asock commands

@chardan

This comment has been minimized.

Contributor

chardan commented Feb 3, 2017

@tchaikov: I'm working on figuring out how to do that, but it looks like it is beyond my feeble git-fu at this time. (I believe the problem is the merge commit-- when I "git rebase -i HEAD~2", it's picking up all the individual commits in the merge. I'm not sure what the right way around this is, ideas more than welcomed!)

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Feb 4, 2017

@chardan you need to remove the merge commit first, here is how i do this

git rebase -i 5c18dd4~
# 1. remove all commits way down to 8c79cd1 (exclusive) in the editor
# 2. resolve the conflict
# 3. reorder the commits and squash the related ones
# 4. voilà
@jecluis

This comment has been minimized.

Member

jecluis commented Feb 4, 2017

Personally, I think the commit messages are verbose enough as they are, and do not necessarily need to be changed.

However, I'm with @tchaikov on the commits needing squashing, and I would not mind the commits being properly rebased on top of master instead of having merge commits polluting the branch's history :)

(edit: seems like I didn't refresh the page and the party continued in the mean time; consider above comments with that in mind)

@yuriw

This comment has been minimized.

Contributor

yuriw commented Feb 5, 2017

test this please

@yuriw

This comment has been minimized.

Contributor

yuriw commented Feb 5, 2017

@chardan @jdurgin FYI Build failed:

[ 34%] Building CXX object src/CMakeFiles/common-objs.dir/log/Log.cc.o
/home/jenkins-build/build/workspace/ceph-pull-requests/src/osd/OSD.cc: In member function ‘bool OSD::asok_command(std::string, cmdmap_t&, std::string, std::ostream&)’:
/home/jenkins-build/build/workspace/ceph-pull-requests/src/osd/OSD.cc:1958:14: error: ‘command’ was not declared in this scope
} else if (command == "calc_objectstore_db_histogram") {
^
[ 34%] Building CXX object src/osd/CMakeFiles/osd.dir/Watch.cc.o

@@ -1726,7 +1726,7 @@ function test_admin_heap_profiler()
[[ $do_test -eq 0 ]] && return 0
admin_socket = "--admin-deamon out/osd.0.asok"
admin_socket = "--admin-daemon out/osd.0.asok"

This comment has been minimized.

@tchaikov

tchaikov Feb 6, 2017

Contributor

please squash this change into 48116a6

@@ -1951,9 +1951,9 @@ bool OSD::asok_command(string admin_command, cmdmap_t& cmdmap, string format,
f->dump_bool("success", success);
f->dump_int("value", value);
f->close_section();
} else if (command == "dump_objectstore_kv_stats") {
} else if (admin_command == "dump_objectstore_kv_stats") {

This comment has been minimized.

@tchaikov

tchaikov Feb 6, 2017

Contributor

and squash this change into 1371e27

This comment has been minimized.

@chardan

chardan Feb 6, 2017

Contributor

Done.

namespace ceph {
namespace osd_cmds {
int heap(CephContext& cct, cmdmap_t& cmdmap, Formatter& f, std::ostream& os)

This comment has been minimized.

@tchaikov

tchaikov Feb 6, 2017

Contributor

nit, this helper function should be in an anonymous namespace.

This comment has been minimized.

@chardan

chardan Feb 6, 2017

Contributor

The reason I've added them to a named namespace is that I foresee moving these into a new source module. One factor is that this having a function that works in both consoles is a different pattern than already exists in the code; they have to be given some sort of name, and I think the namespace helps to make the purpose clear. As more such commands are added, I'm confident the organizational benefit will provide better returns than for this single function.

namespace ceph {
namespace osd_cmds {
int heap(CephContext& cct, cmdmap_t& cmdmap, Formatter& f, std::ostream& os);

This comment has been minimized.

@tchaikov

tchaikov Feb 6, 2017

Contributor

it's not necessary to have this declaration in the header file. it's only used in OSD.cc.

This comment has been minimized.

@chardan

chardan Feb 6, 2017

Contributor

I agree, will do. I think ultimately the "real" right solution is to split these into their own source file away from the other OSD.cc code, but that's a much more significant change.

store->get_db_statistics(f);
} else if (command == "dump_scrubs") {
} else if (admin_command == "dump_scrubs") {

This comment has been minimized.

@@ -1782,20 +1782,20 @@ class OSDSocketHook : public AdminSocketHook {
OSD *osd;
public:
explicit OSDSocketHook(OSD *o) : osd(o) {}
bool call(std::string command, cmdmap_t& cmdmap, std::string format,
bool call(std::string admin_command, cmdmap_t& cmdmap, std::string format,

This comment has been minimized.

@tchaikov

tchaikov Feb 6, 2017

Contributor

i don't understand how this helps us to search the admin_command? IIUC, all commands in OSD.cc are admin commands.

This comment has been minimized.

@chardan

chardan Feb 6, 2017

Contributor

Consider the console commands, around line 5509 or so. It was not obvious to me which of these were "admin" vs. "console" commands at first, and having the above change in would have saved me quite a bit of time. That said, the reason the change is in a separate commit is because it's indeed optional; if there are a lot of strong feelings against it, we don't need to include it, but I know I would have found it helpful.

@chardan

This comment has been minimized.

Contributor

chardan commented Feb 10, 2017

Any next steps or things I should attend to here? Thanks to everyone for the great input!

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Mar 31, 2017

@chardan needs rebase.

chardan added some commits Dec 1, 2016

osd: Unifies heap command for OSD console and admin interface.
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
osd: Add test for admin heap profiling command.
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
osd: changes "command" to "admin_command" for admin commands.
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
osd: heap profiling cleanups in response to review notes, build fixes.
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
@jdurgin

This comment has been minimized.

Member

jdurgin commented Apr 4, 2017

jenkins: retest this please

rebased.

@liewegas liewegas merged commit 7e547a1 into ceph:master Apr 10, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment