kube: notifyproxy: fix lost READY message #15820
Conversation
openshift-ci
bot
added
release-note
approved
vrothberg
force-pushed
the
fix-15800
branch
from
c26d9f8 to
b81c12d
Compare
|
Almost, still needs some massaging |
vrothberg
force-pushed
the
fix-15800
branch
from
b81c12d to
82e0147
Compare
| // goroutines. One waiting for the `READY` message, the other waiting | ||
| // for the container to stop running. | ||
| errorChan := make(chan error, 1) | ||
| readyChan := make(chan bool) |
There was a problem hiding this comment.
I think this needs a defer close for both channels inside their goroutines to prevent write after close problems?
There was a problem hiding this comment.
Wouldn't that cause a write after close? Let's assume we have a read error and the container exits. If routine 1 closes the channel that routine 2 wants to write the error to, we'd run into this issue.
There was a problem hiding this comment.
I made both channels buffered to make sure none of the two routines can block. Once both are done, the channels will get garbage collected. But there is no guarantee that both routines will be finished once the function returns. There is a chance that the container sends ready, does something and exits. Routine 1 receives the READY, routine 2 detects the container isn't running and sends the error before ctx has been cancelled.
There was a problem hiding this comment.
I was trying to say close them inside the goroutine with defer, i.e. on line 121 and 160
There was a problem hiding this comment.
Yes, I understood. You owe an explanation though. Only when closing the channels, they are subject to a write after close. Since they're buffered, the routines cannot block and the channel will be garbage collected when both routines returned. I think that closing makes it more complicated than necessary. But I may be missing something.
Use a wait group to a) wait for all proxies in parallel
b) avoid the potential for ABBA deadlocks
[NO NEW TESTS NEEDED] as it is not changing functionality
Signed-off-by: Valentin Rothberg <[email protected]>
The read deadline may yield the READY message to be lost in space. Instead, use a more Go-idiomatic alternative by using two goroutines; one reading from the connection, the other watching the container. [NO NEW TESTS NEEDED] since existing tests are exercising this functionality already. Fixes: containers#15800 Signed-off-by: Valentin Rothberg <[email protected]>
vrothberg
force-pushed
the
fix-15800
branch
from
82e0147 to
4a053a8
Compare
|
@containers/podman-maintainers PTAL |
|
LGTM |
|
Can we please get this in? If there are concerns left on the code I appreciate we'd resolve them in a timely manner. |
|
I don't feel qualified to lgtm anything having to do with Go channels, but I approve this in principle. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc, giuseppe, vrothberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
github-actions
bot
added
the
locked - please file new issue/PR
See the messages of the two commits.
Does this PR introduce a user-facing change?