[JENKINS-76171] fix overprovisioning of ec2 agents when rapidly scheduling nodes (#1149)

mikecirioli · apuig · web-flow · commit 95ca008ed92a · 2025-10-10T21:13:28.000+08:00
* initial impl fixing overprovisioning * not needed * tests * cleanup * noop * Apply suggestion from @apuig Co-authored-by: Albert Puig <apuig@cloudbees.com> * better test readability * missed commit --------- Co-authored-by: Albert Puig <apuig@cloudbees.com>
diff --git a/src/main/java/hudson/plugins/ec2/EC2Cloud.java b/src/main/java/hudson/plugins/ec2/EC2Cloud.java
@@ -1189,10 +1189,11 @@ public Node call() throws Exception {
 
                             InstanceStateName state = instance.state().name();
                             if (state.equals(InstanceStateName.RUNNING)) {
-                                // Spot instance are not reconnected automatically,
-                                // but could be new orphans that has the option enable
+                                // JENKINS-76171: Always initiate connection when instance is RUNNING
+                                // This reduces the gap between PlannedNode completion and agent connection,
+                                // preventing over-provisioning in NodeProvisioner
                                 Computer c = slave.toComputer();
-                                if (slave.getStopOnTerminate() && (c != null)) {
+                                if (c != null) {
                                     c.connect(false);
                                 }
 
diff --git a/src/main/java/hudson/plugins/ec2/NoDelayProvisionerStrategy.java b/src/main/java/hudson/plugins/ec2/NoDelayProvisionerStrategy.java
@@ -1,8 +1,11 @@
 package hudson.plugins.ec2;
 
+import com.google.common.annotations.VisibleForTesting;
 import hudson.Extension;
+import hudson.model.Computer;
 import hudson.model.Label;
 import hudson.model.LoadStatistics;
+import hudson.model.Node;
 import hudson.slaves.Cloud;
 import hudson.slaves.NodeProvisioner;
 import java.util.Collection;
@@ -27,12 +30,22 @@ public NodeProvisioner.StrategyDecision apply(NodeProvisioner.StrategyState stra
         final Label label = strategyState.getLabel();
 
         LoadStatistics.LoadStatisticsSnapshot snapshot = strategyState.getSnapshot();
-        int availableCapacity = snapshot.getAvailableExecutors() // live executors
+
+        // JENKINS-76171: Count provisioned EC2 nodes that exist but haven't started executing jobs yet.
+        // This prevents over-provisioning by accounting for nodes in the gap between:
+        // 1) PlannedNode future completing (instance RUNNING)
+        // 2) Agent showing as "connecting" in the snapshot
+        // 3) Agent executing jobs
+        int provisionedButNotExecuting = countProvisionedButNotExecutingNodes(label);
+
+        int availableCapacity = snapshot.getAvailableExecutors() // live executors (idle)
                 + snapshot.getConnectingExecutors() // executors present but not yet connected
                 + strategyState
                         .getPlannedCapacitySnapshot() // capacity added by previous strategies from previous rounds
-                + strategyState.getAdditionalPlannedCapacity(); // capacity added by previous strategies _this round_
+                + strategyState.getAdditionalPlannedCapacity() // capacity added by previous strategies _this round_
+                + provisionedButNotExecuting; // EC2 nodes that exist but aren't yet counted above
         int currentDemand = snapshot.getQueueLength();
+
         LOGGER.log(
                 Level.FINE, "Available capacity={0}, currentDemand={1}", new Object[] {availableCapacity, currentDemand
                 });
@@ -49,8 +62,12 @@ public NodeProvisioner.StrategyDecision apply(NodeProvisioner.StrategyState stra
                     continue;
                 }
 
+                int numToProvision = currentDemand - availableCapacity;
+                LOGGER.log(Level.FINE, "Planned {0} new nodes", numToProvision);
+
                 Collection<NodeProvisioner.PlannedNode> plannedNodes =
-                        cloud.provision(new Cloud.CloudState(label, 0), currentDemand - availableCapacity);
+                        cloud.provision(new Cloud.CloudState(label, 0), numToProvision);
+
                 LOGGER.log(Level.FINE, "Planned {0} new nodes", plannedNodes.size());
                 strategyState.recordPendingLaunches(plannedNodes);
                 availableCapacity += plannedNodes.size();
@@ -68,4 +85,68 @@ public NodeProvisioner.StrategyDecision apply(NodeProvisioner.StrategyState stra
             return NodeProvisioner.StrategyDecision.CONSULT_REMAINING_STRATEGIES;
         }
     }
+
+    /**
+     * Counts executors in EC2 nodes that have been provisioned (exist in Jenkins) but are NOT yet counted in the
+     * LoadStatistics snapshot. This specifically targets the gap where nodes exist but are:
+     * - Offline (just added to Jenkins, before connecting starts)
+     *
+     * We explicitly DO NOT count:
+     * - Connecting nodes (already in snapshot.getConnectingExecutors())
+     * - Online nodes (already in snapshot.getAvailableExecutors() or busy executors)
+     *
+     * This prevents over-provisioning by accounting for nodes in the critical gap between:
+     * 1) Node added to Jenkins (after PlannedNode future completes)
+     * 2) Node starts connecting (shows up in snapshot.getConnectingExecutor())
+     *
+     * @param label the label to match, or null for unlabeled nodes
+     * @return the number of executors from provisioned EC2 nodes in the offline->connecting gap
+     */
+    @VisibleForTesting
+    int countProvisionedButNotExecutingNodes(Label label) {
+        Jenkins jenkins = Jenkins.get();
+        // Use Label.getNodes() to leverage core's label matching and caching
+        java.util.Set<Node> nodes = (label != null) ? label.getNodes() : java.util.Set.copyOf(jenkins.getNodes());
+
+        int count = 0;
+        int totalEC2Nodes = 0;
+        int offlineNodes = 0;
+        int connectingNodes = 0;
+        int onlineNodes = 0;
+
+        for (Node node : nodes) {
+            // Only count EC2 nodes
+            if (!(node instanceof EC2AbstractSlave)) {
+                continue;
+            }
+            totalEC2Nodes++;
+
+            Computer computer = node.toComputer();
+            if (computer == null) {
+                continue;
+            }
+
+            // Track node states for debugging
+            if (computer.isOnline()) {
+                onlineNodes++;
+            } else if (computer.isConnecting()) {
+                connectingNodes++;
+            } else if (computer.isOffline()) {
+                offlineNodes++;
+            }
+
+            // Only count nodes that are OFFLINE (not connecting, not online)
+            // These are in the gap between being added to Jenkins and starting to connect
+            if (computer.isOffline() && !computer.isConnecting()) {
+                count += node.getNumExecutors();
+            }
+        }
+
+        LOGGER.log(
+                Level.FINER,
+                "EC2 nodes for label {0}: total={1}, offline={2}, connecting={3}, online={4}",
+                new Object[] {label, totalEC2Nodes, offlineNodes, connectingNodes, onlineNodes});
+
+        return count;
+    }
 }
diff --git a/src/main/java/hudson/plugins/ec2/util/MinimumInstanceChecker.java b/src/main/java/hudson/plugins/ec2/util/MinimumInstanceChecker.java
@@ -19,6 +19,7 @@
 import java.util.Objects;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
+import java.util.logging.Level;
 import java.util.logging.Logger;
 import java.util.stream.Stream;
 import jenkins.model.Jenkins;
@@ -74,8 +75,30 @@ public static int countQueueItemsForAgentTemplate(@NonNull SlaveTemplate agentTe
                 .count();
     }
 
-    public static void checkForMinimumInstances() {
-        Jenkins.get().clouds.stream()
+    /**
+     * Checks all EC2 cloud templates and provisions agents to meet minimum instance requirements.
+     * Synchronized to prevent concurrent provisioning decisions that could lead to over-provisioning
+     * when multiple agents accept tasks simultaneously.
+     *
+     * @see <a href="https://issues.jenkins.io/browse/JENKINS-76171">JENKINS-76171</a>
+     */
+    public static synchronized void checkForMinimumInstances() {
+        Jenkins jenkins = Jenkins.get();
+
+        // Early exit if no templates have minimum instance requirements
+        boolean hasMinimumRequirements = jenkins.clouds.stream()
+                .filter(EC2Cloud.class::isInstance)
+                .map(EC2Cloud.class::cast)
+                .flatMap(cloud -> cloud.getTemplates().stream())
+                .anyMatch(template ->
+                        template.getMinimumNumberOfInstances() > 0 || template.getMinimumNumberOfSpareInstances() > 0);
+
+        if (!hasMinimumRequirements) {
+            // No templates require minimum instances - exit immediately
+            return;
+        }
+
+        jenkins.clouds.stream()
                 .filter(EC2Cloud.class::isInstance)
                 .map(EC2Cloud.class::cast)
                 .forEach(cloud -> cloud.getTemplates().forEach(agentTemplate -> {
@@ -104,15 +127,25 @@ public static void checkForMinimumInstances() {
                     // Check if we need to provision any agents because we
                     // don't have the minimum number of spare agents.
                     // Don't double provision if minAgents and minSpareAgents are set.
-                    provisionForMinSpareAgents = (requiredMinSpareAgents + currentBuildsWaitingForTemplate)
-                            - (currentNumberOfSpareAgentsForTemplate
-                                    + provisionForMinAgents
-                                    + currentNumberOfProvisioningAgentsForTemplate);
-                    if (provisionForMinSpareAgents < 0) {
-                        provisionForMinSpareAgents = 0;
+                    if (requiredMinSpareAgents > 0) {
+                        provisionForMinSpareAgents = (requiredMinSpareAgents + currentBuildsWaitingForTemplate)
+                                - (currentNumberOfSpareAgentsForTemplate
+                                        + provisionForMinAgents
+                                        + currentNumberOfProvisioningAgentsForTemplate);
+                        if (provisionForMinSpareAgents < 0) {
+                            provisionForMinSpareAgents = 0;
+                        }
                     }
 
                     int numberToProvision = provisionForMinAgents + provisionForMinSpareAgents;
+
+                    if (numberToProvision > 0 || requiredMinAgents > 0 || requiredMinSpareAgents > 0) {
+                        LOGGER.log(
+                                Level.FINE,
+                                "MinimumInstanceChecker for template {0}: toProvision={1}",
+                                new Object[] {agentTemplate.description, numberToProvision});
+                    }
+
                     if (numberToProvision > 0) {
                         cloud.provision(agentTemplate, numberToProvision);
                     }
diff --git a/src/test/java/hudson/plugins/ec2/NoDelayProvisionerStrategyTest.java b/src/test/java/hudson/plugins/ec2/NoDelayProvisionerStrategyTest.java
