agent: Make the agent the subreaper of all processes #178

sboeuf · 2017-12-05T23:39:07Z

agent: Make the agent the subreaper of all processes

Our agent had a flaw, it potentially lives with some zombie processes
according to what has been done by the container user. The explanation
is that we only wait for container and exec processes but we don't
handle the children and grandchildren processes coming from those
initial container and exec processes. The example of such a case can
be shown demonstrated in case the container process spawn a child
process, and this one will also spawn another child process. In case
the child terminates before its own children, those ones expect to be
reaped by the init process of the PID namespace, i.e the container
process. But this container process is not written to reap descendant
processes, which will lead to a zombie process left behind.

This commit solves those cases by setting the agent process, who will
be the father of all container processes, as a subreaper. This way, it
will receive all the SIGCHLD signals from any child left behind, and
will reap them. Notice this patch makes sure we don't check the error
from the libcontainer call to process.Wait() since it won't be able to
reap the container or exec processes as expected. This libcontainer
Wait() function is only used here to properly cleanup the pipes and
internal structures created by libcontainer.

Fixes #177

Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com>

sboeuf · 2017-12-05T23:39:19Z

Fixes #177

sboeuf · 2017-12-11T23:00:37Z

Cannot be merged because of libcontainer issue: opencontainers/runc#1677

sboeuf · 2017-12-15T20:44:08Z

opencontainers/runc#1677 has been fixed with opencontainers/runc#1678 which has been merged !
This PR can be unblocked now.

sboeuf · 2017-12-18T20:01:35Z

@sameo @amshinde please take a look.

sboeuf · 2017-12-20T20:37:28Z

@jodh-intel @grahamwhaley @jcvenegas @sameo @amshinde
Please take a look :)

devimc · 2017-12-20T22:14:32Z

I wonder if runC has the same issue

In case the child terminates before its own children, those ones expect to be reaped by the init process of the PID namespace

yee, but

If the "init" process of a PID namespace terminates, the kernel terminates all of the processes in the namespace via a SIGKILL signal (pid_namespaces(7))

basically once the workload ends, all processes are killed, but I guess you want to handle the case where the workload never ends and its children spawn and leave processes running

lgtm

sboeuf · 2017-12-20T22:22:19Z

@devimc, runc does not have the same issue because it actually set itself as a subreaper (there a flag to disable the subreaper if needed).
And yes you're right, I am trying to handle the case where the container process does not end, but the workload has started a bunch of children and grandchildren who might terminate after their parent.

amshinde · 2017-12-20T22:28:15Z

reaper.go

+	sync.RWMutex
+
+	chansLock     sync.RWMutex
+	exitCodeChans map[int]chan int


Can you add a comment explaining this structure?

Sure I can.

amshinde · 2017-12-20T23:50:14Z

reaper.go

