From 6b8af39b0317bad90851d4f8025fd71fadb9deaa Mon Sep 17 00:00:00 2001
From: Stephen Connolly <stephen.alan.connolly@gmail.com>
Date: Wed, 3 Dec 2014 16:41:41 +0000
Subject: [PATCH 1/2] [FIXED JENKINS-21618] Ensure that the LoadStatistics
 return a self-consistent result.

The original method of computing load statistics would compute the total and idle counts independently
which could lead to counting errors while jobs started in between the different state counting operations.
This change switches to returning a `LoadStatisticsSnapshot` so that callers will get a single consistent
view of the counts which was valid for at least one point in time during the collection of the snapshot.
---
 core/src/main/java/hudson/model/Label.java    |  11 +
 .../java/hudson/model/LoadStatistics.java     | 426 +++++++++++++++++-
 .../hudson/model/OverallLoadStatistics.java   |  13 +-
 .../java/hudson/slaves/NodeProvisioner.java   | 140 ++++--
 core/src/main/java/jenkins/model/Jenkins.java |   3 +-
 .../model/UnlabeledLoadStatistics.java        |  48 ++
 .../hudson/model/Messages.properties          |   5 +
 .../java/hudson/model/LoadStatisticsTest.java |  17 +-
 8 files changed, 589 insertions(+), 74 deletions(-)

diff --git a/core/src/main/java/hudson/model/Label.java b/core/src/main/java/hudson/model/Label.java
index cdfcfb5e86..7e8510e06a 100644
--- a/core/src/main/java/hudson/model/Label.java
+++ b/core/src/main/java/hudson/model/Label.java
@@ -39,6 +39,7 @@ import hudson.model.labels.LabelExpressionLexer;
 import hudson.model.labels.LabelExpressionParser;
 import hudson.model.labels.LabelOperatorPrecedence;
 import hudson.model.labels.LabelVisitor;
+import hudson.model.queue.SubTask;
 import hudson.security.ACL;
 import hudson.slaves.NodeProvisioner;
 import hudson.slaves.Cloud;
@@ -107,6 +108,16 @@ public abstract class Label extends Actionable implements Comparable<Label>, Mod
             public int computeQueueLength() {
                 return Jenkins.getInstance().getQueue().countBuildableItemsFor(Label.this);
             }
+
+            @Override
+            protected Set<Node> getNodes() {
+                return Label.this.getNodes();
+            }
+
+            @Override
+            protected boolean matches(SubTask item) {
+                return item.getAssignedLabel() == Label.this;
+            }
         };
         this.nodeProvisioner = new NodeProvisioner(this, loadStatistics);
     }
diff --git a/core/src/main/java/hudson/model/LoadStatistics.java b/core/src/main/java/hudson/model/LoadStatistics.java
index 34aa94af00..5d4640c24a 100644
--- a/core/src/main/java/hudson/model/LoadStatistics.java
+++ b/core/src/main/java/hudson/model/LoadStatistics.java
@@ -24,6 +24,7 @@
 package hudson.model;
 
 import hudson.Extension;
+import hudson.Util;
 import hudson.model.MultiStageTimeSeries.TimeScale;
 import hudson.model.MultiStageTimeSeries.TrendChart;
 import hudson.model.queue.SubTask;
@@ -47,6 +48,7 @@ import org.kohsuke.stapler.export.Exported;
 
 import java.awt.*;
 import java.io.IOException;
