Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remote: always send resize before the container starts #10549

Merged
merged 1 commit into from
Jun 5, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 4 additions & 11 deletions pkg/api/handlers/compat/resize.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,20 +46,13 @@ func ResizeTTY(w http.ResponseWriter, r *http.Request) {
utils.ContainerNotFound(w, name, err)
return
}
if state, err := ctnr.State(); err != nil {
utils.InternalServerError(w, errors.Wrapf(err, "cannot obtain container state"))
return
} else if state != define.ContainerStateRunning && !query.IgnoreNotRunning {
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 {
if errors.Cause(err) != define.ErrCtrStateInvalid {
utils.InternalServerError(w, errors.Wrapf(err, "cannot resize container"))
return
} else {
utils.Error(w, "Container not running", http.StatusConflict, err)
}
return
}
// This is not a 204, even though we write nothing, for compatibility
// reasons.
Expand Down
2 changes: 2 additions & 0 deletions pkg/api/server/register_containers.go
Original file line number Diff line number Diff line change
Expand Up @@ -1364,6 +1364,8 @@ func (s *APIServer) registerContainersHandlers(r *mux.Router) error {
// $ref: "#/responses/ok"
// 404:
// $ref: "#/responses/NoSuchContainer"
// 409:
// $ref: "#/responses/ConflictError"
// 500:
// $ref: "#/responses/InternalError"
r.HandleFunc(VersionedPath("/libpod/containers/{name}/resize"), s.APIHandler(compat.ResizeTTY)).Methods(http.MethodPost)
Expand Down
54 changes: 30 additions & 24 deletions pkg/bindings/containers/attach.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ func Attach(ctx context.Context, nameOrID string, stdin io.Reader, stdout io.Wri
winCtx, winCancel := context.WithCancel(ctx)
defer winCancel()

go attachHandleResize(ctx, winCtx, winChange, false, nameOrID, file)
attachHandleResize(ctx, winCtx, winChange, false, nameOrID, file)
}

// If we are attaching around a start, we need to "signal"
Expand Down Expand Up @@ -327,32 +327,38 @@ func (f *rawFormatter) Format(entry *logrus.Entry) ([]byte, error) {
return append(buffer, '\r'), nil
}

// This is intended to be run as a goroutine, handling resizing for a container
// or exec session.
// This is intended to not be run as a goroutine, handling resizing for a container
// or exec session. It will call resize once and then starts a goroutine which calls resize on winChange
func attachHandleResize(ctx, winCtx context.Context, winChange chan os.Signal, isExec bool, id string, file *os.File) {
// Prime the pump, we need one reset to ensure everything is ready
winChange <- sig.SIGWINCH
for {
select {
case <-winCtx.Done():
return
case <-winChange:
w, h, err := terminal.GetSize(int(file.Fd()))
if err != nil {
logrus.Warnf("failed to obtain TTY size: %v", err)
}
resize := func() {
w, h, err := terminal.GetSize(int(file.Fd()))
if err != nil {
logrus.Warnf("failed to obtain TTY size: %v", err)
}

var resizeErr error
if isExec {
resizeErr = ResizeExecTTY(ctx, id, new(ResizeExecTTYOptions).WithHeight(h).WithWidth(w))
} else {
resizeErr = ResizeContainerTTY(ctx, id, new(ResizeTTYOptions).WithHeight(h).WithWidth(w))
}
if resizeErr != nil {
logrus.Warnf("failed to resize TTY: %v", resizeErr)
}
var resizeErr error
if isExec {
resizeErr = ResizeExecTTY(ctx, id, new(ResizeExecTTYOptions).WithHeight(h).WithWidth(w))
} else {
resizeErr = ResizeContainerTTY(ctx, id, new(ResizeTTYOptions).WithHeight(h).WithWidth(w))
}
if resizeErr != nil {
logrus.Warnf("failed to resize TTY: %v", resizeErr)
}
}

resize()

go func() {
for {
select {
case <-winCtx.Done():
return
case <-winChange:
resize()
}
}
}()
}

// Configure the given terminal for raw mode
Expand Down Expand Up @@ -457,7 +463,7 @@ func ExecStartAndAttach(ctx context.Context, sessionID string, options *ExecStar
winCtx, winCancel := context.WithCancel(ctx)
defer winCancel()

go attachHandleResize(ctx, winCtx, winChange, true, sessionID, terminalFile)
attachHandleResize(ctx, winCtx, winChange, true, sessionID, terminalFile)
}

if options.GetAttachInput() {
Expand Down
2 changes: 1 addition & 1 deletion test/apiv2/python/rest_api/fixtures/api_testcase.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ def tearDownClass(cls):

def setUp(self):
super().setUp()
APITestCase.podman.run("run", "alpine", "/bin/ls", check=True)
APITestCase.podman.run("run", "-d", "alpine", "top", check=True)

def tearDown(self) -> None:
APITestCase.podman.run("pod", "rm", "--all", "--force", check=True)
Expand Down
4 changes: 2 additions & 2 deletions test/apiv2/python/rest_api/test_v2_0_0_container.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ def test_list(self):
r = requests.get(self.uri("/containers/json"), timeout=5)
self.assertEqual(r.status_code, 200, r.text)
obj = r.json()
self.assertEqual(len(obj), 0)
self.assertEqual(len(obj), 1)

def test_list_all(self):
r = requests.get(self.uri("/containers/json?all=true"))
Expand All @@ -36,7 +36,7 @@ def test_stats(self):
self.assertId(r.content)

def test_delete(self):
r = requests.delete(self.uri(self.resolve_container("/containers/{}")))
r = requests.delete(self.uri(self.resolve_container("/containers/{}?force=true")))
self.assertEqual(r.status_code, 204, r.text)

def test_stop(self):
Expand Down
3 changes: 1 addition & 2 deletions test/system/450-interactive.bats
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,7 @@ function teardown() {
stty rows $rows cols $cols <$PODMAN_TEST_PTY

# ...and make sure stty under podman reads that.
# FIXME: 'sleep 1' is needed for podman-remote; without it, there's
run_podman run -it --name mystty $IMAGE sh -c 'sleep 1;stty size' <$PODMAN_TEST_PTY
run_podman run -it --name mystty $IMAGE stty size <$PODMAN_TEST_PTY
is "$output" "$rows $cols" "stty under podman reads the correct dimensions"
}

Expand Down