osd: fix the "heap" admin cmd printing always to error stream #47650

rzarzynski · 2022-08-17T13:24:10Z

Before the patch ceph::osd_cmds::heap() was confusing the concepts of stderr and stdout. This was the direct cause of the differences in output between ceph tell and ceph daeamon.

Thanks to Laura Flores who made the extremely useful observation noted in https://tracker.ceph.com/issues/57119#note-3.

Fixes: https://tracker.ceph.com/issues/57119
Signed-off-by: Radoslaw Zarzynski rzarzyns@redhat.com

CC: @ljflores.

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

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 dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows

pdvian

LGTM. Thanks Radoslaw! I saw your PR a bit late, actually I was working on similar approach. We need similar fix for MDS as well.

src/osd/OSD.cc

ljflores · 2022-08-17T14:23:28Z

LGTM. Thanks Radoslaw! I saw your PR a bit late, actually I was working on similar approach. We need similar fix for MDS as well.

Agree with @pdvian, this looks good. Just need to be applied to MDS as well.

rzarzynski · 2022-08-17T14:39:17Z

We need similar fix for MDS as well.

Working on that. However, when experimenting, I encountered a broader difference between tell and daemon on what is being done with the error stream if rval >= 0. It can be affecting more commands . See: https://tracker.ceph.com/issues/57119#note-9.

Before the patch `ceph::osd_cmds::heap()` was confusing the concepts of _stderr_ and _stdout_. This was the direct cause of the differences in output between `ceph tell` and `ceph daeamon`. Thanks to Laura Flores who made the extremely useful observation noted in https://tracker.ceph.com/issues/57119#note-3. Fixes: https://tracker.ceph.com/issues/57119 Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

vshankar

Thanks for the fix.

rzarzynski · 2022-08-18T07:48:32Z

jenkins test make check

rzarzynski · 2022-08-19T09:00:21Z

The following tests FAILED:
	195 - unittest_osdmap (Subprocess aborted)

rzarzynski · 2022-08-19T09:00:33Z

jenkins retest this please

ljflores · 2022-08-19T21:55:38Z

jenkins retest this please

ljflores

I was initially worried about how this PR might change how mgr modules can access heap command output, i.e. Telemetry. Currently, Telemetry accesses heap stats output via the "error" stream, which is a bug as pointed out in this PR. But we still need to maintain this connection between the heap admin command and the mgr module.

After a conversation with @rzarzynski, we found that manager modules will still be able to access heap stats output despite it not being sent in the error stream. Instead, it will be accessible via outbl, as the out stream is appended there.

For example, Telemetry's current implementation for accessing heap stats is:
https://github.com/ceph/ceph/blob/main/src/pybind/mgr/telemetry/module.py#L478-L483

cmd_dict = {
    'prefix': 'heap',
    'heapcmd': 'stats',
    'id': str(osd_id),
}
r, outb, outs = self.osd_command(cmd_dict)

Where r is the ret value, outb contains JSON where applicable, and outs is the error stream which would contain heap stats string output.

With Radek's change, the heap stats output would be accessible via outb instead of outs. I previously thought that outb only held JSON output, but per Radek's PR, the result from the outstream is also appended there. This means that we can still access heap stats that way, simply by referencing outb instead of outs.

It is important to me that this communication between the heap admin command and the mgr is maintained, and now I see that it is.

@jdurgin this is was I was explaining to you earlier today; we have sorted it out! :)

jdurgin

Great! Glad all the output is still available for the mgr too!

ljflores

@rzarzynski can we leave a note here under send_command to clarify how output is received?

ceph/src/pybind/mgr/mgr_module.py

Lines 1652 to 1653 in 9a5e897

    
                   Called by the plugin to send a command to the mon 
        
                   cluster.

Called by the plugin to send a command to the mon
cluster.
+
+ Result contains three values:
+ 1. status int
+ 2. out std (can be used to load JSON output or receive standard string output)
+ 3. err str  (receives error string output)

Something like this to clarify how output can be processed in the manager module.

Following the merge of ceph#47650, which fixes the confusion between std out and std err in admin socket commands, we will need to reference the out stream (outb) instead of the error stream (outs) when we parse heap stats. Signed-off-by: Laura Flores <lflores@redhat.com>

ljflores · 2022-08-24T23:27:54Z

Gonna get started on testing this: https://shaman.ceph.com/builds/ceph/rzarzynski-wip-bug-57119/94436475c4c9a11a94dd21c619c9ae0014ac393d/default/315415/

rishabh-d-dave · 2022-08-26T12:51:01Z

QA run for CephFS was successful - https://tracker.ceph.com/projects/cephfs/wiki/Main#2022-Aug-26

Following the merge of #47650, which fixes the confusion between std out and std err in admin socket commands, we will need to reference the out stream (outb) instead of the error stream (outs) when we parse heap stats. Signed-off-by: Laura Flores <lflores@redhat.com> (cherry picked from commit 025f810)

yaarith · 2022-08-31T15:20:59Z

