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

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

Closed
apostasie opened this issue Sep 17, 2024 · 8 comments · Fixed by #3434
Closed

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

apostasie opened this issue Sep 17, 2024 · 8 comments · Fixed by #3434
Labels
kind/unconfirmed-bug-claim Unconfirmed bug claim

Comments

@apostasie
Copy link
Contributor

apostasie commented Sep 17, 2024

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

na

Describe the results you received and expected

na

What version of nerdctl are you using?

main

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

None

Host information

No response

@apostasie apostasie added the kind/unconfirmed-bug-claim Unconfirmed bug claim label Sep 17, 2024
@alitvak69
Copy link

Do you have any examples of how you wrap?

@apostasie
Copy link
Contributor Author

apostasie commented Sep 17, 2024

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
Copy link

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

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:

Thanks.

apostasie added a commit to apostasie/nerdctl that referenced this issue Sep 18, 2024
As outlined in containerd#3433, containerd/console will panic on a call to console.Current().
This patch provides a simple consoleutil wrapper that will return an error instead.

Note that part of containerd#3300 is being reverted, as no longer necessary.

This patch does not try to be "smart" and does not check the status of stdin/out/err otherwise or if it is consistent with user provided flags,
but merely ensures we do not crash.

Signed-off-by: apostasie <[email protected]>
apostasie added a commit to apostasie/nerdctl that referenced this issue Sep 18, 2024
As outlined in containerd#3433, containerd/console will panic on a call to console.Current().
This patch provides a simple consoleutil wrapper that will return an error instead.

Note that part of containerd#3300 is being reverted, as no longer necessary.

This patch does not try to be "smart" and does not check the status of stdin/out/err otherwise or if it is consistent with user provided flags,
but merely ensures we do not crash.

Signed-off-by: apostasie <[email protected]>
apostasie added a commit to apostasie/nerdctl that referenced this issue Sep 18, 2024
As outlined in containerd#3433, containerd/console will panic on a call to console.Current().
This patch provides a simple consoleutil wrapper that will return an error instead.

Note that part of containerd#3300 is being reverted, as no longer necessary.

This patch does not try to be "smart" and does not check the status of stdin/out/err otherwise or if it is consistent with user provided flags,
but merely ensures we do not crash.

Signed-off-by: apostasie <[email protected]>
apostasie added a commit to apostasie/nerdctl that referenced this issue Sep 19, 2024
As outlined in containerd#3433, containerd/console will panic on a call to console.Current().
This patch provides a simple consoleutil wrapper that will return an error instead.

Note that part of containerd#3300 is being reverted, as no longer necessary.

This patch does not try to be "smart" and does not check the status of stdin/out/err otherwise or if it is consistent with user provided flags,
but merely ensures we do not crash.

Signed-off-by: apostasie <[email protected]>
@alitvak69
Copy link

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
Copy link
Contributor Author

@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.

apostasie added a commit to apostasie/nerdctl that referenced this issue Sep 23, 2024
As outlined in containerd#3433, containerd/console will panic on a call to console.Current().
This patch provides a simple consoleutil wrapper that will return an error instead.

Note that part of containerd#3300 is being reverted, as no longer necessary.

This patch does not try to be "smart" and does not check the status of stdin/out/err otherwise or if it is consistent with user provided flags,
but merely ensures we do not crash.

Signed-off-by: apostasie <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/unconfirmed-bug-claim Unconfirmed bug claim
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants