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.

Sign up for GitHub

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

Jump to bottom

fix:tty console leak #11166

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix:tty console leak #11166

wants to merge 1 commit into from

Conversation

ningmingxiao
Copy link
Contributor

No description provided.

@k8s-ci-robot
Copy link

Hi @ningmingxiao. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@ningmingxiao
Copy link
Contributor Author

ningmingxiao commented Dec 16, 2024 •
edited
Loading

fix:#11160

akhilerm
akhilerm reviewed Dec 16, 2024
View reviewed changes
Copy link
Member

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldnt this be a part of #11161 ?

@ningmingxiao
Copy link
Contributor Author

ningmingxiao commented Dec 16, 2024 •
edited
Loading

sorry I paste wrong link. I try to solve this #11160
I think we should maintain consistency with the coding style of stdin
https://github.com/containerd/containerd/blob/v2.0.0/cmd/containerd-shim-runc-v2/runc/platform.go#L86

@ningmingxiao ningmingxiao changed the title fix:tty leak fix:tty console leak Dec 16, 2024
@ningmingxiao ningmingxiao force-pushed the tty_leak branch 2 times, most recently from 764d0af to 189cf7f Compare December 16, 2024 15:53
@ningmingxiao ningmingxiao force-pushed the tty_leak branch 5 times, most recently from 3311247 to f88395f Compare December 17, 2024 10:05
@ningmingxiao
Copy link
Contributor Author

can you review this pr ? thank you @akhilerm

@ningmingxiao ningmingxiao force-pushed the tty_leak branch 2 times, most recently from e4ddca2 to fb948a0 Compare December 17, 2024 11:32
@thaJeztah
Copy link
Member

Is this one still needed now thathttps://github.com//pull/11161 was merged?

@ningmingxiao
Copy link
Contributor Author

ningmingxiao commented Jan 11, 2025 •
edited
Loading

Is this one still needed now thathttps://github.com//pull/11161 was merged?
#11161 fix when container is deleted. but I think it is necessary to close fd when container exited. before delete we should close it.

@ningmingxiao
fix:tty console leak
75e97b5 
Signed-off-by: ningmingxiao <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akhilerm akhilerm akhilerm left review comments

@chrishenzie chrishenzie chrishenzie left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
area/runtime Runtime kind/bug needs-ok-to-test size/XS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ningmingxiao @k8s-ci-robot @thaJeztah @chrishenzie @akhilerm