+		agentLog.Infof("SIGCHLD pid %d, status %d", pid, status)
+
+		exitCodeCh, err := r.getExitCodeCh(pid)
+		if err != nil {


Can you make it clear in the comments that we are only interested in the exit code of the Container and Exec processes, and ignoring the exit code from children and grandchildren(just reaping them)

Yes good point, I'll try to add more comments !

amshinde · 2017-12-20T23:52:44Z

agent.go

@@ -856,12 +895,36 @@ func (p *pod) runContainerProcess(cid, pid string, terminal bool, started chan e

 	fieldLogger := agentLog.WithField("container-pid", pid)

+	// This lock is very important to avoid any race with reaper.reap().
+	// Indeed, if we don't lock this here, we could potentially get the
+	// SIGCHLD signal before the channel has been created, meaning we will


Can you mention exit code channel to be more clear.

amshinde · 2017-12-20T23:58:25Z

agent.go

 	}
-	// Close pipes to terminate routeOutput() go routines.
-	ctr.closeProcessPipes(pid)


this is not required anymore?

This is still called from the defer here: https://github.com/clearcontainers/agent/pull/178/files/e8d8a5fc6359f172177a782b85f153011d008015..c36a44adc6e06215b1c4cb736aa1e5d57699f82f#diff-2fe34e1b6f7f9aece0af74b1635502eeR889

We need the libcontainer vendoring to be updated so that we can rely on a recent fix preventing libcontainer code to wait on signalled processes in case a subreaper is already set on the system. Fixes #177 Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com>

Our agent had a flaw, it potentially lives with some zombie processes according to what has been done by the container user. The explanation is that we only wait for container and exec processes but we don't handle the children and grandchildren processes coming from those initial container and exec processes. The example of such a case can be shown demonstrated in case the container process spawn a child process, and this one will also spawn another child process. In case the child terminates before its own children, those ones expect to be reaped by the init process of the PID namespace, i.e the container process. But this container process is not written to reap descendant processes, which will lead to a zombie process left behind. This commit solves those cases by setting the agent process, who will be the father of all container processes, as a subreaper. This way, it will receive all the SIGCHLD signals from any child left behind, and will reap them. Notice this patch makes sure we don't check the error from the libcontainer call to process.Wait() since it won't be able to reap the container or exec processes as expected. This libcontainer Wait() function is only used here to properly cleanup the pipes and internal structures created by libcontainer. Fixes #177 Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com>

sboeuf · 2017-12-21T18:19:09Z

@amshinde I have updated the PR with more comments.

amshinde

lgtm

sboeuf · 2017-12-23T01:54:20Z

@amshinde Could you merge if you're fine with this ?

sboeuf added the in progress label Dec 5, 2017

sboeuf mentioned this pull request Dec 5, 2017

agent: Implement subreaper support kata-containers/agent#28

Merged

sboeuf force-pushed the sboeuf/implement_subreaper branch 3 times, most recently from 386b651 to 52a8fa9 Compare December 11, 2017 22:58

sboeuf added the do-not-merge label Dec 11, 2017

sboeuf force-pushed the sboeuf/implement_subreaper branch from 52a8fa9 to 336e6e1 Compare December 15, 2017 20:43

sboeuf force-pushed the sboeuf/implement_subreaper branch 2 times, most recently from f1dd79c to c36a44a Compare December 18, 2017 20:00

sboeuf removed the do-not-merge label Dec 18, 2017

amshinde reviewed Dec 20, 2017

View reviewed changes

Sebastien Boeuf added 2 commits December 21, 2017 09:49

sboeuf force-pushed the sboeuf/implement_subreaper branch from c36a44a to bd925f1 Compare December 21, 2017 18:18

amshinde approved these changes Dec 21, 2017

View reviewed changes

amshinde merged commit e5fb70c into master Dec 23, 2017

sboeuf deleted the sboeuf/implement_subreaper branch January 10, 2018 21:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

agent: Make the agent the subreaper of all processes #178

agent: Make the agent the subreaper of all processes #178

sboeuf commented Dec 5, 2017

sboeuf commented Dec 5, 2017

sboeuf commented Dec 11, 2017

sboeuf commented Dec 15, 2017

sboeuf commented Dec 18, 2017

sboeuf commented Dec 20, 2017

devimc commented Dec 20, 2017 •

edited by grahamwhaley

Loading

sboeuf commented Dec 20, 2017

amshinde Dec 20, 2017

sboeuf Dec 21, 2017

amshinde Dec 20, 2017

sboeuf Dec 21, 2017

amshinde Dec 20, 2017

sboeuf Dec 21, 2017

amshinde Dec 20, 2017

sboeuf Dec 21, 2017

sboeuf commented Dec 21, 2017

amshinde left a comment

sboeuf commented Dec 23, 2017

agent: Make the agent the subreaper of all processes #178

agent: Make the agent the subreaper of all processes #178

Conversation

sboeuf commented Dec 5, 2017

sboeuf commented Dec 5, 2017

sboeuf commented Dec 11, 2017

sboeuf commented Dec 15, 2017

sboeuf commented Dec 18, 2017

sboeuf commented Dec 20, 2017

devimc commented Dec 20, 2017 • edited by grahamwhaley Loading

sboeuf commented Dec 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sboeuf commented Dec 21, 2017

amshinde left a comment

Choose a reason for hiding this comment

sboeuf commented Dec 23, 2017

devimc commented Dec 20, 2017 •

edited by grahamwhaley

Loading