[JENKINS-10944] [JENKINS-24519] Noting merge of #1513.

jenkinsci · Jan 9, 2015 · 9e333bc · kd5rsa · Aug 19, 2015 · jglick
2 parents 7ce5132 + 1a80973
commit 9e333bc
Show file tree

Hide file tree

Showing 5 changed files with 100 additions and 21 deletions.
diff --git a/changelog.html b/changelog.html
@@ -58,6 +58,10 @@
   <li class=bug>
     FutureImpl does not cancel its start future.
     (<a href="https://issues.jenkins-ci.org/browse/JENKINS-25514">issue 25514</a>)
+  <li class=bug>
+    Flyweight tasks were under some conditions actually being run on heavyweight executors.
+    (<a href="https://issues.jenkins-ci.org/browse/JENKINS-10944">issue 10944</a>)
+    (<a href="https://issues.jenkins-ci.org/browse/JENKINS-24519">issue 24519</a>)
   <li class=bug>
     Folder loading broken when child item loading throws exception.
     (<a href="https://issues.jenkins-ci.org/browse/JENKINS-22811">issue 22811</a>)

diff --git a/core/src/main/java/hudson/model/LoadBalancer.java b/core/src/main/java/hudson/model/LoadBalancer.java
@@ -140,7 +140,7 @@ protected LoadBalancer sanitize() {
         return new LoadBalancer() {
             @Override
             public Mapping map(Task task, MappingWorksheet worksheet) {
-                if (Queue.ifBlockedByHudsonShutdown(task)) {
+                if (Queue.isBlockedByShutdown(task)) {
                     // if we are quieting down, don't start anything new so that
                     // all executors will be eventually free.
                     return null;

diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java
@@ -106,7 +106,6 @@
 
 import jenkins.model.Jenkins;
 import jenkins.security.QueueItemAuthenticator;
-import jenkins.security.QueueItemAuthenticatorConfiguration;
 import jenkins.util.AtmostOneTaskExecutor;
 import org.acegisecurity.AccessDeniedException;
 import org.acegisecurity.Authentication;
@@ -1098,8 +1097,11 @@ public synchronized void maintain() {
             for (BlockedItem p : new ArrayList<BlockedItem>(blockedProjects.values())) {// copy as we'll mutate the list
                 if (!isBuildBlocked(p) && allowNewBuildableTask(p.task)) {
                     // ready to be executed
-                    p.leave(this);
-                    makeBuildable(new BuildableItem(p));
+                    Runnable r = makeBuildable(new BuildableItem(p));
+                    if (r != null) {
+                        p.leave(this);
+                        r.run();
+                    }
                 }
             }
         }
@@ -1115,7 +1117,12 @@ public synchronized void maintain() {
             Task p = top.task;
             if (!isBuildBlocked(top) && allowNewBuildableTask(p)) {
                 // ready to be executed immediately
-                makeBuildable(new BuildableItem(top));
+                Runnable r = makeBuildable(new BuildableItem(top));
+                if (r != null) {
+                    r.run();
+                } else {
+                    new BlockedItem(top).enter(this);
+                }
             } else {
                 // this can't be built now because another build is in progress
                 // set this project aside.
@@ -1164,10 +1171,14 @@ public synchronized void maintain() {
         }
     }
 
-    private void makeBuildable(BuildableItem p) {
-        if(Jenkins.FLYWEIGHT_SUPPORT && p.task instanceof FlyweightTask && !ifBlockedByHudsonShutdown(p.task)) {
-
-
+    /**
+     * Tries to make an item ready to build.
+     * @param p a proposed buildable item
+     * @return a thunk to actually prepare it (after leaving an earlier list), or null if it cannot be run now
+     */
+    private @CheckForNull Runnable makeBuildable(final BuildableItem p) {
+        if (p.task instanceof FlyweightTask) {
+            if (!isBlockedByShutdown(p.task)) {
             Jenkins h = Jenkins.getInstance();
             Map<Node,Integer> hashSource = new HashMap<Node, Integer>(h.getNodes().size());
 
@@ -1183,19 +1194,27 @@ private void makeBuildable(BuildableItem p) {
 
             Label lbl = p.getAssignedLabel();
             for (Node n : hash.list(p.task.getFullDisplayName())) {
-                Computer c = n.toComputer();
+                final Computer c = n.toComputer();
                 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));
-                makePending(p);
-                return;
+                return new Runnable() {
+                    @Override public void run() {
+                        c.startFlyWeightTask(new WorkUnitContext(p).createWorkUnit(p.task));
+                        makePending(p);
+                    }
+                };
+            }
             }
             // if the execution get here, it means we couldn't schedule it anywhere.
-            // so do the scheduling like other normal jobs.
+            return null;
+        } else { // regular heavyweight task
+            return new Runnable() {
+                @Override public void run() {
+                    p.enter(Queue.this);
+                }
+            };
         }
-
-        p.enter(this);
     }
 
 
@@ -1211,7 +1230,18 @@ private boolean makePending(BuildableItem p) {
         return pendings.add(p);
     }
 
+    /** @deprecated Use {@link #isBlockedByShutdown} instead. */
     public static boolean ifBlockedByHudsonShutdown(Task task) {
+        return isBlockedByShutdown(task);
+    }
+
+    /**
+     * Checks whether a task should not be scheduled because {@link Jenkins#isQuietingDown()}.
+     * @param task some queue task
+     * @return true if {@link Jenkins#isQuietingDown()} unless this is a {@link NonBlockingTask}
+     * @since 1.598
+     */
+    public static boolean isBlockedByShutdown(Task task) {
         return Jenkins.getInstance().isQuietingDown() && !(task instanceof NonBlockingTask);
     }
 
@@ -1235,7 +1265,7 @@ public interface FlyweightTask extends Task {}
     /**
      * Marks {@link Task}s that are not affected by the {@linkplain Jenkins#isQuietingDown()}  quieting down},
      * because these tasks keep other tasks executing.
-     *
+     * @see #isBlockedByShutdown
      * @since 1.336
      */
     public interface NonBlockingTask extends Task {}
@@ -1926,7 +1956,7 @@ public BuildableItem(NotWaitingItem ni) {
 
         public CauseOfBlockage getCauseOfBlockage() {
             Jenkins jenkins = Jenkins.getInstance();
-            if(ifBlockedByHudsonShutdown(task))
+            if(isBlockedByShutdown(task))
                 return CauseOfBlockage.fromMessage(Messages._Queue_HudsonIsAboutToShutDown());
 
             Label label = getAssignedLabel();

diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java
@@ -4183,9 +4183,9 @@ public static VersionNumber getVersion() {
     public static boolean PARALLEL_LOAD = Configuration.getBooleanConfigParameter("parallelLoad", true);
     public static boolean KILL_AFTER_LOAD = Configuration.getBooleanConfigParameter("killAfterLoad", false);
     /**
-     * Enabled by default as of 1.337. Will keep it for a while just in case we have some serious problems.
+     * @deprecated No longer used.
      */
-    public static boolean FLYWEIGHT_SUPPORT = Configuration.getBooleanConfigParameter("flyweightSupport", true);
+    public static boolean FLYWEIGHT_SUPPORT = true;
 
     /**
      * Tentative switch to activate the concurrent build behavior.

diff --git a/test/src/test/java/hudson/model/QueueTest.java b/test/src/test/java/hudson/model/QueueTest.java
@@ -346,6 +346,49 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
         assertEquals(1, runs.size());
         assertEquals("slave0", runs.get(0).getBuiltOnStr());
     }
+
+    @Issue("JENKINS-10944")
+    @Test public void flyweightTasksBlockedByShutdown() throws Exception {
+        r.jenkins.doQuietDown(true, 0);
+        AtomicInteger cnt = new AtomicInteger();
+        TestFlyweightTask task = new TestFlyweightTask(cnt, null);
+        assertTrue(Queue.isBlockedByShutdown(task));
+        r.jenkins.getQueue().schedule2(task, 0);
+        r.jenkins.getQueue().maintain();
+        r.jenkins.doCancelQuietDown();
+        assertFalse(Queue.isBlockedByShutdown(task));
+        r.waitUntilNoActivity();
+        assertEquals(1, cnt.get());
+        assert task.exec instanceof OneOffExecutor : task.exec;
+    }
+
+    @Issue("JENKINS-24519")
+    @Test public void flyweightTasksBlockedBySlave() throws Exception {
+        Label label = Label.get("myslave");
+        AtomicInteger cnt = new AtomicInteger();
+        TestFlyweightTask task = new TestFlyweightTask(cnt, label);
+        r.jenkins.getQueue().schedule2(task, 0);
+        r.jenkins.getQueue().maintain();
+        r.createSlave(label);
+        r.waitUntilNoActivity();
+        assertEquals(1, cnt.get());
+        assert task.exec instanceof OneOffExecutor : task.exec;
+    }
+
+    private static class TestFlyweightTask extends TestTask implements Queue.FlyweightTask {
+        Executor exec;
+        private final Label assignedLabel;
+        TestFlyweightTask(AtomicInteger cnt, Label assignedLabel) {
+            super(cnt);
+            this.assignedLabel = assignedLabel;
+        }
+        @Override protected void doRun() {
+            exec = Executor.currentExecutor();
+        }
+        @Override public Label getAssignedLabel() {
+            return assignedLabel;
+        }
+    }
 
     @Test public void taskEquality() throws Exception {
         AtomicInteger cnt = new AtomicInteger();
@@ -357,7 +400,7 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
         r.waitUntilNoActivity();
         assertEquals(1, cnt.get());
     }
-    private static final class TestTask extends AbstractQueueTask {
+    private static class TestTask extends AbstractQueueTask {
         private final AtomicInteger cnt;
         TestTask(AtomicInteger cnt) {
             this.cnt = cnt;
@@ -380,11 +423,13 @@ private static final class TestTask extends AbstractQueueTask {
         @Override public Node getLastBuiltOn() {return null;}
         @Override public long getEstimatedDuration() {return -1;}
         @Override public ResourceList getResourceList() {return new ResourceList();}
+        protected void doRun() {}
         @Override public Executable createExecutable() throws IOException {
             return new Executable() {
                 @Override public SubTask getParent() {return TestTask.this;}
                 @Override public long getEstimatedDuration() {return -1;}
                 @Override public void run() {
+                    doRun();
                     cnt.incrementAndGet();
                 }
             };