fix shim std logs not close after shim exit #3311
Conversation
|
Build succeeded.
|
runtime/v1/linux/runtime.go
Outdated
|
|
||
| go func() { | ||
| select { | ||
| case <-shimExit: |
There was a problem hiding this comment.
runtime/v1/linux/runtime.go:397:4:warning: should use a simple channel send/receive instead of select with a single case (S1000) (staticcheck)
please update it~
|
@jing-rui sorry. could you repush it? the failed case is flaky one |
|
Build succeeded.
|
Codecov Report
@@ Coverage Diff @@
## master #3311 +/- ##
======================================
Coverage 44.6% 44.6%
======================================
Files 112 112
Lines 12180 12180
======================================
Hits 5433 5433
Misses 5913 5913
Partials 834 834
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #3311 +/- ##
=======================================
Coverage 44.86% 44.86%
=======================================
Files 113 113
Lines 12338 12338
=======================================
Hits 5535 5535
Misses 5953 5953
Partials 850 850
Continue to review full report at Codecov.
|
runtime/v1/linux/runtime.go
Outdated
| @@ -392,6 +394,16 @@ func (r *Runtime) loadTasks(ctx context.Context, ns string) ([]*Task, error) { | |||
| go io.Copy(ioutil.Discard, shimStderrLog) | |||
| } | |||
|
|
|||
| go func() { | |||
There was a problem hiding this comment.
Would it be better to close these after the io.Copy operations unblock?
There was a problem hiding this comment.
or put the go goroutine at the beginning to make sure that the log can be closed.
There was a problem hiding this comment.
@crosbymichael the io.Copy still block after shim exit, so close it directly after shim exit. or wait it with timeout?
There was a problem hiding this comment.
hi @crosbymichael , the fifo is opened with write and read so that the copy still is blocked after shim exits. So I think we should close the fifo after NewShimClient call.
|
@jing-rui ping ^^ |
|
Build succeeded.
|
runtime/v1/linux/runtime.go
Outdated
| case <-copyDone: | ||
| break | ||
| } | ||
| if src != nil { |
There was a problem hiding this comment.
I think we can remove the nil check here, because the scope is clear here. And the if src is nil, the inner goroutine will panic.
|
Build succeeded.
|
Signed-off-by: Jing Rui <jingrui@huawei.com>
|
LGTM |
Signed-off-by: Jing Rui jingrui@huawei.com