From 8cd99cac90b180a3e84d17c56c5fb1f11744f263 Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Mon, 31 Aug 2015 18:30:06 +0200 Subject: [PATCH 01/29] [JENKINS-30084] flyweight tasks gets added to the buildable list when no executor is available --- core/src/main/java/hudson/model/Queue.java | 103 +++++++++++++++++- .../hudson/model/queue/QueueListener.java | 4 + 2 files changed, 104 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index 12d307892728..8172273f5329 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -194,6 +194,17 @@ public class Queue extends ResourceController implements Saveable { */ private final ItemList blockedProjects = new ItemList(); + /** + * List of type {@link FlyWeightItem}. + * + *

+ * This consists of {@link Item}s which require one additional executor, + * which is a temporary executor, to manage the execution of its nested tasks. + * This applies to Matrix project. + */ + + private final ItemList flyWeightTasks = new ItemList(); + /** * {@link Task}s that can be built immediately * that are waiting for available {@link Executor}. @@ -1427,6 +1438,20 @@ public void maintain() { } } + //this is to solve OSS-192. We iterate on the list of flyweight tasks and execute them + List flyweightItems = new ArrayList<>(flyWeightTasks.values()); + for (FlyWeightItem f : flyweightItems) { + if (!isBuildBlocked(f)) { + // ready to be executed + Runnable r = makeBuildable(new BuildableItem(f)); + if (r != null) { + f.leave(this); + r.run(); + updateSnapshot(); + } + } + } + // waitingList -> buildable/blocked while (!waitingList.isEmpty()) { WaitingItem top = peek(); @@ -1442,7 +1467,8 @@ public void maintain() { if (r != null) { r.run(); } else { - new BlockedItem(top).enter(this); + new BuildableItem(top).enter(this); + new FlyWeightItem(top).enter(this); } } else { // this can't be built now because another build is in progress @@ -1460,6 +1486,9 @@ public void maintain() { // allocate buildable jobs to executors for (BuildableItem p : new ArrayList( buildables)) {// copy as we'll mutate the list in the loop + if (p.task instanceof FlyweightTask){ + continue; + } // one last check to make sure this build is not blocked. if (isBuildBlocked(p)) { p.leave(this); @@ -1538,12 +1567,15 @@ public void maintain() { Label lbl = p.getAssignedLabel(); for (Node n : hash.list(p.task.getFullDisplayName())) { final Computer c = n.toComputer(); - if (c==null || c.isOffline()) continue; - if (lbl!=null && !lbl.contains(n)) continue; + // if (c==null || c.isOffline()) continue; + // if (lbl!=null && !lbl.contains(n)) continue; + if (n.canTake(p) != null) continue; + if (c==null || c.isOffline()) continue; return new Runnable() { @Override public void run() { c.startFlyWeightTask(new WorkUnitContext(p).createWorkUnit(p.task)); + p.leave(Queue.this); makePending(p); } }; @@ -2316,6 +2348,71 @@ public CauseOfBlockage getCauseOfBlockage() { } } + /** + * {@link Item} in the {@link Queue#flyWeightTasks} stage. + */ + public final class FlyWeightItem extends NotWaitingItem { + + boolean buildable; + + public FlyWeightItem(WaitingItem wi) { + super(wi); + } + + public FlyWeightItem(NotWaitingItem ni) { + super(ni); + } + + public CauseOfBlockage getCauseOfBlockage() { + ResourceActivity r = getBlockingActivity(task); + if (r != null) { + if (r == task) // blocked by itself, meaning another build is in progress + return CauseOfBlockage.fromMessage(Messages._Queue_InProgress()); + return CauseOfBlockage.fromMessage(Messages._Queue_BlockedBy(r.getDisplayName())); + } + + for (QueueTaskDispatcher d : QueueTaskDispatcher.all()) { + CauseOfBlockage cause = d.canRun(this); + if (cause != null) + return cause; + } + + return task.getCauseOfBlockage(); + } + + /*package*/ void enter(Queue q) { + LOGGER.log(Level.FINE, "{0} is blocked", this); + flyWeightTasks.add(this); + buildable = true; + for (QueueListener ql : QueueListener.all()) { + try { + ql.onEnterFlyWeight(this); + } catch (Throwable e) { + // don't let this kill the queue + LOGGER.log(Level.WARNING, "QueueListener failed while processing "+this,e); + } + } + } + + /*package*/ boolean leave(Queue q) { + boolean r = flyWeightTasks.remove(this); + buildable = false; + if (r) { + LOGGER.log(Level.FINE, "{0} no longer blocked", this); + for (QueueListener ql : QueueListener.all()) { + try { + ql.onLeaveFlyWeight(this); + } catch (Throwable e) { + // don't let this kill the queue + LOGGER.log(Level.WARNING, "QueueListener failed while processing "+this,e); + } + } + } + return r; + } + } + + /** * {@link Item} in the {@link Queue#buildables} stage. */ diff --git a/core/src/main/java/hudson/model/queue/QueueListener.java b/core/src/main/java/hudson/model/queue/QueueListener.java index 527a9485b4dc..db21006e8285 100644 --- a/core/src/main/java/hudson/model/queue/QueueListener.java +++ b/core/src/main/java/hudson/model/queue/QueueListener.java @@ -9,6 +9,7 @@ import hudson.model.Queue.Item; import hudson.model.Queue.LeftItem; import hudson.model.Queue.WaitingItem; +import hudson.model.Queue.FlyWeightItem; import jenkins.model.Jenkins; import java.util.concurrent.Executor; @@ -29,6 +30,9 @@ * @since 1.520 */ public abstract class QueueListener implements ExtensionPoint { + + public void onEnterFlyWeight(FlyWeightItem fw) {} + public void onLeaveFlyWeight(FlyWeightItem fw) {} /** * When a task is submitted to the queue, it first gets to the waiting phase, * where it spends until the quiet period runs out and the item becomes executable. From cb6f2b2c713beb483800ab9585a775878a89ccb5 Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Mon, 31 Aug 2015 18:30:41 +0200 Subject: [PATCH 02/29] [JENKINS-30084] fix indentation --- core/src/main/java/hudson/model/Queue.java | 52 +++++++++++----------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index 8172273f5329..6113c84bde48 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -1551,35 +1551,35 @@ public void maintain() { private @CheckForNull Runnable makeBuildable(final BuildableItem p) { if (p.task instanceof FlyweightTask) { if (!isBlockedByShutdown(p.task)) { - Jenkins h = Jenkins.getInstance(); - Map hashSource = new HashMap(h.getNodes().size()); + Jenkins h = Jenkins.getInstance(); + Map hashSource = new HashMap(h.getNodes().size()); - // Even if master is configured with zero executors, we may need to run a flyweight task like MatrixProject on it. - hashSource.put(h, Math.max(h.getNumExecutors() * 100, 1)); + // Even if master is configured with zero executors, we may need to run a flyweight task like MatrixProject on it. + hashSource.put(h, Math.max(h.getNumExecutors() * 100, 1)); - for (Node n : h.getNodes()) { - hashSource.put(n, n.getNumExecutors() * 100); - } + for (Node n : h.getNodes()) { + hashSource.put(n, n.getNumExecutors() * 100); + } - ConsistentHash hash = new ConsistentHash(NODE_HASH); - hash.addAll(hashSource); - - Label lbl = p.getAssignedLabel(); - for (Node n : hash.list(p.task.getFullDisplayName())) { - final Computer c = n.toComputer(); - // if (c==null || c.isOffline()) continue; - // if (lbl!=null && !lbl.contains(n)) continue; - - if (n.canTake(p) != null) continue; - if (c==null || c.isOffline()) continue; - return new Runnable() { - @Override public void run() { - c.startFlyWeightTask(new WorkUnitContext(p).createWorkUnit(p.task)); - p.leave(Queue.this); - makePending(p); - } - }; - } + ConsistentHash hash = new ConsistentHash(NODE_HASH); + hash.addAll(hashSource); + + Label lbl = p.getAssignedLabel(); + for (Node n : hash.list(p.task.getFullDisplayName())) { + final Computer c = n.toComputer(); + // if (c==null || c.isOffline()) continue; + // if (lbl!=null && !lbl.contains(n)) continue; + + if (n.canTake(p) != null) continue; + if (c==null || c.isOffline()) continue; + return new Runnable() { + @Override public void run() { + c.startFlyWeightTask(new WorkUnitContext(p).createWorkUnit(p.task)); + p.leave(Queue.this); + makePending(p); + } + }; + } } // if the execution get here, it means we couldn't schedule it anywhere. return null; From ca4668f5a2820b76673aebfe283f14b0cfb5e72d Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Tue, 1 Sep 2015 08:54:16 +0200 Subject: [PATCH 03/29] [JENKINS-30084] updateSnapshot --- core/src/main/java/hudson/model/Queue.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index 6113c84bde48..b54ef78fe8b1 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -1469,6 +1469,7 @@ public void maintain() { } else { new BuildableItem(top).enter(this); new FlyWeightItem(top).enter(this); + updateSnapshot(); } } else { // this can't be built now because another build is in progress @@ -1574,9 +1575,10 @@ public void maintain() { if (c==null || c.isOffline()) continue; return new Runnable() { @Override public void run() { + //p.leave(Queue.this); this won t work + buildables.get(p.task).leave(Queue.this); c.startFlyWeightTask(new WorkUnitContext(p).createWorkUnit(p.task)); - p.leave(Queue.this); - makePending(p); + updateSnapshot(); } }; } From 505f492e29a0972cde8f50059f743a2d737f2047 Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Tue, 1 Sep 2015 11:15:13 +0200 Subject: [PATCH 04/29] [JENKINS-30084] check on null --- core/src/main/java/hudson/model/Queue.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index b54ef78fe8b1..2876444cf2d8 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -1576,7 +1576,9 @@ public void maintain() { return new Runnable() { @Override public void run() { //p.leave(Queue.this); this won t work - buildables.get(p.task).leave(Queue.this); + if(buildables.get(p.task)!=null){ + buildables.get(p.task).leave(Queue.this); + } c.startFlyWeightTask(new WorkUnitContext(p).createWorkUnit(p.task)); updateSnapshot(); } From a5a2c45d2e787e5456b27f0162b6253540f4ec2e Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Tue, 1 Sep 2015 11:17:22 +0200 Subject: [PATCH 05/29] [JENKINS-30084] added test case --- .../hudson/model/queue/WideExecutionTest.java | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/test/src/test/java/hudson/model/queue/WideExecutionTest.java b/test/src/test/java/hudson/model/queue/WideExecutionTest.java index 304b6ac38dcf..271613fc1eec 100644 --- a/test/src/test/java/hudson/model/queue/WideExecutionTest.java +++ b/test/src/test/java/hudson/model/queue/WideExecutionTest.java @@ -24,22 +24,30 @@ package hudson.model.queue; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import hudson.matrix.Axis; +import hudson.matrix.AxisList; +import hudson.matrix.MatrixProject; import hudson.model.AbstractBuild; import hudson.model.AbstractProject; import hudson.model.Executor; import hudson.model.FreeStyleBuild; import hudson.model.FreeStyleProject; +import hudson.model.Queue; import hudson.model.Queue.Executable; import hudson.model.Queue.Task; +import hudson.model.labels.LabelExpression; import org.junit.Rule; import org.junit.Test; +import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; import org.jvnet.hudson.test.TestExtension; import java.io.IOException; import java.util.Collection; import java.util.Collections; +import java.util.concurrent.ExecutionException; /** * @author Kohsuke Kawaguchi @@ -87,6 +95,33 @@ public String getDisplayName() { } } + @Issue("OSS-192") + @Test + /* + * this is to test that when the assigned executor is not available the flyweighttask is put into the buildable list, + * thus the node will be provisioned. + * when the flyweight task is not assigned to an offline executors the buildable list is empty. + * + */ + public void flyWeightTaskQueue () throws IOException { + MatrixProject project2 = j.createMatrixProject(); + project2.setAxes(new AxisList( + new Axis("axis", "a", "b") + )); + project2.scheduleBuild2(0); + Queue.getInstance().maintain(); + assertTrue(Queue.getInstance().getBuildableItems().isEmpty()); + MatrixProject project = j.createMatrixProject(); + project.setAxes(new AxisList( + new Axis("axis", "a", "b") + )); + project.setAssignedLabel(LabelExpression.get("aws-linux")); + project.scheduleBuild2(0); + Queue.getInstance().maintain(); + assertTrue(Queue.getInstance().getBuildableItems().get(0).task.equals(project)); + assertEquals(Queue.getInstance().getBuildableItems().get(0).getAssignedLabel().getExpression(), "aws-linux"); + } + @Test public void run() throws Exception { FreeStyleProject p = j.createFreeStyleProject(); From c923001025ecbf9d027d7a3d629e043bee72ee5a Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Tue, 1 Sep 2015 11:23:48 +0200 Subject: [PATCH 06/29] [JENKINS-30084] added comments to QueueListener --- core/src/main/java/hudson/model/queue/QueueListener.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/core/src/main/java/hudson/model/queue/QueueListener.java b/core/src/main/java/hudson/model/queue/QueueListener.java index db21006e8285..8b623be6e48c 100644 --- a/core/src/main/java/hudson/model/queue/QueueListener.java +++ b/core/src/main/java/hudson/model/queue/QueueListener.java @@ -31,7 +31,15 @@ */ public abstract class QueueListener implements ExtensionPoint { + /** + * When a flyWeightTask is submitted to the waiting queue, but it is restricted + * to run on an offline node, then it gets added to the flyWeightTasks queue. + */ public void onEnterFlyWeight(FlyWeightItem fw) {} + /** + * When the executor of the flyWeightTask becomes available, the tasks is removed + * from the list and executed. + */ public void onLeaveFlyWeight(FlyWeightItem fw) {} /** * When a task is submitted to the queue, it first gets to the waiting phase, From 750cdb64bf7115c225714eb19d9676817d0f1b23 Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Tue, 1 Sep 2015 12:19:57 +0200 Subject: [PATCH 07/29] [JENKINS-30084] updated references of Jenkins Issue --- core/src/main/java/hudson/model/Queue.java | 4 +--- test/src/test/java/hudson/model/queue/WideExecutionTest.java | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index 2876444cf2d8..21fe274e390a 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -1438,7 +1438,7 @@ public void maintain() { } } - //this is to solve OSS-192. We iterate on the list of flyweight tasks and execute them + //this is to solve JENKINS-30084. We iterate on the list of flyweight tasks and execute them List flyweightItems = new ArrayList<>(flyWeightTasks.values()); for (FlyWeightItem f : flyweightItems) { if (!isBuildBlocked(f)) { @@ -2357,8 +2357,6 @@ public CauseOfBlockage getCauseOfBlockage() { */ public final class FlyWeightItem extends NotWaitingItem { - boolean buildable; - public FlyWeightItem(WaitingItem wi) { super(wi); } diff --git a/test/src/test/java/hudson/model/queue/WideExecutionTest.java b/test/src/test/java/hudson/model/queue/WideExecutionTest.java index 271613fc1eec..29fbd1602716 100644 --- a/test/src/test/java/hudson/model/queue/WideExecutionTest.java +++ b/test/src/test/java/hudson/model/queue/WideExecutionTest.java @@ -95,7 +95,7 @@ public String getDisplayName() { } } - @Issue("OSS-192") + @Issue("JENKINS-30084") @Test /* * this is to test that when the assigned executor is not available the flyweighttask is put into the buildable list, From 88443b08c17c4d0e6609eb7b1f6f66146fc37e20 Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Tue, 1 Sep 2015 14:56:08 +0200 Subject: [PATCH 08/29] [JENKINS-30084] we do not need a new flyweight tasks list: lighter and cleaner approach --- core/src/main/java/hudson/model/Queue.java | 205 +++++------------- .../hudson/model/queue/QueueListener.java | 12 - 2 files changed, 59 insertions(+), 158 deletions(-) diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index 21fe274e390a..10ea1fb9d34e 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -194,17 +194,6 @@ public class Queue extends ResourceController implements Saveable { */ private final ItemList blockedProjects = new ItemList(); - /** - * List of type {@link FlyWeightItem}. - * - *

- * This consists of {@link Item}s which require one additional executor, - * which is a temporary executor, to manage the execution of its nested tasks. - * This applies to Matrix project. - */ - - private final ItemList flyWeightTasks = new ItemList(); - /** * {@link Task}s that can be built immediately * that are waiting for available {@link Executor}. @@ -1438,20 +1427,6 @@ public void maintain() { } } - //this is to solve JENKINS-30084. We iterate on the list of flyweight tasks and execute them - List flyweightItems = new ArrayList<>(flyWeightTasks.values()); - for (FlyWeightItem f : flyweightItems) { - if (!isBuildBlocked(f)) { - // ready to be executed - Runnable r = makeBuildable(new BuildableItem(f)); - if (r != null) { - f.leave(this); - r.run(); - updateSnapshot(); - } - } - } - // waitingList -> buildable/blocked while (!waitingList.isEmpty()) { WaitingItem top = peek(); @@ -1467,9 +1442,10 @@ public void maintain() { if (r != null) { r.run(); } else { + //this is to solve JENKINS-30084: the task has to be buildable to force + //the provisioning of nodes new BuildableItem(top).enter(this); - new FlyWeightItem(top).enter(this); - updateSnapshot(); + } } else { // this can't be built now because another build is in progress @@ -1487,57 +1463,64 @@ public void maintain() { // allocate buildable jobs to executors for (BuildableItem p : new ArrayList( buildables)) {// copy as we'll mutate the list in the loop - if (p.task instanceof FlyweightTask){ - continue; + if (p.task instanceof FlyweightTask) { + Runnable r = makeBuildable(new BuildableItem(p)); + if (r != null) { + p.leave(this); + r.run(); + updateSnapshot(); + } } - // one last check to make sure this build is not blocked. - if (isBuildBlocked(p)) { + else { + // one last check to make sure this build is not blocked. + if (isBuildBlocked(p)) { + p.leave(this); + new BlockedItem(p).enter(this); + LOGGER.log(Level.FINE, "Catching that {0} is blocked in the last minute", p); + // JENKINS-28926 we have moved an unblocked task into the blocked state, update snapshot + // so that other buildables which might have been blocked by this can see the state change + updateSnapshot(); + continue; + } + + List candidates = new ArrayList(parked.size()); + for (JobOffer j : parked.values()) + if (j.canTake(p)) + candidates.add(j); + + MappingWorksheet ws = new MappingWorksheet(p, candidates); + Mapping m = loadBalancer.map(p.task, ws); + if (m == null) { + // if we couldn't find the executor that fits, + // just leave it in the buildables list and + // check if we can execute other projects + LOGGER.log(Level.FINER, "Failed to map {0} to executors. candidates={1} parked={2}", + new Object[]{p, candidates, parked.values()}); + continue; + } + + // found a matching executor. use it. + WorkUnitContext wuc = new WorkUnitContext(p); + m.execute(wuc); + p.leave(this); - new BlockedItem(p).enter(this); - LOGGER.log(Level.FINE, "Catching that {0} is blocked in the last minute", p); - // JENKINS-28926 we have moved an unblocked task into the blocked state, update snapshot - // so that other buildables which might have been blocked by this can see the state change + if (!wuc.getWorkUnits().isEmpty()) + makePending(p); + else + LOGGER.log(Level.FINE, "BuildableItem {0} with empty work units!?", p); + + // Ensure that identification of blocked tasks is using the live state: JENKINS-27708 & JENKINS-27871 + // The creation of a snapshot itself should be relatively cheap given the expected rate of + // job execution. You probably would need 100's of jobs starting execution every iteration + // of maintain() before this could even start to become an issue and likely the calculation + // of isBuildBlocked(p) will become a bottleneck before updateSnapshot() will. Additionally + // since the snapshot itself only ever has at most one reference originating outside of the stack + // it should remain in the eden space and thus be cheap to GC. + // See https://issues.jenkins-ci.org/browse/JENKINS-27708?focusedCommentId=225819&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-225819 + // or https://issues.jenkins-ci.org/browse/JENKINS-27708?focusedCommentId=225906&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-225906 + // for alternative fixes of this issue. updateSnapshot(); - continue; } - - List candidates = new ArrayList(parked.size()); - for (JobOffer j : parked.values()) - if (j.canTake(p)) - candidates.add(j); - - MappingWorksheet ws = new MappingWorksheet(p, candidates); - Mapping m = loadBalancer.map(p.task, ws); - if (m == null) { - // if we couldn't find the executor that fits, - // just leave it in the buildables list and - // check if we can execute other projects - LOGGER.log(Level.FINER, "Failed to map {0} to executors. candidates={1} parked={2}", - new Object[]{p, candidates, parked.values()}); - continue; - } - - // found a matching executor. use it. - WorkUnitContext wuc = new WorkUnitContext(p); - m.execute(wuc); - - p.leave(this); - if (!wuc.getWorkUnits().isEmpty()) - makePending(p); - else - LOGGER.log(Level.FINE, "BuildableItem {0} with empty work units!?", p); - - // Ensure that identification of blocked tasks is using the live state: JENKINS-27708 & JENKINS-27871 - // The creation of a snapshot itself should be relatively cheap given the expected rate of - // job execution. You probably would need 100's of jobs starting execution every iteration - // of maintain() before this could even start to become an issue and likely the calculation - // of isBuildBlocked(p) will become a bottleneck before updateSnapshot() will. Additionally - // since the snapshot itself only ever has at most one reference originating outside of the stack - // it should remain in the eden space and thus be cheap to GC. - // See https://issues.jenkins-ci.org/browse/JENKINS-27708?focusedCommentId=225819&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-225819 - // or https://issues.jenkins-ci.org/browse/JENKINS-27708?focusedCommentId=225906&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-225906 - // for alternative fixes of this issue. - updateSnapshot(); } } finally { updateSnapshot(); } } finally { lock.unlock(); @@ -1565,22 +1548,15 @@ public void maintain() { ConsistentHash hash = new ConsistentHash(NODE_HASH); hash.addAll(hashSource); - Label lbl = p.getAssignedLabel(); for (Node n : hash.list(p.task.getFullDisplayName())) { final Computer c = n.toComputer(); - // if (c==null || c.isOffline()) continue; - // if (lbl!=null && !lbl.contains(n)) continue; - if (n.canTake(p) != null) continue; if (c==null || c.isOffline()) continue; return new Runnable() { @Override public void run() { - //p.leave(Queue.this); this won t work - if(buildables.get(p.task)!=null){ - buildables.get(p.task).leave(Queue.this); - } c.startFlyWeightTask(new WorkUnitContext(p).createWorkUnit(p.task)); - updateSnapshot(); + makePending(p); + } }; } @@ -2352,69 +2328,6 @@ public CauseOfBlockage getCauseOfBlockage() { } } - /** - * {@link Item} in the {@link Queue#flyWeightTasks} stage. - */ - public final class FlyWeightItem extends NotWaitingItem { - - public FlyWeightItem(WaitingItem wi) { - super(wi); - } - - public FlyWeightItem(NotWaitingItem ni) { - super(ni); - } - - public CauseOfBlockage getCauseOfBlockage() { - ResourceActivity r = getBlockingActivity(task); - if (r != null) { - if (r == task) // blocked by itself, meaning another build is in progress - return CauseOfBlockage.fromMessage(Messages._Queue_InProgress()); - return CauseOfBlockage.fromMessage(Messages._Queue_BlockedBy(r.getDisplayName())); - } - - for (QueueTaskDispatcher d : QueueTaskDispatcher.all()) { - CauseOfBlockage cause = d.canRun(this); - if (cause != null) - return cause; - } - - return task.getCauseOfBlockage(); - } - - /*package*/ void enter(Queue q) { - LOGGER.log(Level.FINE, "{0} is blocked", this); - flyWeightTasks.add(this); - buildable = true; - for (QueueListener ql : QueueListener.all()) { - try { - ql.onEnterFlyWeight(this); - } catch (Throwable e) { - // don't let this kill the queue - LOGGER.log(Level.WARNING, "QueueListener failed while processing "+this,e); - } - } - } - - /*package*/ boolean leave(Queue q) { - boolean r = flyWeightTasks.remove(this); - buildable = false; - if (r) { - LOGGER.log(Level.FINE, "{0} no longer blocked", this); - for (QueueListener ql : QueueListener.all()) { - try { - ql.onLeaveFlyWeight(this); - } catch (Throwable e) { - // don't let this kill the queue - LOGGER.log(Level.WARNING, "QueueListener failed while processing "+this,e); - } - } - } - return r; - } - } - - /** * {@link Item} in the {@link Queue#buildables} stage. */ diff --git a/core/src/main/java/hudson/model/queue/QueueListener.java b/core/src/main/java/hudson/model/queue/QueueListener.java index 8b623be6e48c..89116ab423c2 100644 --- a/core/src/main/java/hudson/model/queue/QueueListener.java +++ b/core/src/main/java/hudson/model/queue/QueueListener.java @@ -9,8 +9,6 @@ import hudson.model.Queue.Item; import hudson.model.Queue.LeftItem; import hudson.model.Queue.WaitingItem; -import hudson.model.Queue.FlyWeightItem; -import jenkins.model.Jenkins; import java.util.concurrent.Executor; @@ -31,16 +29,6 @@ */ public abstract class QueueListener implements ExtensionPoint { - /** - * When a flyWeightTask is submitted to the waiting queue, but it is restricted - * to run on an offline node, then it gets added to the flyWeightTasks queue. - */ - public void onEnterFlyWeight(FlyWeightItem fw) {} - /** - * When the executor of the flyWeightTask becomes available, the tasks is removed - * from the list and executed. - */ - public void onLeaveFlyWeight(FlyWeightItem fw) {} /** * When a task is submitted to the queue, it first gets to the waiting phase, * where it spends until the quiet period runs out and the item becomes executable. From 53f909620dff65616a364092d35198b7ca4a8b7f Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Tue, 1 Sep 2015 15:20:39 +0200 Subject: [PATCH 09/29] [JENKINS-30084] new location and implementation of the test --- .../src/test/java/hudson/model/QueueTest.java | 23 ++++++++++++++++ .../hudson/model/queue/WideExecutionTest.java | 27 ------------------- 2 files changed, 23 insertions(+), 27 deletions(-) diff --git a/test/src/test/java/hudson/model/QueueTest.java b/test/src/test/java/hudson/model/QueueTest.java index 6e566f712988..1c355b08e3ec 100644 --- a/test/src/test/java/hudson/model/QueueTest.java +++ b/test/src/test/java/hudson/model/QueueTest.java @@ -30,6 +30,7 @@ import com.gargoylesoftware.htmlunit.xml.XmlPage; import hudson.Launcher; import hudson.XmlFile; +import hudson.matrix.Axis; import hudson.matrix.AxisList; import hudson.matrix.LabelAxis; import hudson.matrix.MatrixBuild; @@ -41,6 +42,7 @@ import hudson.model.Queue.BlockedItem; import hudson.model.Queue.Executable; import hudson.model.Queue.WaitingItem; +import hudson.model.labels.LabelExpression; import hudson.model.queue.AbstractQueueTask; import hudson.model.queue.QueueTaskFuture; import hudson.model.queue.ScheduleResult; @@ -780,4 +782,25 @@ public void run() { "B finished at " + buildBEndTime + ", C started at " + buildC.getStartTimeInMillis(), buildC.getStartTimeInMillis() >= buildBEndTime); } + + @Issue("JENKINS-30084") + @Test + /* + * this is to test that when the assigned executor is not available the flyweighttask is put into the buildable list, + * thus the node will be provisioned. + * when the flyweight task is not assigned to an offline executors the buildable list is empty. + * + */ + public void flyWeightTaskQueue () throws Exception { + MatrixProject project = r.createMatrixProject(); + project.setAxes(new AxisList( + new Axis("axis", "a", "b") + )); + DummyCloudImpl dummyCloud = new DummyCloudImpl(r, 0); + dummyCloud.label = LabelExpression.get("aws-linux-dummy"); + r.getInstance().clouds.add(dummyCloud); + project.setAssignedLabel(LabelExpression.get("aws-linux-dummy")); + r.assertBuildStatusSuccess(project.scheduleBuild2(0)); + } + } diff --git a/test/src/test/java/hudson/model/queue/WideExecutionTest.java b/test/src/test/java/hudson/model/queue/WideExecutionTest.java index 29fbd1602716..74e7644d9f53 100644 --- a/test/src/test/java/hudson/model/queue/WideExecutionTest.java +++ b/test/src/test/java/hudson/model/queue/WideExecutionTest.java @@ -95,33 +95,6 @@ public String getDisplayName() { } } - @Issue("JENKINS-30084") - @Test - /* - * this is to test that when the assigned executor is not available the flyweighttask is put into the buildable list, - * thus the node will be provisioned. - * when the flyweight task is not assigned to an offline executors the buildable list is empty. - * - */ - public void flyWeightTaskQueue () throws IOException { - MatrixProject project2 = j.createMatrixProject(); - project2.setAxes(new AxisList( - new Axis("axis", "a", "b") - )); - project2.scheduleBuild2(0); - Queue.getInstance().maintain(); - assertTrue(Queue.getInstance().getBuildableItems().isEmpty()); - MatrixProject project = j.createMatrixProject(); - project.setAxes(new AxisList( - new Axis("axis", "a", "b") - )); - project.setAssignedLabel(LabelExpression.get("aws-linux")); - project.scheduleBuild2(0); - Queue.getInstance().maintain(); - assertTrue(Queue.getInstance().getBuildableItems().get(0).task.equals(project)); - assertEquals(Queue.getInstance().getBuildableItems().get(0).getAssignedLabel().getExpression(), "aws-linux"); - } - @Test public void run() throws Exception { FreeStyleProject p = j.createFreeStyleProject(); From 9a24957247ea1a403ba4a108d005e421390382b9 Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Tue, 1 Sep 2015 15:23:23 +0200 Subject: [PATCH 10/29] [JENKINS-30084] new comment for test --- test/src/test/java/hudson/model/QueueTest.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/src/test/java/hudson/model/QueueTest.java b/test/src/test/java/hudson/model/QueueTest.java index 1c355b08e3ec..5cdb0017d0dd 100644 --- a/test/src/test/java/hudson/model/QueueTest.java +++ b/test/src/test/java/hudson/model/QueueTest.java @@ -786,10 +786,8 @@ public void run() { @Issue("JENKINS-30084") @Test /* - * this is to test that when the assigned executor is not available the flyweighttask is put into the buildable list, - * thus the node will be provisioned. - * when the flyweight task is not assigned to an offline executors the buildable list is empty. - * + * When a flyweight task is restricted to run on a specific node, the node will be provisioned + * and the flyweight task will be executed. */ public void flyWeightTaskQueue () throws Exception { MatrixProject project = r.createMatrixProject(); From 50ae0f4da9d7445a6d9da11dd5615ef4c1d6c315 Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Tue, 1 Sep 2015 15:47:40 +0200 Subject: [PATCH 11/29] [JENKINS-30084] cleanup --- .../test/java/hudson/model/queue/WideExecutionTest.java | 9 --------- 1 file changed, 9 deletions(-) diff --git a/test/src/test/java/hudson/model/queue/WideExecutionTest.java b/test/src/test/java/hudson/model/queue/WideExecutionTest.java index 74e7644d9f53..43cb0c09cea5 100644 --- a/test/src/test/java/hudson/model/queue/WideExecutionTest.java +++ b/test/src/test/java/hudson/model/queue/WideExecutionTest.java @@ -24,30 +24,21 @@ package hudson.model.queue; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; - -import hudson.matrix.Axis; -import hudson.matrix.AxisList; -import hudson.matrix.MatrixProject; import hudson.model.AbstractBuild; import hudson.model.AbstractProject; import hudson.model.Executor; import hudson.model.FreeStyleBuild; import hudson.model.FreeStyleProject; -import hudson.model.Queue; import hudson.model.Queue.Executable; import hudson.model.Queue.Task; -import hudson.model.labels.LabelExpression; import org.junit.Rule; import org.junit.Test; -import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; import org.jvnet.hudson.test.TestExtension; import java.io.IOException; import java.util.Collection; import java.util.Collections; -import java.util.concurrent.ExecutionException; /** * @author Kohsuke Kawaguchi From 1d731693fbd5b2806bdac30fd4a9e1c00abd31ab Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Tue, 1 Sep 2015 15:49:33 +0200 Subject: [PATCH 12/29] [JENKINS-30084] cleanup --- core/src/main/java/hudson/model/queue/QueueListener.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/model/queue/QueueListener.java b/core/src/main/java/hudson/model/queue/QueueListener.java index 89116ab423c2..527a9485b4dc 100644 --- a/core/src/main/java/hudson/model/queue/QueueListener.java +++ b/core/src/main/java/hudson/model/queue/QueueListener.java @@ -9,6 +9,7 @@ import hudson.model.Queue.Item; import hudson.model.Queue.LeftItem; import hudson.model.Queue.WaitingItem; +import jenkins.model.Jenkins; import java.util.concurrent.Executor; @@ -28,7 +29,6 @@ * @since 1.520 */ public abstract class QueueListener implements ExtensionPoint { - /** * When a task is submitted to the queue, it first gets to the waiting phase, * where it spends until the quiet period runs out and the item becomes executable. From cfef72b1b2487ba86d07f66e30dc98651d666a9e Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Tue, 1 Sep 2015 15:53:54 +0200 Subject: [PATCH 13/29] [JENKINS-30084] cleanup --- test/src/test/java/hudson/model/queue/WideExecutionTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/test/src/test/java/hudson/model/queue/WideExecutionTest.java b/test/src/test/java/hudson/model/queue/WideExecutionTest.java index 43cb0c09cea5..304b6ac38dcf 100644 --- a/test/src/test/java/hudson/model/queue/WideExecutionTest.java +++ b/test/src/test/java/hudson/model/queue/WideExecutionTest.java @@ -24,6 +24,7 @@ package hudson.model.queue; import static org.junit.Assert.assertEquals; + import hudson.model.AbstractBuild; import hudson.model.AbstractProject; import hudson.model.Executor; From a19c7936bf00a43d2c8707fca8b9dd32341d550e Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Tue, 1 Sep 2015 16:17:49 +0200 Subject: [PATCH 14/29] [JENKINS-30084] cleanup --- test/src/test/java/hudson/model/QueueTest.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/src/test/java/hudson/model/QueueTest.java b/test/src/test/java/hudson/model/QueueTest.java index 5cdb0017d0dd..7bc83184e7e2 100644 --- a/test/src/test/java/hudson/model/QueueTest.java +++ b/test/src/test/java/hudson/model/QueueTest.java @@ -794,10 +794,11 @@ public void flyWeightTaskQueue () throws Exception { project.setAxes(new AxisList( new Axis("axis", "a", "b") )); + Label label = LabelExpression.get("aws-linux-dummy"); DummyCloudImpl dummyCloud = new DummyCloudImpl(r, 0); - dummyCloud.label = LabelExpression.get("aws-linux-dummy"); - r.getInstance().clouds.add(dummyCloud); - project.setAssignedLabel(LabelExpression.get("aws-linux-dummy")); + dummyCloud.label = label; + r.jenkins.clouds.add(dummyCloud); + project.setAssignedLabel(label); r.assertBuildStatusSuccess(project.scheduleBuild2(0)); } From 2934f4502001fa2d2182a4482686e547f44890c4 Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Tue, 1 Sep 2015 17:40:41 +0200 Subject: [PATCH 15/29] [JENKINS-30084] a flyweight task can be blocked by shutdown and if so should go in the blocked list --- core/src/main/java/hudson/model/Queue.java | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index 10ea1fb9d34e..8b73ed21a975 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -1442,10 +1442,7 @@ public void maintain() { if (r != null) { r.run(); } else { - //this is to solve JENKINS-30084: the task has to be buildable to force - //the provisioning of nodes - new BuildableItem(top).enter(this); - + new BlockedItem(top).enter(this); } } else { // this can't be built now because another build is in progress @@ -1560,8 +1557,17 @@ public void maintain() { } }; } + //this is to solve JENKINS-30084: the task has to be buildable to force + //the provisioning of nodes + //if the execution gets here, it means the task could not be scheduled since the node + //the task is supposed to run on is offline + return new Runnable() { + @Override public void run() { + p.enter(Queue.this); + } + }; } - // if the execution get here, it means we couldn't schedule it anywhere. + // if the execution gets here, it means the task is blocked by shutdown. return null; } else { // regular heavyweight task return new Runnable() { From e1c197c08feda0cfa0118df100ef36a118d4a15a Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Tue, 1 Sep 2015 18:13:15 +0200 Subject: [PATCH 16/29] [JENKINS-30084] a flyweight task can be blocked by shutdown and if so should go in the blocked list --- core/src/main/java/hudson/model/Queue.java | 58 ++++++++++++---------- 1 file changed, 33 insertions(+), 25 deletions(-) diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index 8b73ed21a975..0af8cbbdb49e 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -1461,7 +1461,7 @@ public void maintain() { for (BuildableItem p : new ArrayList( buildables)) {// copy as we'll mutate the list in the loop if (p.task instanceof FlyweightTask) { - Runnable r = makeBuildable(new BuildableItem(p)); + Runnable r = buildOnTemporaryNode(new BuildableItem(p)); if (r != null) { p.leave(this); r.run(); @@ -1532,35 +1532,15 @@ public void maintain() { private @CheckForNull Runnable makeBuildable(final BuildableItem p) { if (p.task instanceof FlyweightTask) { if (!isBlockedByShutdown(p.task)) { - Jenkins h = Jenkins.getInstance(); - Map hashSource = new HashMap(h.getNodes().size()); - // Even if master is configured with zero executors, we may need to run a flyweight task like MatrixProject on it. - hashSource.put(h, Math.max(h.getNumExecutors() * 100, 1)); - - for (Node n : h.getNodes()) { - hashSource.put(n, n.getNumExecutors() * 100); - } - - ConsistentHash hash = new ConsistentHash(NODE_HASH); - hash.addAll(hashSource); - - for (Node n : hash.list(p.task.getFullDisplayName())) { - final Computer c = n.toComputer(); - if (n.canTake(p) != null) continue; - if (c==null || c.isOffline()) continue; - return new Runnable() { - @Override public void run() { - c.startFlyWeightTask(new WorkUnitContext(p).createWorkUnit(p.task)); - makePending(p); - - } - }; - } + Runnable runnable = buildOnTemporaryNode(p); //this is to solve JENKINS-30084: the task has to be buildable to force //the provisioning of nodes //if the execution gets here, it means the task could not be scheduled since the node //the task is supposed to run on is offline + if(runnable!=null){ + return runnable; + } return new Runnable() { @Override public void run() { p.enter(Queue.this); @@ -1578,6 +1558,34 @@ public void maintain() { } } + private Runnable buildOnTemporaryNode(final BuildableItem p){ + Jenkins h = Jenkins.getInstance(); + Map hashSource = new HashMap(h.getNodes().size()); + + // Even if master is configured with zero executors, we may need to run a flyweight task like MatrixProject on it. + hashSource.put(h, Math.max(h.getNumExecutors() * 100, 1)); + + for (Node n : h.getNodes()) { + hashSource.put(n, n.getNumExecutors() * 100); + } + + ConsistentHash hash = new ConsistentHash(NODE_HASH); + hash.addAll(hashSource); + + for (Node n : hash.list(p.task.getFullDisplayName())) { + final Computer c = n.toComputer(); + if (n.canTake(p) != null) continue; + if (c==null || c.isOffline()) continue; + return new Runnable() { + @Override public void run() { + c.startFlyWeightTask(new WorkUnitContext(p).createWorkUnit(p.task)); + makePending(p); + + } + }; + } + return null; + } private static Hash NODE_HASH = new Hash() { public String hash(Node node) { From fced6f987e79e2e0192a5e90e28d96c8c8991167 Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Tue, 1 Sep 2015 18:18:57 +0200 Subject: [PATCH 17/29] [JENKINS-30084] more comments --- core/src/main/java/hudson/model/Queue.java | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index 0af8cbbdb49e..bc59563c6ef8 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -1534,20 +1534,22 @@ public void maintain() { if (!isBlockedByShutdown(p.task)) { Runnable runnable = buildOnTemporaryNode(p); - //this is to solve JENKINS-30084: the task has to be buildable to force - //the provisioning of nodes - //if the execution gets here, it means the task could not be scheduled since the node - //the task is supposed to run on is offline + if(runnable!=null){ return runnable; } + + //this is to solve JENKINS-30084: the task has to be buildable to force the provisioning of nodes. + //if the execution gets here, it means the task could not be scheduled since the node + //the task is supposed to run on is offline or not available. return new Runnable() { @Override public void run() { + //the flyweighttask enters the buildables queue and will ask Jenkins to provision a node p.enter(Queue.this); } }; } - // if the execution gets here, it means the task is blocked by shutdown. + // if the execution gets here, it means the task is blocked by shutdown and null is returned. return null; } else { // regular heavyweight task return new Runnable() { From 319382919c3cb67d0ced318b8b8aa8ee3e1b205d Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Thu, 3 Sep 2015 15:07:49 +0200 Subject: [PATCH 18/29] [JENKINS-30084] method renamed and added check on field type --- core/src/main/java/hudson/model/Queue.java | 53 +++++++++++++--------- 1 file changed, 31 insertions(+), 22 deletions(-) diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index bc59563c6ef8..50f3bd386caa 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -1461,7 +1461,7 @@ public void maintain() { for (BuildableItem p : new ArrayList( buildables)) {// copy as we'll mutate the list in the loop if (p.task instanceof FlyweightTask) { - Runnable r = buildOnTemporaryNode(new BuildableItem(p)); + Runnable r = makeFlyWeightTaskBuildable(new BuildableItem(p)); if (r != null) { p.leave(this); r.run(); @@ -1533,7 +1533,7 @@ public void maintain() { if (p.task instanceof FlyweightTask) { if (!isBlockedByShutdown(p.task)) { - Runnable runnable = buildOnTemporaryNode(p); + Runnable runnable = makeFlyWeightTaskBuildable(p); if(runnable!=null){ return runnable; @@ -1560,31 +1560,40 @@ public void maintain() { } } - private Runnable buildOnTemporaryNode(final BuildableItem p){ - Jenkins h = Jenkins.getInstance(); - Map hashSource = new HashMap(h.getNodes().size()); + /** + * This method checks if the flyweight task can be run on any of the available executors + * @param p - the flyweight task to be scheduled + * @return a Runnable if there is an executor that can take the task, null otherwise + */ + private Runnable makeFlyWeightTaskBuildable(final BuildableItem p){ + //we double check if this is a flyweight task + if (p.task instanceof FlyweightTask) { + Jenkins h = Jenkins.getInstance(); + Map hashSource = new HashMap(h.getNodes().size()); - // Even if master is configured with zero executors, we may need to run a flyweight task like MatrixProject on it. - hashSource.put(h, Math.max(h.getNumExecutors() * 100, 1)); + // Even if master is configured with zero executors, we may need to run a flyweight task like MatrixProject on it. + hashSource.put(h, Math.max(h.getNumExecutors() * 100, 1)); - for (Node n : h.getNodes()) { - hashSource.put(n, n.getNumExecutors() * 100); - } + for (Node n : h.getNodes()) { + hashSource.put(n, n.getNumExecutors() * 100); + } - ConsistentHash hash = new ConsistentHash(NODE_HASH); - hash.addAll(hashSource); + ConsistentHash hash = new ConsistentHash(NODE_HASH); + hash.addAll(hashSource); - for (Node n : hash.list(p.task.getFullDisplayName())) { - final Computer c = n.toComputer(); - if (n.canTake(p) != null) continue; - if (c==null || c.isOffline()) continue; - return new Runnable() { - @Override public void run() { - c.startFlyWeightTask(new WorkUnitContext(p).createWorkUnit(p.task)); - makePending(p); + for (Node n : hash.list(p.task.getFullDisplayName())) { + final Computer c = n.toComputer(); + if (n.canTake(p) != null) continue; + if (c == null || c.isOffline()) continue; + return new Runnable() { + @Override + public void run() { + c.startFlyWeightTask(new WorkUnitContext(p).createWorkUnit(p.task)); + makePending(p); - } - }; + } + }; + } } return null; } From e1904e02d45bf2407d6fb4513a5f37fed9a13268 Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Mon, 7 Sep 2015 09:36:55 +0200 Subject: [PATCH 19/29] [JENKINS-30084] some stylish changes --- core/src/main/java/hudson/model/Queue.java | 11 +++++++---- test/src/test/java/hudson/model/QueueTest.java | 2 +- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index 50f3bd386caa..d5492a4c1a89 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -1467,8 +1467,7 @@ public void maintain() { r.run(); updateSnapshot(); } - } - else { + } else { // one last check to make sure this build is not blocked. if (isBuildBlocked(p)) { p.leave(this); @@ -1583,8 +1582,12 @@ private Runnable makeFlyWeightTaskBuildable(final BuildableItem p){ for (Node n : hash.list(p.task.getFullDisplayName())) { final Computer c = n.toComputer(); - if (n.canTake(p) != null) continue; - if (c == null || c.isOffline()) continue; + if (n.canTake(p) != null) { + continue; + } + if (c == null || c.isOffline()) { + continue; + } return new Runnable() { @Override public void run() { diff --git a/test/src/test/java/hudson/model/QueueTest.java b/test/src/test/java/hudson/model/QueueTest.java index 7bc83184e7e2..fd625895e5ae 100644 --- a/test/src/test/java/hudson/model/QueueTest.java +++ b/test/src/test/java/hudson/model/QueueTest.java @@ -789,7 +789,7 @@ public void run() { * When a flyweight task is restricted to run on a specific node, the node will be provisioned * and the flyweight task will be executed. */ - public void flyWeightTaskQueue () throws Exception { + public void shouldRunFlyweightTaskOnProvisionedNodeWhenNodeRestricted() throws Exception { MatrixProject project = r.createMatrixProject(); project.setAxes(new AxisList( new Axis("axis", "a", "b") From eb52333df6a78d3cd79898edd7599753ba11d5bb Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Mon, 7 Sep 2015 10:02:37 +0200 Subject: [PATCH 20/29] [JENKINS-30084] extract inline class --- core/src/main/java/hudson/model/Queue.java | 31 +++++++++++++--------- 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index d5492a4c1a89..2d69ed9d11cc 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -1541,21 +1541,14 @@ public void maintain() { //this is to solve JENKINS-30084: the task has to be buildable to force the provisioning of nodes. //if the execution gets here, it means the task could not be scheduled since the node //the task is supposed to run on is offline or not available. - return new Runnable() { - @Override public void run() { - //the flyweighttask enters the buildables queue and will ask Jenkins to provision a node - p.enter(Queue.this); - } - }; + //Thus, the flyweighttask enters the buildables queue and will ask Jenkins to provision a node + return new BuildableRunnable(p); } // if the execution gets here, it means the task is blocked by shutdown and null is returned. return null; - } else { // regular heavyweight task - return new Runnable() { - @Override public void run() { - p.enter(Queue.this); - } - }; + } else { + // regular heavyweight task + return new BuildableRunnable(p); } } @@ -2712,6 +2705,20 @@ public void run() { } } + private class BuildableRunnable implements Runnable { + private final BuildableItem buildableItem; + + private BuildableRunnable(BuildableItem p) { + this.buildableItem = p; + } + + @Override + public void run() { + //the flyweighttask enters the buildables queue and will ask Jenkins to provision a node + buildableItem.enter(Queue.this); + } + } + private static class LockedJUCCallable implements java.util.concurrent.Callable { private final java.util.concurrent.Callable delegate; From 65d1fe5b1d17650f9f5cad97e28b5528d616a468 Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Mon, 14 Sep 2015 15:19:58 +0200 Subject: [PATCH 21/29] [JENKINS-30084] some more polish --- core/src/main/java/hudson/model/Queue.java | 8 +++++--- test/src/test/java/hudson/model/QueueTest.java | 1 + 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index 2d69ed9d11cc..625bb9d04a37 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -1500,8 +1500,9 @@ public void maintain() { m.execute(wuc); p.leave(this); - if (!wuc.getWorkUnits().isEmpty()) + if (!wuc.getWorkUnits().isEmpty()) { makePending(p); + } else LOGGER.log(Level.FINE, "BuildableItem {0} with empty work units!?", p); @@ -1557,6 +1558,7 @@ public void maintain() { * @param p - the flyweight task to be scheduled * @return a Runnable if there is an executor that can take the task, null otherwise */ + @CheckForNull private Runnable makeFlyWeightTaskBuildable(final BuildableItem p){ //we double check if this is a flyweight task if (p.task instanceof FlyweightTask) { @@ -1575,10 +1577,10 @@ private Runnable makeFlyWeightTaskBuildable(final BuildableItem p){ for (Node n : hash.list(p.task.getFullDisplayName())) { final Computer c = n.toComputer(); - if (n.canTake(p) != null) { + if (c == null || c.isOffline()) { continue; } - if (c == null || c.isOffline()) { + if (n.canTake(p) != null) { continue; } return new Runnable() { diff --git a/test/src/test/java/hudson/model/QueueTest.java b/test/src/test/java/hudson/model/QueueTest.java index fd625895e5ae..30911f1bad85 100644 --- a/test/src/test/java/hudson/model/QueueTest.java +++ b/test/src/test/java/hudson/model/QueueTest.java @@ -800,6 +800,7 @@ public void shouldRunFlyweightTaskOnProvisionedNodeWhenNodeRestricted() throws E r.jenkins.clouds.add(dummyCloud); project.setAssignedLabel(label); r.assertBuildStatusSuccess(project.scheduleBuild2(0)); + assertEquals("aws-linux-dummy", project.getBuilds().getLastBuild().getBuiltOn().getLabelString()); } } From 61a1f6c1fae28ca37e4352eb4940bed567b8e14a Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Thu, 24 Sep 2015 12:40:08 +0200 Subject: [PATCH 22/29] [JENKINS-30084] fix regression when flyweight task is blocked by upstream/downstream project --- core/src/main/java/hudson/model/Queue.java | 91 +++++++++++----------- 1 file changed, 46 insertions(+), 45 deletions(-) diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index 625bb9d04a37..1dbc29305c19 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -1460,14 +1460,6 @@ public void maintain() { // allocate buildable jobs to executors for (BuildableItem p : new ArrayList( buildables)) {// copy as we'll mutate the list in the loop - if (p.task instanceof FlyweightTask) { - Runnable r = makeFlyWeightTaskBuildable(new BuildableItem(p)); - if (r != null) { - p.leave(this); - r.run(); - updateSnapshot(); - } - } else { // one last check to make sure this build is not blocked. if (isBuildBlocked(p)) { p.leave(this); @@ -1479,44 +1471,53 @@ public void maintain() { continue; } - List candidates = new ArrayList(parked.size()); - for (JobOffer j : parked.values()) - if (j.canTake(p)) - candidates.add(j); - - MappingWorksheet ws = new MappingWorksheet(p, candidates); - Mapping m = loadBalancer.map(p.task, ws); - if (m == null) { - // if we couldn't find the executor that fits, - // just leave it in the buildables list and - // check if we can execute other projects - LOGGER.log(Level.FINER, "Failed to map {0} to executors. candidates={1} parked={2}", - new Object[]{p, candidates, parked.values()}); - continue; - } + if (p.task instanceof FlyweightTask) { + Runnable r = makeFlyWeightTaskBuildable(new BuildableItem(p)); + if (r != null) { + p.leave(this); + r.run(); + updateSnapshot(); + } + } else { - // found a matching executor. use it. - WorkUnitContext wuc = new WorkUnitContext(p); - m.execute(wuc); + List candidates = new ArrayList(parked.size()); + for (JobOffer j : parked.values()) + if (j.canTake(p)) + candidates.add(j); + + MappingWorksheet ws = new MappingWorksheet(p, candidates); + Mapping m = loadBalancer.map(p.task, ws); + if (m == null) { + // if we couldn't find the executor that fits, + // just leave it in the buildables list and + // check if we can execute other projects + LOGGER.log(Level.FINER, "Failed to map {0} to executors. candidates={1} parked={2}", + new Object[]{p, candidates, parked.values()}); + continue; + } - p.leave(this); - if (!wuc.getWorkUnits().isEmpty()) { - makePending(p); - } - else - LOGGER.log(Level.FINE, "BuildableItem {0} with empty work units!?", p); - - // Ensure that identification of blocked tasks is using the live state: JENKINS-27708 & JENKINS-27871 - // The creation of a snapshot itself should be relatively cheap given the expected rate of - // job execution. You probably would need 100's of jobs starting execution every iteration - // of maintain() before this could even start to become an issue and likely the calculation - // of isBuildBlocked(p) will become a bottleneck before updateSnapshot() will. Additionally - // since the snapshot itself only ever has at most one reference originating outside of the stack - // it should remain in the eden space and thus be cheap to GC. - // See https://issues.jenkins-ci.org/browse/JENKINS-27708?focusedCommentId=225819&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-225819 - // or https://issues.jenkins-ci.org/browse/JENKINS-27708?focusedCommentId=225906&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-225906 - // for alternative fixes of this issue. - updateSnapshot(); + // found a matching executor. use it. + WorkUnitContext wuc = new WorkUnitContext(p); + m.execute(wuc); + + p.leave(this); + if (!wuc.getWorkUnits().isEmpty()) { + makePending(p); + } + else + LOGGER.log(Level.FINE, "BuildableItem {0} with empty work units!?", p); + + // Ensure that identification of blocked tasks is using the live state: JENKINS-27708 & JENKINS-27871 + // The creation of a snapshot itself should be relatively cheap given the expected rate of + // job execution. You probably would need 100's of jobs starting execution every iteration + // of maintain() before this could even start to become an issue and likely the calculation + // of isBuildBlocked(p) will become a bottleneck before updateSnapshot() will. Additionally + // since the snapshot itself only ever has at most one reference originating outside of the stack + // it should remain in the eden space and thus be cheap to GC. + // See https://issues.jenkins-ci.org/browse/JENKINS-27708?focusedCommentId=225819&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-225819 + // or https://issues.jenkins-ci.org/browse/JENKINS-27708?focusedCommentId=225906&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-225906 + // for alternative fixes of this issue. + updateSnapshot(); } } } finally { updateSnapshot(); } } finally { @@ -1535,7 +1536,7 @@ public void maintain() { Runnable runnable = makeFlyWeightTaskBuildable(p); - if(runnable!=null){ + if(runnable != null){ return runnable; } From e33f3f62db662487707f24d5b2356fac446166a9 Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Fri, 25 Sep 2015 11:38:37 +0200 Subject: [PATCH 23/29] [JENKINS-30084] test added to make sure a flyweight task can be blocked at last minute --- .../src/test/java/hudson/model/QueueTest.java | 75 +++++++++++++++++-- .../java/hudson/slaves/DummyCloudImpl.java | 21 ++++++ 2 files changed, 90 insertions(+), 6 deletions(-) diff --git a/test/src/test/java/hudson/model/QueueTest.java b/test/src/test/java/hudson/model/QueueTest.java index 30911f1bad85..0eeec5308623 100644 --- a/test/src/test/java/hudson/model/QueueTest.java +++ b/test/src/test/java/hudson/model/QueueTest.java @@ -44,6 +44,7 @@ import hudson.model.Queue.WaitingItem; import hudson.model.labels.LabelExpression; import hudson.model.queue.AbstractQueueTask; +import hudson.model.queue.CauseOfBlockage; import hudson.model.queue.QueueTaskFuture; import hudson.model.queue.ScheduleResult; import hudson.model.queue.SubTask; @@ -52,6 +53,8 @@ import hudson.security.SparseACL; import hudson.slaves.DumbSlave; import hudson.slaves.DummyCloudImpl; +import hudson.slaves.NodeProperty; +import hudson.slaves.NodePropertyDescriptor; import hudson.slaves.NodeProvisionerRule; import hudson.tasks.BuildTrigger; import hudson.tasks.Shell; @@ -97,10 +100,10 @@ import org.junit.Test; import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; -import org.jvnet.hudson.test.MockQueueItemAuthenticator; import org.jvnet.hudson.test.SequenceLock; import org.jvnet.hudson.test.SleepBuilder; import org.jvnet.hudson.test.TestBuilder; +import org.jvnet.hudson.test.TestExtension; import org.jvnet.hudson.test.recipes.LocalData; import org.mortbay.jetty.Server; import org.mortbay.jetty.bio.SocketConnector; @@ -790,17 +793,77 @@ public void run() { * and the flyweight task will be executed. */ public void shouldRunFlyweightTaskOnProvisionedNodeWhenNodeRestricted() throws Exception { - MatrixProject project = r.createMatrixProject(); - project.setAxes(new AxisList( + MatrixProject matrixProject = r.createMatrixProject(); + matrixProject.setAxes(new AxisList( new Axis("axis", "a", "b") )); Label label = LabelExpression.get("aws-linux-dummy"); DummyCloudImpl dummyCloud = new DummyCloudImpl(r, 0); dummyCloud.label = label; r.jenkins.clouds.add(dummyCloud); - project.setAssignedLabel(label); - r.assertBuildStatusSuccess(project.scheduleBuild2(0)); - assertEquals("aws-linux-dummy", project.getBuilds().getLastBuild().getBuiltOn().getLabelString()); + matrixProject.setAssignedLabel(label); + r.assertBuildStatusSuccess(matrixProject.scheduleBuild2(0)); + assertEquals("aws-linux-dummy", matrixProject.getBuilds().getLastBuild().getBuiltOn().getLabelString()); } + @Test + public void shouldBeAbleToBlockFlyWeightTaskOnLastMinute() throws Exception { + MatrixProject matrixProject = r.createMatrixProject("downstream"); + matrixProject.setAxes(new AxisList( + new Axis("axis", "a", "b") + )); + Label label = LabelExpression.get("aws-linux-dummy"); + DummyCloudImpl dummyCloud = new DummyCloudImpl(r, 0); + dummyCloud.label = label; + PropertyImpl property = new PropertyImpl(); + dummyCloud.getNodeProperties().add(property); + r.jenkins.clouds.add(dummyCloud); + matrixProject.setAssignedLabel(label); + FreeStyleProject upstreamProject = r.createFreeStyleProject("upstream"); + //let's assume the flyweighttask has an upstream project and that must be blocked + // when the upstream project is running + matrixProject.addTrigger(new ReverseBuildTrigger("upstream", Result.SUCCESS)); + matrixProject.setBlockBuildWhenUpstreamBuilding(true); + //we schedule the project but we pretend no executors are available thus + //the flyweight task is in the buildable queue without being executed + matrixProject.scheduleBuild2(0); + //in this state the build is not blocked, it's just waiting for an available executor + assertFalse(matrixProject.isBuildBlocked()); + //we start the upstream project that should block the downstream one + upstreamProject.getBuildersList().add(new SleepBuilder(10000)); + QueueTaskFuture upstream = upstreamProject.scheduleBuild2(0); + upstream.waitForStart(); + //let's wait for the Queue to be updated + Thread.sleep(5000); + //the downstream project is blocked waiting for the upstream to finish + assertTrue(matrixProject.isBuildBlocked()); + r.assertBuildStatusSuccess(upstream); + //once the upstream is completed, the downstream can leave the buildable queue. + assertFalse(matrixProject.isBuildBlocked()); + } + + //let's make sure that the downstram project is not started before the upstream --> we want to simulate + // the case: buildable-->blocked-->buildable + public static class PropertyImpl extends NodeProperty { + @Override + public CauseOfBlockage canTake(Queue.BuildableItem item) { + if (item.task.getName().equals("downstream")) { + return new CauseOfBlockage() { + @Override + public String getShortDescription() { + return "slave not provisioned"; + } + }; + } + return null; + } + + @TestExtension("shouldBeAbleToBlockFlyWeightTaskOnLastMinute") + public static class DescriptorImpl extends NodePropertyDescriptor { + @Override + public String getDisplayName() { + return "Some Property"; + } + } + } } diff --git a/test/src/test/java/hudson/slaves/DummyCloudImpl.java b/test/src/test/java/hudson/slaves/DummyCloudImpl.java index 513441492231..1b39aa43bb7a 100644 --- a/test/src/test/java/hudson/slaves/DummyCloudImpl.java +++ b/test/src/test/java/hudson/slaves/DummyCloudImpl.java @@ -34,6 +34,9 @@ import java.util.List; import java.util.concurrent.Callable; import java.util.concurrent.Future; + +import hudson.util.DescribableList; +import jenkins.model.Jenkins; import org.jvnet.hudson.test.JenkinsRule; /** @@ -64,12 +67,29 @@ public class DummyCloudImpl extends Cloud { */ public Label label; + public void setNodeProperties(DescribableList, NodePropertyDescriptor> nodeProperties) { + this.nodeProperties = nodeProperties; + } + public DescribableList, NodePropertyDescriptor> getNodeProperties() { + return this.nodeProperties; + } + + DescribableList,NodePropertyDescriptor> nodeProperties = + new DescribableList,NodePropertyDescriptor>(Jenkins.getInstance().getNodesObject()); + public DummyCloudImpl(JenkinsRule rule, int delay) { super("test"); this.rule = rule; this.delay = delay; } + public DummyCloudImpl(JenkinsRule rule, int delay, DescribableList, NodePropertyDescriptor> nodeProperties) { + super("test"); + this.rule = rule; + this.delay = delay; + this.nodeProperties = nodeProperties; + } + public Collection provision(Label label, int excessWorkload) { List r = new ArrayList(); if(label!=this.label) return r; // provisioning impossible @@ -106,6 +126,7 @@ public Node call() throws Exception { System.out.println("launching slave"); DumbSlave slave = rule.createSlave(label); + slave.getNodeProperties().addAll(nodeProperties); computer = slave.toComputer(); computer.connect(false).get(); synchronized (DummyCloudImpl.this) { From 3b4c09e4e93b361e93c93f33609e752b4ab8ab5d Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Mon, 28 Sep 2015 13:18:05 +0200 Subject: [PATCH 24/29] [JENKINS-30084] enhancing test case --- core/src/main/java/hudson/model/Queue.java | 7 ++++ .../src/test/java/hudson/model/QueueTest.java | 41 +++++++++++++++---- .../java/hudson/slaves/DummyCloudImpl.java | 26 ++++++++---- 3 files changed, 57 insertions(+), 17 deletions(-) diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index 1dbc29305c19..78d227dc3a27 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -876,6 +876,13 @@ public List getPendingItems() { return new ArrayList(snapshot.pendings); } + /** + * Gets the snapshot of all {@link BlockedItem}s. + */ + public List getBlockedItems() { + return new ArrayList(snapshot.blockedProjects); + } + /** * Returns the snapshot of all {@link LeftItem}s. * diff --git a/test/src/test/java/hudson/model/QueueTest.java b/test/src/test/java/hudson/model/QueueTest.java index 0eeec5308623..3e4a8909ffa9 100644 --- a/test/src/test/java/hudson/model/QueueTest.java +++ b/test/src/test/java/hudson/model/QueueTest.java @@ -100,6 +100,7 @@ import org.junit.Test; import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.MockQueueItemAuthenticator; import org.jvnet.hudson.test.SequenceLock; import org.jvnet.hudson.test.SleepBuilder; import org.jvnet.hudson.test.TestBuilder; @@ -807,8 +808,9 @@ public void shouldRunFlyweightTaskOnProvisionedNodeWhenNodeRestricted() throws E } @Test - public void shouldBeAbleToBlockFlyWeightTaskOnLastMinute() throws Exception { + public void shouldBeAbleToBlockFlyweightTaskAtTheLastMinute() throws Exception { MatrixProject matrixProject = r.createMatrixProject("downstream"); + matrixProject.setDisplayName("downstream"); matrixProject.setAxes(new AxisList( new Axis("axis", "a", "b") )); @@ -827,19 +829,42 @@ public void shouldBeAbleToBlockFlyWeightTaskOnLastMinute() throws Exception { //we schedule the project but we pretend no executors are available thus //the flyweight task is in the buildable queue without being executed matrixProject.scheduleBuild2(0); + //let s wait for the Queue instance to be updated + while (Queue.getInstance().getBuildableItems().size() != 1) { + Thread.sleep(10); + } //in this state the build is not blocked, it's just waiting for an available executor assertFalse(matrixProject.isBuildBlocked()); + assertFalse(Queue.getInstance().getItem(1).isBlocked()); //we start the upstream project that should block the downstream one upstreamProject.getBuildersList().add(new SleepBuilder(10000)); + upstreamProject.setDisplayName("upstream"); QueueTaskFuture upstream = upstreamProject.scheduleBuild2(0); - upstream.waitForStart(); - //let's wait for the Queue to be updated - Thread.sleep(5000); - //the downstream project is blocked waiting for the upstream to finish - assertTrue(matrixProject.isBuildBlocked()); + //let s wait for the Upstream to enter the buildable Queue + boolean enteredTheQueue = false; + while (!enteredTheQueue) { + for (Queue.BuildableItem item : Queue.getInstance().getBuildableItems()) { + if (item.task.getDisplayName() != null && item.task.getDisplayName().equals(upstreamProject.getDisplayName())) { + enteredTheQueue = true; + } + } + } + //let's wait for the upstream project to actually start so that we're sure the Queue has been updated + //when the upstream starts the downstream has already left the buildable queue and the queue is empty + while (!Queue.getInstance().getBuildableItems().isEmpty()) { + Thread.sleep(10); + } + assertTrue(Queue.getInstance().getItem(1).isBlocked()); + assertTrue(Queue.getInstance().getBlockedItems().get(0).task.getDisplayName().equals(matrixProject.displayName)); + r.assertBuildStatusSuccess(upstream); - //once the upstream is completed, the downstream can leave the buildable queue. - assertFalse(matrixProject.isBuildBlocked()); + //once the upstream is completed, the downstream can join the buildable queue again. + while (Queue.getInstance().getBuildableItems().isEmpty()) { + Thread.sleep(10); + } + assertFalse(Queue.getInstance().getItem(1).isBlocked()); + assertTrue(Queue.getInstance().getBlockedItems().isEmpty()); + assertTrue(Queue.getInstance().getBuildableItems().get(0).task.getDisplayName().equals(matrixProject.displayName)); } //let's make sure that the downstram project is not started before the upstream --> we want to simulate diff --git a/test/src/test/java/hudson/slaves/DummyCloudImpl.java b/test/src/test/java/hudson/slaves/DummyCloudImpl.java index 1b39aa43bb7a..132c22e9eb0f 100644 --- a/test/src/test/java/hudson/slaves/DummyCloudImpl.java +++ b/test/src/test/java/hudson/slaves/DummyCloudImpl.java @@ -39,6 +39,8 @@ import jenkins.model.Jenkins; import org.jvnet.hudson.test.JenkinsRule; +import javax.xml.ws.Provider; + /** * {@link Cloud} implementation useful for testing. * @@ -67,15 +69,12 @@ public class DummyCloudImpl extends Cloud { */ public Label label; - public void setNodeProperties(DescribableList, NodePropertyDescriptor> nodeProperties) { - this.nodeProperties = nodeProperties; - } - public DescribableList, NodePropertyDescriptor> getNodeProperties() { + public List> getNodeProperties() { return this.nodeProperties; } - DescribableList,NodePropertyDescriptor> nodeProperties = - new DescribableList,NodePropertyDescriptor>(Jenkins.getInstance().getNodesObject()); + List> nodeProperties = + new ArrayList>(); public DummyCloudImpl(JenkinsRule rule, int delay) { super("test"); @@ -83,7 +82,7 @@ public DummyCloudImpl(JenkinsRule rule, int delay) { this.delay = delay; } - public DummyCloudImpl(JenkinsRule rule, int delay, DescribableList, NodePropertyDescriptor> nodeProperties) { + public DummyCloudImpl(JenkinsRule rule, int delay, List> nodeProperties) { super("test"); this.rule = rule; this.delay = delay; @@ -125,8 +124,17 @@ public Node call() throws Exception { Thread.sleep(time); System.out.println("launching slave"); - DumbSlave slave = rule.createSlave(label); - slave.getNodeProperties().addAll(nodeProperties); + final DumbSlave slave = rule.createSlave(label); + Provider nodePropertyProvider = new Provider() { + @Override + public NodeProperty invoke(NodeProperty request) { + request.setNode(slave); + return request; + } + }; + for (NodeProperty nodeProperty : nodeProperties) { + slave.getNodeProperties().add(nodePropertyProvider.invoke(nodeProperty)); + } computer = slave.toComputer(); computer.connect(false).get(); synchronized (DummyCloudImpl.this) { From 562c0e5f0013aa4dfc829443d9bf22b9fd7f2749 Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Mon, 28 Sep 2015 15:03:09 +0200 Subject: [PATCH 25/29] [JENKINS-30084] fixing test --- test/src/test/java/hudson/model/QueueTest.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/test/src/test/java/hudson/model/QueueTest.java b/test/src/test/java/hudson/model/QueueTest.java index 3e4a8909ffa9..28224fdcf477 100644 --- a/test/src/test/java/hudson/model/QueueTest.java +++ b/test/src/test/java/hudson/model/QueueTest.java @@ -834,8 +834,7 @@ public void shouldBeAbleToBlockFlyweightTaskAtTheLastMinute() throws Exception { Thread.sleep(10); } //in this state the build is not blocked, it's just waiting for an available executor - assertFalse(matrixProject.isBuildBlocked()); - assertFalse(Queue.getInstance().getItem(1).isBlocked()); + assertFalse(Queue.getInstance().getItems()[0].isBlocked()); //we start the upstream project that should block the downstream one upstreamProject.getBuildersList().add(new SleepBuilder(10000)); upstreamProject.setDisplayName("upstream"); @@ -854,7 +853,7 @@ public void shouldBeAbleToBlockFlyweightTaskAtTheLastMinute() throws Exception { while (!Queue.getInstance().getBuildableItems().isEmpty()) { Thread.sleep(10); } - assertTrue(Queue.getInstance().getItem(1).isBlocked()); + assertFalse(Queue.getInstance().getItems()[0].isBlocked()); assertTrue(Queue.getInstance().getBlockedItems().get(0).task.getDisplayName().equals(matrixProject.displayName)); r.assertBuildStatusSuccess(upstream); @@ -862,7 +861,7 @@ public void shouldBeAbleToBlockFlyweightTaskAtTheLastMinute() throws Exception { while (Queue.getInstance().getBuildableItems().isEmpty()) { Thread.sleep(10); } - assertFalse(Queue.getInstance().getItem(1).isBlocked()); + assertFalse(Queue.getInstance().getItems()[0].isBlocked()); assertTrue(Queue.getInstance().getBlockedItems().isEmpty()); assertTrue(Queue.getInstance().getBuildableItems().get(0).task.getDisplayName().equals(matrixProject.displayName)); } From 37a372d15442c0ff7e018f29ca135c718f9d4b61 Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Mon, 28 Sep 2015 15:20:08 +0200 Subject: [PATCH 26/29] [JENKINS-30084] fixing test --- test/src/test/java/hudson/model/QueueTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/src/test/java/hudson/model/QueueTest.java b/test/src/test/java/hudson/model/QueueTest.java index 28224fdcf477..6e5505af52f3 100644 --- a/test/src/test/java/hudson/model/QueueTest.java +++ b/test/src/test/java/hudson/model/QueueTest.java @@ -853,7 +853,7 @@ public void shouldBeAbleToBlockFlyweightTaskAtTheLastMinute() throws Exception { while (!Queue.getInstance().getBuildableItems().isEmpty()) { Thread.sleep(10); } - assertFalse(Queue.getInstance().getItems()[0].isBlocked()); + assertTrue(Queue.getInstance().getItems()[0].isBlocked()); assertTrue(Queue.getInstance().getBlockedItems().get(0).task.getDisplayName().equals(matrixProject.displayName)); r.assertBuildStatusSuccess(upstream); From 7ea21bbf2cfb11a707702034e0fad2ee09bdcc02 Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Tue, 29 Sep 2015 10:33:23 +0200 Subject: [PATCH 27/29] [JENKINS-30084] indent back --- core/src/main/java/hudson/model/Queue.java | 104 ++++++++++----------- 1 file changed, 52 insertions(+), 52 deletions(-) diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index 78d227dc3a27..70f9fc763eae 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -1467,64 +1467,64 @@ public void maintain() { // allocate buildable jobs to executors for (BuildableItem p : new ArrayList( buildables)) {// copy as we'll mutate the list in the loop - // one last check to make sure this build is not blocked. - if (isBuildBlocked(p)) { + // one last check to make sure this build is not blocked. + if (isBuildBlocked(p)) { + p.leave(this); + new BlockedItem(p).enter(this); + LOGGER.log(Level.FINE, "Catching that {0} is blocked in the last minute", p); + // JENKINS-28926 we have moved an unblocked task into the blocked state, update snapshot + // so that other buildables which might have been blocked by this can see the state change + updateSnapshot(); + continue; + } + + if (p.task instanceof FlyweightTask) { + Runnable r = makeFlyWeightTaskBuildable(new BuildableItem(p)); + if (r != null) { p.leave(this); - new BlockedItem(p).enter(this); - LOGGER.log(Level.FINE, "Catching that {0} is blocked in the last minute", p); - // JENKINS-28926 we have moved an unblocked task into the blocked state, update snapshot - // so that other buildables which might have been blocked by this can see the state change + r.run(); updateSnapshot(); - continue; } + } else { - if (p.task instanceof FlyweightTask) { - Runnable r = makeFlyWeightTaskBuildable(new BuildableItem(p)); - if (r != null) { - p.leave(this); - r.run(); - updateSnapshot(); - } - } else { - - List candidates = new ArrayList(parked.size()); - for (JobOffer j : parked.values()) - if (j.canTake(p)) - candidates.add(j); - - MappingWorksheet ws = new MappingWorksheet(p, candidates); - Mapping m = loadBalancer.map(p.task, ws); - if (m == null) { - // if we couldn't find the executor that fits, - // just leave it in the buildables list and - // check if we can execute other projects - LOGGER.log(Level.FINER, "Failed to map {0} to executors. candidates={1} parked={2}", - new Object[]{p, candidates, parked.values()}); - continue; - } + List candidates = new ArrayList(parked.size()); + for (JobOffer j : parked.values()) + if (j.canTake(p)) + candidates.add(j); + + MappingWorksheet ws = new MappingWorksheet(p, candidates); + Mapping m = loadBalancer.map(p.task, ws); + if (m == null) { + // if we couldn't find the executor that fits, + // just leave it in the buildables list and + // check if we can execute other projects + LOGGER.log(Level.FINER, "Failed to map {0} to executors. candidates={1} parked={2}", + new Object[]{p, candidates, parked.values()}); + continue; + } - // found a matching executor. use it. - WorkUnitContext wuc = new WorkUnitContext(p); - m.execute(wuc); + // found a matching executor. use it. + WorkUnitContext wuc = new WorkUnitContext(p); + m.execute(wuc); - p.leave(this); - if (!wuc.getWorkUnits().isEmpty()) { - makePending(p); - } - else - LOGGER.log(Level.FINE, "BuildableItem {0} with empty work units!?", p); - - // Ensure that identification of blocked tasks is using the live state: JENKINS-27708 & JENKINS-27871 - // The creation of a snapshot itself should be relatively cheap given the expected rate of - // job execution. You probably would need 100's of jobs starting execution every iteration - // of maintain() before this could even start to become an issue and likely the calculation - // of isBuildBlocked(p) will become a bottleneck before updateSnapshot() will. Additionally - // since the snapshot itself only ever has at most one reference originating outside of the stack - // it should remain in the eden space and thus be cheap to GC. - // See https://issues.jenkins-ci.org/browse/JENKINS-27708?focusedCommentId=225819&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-225819 - // or https://issues.jenkins-ci.org/browse/JENKINS-27708?focusedCommentId=225906&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-225906 - // for alternative fixes of this issue. - updateSnapshot(); + p.leave(this); + if (!wuc.getWorkUnits().isEmpty()) { + makePending(p); + } + else + LOGGER.log(Level.FINE, "BuildableItem {0} with empty work units!?", p); + + // Ensure that identification of blocked tasks is using the live state: JENKINS-27708 & JENKINS-27871 + // The creation of a snapshot itself should be relatively cheap given the expected rate of + // job execution. You probably would need 100's of jobs starting execution every iteration + // of maintain() before this could even start to become an issue and likely the calculation + // of isBuildBlocked(p) will become a bottleneck before updateSnapshot() will. Additionally + // since the snapshot itself only ever has at most one reference originating outside of the stack + // it should remain in the eden space and thus be cheap to GC. + // See https://issues.jenkins-ci.org/browse/JENKINS-27708?focusedCommentId=225819&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-225819 + // or https://issues.jenkins-ci.org/browse/JENKINS-27708?focusedCommentId=225906&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-225906 + // for alternative fixes of this issue. + updateSnapshot(); } } } finally { updateSnapshot(); } } finally { From 8d113a10678ee172ce4e4987867088de5e510991 Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Thu, 1 Oct 2015 09:54:58 +0200 Subject: [PATCH 28/29] [JENKINS-30084] address feedbacks --- core/src/main/java/hudson/model/Queue.java | 9 +++++--- .../src/test/java/hudson/model/QueueTest.java | 23 ++++++++++++++----- 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index 70f9fc763eae..b83d096c76d6 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -879,7 +879,7 @@ public List getPendingItems() { /** * Gets the snapshot of all {@link BlockedItem}s. */ - public List getBlockedItems() { + protected List getBlockedItems() { return new ArrayList(snapshot.blockedProjects); } @@ -1583,17 +1583,20 @@ private Runnable makeFlyWeightTaskBuildable(final BuildableItem p){ ConsistentHash hash = new ConsistentHash(NODE_HASH); hash.addAll(hashSource); + Label lbl = p.getAssignedLabel(); for (Node n : hash.list(p.task.getFullDisplayName())) { final Computer c = n.toComputer(); if (c == null || c.isOffline()) { continue; } + if (lbl!=null && !lbl.contains(n)) { + continue; + } if (n.canTake(p) != null) { continue; } return new Runnable() { - @Override - public void run() { + @Override public void run() { c.startFlyWeightTask(new WorkUnitContext(p).createWorkUnit(p.task)); makePending(p); diff --git a/test/src/test/java/hudson/model/QueueTest.java b/test/src/test/java/hudson/model/QueueTest.java index 6e5505af52f3..56a23a7839f8 100644 --- a/test/src/test/java/hudson/model/QueueTest.java +++ b/test/src/test/java/hudson/model/QueueTest.java @@ -814,31 +814,42 @@ public void shouldBeAbleToBlockFlyweightTaskAtTheLastMinute() throws Exception { matrixProject.setAxes(new AxisList( new Axis("axis", "a", "b") )); + Label label = LabelExpression.get("aws-linux-dummy"); DummyCloudImpl dummyCloud = new DummyCloudImpl(r, 0); dummyCloud.label = label; - PropertyImpl property = new PropertyImpl(); + BlockDownstreamProjectExecution property = new BlockDownstreamProjectExecution(); dummyCloud.getNodeProperties().add(property); r.jenkins.clouds.add(dummyCloud); matrixProject.setAssignedLabel(label); + FreeStyleProject upstreamProject = r.createFreeStyleProject("upstream"); + upstreamProject.getBuildersList().add(new SleepBuilder(10000)); + upstreamProject.setDisplayName("upstream"); + //let's assume the flyweighttask has an upstream project and that must be blocked // when the upstream project is running matrixProject.addTrigger(new ReverseBuildTrigger("upstream", Result.SUCCESS)); matrixProject.setBlockBuildWhenUpstreamBuilding(true); + //we schedule the project but we pretend no executors are available thus //the flyweight task is in the buildable queue without being executed - matrixProject.scheduleBuild2(0); + QueueTaskFuture downstream = matrixProject.scheduleBuild2(0); + if (downstream == null) { + throw new Exception("the flyweight task could not be scheduled, thus the test will be interrupted"); + } //let s wait for the Queue instance to be updated while (Queue.getInstance().getBuildableItems().size() != 1) { Thread.sleep(10); } //in this state the build is not blocked, it's just waiting for an available executor assertFalse(Queue.getInstance().getItems()[0].isBlocked()); + //we start the upstream project that should block the downstream one - upstreamProject.getBuildersList().add(new SleepBuilder(10000)); - upstreamProject.setDisplayName("upstream"); QueueTaskFuture upstream = upstreamProject.scheduleBuild2(0); + if (upstream == null) { + throw new Exception("the upstream task could not be scheduled, thus the test will be interrupted"); + } //let s wait for the Upstream to enter the buildable Queue boolean enteredTheQueue = false; while (!enteredTheQueue) { @@ -856,8 +867,8 @@ public void shouldBeAbleToBlockFlyweightTaskAtTheLastMinute() throws Exception { assertTrue(Queue.getInstance().getItems()[0].isBlocked()); assertTrue(Queue.getInstance().getBlockedItems().get(0).task.getDisplayName().equals(matrixProject.displayName)); - r.assertBuildStatusSuccess(upstream); //once the upstream is completed, the downstream can join the buildable queue again. + r.assertBuildStatusSuccess(upstream); while (Queue.getInstance().getBuildableItems().isEmpty()) { Thread.sleep(10); } @@ -868,7 +879,7 @@ public void shouldBeAbleToBlockFlyweightTaskAtTheLastMinute() throws Exception { //let's make sure that the downstram project is not started before the upstream --> we want to simulate // the case: buildable-->blocked-->buildable - public static class PropertyImpl extends NodeProperty { + public static class BlockDownstreamProjectExecution extends NodeProperty { @Override public CauseOfBlockage canTake(Queue.BuildableItem item) { if (item.task.getName().equals("downstream")) { From a42f1812e8b1948ddca763e4623cf04f207ee005 Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Thu, 1 Oct 2015 10:44:10 +0200 Subject: [PATCH 29/29] [JENKINS-30084] remove extra space --- core/src/main/java/hudson/model/Queue.java | 1 - 1 file changed, 1 deletion(-) diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index b83d096c76d6..c265e477df46 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -1599,7 +1599,6 @@ private Runnable makeFlyWeightTaskBuildable(final BuildableItem p){ @Override public void run() { c.startFlyWeightTask(new WorkUnitContext(p).createWorkUnit(p.task)); makePending(p); - } }; }