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

libcontainerd: fix leaking container/exec state #35484

Merged
merged 1 commit into from Nov 14, 2017

Conversation

Projects
None yet
6 participants
@tonistiigi
Member

tonistiigi commented Nov 13, 2017

  • fixes removing containerd state directory on stop/rm
  • fixes removing exec fifos when exec process exits
  • fixes removing exec fifos on exec error (supersedes #32465)

@andrewhsu @dmcgowan @crosbymichael

Signed-off-by: Tonis Tiigi tonistiigi@gmail.com

@tonistiigi tonistiigi requested review from mlaventure and removed request for mlaventure Nov 13, 2017

@tonistiigi tonistiigi requested review from dnephin and vdemeester as code owners Nov 13, 2017

libcontainerd: fix leaking container/exec state
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@@ -639,6 +643,14 @@ func (c *client) processEvent(ctr *container, et EventType, ei EventInfo) {
c.Lock()
delete(ctr.execs, ei.ProcessID)
c.Unlock()
ctr := c.getContainer(ei.ContainerID)

This comment has been minimized.

@mlaventure

mlaventure Nov 14, 2017

Contributor

On error (e.g. failure to create the task) this would cause a deadlock if I'm not mistaken

@mlaventure

mlaventure Nov 14, 2017

Contributor

On error (e.g. failure to create the task) this would cause a deadlock if I'm not mistaken

This comment has been minimized.

@tonistiigi

tonistiigi Nov 14, 2017

Member

Can you explain more? You mean the RLock inside getContainer() ?

@tonistiigi

tonistiigi Nov 14, 2017

Member

Can you explain more? You mean the RLock inside getContainer() ?

This comment has been minimized.

@mlaventure

mlaventure Nov 14, 2017

Contributor

Yes, I think I had this initially (a call to getContainer) and it caused a deadlock in some error conditions and for execs. Can't remember the exact details

@mlaventure

mlaventure Nov 14, 2017

Contributor

Yes, I think I had this initially (a call to getContainer) and it caused a deadlock in some error conditions and for execs. Can't remember the exact details

This comment has been minimized.

@tonistiigi

tonistiigi Nov 14, 2017

Member

Well, it is taking the same lock as line 643 so I'm not sure how this can cause a deadlock and the previous one doesn't.

@tonistiigi

tonistiigi Nov 14, 2017

Member

Well, it is taking the same lock as line 643 so I'm not sure how this can cause a deadlock and the previous one doesn't.

This comment has been minimized.

@mlaventure

mlaventure Nov 14, 2017

Contributor

I may just be confused, I'll have to have a closer look, I had to change this part quite a few time to get rid of all the deadlocks. I'll try to have a proper look sometime tomorrow.

@mlaventure

mlaventure Nov 14, 2017

Contributor

I may just be confused, I'll have to have a closer look, I had to change this part quite a few time to get rid of all the deadlocks. I'll try to have a proper look sometime tomorrow.

This comment has been minimized.

@mlaventure

mlaventure Nov 14, 2017

Contributor

Just ignore me, this is safe 😇 I got confused with another part of the code.

@mlaventure

mlaventure Nov 14, 2017

Contributor

Just ignore me, this is safe 😇 I got confused with another part of the code.

@mlaventure

LGTM

@dmcgowan

LGTM

@vdemeester

LGTM 🐯

@vdemeester vdemeester merged commit 1c99bc4 into moby:master Nov 14, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 37831 has succeeded
Details
janky Jenkins build Docker-PRs 46542 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 6955 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 18100 has succeeded
Details
z Jenkins build Docker-PRs-s390x 6760 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment