[JENKINS-74975] Fix Semaphore over-allocation of permits / limit all CleanupTasks #495
Conversation
| @@ -382,22 +389,27 @@ public static class Deleter extends ItemListener { | |||
|
|
|||
| private static /* almost final */ int CLEANUP_THREAD_LIMIT = SystemProperties.getInteger(Deleter.class.getName() + ".CLEANUP_THREAD_LIMIT", Integer.valueOf(0)).intValue(); | |||
|
|
|||
| private final transient ExecutorService executorService = Executors.newSingleThreadExecutor(threadFactory()); | |||
There was a problem hiding this comment.
I'm unsure; Will this survive a reload configuration from disk?
There was a problem hiding this comment.
I don't think this class is saved to disk at all; it's just an extension. If you restart Jenkins while a lot of deletion tasks are waiting to run, I do think they will all be lost if that's what you mean, but it looks like that was the case before this PR too.
| @@ -382,22 +389,27 @@ public static class Deleter extends ItemListener { | |||
|
|
|||
| private static /* almost final */ int CLEANUP_THREAD_LIMIT = SystemProperties.getInteger(Deleter.class.getName() + ".CLEANUP_THREAD_LIMIT", Integer.valueOf(0)).intValue(); | |||
|
|
|||
| private final transient ExecutorService executorService = Executors.newSingleThreadExecutor(threadFactory()); | |||
There was a problem hiding this comment.
I don't think this class is saved to disk at all; it's just an extension. If you restart Jenkins while a lot of deletion tasks are waiting to run, I do think they will all be lost if that's what you mean, but it looks like that was the case before this PR too.
|
@dwnusbaum Can we merge this ? |
|
@Dohbedoh sure yes sorry I missed your earlier message |
(amends #237)
The
CleanupTaskof the built-in node were scheduled instantly without acquiring permit from the Semaphore but they were releasing permits, hence creating additional permits over time. Reducing the effectiveness of the limit overtime.Add the Jenkins
built-innode to list of nodes to be limited.NOTES: I guess another solution is to continue not limit the number of
CleanupTaskexecuted for thebuilt-innode but that looks rather confusing in the first place to have aCLEANUP_THREAD_LIMITthat only partially limit the Cleanup tasks..Testing done
-Djenkins.branch.WorkspaceLocatorImpl\$Deleter.CLEANUP_THREAD_LIMIT=1jenkins.branch.WorkspaceLocatorImpl.Deleter.cleanupPool.availablePermits()and validate that it shows1jenkins.branch.WorkspaceLocatorImpl.Deleter.cleanupPool.availablePermits()and validate that it still shows1Submitter checklist