ceph CLI: introduce daemonhistogram command by ifed01 · Pull Request #41600 · ceph/ceph

ifed01 · 2021-05-31T16:14:18Z

This patch embeds daemon's histogram presentation functionality originally brought by ../src/tools/histogram_dump.py script into primary Ceph's CLI.
Some minor usability improvements have been made too:

allow daemon specification via both daemon's name and admin socket path
allow one-shot every daemon specification via type.all clause, e.g. osd.all
minor improvements in axis presentation

Signed-off-by: Igor Fedotov ifedotov@suse.com

Checklist

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

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox

ifed01 · 2021-05-31T16:18:38Z

jdurgin

Great idea to integrate this to make it easily accessible!

Could you add a test to invoke it somewhere so we can be sure it at least does not crash?

jdurgin · 2021-06-09T07:12:27Z

doc/man/8/ceph.rst

+
+Usage::
+
+	ceph daemonperf {type.all|daemon_names|socket_paths} [ {batch|live} [{interval} [{count}]]]


can this describe the meaning of the parameters / that you need to run it on the same host as the daemon?

ifed01 · 2021-06-16T17:35:13Z

Great idea to integrate this to make it easily accessible!

Could you add a test to invoke it somewhere so we can be sure it at least does not crash?

@jdurgin - done!

neha-ojha · 2021-06-24T23:25:21Z

jenkins test make check

ifed01 · 2021-07-20T17:08:17Z

@jdurgin - Josh, mind reviewing? Or may be recommend someone for that? Thanks!

This allows to monitor how fragmented resulting allocations are depending on the requested block size. Could be coupled with ceph#41600 for easy live monitoring for either an individual OSD or full set of them. Signed-off-by: Igor Fedotov <ifedotov@suse.com>

jdurgin

Looks great to me!

tchaikov · 2021-07-29T09:16:54Z

qa/workunits/cephtool/test.sh

 OSD_TESTS+=" admin_heap_profiler"
 OSD_TESTS+=" osd_tell_help_command"
 OSD_TESTS+=" osd_compact"
+OSD_TESTS+=" osd_daemon_histogram"


2021-07-29T08:48:54.174 INFO:tasks.workunit.client.0.smithi143.stderr:/home/ubuntu/cephtest/clone.client.0/qa/workunits/cephtool/test.sh:2960: main: test_osd_daemon_histogram 2021-07-29T08:48:54.174 INFO:tasks.workunit.client.0.smithi143.stderr:/home/ubuntu/cephtest/clone.client.0/qa/workunits/cephtool/test.sh:2731: test_osd_daemon_histogram: sudo ceph daemonhistogram osd.all osd.op_r_latency_out_bytes_histogram batch 1 3 2021-07-29T08:48:54.654 INFO:tasks.workunit.client.0.smithi143.stderr:no valid command found; 10 closest matches: 2021-07-29T08:48:54.654 INFO:tasks.workunit.client.0.smithi143.stderr:pg stat 2021-07-29T08:48:54.655 INFO:tasks.workunit.client.0.smithi143.stderr:pg getmap 2021-07-29T08:48:54.655 INFO:tasks.workunit.client.0.smithi143.stderr:pg dump [<dumpcontents:all|summary|sum|delta|pools|osds|pgs|pgs_brief>...] 2021-07-29T08:48:54.655 INFO:tasks.workunit.client.0.smithi143.stderr:pg dump_json [<dumpcontents:all|summary|sum|pools|osds|pgs>...] 2021-07-29T08:48:54.656 INFO:tasks.workunit.client.0.smithi143.stderr:pg dump_pools_json 2021-07-29T08:48:54.656 INFO:tasks.workunit.client.0.smithi143.stderr:pg ls-by-pool <poolstr> [<states>...] 2021-07-29T08:48:54.656 INFO:tasks.workunit.client.0.smithi143.stderr:pg ls-by-primary <id|osd.id> [<pool:int>] [<states>...] 2021-07-29T08:48:54.656 INFO:tasks.workunit.client.0.smithi143.stderr:pg ls-by-osd <id|osd.id> [<pool:int>] [<states>...] 2021-07-29T08:48:54.656 INFO:tasks.workunit.client.0.smithi143.stderr:pg ls [<pool:int>] [<states>...] 2021-07-29T08:48:54.657 INFO:tasks.workunit.client.0.smithi143.stderr:pg dump_stuck [<stuckops:inactive|unclean|stale|undersized|degraded>...] [<threshold:int>] 2021-07-29T08:48:54.657 INFO:tasks.workunit.client.0.smithi143.stderr:Error EINVAL: invalid command

see https://pulpito.ceph.com/kchai-2021-07-29_08:05:42-rados-wip-kefu-testing-2021-07-29-1336-distro-basic-smithi/6300873/