+import java.io.Serializable;
 import java.util.List;
 
 /**
@@ -66,6 +68,33 @@ import java.util.List;
  */
 @ExportedBean
 public abstract class LoadStatistics {
+    /**
+     * {@code true} if and only if the new way of building statistics has been implemented by this class.
+     * @since 1.FIXME
+     */
+    private final boolean modern;
+
+    /**
+     * Number of executors defined for Jenkins and how it changes over time.
+     * @since 1.FIXME
+     */
+    @Exported
+    public final MultiStageTimeSeries definedExecutors;
+
+    /**
+     * Number of executors on-line and how it changes over time. Replaces {@link #totalExecutors}
+     * @since 1.FIXME
+     */
+    @Exported
+    public final MultiStageTimeSeries onlineExecutors;
+
+    /**
+     * Number of executors in the process of coming on-line and how it changes over time.
+     * @since 1.FIXME
+     */
+    @Exported
+    public final MultiStageTimeSeries connectingExecutors;
+
     /**
      * Number of busy executors and how it changes over time.
      */
@@ -73,9 +102,27 @@ public abstract class LoadStatistics {
     public final MultiStageTimeSeries busyExecutors;
 
     /**
-     * Number of total executors and how it changes over time.
+     * Number of executors not executing and how it changes over time. Note the these executors may not be able to
+     * take work, see {@link #availableExecutors}.
+     * @since 1.FIXME
      */
     @Exported
+    public final MultiStageTimeSeries idleExecutors;
+
+    /**
+     * Number of executors not executing and available to take work and how it changes over time.
+     * @since 1.FIXME
+     */
+    @Exported
+    public final MultiStageTimeSeries availableExecutors;
+
+    /**
+     * Number of total executors and how it changes over time.
+     * @deprecated use {@link #onlineExecutors}. Note {@code totalExecutors==onlineExecutors} for backward
+     * compatibility support.
+     */
+    @Exported
+    @Deprecated
     public final MultiStageTimeSeries totalExecutors;
 
     /**
@@ -84,32 +131,54 @@ public abstract class LoadStatistics {
     @Exported
     public final MultiStageTimeSeries queueLength;
 
-    protected LoadStatistics(int initialTotalExecutors, int initialBusyExecutors) {
-        this.totalExecutors = new MultiStageTimeSeries(
-                Messages._LoadStatistics_Legends_TotalExecutors(), ColorPalette.BLUE, initialTotalExecutors,DECAY);
+
+    protected LoadStatistics(int initialOnlineExecutors, int initialBusyExecutors) {
+        this.definedExecutors = new MultiStageTimeSeries(Messages._LoadStatistics_Legends_DefinedExecutors(),
+                ColorPalette.YELLOW, initialOnlineExecutors, DECAY);
+        this.onlineExecutors = new MultiStageTimeSeries(
+                Messages._LoadStatistics_Legends_OnlineExecutors(), ColorPalette.BLUE, initialOnlineExecutors,DECAY);
+        this.connectingExecutors = new MultiStageTimeSeries(Messages._LoadStatistics_Legends_ConnectingExecutors(),
+                ColorPalette.YELLOW, 0, DECAY);
         this.busyExecutors = new MultiStageTimeSeries(
                 Messages._LoadStatistics_Legends_BusyExecutors(), ColorPalette.RED, initialBusyExecutors,DECAY);
+        this.idleExecutors = new MultiStageTimeSeries(Messages._LoadStatistics_Legends_IdleExecutors(),
+                ColorPalette.YELLOW, initialOnlineExecutors - initialBusyExecutors, DECAY);
+        this.availableExecutors = new MultiStageTimeSeries(Messages._LoadStatistics_Legends_AvailableExecutors(),
+                ColorPalette.YELLOW, initialOnlineExecutors - initialBusyExecutors, DECAY);
         this.queueLength = new MultiStageTimeSeries(
                 Messages._LoadStatistics_Legends_QueueLength(),ColorPalette.GREY, 0, DECAY);
+        this.totalExecutors = onlineExecutors;
+        modern = Util.isOverridden(LoadStatistics.class, getClass(), "getNodes")
+                && Util.isOverridden(LoadStatistics.class, getClass(), "matches", SubTask.class);
     }
 
+    /**
+     * @deprecated use {@link #idleExecutors} directly.
+     */
+    @Deprecated
     public float getLatestIdleExecutors(TimeScale timeScale) {
-        return totalExecutors.pick(timeScale).getLatest() - busyExecutors.pick(timeScale).getLatest();
+        return idleExecutors.pick(timeScale).getLatest();
     }
 
     /**
      * Computes the # of idle executors right now and obtains the snapshot value.
+     * @deprecated use {@link #computeSnapshot()} and then {@link LoadStatisticsSnapshot#getIdleExecutors()}
      */
+    @Deprecated
     public abstract int computeIdleExecutors();
 
     /**
      * Computes the # of total executors right now and obtains the snapshot value.
+     * @deprecated use {@link #computeSnapshot()} and then {@link LoadStatisticsSnapshot#getOnlineExecutors()}
      */
+    @Deprecated
     public abstract int computeTotalExecutors();
 
     /**
      * Computes the # of queue length right now and obtains the snapshot value.
+     * @deprecated use {@link #computeSnapshot()} and then {@link LoadStatisticsSnapshot#getQueueLength()}
      */
+    @Deprecated
     public abstract int computeQueueLength();
 
     /**
@@ -155,9 +224,10 @@ public abstract class LoadStatistics {
     }
 
     protected void configureRenderer(LineAndShapeRenderer renderer) {
-        renderer.setSeriesPaint(0, ColorPalette.BLUE);  // total
+        renderer.setSeriesPaint(0, ColorPalette.BLUE);  // online
         renderer.setSeriesPaint(1, ColorPalette.RED);   // busy
         renderer.setSeriesPaint(2, ColorPalette.GREY);  // queue
+        renderer.setSeriesPaint(3, ColorPalette.YELLOW);// available
     }
 
     /**
@@ -165,7 +235,7 @@ public abstract class LoadStatistics {
      * of the load statistics graph.
      */
     public TrendChart createTrendChart(TimeScale timeScale) {
-        return MultiStageTimeSeries.createTrendChart(timeScale,totalExecutors,busyExecutors,queueLength);
+        return MultiStageTimeSeries.createTrendChart(timeScale,onlineExecutors,busyExecutors,queueLength,availableExecutors);
     }
 
     /**
@@ -180,17 +250,89 @@ public abstract class LoadStatistics {
     }
 
     /**
-     * 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.
+     * @deprecated use {@link #updateCounts(LoadStatisticsSnapshot)}
      */
+    @Deprecated
     protected void updateExecutorCounts() {
-        int t = computeTotalExecutors();
-        int i = computeIdleExecutors();
-        totalExecutors.update(t);
-        busyExecutors.update(t-i);
+        updateCounts(computeSnapshot());
+    }
+
+    /**
+     * Updates all the series from the current snapshot.
+     * @param current the current snapshot.
+     * @since 1.FIXME
+     */
+    protected void updateCounts(LoadStatisticsSnapshot current) {
+        definedExecutors.update(current.getDefinedExecutors());
+        onlineExecutors.update(current.getOnlineExecutors());
+        connectingExecutors.update(current.getConnectingExecutors());
+        busyExecutors.update(current.getBusyExecutors());
+        idleExecutors.update(current.getIdleExecutors());
+        availableExecutors.update(current.getAvailableExecutors());
+        queueLength.update(current.getQueueLength());
+    }
+
+    /**
+     * Returns the {@link Node} instances that this statistic counts.
+     * @return the {@link Node}
+     * @since 1.FIXME
+     */
+    protected abstract Iterable<Node> getNodes();
+
+    /**
+     * Returns {@code true} is the specified {@link SubTask} from the {@link Queue} should be counted.
+     * @param item the {@link SubTask}
+     * @return {@code true} IFF the specified {@link SubTask} from the {@link Queue} should be counted.
+     * @since 1.FIXME
+     */
+    protected abstract boolean matches(SubTask item);
+
+    /**
+     * Computes a self-consistent snapshot of the load statistics.
+     *
+     * Note: The original method of computing load statistics would compute the total and idle counts independently
+     * which could lead to counting errors while jobs started in between the different state counting operations.
+     * By returning a {@link LoadStatisticsSnapshot} we get a single consistent view of the counts which was valid
+     * for at least one point in time during the execution of this method.
+     *
+     * @return a self-consistent snapshot of the load statistics.
+     * @since 1.FIXME
+     */
+    public LoadStatisticsSnapshot computeSnapshot() {
+        if (modern) {
+            return computeSnapshot(Jenkins.getInstance().getQueue().getBuildableItems());
+        } else {
+            int t = computeTotalExecutors();
+            int i = computeIdleExecutors();
+            return new LoadStatisticsSnapshot(t, t, Math.max(i-t,0), Math.max(t-i,0), i, i, computeQueueLength());
+        }
+    }
+
+    /**
+     * Computes the self-consistent snapshot with the specified queue items.
+
+     * @param queue the queue items.
+     * @return a self-consistent snapshot of the load statistics.
+     * @since 1.FIXME
+     */
+    protected LoadStatisticsSnapshot computeSnapshot(Iterable<Queue.BuildableItem> queue) {
+        final LoadStatisticsSnapshot.Builder builder = LoadStatisticsSnapshot.builder();
+        final Iterable<Node> nodes = getNodes();
+        if (nodes != null) {
+            for (Node node : nodes) {
+                builder.with(node);
+            }
+        }
+        int q = 0;
+        if (queue != null) {
+            for (Queue.BuildableItem item : queue) {
+                for (SubTask st : item.task.getSubTasks()) {
+                    if (matches(st))
+                        q++;
+                }
+            }
+        }
+        return builder.withQueueLength(q).build();
     }
 
     /**
@@ -200,7 +342,7 @@ public abstract class LoadStatistics {
     /**
      * Load statistics clock cycle in milliseconds. Specify a small value for quickly debugging this feature and node provisioning through cloud.
      */
-    public static int CLOCK = Integer.getInteger(LoadStatistics.class.getName()+".clock",10*1000);
+    public static int CLOCK = Integer.getInteger(LoadStatistics.class.getName() + ".clock", 10 * 1000);
 
     /**
      * Periodically update the load statistics average.
@@ -217,16 +359,13 @@ public abstract class LoadStatistics {
 
             // update statistics on slaves
             for( Label l : j.getLabels() ) {
-                l.loadStatistics.updateExecutorCounts();
-                l.loadStatistics.queueLength.update(count(bis, l));
+                l.loadStatistics.updateCounts(l.loadStatistics.computeSnapshot(bis));
             }
 
             // update statistics of the entire system
-            j.unlabeledLoad.updateExecutorCounts();
-            j.unlabeledLoad.queueLength.update(count(bis, null));
+            j.unlabeledLoad.updateCounts(j.unlabeledLoad.computeSnapshot(bis));
 
-            j.overallLoad.updateExecutorCounts();
-            j.overallLoad.queueLength.update(bis.size());
+            j.overallLoad.updateCounts(j.overallLoad.computeSnapshot(bis));
         }
 
         private int count(List<Queue.BuildableItem> bis, Label l) {
@@ -239,4 +378,245 @@ public abstract class LoadStatistics {
             return q;
         }
     }
+
+    /**
+     * Holds a snapshot of the current statistics.
+     * @since 1.FIXME
+     */
+    @ExportedBean
+    public static class LoadStatisticsSnapshot implements Serializable {
+        private static final long serialVersionUID = 1L;
+
+        /**
+         * The total number of executors that Jenkins currently knows, this includes all off-line slaves.
+         */
+        private final int definedExecutors;
+        /**
+         * The total number of executors that are currently on-line.
+         */
+        private final int onlineExecutors;
+        /**
+         * The total number of executors that are currently in the process of connecting to Jenkins.
+         */
+        private final int connectingExecutors;
+        /**
+         * The total number of executors that are currently busy running jobs.
+         */
+        private final int busyExecutors;
+        /**
+         * The total number of executors that are currently on-line and idle. This includes executors that are
+         * not accepting tasks.
+         */
+        private final int idleExecutors;
+        /**
+         * The total number of executors that are currently on-line, idle and accepting tasks.
+         */
+        private final int availableExecutors;
+        /**
+         * The number of items in the queue.
+         */
+        private final int queueLength;
+
+        private LoadStatisticsSnapshot(
+                int definedExecutors, int onlineExecutors, int connectingExecutors,
+                int busyExecutors, int idleExecutors, int availableExecutors,
+                int queueLength) {
+            this.definedExecutors = definedExecutors;
+            this.onlineExecutors = onlineExecutors;
+            this.connectingExecutors = connectingExecutors;
+            // assert definedExecutors == onlineExecutors + connectingExecutors;
+            this.busyExecutors = busyExecutors;
+            this.idleExecutors = idleExecutors;
+            // assert onlineExecutors == busyExecutors + idleExecutors;
+            this.availableExecutors = availableExecutors;
+            // assert availableExecutors <= idleExecutors;
+            this.queueLength = queueLength;
+        }
+
+        /**
+          * The total number of executors that Jenkins currently knows, this includes all off-line slaves.
+          */
+        @Exported
+        public int getDefinedExecutors() {
+            return definedExecutors;
+        }
+
+        /**
+         * The total number of executors that are currently on-line.
+         */
+        @Exported
+        public int getOnlineExecutors() {
+            return onlineExecutors;
+        }
+
+        /**
+         * The total number of executors that are currently in the process of connecting to Jenkins.
+         */
+        @Exported
+        public int getConnectingExecutors() {
+            return connectingExecutors;
+        }
+
+        /**
+         * The total number of executors that are currently busy running jobs.
+         */
+        @Exported
+        public int getBusyExecutors() {
+            return busyExecutors;
+        }
+
+        /**
+         * The total number of executors that are currently on-line and idle. This includes executors that are
+         * not accepting tasks.
+         */
+        @Exported
+        public int getIdleExecutors() {
+            return idleExecutors;
+        }
+
+        /**
+         * The total number of executors that are currently on-line, idle and accepting tasks.
+         */
+        @Exported
+        public int getAvailableExecutors() {
+            return availableExecutors;
+        }
+
+        /**
+         * The number of items in the queue.
+         */
+        @Exported
+        public int getQueueLength() {
+            return queueLength;
+        }
+
+        /** {@inheritDoc} */
+        @Override
+        public boolean equals(Object o) {
+            if (this == o) {
+                return true;
+            }
+            if (o == null || getClass() != o.getClass()) {
+                return false;
+            }
+
+            LoadStatisticsSnapshot that = (LoadStatisticsSnapshot) o;
+
+            if (availableExecutors != that.availableExecutors) {
+                return false;
+            }
+            if (busyExecutors != that.busyExecutors) {
+                return false;
+            }
+            if (connectingExecutors != that.connectingExecutors) {
+                return false;
+            }
+            if (definedExecutors != that.definedExecutors) {
+                return false;
+            }
+            if (idleExecutors != that.idleExecutors) {
+                return false;
+            }
+            if (onlineExecutors != that.onlineExecutors) {
+                return false;
+            }
+            if (queueLength != that.queueLength) {
+                return false;
+            }
+
+            return true;
+        }
+
+        /** {@inheritDoc} */
+        @Override
+        public int hashCode() {
+            int result = definedExecutors;
+            result = 31 * result + onlineExecutors;
+            result = 31 * result + connectingExecutors;
+            result = 31 * result + busyExecutors;
+            result = 31 * result + idleExecutors;
+            result = 31 * result + availableExecutors;
+            result = 31 * result + queueLength;
+            return result;
+        }
+
+        /** {@inheritDoc} */
+        @Override
+        public String toString() {
+            final StringBuilder sb = new StringBuilder("LoadStatisticsSnapshot{");
+            sb.append("definedExecutors=").append(definedExecutors);
+            sb.append(", onlineExecutors=").append(onlineExecutors);
+            sb.append(", connectingExecutors=").append(connectingExecutors);
+            sb.append(", busyExecutors=").append(busyExecutors);
+            sb.append(", idleExecutors=").append(idleExecutors);
+            sb.append(", availableExecutors=").append(availableExecutors);
+            sb.append(", queueLength=").append(queueLength);
+            sb.append('}');
+            return sb.toString();
+        }
+
+        /**
+         * Use a builder so we can add more stats if needed.
+         * Not thread safe
+         * @since 1.FIXME
+         */
+        public static class Builder {
+            private int definedExecutors;
+            private int onlineExecutors;
+            private int connectingExecutors;
+            private int busyExecutors;
+            private int idleExecutors;
+            private int availableExecutors;
+            private int queueLength;
+
+            public LoadStatisticsSnapshot build() {
+                return new LoadStatisticsSnapshot(
+                        definedExecutors, onlineExecutors, connectingExecutors,
+                        busyExecutors, idleExecutors, availableExecutors,
+                        queueLength
+                );
+            }
+
+            public Builder withQueueLength(int queueLength) {
+                this.queueLength = queueLength;
+                return this;
+            }
+
+            public Builder with(Node node) {
+                if (node != null)
+                return with(node.toComputer());
+                return this;
+            }
+
+            public Builder with(Computer computer) {
+                if (computer.isOnline()) {
+                    final List<Executor> executors = computer.getExecutors();
+                    final boolean acceptingTasks = computer.isAcceptingTasks();
+                    for (Executor e : executors) {
+                        definedExecutors++;
+                        onlineExecutors++;
+                        if (e.getCurrentWorkUnit() != null) {
+                            busyExecutors++;
+                        } else {
+                            idleExecutors++;
+                            if (acceptingTasks) availableExecutors++;
+                        }
+                    }
+                } else {
+                    final int numExecutors = computer.getNumExecutors();
+                    definedExecutors += numExecutors;
+                    if (computer.isConnecting()) {
+                        connectingExecutors += numExecutors;
+                    }
+                }
+                return this;
+            }
+
+        }
+
+        public static Builder builder() {
+            return new Builder();
+        }
+    }
+
 }
diff --git a/core/src/main/java/hudson/model/OverallLoadStatistics.java b/core/src/main/java/hudson/model/OverallLoadStatistics.java
index 7dea1ad956..736a4630f7 100644
--- a/core/src/main/java/hudson/model/OverallLoadStatistics.java
+++ b/core/src/main/java/hudson/model/OverallLoadStatistics.java
@@ -25,6 +25,7 @@ package hudson.model;
 
 import hudson.model.MultiStageTimeSeries.TimeScale;
 import hudson.model.MultiStageTimeSeries.TrendChart;
+import hudson.model.queue.SubTask;
 import jenkins.model.Jenkins;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
@@ -68,11 +69,21 @@ public class OverallLoadStatistics extends LoadStatistics {
         return Jenkins.getInstance().getQueue().countBuildableItems();
     }
 
+    @Override
+    protected Iterable<Node> getNodes() {
+        return Jenkins.getActiveInstance().getNodes();
+    }
+
+    @Override
+    protected boolean matches(SubTask item) {
+        return true;
+    }
+
     /**
      * When drawing the overall load statistics, use the total queue length,
      * 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,queueLength);
+        return MultiStageTimeSeries.createTrendChart(timeScale,busyExecutors,onlineExecutors,queueLength,availableExecutors);
     }
 }
diff --git a/core/src/main/java/hudson/slaves/NodeProvisioner.java b/core/src/main/java/hudson/slaves/NodeProvisioner.java
index 25018f190f..697e9ca115 100644
--- a/core/src/main/java/hudson/slaves/NodeProvisioner.java
+++ b/core/src/main/java/hudson/slaves/NodeProvisioner.java
@@ -225,17 +225,17 @@ public class NodeProvisioner {
         float plannedCapacity = plannedCapacitySnapshot;
         plannedCapacitiesEMA.update(plannedCapacity);
 
-        int idleSnapshot = stat.computeIdleExecutors();
-        int queueLengthSnapshot = stat.computeQueueLength();
+        final LoadStatistics.LoadStatisticsSnapshot snapshot = stat.computeSnapshot();
 
-        if (queueLengthSnapshot <= idleSnapshot) {
+        int availableSnapshot = snapshot.getAvailableExecutors();
+        int queueLengthSnapshot = snapshot.getQueueLength();
+
+        if (queueLengthSnapshot <= availableSnapshot) {
             LOGGER.log(Level.FINE,
-                    "Queue length {0} is less than the idle capacity {1}. No provisioning strategy required",
-                    new Object[]{queueLengthSnapshot, idleSnapshot});
+                    "Queue length {0} is less than the available capacity {1}. No provisioning strategy required",
+                    new Object[]{queueLengthSnapshot, availableSnapshot});
         } else {
-            StrategyState state =
-                    new StrategyState(queueLengthSnapshot, label, idleSnapshot, stat.computeTotalExecutors(),
-                            plannedCapacitySnapshot);
+            StrategyState state = new StrategyState(snapshot, label, plannedCapacitySnapshot);
             List<Strategy> strategies = Jenkins.getInstance().getExtensionList(Strategy.class);
             for (Strategy strategy : strategies.isEmpty()
                     ? Arrays.<Strategy>asList(new StandardStrategyImpl())
@@ -303,22 +303,14 @@ public class NodeProvisioner {
          * The label under consideration.
          */
         private final Label label;
-        /**
-         * The number of items in the queue requiring this {@link #label}.
-         */
-        private final int queueLengthSnapshot;
         /**
          * The planned capacity for this {@link #label}.
          */
         private final int plannedCapacitySnapshot;
         /**
-         * The number of idle executors for this {@link #label}
+         * The current statistics snapshot for this {@link #label}.
          */
-        private final int idleSnapshot;
-        /**
-         * The total number of executors for this {@link #label}
-         */
-        private final int totalSnapshot;
+        private final LoadStatistics.LoadStatisticsSnapshot snapshot;
         private final List<PlannedNode> pendingLaunches;
         /**
          * The additional planned capacity for this {@link #label} and provisioned by previous strategies during the
@@ -329,18 +321,12 @@ public class NodeProvisioner {
 
         /**
          * Should only be instantiated by {@link NodeProvisioner#update()}
-         * @param queueLengthSnapshot the queue length.
          * @param label the label.
-         * @param idleSnapshot the idle executor count.
-         * @param totalSnapshot the totoal executor count.
          * @param plannedCapacitySnapshot the planned executor count.
          */
-        private StrategyState(int queueLengthSnapshot, Label label, int idleSnapshot, int totalSnapshot,
-                              int plannedCapacitySnapshot) {
-            this.queueLengthSnapshot = queueLengthSnapshot;
+        private StrategyState(LoadStatistics.LoadStatisticsSnapshot snapshot, Label label, int plannedCapacitySnapshot) {
+            this.snapshot = snapshot;
             this.label = label;
-            this.idleSnapshot = idleSnapshot;
-            this.totalSnapshot = totalSnapshot;
             this.plannedCapacitySnapshot = plannedCapacitySnapshot;
             pendingLaunches = NodeProvisioner.this.pendingLaunches;
         }
@@ -352,11 +338,20 @@ public class NodeProvisioner {
             return label;
         }
 
+        /**
+         * The current snapshot of the load statistics for this {@link #getLabel()}.
+         * @since 1.FIXME
+         */
+        public LoadStatistics.LoadStatisticsSnapshot getSnapshot() {
+            return snapshot;
+        }
+
         /**
          * The number of items in the queue requiring this {@link #getLabel()}.
+         * @deprecated use {@link #getSnapshot()}, {@link LoadStatistics.LoadStatisticsSnapshot#getQueueLength()}
          */
         public int getQueueLengthSnapshot() {
-            return queueLengthSnapshot;
+            return snapshot.getQueueLength();
         }
 
         /**
@@ -368,16 +363,18 @@ public class NodeProvisioner {
 
         /**
          * The number of idle executors for this {@link #getLabel()}
+         * @deprecated use {@link #getSnapshot()}, {@link LoadStatistics.LoadStatisticsSnapshot#getAvailableExecutors()}
          */
         public int getIdleSnapshot() {
-            return idleSnapshot;
+            return snapshot.getAvailableExecutors();
         }
 
         /**
          * The total number of executors for this {@link #getLabel()}
+         * @deprecated use {@link #getSnapshot()}, {@link LoadStatistics.LoadStatisticsSnapshot#getOnlineExecutors()}
          */
         public int getTotalSnapshot() {
-            return totalSnapshot;
+            return snapshot.getOnlineExecutors();
         }
 
         /**
@@ -404,16 +401,66 @@ public class NodeProvisioner {
 
         /**
          * The time series average number of idle executors for this {@link #getLabel()}
+         * @deprecated use {@link #getAvailableExecutorsLatest()}
          */
         public float getIdleLatest() {
-            return stat.getLatestIdleExecutors(TIME_SCALE);
+            return getAvailableExecutorsLatest();
         }
 
         /**
          * The time series average total number of executors for this {@link #getLabel()}
+         * @deprecated use {@link #getOnlineExecutorsLatest()}
          */
         public float getTotalLatest() {
-            return stat.totalExecutors.getLatest(TIME_SCALE);
+            return getOnlineExecutorsLatest();
+        }
+
+        /**
+         * The time series average number of defined executors for this {@link #getLabel()}
+         * @since 1.FIXME
+         */
+        public float getDefinedExecutorsLatest() {
+            return stat.definedExecutors.getLatest(TIME_SCALE);
+        }
+
+        /**
+         * The time series average number of online executors for this {@link #getLabel()}
+         * @since 1.FIXME
+         */
+        public float getOnlineExecutorsLatest() {
+            return stat.onlineExecutors.getLatest(TIME_SCALE);
+        }
+
+        /**
+         * The time series average number of connecting executors for this {@link #getLabel()}
+         * @since 1.FIXME
+         */
+        public float getConnectingExecutorsLatest() {
+            return stat.connectingExecutors.getLatest(TIME_SCALE);
+        }
+
+        /**
+         * The time series average number of busy executors for this {@link #getLabel()}
+         * @since 1.FIXME
+         */
+        public float getBusyExecutorsLatest() {
+            return stat.busyExecutors.getLatest(TIME_SCALE);
+        }
+
+        /**
+         * The time series average number of idle executors for this {@link #getLabel()}
+         * @since 1.FIXME
+         */
+        public float getIdleExecutorsLatest() {
+            return stat.idleExecutors.getLatest(TIME_SCALE);
+        }
+
+        /**
+         * The time series average number of available executors for this {@link #getLabel()}
+         * @since 1.FIXME
+         */
+        public float getAvailableExecutorsLatest() {
+            return stat.availableExecutors.getLatest(TIME_SCALE);
         }
 
         /**
@@ -468,10 +515,8 @@ public class NodeProvisioner {
         public String toString() {
             final StringBuilder sb = new StringBuilder("StrategyState{");
             sb.append("label=").append(label);
-            sb.append(", queueLengthSnapshot=").append(queueLengthSnapshot);
+            sb.append(", snapshot=").append(snapshot);
             sb.append(", plannedCapacitySnapshot=").append(plannedCapacitySnapshot);
-            sb.append(", idleSnapshot=").append(idleSnapshot);
-            sb.append(", totalSnapshot=").append(totalSnapshot);
             sb.append(", additionalPlannedCapacity=").append(additionalPlannedCapacity);
             sb.append('}');
             return sb.toString();
@@ -522,21 +567,24 @@ public class NodeProvisioner {
             estimate won't create a starvation.
          */
 
-            boolean needSomeWhenNoneAtAll = (state.getIdleSnapshot() == 0)
-                    && (state.getTotalSnapshot() + state.getPlannedCapacitySnapshot() + state.getAdditionalPlannedCapacity() == 0)
-                    && (state.getQueueLengthSnapshot() > 0);
-            float idle = Math.max(state.getIdleLatest(), state.getIdleSnapshot());
-            if (idle < MARGIN || needSomeWhenNoneAtAll) {
+            final LoadStatistics.LoadStatisticsSnapshot snapshot = state.getSnapshot();
+            boolean needSomeWhenNoneAtAll = (snapshot.getAvailableExecutors() + snapshot.getConnectingExecutors() == 0)
+                    && (snapshot.getOnlineExecutors() + state.getPlannedCapacitySnapshot() + state.getAdditionalPlannedCapacity() == 0)
+                    && (snapshot.getQueueLength() > 0);
+            float available = Math.max(snapshot.getAvailableExecutors(), state.getAvailableExecutorsLatest());
+            if (available < MARGIN || needSomeWhenNoneAtAll) {
                 // make sure the system is fully utilized before attempting any new launch.
 
                 // this is the amount of work left to be done
-                float qlen = Math.min(state.getQueueLengthLatest(), state.getQueueLengthSnapshot());
+                float qlen = Math.min(state.getQueueLengthLatest(), snapshot.getQueueLength());
+
+                float connectingCapacity = Math.min(state.getConnectingExecutorsLatest(), snapshot.getConnectingExecutors());
 
                 // ... and this is the additional executors we've already provisioned.
                 float plannedCapacity = Math.max(state.getPlannedCapacityLatest(), state.getPlannedCapacitySnapshot())
                         + state.getAdditionalPlannedCapacity();
 
-                float excessWorkload = qlen - plannedCapacity;
+                float excessWorkload = qlen - plannedCapacity - connectingCapacity;
                 if (needSomeWhenNoneAtAll && excessWorkload < 1) {
                     // in this specific exceptional case we should just provision right now
                     // the exponential smoothing will delay the build unnecessarily
@@ -545,12 +593,12 @@ public class NodeProvisioner {
                 float m = calcThresholdMargin(state.getTotalSnapshot());
                 if (excessWorkload > 1 - m) {// and there's more work to do...
                     LOGGER.log(Level.FINE, "Excess workload {0,number,#.###} detected. "
-                                    + "(planned capacity={1,number,#.###},"
-                                    + "Qlen={2,number,#.###},idle={3,number,#.###}&{4,number,integer},"
-                                    + "total={5,number,integer},m={6,number,#.###})",
+                                    + "(planned capacity={1,number,#.###},connecting capacity={7,number,#.###},"
+                                    + "Qlen={2,number,#.###},available={3,number,#.###}&{4,number,integer},"
+                                    + "online={5,number,integer},m={6,number,#.###})",
                             new Object[]{
-                                    excessWorkload, plannedCapacity, qlen, idle, state.getIdleSnapshot(),
-                                    state.getTotalSnapshot(), m
+                                    excessWorkload, plannedCapacity, qlen, available, snapshot.getAvailableExecutors(),
+                                    snapshot.getOnlineExecutors(), m , snapshot.getConnectingExecutors()
                             });
 
                     CLOUD:
diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java
index d1084bd202..3c4f5d493c 100644
--- a/core/src/main/java/jenkins/model/Jenkins.java
+++ b/core/src/main/java/jenkins/model/Jenkins.java
@@ -3492,8 +3492,7 @@ public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve
                     LOGGER.severe(String.format("Shutting down VM as requested by %s from %s",
                                                 exitUser, exitAddr));
                     // Wait 'til we have no active executors.
-                    while (isQuietingDown
-                           && (overallLoad.computeTotalExecutors() > overallLoad.computeIdleExecutors())) {
+                    while (isQuietingDown && (overallLoad.computeSnapshot().getBusyExecutors() > 0)) {
                         Thread.sleep(5000);
                     }
                     // Make sure isQuietingDown is still true.
diff --git a/core/src/main/java/jenkins/model/UnlabeledLoadStatistics.java b/core/src/main/java/jenkins/model/UnlabeledLoadStatistics.java
index 7809356ecb..5ce2d63d6c 100644
--- a/core/src/main/java/jenkins/model/UnlabeledLoadStatistics.java
+++ b/core/src/main/java/jenkins/model/UnlabeledLoadStatistics.java
@@ -29,6 +29,14 @@ import hudson.model.Node;
 import hudson.model.Node.Mode;
 import hudson.model.OverallLoadStatistics;
 import hudson.model.Queue.Task;
+import hudson.model.queue.SubTask;
+import hudson.util.Iterators;
+
+import java.util.Iterator;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.NoSuchElementException;
+import java.util.Set;
 
 /**
  * {@link LoadStatistics} that track the "free roam" jobs (whose {@link Task#getAssignedLabel()} is null)
@@ -41,6 +49,8 @@ import hudson.model.Queue.Task;
  */
 public class UnlabeledLoadStatistics extends LoadStatistics {
 
+    private final Iterable<Node> nodes = new UnlabeledNodesIterable();
+
     UnlabeledLoadStatistics() {
         super(0, 0);
     }
@@ -73,4 +83,42 @@ public class UnlabeledLoadStatistics extends LoadStatistics {
     public int computeQueueLength() {
         return Jenkins.getInstance().getQueue().countBuildableItemsFor(null);
     }
+
+    @Override
+    protected Iterable<Node> getNodes() {
+        return nodes;
+    }
+
+    @Override
+    protected boolean matches(SubTask item) {
+        return true;
+    }
+
+    private static class UnlabeledNodesIterable implements Iterable<Node> {
+
+        @Override
+        public Iterator<Node> iterator() {
+            return new UnlabeledNodesIterator();
+        }
+    }
+
+    private static class UnlabeledNodesIterator extends Iterators.FilterIterator<Node> {
+
+        protected UnlabeledNodesIterator() {
+            super(Jenkins.getActiveInstance().getNodes().iterator());
+        }
+
+        @Override
+        protected boolean filter(Node n) {
+            return n != null && n.getMode() == Mode.NORMAL;
+        }
+
+        public void remove() {
+            // why does Iterators.FilterIterator do the stupid thing and allow remove?
+            // (remove should remove the object last returned by next(), but it won't if hasNext() is called
+            // the way Iterators.FilterIterator is written... it should just return a read-only
+            // view... which is what we do!
+            throw new UnsupportedOperationException("remove");
+        }
+    }
 }
diff --git a/core/src/main/resources/hudson/model/Messages.properties b/core/src/main/resources/hudson/model/Messages.properties
index 1629372412..5eacc569cd 100644
--- a/core/src/main/resources/hudson/model/Messages.properties
+++ b/core/src/main/resources/hudson/model/Messages.properties
@@ -322,8 +322,13 @@ ListView.DisplayName=List View
 
 MyView.DisplayName=My View
 
+LoadStatistics.Legends.DefinedExecutors=Defined executors
+LoadStatistics.Legends.OnlineExecutors=Online executors
+LoadStatistics.Legends.ConnectingExecutors=Connecting executors
 LoadStatistics.Legends.TotalExecutors=Total executors
 LoadStatistics.Legends.BusyExecutors=Busy executors
+LoadStatistics.Legends.IdleExecutors=Idle executors
+LoadStatistics.Legends.AvailableExecutors=Available executors
 LoadStatistics.Legends.QueueLength=Queue length
 
 Cause.LegacyCodeCause.ShortDescription=Legacy code started this job.  No cause information is available
diff --git a/core/src/test/java/hudson/model/LoadStatisticsTest.java b/core/src/test/java/hudson/model/LoadStatisticsTest.java
index 57fc4889c7..b23bf51c35 100644
--- a/core/src/test/java/hudson/model/LoadStatisticsTest.java
+++ b/core/src/test/java/hudson/model/LoadStatisticsTest.java
@@ -24,6 +24,8 @@
 package hudson.model;
 
 import hudson.model.MultiStageTimeSeries.TimeScale;
+import hudson.model.queue.SubTask;
+import junit.framework.TestCase;
 
 import org.apache.commons.io.IOUtils;
 import org.jfree.chart.JFreeChart;
@@ -54,17 +56,28 @@ public class LoadStatisticsTest {
             public int computeQueueLength() {
                 throw new UnsupportedOperationException();
             }
+            @Override
+            protected Iterable<Node> getNodes() {
+                throw new UnsupportedOperationException();
+            }
+
+            @Override
+            protected boolean matches(SubTask item) {
+                throw new UnsupportedOperationException();
+            }
         };
 
         for (int i = 0; i < 50; i++) {
-            ls.totalExecutors.update(4);
+            ls.onlineExecutors.update(4);
             ls.busyExecutors.update(3);
+            ls.availableExecutors.update(1);
             ls.queueLength.update(3);
         }
 
         for (int i = 0; i < 50; i++) {
-            ls.totalExecutors.update(0);
+            ls.onlineExecutors.update(0);
             ls.busyExecutors.update(0);
+            ls.availableExecutors.update(0);
             ls.queueLength.update(1);
         }
 

From b8a4a532455817f4bf723585763ec16569e1c4bf Mon Sep 17 00:00:00 2001
From: Stephen Connolly <stephen.alan.connolly@gmail.com>
Date: Wed, 25 Mar 2015 13:05:46 +0000
Subject: [PATCH 2/2] Arg! cannot use Util.isOverridden as these are protected
 methods

---
 .../java/hudson/model/LoadStatistics.java     | 33 ++++++++++++++-
 .../java/hudson/model/LoadStatisticsTest.java | 42 ++++++++++++++++++-
 2 files changed, 72 insertions(+), 3 deletions(-)

diff --git a/core/src/main/java/hudson/model/LoadStatistics.java b/core/src/main/java/hudson/model/LoadStatistics.java
index 5d4640c24a..7d2f514d9a 100644
--- a/core/src/main/java/hudson/model/LoadStatistics.java
+++ b/core/src/main/java/hudson/model/LoadStatistics.java
@@ -49,6 +49,8 @@ import org.kohsuke.stapler.export.Exported;
 import java.awt.*;
 import java.io.IOException;
 import java.io.Serializable;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
 import java.util.List;
 
 /**
@@ -148,8 +150,35 @@ public abstract class LoadStatistics {
         this.queueLength = new MultiStageTimeSeries(
                 Messages._LoadStatistics_Legends_QueueLength(),ColorPalette.GREY, 0, DECAY);
         this.totalExecutors = onlineExecutors;
-        modern = Util.isOverridden(LoadStatistics.class, getClass(), "getNodes")
-                && Util.isOverridden(LoadStatistics.class, getClass(), "matches", SubTask.class);
+        modern = isModern(getClass());
+    }
+
+    /*package*/ static boolean isModern(Class<? extends LoadStatistics> clazz) {
+        // cannot use Util.isOverridden as these are protected methods.
+        boolean hasGetNodes = false;
+        boolean hasMatches = false;
+        while (clazz != LoadStatistics.class && clazz != null && !(hasGetNodes && hasMatches)) {
+            if (!hasGetNodes) {
+                try {
+                    final Method getNodes = clazz.getDeclaredMethod("getNodes");
+                    hasGetNodes = !Modifier.isAbstract(getNodes.getModifiers());
+                } catch (NoSuchMethodException e) {
+                    // ignore
+                }
+            }
+            if (!hasMatches) {
+                try {
+                    final Method getNodes = clazz.getDeclaredMethod("matches", SubTask.class);
+                    hasMatches = !Modifier.isAbstract(getNodes.getModifiers());
+                } catch (NoSuchMethodException e) {
+                    // ignore
+                }
+            }
+            if (!(hasGetNodes && hasMatches) && LoadStatistics.class.isAssignableFrom(clazz.getSuperclass())) {
+                clazz = (Class<? extends LoadStatistics>) clazz.getSuperclass();
+            }
+        }
+        return hasGetNodes && hasMatches;
     }
 
     /**
diff --git a/core/src/test/java/hudson/model/LoadStatisticsTest.java b/core/src/test/java/hudson/model/LoadStatisticsTest.java
index b23bf51c35..f2eeee4aed 100644
--- a/core/src/test/java/hudson/model/LoadStatisticsTest.java
+++ b/core/src/test/java/hudson/model/LoadStatisticsTest.java
@@ -25,7 +25,6 @@ package hudson.model;
 
 import hudson.model.MultiStageTimeSeries.TimeScale;
 import hudson.model.queue.SubTask;
-import junit.framework.TestCase;
 
 import org.apache.commons.io.IOUtils;
 import org.jfree.chart.JFreeChart;
@@ -37,6 +36,9 @@ import java.io.File;
 import java.io.FileOutputStream;
 import java.io.IOException;
 
+import static org.hamcrest.CoreMatchers.is;
+import static org.junit.Assert.assertThat;
+
 /**
  * @author Kohsuke Kawaguchi
  */
@@ -93,4 +95,42 @@ public class LoadStatisticsTest {
             tempFile.delete();
         }
     }
+
+    @Test
+    public void isModernWorks() throws Exception {
+        assertThat(LoadStatistics.isModern(Modern.class), is(true));
+        assertThat(LoadStatistics.isModern(LoadStatistics.class), is(false));
+    }
+
+    private class Modern extends LoadStatistics {
+
+        protected Modern(int initialOnlineExecutors, int initialBusyExecutors) {
+            super(initialOnlineExecutors, initialBusyExecutors);
+        }
+
+        @Override
+        public int computeIdleExecutors() {
+            return 0;
+        }
+
+        @Override
+        public int computeTotalExecutors() {
+            return 0;
+        }
+
+        @Override
+        public int computeQueueLength() {
+            return 0;
+        }
+
+        @Override
+        protected Iterable<Node> getNodes() {
+            return null;
+        }
+
+        @Override
+        protected boolean matches(SubTask item) {
+            return false;
+        }
+    }
 }