Remove resize race condition

Since podman-remote resize requests can come in at random times, this
generates a real potential for race conditions. We should only be
attempting to resize TTY on running containers, but the containers can
go from running to stopped at any time, and returning an error to the
caller is just causing noice.

This change will basically ignore requests to resize terminals if the
container is not running and return the caller to success.  All other
callers will still return failure.

Fixes: https://github.com/containers/podman/issues/9831

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
This commit is contained in:
Daniel J Walsh
2021-03-26 10:54:33 -04:00
parent c81e273835
commit dcabf6dd71
7 changed files with 44 additions and 13 deletions

View File

@ -21,6 +21,7 @@ func ResizeTTY(w http.ResponseWriter, r *http.Request) {
query := struct { query := struct {
Height uint16 `schema:"h"` Height uint16 `schema:"h"`
Width uint16 `schema:"w"` Width uint16 `schema:"w"`
IgnoreNotRunning bool `schema:"running"`
}{ }{
// override any golang type defaults // override any golang type defaults
} }
@ -48,15 +49,18 @@ func ResizeTTY(w http.ResponseWriter, r *http.Request) {
if state, err := ctnr.State(); err != nil { if state, err := ctnr.State(); err != nil {
utils.InternalServerError(w, errors.Wrapf(err, "cannot obtain container state")) utils.InternalServerError(w, errors.Wrapf(err, "cannot obtain container state"))
return return
} else if state != define.ContainerStateRunning { } else if state != define.ContainerStateRunning && !query.IgnoreNotRunning {
utils.Error(w, "Container not running", http.StatusConflict, utils.Error(w, "Container not running", http.StatusConflict,
fmt.Errorf("container %q in wrong state %q", name, state.String())) fmt.Errorf("container %q in wrong state %q", name, state.String()))
return 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 err := ctnr.AttachResize(sz); err != nil {
if errors.Cause(err) != define.ErrCtrStateInvalid || !query.IgnoreNotRunning {
utils.InternalServerError(w, errors.Wrapf(err, "cannot resize container")) utils.InternalServerError(w, errors.Wrapf(err, "cannot resize container"))
return return
} }
}
// This is not a 204, even though we write nothing, for compatibility // This is not a 204, even though we write nothing, for compatibility
// reasons. // reasons.
status = http.StatusOK status = http.StatusOK
@ -70,15 +74,17 @@ func ResizeTTY(w http.ResponseWriter, r *http.Request) {
if state, err := ctnr.State(); err != nil { if state, err := ctnr.State(); err != nil {
utils.InternalServerError(w, errors.Wrapf(err, "cannot obtain session container state")) utils.InternalServerError(w, errors.Wrapf(err, "cannot obtain session container state"))
return return
} else if state != define.ContainerStateRunning { } else if state != define.ContainerStateRunning && !query.IgnoreNotRunning {
utils.Error(w, "Container not running", http.StatusConflict, utils.Error(w, "Container not running", http.StatusConflict,
fmt.Errorf("container %q in wrong state %q", name, state.String())) fmt.Errorf("container %q in wrong state %q", name, state.String()))
return return
} }
if err := ctnr.ExecResize(name, sz); err != nil { if err := ctnr.ExecResize(name, sz); err != nil {
if errors.Cause(err) != define.ErrCtrStateInvalid || !query.IgnoreNotRunning {
utils.InternalServerError(w, errors.Wrapf(err, "cannot resize session")) utils.InternalServerError(w, errors.Wrapf(err, "cannot resize session"))
return return
} }
}
// This is not a 204, even though we write nothing, for compatibility // This is not a 204, even though we write nothing, for compatibility
// reasons. // reasons.
status = http.StatusCreated status = http.StatusCreated

View File

@ -587,6 +587,11 @@ func (s *APIServer) registerContainersHandlers(r *mux.Router) error {
// type: integer // type: integer
// required: false // required: false
// description: Width to set for the terminal, in characters // description: Width to set for the terminal, in characters
// - in: query
// name: running
// type: boolean
// required: false
// description: Ignore containers not running errors
// produces: // produces:
// - application/json // - application/json
// responses: // responses:

View File

@ -136,6 +136,11 @@ func (s *APIServer) registerExecHandlers(r *mux.Router) error {
// name: w // name: w
// type: integer // type: integer
// description: Width of the TTY session in characters // description: Width of the TTY session in characters
// - in: query
// name: running
// type: boolean
// required: false
// description: Ignore containers not running errors
// produces: // produces:
// - application/json // - application/json
// responses: // responses:

View File

@ -307,6 +307,7 @@ func resizeTTY(ctx context.Context, endpoint string, height *int, width *int) er
if width != nil { if width != nil {
params.Set("w", strconv.Itoa(*width)) params.Set("w", strconv.Itoa(*width))
} }
params.Set("running", "true")
rsp, err := conn.DoRequest(nil, http.MethodPost, endpoint, params, nil) rsp, err := conn.DoRequest(nil, http.MethodPost, endpoint, params, nil)
if err != nil { if err != nil {
return err return err

View File

@ -212,6 +212,7 @@ type RenameOptions struct {
type ResizeTTYOptions struct { type ResizeTTYOptions struct {
Height *int Height *int
Width *int Width *int
Running *bool
} }
//go:generate go run ../generator/generator.go ResizeExecTTYOptions //go:generate go run ../generator/generator.go ResizeExecTTYOptions

View File

@ -51,3 +51,19 @@ func (o *ResizeTTYOptions) GetWidth() int {
} }
return *o.Width return *o.Width
} }
// WithRunning
func (o *ResizeTTYOptions) WithRunning(value bool) *ResizeTTYOptions {
v := &value
o.Running = v
return o
}
// GetRunning
func (o *ResizeTTYOptions) GetRunning() bool {
var running bool
if o.Running == nil {
return running
}
return *o.Running
}

View File

@ -57,9 +57,6 @@ function teardown() {
# ...and make sure stty under podman reads that. # ...and make sure stty under podman reads that.
# FIXME: 'sleep 1' is needed for podman-remote; without it, there's # FIXME: 'sleep 1' is needed for podman-remote; without it, there's
# a race condition resulting in the following warning:
# WARN[0000] failed to resize TTY: container "xx" in wrong state "stopped"
# (also "created")
run_podman run -it --name mystty $IMAGE sh -c 'sleep 1;stty size' <$PODMAN_TEST_PTY run_podman run -it --name mystty $IMAGE sh -c 'sleep 1;stty size' <$PODMAN_TEST_PTY
is "$output" "$rows $cols" "stty under podman reads the correct dimensions" is "$output" "$rows $cols" "stty under podman reads the correct dimensions"
} }