jenkins test make check arm64

yaarith · 2022-08-31T15:21:48Z

jenkins test windows

NitzanMordhai · 2022-09-01T12:35:29Z

Unrelated failure:

7003409\7003415\7003419 - The CustomResourceDefinition "installations.operator.tigera.io" is invalid: metadata.annotations: Too long: must have at most 262144 bytes
7003410\7003424\7003414 - ERROR: A cluster with the same fsid '00000000-0000-0000-0000-0000deadbeef' already exists.
7003412 - 'check osd count' reached maximum tries (90) after waiting for 900 seconds
7003413\7003422 - free(): invalid pointer

tracked by:

yuriw · 2022-09-01T15:30:17Z

jenkins test make check

yuriw · 2022-09-01T20:34:09Z

jenkins test make check

Following the merge of #47650, which fixes the confusion between std out and std err in admin socket commands, we will need to reference the out stream (outb) instead of the error stream (outs) when we parse heap stats. Signed-off-by: Laura Flores <lflores@redhat.com>

rzarzynski added the bug-fix label Aug 17, 2022

rzarzynski requested a review from a team as a code owner August 17, 2022 13:24

github-actions bot added the core label Aug 17, 2022

pdvian approved these changes Aug 17, 2022

View reviewed changes

tchaikov reviewed Aug 17, 2022

View reviewed changes

src/osd/OSD.cc Show resolved Hide resolved

rzarzynski force-pushed the wip-bug-57119 branch from 8c15e54 to 9443647 Compare August 17, 2022 14:43

github-actions bot added the cephfs Ceph File System label Aug 17, 2022

vshankar approved these changes Aug 18, 2022

View reviewed changes

vshankar added the wip-rishabh-testing Rishabh's testing label label Aug 18, 2022

ljflores approved these changes Aug 19, 2022

View reviewed changes

ljflores requested a review from jdurgin August 19, 2022 22:12

jdurgin approved these changes Aug 19, 2022

View reviewed changes

ljflores reviewed Aug 19, 2022

View reviewed changes

ljflores mentioned this pull request Aug 19, 2022

mgr/telemetry: add perf_memory_metrics collection to telemetry #47184

Merged

14 tasks

ljflores added needs-qa needs-quincy-backport backport required for quincy needs-pacific-backport PR needs a pacific backport labels Aug 23, 2022

ljflores added the wip-lflores-testing label Aug 24, 2022

rishabh-d-dave removed the wip-rishabh-testing Rishabh's testing label label Aug 26, 2022

ljflores mentioned this pull request Aug 26, 2022

quincy: osd, mds: fix the "heap" admin cmd printing always to error stream #47825

Merged

14 tasks

yuriw added the wip-yuri7-testing label Aug 31, 2022

yuriw added TESTED ready-to-merge labels Sep 1, 2022

yuriw merged commit e1f12f7 into ceph:main Sep 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

osd: fix the "heap" admin cmd printing always to error stream #47650

osd: fix the "heap" admin cmd printing always to error stream #47650

rzarzynski commented Aug 17, 2022

pdvian left a comment

ljflores commented Aug 17, 2022

rzarzynski commented Aug 17, 2022

vshankar left a comment

rzarzynski commented Aug 18, 2022

rzarzynski commented Aug 19, 2022

rzarzynski commented Aug 19, 2022

ljflores commented Aug 19, 2022

ljflores left a comment •

edited

jdurgin left a comment

ljflores left a comment •

edited

ljflores commented Aug 24, 2022

rishabh-d-dave commented Aug 26, 2022

yaarith commented Aug 31, 2022

yaarith commented Aug 31, 2022

NitzanMordhai commented Sep 1, 2022 •

edited

yuriw commented Sep 1, 2022

yuriw commented Sep 1, 2022

osd: fix the "heap" admin cmd printing always to error stream #47650

osd: fix the "heap" admin cmd printing always to error stream #47650

Conversation

rzarzynski commented Aug 17, 2022

Contribution Guidelines

Checklist

pdvian left a comment

Choose a reason for hiding this comment

ljflores commented Aug 17, 2022

rzarzynski commented Aug 17, 2022

vshankar left a comment

Choose a reason for hiding this comment

rzarzynski commented Aug 18, 2022

rzarzynski commented Aug 19, 2022

rzarzynski commented Aug 19, 2022

ljflores commented Aug 19, 2022

ljflores left a comment • edited

Choose a reason for hiding this comment

jdurgin left a comment

Choose a reason for hiding this comment

ljflores left a comment • edited

Choose a reason for hiding this comment

ljflores commented Aug 24, 2022

rishabh-d-dave commented Aug 26, 2022

yaarith commented Aug 31, 2022

yaarith commented Aug 31, 2022

NitzanMordhai commented Sep 1, 2022 • edited

yuriw commented Sep 1, 2022

yuriw commented Sep 1, 2022

ljflores left a comment •

edited

ljflores left a comment •

edited

NitzanMordhai commented Sep 1, 2022 •

edited