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

make sure console master tty is closed on task exit #11161

Merged
merged 2 commits into from
Jan 10, 2025
Merged

make sure console master tty is closed on task exit #11161

merged 2 commits into from
Jan 10, 2025

Conversation

henry118
Copy link
Member

@henry118 henry118 commented Dec 14, 2024 •
edited
Loading

Fixes: #11160

Tldr; this issue turned out to be a regression in v1.7.22 (#10651) where init container object is never deleted in a running shim.

It seems like that the init/exec process's console object are closed by golang's gc. In #10651, init container is added to containerInitExit to track its exit status, but never got removed. This prevented gc from deleting the object, and closing the console FD.

This is okay in regular case, there the shim will exit with the init container. But in CRI, the shim has the lifetime of a POD, therefore container objects will accumulate within the shim.

I think we should delete the init objects from the map on task deletion.

cc @laurazard

@henry118 henry118 force-pushed the ttyleak branch 2 times, most recently from c448293 to 4dd66e1 Compare December 14, 2024 05:47
@akhilerm akhilerm mentioned this pull request Dec 16, 2024
Open
@henry118 henry118 force-pushed the ttyleak branch 3 times, most recently from d2728aa to dfc8bd6 Compare December 17, 2024 02:32
@henry118 henry118 mentioned this pull request Dec 17, 2024
Closed
@henry118 henry118 changed the title make sure console master tty is closed [WIP] make sure console master tty is closed Dec 17, 2024
@henry118 henry118 force-pushed the ttyleak branch 2 times, most recently from dfc8bd6 to 8a238f6 Compare December 17, 2024 17:50
@henry118 henry118 changed the title [WIP] make sure console master tty is closed make sure console master tty is closed Dec 17, 2024
@henry118 henry118 force-pushed the ttyleak branch 8 times, most recently from ad49a39 to e593ccd Compare December 18, 2024 20:50
@henry118 henry118 changed the title make sure console master tty is closed make sure console master tty is closed on task exit Dec 18, 2024
@ningmingxiao
Copy link
Contributor

ningmingxiao commented Dec 25, 2024 •
edited
Loading

the pr will delete epollConsole, but before delete I think it should be closed when container exited. @henry118

@henry118
Copy link
Member Author

the pr will delete epollConsole, but before delete I think it should be closed when container exited. @henry118

The console FD will be closed by go runtime's finalizer during GC. This logic is setup in os/file_unix.go. Relevant stack trace for in our case is below.

> goroutine(17): os.(*file).close((*os.file)(0xc000104fc0))

	Stack:
		0  0x00000000004e79ee in os.(*file).close
		     at /usr/local/go/src/os/file_unix.go:325
		1  0x0000000000472b43 in runtime.call32
		     at /usr/local/go/src/runtime/asm_amd64.s:776
		2  0x0000000000417091 in runtime.runfinq
		     at /usr/local/go/src/runtime/mfinal.go:255
		3  0x00000000004743e1 in runtime.goexit
		     at /usr/local/go/src/runtime/asm_amd64.s:1700

I think it should be good enough for this fix. However I do agree that explicitly closing the FD in the shim could be a good enhancement. But I think it can be a follow-up work.

@ningmingxiao
Copy link
Contributor

can this pr be merged ? @fuweid @laurazard @dmcgowan we meet same issue.

@fuweid fuweid added this pull request to the merge queue Jan 9, 2025
@fuweid fuweid added the cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch label Jan 9, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 10, 2025
@kzys kzys added this pull request to the merge queue Jan 10, 2025
Merged via the queue into containerd:main with commit feeef24 Jan 10, 2025
58 checks passed
@austinvazquez austinvazquez added cherry-pick/1.6.x Change to be cherry picked to release/1.6 branch cherry-pick/2.0.x Change to be cherry picked to release/2.0 branch labels Jan 10, 2025
@austinvazquez
Copy link
Member

/cherry-pick release/2.0

@k8s-infra-cherrypick-robot
Copy link

@austinvazquez: new pull request created: #11246

In response to this:

/cherry-pick release/2.0

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.

@k8s-infra-cherrypick-robot k8s-infra-cherrypick-robot mentioned this pull request Jan 10, 2025
Merged
@austinvazquez austinvazquez added cherry-picked/2.0.x PR commits are cherry picked into the release/2.0 branch and removed cherry-pick/2.0.x Change to be cherry picked to release/2.0 branch labels Jan 10, 2025
@austinvazquez
Copy link
Member

/cherry-pick release/1.7

@k8s-infra-cherrypick-robot
Copy link

@austinvazquez: new pull request created: #11247

In response to this:

/cherry-pick release/1.7

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.

@k8s-infra-cherrypick-robot k8s-infra-cherrypick-robot mentioned this pull request Jan 10, 2025
Closed
@henry118 henry118 deleted the ttyleak branch January 10, 2025 22:47
@austinvazquez austinvazquez mentioned this pull request Jan 11, 2025
Merged
@austinvazquez austinvazquez added cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch and removed cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch labels Jan 11, 2025
@fuweid
Copy link
Member

fuweid commented Jan 29, 2025

/cherry-pick release/1.6

@k8s-infra-cherrypick-robot
Copy link

@fuweid: new pull request created: #11322

In response to this:

/cherry-pick release/1.6

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.

@k8s-infra-cherrypick-robot k8s-infra-cherrypick-robot mentioned this pull request Jan 29, 2025
Closed
@laurazard
Copy link
Member

Great catch, thank you @henry118!

@henry118 henry118 mentioned this pull request Feb 8, 2025
Merged
@siolta siolta mentioned this pull request Feb 19, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fuweid fuweid fuweid approved these changes

@dcantah dcantah dcantah approved these changes

Assignees
No one assigned
Labels
area/runtime Runtime cherry-pick/1.6.x Change to be cherry picked to release/1.6 branch cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch cherry-picked/2.0.x PR commits are cherry picked into the release/2.0 branch kind/bug size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Console TTY leak in runc shim
9 participants
@henry118 @ningmingxiao @austinvazquez @k8s-infra-cherrypick-robot @fuweid @laurazard @dcantah @kzys @k8s-ci-robot