From be1f8f91a3dcdcdfd2ed07198659e7eb68abf1f7 Mon Sep 17 00:00:00 2001 From: Kohsuke Kawaguchi Date: Wed, 23 May 2012 17:22:08 -0700 Subject: [PATCH] Fixed a bug in Jenkins.overallNodeProvisioner Until now, Jenkins.overallNodeProvisioner was tracking an inconsistent pair of numbers. On one hand, it was tracking all the executors in the system, but then it was also only tracking the # of free-roaming label-unassigned jobs as the queue length. So if all your slaves are set with Node.Mode.EXCLUSIVE and you have some free-roaming jobs, then it'll never tickle NodePrivisioner. In other words, the # of executors weren't reflecting the actual # of executors that can execute what's counted as the queue length. See the problem report at https://groups.google.com/forum/?fromgroups#!topic/jenkinsci-dev/bUwGEgOwv4Q To fix this, I introduced another LoadStatistics that only counts the # of executors that can execute free-roaming jobs, and # of free-roaming jobs as the queue length. In this way, two pairs of numbers are consistent. This now allows us to use overallLoadStatistics for really the entire system, including all executors and the total queue length. This is primarily for administrators to see the resource utilization, and it is not useful for NodeProvisioner input because it's mixing too many different things. The semantics change in OverallLoadStatistics.queueLength allows us to deprecate its totalQueueLength field. --- changelog.html | 3 + .../java/hudson/model/LoadStatistics.java | 50 ++++++++----- .../hudson/model/OverallLoadStatistics.java | 24 ++++--- core/src/main/java/hudson/model/Queue.java | 15 ++++ .../java/hudson/slaves/NodeProvisioner.java | 2 +- core/src/main/java/jenkins/model/Jenkins.java | 29 +++++++- .../jenkins/model/UnlabeldLoadStatistics.java | 71 +++++++++++++++++++ 7 files changed, 161 insertions(+), 33 deletions(-) create mode 100644 core/src/main/java/jenkins/model/UnlabeldLoadStatistics.java diff --git a/changelog.html b/changelog.html index a4c4a9349e..ef152ab5f2 100644 --- a/changelog.html +++ b/changelog.html @@ -58,6 +58,9 @@ Upcoming changes
  • When accessing a page that requires authentication, redirection to start authentication results in a content decoding failure. (issue 13625) +
  • + Fixed a bug in the way cloud support handles free-roaming jobs. + (discussion)
  • Fixed a regression in untar operation in exotic platforms (issue 13202) diff --git a/core/src/main/java/hudson/model/LoadStatistics.java b/core/src/main/java/hudson/model/LoadStatistics.java index bbf03fd3cf..40185eb042 100644 --- a/core/src/main/java/hudson/model/LoadStatistics.java +++ b/core/src/main/java/hudson/model/LoadStatistics.java @@ -60,6 +60,7 @@ import java.util.List; * @author Kohsuke Kawaguchi * @see Label#loadStatistics * @see Jenkins#overallLoad + * @see Jenkins#unlabeledLoad */ @ExportedBean public abstract class LoadStatistics { @@ -176,6 +177,20 @@ public abstract class LoadStatistics { return new Api(this); } + /** + * Updates {@link #totalExecutors} and {@link #busyExecutors} by using + * the current snapshot value. + * + * {@link #queueLength} is updated separately via {@link LoadStatisticsUpdater} to + * improve the efficiency because we are counting this for all {@link LoadStatistics} at once. + */ + protected void updateExecutorCounts() { + int t = computeTotalExecutors(); + int i = computeIdleExecutors(); + totalExecutors.update(t); + busyExecutors.update(t-i); + } + /** * With 0.90 decay ratio for every 10sec, half reduction is about 1 min. */ @@ -195,33 +210,30 @@ public abstract class LoadStatistics { } protected void doRun() { - Jenkins h = Jenkins.getInstance(); - List bis = h.getQueue().getBuildableItems(); + Jenkins j = Jenkins.getInstance(); + List bis = j.getQueue().getBuildableItems(); // update statistics on slaves - for( Label l : h.getLabels() ) { - l.loadStatistics.totalExecutors.update(l.getTotalExecutors()); - l.loadStatistics.busyExecutors .update(l.getBusyExecutors()); - - int q=0; - for (hudson.model.Queue.BuildableItem bi : bis) { - if(bi.task.getAssignedLabel()==l) - q++; - } - l.loadStatistics.queueLength.update(q); + for( Label l : j.getLabels() ) { + l.loadStatistics.updateExecutorCounts(); + l.loadStatistics.queueLength.update(count(bis, l)); } // update statistics of the entire system - ComputerSet cs = new ComputerSet(); - h.overallLoad.totalExecutors.update(cs.getTotalExecutors()); - h.overallLoad.busyExecutors .update(cs.getBusyExecutors()); + j.unlabeledLoad.updateExecutorCounts(); + j.unlabeledLoad.queueLength.update(count(bis, null)); + + j.overallLoad.updateExecutorCounts(); + j.overallLoad.queueLength.update(bis.size()); + } + + private int count(List bis, Label l) { int q=0; - for (hudson.model.Queue.BuildableItem bi : bis) { - if(bi.task.getAssignedLabel()==null) + for (Queue.BuildableItem bi : bis) { + if(bi.task.getAssignedLabel()==l) q++; } - h.overallLoad.queueLength.update(q); - h.overallLoad.totalQueueLength.update(bis.size()); + return q; } } } diff --git a/core/src/main/java/hudson/model/OverallLoadStatistics.java b/core/src/main/java/hudson/model/OverallLoadStatistics.java index dbaaa058d2..9aeea70368 100644 --- a/core/src/main/java/hudson/model/OverallLoadStatistics.java +++ b/core/src/main/java/hudson/model/OverallLoadStatistics.java @@ -25,28 +25,30 @@ package hudson.model; import hudson.model.MultiStageTimeSeries.TimeScale; import hudson.model.MultiStageTimeSeries.TrendChart; -import hudson.util.ColorPalette; import jenkins.model.Jenkins; +import jenkins.model.UnlabeldLoadStatistics; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; import org.kohsuke.stapler.export.Exported; /** - * {@link LoadStatistics} for the entire system (the master and all the slaves combined.) - * - *

    - * {@link #computeQueueLength()} and {@link #queueLength} counts those tasks - * that are unassigned to any node, whereas {@link #totalQueueLength} - * tracks the queue length including tasks that are assigned to a specific node. + * {@link LoadStatistics} for the entire system (the master and all the slaves combined), + * and all the jobs that are running on it. * * @author Kohsuke Kawaguchi * @see Jenkins#overallLoad + * @see UnlabeldLoadStatistics */ public class OverallLoadStatistics extends LoadStatistics { /** * Number of total {@link Queue.BuildableItem}s that represents blocked builds. + * + * @deprecated as of 1.467 + * Use {@link #queueLength}. Left as an alias here for backward compatibility. */ @Exported - public final MultiStageTimeSeries totalQueueLength = new MultiStageTimeSeries( - Messages._LoadStatistics_Legends_QueueLength(), ColorPalette.GREY, 0,DECAY); + @Restricted(NoExternalUse.class) + public final MultiStageTimeSeries totalQueueLength = queueLength; public OverallLoadStatistics() { super(0,0); @@ -64,7 +66,7 @@ public class OverallLoadStatistics extends LoadStatistics { @Override public int computeQueueLength() { - return Jenkins.getInstance().getQueue().countBuildableItemsFor(null); + return Jenkins.getInstance().getQueue().countBuildableItems(); } /** @@ -72,6 +74,6 @@ public class OverallLoadStatistics extends LoadStatistics { * not {@link #queueLength}, which just shows jobs that are to be run on the master. */ protected TrendChart createOverallTrendChart(TimeScale timeScale) { - return MultiStageTimeSeries.createTrendChart(timeScale,busyExecutors,totalExecutors,totalQueueLength); + return MultiStageTimeSeries.createTrendChart(timeScale,busyExecutors,totalExecutors,queueLength); } } diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index 4d041a2d70..ee727dcf7a 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -691,6 +691,13 @@ public class Queue extends ResourceController implements Saveable { return r; } + /** + * Counts all the {@link BuildableItem}s currently in the queue. + */ + public synchronized int countBuildableItems() { + return buildables.size()+pendings.size(); + } + /** * Gets the information about the queue item for the given project. * @@ -1260,6 +1267,14 @@ public class Queue extends ResourceController implements Saveable { */ public Future getFuture() { return future; } + /** + * If this task needs to be run on a node with a particular label, + * return that {@link Label}. Otherwise null, indicating + * it can run on anywhere. + * + *

    + * This code takes {@link LabelAssignmentAction} into account, then fall back to {@link SubTask#getAssignedLabel()} + */ public Label getAssignedLabel() { for (LabelAssignmentAction laa : getActions(LabelAssignmentAction.class)) { Label l = laa.getAssignedLabel(task); diff --git a/core/src/main/java/hudson/slaves/NodeProvisioner.java b/core/src/main/java/hudson/slaves/NodeProvisioner.java index 0ad1519cff..c1b028cdb4 100644 --- a/core/src/main/java/hudson/slaves/NodeProvisioner.java +++ b/core/src/main/java/hudson/slaves/NodeProvisioner.java @@ -305,7 +305,7 @@ public class NodeProvisioner { @Override protected void doRun() { Jenkins h = Jenkins.getInstance(); - h.overallNodeProvisioner.update(); + h.unlabeledNodeProvisioner.update(); for( Label l : h.getLabels() ) l.nodeProvisioner.update(); } diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java index 62c7978332..9c02c3c72d 100755 --- a/core/src/main/java/jenkins/model/Jenkins.java +++ b/core/src/main/java/jenkins/model/Jenkins.java @@ -30,6 +30,7 @@ import com.google.common.collect.Lists; import com.google.inject.Injector; import hudson.ExtensionComponent; import hudson.ExtensionFinder; +import hudson.model.LoadStatistics; import hudson.model.Messages; import hudson.model.Node; import hudson.model.AbstractCIBase; @@ -569,14 +570,38 @@ public class Jenkins extends AbstractCIBase implements ModifiableItemGroup