Skip to content

Conversation

@Ace-Tang
Copy link
Contributor

when we start a exec process in containerd, we should do
delete operate after exec process exit, to delete exec pid file.

Signed-off-by: Ace-Tang [email protected]

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

fix exec process pid leak, like:

root@www:/var/lib/pouch/containerd# ls state/io.containerd.runtime.v1.linux/default/46b849a76f28b195a086c35ac1fd4006f9010135e38607e6882e813c7222f2d9
0cdf0d112068e221068d2fcaa578f2cc7dc5803616538be048415c5c48ce55c5.pid  93c116b3f7200d025d251be92597f50fa4da8f4b9bc8837d92f8a65ffa799dae.pid  init.pid
5247e2472a6bce3eb669c083cf241b2dc27203348a96f446c7426b273637e911.pid  bc31ff95cc4f2cdb98b55ac565fe5698fc2adaa48ac2280ac128cb19287d7331.pid  log.json
8553a230f58b376c52c67bf6bfb5b4cb6991434fed77eac7f294492dbeba1fcf.pid  config.json                                                           rootfs
87f86d9267cd8bacacfbbc0f9d114392eaf526c36672b4174729b79c768b562e.pid  f64765a4899e0ac94a026b0c34c6dbcdd86bbb3936470525c99314a5cd367edf.pid

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@pouchrobot pouchrobot added kind/bug This is bug report for project size/XS labels Apr 27, 2018
@codecov-io
Copy link

codecov-io commented Apr 27, 2018

Codecov Report

Merging #1240 into master will increase coverage by 0.14%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1240      +/-   ##
==========================================
+ Coverage   15.21%   15.35%   +0.14%     
==========================================
  Files         172      172              
  Lines       10735    10632     -103     
==========================================
  Hits         1633     1633              
+ Misses       8981     8879     -102     
+ Partials      121      120       -1
Impacted Files Coverage Δ
ctrd/container.go 0% <0%> (ø) ⬆️
daemon/mgr/network.go 3.47% <0%> (-0.01%) ⬇️
cli/rm.go 0% <0%> (ø) ⬆️
cli/cli.go 0% <0%> (ø) ⬆️
apis/server/volume_bridge.go 0% <0%> (ø) ⬆️
cli/inspect.go 0% <0%> (ø) ⬆️
daemon/mgr/volume.go 0% <0%> (ø) ⬆️
daemon/mgr/cri.go 0% <0%> (ø) ⬆️
daemon/mgr/container_types.go 25.31% <0%> (ø) ⬆️
cli/update.go 0% <0%> (ø) ⬆️
... and 10 more

}
}

// delete the finish exec process in containerd
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/finish/finished ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have updated.

@allencloud
Copy link
Collaborator

I am wondering if we could add a integration to cover this case? @Ace-Tang

@Ace-Tang
Copy link
Contributor Author

Ace-Tang commented May 6, 2018

I donnot think so, just like not close file or some other resource, WDYT, @allencloud

when we start a exec process in containerd, we should do
delete operate after exec process exit, to delete exec pid file.

Signed-off-by: Ace-Tang <[email protected]>
@allencloud
Copy link
Collaborator

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label May 8, 2018
@allencloud allencloud merged commit 7d54999 into AliyunContainerService:master May 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug This is bug report for project LGTM one maintainer or community participant agrees to merge the pull reuqest. size/XS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants