[NO TESTS NEEDED] Fix swapped dimensions from terminal.GetSize

[afbjorklund]

Closes #9756

[afbjorklund]

Seems to fix it:

$ resize
COLUMNS=120;
LINES=40;
export COLUMNS LINES;
$ stty size
40 120
$ ./bin/podman run -it fedora stty size
40 120
$ ./bin/podman system service --timeout=0 &
$ ./bin/podman-remote run -it fedora stty size
40 120

[edsantiago]

I'm guessing that @rhatdan's IRC ping was for this PR. In case you need a test:

@test "foo" {
    pty=$PODMAN_TMPDIR/mypty
    dummy=$PODMAN_TMPDIR/dummy

    # Create a pty. Run under 'timeout' because BATS reaps child processes
    # and if we exit before killing socat, bats will hang forever.
    timeout 10 socat PTY,link=$pty,raw,echo=0,waitslave $dummy &
    socat_pid=$!

    # Wait for pty
    retries=5
    while [[ ! -e $pty ]]; do
        retries=$(( retries - 1 ))
        if [[ $retries -eq 0 ]]; then
            die "Timed out waiting for $pty"
        fi
        sleep 0.5
    done

    # Set the pty to a random size ...
    rows=$(( RANDOM % 60 ))
    cols=$(( RANDOM % 60 ))
    stty rows $rows columns $cols <$pty

    # ...and make sure stty under podman reads that.
    run_podman run --rm -it $IMAGE stty size <$pty
    size=$output

    kill $socat_pid
    is "$output" "$rows $cols" "stty under podman reads the correct dimensions"
}

[mheon]

/approve
I like simple fixes.
LGTM, though it would be appreciated if you add the test from @edsantiago to the system tests.

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afbjorklund, mheon

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:

