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

ceph: perfcounter priorities and daemonperf updates to use them #14793

Merged
merged 28 commits into from May 5, 2017

Conversation

Projects
None yet
3 participants
@dmick
Member

dmick commented Apr 25, 2017

Builds on Sage's perfcounter priorities branch, adds to daemonperf.

New features:

  • [stat-pats]: optional list of shell-glob patterns to select stats
  • [priority]: optional minimum priority of stat to allow (default: 'useful')
  • daemonperf list|ls: Show tabular list of selected stats and their nicknames
  • daemonperf display now reacts when terminal is resized, truncating sections if necessary

liewegas and others added some commits Mar 28, 2017

os/bluestore: rename nicks to be <= 4 chars
Signed-off-by: Sage Weil <sage@redhat.com>
osd: rename nicks to be <= 4 chars
Signed-off-by: Sage Weil <sage@redhat.com>
osdc/Objecter: rename nicks to be <= 4 chars
Signed-off-by: Sage Weil <sage@redhat.com>
common/perf_counters: require nicks be <= 4 chars
This keeps the column sizes at 5 chars for 'ceph daemonperf'.

Signed-off-by: Sage Weil <sage@redhat.com>
common/perf_counters: allow priorities to be assigned to perf counters
The idea is that higher priority counters can be surfaced by user tools
automatically (e.g., 'ceph daemonperf') without enshrining knowledge in
the tool of the internal workings of the daemon.  Similarly, the tool
may not know which counters are in use for a particular daemon (e.g.,
filestore vs bluestore).

This requires a bit of discipline in the ceph code to use consistent
priorities, but as long as developers are using the tools that consume
the priorities (and I expect they will if we make the counters useful)
we should converge on good values.

Signed-off-by: Sage Weil <sage@redhat.com>
osd: clean up whitespace in create_logger()
Signed-off-by: Sage Weil <sage@redhat.com>
osd: set perf counter priorities
Signed-off-by: Sage Weil <sage@redhat.com>
mds: add perf counter nicks, priorities
Signed-off-by: Sage Weil <sage@redhat.com>
osdc/Objecter: set perf counter priorities
Signed-off-by: Sage Weil <sage@redhat.com>
ceph.in: move daemon commands to their own function
Signed-off-by: Dan Mick <dan.mick@redhat.com>
ceph.in: Move daemonperf to its own function
Signed-off-by: Dan Mick <dan.mick@redhat.com>
pybind/ceph_daemon.py: DaemonWatcher: fit display to window size
Keep track of terminal width/height, and output as many
candidate statistics as will fit in the window.

Signed-off-by: Dan Mick <dan.mick@redhat.com>
pybind/ceph_daemon.py: stop early termination of sleep
Any unignored signal will EINTR underneath and cause early
return of sleep().  Fix that by sleeping until end time is
reached.

Signed-off-by: Dan Mick <dan.mick@redhat.com>
pybind/ceph_daemon.py: reprint headers as soon as they disappear
Signed-off-by: Dan Mick <dan.mick@redhat.com>
ceph.in, pybind/ceph_daemon.py: allow stat filter patterns
optional second argument to ceph

Signed-off-by: Dan Mick <dan.mick@redhat.com>
pybind/ceph_daemon.py: allow sections to split for 'fitting'
Relax requirement that entire section fit; show whatever stats
fit out of the sections selected.

Signed-off-by: Dan Mick <dan.mick@redhat.com>
ceph.in, pybind/ceph_daemon.py: add priority filtering
Signed-off-by: Dan Mick <dan.mick@redhat.com>
ceph.in, pybind/ceph_daemon.py: add 'daemonperf list'
respect statpats and priority; display as a PrettyTable

Signed-off-by: Dan Mick <dan.mick@redhat.com>

@dmick dmick added the needs-qa label Apr 25, 2017

@dmick dmick requested a review from liewegas Apr 25, 2017

@dmick dmick referenced this pull request Apr 25, 2017

Closed

RFC common/perf_counter: improve nick system #14172

3 of 7 tasks complete
src/ceph.in Outdated
@@ -664,6 +668,10 @@ def daemonperf(childargs, sockpath):
# consume and analyze non-numeric args
while len(childargs) and not isnum(childargs[0]):
arg = childargs.pop(0)
# 'list'?
if arg == 'list':

This comment has been minimized.

@liewegas

liewegas Apr 26, 2017

