Fixing JENKINS-28403 and JENKINS-27471 (WIP)

jenkinsci · Nov 13, 2016 · f141bc7 · f141bc7
1 parent 9031b82
commit f141bc7
Show file tree

Hide file tree

Showing 4 changed files with 87 additions and 30 deletions.
diff --git a/jclouds-plugin/src/main/java/jenkins/plugins/jclouds/compute/JCloudsComputer.java b/jclouds-plugin/src/main/java/jenkins/plugins/jclouds/compute/JCloudsComputer.java
@@ -7,6 +7,7 @@
 import jenkins.model.Jenkins;
 
 import java.io.IOException;
+import java.util.logging.Level;
 import java.util.logging.Logger;
 
 import javax.annotation.CheckForNull;
@@ -43,34 +44,73 @@ public String getCloudName() {
     }
 
     /**
-     * Really deletes the slave, after terminating the instance.
+     * Deletes a jenkins slave node.
+     * The not is first marked pending delete and the actual deletion will
+     * be performed at the next run of {@link JCloudsCleanupThread}.
+     * If called again after already being marked, the deletion is
+     * performed immediately.
      */
     @Override
     public HttpResponse doDoDelete() throws IOException {
         disconnect(OfflineCause.create(Messages._DeletedCause()));
         final JCloudsSlave node = getNode();
         if (null != node) {
-            node.setPendingDelete(true);
+            if (node.isPendingDelete()) {
+                // User attempts to delete an already delete-pending slave
+                LOGGER.info("Slave already pendig delete: " + getName());
+                deleteSlave(true);
+            } else {
+                node.setPendingDelete(true);
+            }
         }
         return new HttpRedirect("..");
     }
 
     /**
-     * Delete the slave, terminate the instance. Can be called either by doDoDelete() or from JCloudsRetentionStrategy.
+     * Delete the slave, terminate or suspend the instance.
+     * Can be called either by doDoDelete() or from JCloudsRetentionStrategy.
+     * Whether the instance gets terminated or suspended is handled in
+     * {@link JCloudsSlave#_terminate}
      *
      * @throws InterruptedException if the deletion gets interrupted.
      * @throws IOException if an error occurs.
      */
     public void deleteSlave() throws IOException, InterruptedException {
-        LOGGER.info("Terminating " + getName() + " slave");
-        JCloudsSlave slave = getNode();
-        if (null != slave ) {
-            final VirtualChannel ch = slave.getChannel();
-            if (null != ch) {
-                ch.close();
+        if (isIdle()) { // Fixes JENKINS-27471
+            LOGGER.info("Deleting slave: " + getName());
+            JCloudsSlave slave = getNode();
+            if (null != slave ) {
+                final VirtualChannel ch = slave.getChannel();
+                if (null != ch) {
+                    ch.close();
+                }
+                slave.terminate();
+                Jenkins.getInstance().removeNode(slave);
+            }
+        } else {
+            LOGGER.info(String.format("Slave %s is not idle, postponing deletion", getName()));
+            // Fixes JENKINS-28403
+            final JCloudsSlave node = getNode();
+            if (null != node && !node.isPendingDelete()) {
+                node.setPendingDelete(true);
+            }
+        }
+    }
+
+    /**
+     * Delete the slave, terminate or suspend the instance.
+     * Like {@link #deleteSlave}, but catching all exceptions and logging the if desired.
+     *
+     * @param logging {@code true}, if exception logging is desired.
+     */
+    public void deleteSlave(final boolean logging) {
+        try {
+            deleteSlave();
+        } catch (Exception e) {
+            if (logging) {
+                LOGGER.log(Level.WARNING, "Failed to delete slave", e);
             }
-            slave.terminate();
-            Jenkins.getInstance().removeNode(slave);
         }
     }
+
 }
diff --git a/jclouds-plugin/src/main/java/jenkins/plugins/jclouds/compute/JCloudsOneOffSlave.java b/jclouds-plugin/src/main/java/jenkins/plugins/jclouds/compute/JCloudsOneOffSlave.java
@@ -37,6 +37,8 @@ public Environment setUp(AbstractBuild build, Launcher launcher, final BuildList
                 public boolean tearDown(AbstractBuild build, final BuildListener listener) throws IOException, InterruptedException {
                     LOGGER.warning("Single-use slave " + c.getName() + " getting torn down.");
                     c.setTemporarilyOffline(true, OfflineCause.create(Messages._OneOffCause()));
+                    // Fixes JENKINS-28403
+                    c.deleteSlave();
                     return true;
                 }
             };

diff --git a/jclouds-plugin/src/main/java/jenkins/plugins/jclouds/compute/JCloudsRetentionStrategy.java b/jclouds-plugin/src/main/java/jenkins/plugins/jclouds/compute/JCloudsRetentionStrategy.java
@@ -21,27 +21,42 @@ public JCloudsRetentionStrategy() {
         readResolve();
     }
 
+    private void fastTerminate(final JCloudsComputer c) {
+        if (!c.isOffline()) {
+            LOGGER.info("Setting " + c.getName() + " to be deleted.");
+            try {
+                c.disconnect(OfflineCause.create(Messages._DeletedCause())).get();
+            } catch (Exception e) {
+                LOGGER.info("Caught " + e.toString());
+            }
+        }
+        c.deleteSlave(true);
+    }
+
     @Override
     public long check(JCloudsComputer c) {
         if (!checkLock.tryLock()) {
             return 1;
         } else {
             try {
                 final JCloudsSlave node = c.getNode();
-                if (null != node && c.isIdle() && !node.isPendingDelete() && !disabled) {
-                    // Get the retention time, in minutes, from the JCloudsCloud this JCloudsComputer belongs to.
-                    final int retentionTime = c.getRetentionTime();
-                    // check executor to ensure we are terminating online slaves
-                    if (retentionTime > -1 && c.countExecutors() > 0) {
-                        final long idleMilliseconds = System.currentTimeMillis() - c.getIdleStartMilliseconds();
-                        LOGGER.fine("Node " + c.getName() + " retentionTime: " + retentionTime + " idle: "
-                                + TimeUnit2.MILLISECONDS.toMinutes(idleMilliseconds) + "min");
-                        if (idleMilliseconds > TimeUnit2.MINUTES.toMillis(retentionTime)) {
-                            LOGGER.info("Setting " + c.getName() + " to be deleted.");
-                            if (!c.isOffline()) {
-                                c.disconnect(OfflineCause.create(Messages._DeletedCause()));
+                // check isIdle() to ensure we are terminating busy slaves (including FlyWeight)
+                if (null != node && c.isIdle()) {
+                    if (node.isPendingDelete()) {
+                        // Fixes JENKINS-28403
+                        fastTerminate(c);
+                    } else {
+                        // Get the retention time, in minutes, from the JCloudsCloud this JCloudsComputer belongs to.
+                        final int retentionTime = c.getRetentionTime();
+                        if (retentionTime > -1) {
+                            final long idleMilliseconds = System.currentTimeMillis() - c.getIdleStartMilliseconds();
+                            LOGGER.fine("Node " + c.getName() + " retentionTime: " + retentionTime + " idle: "
+                                    + TimeUnit2.MILLISECONDS.toMinutes(idleMilliseconds) + "min");
+                            if (idleMilliseconds > TimeUnit2.MINUTES.toMillis(retentionTime)) {
+                                LOGGER.info("Retention time for " + c.getName() + " has expired.");
+                                node.setPendingDelete(true);
+                                fastTerminate(c);
                             }
-                            node.setPendingDelete(true);
                         }
                     }
                 }
@@ -69,13 +84,11 @@ public String getDisplayName() {
         }
     }
 
+    // Serialization
     protected Object readResolve() {
         checkLock = new ReentrantLock(false);
         return this;
     }
 
     private static final Logger LOGGER = Logger.getLogger(JCloudsRetentionStrategy.class.getName());
-
-    public static final boolean disabled = Boolean.getBoolean(JCloudsRetentionStrategy.class.getName() + ".disabled");
-
 }
diff --git a/jclouds-plugin/src/main/java/jenkins/plugins/jclouds/compute/JCloudsSlave.java b/jclouds-plugin/src/main/java/jenkins/plugins/jclouds/compute/JCloudsSlave.java
@@ -293,17 +293,19 @@ public boolean isInstantiable() {
     }
 
     /**
-     * Destroy the node calls {@link ComputeService#destroyNode}
+     * Destroy the node.
+     * If stopOnTerminate is {@code true}, calls {@link ComputeService#suspendNode},
+     * otherwise {@link ComputeService#destroyNode}.
      */
     @Override
     protected void _terminate(TaskListener listener) throws IOException, InterruptedException {
         final ComputeService compute = JCloudsCloud.getByName(cloudName).getCompute();
         if (compute.getNodeMetadata(nodeId) != null && compute.getNodeMetadata(nodeId).getStatus().equals(NodeMetadata.Status.RUNNING)) {
             if (stopOnTerminate) {
-                LOGGER.info("Suspending the Slave : " + getNodeName());
+                LOGGER.info("Suspending slave : " + getNodeName());
                 compute.suspendNode(nodeId);
             } else {
-                LOGGER.info("Terminating the Slave : " + getNodeName());
+                LOGGER.info("Terminating slave : " + getNodeName());
                 compute.destroyNode(nodeId);
             }
         } else {