Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing throttle based on parameter examination (Replaces PR#33) #38

Merged
merged 27 commits into from
Apr 4, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
6394b98
Fixing throttle based on parameter examination
Feb 22, 2016
083b0d2
Adding me as a developer
Feb 22, 2016
06ae282
Turning off auto organize imports
Feb 22, 2016
39826bf
Turning off auto organize imports for Test object
Feb 22, 2016
81cb354
Cleaning up pom file.
Feb 22, 2016
e85f7cb
Adding previous property logic
Feb 22, 2016
f29281e
Adding previous property logic
Feb 23, 2016
3502248
Restoring whitespace
Feb 23, 2016
8569c5f
Restoring whitespace - tests
Feb 23, 2016
c7f5f5c
Restoring whitespace - ThrottleProperty.java
Feb 23, 2016
8af5067
Restoring whitespace - ThrottleProperty.java
Feb 23, 2016
9927810
Restoring whitespace - ThrottleMatrixProjectOptions.java
Feb 23, 2016
c187ec9
Restoring whitespace - ThrottleQueueTaskDispatcher.java
Feb 23, 2016
4416d5d
Restoring whitespace - ThrottleIntegrationTest.java
Feb 23, 2016
0f9777f
Restoring whitespace - ThrottleJobPropertyTest.java
Feb 23, 2016
74e251a
Restoring whitespace - ThrottleQueueTaskDispatcherTest.java
Feb 23, 2016
f2039ed
Restoring whitespace - ThrottleCategoryTest.java
Feb 23, 2016
0194eaa
Re-organize pom for easier review.
Feb 23, 2016
b988253
Adding contributors to pom
Feb 23, 2016
6c033fa
Whitespace
Feb 23, 2016
e505a08
Removing extra jelly.
Feb 23, 2016
c20aa40
Fixing source and target and allowing parent pom to source matrix pro…
Feb 23, 2016
884c411
Adding back version.
Feb 23, 2016
1e2f8a4
Trying without HtmlUnit specific version
Feb 23, 2016
395f1f6
Array utils recommendation, and changing job lookup/compare to be bas…
Feb 23, 2016
58f6531
Removing extra brace
Feb 23, 2016
6ad147c
Changing optional block help location
Feb 24, 2016
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 34 additions & 13 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ THE SOFTWARE.
<parent>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>plugin</artifactId>
<version>1.424</version>
<version>1.596.1</version>
Copy link
Member

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.

Copy link
Author

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?

Copy link
Member

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".

should plugins not move forward with the code base?

They should not unless the developer know what he / she do and can explain why he / she do that.

Copy link
Author

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.

Copy link
Member

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

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.

Copy link
Author

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.

</parent>

<artifactId>throttle-concurrents</artifactId>
Expand All @@ -42,25 +42,33 @@ THE SOFTWARE.
<comments>All source code is under the MIT license.</comments>
</license>
</licenses>


<properties>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<compileSource>1.6</compileSource>
<compileTarget>1.6</compileTarget>
</properties>

<build>
<pluginManagement>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-enforcer-plugin</artifactId>
<version>1.0-beta-1</version>
<artifactId>maven-clean-plugin</artifactId>
<version>${maven-clean-plugin.version}</version>
</plugin>
<plugin>
<artifactId>maven-compiler-plugin</artifactId>
<version>${maven-compiler-plugin.version}</version>
<configuration>
<source>${compileSource}</source>
<target>${compileTarget}</target>
<showDeprecation>false</showDeprecation>
<showWarnings>false</showWarnings>
</configuration>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we actually need those configurations.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know why we don't. I think they are very useful. I've turned them off until I can address all the deprecated code (HudsonTestCase, HtmlUnit) and the warnings that are abundent. I will start to address some of these next.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... Somehow the comments are outdated.

Those configurations look the default behavior and I think they don't affect the actual behavior.
I don't know so much about maven, and I expect a review by another developer good at maven.

</plugin>
</plugins>
</pluginManagement>
<plugins>
<plugin>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<source>1.6</source>
<target>1.6</target>
</configuration>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-release-plugin</artifactId>
Expand All @@ -85,7 +93,14 @@ THE SOFTWARE.
<timezone>+3</timezone>
</developer>
</developers>


<contributors>
<contributor>
<name>Darren Ball</name>
<email>[email protected]</email>
</contributor>
</contributors>

<repositories>
<repository>
<id>repo.jenkins-ci.org</id>
Expand Down Expand Up @@ -113,6 +128,12 @@ THE SOFTWARE.
<version>2.0.1</version>
<type>jar</type>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>matrix-project</artifactId>
<version>1.4.1</version>
</dependency>

</dependencies>
</project>

Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import hudson.matrix.MatrixBuild;
import hudson.matrix.MatrixProject;

import java.util.Arrays;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
Expand All @@ -29,6 +30,8 @@

import net.sf.json.JSONObject;

import org.apache.commons.lang.ArrayUtils;
import org.apache.commons.lang.StringUtils;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.QueryParameter;
import org.kohsuke.stapler.StaplerRequest;
Expand All @@ -42,9 +45,13 @@ public class ThrottleJobProperty extends JobProperty<Job<?,?>> {
private List<String> categories;
private boolean throttleEnabled;
private String throttleOption;
private boolean limitOneJobWithMatchingParams;
private transient boolean throttleConfiguration;
private @CheckForNull ThrottleMatrixProjectOptions matrixOptions;

private String paramsToUseForLimit;
private transient List<String> paramsToCompare;

/**
* Store a config version so we're able to migrate config on various
* functionality upgrades.
Expand All @@ -57,6 +64,8 @@ public ThrottleJobProperty(Integer maxConcurrentPerNode,
List<String> categories,
boolean throttleEnabled,
String throttleOption,
boolean limitOneJobWithMatchingParams,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change breaks the binary compatibility.
As well as previous ones, so no complains

String paramsToUseForLimit,
@CheckForNull ThrottleMatrixProjectOptions matrixOptions
) {
this.maxConcurrentPerNode = maxConcurrentPerNode == null ? 0 : maxConcurrentPerNode;
Expand All @@ -66,7 +75,20 @@ public ThrottleJobProperty(Integer maxConcurrentPerNode,
new CopyOnWriteArrayList<String>(categories);
this.throttleEnabled = throttleEnabled;
this.throttleOption = throttleOption;
this.limitOneJobWithMatchingParams = limitOneJobWithMatchingParams;
this.matrixOptions = matrixOptions;
this.paramsToUseForLimit = paramsToUseForLimit;
if ((this.paramsToUseForLimit != null)) {
if ((this.paramsToUseForLimit.length() > 0)) {
this.paramsToCompare = Arrays.asList(ArrayUtils.nullToEmpty(StringUtils.split(this.paramsToUseForLimit)));
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

}
else {
this.paramsToCompare = new ArrayList<String>();
}
}
else {
this.paramsToCompare = new ArrayList<String>();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StringUtils.split provides split + null check.
ArrayUtils.nullToEmpty converts null to empty array.
They will help you.

}


Expand Down Expand Up @@ -126,6 +148,10 @@ public boolean getThrottleEnabled() {
return throttleEnabled;
}

public boolean isLimitOneJobWithMatchingParams() {
return limitOneJobWithMatchingParams;
}

public String getThrottleOption() {
return throttleOption;
}
Expand All @@ -148,6 +174,10 @@ public Integer getMaxConcurrentTotal() {
return maxConcurrentTotal;
}

public String getParamsToUseForLimit() {
return paramsToUseForLimit;
}

@CheckForNull
public ThrottleMatrixProjectOptions getMatrixOptions() {
return matrixOptions;
Expand All @@ -171,6 +201,23 @@ public boolean isThrottleMatrixConfigurations() {
: ThrottleMatrixProjectOptions.DEFAULT.isThrottleMatrixConfigurations();
}

public List<String> getParamsToCompare() {
if (paramsToCompare == null) {
if ((paramsToUseForLimit != null)) {
if ((paramsToUseForLimit.length() > 0)) {
paramsToCompare = Arrays.asList(paramsToUseForLimit.split(","));
}
else {
paramsToCompare = new ArrayList<String>();
}
}
else {
paramsToCompare = new ArrayList<String>();
}
}
return paramsToCompare;
}

static List<Queue.Task> getCategoryTasks(String category) {
assert category != null && !category.equals("");
List<Queue.Task> categoryTasks = new ArrayList<Queue.Task>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In corner cases there may be an NPE here, because ThrottleJobProperty may be deleted during the run.
I'll fix it in #42

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.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logger provides message-format-style formatting.

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) {
Expand Down
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
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,17 @@
field="maxConcurrentPerNode">
<f:textbox />
</f:entry>


<f:optionalBlock field="limitOneJobWithMatchingParams"
title="${%Prevent multiple jobs with identical parameters from running concurrently}"
inline="true">
<f:entry title="${%List of parameters to check (comma-separated)}"
field="paramsToUseForLimit">
<f:textbox />
</f:entry>
</f:optionalBlock>

<j:if test="${!empty(descriptor.categories)}">
<f:entry title="${%Multi-Project Throttle Category}">
<j:forEach var="cat" items="${descriptor.categories}">
Expand Down
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>
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,8 @@ public void testThrottlingWithCategory() throws Exception {
Arrays.asList(category), // categories
true, // throttleEnabled
"category", // throttleOption
false,
null,
ThrottleMatrixProjectOptions.DEFAULT
));
p1.getBuildersList().add(new SleepBuilder(SLEEP_TIME));
Expand All @@ -135,6 +137,8 @@ public void testThrottlingWithCategory() throws Exception {
Arrays.asList(category), // categories
true, // throttleEnabled
"category", // throttleOption
false,
null,
ThrottleMatrixProjectOptions.DEFAULT
));
p2.getBuildersList().add(new SleepBuilder(SLEEP_TIME));
Expand Down
Loading