Member

can we make 'ls' a synonym, since that's what we tend to use elsewhere?

This comment has been minimized.

@dmick

dmick Apr 26, 2017

Member

sure

@liewegas liewegas added the tools label Apr 26, 2017

@liewegas liewegas changed the title from perfcounter priorities and daemonperf updates to use them to ceph: perfcounter priorities and daemonperf updates to use them Apr 26, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 26, 2017

gnit:build (master) 10:03 AM $ bin/ceph -s
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
Traceback (most recent call last):
  File "bin/ceph", line 126, in 
    from ceph_daemon import DaemonWatcher, admin_socket
  File "/home/sage/src/ceph3/src/pybind/ceph_daemon.py", line 21, in 
    from prettytable import PrettyTable, HEADER
ImportError: No module named prettytable

i re-ran sudo ./install-deps.sh so this is presuambly a missing package dependency?

@liewegas liewegas added core and removed needs-qa labels Apr 26, 2017

@dmick

This comment has been minimized.

Member

dmick commented Apr 26, 2017

Hm. It was already used in mgr/fsstatus, but I guess it hasn't been added to dependencies yet. Will do or find a substitute.

pybind/ceph_daemon: default to prio 0 if unspecified
Signed-off-by: Sage Weil <sage@redhat.com>

liewegas and others added some commits Apr 26, 2017

pybind/ceph_daemon: preserve ordering
Signed-off-by: Sage Weil <sage@redhat.com>
os/bluestore: set priorities
Signed-off-by: Sage Weil <sage@redhat.com>
os/bluestore/BlueFS: set priorities
Signed-off-by: Sage Weil <sage@redhat.com>
osd: clean up perf counter priorities whitespace
Signed-off-by: Sage Weil <sage@redhat.com>
ceph.in: daemonperf: add 'ls' as a synonym for 'list'
Signed-off-by: Dan Mick <dan.mick@redhat.com>

@dmick dmick changed the title from ceph: perfcounter priorities and daemonperf updates to use them to DNM: ceph: perfcounter priorities and daemonperf updates to use them Apr 27, 2017

@dmick

This comment has been minimized.

Member

dmick commented Apr 27, 2017

Incorporated most of dmick#1 with a fix or two, added 'ls' as suggested; still want to verify prettytable is universally available and add to package deps

@dmick

This comment has been minimized.

Member

dmick commented Apr 27, 2017

also, clearly, I must fix the test code I hadn't realized existed

dmick added some commits Apr 27, 2017

ceph.spec.in, debian/control: add dep for python-prettytable
mgr/pybind/fsstatus already required this, but ceph.in does now.
Add as a build dependency as well to handle tests.

Signed-off-by: Dan Mick <dan.mick@redhat.com>
pybind/ceph_daemon.py: _gettermsize fails on non-terminals
default to 80x25; this should be for terminalless tests only

Signed-off-by: Dan Mick <dan.mick@redhat.com>
@dmick

This comment has been minimized.

Member

dmick commented Apr 28, 2017

OK, tests fixed, build and runtime dependencies added

@@ -121,6 +121,7 @@ BuildRequires: pkgconfig
BuildRequires: python
BuildRequires: python-devel
BuildRequires: python-nose
BuildRequires: python-prettytable

This comment has been minimized.

@tchaikov

tchaikov Apr 28, 2017

Contributor

do we run ceph at build-time? "make_check" is disabled by default. maybe we can guard this line like

%if 0%{with make_check}
BuildRequires:	python-prettytable
%endif

?

This comment has been minimized.

@dmick

dmick Apr 28, 2017

Member

