exec_cmd(): pass stderr along to sos_get_command_output() #2306
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mfoliveira
added a commit
to mfoliveira/sos
that referenced
this pull request
Nov 11, 2020
The sosreport of a system with this issue:
```
# ip netns
Error: Peer netns reference is invalid.
Error: Peer netns reference is invalid.
test-ns
```
Shows that the networking plugin runs commands for the `Error:` lines:
(the ebpf plugin does not; due to differences addressed in PR sosreport#2306.)
```
# ./bin/sos report -o ebpf,networking --batch
# tar tf /tmp/sosreport-*.tar.xz | grep ip_netns_exec
.../sos_commands/ebpf/ip_netns_exec_test-ns_bpftool_net_list
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save.1
...
.../sos_commands/networking/ip_netns_exec_test-ns_ip6tables-save
...
```
This happens because the networking plugin calls `collect_cmd_output()`
with default `stderr=True` and does not handle such line type.
For the purposes of getting the list of network namespaces, it is OK to
ignore `stderr` since it does not provide that information. However, we
do want it in the archive, so it is fully documented for analysis/debug.
And if we get rid of `stderr` there, we can drop the existing check for
`Object unknown` in `ip`, as `iproute2` has always had it in `stderr`.
(This applies to the ebpf plugin as well; it's copied from networking.)
```
@ iproute2.git:
commit aba5acdfdb347d2c21fc67d613d83d4430ca3937
Author: Stephen Hemminger <stephen@networkplumber.org>
Date: Thu Apr 15 20:56:59 2004 +0000
(Logical change 1.3)
...
diff --git a/ip/ip.c b/ip/ip.c
...
+ fprintf(stderr, "Object \"%s\" is unknown, try
\"ip help\".\n", argv[1]);
+ exit(-1);
...
```
Note that both plugins _currently_ do not need `exec_cmd(stderr=False)`
(see output above) as described in PR sosreport#2306, but it will once/if that's
applied. So, add that proactively; and it doesn't hurt/has any effects
without that PR anyway.
Before:
```
# tar tf /tmp/sosreport-*.tar.xz | grep ip_netns_exec
.../sos_commands/ebpf/ip_netns_exec_test-ns_bpftool_net_list
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save.1
...
.../sos_commands/networking/ip_netns_exec_test-ns_ip6tables-save
...
```
After:
```
# tar tf /tmp/sosreport-*.tar.xz | grep ip_netns_exec
.../sos_commands/ebpf/ip_netns_exec_test-ns_bpftool_net_list
.../sos_commands/networking/ip_netns_exec_test-ns_ip6tables-save
.../sos_commands/networking/ip_netns_exec_test-ns_ip_-s_-s_neigh_show
...
```
And the `ip netns` contents with `stderr` still remain in the archive:
```
# tar xf /tmp/sosreport-*.tar.xz --to-stdout \
--wildcards '*/sos_commands/networking/ip_netns'
Error: Peer netns reference is invalid.
Error: Peer netns reference is invalid.
test-ns
```
Test suite:
```
# ./tests/simple.sh
...
Everything worked!
```
Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
6 tasks
mfoliveira
added a commit
to mfoliveira/sos
that referenced
this pull request
Nov 12, 2020
The sosreport of a system with this issue:
```
# ip netns
Error: Peer netns reference is invalid.
Error: Peer netns reference is invalid.
test-ns
```
Shows that the networking plugin runs commands for the `Error:` lines:
(note the ebpf plugin does not due to differences addressed in PR sosreport#2306)
```
# ./bin/sos report -o ebpf,networking --batch
# tar tf /tmp/sosreport-*.tar.xz | grep ip_netns_exec
.../sos_commands/ebpf/ip_netns_exec_test-ns_bpftool_net_list
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save.1
...
.../sos_commands/networking/ip_netns_exec_test-ns_ip6tables-save
...
```
This happens because the networking plugin calls `collect_cmd_output()`
with default `stderr=True` and does not handle such line type.
For the purposes of getting the list of network namespaces, it is OK to
ignore `stderr` since it does not provide that information. However, we
do want it in the archive, so it is fully documented for analysis/debug.
So change the call from `collect_cmd_output()` to `add_cmd_output()` to
include both `stdout` and `stderr` in the archive, and call `exec_cmd()`
that ignores `stderr` to get the list of network namespaces.
Note that the plugin _currently_ does not need `exec_cmd(stderr=False)`
to ignore `stderr`, as described in PR#2306, but will once/if that is
applied. However, with the next patch, `stderr=False` won't be needed.
Before:
```
# tar tf /tmp/sosreport-*.tar.xz | grep ip_netns_exec
.../sos_commands/ebpf/ip_netns_exec_test-ns_bpftool_net_list
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save.1
...
.../sos_commands/networking/ip_netns_exec_test-ns_ip6tables-save
...
```
After:
```
# tar tf /tmp/sosreport-*.tar.xz | grep ip_netns_exec
.../sos_commands/ebpf/ip_netns_exec_test-ns_bpftool_net_list
.../sos_commands/networking/ip_netns_exec_test-ns_ip6tables-save
.../sos_commands/networking/ip_netns_exec_test-ns_ip_-s_-s_neigh_show
...
```
And the `ip netns` contents with `stderr` still remain in the archive:
```
# tar xf /tmp/sosreport-*.tar.xz --to-stdout \
--wildcards '*/sos_commands/networking/ip_netns'
Error: Peer netns reference is invalid.
Error: Peer netns reference is invalid.
test-ns
```
Test suite:
```
# ./tests/simple.sh
...
Everything worked!
```
Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
mfoliveira
added a commit
to mfoliveira/sos
that referenced
this pull request
Nov 12, 2020
The sosreport of a system with this issue:
```
# ip netns
Error: Peer netns reference is invalid.
Error: Peer netns reference is invalid.
test-ns
```
Shows that the networking plugin runs commands for the `Error:` lines:
(note the ebpf plugin does not due to differences addressed in PR sosreport#2306)
```
# ./bin/sos report -o ebpf,networking --batch
# tar tf /tmp/sosreport-*.tar.xz | grep ip_netns_exec
.../sos_commands/ebpf/ip_netns_exec_test-ns_bpftool_net_list
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save.1
...
.../sos_commands/networking/ip_netns_exec_test-ns_ip6tables-save
...
```
This happens because the networking plugin calls `collect_cmd_output()`
with default `stderr=True` and does not handle such line type.
For the purposes of getting the list of network namespaces, it is OK to
ignore `stderr` since it does not provide that information. However, we
do want it in the archive, so it is fully documented for analysis/debug.
So change the call from `collect_cmd_output()` to `add_cmd_output()` to
include both `stdout` and `stderr` in the archive, and call `exec_cmd()`
that ignores `stderr` to get the list of network namespaces.
Note that the plugin _currently_ does not need `exec_cmd(stderr=False)`
to ignore `stderr`, as described in PR#2306, but will once/if that is
applied. However, with the next patch, `stderr=False` won't be needed.
Before:
```
# tar tf /tmp/sosreport-*.tar.xz | grep ip_netns_exec
.../sos_commands/ebpf/ip_netns_exec_test-ns_bpftool_net_list
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save.1
...
.../sos_commands/networking/ip_netns_exec_test-ns_ip6tables-save
...
```
After:
```
# tar tf /tmp/sosreport-*.tar.xz | grep ip_netns_exec
.../sos_commands/ebpf/ip_netns_exec_test-ns_bpftool_net_list
.../sos_commands/networking/ip_netns_exec_test-ns_ip6tables-save
.../sos_commands/networking/ip_netns_exec_test-ns_ip_-s_-s_neigh_show
...
```
And the `ip netns` contents with `stderr` still remain in the archive:
```
# tar xf /tmp/sosreport-*.tar.xz --to-stdout \
--wildcards '*/sos_commands/networking/ip_netns'
Error: Peer netns reference is invalid.
Error: Peer netns reference is invalid.
test-ns
```
Test suite:
```
# ./tests/simple.sh
...
Everything worked!
```
Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
The sosreport of a system with this issue:
```
# ip netns
Error: Peer netns reference is invalid.
Error: Peer netns reference is invalid.
test-ns
```
Shows this difference between the plugins ebpf and networking (callers
of `ip netns`):
```
# ./bin/sos report -o ebpf,networking --batch
# tar tf /tmp/sosreport-*.tar.xz | grep ip_netns_exec
.../sos_commands/ebpf/ip_netns_exec_test-ns_bpftool_net_list
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save.1
...
.../sos_commands/networking/ip_netns_exec_test-ns_ip6tables-save
...
```
Only the networking plugin called `ip netns exec <ns> <cmd>` on `Error:`
lines. Hmm.
...
The networking plugin calls `collect_cmd_output()`, which has parameter
stderr=True` as default:
```
def collect_cmd_output(self, cmd, suggest_filename=None,
...
stderr=True, ...
```
The ebpf plugin calls `exec_cmd()`, BUT it has parameter `stderr=True`
as default AS WELL:
```
def exec_cmd(self, cmd, timeout=cmd_timeout, stderr=True, ...
```
So, why the difference?
...
Well, it turns out `exec_cmd()` does NOT pass the `stderr` parameter
along to `sos_get_command_output()`: [but `collect_cmd_output()` does.]
```
return sos_get_command_output(cmd, timeout=timeout, chroot=root,
chdir=runat, binary=binary, env=env,
foreground=foreground)
```
And `sos_get_command_output()` has `stderr=False` as default:
```
def sos_get_command_output(command, timeout=300, stderr=False, ...
```
Thus, despite `exec_cmd()` has `stderr=True` as default it is _ignored_,
as its callee `sos_get_command_output()` has `stderr=False` as default.
This explains the output difference between the ebpf/networking plugins.
...
Looking back for details, the origin of the change is in PR#1807 [1],
with commit e8bb94c (refactor command functions/introduce `exec_cmd()`)
and commit e51d3e6 (update caller plugins).
The PR has no mentions/discussion around `stderr` in comments/review,
so it's apparently an oversight during the refactor, I guess.
Because, previously the refactored callers of `sos_get_command_output()`
(`get_command_output()`, `call_ext_prog()`, `[_]get_cmd_output_now()`)
all had `stderr=True` as parameter/default, passing it along to callee.
(The only exception is in `_collect_cmd_output()` which does the first
call in the chroot with `stderr`, and falls back to non-chroot without
`stderr`, but it was already that way. Thus not changing this case.)
...
So, it seems right and safe and opportune to fix this inconsistency
between default `stderr` in `exec_cmd()` / `sos_get_command_output()`
by just passing it along between them:
Right: because it restores the previous behavior (before refactor)
assumed/accepted by callers that did not specify `stderr=False`.
Safe: because it restores the actual behavior to previous behavior,
and if plugins handled `stderr` lines previously, they should also
handle it now, for the most part (i.e., not any new `stderr` lines,
but they are currently ignoring those, and should get them covered.)
Opportune: because if any plugin breaks _now_ because of this, it is
a new/post-refactor change that decided to use `exec_cmd()` with the
default that is inconsistent with actual behavior, and it seems okay
to take bugs for that, so to identify and fix such cases.
Hopefully those points are reasonable with the project's philosophy.
...
Test-case:
Setup:
```
$ sudo python3
from sos import SoS
from sos.report.plugins import Plugin
sos = SoS(['report'])
plugin = Plugin(sos._component.get_commons())
```
Before:
```
>>> print(plugin.exec_cmd('ip netns')['output'])
test-ns
>>> print(plugin.exec_cmd('ip netns', stderr=True)['output'])
test-ns
>>> print(plugin.exec_cmd('ip netns', stderr=False)['output'])
test-ns
```
After:
```
>>> print(plugin.exec_cmd('ip netns')['output'])
Error: Peer netns reference is invalid.
Error: Peer netns reference is invalid.
test-ns
>>> print(plugin.exec_cmd('ip netns', stderr=True)['output'])
Error: Peer netns reference is invalid.
Error: Peer netns reference is invalid.
test-ns
>>> print(plugin.exec_cmd('ip netns', stderr=False)['output'])
test-ns
```
...
Test-suite:
```
# ./tests/simple.sh
<...>
Summary
failures false time 2 -l
failures false time 3 --list-presets
failures false time 3 --list-profiles
failures false time 17 --batch --build
--no-env-vars
failures false time 3 Size 14.23KiB --batch
--no-report -o hardware
failures false time 20 Size 2.99MiB --batch
--label TEST -a -c never
failures false time 20 Size 2.81MiB --batch
--debug --log-size 0 -c always
failures false time 20 Size 2.84MiB --batch
-z xz --log-size 1
failures false time 19 Size 4.01MiB --batch
-z gzip
failures false time 32 Size 2.76MiB --batch
-t 1 -n hardware
failures false time 19 --batch --quiet
-e opencl -k kernel.with-timer
failures false time 22 Size 4.31MiB --batch
--case-id 10101 --all-logs --since=20201110
failures false time 21 Size 3.06MiB --batch
--verbose --no-postproc
failures false time 39 Size 2.82MiB --batch
--mask
Everything worked!
```
[1] sosreport#1807
Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
mfoliveira
force-pushed
the
exec_cmd_stderr
branch
from
e8e14cd
to
8e25690
Compare
TurboTurtle
approved these changes
Nov 12, 2020
|
Nice catch on this! |
pmoravec
approved these changes
Nov 13, 2020
mfoliveira
added a commit
to mfoliveira/sos
that referenced
this pull request
Nov 18, 2020
The sosreport of a system with this issue:
```
# ip netns
Error: Peer netns reference is invalid.
Error: Peer netns reference is invalid.
test-ns
```
Shows that the networking plugin runs commands for the `Error:` lines:
(note the ebpf plugin does not due to differences addressed in PR sosreport#2306)
```
# ./bin/sos report -o ebpf,networking --batch
# tar tf /tmp/sosreport-*.tar.xz | grep ip_netns_exec
.../sos_commands/ebpf/ip_netns_exec_test-ns_bpftool_net_list
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save.1
...
.../sos_commands/networking/ip_netns_exec_test-ns_ip6tables-save
...
```
This happens because the networking plugin calls `collect_cmd_output()`
with default `stderr=True` and does not handle such line type.
For the purposes of getting the list of network namespaces, it is OK to
ignore `stderr` since it does not provide that information. However, we
do want it in the archive, so it is fully documented for analysis/debug.
So change the call from `collect_cmd_output()` to `add_cmd_output()` to
include both `stdout` and `stderr` in the archive, and call `exec_cmd()`
that ignores `stderr` to get the list of network namespaces.
Note that the plugin _currently_ does not need `exec_cmd(stderr=False)`
to ignore `stderr`, as described in PR#2306, but will once/if that is
applied. However, with the next patch, `stderr=False` won't be needed.
Before:
```
# tar tf /tmp/sosreport-*.tar.xz | grep ip_netns_exec
.../sos_commands/ebpf/ip_netns_exec_test-ns_bpftool_net_list
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save.1
...
.../sos_commands/networking/ip_netns_exec_test-ns_ip6tables-save
...
```
After:
```
# tar tf /tmp/sosreport-*.tar.xz | grep ip_netns_exec
.../sos_commands/ebpf/ip_netns_exec_test-ns_bpftool_net_list
.../sos_commands/networking/ip_netns_exec_test-ns_ip6tables-save
.../sos_commands/networking/ip_netns_exec_test-ns_ip_-s_-s_neigh_show
...
```
And the `ip netns` contents with `stderr` still remain in the archive:
```
# tar xf /tmp/sosreport-*.tar.xz --to-stdout \
--wildcards '*/sos_commands/networking/ip_netns'
Error: Peer netns reference is invalid.
Error: Peer netns reference is invalid.
test-ns
```
Test suite:
```
# ./tests/simple.sh
...
Everything worked!
```
Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
mfoliveira
added a commit
to mfoliveira/sos
that referenced
this pull request
Nov 19, 2020
The sosreport of a system with this issue:
```
# ip netns
Error: Peer netns reference is invalid.
Error: Peer netns reference is invalid.
test-ns
```
Shows that the networking plugin runs commands for the `Error:` lines:
(note the ebpf plugin does not due to differences addressed in PR sosreport#2306)
```
# ./bin/sos report -o ebpf,networking --batch
# tar tf /tmp/sosreport-*.tar.xz | grep ip_netns_exec
.../sos_commands/ebpf/ip_netns_exec_test-ns_bpftool_net_list
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save.1
...
.../sos_commands/networking/ip_netns_exec_test-ns_ip6tables-save
...
```
This happens because the networking plugin calls `collect_cmd_output()`
with default `stderr=True` and does not handle such line type.
For the purposes of getting the list of network namespaces, it is OK to
ignore `stderr` since it does not provide that information. However, we
do want it in the archive, so it is fully documented for analysis/debug.
So change the call from `collect_cmd_output()` to `add_cmd_output()` to
include both `stdout` and `stderr` in the archive, and call `exec_cmd()`
that ignores `stderr` to get the list of network namespaces.
Note that the plugin _currently_ does not need `exec_cmd(stderr=False)`
to ignore `stderr`, as described in PR#2306, but will once/if that is
applied. However, with the next patch, `stderr=False` won't be needed.
Before:
```
# tar tf /tmp/sosreport-*.tar.xz | grep ip_netns_exec
.../sos_commands/ebpf/ip_netns_exec_test-ns_bpftool_net_list
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save.1
...
.../sos_commands/networking/ip_netns_exec_test-ns_ip6tables-save
...
```
After:
```
# tar tf /tmp/sosreport-*.tar.xz | grep ip_netns_exec
.../sos_commands/ebpf/ip_netns_exec_test-ns_bpftool_net_list
.../sos_commands/networking/ip_netns_exec_test-ns_ip6tables-save
.../sos_commands/networking/ip_netns_exec_test-ns_ip_-s_-s_neigh_show
...
```
And the `ip netns` contents with `stderr` still remain in the archive:
```
# tar xf /tmp/sosreport-*.tar.xz --to-stdout \
--wildcards '*/sos_commands/networking/ip_netns'
Error: Peer netns reference is invalid.
Error: Peer netns reference is invalid.
test-ns
```
Test suite:
```
# ./tests/simple.sh
...
Everything worked!
```
Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
mfoliveira
added a commit
to mfoliveira/sos
that referenced
this pull request
Nov 19, 2020
The sosreport of a system with this issue:
```
# ip netns
Error: Peer netns reference is invalid.
Error: Peer netns reference is invalid.
test-ns
```
Shows that the networking plugin runs commands for the `Error:` lines:
(note the ebpf plugin does not due to differences addressed in PR sosreport#2306)
```
# ./bin/sos report -o ebpf,networking --batch
# tar tf /tmp/sosreport-*.tar.xz | grep ip_netns_exec
.../sos_commands/ebpf/ip_netns_exec_test-ns_bpftool_net_list
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save.1
...
.../sos_commands/networking/ip_netns_exec_test-ns_ip6tables-save
...
```
This happens because the networking plugin calls `collect_cmd_output()`
with default `stderr=True` and does not handle such line type.
For the purposes of getting the list of network namespaces, it is OK to
ignore `stderr` since it does not provide that information. However, we
do want it in the archive, so it is fully documented for analysis/debug.
So change the call from `collect_cmd_output()` to `add_cmd_output()` to
include both `stdout` and `stderr` in the archive, and call `exec_cmd()`
that ignores `stderr` to get the list of network namespaces.
Note that the plugin _currently_ does not need `exec_cmd(stderr=False)`
to ignore `stderr`, as described in PR#2306, but will once/if that is
applied. However, with the next patch, `stderr=False` won't be needed.
Before:
```
# tar tf /tmp/sosreport-*.tar.xz | grep ip_netns_exec
.../sos_commands/ebpf/ip_netns_exec_test-ns_bpftool_net_list
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save.1
...
.../sos_commands/networking/ip_netns_exec_test-ns_ip6tables-save
...
```
After:
```
# tar tf /tmp/sosreport-*.tar.xz | grep ip_netns_exec
.../sos_commands/ebpf/ip_netns_exec_test-ns_bpftool_net_list
.../sos_commands/networking/ip_netns_exec_test-ns_ip6tables-save
.../sos_commands/networking/ip_netns_exec_test-ns_ip_-s_-s_neigh_show
...
```
And the `ip netns` contents with `stderr` still remain in the archive:
```
# tar xf /tmp/sosreport-*.tar.xz --to-stdout \
--wildcards '*/sos_commands/networking/ip_netns'
Error: Peer netns reference is invalid.
Error: Peer netns reference is invalid.
test-ns
```
Test suite:
```
# ./tests/simple.sh
...
Everything worked!
```
Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
TurboTurtle
pushed a commit
that referenced
this pull request
Nov 23, 2020
The sosreport of a system with this issue:
```
# ip netns
Error: Peer netns reference is invalid.
Error: Peer netns reference is invalid.
test-ns
```
Shows that the networking plugin runs commands for the `Error:` lines:
(note the ebpf plugin does not due to differences addressed in PR #2306)
```
# ./bin/sos report -o ebpf,networking --batch
# tar tf /tmp/sosreport-*.tar.xz | grep ip_netns_exec
.../sos_commands/ebpf/ip_netns_exec_test-ns_bpftool_net_list
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save.1
...
.../sos_commands/networking/ip_netns_exec_test-ns_ip6tables-save
...
```
This happens because the networking plugin calls `collect_cmd_output()`
with default `stderr=True` and does not handle such line type.
For the purposes of getting the list of network namespaces, it is OK to
ignore `stderr` since it does not provide that information. However, we
do want it in the archive, so it is fully documented for analysis/debug.
So change the call from `collect_cmd_output()` to `add_cmd_output()` to
include both `stdout` and `stderr` in the archive, and call `exec_cmd()`
that ignores `stderr` to get the list of network namespaces.
Note that the plugin _currently_ does not need `exec_cmd(stderr=False)`
to ignore `stderr`, as described in PR#2306, but will once/if that is
applied. However, with the next patch, `stderr=False` won't be needed.
Before:
```
# tar tf /tmp/sosreport-*.tar.xz | grep ip_netns_exec
.../sos_commands/ebpf/ip_netns_exec_test-ns_bpftool_net_list
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save
.../sos_commands/networking/ip_netns_exec_Error_ip6tables-save.1
...
.../sos_commands/networking/ip_netns_exec_test-ns_ip6tables-save
...
```
After:
```
# tar tf /tmp/sosreport-*.tar.xz | grep ip_netns_exec
.../sos_commands/ebpf/ip_netns_exec_test-ns_bpftool_net_list
.../sos_commands/networking/ip_netns_exec_test-ns_ip6tables-save
.../sos_commands/networking/ip_netns_exec_test-ns_ip_-s_-s_neigh_show
...
```
And the `ip netns` contents with `stderr` still remain in the archive:
```
# tar xf /tmp/sosreport-*.tar.xz --to-stdout \
--wildcards '*/sos_commands/networking/ip_netns'
Error: Peer netns reference is invalid.
Error: Peer netns reference is invalid.
test-ns
```
Test suite:
```
# ./tests/simple.sh
...
Everything worked!
```
Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
Signed-off-by: Jake Hunsaker <jhunsake@redhat.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The sosreport of a system with this issue:
Shows this difference between the plugins ebpf and networking (callers
of
ip netns):Only the networking plugin called
ip netns exec <ns> <cmd>onError:lines. Hmm.
...
The networking plugin calls
collect_cmd_output(), which has parameterstderr=True` as default:
The ebpf plugin calls
exec_cmd(), BUT it has parameterstderr=Trueas default AS WELL:
So, why the difference?
...
Well, it turns out
exec_cmd()does NOT pass thestderrparameteralong to
sos_get_command_output(): [butcollect_cmd_output()does.]And
sos_get_command_output()hasstderr=Falseas default:Thus, despite
exec_cmd()hasstderr=Trueas default it is ignored,as its callee
sos_get_command_output()hasstderr=Falseas default.This explains the output difference between the ebpf/networking plugins.
...
Looking back for details, the origin of the change is in PR#1807 [1],
with commit e8bb94c (refactor command functions/introduce
exec_cmd())and commit e51d3e6 (update caller plugins).
The PR has no mentions/discussion around
stderrin comments/review,so it's apparently an oversight during the refactor, I guess.
Because, previously the refactored callers of
sos_get_command_output()(
get_command_output(),call_ext_prog(),[_]get_cmd_output_now())all had
stderr=Trueas parameter/default, passing it along to callee.(The only exception is in
_collect_cmd_output()which does the firstcall in the chroot with
stderr, and falls back to non-chroot withoutstderr, but it was already that way. Thus not changing this case.)...
So, it seems right and safe and opportune to fix this inconsistency
between default
stderrinexec_cmd()/sos_get_command_output()by just passing it along between them:
Right: because it restores the previous behavior (before refactor)
assumed/accepted by callers that did not specify
stderr=False.Safe: because it restores the actual behavior to previous behavior,
and if plugins handled
stderrlines previously, they should alsohandle it now, for the most part (i.e., not any new
stderrlines,but they are currently ignoring those, and should get them covered.)
Opportune: because if any plugin breaks now because of this, it is
a new/post-refactor change that decided to use
exec_cmd()with thedefault that is inconsistent with actual behavior, and it seems okay
to take bugs for that, so to identify and fix such cases.
Hopefully those points are reasonable with the project's philosophy.
...
Test-case:
Setup:
Before:
After:
...
Test-suite:
[1] #1807
Signed-off-by: Mauricio Faria de Oliveira mfo@canonical.com
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines
Closes: #ISSUENUMBERincluded in an independent line?Resolves: #PRNUMBERincluded in an independent line?