remote exec: write conmon error on hijacked connection #9105
Conversation
openshift-ci-robot
added
do-not-merge/work-in-progress
|
Thank you! |
|
@edsantiago this might be it. The code is tricky ... lot's of spaghetti dependencies and I didn't help making it simpler. I'll have another look tomorrow morning with a fresh pair of eyes in hope to find a simpler solution without larger refactorings |
|
Flake #8972, restarted |
Make sure to write error from conmon on the hijacked http connection. This fixes issues where errors were not reported on the client side, for instance, when specified command was not found on the container. To future generations: I am sorry. The code is complex, and there are many interdependencies among the concurrent goroutines. I added more complexity on top but I don't have a good idea of how to reduce complexity in the available time. Fixes: containers#8281 Signed-off-by: Valentin Rothberg <[email protected]>
vrothberg
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
@containers/podman-maintainers PTAL, ready to merge from my side. |
| // Many things happen concurrently and yet are interdependent. | ||
| // If you ever change this function, make sure to write to the | ||
| // conmonPipeDataChan in case of an error. | ||
|
|
There was a problem hiding this comment.
Would it be more elegant to catch the errors and do the conmonPipeData once?
defer func() {
if Err != nil {
conmonPipeDataChan <- conmonPipeData{-1, Err}
return Err
}
}
There was a problem hiding this comment.
Unfortunately that doesn't work unless we introduce another (thread safe) variable that would allow us to know whether we already wrote to the channel.
I can try to make it nicer if you want to.
There was a problem hiding this comment.
I just worry abour a future deveopers adding a return error and not doing writing to the pipe. For now, I will merge.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan, 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 |
|
LGTM |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
Make sure to write error from conmon on the hijacked http connection.
This fixes issues where errors were not reported on the client side,
for instance, when specified command was not found on the container.
To future generations: I am sorry. The code is complex, and there are
many interdependencies among the concurrent goroutines. I added more
complexity on top but I don't have a good idea of how to reduce
complexity in the available time.
Fixes: #8281
Signed-off-by: Valentin Rothberg [email protected]