Ensure maxConcurrentPerNode is honored for declarative pipeline jobs#461
Open
anlambert wants to merge 3 commits into
Open
Ensure maxConcurrentPerNode is honored for declarative pipeline jobs#461anlambert wants to merge 3 commits into
anlambert wants to merge 3 commits into
Conversation
Due to WorkflowJob instance being wrapped in a PlaceHolderTask instance when ThrottleQueueTaskDispatcher.canTake is called, the computed run count for a declarative pipeline job was always equal to 0 as the wrong task was used to compare with those running on node executors. The changes in this commit ensures that maxConcurrentPerNode is now honored on all slave nodes. Regarding the built-in (or master) node, throttling issues remain as flyweight tasks running on it are considered when counting job runs, but flyweight tasks on master node are used to track and coordinate pipeline builds running on slave nodes so jobs running on slaves are counted as running on master. Fixes jenkinsci#413 and jenkinsci#446.
This change fixes the per node throtlling of pipeline jobs on the built-in node when slave nodes are also available. Do no count running tasks on one-off executors for pipeline jobs are those are used by the built-in node to track and coordinate pipeline build on slave nodes. That fix is quite similar to the one from jenkinsci#57 whose purpose was to not use code path counting flyweight tasks for pipeline jobs, but for the canRun case while that fix is for the canTake one. Update test checking the one job per node case by also creating pipeline jobs on the built-in node to validate the changes.
There was a problem hiding this comment.
Pull request overview
This PR fixes pipeline throttling in the Jenkins throttle-concurrent-builds plugin so maxConcurrentPerNode is honored for declarative WorkflowJobs, including scenarios involving the built-in node. It updates the dispatcher’s pipeline task accounting and re-enables/expands the previously disabled regression test covering per-node throttling.
Changes:
- Unwrap
PlaceholderTaskinstances to evaluate throttling against the underlyingWorkflowJob. - Adjust executor/run counting so pipeline jobs are counted correctly on nodes, especially around built-in-node flyweight tasks.
- Rework the pipeline regression test to cover both an agent node and the built-in node.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/main/java/hudson/plugins/throttleconcurrents/ThrottleQueueTaskDispatcher.java |
Updates queue-dispatch throttling logic and executor counting for pipeline jobs. |
src/test/java/hudson/plugins/throttleconcurrents/ThrottleJobPropertyPipelineTest.java |
Re-enables and broadens the pipeline per-node throttling test to include built-in-node behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
86
to
87
| if (!pipelineCategories.isEmpty() || (tjp != null && tjp.getThrottleEnabled())) { | ||
| CauseOfBlockage cause = canRunImpl(task, tjp, pipelineCategories); | ||
| if (cause != null) { | ||
| return cause; | ||
| } | ||
| if (tjp != null) { |
Comment on lines
+590
to
+597
| SubTask executorTask = currentExecutable.getParent(); | ||
| if (executorTask instanceof PlaceholderTask placeholderTask) { | ||
| // when dealing with a pipeline job, we need to extract the wrapped | ||
| // WorkflowJob to ensure correct run counts | ||
| executorTask = placeholderTask.getOwnerTask(); | ||
| } | ||
| if (task.equals(executorTask)) { | ||
| runCount++; |
| assertFalse(j.jenkins.getQueue().isEmpty()); | ||
|
|
||
| } else { | ||
| // after terminating first job on seconde node, |
|
|
||
| if (task instanceof PlaceholderTask placeholderTask) { | ||
| // when dealing with a pipeline job, ThrottleJobProperty is defined in the | ||
| // WorkflowJob wrapped in a PlaceHolderTask so update task to ensure correct |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I noticed that
maxConcurrentPerNodeparameter was not honored for declarative pipeline jobs and discovered that it was a known issue (#413 and #446).Fixes #413
Fixes #446
I would like that feature to work as most of the jobs on our Jenkins instance are pipeline jobs. So I took some time to debug the issue and found a fix ensuring that parameter is now honored on all slave nodes, throttling issues still remain on the built-in node, see commit message below.
Due to
WorkflowJobinstance being wrapped in aPlaceHolderTaskinstance whenThrottleQueueTaskDispatcher.canTakeis called, the computed run count for a declarative pipeline job was always equal to 0 as the wrong task was used to compare with those running on node executors.The changes in this commit ensures that
maxConcurrentPerNodeis now honored on all slave nodes.Regarding the built-in (or master) node, throttling issues remain as flyweight tasks running on it are considered when counting job runs, but flyweight tasks on master node are used to track and coordinate pipeline builds running on slave nodes so jobs running on slaves are counted as running on master.
All tests are still passing and the remaining failing one related to that issue now succeeds.
Update: I also added a commit that fixes the remaining per node throttling issue for pipeline jobs on the built-in node when slave nodes are also available.