remote: always send resize before the container starts #10549
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99 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 |
openshift-ci
bot
added
the
approved
|
@edsantiago PTAL I think this fixes your issue. |
|
Running this infinite loop for the past five minutes, and it's passing so far: $ while :;do bin/podman-remote run -it --rm quay.io/libpod/testimage:20210427 stty size;doneLGTM, thank you! And thank you for identifying and fixing the appropriate test! |
|
@Luap99 If we don't have a thread watching for resize, then if the terminal actually get's resized, the container will now realize it. Have you tried this out with this change. |
|
@rhatdan Yes the extra thread is still created and resizes when SIGWINCH is received. Only the first resize is made from the attach thread. |
|
LGTM |
pkg/api/handlers/compat/resize.go
Outdated
| @@ -71,14 +62,6 @@ func ResizeTTY(w http.ResponseWriter, r *http.Request) { | |||
| utils.SessionNotFound(w, name, err) | |||
| return | |||
| } | |||
| if state, err := ctnr.State(); err != nil { | |||
There was a problem hiding this comment.
I think this bit is still reasonable to hit - exec sessions in a non-running container will never happen.
There was a problem hiding this comment.
I added this back. It looks podman exec -it con1 stty size is broken as well but for both local and remote.
Luap99
force-pushed
the
fix-9859
branch
2 times, most recently
from
be0f47f to
d747ef7
Compare
pkg/api/handlers/compat/resize.go
Outdated
| utils.Error(w, "Container not running", http.StatusConflict, | ||
| fmt.Errorf("container %q in wrong state %q", name, state.String())) | ||
| return | ||
| } | ||
| // If container is not running, ignore since this can be a race condition, and is expected | ||
| if err := ctnr.AttachResize(sz); err != nil { | ||
| if errors.Cause(err) != define.ErrCtrStateInvalid || !query.IgnoreNotRunning { |
There was a problem hiding this comment.
I think we probably need to catch define.ErrCtrStateInvalid and return a http.StatusConflict down here. It should be a lot more forgiving than the above check, which only allows Running (we can also resize Created containers, and probably Paused containers as well, I forget) but ensures we're still returning good HTTP status codes.
There was a problem hiding this comment.
Not sure about the http status code, this is used for the compat api as well and the docker api does not specify 409.
|
One more nit then LGTM |
There is race condition in the remote client attach logic. Because the resize api call was handled in an extra goroutine the container was started before the resize call happend. To fix this we have to call resize in the same goroutine as attach. When the first resize is done start a goroutine to listen on SIGWINCH in the background and resize again if the signal is received. Fixes containers#9859 Signed-off-by: Paul Holzinger <[email protected]>
|
I think the compose test flaked - restarted /lgtm |
openshift-ci
bot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
Nice work @Luap99. This is a very old issue that @edsantiago has wanted fix for a while. |
github-actions
bot
added
the
locked - please file new issue/PR
There is race condition in the remote client attach logic. Because the
resize api call was handled in an extra goroutine the container was
started before the resize call happend. To fix this we have to call
resize in the same goroutine as attach. When the first resize is done
start a goroutine to listen on SIGWINCH in the background and resize
again if the signal is received.
Fixes #9859