investigate terminal resize issues, and (if possible) set size on creation

[thaJeztah]

relates to:

• There was a problem entering the container #3231
• Flaky test: TestInitTtySizeErrors() (on macOS "latest") #3441
• There was a problem entering the container moby/moby#42707

The TestInitTtySizeErrors test is failing frequently (see #3441), and it looks like it's not just a flaky test, but a bug / regression in the CLI:

--- FAIL: TestInitTtySizeErrors (0.16s)
    tty_test.go:29: assertion failed: 
        --- expectedError
        +++ →
        @@ -1,2 +1 @@
        -failed to resize tty, using default size

We had issues in this area before;

• Docker Exec does not resize terminal moby/moby#35407
• initial windows size of docker exec -it is too small #1492
• fixes 1492: tty initial size error #1529
• Strange terminal behaviour inside docker moby/moby#33794
• various other issues

Previous "fixes" implemented a retry loop to try resizing the terminal if it didn't work the first time (there's some race between the container being created and it being receptacle for resizing), so the existing code continues to be a "best effort";

cli/cli/command/container/run.go

Lines 189 to 193 in b68db38

	if (config.AttachStdin || config.AttachStdout || config.AttachStderr) && config.Tty && dockerCli.Out().IsTerminal() {
	if err := MonitorTtySize(ctx, dockerCli, createResponse.ID, false); err != nil {
	fmt.Fprintln(stderr, "Error monitoring TTY size:", err)
	}
	}

Reproduction steps

The issue can be reproduced with this script (which just starts a lot of containers in a loop); I haven't counted the success/failure ratio, but it's failing very frequently:

#!/bin/bash

A=0
while true; do
    for a in $(seq 1 15); do
        com.docker.cli run --rm -t alpine echo $A/$a &
    done

    wait
    date
    A=$((A+1))
    echo $A
done

Coincidentally, when using a Windows machine, the CLI sets the initial size when creating the container.

cli/cli/command/container/run.go

Lines 122 to 127 in b68db38

	// Telling the Windows daemon the initial size of the tty during start makes
	// a far better user experience rather than relying on subsequent resizes
	// to cause things to catch up.
	if runtime.GOOS == "windows" {
	hostConfig.ConsoleSize[0], hostConfig.ConsoleSize[1] = dockerCli.Out().GetTtySize()
	}

The code above looks wrong though;

• it checks for the operating system on the CLI side, but does not take into account that the daemon could be either a Linux or a Windows daemon
• which could be either an "omission" (initial size supported on both Linux and Windows daemons)
• or (if Linux daemons do not support this) result in Linux container shells never getting the right size when started from a Windows CLI.
• or "redundant" (initial size already connect, but after this, doing a resize to the same size again)

What's needed?