@tchaikov - thanks for testing! This however looks like a broken/uncomplete ceph tool deployment - the daemonhistogram call from the test case looks valid but not available in ceph tool. I presume legacy tool version is in use. I added a ceph tool help call prior to daemonhistogram invocation to verify that proper tool is in use. Could you please re-run the QA.

I'm assuming this has been solved.

tchaikov · 2022-07-27T11:23:17Z

src/ceph.in


-def monids():
-    ret, outbuf, outs = json_command(cluster_handle, prefix='mon dump',
+def monids(h):


this breaks the test of cephtool/test.sh, like

2022-07-27T09:23:37.246 INFO:tasks.workunit.client.0.smithi154.stderr:/home/ubuntu/cephtest/clone.client.0/qa/workunits/cephtool/test.sh:35: expect_false: set -x 2022-07-27T09:23:37.247 INFO:tasks.workunit.client.0.smithi154.stderr:/home/ubuntu/cephtest/clone.client.0/qa/workunits/cephtool/test.sh:36: expect_false: ceph ping mon.foo 2022-07-27T09:23:37.312 INFO:tasks.workunit.client.0.smithi154.stderr:[errno 2] RADOS object not found (error calling ping_monitor) 2022-07-27T09:23:37.313 INFO:tasks.workunit.client.0.smithi154.stderr:/home/ubuntu/cephtest/clone.client.0/qa/workunits/cephtool/test.sh:36: expect_false: return 0 2022-07-27T09:23:37.313 INFO:tasks.workunit.client.0.smithi154.stderr:/home/ubuntu/cephtest/clone.client.0/qa/workunits/cephtool/test.sh:2639: test_mon_ping: ceph ping 'mon.*' 2022-07-27T09:23:37.415 INFO:tasks.workunit.client.0.smithi154.stderr:monids() missing 1 required positional argument: 'h'

see https://pulpito.ceph.com/kchai-2022-07-27_06:59:49-rados-wip-kefu-testing-2022-07-27-0857-distro-default-smithi/

github-actions · 2022-11-28T01:29:12Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

github-actions · 2022-12-28T02:18:35Z

This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution!

anthonyeleven · 2024-07-12T15:50:55Z

doc/man/8/ceph.rst

+daemonhistogram
+---------------
+
+Visualize Ceph daemons' performance histogram. To be executed from daemon's host.


anthonyeleven · 2024-07-12T15:51:28Z

doc/man/8/ceph.rst

+Parameters::
+
+ * daemons -  Daemons to collect data from. This can be either 'daemon_type.all' specification to use every daemon of a specific type. Or {daemon_names|socket_paths}*}. Or a comma separated list of daemon ids or admin socket paths.
+ * histogram_name - Performance histogram name, formatted as <component>.<name>. List of possible values is available via 'ceph daemon <daemon_name> perf histogram schema' command


s/command//

anthonyeleven · 2024-07-12T15:52:00Z

doc/man/8/ceph.rst

+ * daemons -  Daemons to collect data from. This can be either 'daemon_type.all' specification to use every daemon of a specific type. Or {daemon_names|socket_paths}*}. Or a comma separated list of daemon ids or admin socket paths.
+ * histogram_name - Performance histogram name, formatted as <component>.<name>. List of possible values is available via 'ceph daemon <daemon_name> perf histogram schema' command
+ * mode - output mode, either live(default) or batch.
+ * interval - period (in seconds) to update the histogram, 1 sec if omitted


s/1 sec if omitted/defaults to one second/

rzarzynski

Basically LGTM! The comments are about minors.

I will be happy to merge assuming it passes QA. The teuthology problem is already tackled, right?

rzarzynski · 2024-07-13T11:25:56Z

qa/workunits/cephtool/test.sh

 OSD_TESTS+=" admin_heap_profiler"
 OSD_TESTS+=" osd_tell_help_command"
 OSD_TESTS+=" osd_compact"
+OSD_TESTS+=" osd_daemon_histogram"


I'm assuming this has been solved.

rzarzynski · 2024-07-13T11:48:26Z

src/ceph.in

+
+    sockpaths = []
+    for_whom = None
+    if len(childargs) > 0 and childargs[0] in ["daemonhistogram"]:


Is the intention here allow user to use shortcuts of the command?

>>> "daemonhistog" in "daemonhistogram" True

If so, is this coherent with other parts of the CLI interface?

rzarzynski · 2024-07-13T11:54:39Z

src/ceph.in

+                target = mask.split('.', 1)
+                service = target[0]
+                targets = ids_by_service_for_cluster(name, cluster_name, conffile, service)
+                if len(targets) <= 0:


nit: len() below zero?

rzarzynski · 2024-07-13T12:01:53Z

