Skip to content
Permalink
Browse files

Always adding Computer for master as a fallback

The original proposed fix for JENKINS-7291 creates a Computer object
transitively. This seems unwise as it violates the design of Computer
as stated in the javadoc, and for example we can end up creating two
Computers for the master.

I think a better fix is to create a Computer for the master all the
time, even if there's no executors configured. The discrimination in
Queue.makeBuildable would ensure that such phantom Computer is only used
as a last resort.

I've also tweaked executors.jelly a bit. I simplified it somewhat
based on the idea that "if there's only one computer to show, the
context is likely making it obvious".

(I must be missing the intricacy in the current code.)
  • Loading branch information
kohsuke committed Mar 25, 2013
1 parent 7e1806b commit 2c5b57fcc1f39ed39057254e802f4183db5aa0dc
@@ -30,6 +30,7 @@
import hudson.security.AccessControlled;
import hudson.slaves.ComputerListener;
import hudson.slaves.RetentionStrategy;
import jenkins.model.Jenkins;
import org.kohsuke.stapler.StaplerFallback;
import org.kohsuke.stapler.StaplerProxy;

@@ -116,7 +117,8 @@ private void updateComputer(Node n, Map<String,Computer> byNameMap, Set<Computer
if (c!=null) {
c.setNode(n); // reuse
} else {
if(n.getNumExecutors()>0) {
// 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());
if (!n.isHoldOffLaunchUntilSave() && automaticSlaveLaunch) {
RetentionStrategy retentionStrategy = c.getRetentionStrategy();
@@ -32,6 +32,7 @@
import hudson.console.AnnotatedLargeText;
import hudson.init.Initializer;
import hudson.model.Descriptor.FormException;
import hudson.model.Queue.FlyweightTask;
import hudson.model.labels.LabelAtom;
import hudson.model.queue.WorkUnit;
import hudson.node_monitors.NodeMonitor;
@@ -111,7 +112,9 @@
* This object is related to {@link Node} but they have some significant difference.
* {@link Computer} primarily works as a holder of {@link Executor}s, so
* if a {@link Node} is configured (probably temporarily) with 0 executors,
* you won't have a {@link Computer} object for it.
* you won't have a {@link Computer} object for it (except for the master node,
* which always get its {@link Computer} in case we have no static executors and
* we need to run a {@link FlyweightTask} - see JENKINS-7291 for more discussion.)
*
* Also, even if you remove a {@link Node}, it takes time for the corresponding
* {@link Computer} to be removed, if some builds are already in progress on that
@@ -817,7 +820,7 @@ public final long getDemandStartMilliseconds() {
* Note that if an executor dies, we'll leave it in {@link #executors} until
* the administrator yanks it out, so that we can see why it died.
*/
private boolean isAlive() {
protected boolean isAlive() {

This comment has been minimized.

Copy link
@jglick

jglick Mar 25, 2013

Member

@since

for (Executor e : executors)
if (e.isAlive())
return true;
@@ -1061,22 +1061,12 @@ public String hash(Node node) {
// Even if master is configured with zero executors, we may need to run a flyweight task like MatrixProject on it.
hash.add(h, Math.max(h.getNumExecutors()*100, 1));
for (Node n : h.getNodes())
hash.add(n, Math.max(n.getNumExecutors()*100, 1));
hash.add(n, n.getNumExecutors()*100);

Label lbl = p.getAssignedLabel();
for (Node n : hash.list(p.task.getFullDisplayName())) {
Computer c = n.toComputer();
if (c == null) {
if (n instanceof Jenkins) {
// If n.numExecutors==0 then n.toComputer()==null as well, so make a transient MasterComputer just for this task.
c = n.createComputer();
} else {
continue;
}
}
if (c.isOffline()) {
continue;
}
if (c==null || c.isOffline()) continue;
if (lbl!=null && !lbl.contains(n)) continue;
if (n.canTake(p) != null) continue;
c.startFlyWeightTask(new WorkUnitContext(p).createWorkUnit(p.task));
@@ -134,6 +134,7 @@ public SlaveComputer(Slave slave) {
super(slave);
this.log = new ReopenableRotatingFileOutputStream(getLogFile(),10);
this.taskListener = new StreamTaskListener(log);
assert slave.getNumExecutors()!=0 : "Computer created with 0 executors";
}

/**
@@ -66,6 +66,7 @@
import hudson.model.NoFingerprintMatch;
import hudson.model.OverallLoadStatistics;
import hudson.model.Project;
import hudson.model.Queue.FlyweightTask;
import hudson.model.RestartListener;
import hudson.model.RootAction;
import hudson.model.Slave;
@@ -3759,6 +3760,15 @@ public RetentionStrategy getRetentionStrategy() {
return RetentionStrategy.NOOP;
}

/**
* Will always keep this guy alive so that it can function as a fallback to
* execute {@link FlyweightTask}s. See JENKINS-7291.
*/
@Override
protected boolean isAlive() {
return true;
}

/**
* Report an error.
*/
@@ -105,30 +105,23 @@ THE SOFTWARE.
<l:pane width="3" id="executors"
title="&lt;a href='${rootURL}/computer/'>${%Build Executor Status}&lt;/a>">
<colgroup><col width="1*"/><col width="200*"/><col width="24"/></colgroup>

<tr>
<th class="pane">#</th>
<th class="pane" colspan="2">
<div style="margin-right:19px">
${%Status}
</div>
</th>
</tr>

<j:forEach var="c" items="${computers}">
<tr>
<j:choose>
<j:when test="${c.node==app or computers.size()==1}">
<th class="pane">#</th>
<th class="pane" colspan="2">
<div style="margin-right:19px">
<j:choose>
<j:when test="${empty(app.slaves) or computers.size()==1}">
${%Status}
</j:when>
<j:otherwise>
<local:computerCaption title="${%Master}" />
</j:otherwise>
</j:choose>
</div>
</th>
</j:when>
<j:otherwise>
<th class="pane" colspan="3">
<local:computerCaption title="${c.displayName}" />
</th>
</j:otherwise>
</j:choose>
<j:if test="${computers.size() gt 1 and (c.executors.size()!=0 or c.oneOffExecutors.size()!=0)}">
<th class="pane" colspan="3">
<local:computerCaption title="${c.displayName}" />
</th>
</j:if>
</tr>
<j:forEach var="e" items="${c.executors}" varStatus="eloop">
<local:executor name="${eloop.index+1}" url="executors/${eloop.index}" />

0 comments on commit 2c5b57f

Please sign in to comment.