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.)
jenkinsci · Mar 25, 2013 · 2c5b57f · jglick · Mar 25, 2013 · 2c5b57f
1 parent 7e1806b
commit 2c5b57f
Show file tree

Hide file tree

Showing 6 changed files with 36 additions and 37 deletions.
diff --git a/core/src/main/java/hudson/model/AbstractCIBase.java b/core/src/main/java/hudson/model/AbstractCIBase.java
@@ -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();

diff --git a/core/src/main/java/hudson/model/Computer.java b/core/src/main/java/hudson/model/Computer.java
@@ -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() {
         for (Executor e : executors)
             if (e.isAlive())
                 return true;

diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java
@@ -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));

diff --git a/core/src/main/java/hudson/slaves/SlaveComputer.java b/core/src/main/java/hudson/slaves/SlaveComputer.java
@@ -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";
     }
 
     /**

diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java
@@ -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.
          */

diff --git a/core/src/main/resources/lib/hudson/executors.jelly b/core/src/main/resources/lib/hudson/executors.jelly
@@ -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}" />