cli: container: remove logging in wait-for-exit paths #1731
Conversation
|
This is touching some historically hairy code, sorry for awakening this beast again. :P |
Codecov Report
@@ Coverage Diff @@
## master #1731 +/- ##
==========================================
- Coverage 56.14% 56.13% -0.01%
==========================================
Files 306 306
Lines 21033 21035 +2
==========================================
Hits 11809 11809
- Misses 8369 8370 +1
- Partials 855 856 +1 |
|
Oh boy, this is indeed some hairy code; wondering if @cpuguy83 @mlaventure ptal |
The wait code can output log messages, which can end up with you seeing "errors" due to not being able to get the exit code, when in reality the context is timing our or being cancelled. I'm not sure how common this is on x86, but I can see it all the time on arm64 -- and these error messages seem quite noisy to me (they also cause the integration-cli tests to become quite flaky in my testing). Signed-off-by: Aleksa Sarai <asarai@suse.de>
|
@cpuguy83 ptal |
| statusChan := waitExitOrRemoved(ctx, dockerCli, createResponse.ID, copts.autoRemove) | ||
| // We don't need to wait if we're not attaching. | ||
| var statusChan <-chan int | ||
| if attach { |
There was a problem hiding this comment.
I'm not sure if attach is right here, but I really don't understand config.Attach* at all anyway. Right now we might get a panic if you do -a stdin (or in some of the error paths) because we might read from a nil chan -- but I don't see a nice way of solving that.
A simpler way of stopping the issue behind this problem is to just drop the logging code from waitExitOrRemoved.
There was a problem hiding this comment.
Reading from a nil chain would block forever, not panic.
So we either need another if attach before reading below (so as not to block forever) or make a channel and close it...
I agree waitExitOrRemoved should not be logging.
It could return a channel can handle both exit status and error messages.
I think removing logging, or some way to gate logging, might be a better approach.
The wait code can output log messages, which can end up with you seeing
"errors" due to not being able to get the exit code, when in reality the
context is timing our or being cancelled. I'm not sure how common this
is on x86, but I can see it all the time on arm64 -- and these error
messages seem quite noisy to me (they also cause the integration-cli
tests to become quite flaky in my testing).
Signed-off-by: Aleksa Sarai asarai@suse.de