Panic: `not a console`, follow-up to #3297

containerd / nerdctl

contaiNERD CTL - Docker-compatible CLI for containerd, with support for Compose, Rootless, eStargz, OCIcrypt, IPFS, ...

Apache License 2.0

8.24k stars 612 forks source link

Panic: `not a console`, follow-up to #3297 #3433

Closed apostasie closed 2 months ago

apostasie commented 2 months ago

Description

As outlined in #3297, there are more places that are likely to panic when we do not have a tty.

While the patch in #3300 fixed it for exec for a specific use case, clearly run can crash in the same circumstances.

Suggestion is to wrap the third party term console library so that it returns an error we can check instead of crashing.

Steps to reproduce the issue

Describe the results you received and expected

What version of nerdctl are you using?

main

Are you using a variant of nerdctl? (e.g., Rancher Desktop)

None

Host information

No response

alitvak69 commented 2 months ago

Do you have any examples of how you wrap?

apostasie commented 2 months ago

Do you have any examples of how you wrap?

Sorry, there is no workaround that I know of. Suggestion to "wrap" refers to how to fix the code: you need to search for all the calls to console.Current and replace them by a call to a new function that will test for said condition.

alitvak69 commented 2 months ago

Can I provide some additional information to help you fix the issue? Even if it is impossible to run top inside of the container, at least it shouldn't crash.

apostasie commented 2 months ago

Can I provide some additional information to help you fix the issue? Even if it is impossible to run top inside of the container, at least it shouldn't crash.

Of course. Thanks!

alitvak69 commented 2 months ago

What do you want me to do. Let me know if you want me to run debugging of some sort or test with some different version of nerdctl

apostasie commented 2 months ago

What do you want me to do. Let me know if you want me to run debugging of some sort or test with some different version of nerdctl

Ideally:

use the latest nerdctl release candidate: https://github.com/containerd/nerdctl/releases/tag/v2.0.0-rc.2
confirm that the problem still exist with your scenario
come-up with a as-simple as-possible reproducer - eg: a short shell script that I can run and that shows the issue

Thanks.

alitvak69 commented 2 months ago

Thank you for your work. I built and tested new code. It allows a container to continue. This said are there any plans to make nerdctl started container from systemd more pseudo-terminal friendly?

apostasie commented 2 months ago

@alitvak69 you are welcome!

Roadmap questions should go to @AkihiroSuda or other maintainers. Then again, there is nothing wrong with just going ahead: open issues / feature requests for what you need and let see what we can do.

Cheers.