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

bugfix: free resource after exec exit #1240

Merged
merged 1 commit into from
May 8, 2018
Merged

bugfix: free resource after exec exit #1240

merged 1 commit into from
May 8, 2018

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

@@ -89,6 +89,11 @@ func (c *Client) ExecContainer(ctx context.Context, process *Process) error {
break
}
}

// 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