Skip to content

cri: close leak container's io when restart containerd#11389

Closed
ningmingxiao wants to merge 1 commit intocontainerd:mainfrom
ningmingxiao:leak_io2
Closed

cri: close leak container's io when restart containerd#11389
ningmingxiao wants to merge 1 commit intocontainerd:mainfrom
ningmingxiao:leak_io2

Conversation

@ningmingxiao
Copy link
Copy Markdown
Contributor

@ningmingxiao ningmingxiao commented Feb 14, 2025 •
edited
Loading

If system is busy, shim is busy container status is unknown k8s will create many containers in to one pod,which cause containerd panic(we set default 10000 to debug.SetMaxThreads(20000) still panic )
when restart containerd

Feb 10 20:34:09 paas-controller-0-0 containerd[22730]: runtime: program exceeds 20000-thread limit
Feb 10 20:34:09 paas-controller-0-0 containerd[22730]: fatal error: thread exhaustion
Feb 10 20:34:09 paas-controller-0-0 containerd[22730]: runtime stack:
Feb 10 20:34:09 paas-controller-0-0 containerd[22730]: runtime.throw({0x1a432b2?, 0x7fb3545f2f78?})
Feb 10 20:34:09 paas-controller-0-0 containerd[22730]: #011/usr/local/go/src/runtime/panic.go:1047 +0x5d fp=0x7fb3545f2f38 sp=0x7fb3545f2f08 pc=0x439e5d
Feb 10 20:34:09 paas-controller-0-0 containerd[22730]: runtime.checkmcount()
Feb 10 20:34:09 paas-controller-0-0 containerd[22730]: #011/usr/local/go/src/runtime/proc.go:790 +0x8c fp=0x7fb3545f2f60 sp=0x7fb3545f2f38 pc=0x43dd8c
Feb 10 20:34:09 paas-controller-0-0 containerd[22730]: runtime.mReserveID()
Feb 10 20:34:09 paas-controller-0-0 containerd[22730]: #011/usr/local/go/src/runtime/proc.go:806 +0x36 fp=0x7fb3545f2f88 sp=0x7fb3545f2f60 pc=0x43ddd6

[[email protected]@LIN-FB738BFD367 op-containers-containerd]$ cat 1.log |grep openFifo|wc -l
819
many openfifo panic.

Feb 10 20:34:09 paas-controller-0-0 containerd[22730]: syscall.openat(0x17436c0?, {0xc00c4313c8?, 0xc0010e8010?}, 0x9?, 0x0)
Feb 10 20:34:09 paas-controller-0-0 containerd[22730]: #011/usr/local/go/src/syscall/zsyscall_linux_amd64.go:83 +0x94 fp=0xc010771e80 sp=0xc010771e08 pc=0x4c7914
Feb 10 20:34:09 paas-controller-0-0 containerd[22730]: syscall.Open(...)
Feb 10 20:34:09 paas-controller-0-0 containerd[22730]: #011/usr/local/go/src/syscall/syscall_linux.go:272
Feb 10 20:34:09 paas-controller-0-0 containerd[22730]: os.openFileNolog({0xc00c4313c8, 0x12}, 0x0, 0x0)
Feb 10 20:34:09 paas-controller-0-0 containerd[22730]: #011/usr/local/go/src/os/file_unix.go:245 +0x9b fp=0xc010771ec8 sp=0xc010771e80 pc=0x4f629b
Feb 10 20:34:09 paas-controller-0-0 containerd[22730]: os.OpenFile({0xc00c4313c8, 0x12}, 0x0, 0x10ee1b0?)
Feb 10 20:34:09 paas-controller-0-0 containerd[22730]: #011/usr/local/go/src/os/file.go:326 +0x45 fp=0xc010771f00 sp=0xc010771ec8 pc=0x4f3f45
Feb 10 20:34:09 paas-controller-0-0 containerd[22730]: github.com/containerd/fifo.openFifo.func2()
Feb 10 20:34:09 paas-controller-0-0 containerd[22730]: #011/root/rpmbuild/BUILD/containerd.io-1.7.6/_build/src/github.com/containerd/containerd/vendor/github.com/containerd/fifo/fifo.go:138 +0xc5 fp=0xc010771fe0 sp=0xc010771f00 pc=0xd50a85
Feb 10 20:34:09 paas-controller-0-0 containerd[22730]: runtime.goexit()
Feb 10 20:34:09 paas-controller-0-0 containerd[22730]: #011/usr/local/go/src/runtime/asm_amd64.s:1598 +0x1 fp=0xc010771fe8 sp=0xc010771fe0 pc=0x471721
Feb 10 20:34:09 paas-controller-0-0 containerd[22730]: created by github.com/containerd/fifo.openFifo
Feb 10 20:34:09 paas-controller-0-0 containerd[22730]: #011/root/rpmbuild/BUILD/containerd.io-1.7.6/_build/src/github.com/containerd/containerd/vendor/github.com/containerd/fifo/fifo.go:131 +0x3be

may be fix #9113 and
#10515

but I can't find a good way to understand how " ... is reserved for ..." happend.

panic happened(20:34:09) after containerd is restart(20:03:44).

Feb 10 20:03:44 paas-controller-0-0 containerd[22730]: time="2025-02-10T20:03:44.251063233+08:00" level=info msg="containerd successfully booted in 3.622669s"

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Feb 14, 2025

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.

Details

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 ningmingxiao mentioned this pull request Feb 14, 2025
Closed
@ningmingxiao ningmingxiao changed the title close leak container's io cri:close leak container's io when restart containerd Feb 14, 2025
@ningmingxiao ningmingxiao force-pushed the leak_io2 branch 2 times, most recently from 76b89f0 to 31244a1 Compare February 14, 2025 14:25
@ningmingxiao ningmingxiao mentioned this pull request Feb 18, 2025
Closed
fuweid
fuweid requested changes Feb 18, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@fuweid fuweid left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please use defer to close IO

Comment thread internal/cri/server/restart.go Outdated
fuweid
fuweid reviewed Feb 18, 2025
View reviewed changes
Comment thread internal/cri/server/restart.go Outdated
fuweid
fuweid previously approved these changes Feb 18, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM on green

@ningmingxiao
Copy link
Copy Markdown
Contributor Author

ningmingxiao commented Feb 18, 2025

can this pr be merged? @fuweid

@fuweid fuweid changed the title cri:close leak container's io when restart containerd cri: close leak container's io when restart containerd Feb 18, 2025
@fuweid
Copy link
Copy Markdown
Member

fuweid commented Feb 18, 2025

can this pr be merged? @fuweid

The rule of merge is to have two approvals at least. :) please wait for reviewers. Thanks

@ningmingxiao
Copy link
Copy Markdown
Contributor Author

ningmingxiao commented Feb 18, 2025

cc @mikebrow

mikebrow
mikebrow reviewed Feb 18, 2025
View reviewed changes
Comment thread internal/cri/server/restart.go
@fuweid fuweid dismissed their stale review February 18, 2025 19:17

we need to handle rollback reservation first

@ningmingxiao ningmingxiao force-pushed the leak_io2 branch 6 times, most recently from dbe79a8 to 27a6956 Compare February 20, 2025 02:20
@ningmingxiao
Copy link
Copy Markdown
Contributor Author

ningmingxiao commented Feb 20, 2025 •
edited
Loading

two question:

  1. do we need just reserve biggest attemp container ?
    see
func makeContainerName(c *runtime.ContainerMetadata, s *runtime.PodSandboxMetadata) string {
	return strings.Join([]string{
		c.Name,      // 0: container name
		s.Name,      // 1: pod name
		s.Namespace, // 2: pod namespace
		s.Uid,       // 3: pod uid
		strconv.FormatUint(uint64(c.Attempt), 10), // 4: attempt number of creating the container
	}, nameDelimiter)
}
  1. If let Reserve run before Add old version k8s have chance to see conflict containers, but new version k8s won't see them( since they are not added into store). containerd have to delete them.
    @fuweid @mikebrow

fuweid
fuweid requested changes Feb 20, 2025
View reviewed changes
Comment thread internal/cri/server/restart.go
Comment thread internal/cri/server/restart.go Outdated
Comment thread internal/cri/server/restart.go Outdated
@fuweid
Copy link
Copy Markdown
Member

