Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-20046] - Do not query queue dispatchers from UI #3038

Merged
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -1175,28 +1175,63 @@ public boolean contains(Task t) {
/**
* 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) {

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Oct 11, 2017

Member

sounds overused for non static method, why not just?
getCauseOfBlockage(Item i)
getCauseOfBlockage(Task i)

This comment has been minimized.

Copy link
@Jimilian

Jimilian Oct 11, 2017

Author Contributor

This file contains already several getCauseOfBlockage methods, so, this name is just tradeoff to reduce cognitive complexity of this file :)

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Oct 11, 2017

Member

not a problem while they have argument that are not inherited by each other =)

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;
}

/**
@@ -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);
@@ -1487,6 +1523,8 @@ public void maintain() {
// determine if they are blocked by the lucky winner
updateSnapshot();
}
} else {
p.setCauseOfBlockage(causeOfBlockage);

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Sep 29, 2017

Member

if ``allowNewBuildableTask` returns false, a non-null value may be overridden by a null one here. Is it "as designed"?

This comment has been minimized.

Copy link
@Jimilian

Jimilian Sep 29, 2017

Author Contributor

Good catch. Moved allowNewBuildableTask to getCauseOfBlockage as well.

}
}
}
@@ -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;
@@ -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);
}
}

@@ -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
@@ -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
@@ -1777,7 +1816,7 @@ public Api getApi() {
* for temporary reasons.
*
* <p>
* Short-hand for {@code getCauseOfBlockage()!=null}.
* Short-hand for {@code getCauseOfBlockageForItem()!=null}.
*/
boolean isBuildBlocked();

@@ -1802,6 +1841,9 @@ public Api getApi() {
*/
@CheckForNull
default CauseOfBlockage getCauseOfBlockage() {
if (isBuildBlocked()) {

This comment has been minimized.

Copy link
@jglick
return CauseOfBlockage.fromMessage(Messages._Queue_Unknown());
}
return null;
}

@@ -2443,29 +2485,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) {

This comment has been minimized.

Copy link
@jglick

jglick Oct 15, 2017

Member

Why would this be public? It is only called from within the same source file AFAIK.

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 comment has been minimized.

Copy link
@jglick

jglick Oct 15, 2017

Member

Ditto. Why would this need to be a public API? What plugin would be calling it and why?

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) {
@@ -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;
@@ -527,16 +528,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";}
@@ -769,7 +778,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);
@@ -965,4 +974,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());
}
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.