• Investigate what's missing (if anything) to set the correct terminal size when running (docker run), attaching (docker attach) and exec-ing (docker exec) a container
• If setting the initial size is not supported on a Linux daemon, why not?
• ISTR containerd did not support setting the initial size in all of these cases (is this just something that's not "punched through" in the containerd API, or is more needed?)

If it's possible to set the initial size, we should prefer that over "resizing", although resizing would still be needed if the terminal from which the container was started is resized. Of course, we also need to take into account whether the daemon (and containerd) supports setting the initial size (which may depend on the API version).

[rumpl]

Just a quick comment, if I change the amount of sleep between two resizes to 200ms then the error goes away completely for me, this is obviously not ideal and makes me think that this is more an error on the daemon side, I'll continue looking.

[thaJeztah]

200ms seems quite long to wait between tries; so seems to indicate something has become "slower" 🤔 was the 200ms just a "let's try something bigger", or did you try incrementing in smaller steps? (perhaps we can do a quick try to see where the "it mostly works" threshold is).

For a short term "quick fix", we could increase the timeout (although I hope if doesn't have to be 200ms!!!); of course the ideal solution is to (as described above) look if we can set the correct size when creating (so that we don't have to do the resize loop); of course if we do, that likely would have to be done only on "newer" APIs only (would have to check); and of course if dockerd and containerd actually support it (and if not "why not?")

[rumpl]

We could either do 200ms, or 100ms but retry 10 times, it looks like 1 second is the lowest we should go, see this comment by @cpuguy83

I did a quick check and it seems that you can set the TTY when you create a container, here is the test:

containerd code to set TTY size on creation

package main

import (
	"context"
	"errors"
	"fmt"
	"log"
	"syscall"
	"time"

	"github.com/containerd/console"
	"github.com/containerd/containerd"
	"github.com/containerd/containerd/cio"
	"github.com/containerd/containerd/namespaces"
	"github.com/containerd/containerd/oci"
)

func main() {
	if err := redisExample(); err != nil {
		log.Fatal(err)
	}
}

func redisExample() error {
	// create a new client connected to the default socket path for containerd
	client, err := containerd.New("/run/containerd/containerd.sock")
	if err != nil {
		return err
	}
	defer client.Close()

	// create a new context with an "example" namespace
	ctx := namespaces.WithNamespace(context.Background(), "example")

	// pull the redis image from DockerHub
	image, err := client.Pull(ctx, "docker.io/library/busybox:latest", containerd.WithPullUnpack)
	if err != nil {
		return err
	}

	// create a container
	container, err := client.NewContainer(
		ctx,
		"redis-server",
		containerd.WithImage(image),
		containerd.WithNewSnapshot("redis-server-snapshot", image),
		// Setting a small size terminal to make sure this is taken into account, to test run "ls", it should wrap
		// its output to fit into a small window
		containerd.WithNewSpec(oci.Compose(oci.WithImageConfig(image), oci.WithTTY, oci.WithTTYSize(30, 30))),
	)

	if err != nil {
		return err
	}
	defer container.Delete(ctx, containerd.WithSnapshotCleanup)

	var con console.Console
	con = console.Current()
	defer con.Reset()
	if err := con.SetRaw(); err != nil {
		return fmt.Errorf("failed to SetRaw,err is: %w", err)
	}
	if con == nil {
		return errors.New("got nil con with flagT=true")
	}
	// create a task from the container
	task, err := container.NewTask(ctx, cio.NewCreator(cio.WithStreams(con, con, nil), cio.WithTerminal))
	if err != nil {
		return err
	}
	defer task.Delete(ctx)

	// make sure we wait before calling start
	exitStatusC, err := task.Wait(ctx)
	if err != nil {
		fmt.Println(err)
	}

	// call start on the task to execute the redis server
	if err := task.Start(ctx); err != nil {
		return err
	}

	// sleep for a lil bit to see the logs
	time.Sleep(30 * time.Second)

	// kill the process and get the exit status
	if err := task.Kill(ctx, syscall.SIGTERM); err != nil {
		return err
	}

	// wait for the process to fully exit and print out the exit status

	status := <-exitStatusC
	code, _, err := status.Result()
	if err != nil {
		return err
	}
	fmt.Printf("redis-server exited with status: %d\n", code)

	return nil
}

Running this code doesn't fail and the terminal size is right:

➜  containerdtest sudo ./containerdtest
/ # ls
bin   home  run   usr
dev   proc  sys   var
etc   root  tmp

[thaJeztah]

hm.... rendering bug on GitHub?? The > triangle widget no longer renders on collapsed things (😱); see

[rumpl]

That's odd, works on my machine :D

[thaJeztah]

you on Safari, or on Chrome? (looks indeed to still work on Chrome 🤔)

[rumpl]

I'm on Firefox and yeah, it works well

[thaJeztah]

Arf; didn't post 😂

Back to "on topic"; with your test, could you try if it picks up the size immediately? (I recall the issue when not resizing is that everything "seems" ok, only to discover the size is limited to the default 80-chars, and output getting truncated.

For example;

On my host;

tput cols && tput lines
204
37

And in a container:

docker run -it --rm ubuntu sh -c 'tput cols; tput lines'
80
24

(this is with the current release, so the initial size is still 80x24, but then gets resized if you would run interactively and continue working in that shell