Permalink
Browse files

[JENKINS-43496] - Add handling of the null Node#createComputer() resu…

…lt. (#2922)

* [JENKINS-43496] - Add handling of the null Node#createComputer() result.

it is a follow-up to #2836 (comment)

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
  • Loading branch information...
oleg-nenashev committed Jul 1, 2017
1 parent e7cdd65 commit bcf55ecd7f8a22046c5cb3c4c50016d936e5460c
@@ -39,6 +39,7 @@
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 @@ private void updateComputer(Node n, Map<String,Computer> byNameMap, Set<Computer
} 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 @@ private void updateComputer(Node n, Map<String,Computer> byNameMap, Set<Computer
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 void run() {
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.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 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 final VirtualChannel getChannel() {
/**
* 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 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 @@
* 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 boolean isAcceptingTasks(T c) {
* 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 LabelAtom getSelfLabel() {
return getLabelAtom("master");
}
@Override
@Nonnull
public Computer createComputer() {
return new Hudson.MasterComputer();
}

0 comments on commit bcf55ec

Please sign in to comment.