Speed up Jenkins._cleanUpDisconnectComputers (#11102)

Author: krisstern

core/src/main/java/hudson/model/AbstractCIBase.java

@@ -94,10 +94,6 @@ protected void interruptReloadThread() {
         ViewJob.interruptReloadThread();
     }
 
-    protected void killComputer(Computer c) {
-        c.kill();
-    }
-
     private final Set<String> disabledAdministrativeMonitors = new HashSet<>();
 
     /**
@@ -267,12 +263,12 @@ protected void updateComputerList(final boolean automaticAgentLaunch, @NonNull C
             // we need to start the process of reducing the executors on all computers as distinct
             // from the killing action which should not excessively use the Queue lock.
             for (Computer c : old) {
-                c.inflictMortalWound();
+                c.setNumExecutors(0);
             }
         });
         for (Computer c : old) {
             // when we get to here, the number of executors should be zero so this call should not need the Queue.lock
-            killComputer(c);
+            c.kill();
         }
         getQueue().scheduleMaintenance();
         Listeners.notify(ComputerListener.class, false, ComputerListener::onConfigurationChange);

core/src/main/java/hudson/model/Computer.java

@@ -827,23 +827,6 @@ protected void kill() {
         setNumExecutors(0);
     }
 
-    /**
-     * Called by {@link Jenkins#updateComputerList(boolean, Collection)} to notify {@link Computer} that it will be discarded.
-     *
-     * <p>
-     * Note that at this point {@link #getNode()} returns null.
-     *
-     * <p>
-     * Note that the Queue lock is already held when this method is called.
-     *
-     * @see #onRemoved()
-     */
-    @Restricted(NoExternalUse.class)
-    @GuardedBy("hudson.model.Queue.lock")
-    /*package*/ void inflictMortalWound() {
-        setNumExecutors(0);
-    }
-
     /**
      * Called by {@link Jenkins} when this computer is removed.
      *
@@ -865,16 +848,17 @@ protected void onRemoved(){
      * Calling path, *means protected by Queue.withLock
      *
      * Computer.doConfigSubmit -> Computer.replaceBy ->Jenkins.setNodes* ->Computer.setNode
-     * AbstractCIBase.updateComputerList->Computer.inflictMortalWound*
+     * AbstractCIBase.updateComputerList->Computer.setNumExecutors*
      * AbstractCIBase.updateComputerList->AbstractCIBase.updateComputer* ->Computer.setNode
      * AbstractCIBase.updateComputerList->AbstractCIBase.killComputer->Computer.kill
      * Computer.constructor->Computer.setNode
      * Computer.kill is called after numExecutors set to zero(Computer.inflictMortalWound) so not need the Queue.lock
      *
      * @param n number of executors
      */
+    @Restricted(NoExternalUse.class)
     @GuardedBy("hudson.model.Queue.lock")
-    private void setNumExecutors(int n) {
+    public void setNumExecutors(int n) {
         this.numExecutors = n;
         final int diff = executors.size() - n;
 

core/src/main/java/hudson/slaves/SlaveComputer.java

@@ -916,19 +916,23 @@ public Object getTarget() {
     protected void kill() {
         super.kill();
         closeChannel();
-        try {
-            log.close();
-        } catch (IOException x) {
-            LOGGER.log(Level.WARNING, "Failed to close agent log", x);
-        }
-
+        closeLog();
         try {
             Util.deleteRecursive(getLogDir());
         } catch (IOException ex) {
             logger.log(Level.WARNING, "Unable to delete agent logs", ex);
         }
     }
 
+    @Restricted(NoExternalUse.class)
+    public void closeLog() {
+        try {
+            log.close();
+        } catch (IOException x) {
+            LOGGER.log(Level.WARNING, "Failed to close agent log", x);
+        }
+    }
+
     @Override
     public RetentionStrategy getRetentionStrategy() {
         Slave n = getNode();

core/src/main/java/jenkins/model/Jenkins.java

@@ -176,6 +176,7 @@
 import hudson.slaves.NodeProvisioner;
 import hudson.slaves.OfflineCause;
 import hudson.slaves.RetentionStrategy;
+import hudson.slaves.SlaveComputer;
 import hudson.tasks.BuildWrapper;
 import hudson.tasks.Builder;
 import hudson.tasks.Publisher;
@@ -3775,7 +3776,10 @@ private Set<Future<?>> _cleanUpDisconnectComputers(final List<Throwable> errors)
             for (Computer c : getComputersCollection()) {
                 try {
                     c.interrupt();
-                    killComputer(c);
+                    c.setNumExecutors(0);
+                    if (Main.isUnitTest && c instanceof SlaveComputer sc) {
+                        sc.closeLog(); // help TemporaryDirectoryAllocator.dispose esp. on Windows
+                    }
                     pending.add(c.disconnect(null));
                 } catch (OutOfMemoryError e) {
                     // we should just propagate this, no point trying to log
@@ -3950,9 +3954,15 @@ private void _cleanUpAwaitDisconnects(List<Throwable> errors, Set<Future<?>> pen
         if (!pending.isEmpty()) {
             LOGGER.log(Main.isUnitTest ? Level.FINE : Level.INFO, "Waiting for node disconnection completion");
         }
+        long end = System.nanoTime() + Duration.ofSeconds(10).toNanos();
         for (Future<?> f : pending) {
             try {
-                f.get(10, TimeUnit.SECONDS);    // if clean up operation didn't complete in time, we fail the test
+                long remaining = end - System.nanoTime();
+                if (remaining <= 0) {
+                    LOGGER.warning("Ran out of time waiting for agents to disconnect");
+                    break;
+                }
+                f.get(remaining, TimeUnit.NANOSECONDS);
             } catch (InterruptedException e) {
                 Thread.currentThread().interrupt();
                 break;  // someone wants us to die now. quick!