Fixing throttle based on parameter examination (Replaces PR#33) #38
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,17 +3,24 @@ | |
| import hudson.Extension; | ||
| import hudson.matrix.MatrixConfiguration; | ||
| import hudson.matrix.MatrixProject; | ||
| import hudson.model.AbstractProject; | ||
| import hudson.model.ParameterValue; | ||
| import hudson.model.Computer; | ||
| import hudson.model.Executor; | ||
| import hudson.model.Hudson; | ||
| import hudson.model.Job; | ||
| import hudson.model.Node; | ||
| import hudson.model.Queue; | ||
| import hudson.model.Queue.Task; | ||
| import hudson.model.queue.WorkUnit; | ||
| import hudson.model.labels.LabelAtom; | ||
| import hudson.model.queue.CauseOfBlockage; | ||
| import hudson.model.queue.QueueTaskDispatcher; | ||
| import hudson.model.Action; | ||
| import hudson.model.ParametersAction; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.HashMap; | ||
| import java.util.List; | ||
| import java.util.Set; | ||
| import java.util.logging.Level; | ||
|
|
@@ -92,6 +99,9 @@ else if (tjp.getThrottleOption().equals("category")) { | |
| public CauseOfBlockage canRun(Queue.Item item) { | ||
| ThrottleJobProperty tjp = getThrottleJobProperty(item.task); | ||
| if (tjp!=null && tjp.getThrottleEnabled()) { | ||
| if (tjp.isLimitOneJobWithMatchingParams() && isAnotherBuildWithSameParametersRunningOnAnyNode(item)) { | ||
| return CauseOfBlockage.fromMessage(Messages._ThrottleQueueTaskDispatcher_OnlyOneWithMatchingParameters()); | ||
| } | ||
| return canRun(item.task, tjp); | ||
| } | ||
| return null; | ||
|
|
@@ -178,6 +188,106 @@ else if (tjp.getThrottleOption().equals("category")) { | |
| return null; | ||
| } | ||
|
|
||
| private boolean isAnotherBuildWithSameParametersRunningOnAnyNode(Queue.Item item) { | ||
| if (isAnotherBuildWithSameParametersRunningOnNode(Hudson.getInstance(), item)) { | ||
| return true; | ||
| } | ||
|
|
||
| for (Node node : Hudson.getInstance().getNodes()) { | ||
| if (isAnotherBuildWithSameParametersRunningOnNode(node, item)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| private boolean isAnotherBuildWithSameParametersRunningOnNode(Node node, Queue.Item item) { | ||
| ThrottleJobProperty tjp = getThrottleJobProperty(item.task); | ||
| Computer computer = node.toComputer(); | ||
| List<String> paramsToCompare = tjp.getParamsToCompare(); | ||
|
There was a problem hiding this comment. In corner cases there may be an NPE here, because ThrottleJobProperty may be deleted during the run. |
||
| List<ParameterValue> itemParams = getParametersFromQueueItem(item); | ||
|
|
||
| if (paramsToCompare.size() > 0) { | ||
| itemParams = doFilterParams(paramsToCompare, itemParams); | ||
| } | ||
|
|
||
| if (computer != null) { | ||
| for (Executor exec : computer.getExecutors()) { | ||
| if (item != null && item.task != null) { | ||
| // TODO: refactor into a nameEquals helper method | ||
| if (exec.getCurrentExecutable() != null && | ||
| exec.getCurrentExecutable().getParent() != null && | ||
| exec.getCurrentExecutable().getParent().getOwnerTask() != null && | ||
| exec.getCurrentExecutable().getParent().getOwnerTask().getName().equals(item.task.getName())) { | ||
| List<ParameterValue> executingUnitParams = getParametersFromWorkUnit(exec.getCurrentWorkUnit()); | ||
| executingUnitParams = doFilterParams(paramsToCompare, executingUnitParams); | ||
|
|
||
| if (executingUnitParams.containsAll(itemParams)) { | ||
| LOGGER.log(Level.FINE, "build (" + exec.getCurrentWorkUnit() + | ||
| ") with identical parameters (" + | ||
| executingUnitParams + ") is already running."); | ||
|
There was a problem hiding this comment.
|
||
| return true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Filter job parameters to only include parameters used for throttling | ||
| * @param params | ||
| * @param OriginalParams | ||
| * @return | ||
| */ | ||
| private List<ParameterValue> doFilterParams(List<String> params, List<ParameterValue> OriginalParams) { | ||
| if (params.isEmpty()) { | ||
| return OriginalParams; | ||
| } | ||
|
|
||
| List<ParameterValue> newParams = new ArrayList<ParameterValue>(); | ||
|
|
||
| for (ParameterValue p : OriginalParams) { | ||
| if (params.contains(p.getName())) { | ||
| newParams.add(p); | ||
| } | ||
| } | ||
| return newParams; | ||
| } | ||
|
|
||
| public List<ParameterValue> getParametersFromWorkUnit(WorkUnit unit) { | ||
| List<ParameterValue> paramsList = new ArrayList<ParameterValue>(); | ||
|
|
||
| if (unit != null && unit.context != null && unit.context.actions != null) { | ||
| List<Action> actions = unit.context.actions; | ||
| for (Action action : actions) { | ||
| if (action instanceof ParametersAction) { | ||
| ParametersAction params = (ParametersAction) action; | ||
| if (params != null) { | ||
| paramsList = params.getParameters(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return paramsList; | ||
| } | ||
|
|
||
| public List<ParameterValue> getParametersFromQueueItem(Queue.Item item) { | ||
| List<ParameterValue> paramsList; | ||
|
|
||
| ParametersAction params = item.getAction(ParametersAction.class); | ||
| if (params != null) { | ||
| paramsList = params.getParameters(); | ||
| } | ||
| else | ||
| { | ||
| paramsList = new ArrayList<ParameterValue>(); | ||
| } | ||
| return paramsList; | ||
| } | ||
|
|
||
|
|
||
| @CheckForNull | ||
| private ThrottleJobProperty getThrottleJobProperty(Task task) { | ||
| if (task instanceof Job) { | ||
|
|
||
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| ThrottleQueueTaskDispatcher.MaxCapacityOnNode=Already running {0} builds on node | ||
| ThrottleQueueTaskDispatcher.MaxCapacityTotal=Already running {0} builds across all nodes | ||
| ThrottleQueueTaskDispatcher.BuildPending=A build is pending launch | ||
| ThrottleQueueTaskDispatcher.OnlyOneWithMatchingParameters=A build with matching parameters is already running | ||
|
|
||
| ThrottleMatrixProjectOptions.DisplayName=Additional options for Matrix projects |
This file contains 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
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| <div> | ||
| <p>If this box is checked, only one instance of the job with matching parameters will be allowed to run at a given time. | ||
| Other instances of this job with different parameters will be allowed to run concurrently.</p> | ||
| <p>Optionally, provide a comma-separated list of parameters to use when comparing jobs. If blank, all parameters | ||
| must match for a job to be limited to one running instance.</p> | ||
| </div> |
This file contains 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know why you need this version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the version being used by previous attempts to add this functionality, I just used what they used. Also moving forward solved the Xstream issues they encountered (xstream 2.6). Plus - should should plugins not move forward with the code base?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know that "previous attempts" or "Xstream issues they encounteted".
They should not unless the developer know what he / she do and can explain why he / she do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Firstly, that is a rather obnoxious comment IMO. Relax a little regarding the rhetoric referencing people's abilities to explain things. It comes off as condescending. I sure hope that is not your intent. It was a simple explanation. I am attempting to solve a problem for functionality I need in a plugin.
I was just explaining the version. If you look at previous attempts at adding this functionality, of which failed due to various reason, at least one being a possible xstream incompatibility. The version was bumped to pick up a newer version. I believe this(these) PR are referenced in this PR. If not, please look at #33. Remaining at 1.424 does not allow the current functionality to work (test breakage). The code your are reviewing is a compilation of various others and is based on the logic in PR#33.
Outside of this, the problem is solved, functionality is present, it just relies on moving jenkins up.
If you think you can solve the existing issues, using the existing version, feel free. I have a version to use that works with this functionality and I am good with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for that.
I referenced previous reviews, but I cannot get why this version is required.
It looks expected to know more details about previous reviews, and I'm afraid I'm not a good reviewer for this request.
Anyway, the policy for the targeting Jenkins version is up to the plugin maintainers, and we can wait for the review of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a problem. I am attempting to address everything you are suggesting. I would like to get this up and running in an official way. I did attempt to build with the previous version, but it does error out. Seeing the errors and some preliminary searches lead me to increase the version to help alleviate the issues, of which it did. Let's see what other suggest/comment. Moving up to this version causes deprecation (HudsonTestCase) warnings. I suspect that all this needs a good cleanup at some point. I am just jumping in to try and help get functionality that I would really like to have.