Skip to content

Commit

Permalink
[JENKINS-20046] - Do not query queue dispatchers from UI (#3038)
Browse files Browse the repository at this point in the history 
* Do not query queue dispatchers from UI

* Address comments from review

* Restore old constructors and mark them as @deprecated
* Optimise query from UI even more

* Check non-concurrent builds in getCauseOfBlockageForItem
  • Loading branch information
@Jimilian @oleg-nenashev
Jimilian authored and oleg-nenashev committed Oct 15, 2017
1 parent 0fac49e commit be02386
Show file tree
Hide file tree
Showing 2 changed files with 136 additions and 36 deletions.
126 changes: 92 additions & 34 deletions core/src/main/java/hudson/model/Queue.java
View file
Expand Up @@ -1175,28 +1175,63 @@ public Future<?> scheduleMaintenance() {
/**
* Checks if the given item should be prevented from entering into the {@link #buildables} state
* and instead stay in the {@link #blockedProjects} state.
*
* @return the reason of blockage if it exists null otherwise.
*/
private boolean isBuildBlocked(Item i) {
if (i.task.isBuildBlocked() || !canRun(i.task.getResourceList()))
return true;
@CheckForNull
private CauseOfBlockage getCauseOfBlockageForItem(Item i) {
CauseOfBlockage causeOfBlockage = getCauseOfBlockageForTask(i.task);
if (causeOfBlockage != null) {
return causeOfBlockage;
}

for (QueueTaskDispatcher d : QueueTaskDispatcher.all()) {
if (d.canRun(i)!=null)
return true;
causeOfBlockage = d.canRun(i);
if (causeOfBlockage != null)
return causeOfBlockage;
}

if(!(i instanceof BuildableItem)) {
// Make sure we don't queue two tasks of the same project to be built
// unless that project allows concurrent builds. Once item is buildable it's ok.
//
// This check should never pass. And must be remove once we can completely rely on `getCauseOfBlockage`.
// If `task.isConcurrentBuild` returns `false`,
// it should also return non-null value for `task.getCauseOfBlockage` in case of on-going execution.
// But both are public non-final methods, so, we need to keep backward compatibility here.
// And check one more time across all `buildables` and `pendings` for O(N) each.
if (!i.task.isConcurrentBuild() && (buildables.containsKey(i.task) || pendings.containsKey(i.task))) {
return CauseOfBlockage.fromMessage(Messages._Queue_InProgress());
}
}

return false;
return null;
}

/**
* Make sure we don't queue two tasks of the same project to be built
* unless that project allows concurrent builds.
*
* Checks if the given task knows the reasons to be blocked or it needs some unavailable resources
*
* @param task the task.
* @return the reason of blockage if it exists null otherwise.
*/
private boolean allowNewBuildableTask(Task t) {
if (t.isConcurrentBuild()) {
return true;
@CheckForNull
private CauseOfBlockage getCauseOfBlockageForTask(Task task) {
CauseOfBlockage causeOfBlockage = task.getCauseOfBlockage();
if (causeOfBlockage != null) {
return task.getCauseOfBlockage();
}
return !buildables.containsKey(t) && !pendings.containsKey(t);

if (!canRun(task.getResourceList())) {
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()));
}
}

return null;
}

/**
Expand Down Expand Up @@ -1472,7 +1507,8 @@ public void maintain() {
for (BlockedItem p : blockedItems) {
String taskDisplayName = LOGGER.isLoggable(Level.FINEST) ? p.task.getFullDisplayName() : null;
LOGGER.log(Level.FINEST, "Current blocked item: {0}", taskDisplayName);
if (!isBuildBlocked(p) && allowNewBuildableTask(p.task)) {
CauseOfBlockage causeOfBlockage = getCauseOfBlockageForItem(p);
if (causeOfBlockage == null) {
LOGGER.log(Level.FINEST,
"BlockedItem {0}: blocked -> buildable as the build is not blocked and new tasks are allowed",
taskDisplayName);
Expand All @@ -1487,6 +1523,8 @@ public void maintain() {
// determine if they are blocked by the lucky winner
updateSnapshot();
}
} else {
p.setCauseOfBlockage(causeOfBlockage);
}
}
}
Expand All @@ -1501,8 +1539,8 @@ public void maintain() {
}

top.leave(this);
Task p = top.task;
if (!isBuildBlocked(top) && allowNewBuildableTask(p)) {
CauseOfBlockage causeOfBlockage = getCauseOfBlockageForItem(top);
if (causeOfBlockage == null) {
// ready to be executed immediately
Runnable r = makeBuildable(new BuildableItem(top));
String topTaskDisplayName = LOGGER.isLoggable(Level.FINEST) ? top.task.getFullDisplayName() : null;
Expand All @@ -1511,12 +1549,12 @@ public void maintain() {
r.run();
} else {
LOGGER.log(Level.FINEST, "Item {0} was unable to be made a buildable and is now a blocked item.", topTaskDisplayName);
new BlockedItem(top).enter(this);
new BlockedItem(top, CauseOfBlockage.fromMessage(Messages._Queue_HudsonIsAboutToShutDown())).enter(this);
}
} else {
// this can't be built now because another build is in progress
// set this project aside.
new BlockedItem(top).enter(this);
new BlockedItem(top, causeOfBlockage).enter(this);
}
}

Expand All @@ -1530,9 +1568,10 @@ public void maintain() {
for (BuildableItem p : new ArrayList<BuildableItem>(
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)) {
CauseOfBlockage causeOfBlockage = getCauseOfBlockageForItem(p);
if (causeOfBlockage != null) {
p.leave(this);
new BlockedItem(p).enter(this);
new BlockedItem(p, causeOfBlockage).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
Expand Down Expand Up @@ -1596,7 +1635,7 @@ public void maintain() {
// 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
// of getCauseOfBlockageForItem(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://jenkins-ci.org/issue/27708?focusedCommentId=225819&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-225819
Expand Down Expand Up @@ -1777,7 +1816,7 @@ public interface Task extends ModelObject, SubTask {
* for temporary reasons.
*
* <p>
* Short-hand for {@code getCauseOfBlockage()!=null}.
* Short-hand for {@code getCauseOfBlockageForItem()!=null}.
*/
boolean isBuildBlocked();

Expand All @@ -1802,6 +1841,9 @@ public interface Task extends ModelObject, SubTask {
*/
@CheckForNull
default CauseOfBlockage getCauseOfBlockage() {
if (isBuildBlocked()) {
return CauseOfBlockage.fromMessage(Messages._Queue_Unknown());
}
return null;
}

Expand Down Expand Up @@ -2454,29 +2496,45 @@ protected NotWaitingItem(NotWaitingItem ni) {
* {@link Item} in the {@link Queue#blockedProjects} stage.
*/
public final class BlockedItem extends NotWaitingItem {
private transient CauseOfBlockage causeOfBlockage = null;

/**
* @deprecated Use {@link #BlockedItem(WaitingItem, CauseOfBlockage)} instead
*/
@Deprecated
public BlockedItem(WaitingItem wi) {
super(wi);
this(wi, null);
}

/**
* @deprecated Use {@link #BlockedItem(NotWaitingItem, CauseOfBlockage)} instead
*/
@Deprecated
public BlockedItem(NotWaitingItem ni) {
this(ni, null);
}

public BlockedItem(WaitingItem wi, CauseOfBlockage causeOfBlockage) {
super(wi);
this.causeOfBlockage = causeOfBlockage;
}

public BlockedItem(NotWaitingItem ni, CauseOfBlockage causeOfBlockage) {
super(ni);
this.causeOfBlockage = causeOfBlockage;
}

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()));
}
public void setCauseOfBlockage(CauseOfBlockage causeOfBlockage) {
this.causeOfBlockage = causeOfBlockage;
}

for (QueueTaskDispatcher d : QueueTaskDispatcher.all()) {
CauseOfBlockage cause = d.canRun(this);
if (cause != null)
return cause;
public CauseOfBlockage getCauseOfBlockage() {
if (causeOfBlockage != null) {
return causeOfBlockage;
}

return task.getCauseOfBlockage();
// fallback for backward compatibility
return getCauseOfBlockageForItem(this);
}

/*package*/ void enter(Queue q) {
Expand Down
46 changes: 44 additions & 2 deletions test/src/test/java/hudson/model/QueueTest.java
View file
Expand Up @@ -69,6 +69,7 @@
import hudson.triggers.TimerTrigger.TimerTriggerCause;
import hudson.util.OneShotEvent;
import hudson.util.XStream2;
import jenkins.model.BlockedBecauseOfBuildInProgress;
import jenkins.model.Jenkins;
import jenkins.security.QueueItemAuthenticatorConfiguration;
import jenkins.triggers.ReverseBuildTrigger;
Expand Down Expand Up @@ -529,16 +530,24 @@ public TestFlyweightTask(AtomicInteger cnt, Label assignedLabel) {
}
static class TestTask implements Queue.Task {
private final AtomicInteger cnt;
private final boolean isBlocked;

TestTask(AtomicInteger cnt) {
this(cnt, false);
}

TestTask(AtomicInteger cnt, boolean isBlocked) {
this.cnt = cnt;
this.isBlocked = isBlocked;
}

@Override public boolean equals(Object o) {
return o instanceof TestTask && cnt == ((TestTask) o).cnt;
}
@Override public int hashCode() {
return cnt.hashCode();
}
@Override public boolean isBuildBlocked() {return false;}
@Override public boolean isBuildBlocked() {return isBlocked;}
@Override public String getWhyBlocked() {return null;}
@Override public String getName() {return "test";}
@Override public String getFullDisplayName() {return "Test";}
Expand Down Expand Up @@ -771,7 +780,7 @@ public void run() {
final FreeStyleProject projectB = r.createFreeStyleProject(prefix+"B");
projectB.getBuildersList().add(new SleepBuilder(10000));
projectB.setBlockBuildWhenUpstreamBuilding(true);

final FreeStyleProject projectC = r.createFreeStyleProject(prefix+"C");
projectC.getBuildersList().add(new SleepBuilder(10000));
projectC.setBlockBuildWhenUpstreamBuilding(true);
Expand Down Expand Up @@ -978,4 +987,37 @@ public String getShortDescription() {
};
}
}

@Test
public void testDefaultImplementationOfGetCauseOfBlockageForBlocked() throws Exception {
Queue queue = r.getInstance().getQueue();
queue.schedule2(new TestTask(new AtomicInteger(0), true), 0);

queue.maintain();

assertEquals(1, r.jenkins.getQueue().getBlockedItems().size());
CauseOfBlockage actual = r.jenkins.getQueue().getBlockedItems().get(0).getCauseOfBlockage();
CauseOfBlockage expected = CauseOfBlockage.fromMessage(Messages._Queue_Unknown());

assertEquals(expected.getShortDescription(), actual.getShortDescription());
}

@Test
public void testGetCauseOfBlockageForNonConcurrentFreestyle() throws Exception {
Queue queue = r.getInstance().getQueue();
FreeStyleProject t1 = r.createFreeStyleProject("project");
t1.getBuildersList().add(new SleepBuilder(TimeUnit.SECONDS.toMillis(30)));
t1.setConcurrentBuild(false);

t1.scheduleBuild2(0).waitForStart();
t1.scheduleBuild2(0);

queue.maintain();

assertEquals(1, r.jenkins.getQueue().getBlockedItems().size());
CauseOfBlockage actual = r.jenkins.getQueue().getBlockedItems().get(0).getCauseOfBlockage();
CauseOfBlockage expected = new BlockedBecauseOfBuildInProgress(t1.getFirstBuild());

assertEquals(expected.getShortDescription(), actual.getShortDescription());
}
}

0 comments on commit be02386

Please sign in to comment.