fix: return tty size in container_inspect.go#28579
Conversation
|
A simple video to see the change quickly podman_tty.mp4 |
danishprakash
left a comment
There was a problem hiding this comment.
Can you please also add a test case for this?
| } else { | ||
| defer f.Close() | ||
|
|
||
| height, width, err := xterm.GetSize(int(f.Fd())) |
There was a problem hiding this comment.
GetSize returns (width, height), so the size is flipped.
| procPath := fmt.Sprintf("/proc/%d/fd/0", c.state.PID) | ||
|
|
||
| f, err := os.Open(procPath) |
There was a problem hiding this comment.
That path doesn't exist on FreeBSD, so this would always fail. We could either guard this for Linux or even split this into *_freebsd.go. Bottom line, the behavior on FreeBSD won't change (podman will still return [0,0]), but there's going to be an additional warning, which is superfluous considering we know this won't work.
There was a problem hiding this comment.
Probably safest to have _linux and _freebsd files for this
There was a problem hiding this comment.
So, for *_freebsd.go the console size should be updated with another method or just simply make it [0,0] ?
There was a problem hiding this comment.
0,0 for now, though add a TODO comment saying it should be fixed later
| f, err := os.Open(procPath) | ||
| if err != nil { | ||
| if c.state.State != define.ContainerStateRunning { | ||
| logrus.Warnf("container %q is not running, unable to get console size", c.ID()) |
There was a problem hiding this comment.
To me, this seems to be noise.
There was a problem hiding this comment.
Maybe debug or exit with an error?
| "go.podman.io/podman/v6/libpod/driver" | ||
| "go.podman.io/podman/v6/pkg/signal" | ||
| "go.podman.io/podman/v6/pkg/util" | ||
| xterm "golang.org/x/term" |
There was a problem hiding this comment.
| xterm "golang.org/x/term" | |
| "golang.org/x/term" |
| if c.state.State != define.ContainerStateRunning { | ||
| logrus.Warnf("container %q is not running, unable to get console size", c.ID()) | ||
| } else { | ||
| logrus.Warnf("unable to open %s", procPath) |
There was a problem hiding this comment.
The lack of context here might confuse more than it would help, consider:
| logrus.Warnf("unable to open %s", procPath) | |
| logrus.Debugf("unable to get console size for container %s: %v", c.ID(), err) |
| if c.Terminal() { | ||
| procPath := fmt.Sprintf("/proc/%d/fd/0", c.state.PID) | ||
|
|
||
| f, err := os.Open(procPath) |
There was a problem hiding this comment.
What if the container doesn't run, and some other process gets that PID?
There was a problem hiding this comment.
According to my knowledege,
PID's are reused only when the prev. process with same PID has already been exited.
Even if another process gets same PID,
the prev. container c.state.pid returns 0 because it is stopped, because without the prev. process stopped same PID cannot be assigned to another.
Also, /proc/0/fd/ access is not allowed, so os.Open("/proc/0/fd/") gives error.
Resulting in debug message and [0,0] consoleSize.
| f, err := os.Open(procPath) | ||
| if err != nil { | ||
| if c.state.State != define.ContainerStateRunning { | ||
| logrus.Warnf("container %q is not running, unable to get console size", c.ID()) |
There was a problem hiding this comment.
Maybe debug or exit with an error?
@danishprakash @mheon, |
|
Probably not necessary. You could add an E2E test instead, maybe in |
I think this test would fail in FreeBSD, because we return zero in both case. What do you think? |
|
You could skip the test on freebsd, or check that it returns [0,0]: |
In the output of podman inspect, there is a field for the container's TTY size (ConsoleSize), which Podman presently only returns [0,0]. This PR updates the console size field for containers with terminal enabled. For cases where there is an issue with getting the size, suitable debug information is given, no update happens and default [0,0] is returned. This implementation is only for Linux systems. For FreeBSD, the console size is not implemented yet so, adds a todo for that. Fixes: containers#28368 Signed-off-by: Priyansh Sao <saopriyansh06@gmail.com>
8f5a4a2 to
4728ce1
Compare
|
Implemented all the changes related to implementation asked by reviewers. Just a small change from my side, I replaced the Warnf with Debugf for this If that's not a good change, I would fix that when I would make commit for tests. |
|
For tests, the problem I'm facing is that, to get the consoleSize field updated container's terminal should be enabled + a real terminal should be attached. But this doesn't work in podman test: So I tried this: But the thing is even with terminal enabled the console size is 0 because no terminal is attached. |
|
Very Sorry for this, my keys got wrongly pressed and this pr got closed. |
|
@priyanshsao I think you forgot to push the new test. Can you check your changes once more? Thanks. |
There are some issues I'm facing related to tests, once those are resolved I will add another commit with changes. |
@mheon @danishprakash, Do I need to use some external lib for creating a real terminal or podman tests has some functions for this, can you please provide your thoughts on this. |
|
Hm. I don't know if we've encountered this one before. It looks like the system tests can create terminals. |
|
Yeah, system tests might be safer - see tests like https://github.com/containers/podman/blob/main/test/system/450-interactive.bats#L51-L81 |
In the output of podman inspect, there is a field for the container's TTY size (ConsoleSize), which Podman presently only returns [0,0].
This PR updates the tty size for containers which have terminal enabled and are in running state.
For cases where there is an issue with getting the size or the container is not running, suitable warning is given, no update happens and default [0,0] is returned.
Fixes: #28368
Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. See CONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?