Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Merge pull request #1229 from synopsys-arc-oss/CLI_null_ParameteValue
[READY][FIXED JENKINS-15094] - Handle null parameter values to avoid massive executor deaths
  • Loading branch information
olivergondza committed Jun 1, 2014
2 parents 9f3a2b5 + 7b894f5 commit 9402bd8
Show file tree
Hide file tree
Showing 6 changed files with 178 additions and 12 deletions.
17 changes: 14 additions & 3 deletions core/src/main/java/hudson/cli/BuildCommand.java
Expand Up @@ -51,6 +51,7 @@
import java.util.Map.Entry;
import java.io.FileNotFoundException;
import java.io.PrintStream;
import javax.annotation.Nonnull;

import jenkins.model.Jenkins;

Expand Down Expand Up @@ -101,15 +102,21 @@ protected int run() throws Exception {
if (pdp==null)
throw new AbortException(job.getFullDisplayName()+" is not parameterized but the -p option was specified");

//TODO: switch to type annotations after the migration to Java 1.8
List<ParameterValue> values = new ArrayList<ParameterValue>();

for (Entry<String, String> e : parameters.entrySet()) {
String name = e.getKey();
ParameterDefinition pd = pdp.getParameterDefinition(name);
if (pd==null)
if (pd==null) {
throw new AbortException(String.format("\'%s\' is not a valid parameter. Did you mean %s?",
name, EditDistance.findNearest(name, pdp.getParameterDefinitionNames())));
values.add(pd.createValue(this, Util.fixNull(e.getValue())));
}
ParameterValue val = pd.createValue(this, Util.fixNull(e.getValue()));
if (val == null) {
throw new AbortException(String.format("Cannot resolve the value for the parameter \'%s\'.",name));
}
values.add(val);
}

// handle missing parameters by adding as default values ISSUE JENKINS-7162
Expand All @@ -118,7 +125,11 @@ protected int run() throws Exception {
continue;

// not passed in use default
values.add(pd.getDefaultParameterValue());
ParameterValue defaultValue = pd.getDefaultParameterValue();
if (defaultValue == null) {
throw new AbortException(String.format("No default value for the parameter \'%s\'.",pd.getName()));
}
values.add(defaultValue);
}

a = new ParametersAction(values);
Expand Down
5 changes: 5 additions & 0 deletions core/src/main/java/hudson/model/ParameterDefinition.java
Expand Up @@ -33,6 +33,7 @@
import java.io.Serializable;
import java.io.IOException;
import java.util.logging.Logger;
import javax.annotation.CheckForNull;

import jenkins.model.Jenkins;
import net.sf.json.JSONObject;
Expand Down Expand Up @@ -165,6 +166,7 @@ public ParameterDescriptor getDescriptor() {
* This method is invoked when the user fills in the parameter values in the HTML form
* and submits it to the server.
*/
@CheckForNull
public abstract ParameterValue createValue(StaplerRequest req, JSONObject jo);

/**
Expand All @@ -183,6 +185,7 @@ public ParameterDescriptor getDescriptor() {
* @throws IllegalStateException
* If the parameter is deemed required but was missing in the submission.
*/
@CheckForNull
public abstract ParameterValue createValue(StaplerRequest req);


Expand All @@ -200,6 +203,7 @@ public ParameterDescriptor getDescriptor() {
* the command exits with an error code.
* @since 1.334
*/
@CheckForNull
public ParameterValue createValue(CLICommand command, String value) throws IOException, InterruptedException {
throw new AbortException("CLI parameter submission is not supported for the "+getClass()+" type. Please file a bug report for this");
}
Expand All @@ -210,6 +214,7 @@ public ParameterValue createValue(CLICommand command, String value) throws IOExc
* @return default parameter value or null if no defaults are available
* @since 1.253
*/
@CheckForNull
@Exported
public ParameterValue getDefaultParameterValue() {
return null;
Expand Down
36 changes: 28 additions & 8 deletions core/src/main/java/hudson/model/ParametersAction.java
Expand Up @@ -45,6 +45,8 @@

import static com.google.common.collect.Lists.newArrayList;
import static com.google.common.collect.Sets.newHashSet;
import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;

/**
* Records the parameter values used for a build.
Expand Down Expand Up @@ -74,14 +76,17 @@ public ParametersAction(ParameterValue... parameters) {

public void createBuildWrappers(AbstractBuild<?,?> build, Collection<? super BuildWrapper> result) {
for (ParameterValue p : parameters) {
if (p == null) continue;
BuildWrapper w = p.createBuildWrapper(build);
if(w!=null) result.add(w);
}
}

public void buildEnvVars(AbstractBuild<?,?> build, EnvVars env) {
for (ParameterValue p : parameters)
p.buildEnvironment(build, env);
for (ParameterValue p : parameters) {
if (p == null) continue;
p.buildEnvironment(build, env);
}
}

// TODO do we need an EnvironmentContributingAction variant that takes Run so this can implement it?
Expand All @@ -102,14 +107,16 @@ public String substitute(AbstractBuild<?,?> build, String text) {
public VariableResolver<String> createVariableResolver(AbstractBuild<?,?> build) {
VariableResolver[] resolvers = new VariableResolver[parameters.size()+1];
int i=0;
for (ParameterValue p : parameters)
for (ParameterValue p : parameters) {
if (p == null) continue;
resolvers[i++] = p.createVariableResolver(build);

}

resolvers[i] = build.getBuildVariableResolver();

return new VariableResolver.Union<String>(resolvers);
}

public Iterator<ParameterValue> iterator() {
return parameters.iterator();
}
Expand All @@ -120,14 +127,17 @@ public List<ParameterValue> getParameters() {
}

public ParameterValue getParameter(String name) {
for (ParameterValue p : parameters)
for (ParameterValue p : parameters) {
if (p == null) continue;
if (p.getName().equals(name))
return p;
}
return null;
}

public Label getAssignedLabel(SubTask task) {
for (ParameterValue p : parameters) {
if (p == null) continue;
Label l = p.getAssignedLabel(task);
if (l!=null) return l;
}
Expand Down Expand Up @@ -166,7 +176,9 @@ public boolean shouldSchedule(List<Action> actions) {
/**
* Creates a new {@link ParametersAction} that contains all the parameters in this action
* with the overrides / new values given as parameters.
* @return New {@link ParametersAction}. The result may contain null {@link ParameterValue}s
*/
@Nonnull
public ParametersAction createUpdated(Collection<? extends ParameterValue> overrides) {
if(overrides == null) {
return new ParametersAction(parameters);
Expand All @@ -175,10 +187,12 @@ public ParametersAction createUpdated(Collection<? extends ParameterValue> overr
Set<String> names = newHashSet();

for(ParameterValue v : overrides) {
if (v == null) continue;
names.add(v.getName());
}

for (ParameterValue v : parameters) {
if (v == null) continue;
if (!names.contains(v.getName())) {
combinedParameters.add(v);
}
Expand All @@ -187,8 +201,14 @@ public ParametersAction createUpdated(Collection<? extends ParameterValue> overr
return new ParametersAction(combinedParameters);
}

public ParametersAction merge(ParametersAction overrides) {
if(overrides == null) {
/*
* Creates a new {@link ParametersAction} that contains all the parameters in this action
* with the overrides / new values given as another {@link ParametersAction}.
* @return New {@link ParametersAction}. The result may contain null {@link ParameterValue}s
*/
@Nonnull
public ParametersAction merge(@CheckForNull ParametersAction overrides) {
if (overrides == null) {
return new ParametersAction(parameters);
}
return createUpdated(overrides.getParameters());
Expand Down
Expand Up @@ -147,7 +147,11 @@ public void _doBuild(StaplerRequest req, StaplerResponse rsp, @QueryParameter Ti
if(d==null)
throw new IllegalArgumentException("No such parameter definition: " + name);
ParameterValue parameterValue = d.createValue(req, jo);
values.add(parameterValue);
if (parameterValue != null) {
values.add(parameterValue);
} else {
throw new IllegalArgumentException("Cannot retrieve the parameter value: " + name);
}
}

WaitingItem item = Jenkins.getInstance().getQueue().schedule(
Expand Down
41 changes: 41 additions & 0 deletions core/src/test/java/hudson/model/ParametersActionTest.java
@@ -1,11 +1,21 @@
package hudson.model;

import hudson.EnvVars;
import hudson.model.queue.SubTask;
import hudson.tasks.BuildWrapper;
import java.util.LinkedList;
import org.junit.Before;
import org.junit.Test;

import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotSame;
import org.junit.runner.RunWith;
import org.jvnet.hudson.test.Bug;
import static org.powermock.api.mockito.PowerMockito.mock;
import org.powermock.modules.junit4.PowerMockRunner;

@RunWith(PowerMockRunner.class)
public class ParametersActionTest {

private ParametersAction baseParamsAB;
Expand Down Expand Up @@ -76,4 +86,35 @@ public void createUpdatedShouldReturnNewInstanceWithNullOverride() {

assertNotSame(baseParamsAB, params);
}

@Test
@Bug(15094)
public void checkNullParamaterValues() {
SubTask subtask = mock(SubTask.class);
Build build = mock(Build.class);

// Prepare parameters Action
StringParameterValue A = new StringParameterValue("A", "foo");
StringParameterValue B = new StringParameterValue("B", "bar");
ParametersAction parametersAction = new ParametersAction(A, null, B);
ParametersAction parametersAction2 = new ParametersAction(A,null);

// Non existent parameter
assertNull(parametersAction.getParameter("C"));
assertNull(parametersAction.getAssignedLabel(subtask));

// Interaction with build
EnvVars vars = new EnvVars();
parametersAction.buildEnvVars(build, vars);
assertEquals(2, vars.size());
parametersAction.createVariableResolver(build);

LinkedList<BuildWrapper> wrappers = new LinkedList<BuildWrapper>();
parametersAction.createBuildWrappers(build, wrappers);
assertEquals(0, wrappers.size());

// Merges and overrides
assertEquals(3, parametersAction.createUpdated(parametersAction2.getParameters()).getParameters().size());
assertEquals(3, parametersAction.merge(parametersAction2).getParameters().size());
}
}
85 changes: 85 additions & 0 deletions test/src/test/groovy/hudson/cli/BuildCommandTest.groovy
Expand Up @@ -25,25 +25,37 @@ package hudson.cli

import org.apache.commons.io.output.TeeOutputStream
import static org.junit.Assert.*
import hudson.Extension
import org.junit.Rule
import org.junit.Test
import org.jvnet.hudson.test.Bug
import org.jvnet.hudson.test.CaptureEnvironmentBuilder
import org.jvnet.hudson.test.JenkinsRule
import org.jvnet.hudson.test.RandomlyFails
import org.jvnet.hudson.test.TestBuilder
import org.jvnet.hudson.test.TestExtension
import org.kohsuke.stapler.StaplerRequest

import hudson.Launcher
import hudson.model.AbstractBuild
import hudson.model.Action
import hudson.model.BuildListener
import hudson.model.Executor
import hudson.model.FreeStyleProject;
import hudson.model.ParameterDefinition.ParameterDescriptor
import hudson.model.ParameterValue
import hudson.model.ParametersAction
import hudson.model.ParametersDefinitionProperty
import hudson.model.Queue.QueueDecisionHandler
import hudson.model.Queue.Task;
import hudson.model.SimpleParameterDefinition
import hudson.model.StringParameterDefinition
import hudson.model.StringParameterValue
import hudson.model.labels.LabelAtom
import hudson.tasks.Shell
import hudson.util.OneShotEvent
import java.util.concurrent.Executor
import net.sf.json.JSONObject

/**
* {@link BuildCommand} test.
Expand Down Expand Up @@ -212,4 +224,77 @@ public class BuildCommandTest {
assertEquals("Command is expected to succeed", 0, result.returnCode());
assertEquals("a=b", project.getBuildByNumber(1).getBuildVariables().get("expr"));
}

@Bug(15094)
@Test public void executorsAliveOnParameterWithNullDefaultValue() throws Exception {
def slave = j.createSlave();
FreeStyleProject project = j.createFreeStyleProject("foo");
project.setAssignedNode(slave);

// Create test parameter with Null default value
def nullDefaultDefinition = new NullDefaultValueParameterDefinition();
ParametersDefinitionProperty pdp = new ParametersDefinitionProperty(
new StringParameterDefinition("string", "defaultValue", "description"),
nullDefaultDefinition);
project.addProperty(pdp);
CaptureEnvironmentBuilder builder = new CaptureEnvironmentBuilder();
project.getBuildersList().add(builder);

// Warmup
j.buildAndAssertSuccess(project);

for (def exec : slave.toComputer().getExecutors()) {
assertTrue("Executor has died before the test start: "+exec, exec.isActive());
}

// Create CLI & run command
def invoker = new CLICommandInvoker(j, new BuildCommand());
def result = invoker
.authorizedTo(jenkins.model.Jenkins.ADMINISTER)
.invokeWithArgs("foo","-p","string=value");
assertEquals("Command is expected to fail with -1 code. \nSTDOUT="+result.stdout()
+"\nSTDERR: "+result.stderr(), -1, result.returnCode());
assertTrue("Unexpected error message",
result.stderr().startsWith("No default value for the parameter \'FOO\'."));

// Give the job 5 seconds to be submitted
def q = j.jenkins.getQueue().getItem(project);
Thread.sleep(5000);

// Check executors health after a timeout
for (def exec : slave.toComputer().getExecutors()) {
assertTrue("Executor is dead: "+exec, exec.isActive());
}
}

public static final class NullDefaultValueParameterDefinition extends SimpleParameterDefinition {

/*package*/ NullDefaultValueParameterDefinition() {
super("FOO", "Always null default value");
}

@Override
public ParameterValue createValue(String value) {
return new StringParameterValue("FOO", "BAR");
}

@Override
public ParameterValue createValue(StaplerRequest req, JSONObject jo) {
return createValue("BAR");
}

@Override
public ParameterValue getDefaultParameterValue() {
return null; // Equals to super.getDefaultParameterValue();
}

@Extension
public static class DescriptorImpl extends ParameterDescriptor {

@Override
public String getDisplayName() {
return "Parameter with the default NULL value";
}
}
}
}

0 comments on commit 9402bd8

Please sign in to comment.