Use job control for tasks (and therefore for pipes and streaming)

• Key: HADOOP-2721
• Type: Improvement
• Status: Resolved
• Resolution: Fixed
• Priority: Major
• Assignee: Ravi Gummadi
• Reporter: Owen O'Malley
• Fix Version/s: 0.21.0

Description

We should use the setsid command when the task is launched to create a new session. We should be able to use the setsid program when we launch the bash process to create a new session. That will allow us to kill the entire session with a single signal and remove the need for the ping methods in both TaskTracker.Child and pipes.

The patch uses setsid when creating new tasks sothat subprocesses of this process will be with in this new session(and this process will be the process leader for all the subprocesses). Thus killing the subprocesses becomes easy(just by killing all the processes in this process group) when killing the task.

Comments

[Arun C Murthy]

+10

More discussion here: http://issues.apache.org/jira/browse/HADOOP-2092?focusedCommentId=12537386#action_12537386

[Ravi Gummadi]

Attached the patch for review.
This patch uses setsid while creating tasks(map/reduce) and uses "kill – -processGroupID" to kill all the processes in the process group, thus killing the whole subtree of processes.
Of course, this will not work if the mapper/reducer itself uses setsid or setpgid for any of the processes in the subtree of processes we are trying to handle.

[Hemanth Yamijala]

I've not looked at this patch. But from the description, and a few discussions, the implementation likely will undergo changes after HADOOP-4490. I don't think the feature will change, just that I may need to modify the changes with whatever I am doing. I think it makes more sense for this patch to go first, so I can add this a blocker to HADOOP-4490.

[Ravi Gummadi]

Attached the patch which fixes a problem in earlier patch(In earlier patch, pidFile was not getting created if TaskMemoryManager is not enabled).

Please review the patch and provide your comments.

[Ravi Gummadi]

Fixed a findbugs warning and attached the new patch.

[Ravi Gummadi]

Submitting the patch for review....

Please provide your comments. Thanks.

[Vinod K V]

Had a look at the code. The basic functionality seems fine. Didn't run and check it manually myself, will do so ASAP.

Comments:

• We are now generating pidfiles for every tasks, irrespective of whether memory mangement functionality is enabled or not. This results in an additional trip to disk for every task startup. We should bench-mark so that this doesn't greatly affect the task-startup.
• As we are moving pidfiles beyond the scope of memory management, we should move the pidfile related api(getPidFilePath, removePidFile) out of TaskMemoryManager.
• destroySubprocessTree() should also ensure that the process-tree is indeed terminated/killed by sending SIGKILL if needed. We should do something in the line of ProcfsBasedProcessTree.SigKillThread.
• I think the check to ensure that the process is the process group leader should be made always. This adds one more level of confidence that we are indeed killing the right process. But this should be done in destroySubprocessTree() and only when used by TT. In other cases, this may not be the case. So we need to separate this may be by having separate api for destroySession and destroySubprocessTree. TT would use destroySession.

TestCases:

• We should split the current TestProcfsBasedProcessTree to test basic process-tree killing functionality and memory calculation separately. Also, we somehow need to verify that all the sub-processes are indeed wiped off.
• We also need a MiniMRCluster based test with streaming job to test the process cleanup, simulating a real run.
• Minor priority: Have a test to ensure that the setsid on the PATH that we are using indeed creates the processes with the same pgpid.

That is all I have for now

[Vinod K V]

We should document on which platforms we have setsid available. If it isn't availabe everywhere, we should in the minimum try to fall back to the current mechanism of launching tasks without job control.

Few more code comments:

• destroySession() also goes beyond the scope of ProcfsBasedProcessTree, it should be moved out.
• Add a test-case to test that SIGKILL works.
• I think assertions are disabled by default, instead you should have explicit checks.

[Ravi Gummadi]

Attaching an intermediate patch HADOOP-2721-v2.patch that addresses:

(1) As we are moving pidfiles beyond the scope of memory management, we should move the pidfile related api(getPidFilePath, removePidFile) out of TaskMemoryManager.

Moved them to TaskTracker.

(2) destroySubprocessTree() should also ensure that the process-tree is indeed terminated/killed by sending SIGKILL if needed. We should do something in the line of ProcfsBasedProcessTree.SigKillThread.

destroySubprocessTree() ensures that the process-tree is indeed terminated/killed by sending SIGKILL. SigKillThread is moved from ProcfsBasedProcessTree to a separate file.

(3) I think the check to ensure that the process is the process group leader should be made always. This adds one more level of confidence that we are indeed killing the right process. But this should be done in destroySubprocessTree() and only when used by TT. In other cases, this may not be the case. So we need to separate this may be by having separate api for destroySession and destroySubprocessTree. TT would use destroySession.

The check to ensure that the process is the process group leader is made always. Now we have destroyProcessGroup() instead of destroySubProcessTree().

Testcase: Increased the memoryLimit to 30MB so that the subtree would be bigger(now it has 8 processes). Added a verification in TestProcfsBasedProcessTree that checks if all the sub-processes are indeed wiped off.

[Ravi Gummadi]

Attaching patch HADOOP-2721-v3.patch that moves destroyProcessTree to a new class ProcessTree. Moved isAlive(pid), getValidPID(String), getPidFromPidFile(String) also to this new class as they are independent of ProcfsBasedProcessTree. This patch also checks if setsid is available/supported on the machine and uses only if available. Kills the process group if setsid is available and kills only the root process otherwise.

[Ravi Gummadi]

Attaching the patch HADOOP-2721-v9.patch for trunk.
Please review the patch and provide your comments.

[Ravi Gummadi]

Attaching the patch with a little change — Now destroyProcessGroup and destroyProcess in ProcessTree will take sleeptime-before-sigkill as a parameter.