Well, I assume the same thing applies as for Debian, in that the package needs to be installed for make check to work. Since the make check builds don't install the built packages to pull in prettytable via Requires, there's nothing else to do it that I know of. (if there is some sort of "make check dependencies" mechanism I'm happy to learn about it.)

This comment has been minimized.

@tchaikov

tchaikov Apr 28, 2017

Contributor

so the consumer of this line is install-deps.sh actually. i am far from an expert of dnf or its builddep plugin. so guess this is the best way to document the "make check" dependencies at this moment.

@@ -50,6 +50,7 @@ Build-Depends: bc,
python (>= 2.7),
python-all-dev,
python-nose,
python-prettytable,

This comment has been minimized.

@tchaikov

tchaikov Apr 28, 2017

Contributor

debian packaging does not run "make check", so we can remove python-prettytable from Build-Depends.

This comment has been minimized.

@dmick

dmick Apr 28, 2017

Member

The thing is, if it's not in build-depends, make check fails, because the target host doesn't have the package to run the tests.

This comment has been minimized.

@tchaikov

tchaikov Apr 28, 2017

Contributor

agreed. we debated about "make check" dependencies machinery before, iirc. but no acceptable result back then. i should learn to live with it.

Run <count> times (default forever),
once per <interval> seconds (default 1)
once per <interval> seconds (default 1)

This comment has been minimized.

@tchaikov

tchaikov Apr 28, 2017

Contributor

why added an extra space here?

This comment has been minimized.

@dmick

dmick Apr 28, 2017

Member

to show that it's part of the preceding line more clearly

if len(childargs) > 0:
try:
interval = float(childargs.pop(0))
if interval < 0:

This comment has been minimized.

@tchaikov

tchaikov Apr 28, 2017

Contributor

nit, why not just print the error message and return errno.EINVAL?

This comment has been minimized.

@dmick

dmick Apr 28, 2017

Member

because the exception can also come from float() (bad format), and this way both errors exit the same way

This comment has been minimized.

@tchaikov

tchaikov Apr 28, 2017

Contributor

oic.

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 28, 2017

In a narrow window for daemonperf osd.0, it shows all of the bluefs metrics (including the low priority ones) but not the higher-priority osd ones.

@dmick

This comment has been minimized.

Member

dmick commented Apr 28, 2017

Not sure what you mean. Default priority is 'useful', 5, and the ones it shows are all the ones with 5 or greater:

| section   | name               | nick | prio |
+-----------+--------------------+------+------+
| bluefs    | db_total_bytes     | b    |    5 |
| bluefs    | db_used_bytes      | u    |    5 |
| bluefs    | wal_total_bytes    | walb |    5 |
| bluefs    | wal_used_bytes     | walu |    5 |
| bluefs    | slow_total_bytes   | slob |    5 |
| bluefs    | slow_used_bytes    | slou |    5 |
| bluefs    | num_files          | f    |    5 |
| bluefs    | log_bytes          | jlen |    8 |
| bluefs    | logged_bytes       | j    |   10 |
| bluefs    | bytes_written_wal  | wal  |   10 |
| bluefs    | bytes_written_sst  | sst  |   10 |
| bluestore | kv_flush_lat       | fl_l |    8 |
| bluestore | kv_lat             | k_l  |    8 |
| bluestore | state_aio_wait_lat | io_l |    8 |
| bluestore | throttle_lat       | th_l |   10 |
| bluestore | submit_lat         | s_l  |   10 |
| bluestore | commit_lat         | c_l  |   10 |
| bluestore | read_lat           | r_l  |   10 |
| objecter  | op_active          | actv |    7 |
| objecter  | op_r               | rd   |    7 |
| objecter  | op_w               | wr   |    7 |
| objecter  | op_rmw             | rdwr |    5 |
| osd       | op                 | ops  |   10 |
| osd       | op_in_bytes        | wr   |    8 |
| osd       | op_out_bytes       | rd   |    8 |
| osd       | op_latency         | l    |    9 |
| osd       | recovery_ops       | rop  |    8 |
| osd       | numpg              | pgs  |    5 |

@dmick

This comment has been minimized.

Member

dmick commented Apr 28, 2017

Are you suggesting that you'd rather have it sort by priority?...because of the sections that would be hard

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 28, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 28, 2017

@dmick

This comment has been minimized.

Member

dmick commented Apr 28, 2017

Ah. that would be pretty slick and surprising IMO, but anything's possible of course. You can, however, use patterns to select more specifically.

ceph.in: drop semicolons; relax priority arg ordering
Priority had to be the last of the non-numeric arguments,
or it'd be overwritten

Signed-off-by: Dan Mick <dan.mick@redhat.com>

@dmick dmick changed the title from DNM: ceph: perfcounter priorities and daemonperf updates to use them to ceph: perfcounter priorities and daemonperf updates to use them May 2, 2017

@dmick

This comment has been minimized.

Member

dmick commented May 2, 2017

retest this please

@dmick

This comment has been minimized.

Member

dmick commented May 3, 2017

@liewegas: anything else here?

@liewegas liewegas merged commit 7f72100 into ceph:master May 5, 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