src/ceph.in

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


Can a valid interval be zero?

rzarzynski · 2024-07-13T12:03:15Z

src/ceph.in

+            if interval < 0:
+                raise ValueError
+        except ValueError:
+            print('daemonperf: interval should be a positive number', file=sys.stderr)


This is inconsistent with the interval == 0 case.

rzarzynski · 2024-07-13T12:05:31Z

src/pybind/ceph_daemon.py

+        self.termsize.update()
+
+    def create_histogram(self, sockets, counter, last, batch):
+


nit: do we really need this newline?

rzarzynski · 2024-07-13T12:09:43Z

src/pybind/ceph_daemon.py

+           except Exception as e:
+               return (last,
+                       "Couldn't connect to admin socket, result: \n{}".format(e))
+           current_datasets[socket] = json_d[c[0]][c[1]]['values']


Plenty of magics. Naming c[0] and c[1] would this far more readable.

rzarzynski · 2024-07-13T12:13:47Z

src/pybind/ceph_daemon.py

+    def _handle_sigwinch(self, signo, frame) -> None:
+        self.termsize.update()
+
+    def create_histogram(self, sockets, counter, last, batch):


Why is this a public method? The only caller I'm able to find is _print_vals_histogram().

github-actions · 2024-09-11T13:05:08Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

github-actions · 2024-10-11T14:01:27Z

This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution!

ifed01 added the feature label May 31, 2021

github-actions bot added core documentation pybind labels May 31, 2021

ifed01 requested review from aclamk, jdurgin, markhpc and neha-ojha May 31, 2021 16:16

jdurgin reviewed Jun 9, 2021

View reviewed changes

ifed01 force-pushed the wip-ifed-daemonhistogram branch from 1ad51be to 53cf242 Compare June 16, 2021 17:31

github-actions bot added the tests label Jun 16, 2021

ifed01 force-pushed the wip-ifed-daemonhistogram branch from 53cf242 to b7744fa Compare June 16, 2021 17:43

ifed01 force-pushed the wip-ifed-daemonhistogram branch from b7744fa to 3bbbacd Compare June 28, 2021 12:24

ifed01 force-pushed the wip-ifed-daemonhistogram branch from 3bbbacd to 495b520 Compare July 22, 2021 16:38

ifed01 mentioned this pull request Jul 22, 2021

os/bluestore: implement allocation perf histogram. #42456

Merged

3 tasks

jdurgin approved these changes Jul 28, 2021

View reviewed changes

jdurgin added the needs-qa label Jul 28, 2021

tchaikov added the wip-kefu-testing label Jul 29, 2021

tchaikov requested changes Jul 29, 2021

View reviewed changes

tchaikov removed needs-qa wip-kefu-testing labels Jul 29, 2021

ifed01 force-pushed the wip-ifed-daemonhistogram branch from 495b520 to a504341 Compare July 29, 2021 13:46

tchaikov added needs-qa wip-kefu-testing labels Jul 29, 2021

ifed01 force-pushed the wip-ifed-daemonhistogram branch from 5918bc2 to f4904b8 Compare December 20, 2021 16:24

djgalloway changed the base branch from master to main July 4, 2022 00:00

djgalloway requested a review from a team as a code owner July 4, 2022 00:00

tchaikov added the wip-kefu-testing label Jul 26, 2022

tchaikov requested changes Jul 27, 2022

View reviewed changes

tchaikov removed needs-qa wip-kefu-testing labels Jul 27, 2022

github-actions bot added the stale label Nov 28, 2022

github-actions bot closed this Dec 28, 2022

ifed01 reopened this Jul 12, 2024

ifed01 requested a review from a team as a code owner July 12, 2024 11:49

github-actions bot removed the stale label Jul 12, 2024

anthonyeleven reviewed Jul 12, 2024

View reviewed changes

rzarzynski approved these changes Jul 13, 2024

View reviewed changes

github-actions bot added the stale label Sep 11, 2024

github-actions bot closed this Oct 11, 2024


		Usage::

		ceph daemonperf {type.all\|daemon_names\|socket_paths} [ {batch\|live} [{interval} [{count}]]]

		self.termsize.update()

		def create_histogram(self, sockets, counter, last, batch):

Conversation

ifed01 commented May 31, 2021

Checklist

Uh oh!

ifed01 commented May 31, 2021

Uh oh!

jdurgin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ifed01 commented Jun 16, 2021

Uh oh!

neha-ojha commented Jun 24, 2021

Uh oh!

ifed01 commented Jul 20, 2021

Uh oh!

jdurgin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 28, 2022

Uh oh!

github-actions bot commented Dec 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rzarzynski left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 11, 2024

Uh oh!

github-actions bot commented Oct 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

rzarzynski left a comment •

edited

Loading