• OWNERS [mheon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[afbjorklund]

I like simple fixes.
LGTM, though it would be appreciated if you add the test from @ edsantiago to the system tests.

That's why it was not simple ;-)
I suppose it should not be named "foo", and that it goes in some system test directory somewhere ?

[edsantiago]

I suppose it should not be named "foo", and that it goes in some system test directory somewhere ?

Somewhere in test/system; either added to an existing fie (030-run.bats seems the natural choice, but that file is already way too big) or creating a new one (no obvious ideas come to mind; but please see 000-TEMPLATE). If you create a new file, I'd encourage you to kill socat within teardown() for added safety. And yes, "foo" was sloppiness because this was a proof of concept only. There may be other bugs or problems with my suggested code.

Thank you!

[rhatdan]

I experimented with a couple of tests

+       It("podman run stty", func() {
+               cmd := exec.Command("stty", "size")
+               cmd.Stdin = os.Stdin
+               ttysize, err := cmd.Output()
+               fmt.Println(ttysize)
+               if err != nil {
+                       Fail(err.Error())
+               }
+
+               session := podmanTest.Podman([]string{"run", ALPINE, "stty", "size"})
+               session.WaitWithDefaultTimeout()
+               Expect(session.ExitCode()).To(Equal(0))
+               Expect(session.OutputToString()).To(Equal(string(ttysize)))
+       })

Or in podman system tests.

@test "podman run stty" {
    size=$(stty size)
    run_podman run --rm $IMAGE stty size
    is "$output" "size"
}

This issue here is that there is no tty when running these tests, so the tests fails.

[edsantiago]

I'm not entirely sure my test is useful: first, it passes on podman even without your PR (a good test should never pass prior to the fix in question). Second, IIUC your code handles SIGWINCH? If so, it'll be necessary to create the pty, start podman (presumably with -d), wait_for_ready, then run the (host) stty, then signal the container to run its stty. That gets a little trickier.

[afbjorklund]

I think I will await your suggestions on testing, and settle for the manual verification for now.

[TomSweeneyRedHat]

Other tests aren't looking hip.
But the changes LGTM

[rhatdan]

I would prefer to get this fix in ASAP, do not want to miss the podman 3.1 window, and work on the tests separately if necessary.

[afbjorklund]

The code change worked OK in manual testing, flipped the rows and columns back to normal again.

#9782 (comment)

BEFORE (v3.1)

$ stty size
40 140
$ ./bin/podman run -it fedora stty size
40 140
$ ./bin/podman system service --timeout=0 &
$ ./bin/podman-remote run -it fedora stty size
WARN[0000] failed to resize TTY: container "58d0334f58966afe00cc7f82dc03d3154ac76d0401b72d5201718afd2c9dcd6a" in wrong state "created" 
0 0
$ ./bin/podman-remote run -it fedora stty size
140 40

AFTER

./bin/podman-remote run -it fedora stty size
40 140
./bin/podman-remote run -it fedora stty size
40 140
./bin/podman-remote run -it fedora stty size
WARN[0000] failed to resize TTY: container "1c51c525bd0f6d7d35a6d82c814c8d627c74cce55c57b6171eff8fe25d5591f0" in wrong state "created" 
0 0

[rhatdan]

@afbjorklund Rebase and repush.

[edsantiago]

For posterity, I was never able to get this to work. This is my final attempt:

@test "podman handles SIGWINCH" {
    pty=$PODMAN_TMPDIR/mypty
    dummy=$PODMAN_TMPDIR/dummy

    # Create a pty. Run under 'timeout' because BATS reaps child processes
    # and if we exit before killing socat, bats will hang forever.
    timeout 10 socat PTY,link=$pty,raw,echo=0 PTY,link=$dummy,raw,echo=0 &
    socat_pid=$!

    # Wait for pty
    retries=5
    while [[ ! -e $pty ]]; do
        retries=$(( retries - 1 ))
        if [[ $retries -eq 0 ]]; then
            die "Timed out waiting for $pty"
        fi
        sleep 0.5
    done

    # Set the pty to a fixed size
    stty rows 10 columns 10 <$pty

    # Set the pty to a random size. Make rows/columns odd/even, to guarantee
    # that they can never be the same
    rows=$(( 15 + RANDOM % 60 |   1 ))
    cols=$(( 15 + RANDOM % 60 & 126 ))

    ( sleep 2; wait_for_ready mystty; stty rows $rows columns $cols <$pty; run_podman kill -sWINCH mystty; run_podman exec mystty touch /stop ) &

    # ...and make sure stty under podman reads that.
    run_podman run --rm -it --name mystty $IMAGE sh -c \
               'stty size;echo READY;retries=100;while [ ! -e /stop -a $retries -gt 0 ]; do sleep 0.1; retries=$(($retries - 1));done;stty size' <$pty
    kill $socat_pid

    is "${lines[0]}" "10 10" "stty before sigwinch"
    is "${lines[2]}" "$rows $cols" "stty under podman reads the correct dimensions"
}

Failure: the final stty size always emits 10 10, it never sees the updated size. Giving up, posting in case someone sees any obvious bugs in my approach.

[edsantiago]

I had not understood that this was podman-remote. @afbjorklund please save this as test/system/520-remote.bats:

# -*- bats -*-
#
# tests that might be especially failure-prone in podman-remote
#

load helpers

@test "podman detects correct tty size" {
    pty=$PODMAN_TMPDIR/mypty
    dummy=$PODMAN_TMPDIR/dummy

    # Create a pty. Run under 'timeout' because BATS reaps child processes
    # and if we exit before killing socat, bats will hang forever.
    timeout 10 socat PTY,link=$pty,raw,echo=0 PTY,link=$dummy,raw,echo=0 &
    socat_pid=$!

    # Wait for pty
    retries=5
    while [[ ! -e $pty ]]; do
        retries=$(( retries - 1 ))
        if [[ $retries -eq 0 ]]; then
            die "Timed out waiting for $pty"
        fi
        sleep 0.5
    done

    # Set the pty to a random size. Make rows/columns odd/even, to guarantee
    # that they can never be the same
    rows=$(( 15 + RANDOM % 60 |   1 ))
    cols=$(( 15 + RANDOM % 60 & 126 ))
    stty rows $rows cols $cols <$pty

    # ...and make sure stty under podman reads that.
    # 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' <$pty
    kill $socat_pid

    is "$output" "$rows $cols" "stty under podman reads the correct dimensions"
}

[edsantiago]

Seeing no activity here, I've submitted #9818 to add a system test. @afbjorklund if you're still working on this, please just rebase and re-push, don't bother adding tests. The re-push will pick up the modified subject, and will then not enforce needing tests. If you read this after #9818 merges, then please rebase, remove the skip in test/system/450-interactive.bats, and re-push. Either way, we're hoping you will proceed with this, but you do need to rebase and re-push. Thank you!

[edsantiago]

/lgtm