The overall summary of the patch is as follows:

(1) Using setsid when starting a task sothat all the subprocesses of the task will have the same sessionId and processGroupId as that of the java task(and java task is the process group leader).

(2) Using pidFile(earlier created only for TaskMemoryManagerThread; Now created even if memory manager is disabled) for getting the pid of the java task.

(3) Killing the whole process group if setsid was used to create the java task. Killing only the java task(similar to earlier) if setsid is not supported on the machine.

(4) Moved getPidFilePath(), removePidFile() from TaskMemoryManagerThread to TaskTracker as they are independent of TaskMemoryManagerThread.

(5) Created a new class ProcessTree with destroy() - for destroying the process tree(killing the process group if setsid is supported, killing only the java task otherwise). Also moved isAlive(pid), getPidFromPidFile() to ProcessTree as they are independent of ProcfsBasedProcessTree. destroyProcessGroup verifies if the given pid is indeed a process group leader.

(6) destroyProcessGroup() and destroyProcess() in ProcessTree class ensures that the process-tree is indeed terminated/killed by sending SIGKILL if SIGTERM is ignored. SigKillThread is moved to ProcessTree as a static inner class and made it to kill (a) single process or (b) process group based on a param. sigkill also takes a parameter whether sigkill is to be sent in the same thread OR in a separate thread(in the background).

(7) In TestProcfsBasedProcessTree, (a) using setsid to create java task, (b) using build.test.data as the testdir instead of creating shellScript and pidFile in current dir, (c) added a check that verifies if the whole the process-tree is indeed killed or not(This is done by constructing the whole subtree of processes and traversing it and checking if any of the processes is alive).

(8) Added a new testcase(with MiniMRCluster) that tests killJob of job that has tasks with children(or subtree of processes) and verifies if the subprocesses are also killed. KillMapperWIthChild is the mapper that just sleeps till it gets killed.

[Vinod K V]

+1 for the patch. Can a committer have a look at this and give a code review? Devaraj?

Few miscelleneous points:

• With this patch, everytime a new task is launch, its pid file is written. This is another trip to disk at task startup and so this needs benchmarking. Ravi is on a short vacation, so i'll try to see if I can run a benchmark myself.
• This patch brings job control for task startup for both java tasks and by default to streaming. For pipes, we still need some work, I propose it to be done in another JIRA if nobody else have any objection to this.
• We also need patches for the previous versions starting from 0.18.

[Vinod K V]

For pipes, we still need some work, I propose it to be done in another JIRA if nobody else have any objection to this.

Offline, Ravi commented that it can be done in this patch itself. Only one minor change is needed for pipes, so.

[Ravi Gummadi]

Attaching the patch that handles pipes applications sothat the c++ process will not get its own session id — but instead gets the same session id as the java task(the parent of c++ process).

[Ravi Gummadi]

Ran gridmix to check performance impact. Average perf got is 2860sec, which is fine.

[Ravi Gummadi]

Attaching a patch which fixes a javadoc warning.

test-patch gives:

[exec] +1 overall.
[exec]
[exec] +1 @author. The patch does not contain any @author tags.
[exec]
[exec] +1 tests included. The patch appears to include 6 new or modified tests.
[exec]
[exec] +1 javadoc. The javadoc tool did not generate any warning messages.
[exec]
[exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
[exec]
[exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
[exec]
[exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

Unit tests also passed on my local machine.

[Ravi Gummadi]

Thanks Vinod for pinting out the backwards incompatibility of prototype change for public method captureOutAndError and few in ProcfsBasedProcessTree.
Attaching the patch that fixes the incompatibility issue.

[Vinod K V]

+1 for the patch.

[Ravi Gummadi]

test-patch gives:

[exec] +1 overall.
[exec]
[exec] +1 @author. The patch does not contain any @author tags.
[exec]
[exec] +1 tests included. The patch appears to include 6 new or modified tests.
[exec]
[exec] +1 javadoc. The javadoc tool did not generate any warning messages.
[exec]
[exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
[exec]
[exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
[exec]
[exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

Unit tests also passed on my local machine.

[Devaraj Das]

Looks good overall. Some comments:
1) The ProcessTree class could have destroy methods that take a boolean to decide whether to destroy the process group or a single process.
2) We can avoid calling the ProcfsBasedProcessTree.destroyProcess calls in JvmManager and only call the ProcessTree.destroy. The only thing that is additionally checked for in the ProcfsBasedProcessTree.destroyProcess is whether the pid passed is the process group leader. In our usecase, we are sure that this pid IS the process group leader...

Longer term, we should consider refactoring the ProcessTree class so that it is a generic class with specific implementations like ProcfsBasedProcessTree and others deriving from it and overriding methods, much like the hierarchy we have for the FileSystem classes..

[Vinod K V]

+1 as the code will be a lot cleaner with these suggested changes.

[Ravi Gummadi]

Attaching patch with the above 2 comments from Devaraj.

Unit tests passed on my local machine.

[Ravi Gummadi]

ant test-patch gave:

[exec] +1 overall.
[exec]
[exec] +1 @author. The patch does not contain any @author tags.
[exec]
[exec] +1 tests included. The patch appears to include 6 new or modified tests.
[exec]
[exec] +1 javadoc. The javadoc tool did not generate any warning messages.
[exec]
[exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
[exec]
[exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
[exec]
[exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

Unit tests also passed on my local machine.

[Devaraj Das]

I just committed this. Thanks Ravi and Vinod!

[Hudson]

Integrated in Hadoop-trunk #756 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/756/)

[Robert Chansler]

Editorial pass over all release notes prior to publication of 0.21.