fuweid commented Feb 20, 2025

  1. do we need just reserve biggest attemp container ?

No, we don't. Old, exited containers may still exist. And they are valid.

  1. If let Reserve run before Add old version k8s have chance to see conflict containers, but new version k8s won't see them( since they are not added into store). containerd have to delete them.

If I understand correctly, you're saying that ListContainers can return all containers, including those with the same name. Yes, that's correct. With my suggestion, ListContainers will not return leaky containers that have duplicate names. And we can't close IO.

I don't know how to reproduce containers with same attempt in k8s case.
I think we should hold this pull request until we have more detail on that.

/hold

@ningmingxiao
Copy link
Copy Markdown
Contributor Author

ningmingxiao commented Feb 20, 2025 •
edited
Loading

I find many log "failed to reserve container name" happened after containerd booted successfully.
so it may not happened when recover.

panic happened(20:34:09) after containerd is restart(20:03:44).

Feb 10 20:03:44 paas-controller-0-0 containerd[22730]: time="2025-02-10T20:03:44.251063233+08:00" level=info msg="containerd successfully booted in 3.622669s"

panic reason is that containerd doesn't receive delete request. many containers wait to be started.

lsof -p pidof containerd

container 628887 root *071u     FIFO               0,25       0t0   6956077 /run/containerd/io.containerd.grpc.v1.cri/containers/2834d95a0a5126a88aad328d03bea35f793c4d474a7f46fe148948f25ec7c1e6/io/704010651/2834d95a0a5126a88aad328d03bea35f793c4d474a7f46fe148948f25ec7c1e6-stdout
container 628887 root *075u     FIFO               0,25       0t0   6956080 /run/containerd/io.containerd.grpc.v1.cri/containers/3469913a25af73297aaa4c0da5dca4ad7cd14d52c63964c093299a321fd703dd/io/2614958862/3469913a25af73297aaa4c0da5dca4ad7cd14d52c63964c093299a321fd703dd-stdout
container 628887 root *078u     FIFO               0,25       0t0   6956083 /run/containerd/io.containerd.grpc.v1.cri/containers/24dec3be7e6cf5e061bef5c00025bb37295e654a8bfca08f3cdda6af0f60e9c3/io/2076536548/24dec3be7e6cf5e061bef5c00025bb37295e654a8bfca08f3cdda6af0f60e9c3-stdout

10124 stdout and stderror is hold by containerd.
[[email protected]@LIN-FB738BFD367 op-containers-containerd]$ cat lsof.log |grep stdout|wc -l
10124

@fuweid
Copy link
Copy Markdown
Member

fuweid commented Feb 21, 2025

panic reason is that containerd doesn't receive delete request. many containers wait to be started.

Is it caused by office kubelet release? I mean that it's not the your internal version.

For failed to reserve container name error during recovery, this issue explained one case #7247 .

@ningmingxiao ningmingxiao mentioned this pull request Feb 21, 2025
Closed
@ningmingxiao
Copy link
Copy Markdown
Contributor Author

ningmingxiao commented Feb 21, 2025

we use internal k8s matained by other team kubernetes/kubernetes#130331

@utam0k utam0k mentioned this pull request Feb 27, 2025
Merged
@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Apr 1, 2025

PR needs rebase.

Details

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.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 30, 2025

This PR is stale because it has been open 90 days with no activity. This PR will be closed in 7 days unless new comments are made or the stale label is removed.

@github-actions github-actions Bot added the Stale label Jun 30, 2025
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jul 7, 2025

This PR was closed because it has been stalled for 7 days with no activity.

@github-actions github-actions Bot closed this Jul 7, 2025
@github-project-automation github-project-automation Bot moved this from Needs Update to Done in Pull Request Review Jul 7, 2025
@thameezb
Copy link
Copy Markdown

thameezb commented Aug 14, 2025

Can this be reopened?
Hitting this issue quite frequently now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mikebrow mikebrow mikebrow left review comments

@fuweid fuweid fuweid requested changes

Assignees

No one assigned

Labels

needs-rebase ok-to-test size/S Stale

Projects

Pull Request Review
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

failed to reserve container name- increase in iops didn't worked

5 participants

@ningmingxiao @k8s-ci-robot @fuweid @thameezb @mikebrow