MINOR: Print out stdout output on non-zero exit code in ssh_capture() #234
Conversation
|
@confluentinc It looks like @stanislavkozlovski just signed our Contributor License Agreement. 👍 Always at your service, clabot |
|
It's difficult to catch the stdout output in tests since the exception gets raised in the generator. Here's an example test I had: but it would throw an exception inside the generator, on the last iteration. The output would have been: Making it hard to debug what the issue was. |
| if callback is None: | ||
| yield line | ||
| else: | ||
| yield callback(line) | ||
| try: | ||
| exit_status = stdout.channel.recv_exit_status() | ||
| if exit_status != 0: | ||
| full_output = "stderr: {}\nstdout: {}".format(stderr.read(), ''.join(stdout_lines)) |
There was a problem hiding this comment.
My only nit would be if we want to have some sort of (both easily parseable and visually compelling) delineator between these two output streams. E.g.
===== stderr ===
...
==== stdout ===
...
There was a problem hiding this comment.
The problem with doing this universally though is that it can drastically drive up memory usage when ssh_capture is used for long processes. For example, verifiable_producer and consumer in Kafka actually stream the output of those clients and process them during the test. The tracked state may be much smaller than the raw text output, so tracking all of this could easily more than double the costs (which is why we put SSHOutputIter as the output type originally).
So this should probably be behind a flag (defaulting off at least for now since the new behavior isn't expected by existing uses) or an alternative method.
|
hi @stanislavkozlovski I still feel this would be a great improvment for ducktape as it is one of the greater barriers to debugging right now. Ewen's point still stands. I think something we generally want is the ability to grab stderr from any failed command that fails (maybe by caching stderr into a file on the remote machine and then printing that file out if the command fails otherwise delete the file on completion could be a compromise here) |
|
I would suggest getting together and brainstorming this - it's a good feature for sure, but also no rush on it either. As with any ssh communication changes, I would also want this to be a change on the master branch only (ie no backporting), to have older versions to roll back to |
|
I am also happy to hand this over to anybody willing to take this home |
|
@stanislavkozlovski yeah, our team should be owning this in all honesty. Thanks for pushing it forward! I hope either me or Ian can get some bandwidth to work on this in the near future. |
|
|
This patch changes the ssh_capture command to additionally print out stdout output on non-zero exit codes. While it's expected for the command to print out its error in stderr, not all do - Kafka's tools (e.g kafka-topics) do not, for example.