[JENKINS-43496] - Add handling of the null Node#createComputer() result. (#2922)
* [JENKINS-43496] - Add handling of the null Node#createComputer() result. it is a follow-up to https://github.com/jenkinsci/jenkins/pull/2836#discussion_r110604865 De-facto many Cloud plugins return `null` in `Node#createLauncher()`, but it has never been documented. In order to prevent possible API misusages in the future, I have added annotations and fixed handling of the extension point in `AbstractCIBase#updateComputer()` which may fail in the case of `null` or `RuntimeException` in the Node implementation. * [JENKINS-43496] - Use ProtectedExternally to protect Node#createComputer() * [JENKINS-43496] - Remove the erroneous Nonnull annotation after the feedback from @jglick * [JENKINS-43496] - Fix typos noticed by @daniel-beck
This commit is contained in:
parent
e7cdd6517c
commit
bcf55ecd7f
|
@ -39,6 +39,7 @@ import java.util.concurrent.CopyOnWriteArraySet;
|
|||
import java.util.logging.Level;
|
||||
import java.util.logging.Logger;
|
||||
import javax.annotation.CheckForNull;
|
||||
import javax.annotation.Nonnull;
|
||||
|
||||
import jenkins.model.Configuration;
|
||||
|
||||
|
@ -125,7 +126,18 @@ public abstract class AbstractCIBase extends Node implements ItemGroup<TopLevelI
|
|||
} else {
|
||||
// we always need Computer for the master as a fallback in case there's no other Computer.
|
||||
if(n.getNumExecutors()>0 || n==Jenkins.getInstance()) {
|
||||
computers.put(n, c = n.createComputer());
|
||||
try {
|
||||
c = n.createComputer();
|
||||
} catch(RuntimeException ex) { // Just in case there is a bogus extension
|
||||
LOGGER.log(Level.WARNING, "Error retrieving computer for node " + n.getNodeName() + ", continuing", ex);
|
||||
}
|
||||
if (c == null) {
|
||||
LOGGER.log(Level.WARNING, "Cannot create computer for node {0}, the {1}#createComputer() method returned null. Skipping this node",
|
||||
new Object[]{n.getNodeName(), n.getClass().getName()});
|
||||
return;
|
||||
}
|
||||
|
||||
computers.put(n, c);
|
||||
if (!n.isHoldOffLaunchUntilSave() && automaticSlaveLaunch) {
|
||||
RetentionStrategy retentionStrategy = c.getRetentionStrategy();
|
||||
if (retentionStrategy != null) {
|
||||
|
@ -136,8 +148,11 @@ public abstract class AbstractCIBase extends Node implements ItemGroup<TopLevelI
|
|||
c.connect(true);
|
||||
}
|
||||
}
|
||||
used.add(c);
|
||||
} else {
|
||||
// TODO: Maybe it should be allowed, but we would just get NPE in the original logic before JENKINS-43496
|
||||
LOGGER.log(Level.WARNING, "Node {0} has no executors. Cannot update the Computer instance of it", n.getNodeName());
|
||||
}
|
||||
used.add(c);
|
||||
}
|
||||
}
|
||||
|
||||
|
@ -184,7 +199,7 @@ public abstract class AbstractCIBase extends Node implements ItemGroup<TopLevelI
|
|||
byName.put(node.getNodeName(),c);
|
||||
}
|
||||
|
||||
Set<Computer> used = new HashSet<Computer>(old.size());
|
||||
Set<Computer> used = new HashSet<>(old.size());
|
||||
|
||||
updateComputer(AbstractCIBase.this, byName, used, automaticSlaveLaunch);
|
||||
for (Node s : getNodes()) {
|
||||
|
|
|
@ -40,6 +40,7 @@ import hudson.remoting.VirtualChannel;
|
|||
import hudson.security.ACL;
|
||||
import hudson.security.AccessControlled;
|
||||
import hudson.security.Permission;
|
||||
import hudson.slaves.Cloud;
|
||||
import hudson.slaves.ComputerListener;
|
||||
import hudson.slaves.NodeDescriptor;
|
||||
import hudson.slaves.NodeProperty;
|
||||
|
@ -65,6 +66,8 @@ import jenkins.util.io.OnMaster;
|
|||
import net.sf.json.JSONObject;
|
||||
import org.acegisecurity.Authentication;
|
||||
import org.jvnet.localizer.Localizable;
|
||||
import org.kohsuke.accmod.Restricted;
|
||||
import org.kohsuke.accmod.restrictions.ProtectedExternally;
|
||||
import org.kohsuke.stapler.BindInterceptor;
|
||||
import org.kohsuke.stapler.Stapler;
|
||||
import org.kohsuke.stapler.StaplerRequest;
|
||||
|
@ -214,8 +217,13 @@ public abstract class Node extends AbstractModelObject implements Reconfigurable
|
|||
|
||||
/**
|
||||
* Creates a new {@link Computer} object that acts as the UI peer of this {@link Node}.
|
||||
*
|
||||
* Nobody but {@link Jenkins#updateComputerList()} should call this method.
|
||||
* @return Created instance of the computer.
|
||||
* Can be {@code null} if the {@link Node} implementation does not support it (e.g. {@link Cloud} agent).
|
||||
*/
|
||||
@CheckForNull
|
||||
@Restricted(ProtectedExternally.class)
|
||||
protected abstract Computer createComputer();
|
||||
|
||||
/**
|
||||
|
|
|
@ -39,6 +39,7 @@ import javax.annotation.concurrent.GuardedBy;
|
|||
import java.util.concurrent.TimeUnit;
|
||||
import java.util.logging.Level;
|
||||
import java.util.logging.Logger;
|
||||
import javax.annotation.Nonnull;
|
||||
|
||||
/**
|
||||
* Controls when to take {@link Computer} offline, bring it back online, or even to destroy it.
|
||||
|
@ -57,7 +58,7 @@ public abstract class RetentionStrategy<T extends Computer> extends AbstractDesc
|
|||
* rechecked earlier or later that this!
|
||||
*/
|
||||
@GuardedBy("hudson.model.Queue.lock")
|
||||
public abstract long check(T c);
|
||||
public abstract long check(@Nonnull T c);
|
||||
|
||||
/**
|
||||
* This method is called to determine whether manual launching of the agent is allowed at this point in time.
|
||||
|
@ -92,9 +93,10 @@ public abstract class RetentionStrategy<T extends Computer> extends AbstractDesc
|
|||
* The default implementation of this method delegates to {@link #check(Computer)},
|
||||
* but this allows {@link RetentionStrategy} to distinguish the first time invocation from the rest.
|
||||
*
|
||||
* @param c Computer instance
|
||||
* @since 1.275
|
||||
*/
|
||||
public void start(final T c) {
|
||||
public void start(final @Nonnull T c) {
|
||||
Queue.withLock(new Runnable() {
|
||||
@Override
|
||||
public void run() {
|
||||
|
|
|
@ -3051,6 +3051,8 @@ public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve
|
|||
return getLabelAtom("master");
|
||||
}
|
||||
|
||||
@Override
|
||||
@Nonnull
|
||||
public Computer createComputer() {
|
||||
return new Hudson.MasterComputer();
|
||||
}
|
||||
|
|
Loading…
Reference in New Issue