Details
Bug
Blocker
Description
This happened while trying to change the priority of a job from the scheduler servlet.
Activity
It's actually a 2-way deadlock. The JT UI and FairScheduler.assignTasks also got locked up while trying to obtain the lock on JT object.
Thanks for finding this. Here's a patch that should fix it. It makes the FairSchedulerServlet lock the scheduler rather than the poolManager, which is how the rest of the code works (JobTracker's assignTasks, job listener stuff, and update thread). I haven't added any unit tests because I don't know how one would write a unit test for this, but it's a pretty simple patch.
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12399685/hadoop-5154-v0.patch
against trunk revision 742171.
+1 @author. The patch does not contain any @author tags.
-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no tests are needed for this patch.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs warnings.
+1 Eclipse classpath. The patch retains Eclipse classpath integrity.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
-1 core tests. The patch failed core unit tests.
-1 contrib tests. The patch failed contrib unit tests.
Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3815/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3815/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3815/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3815/console
This message is automatically generated.
A couple of comments:
- The PoolManager instance is not used in the setPriority part of the code, so can be removed.
- Is there a reason that you're not calling update() on the scheduler within the synchronized block?
Here's a new patch taking into account Tom's comments. Tom, the reason I had moved the update outside of the loop was because I thought I might want to acquire a lock on the JobTracker in update later, which could cause a deadlock with assignTasks (which locks the JT and then the scheduler), but now I think that this is not necessary so there's no reason to do it.
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12400154/hadoop-5154-v1.patch
against trunk revision 744406.
+1 @author. The patch does not contain any @author tags.
-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no tests are needed for this patch.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs warnings.
+1 Eclipse classpath. The patch retains Eclipse classpath integrity.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3855/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3855/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3855/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3855/console
This message is automatically generated.
Updated this patch a little so it will continue working after we commit HADOOP-4665. In 4665 (the preemption patch), the update() method in the FairScheduler needs a lock on the JobTracker to be able to call killTasks. However, since the JobTracker locks itself before locking the FairScheduler in its assignTasks logic, the lock order has to be JobTracker then FairScheduler (otherwise we'd get a deadlock). Therefore update() also locks first the JT and then the FairScheduler. I've modified this patch to ensure that the two pieces of code that call scheduler.update() do the same.
Marking this for 0.18.4 and above since it's a deadlock. The patch should work on all fair scheduler versions.
Matei, couple of comments:
- I see you are now locking on the JobTracker and then the scheduler. The Fairscheduler web UI has refresh turned on by default. I am wondering if this can cause excessive lock contention with the JT, which could affect the JT code - like scheduling, etc. Maybe the refresh should be turned off by default, and enabled with a large refresh period on demand ?
- Doesn't showJobs also need synchronization on the JobTracker and scheduler ?
Hi Hemanth,
The reason showJobs doesn't lock the JobTracker is because it only looks at information in the fair scheduler. I have the lock around the JT for the other two methods because update() will require a lock on the JT once preemption is introduced in order to safely call killTask() on the JobTracker without causing a deadlock. If you just lock the JT independently or the FS independently, you can't cause a deadlock, but the problem is that the assignTasks method in the JobTracker locks first the JT and then the FS and thus causes a problem. However, maybe a better solution is to forget the lock on the JT in this patch, and change the way killTasks is called so that it's not called with a lock around the FairScheduler. This could be done by adding the tasks to kill into a queue and having a second thread kill them. Since the preemption patch, HADOOP-4665 is not going into release 0.20, I'm going to do that for this patch and fix up the patch for HADOOP-4665 to ensure that you only need a lock on the FS when calling update().
I can also turn off the refresh by default. How is it handled on the JobTracker's web UI? For some reason I thought that refreshed.
Here's a patch with the changes in my previous comment.
The reason showJobs doesn't lock the JobTracker is because it only looks at information in the fair scheduler.
Matei, we access JobTracker.runningJobs(). This is accessing the jobs data structure in an unsynchronized manner. I see there's a JobTracker.getRunningJobs() which is providing synchronized access.
I also meant that the scheduler instance is not being locked when accessing APIs in the scheduler. Is that not required as well ?
How is it (refresh) handled on the JobTracker's web UI? For some reason I thought that refreshed.
The JobTracker web page is not refreshed. The Job and task details web pages are, and I see that the Job details web page does not synchronize access to the jobs data structure as well. But that is still a bug. smile.
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12400443/hadoop-5154-v3.patch
against trunk revision 745705.
+1 @author. The patch does not contain any @author tags.
-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no tests are needed for this patch.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs warnings.
+1 Eclipse classpath. The patch retains Eclipse classpath integrity.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
-1 core tests. The patch failed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3881/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3881/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3881/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3881/console
This message is automatically generated.
We can change runningJobs to getRunningJobs. Where do you see scheduler APIs being used without a lock though?
We access scheduler.infos and scheduler.getPoolManager. Since infos is being modified in the scheduler code protected by the FairScheduler instance, this leads to inconsistent access. Likewise, access to pool manager is being synchronized on the scheduler instance in other methods. Basically, I am just looking at showJobs as being equivalent to showPools, and hence should have the same synchronization constructs.
If you are modifying the patch to call getRunningJobs and introducing the synchronization around the scheduler, please make sure that there's no inversion in the order of locking between the scheduler and job tracker instances.
Updated patch to take into account your comments. I chose to call getRunningJobs before locking the scheduler (thus locking the JT alone, which is fine). It returns a new list so it should be okay to iterate through this list later.
Marking this for 19.2 instead of 18.4 because there's no fair scheduler in 18.4.
Matei, this looks good. Thanks for the changes.
I made one small change to make access to the useFifo variable also synchronized, and uploaded a patch with this change alone. All other changes are from your patch. (I took this liberty because we are just trying to stabilize the 0.20 release soon, and would very much like this patch into the branch ASAP.) I verified that changes to the scheduling mode from the web ui is working with this change. If you are fine with these changes, I think it is good to go.
Running through Hudson with the new patch
+1, looks good to me.
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12400844/hadoop-5154-v5.patch
against trunk revision 747332.
+1 @author. The patch does not contain any @author tags.
-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no tests are needed for this patch.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs warnings.
+1 Eclipse classpath. The patch retains Eclipse classpath integrity.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
-1 core tests. The patch failed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3912/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3912/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3912/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3912/console
This message is automatically generated.
As explained above, there are no test cases in this patch, because it is a fix for a deadlock. The -1 on core tests is unrelated. I will commit this patch.
This patch applies to the Hadoop 0.19 branch. The earlier one did not.
I've run ant test-patch, test cases for fairscheduler, and also sanity tested the servlet on a 4 node cluster, changing priorities, switching scheduling modes etc.
The patch was not different in content from the earlier one, only merged for the 0.19 branch. So, I am committing this.
I just committed this to trunk and the 0.20 and 0.19 branches. Thanks, Matei !
Integrated in Hadoop-trunk #766 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/766/)
. Fixes a deadlock in the fairshare scheduler. Contributed by Matei Zaharia.
Attached thread dump.
Summary:
JT UI (jobtracker.jsp)
FairScheduler.assignTasks
FairScheduler update thread
FairScheduler.update (from scheduler UI)