Addressing comments

* Kill shouldSlaveBeRetained() func
* Undo changes to ret policies
* minor fixes

src/main/java/org/jenkinsci/plugins/vSphereCloudSlaveComputer.java

@@ -7,6 +7,8 @@
 
 import javax.annotation.Nonnull;
 import org.jenkinsci.plugins.vsphere.tools.VSphere;
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.NoExternalUse;
 
 import com.vmware.vim25.VirtualHardware;
 import com.vmware.vim25.VirtualMachineConfigInfo;
@@ -83,8 +85,9 @@ public String getVmInformationError() {
     }
 
     /**
-     * Get all computers.
+     * Get all vsphere computers.
      */
+    @Restricted(NoExternalUse.class)
     protected static @Nonnull List<vSphereCloudSlaveComputer> getAll() {
         ArrayList<vSphereCloudSlaveComputer> out = new ArrayList<>();
         for (final Computer c : Jenkins.get().getComputers()) {

src/main/java/org/jenkinsci/plugins/vSphereCloudSlaveTemplate.java

@@ -330,42 +330,27 @@ public RetentionStrategy<?> getRetentionStrategy() {
      */
     @Restricted(NoExternalUse.class)
     public List<vSphereCloudSlaveComputer> getOnlineNodes() {
-        return getNodes(false, false);
+        return getNodes(false);
     }
 
     /**
      * Return a list of idle nodes provisioned using this template.
      */
     @Restricted(NoExternalUse.class)
     public List<vSphereCloudSlaveComputer> getIdleNodes() {
-        return getNodes(true, false);
+        return getNodes(true);
     }
 
-    /**
-     * Return a list of busy nodes provisioned using this template
-     * that can be reused.
-     */
-    @Restricted(NoExternalUse.class)
-    public List<vSphereCloudSlaveComputer> getBusyReusableNodes() {
-        return getNodes(false, true);
-    }
-
-    private List<vSphereCloudSlaveComputer> getNodes(Boolean idle, Boolean reusable) {
+    private List<vSphereCloudSlaveComputer> getNodes(boolean idle) {
         List<vSphereCloudSlaveComputer> nodes = new ArrayList<>();
         for (vSphereCloudSlaveComputer node : vSphereCloudSlaveComputer.getAll()) {
             if (!node.isOnline()) continue;
             if (idle && !node.isIdle()) continue;
-            // only busy nodes are counted as reusable
-            if (reusable && node.isIdle()) continue;
             String vmName = node.getName();
             vSphereCloudSlaveTemplate nodeTemplate = getParent().getTemplateForVM(vmName);
             // Filter out nodes from other clouds: nodeTemplate is null for these.
             if (nodeTemplate == null) continue;
             if (getLabelString() != nodeTemplate.getLabelString()) continue;
-            if (reusable) {
-                RetentionStrategy<?> nodeStrategy = nodeTemplate.getRetentionStrategy();
-                if (nodeStrategy instanceof RunOnceCloudRetentionStrategy) continue;
-            }
             nodes.add(node);
         }
         return nodes;

src/main/java/org/jenkinsci/plugins/vsphere/RunOnceCloudRetentionStrategy.java

@@ -65,10 +65,6 @@ public long check(final AbstractCloudComputer c) {
         if (c.isIdle() && !disabled) {
             final long idleMilliseconds = System.currentTimeMillis() - c.getIdleStartMilliseconds();
             if (idleMilliseconds > TimeUnit.MINUTES.toMillis(idleMinutes)) {
-                if (VSphereNodeReconcileWork.shouldNodeBeRetained(c)) {
-                    LOGGER.log(Level.FINE, "Keeping {0} to meet minimum requirements", c.getName());
-                    return 1;
-                }
                 LOGGER.log(
                         Level.FINE,
                         "Disconnecting {0} because it has been idle for more than {1} minutes (has been idle for {2}ms)",

src/main/java/org/jenkinsci/plugins/vsphere/VSphereCloudRetentionStrategy.java

@@ -5,26 +5,15 @@
 import hudson.Extension;
 import hudson.model.Descriptor;
 import hudson.model.DescriptorVisibilityFilter;
-import hudson.slaves.AbstractCloudComputer;
-import hudson.slaves.AbstractCloudSlave;
 import hudson.slaves.CloudRetentionStrategy;
 import hudson.slaves.RetentionStrategy;
 
-import java.io.IOException;
-import java.util.logging.Level;
-import java.util.logging.Logger;
-
-import static java.util.concurrent.TimeUnit.*;
-import javax.annotation.concurrent.GuardedBy;
-
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 import org.kohsuke.stapler.DataBoundConstructor;
 
 public class VSphereCloudRetentionStrategy extends CloudRetentionStrategy {
 
-    private static final Logger LOGGER = Logger.getLogger(VSphereCloudRetentionStrategy.class.getName());
-
     private final int idleMinutes;
 
     @DataBoundConstructor
@@ -37,16 +26,6 @@ public int getIdleMinutes() {
         return idleMinutes;
     }
 
-    @Override
-    @GuardedBy("hudson.model.Queue.lock")
-    public long check(final AbstractCloudComputer c) {
-        if (VSphereNodeReconcileWork.shouldNodeBeRetained(c)) {
-            LOGGER.log(Level.FINE, "Keeping {0} to meet minimum requirements", c.getName());
-            return 1;
-        }
-        return super.check(c);
-    }
-
     @Override
     public DescriptorImpl getDescriptor() {
         return DESCRIPTOR;

src/main/java/org/jenkinsci/plugins/vsphere/VSphereNodeReconcileWork.java

@@ -4,13 +4,11 @@
 import hudson.Functions;
 import hudson.model.TaskListener;
 import hudson.model.AsyncPeriodicWork;
-import hudson.slaves.AbstractCloudComputer;
 import hudson.slaves.AbstractCloudSlave;
 import hudson.slaves.Cloud;
 import hudson.model.Label;
 import jenkins.model.Jenkins;
 import org.jenkinsci.plugins.vSphereCloud;
-import org.jenkinsci.plugins.vSphereCloudSlave;
 import org.jenkinsci.plugins.vSphereCloudSlaveComputer;
 import org.jenkinsci.plugins.vSphereCloudSlaveTemplate;
 
@@ -54,15 +52,13 @@ public void execute(TaskListener listener) {
                 int instancesMin = template.getInstancesMin();
                 List<vSphereCloudSlaveComputer> idleNodes = template.getIdleNodes();
                 List<vSphereCloudSlaveComputer> runningNodes = template.getOnlineNodes();
-                List<vSphereCloudSlaveComputer> reusableBusyNodes = template.getBusyReusableNodes();
                 // Get max number of nodes that could be provisioned
                 int globalMaxNodes = ((vSphereCloud) cloud).getInstanceCap();
                 int templateMaxNodes = template.getTemplateInstanceCap();
                 int maxNodes = Math.min(globalMaxNodes, templateMaxNodes);
 
                 // if maxNumber is lower than instancesMin, we have to ignore instancesMin
-                int toProvision = Math.min(instancesMin - (reusableBusyNodes.size() + idleNodes.size()),
-                        maxNodes - runningNodes.size());
+                int toProvision = Math.min(instancesMin - idleNodes.size(), maxNodes - runningNodes.size());
                 if (toProvision > 0) {
                     // provision desired number of nodes for this label
                     LOGGER.log(Level.INFO, "Pre-creating {0} instance(s) for template {1} in cloud {3}",
@@ -92,28 +88,4 @@ public void execute(TaskListener listener) {
             }
         }
     }
-
-    /**
-     * Should a node be retained to meet the minimum instances constraint?
-     */
-    @SuppressWarnings("rawtypes")
-    public static boolean shouldNodeBeRetained(AbstractCloudComputer c) {
-        // Checks only idle nodes
-        vSphereCloudSlave node = (vSphereCloudSlave) c.getNode();
-        if (node == null) return false;
-        vSphereCloudSlaveTemplate nodeTemplate = node.getTemplate();
-        // nodeTemplate might be null if the template was manually deleted from a cloud
-        if (nodeTemplate == null) return false;
-        int instancesMin = nodeTemplate.getInstancesMin();
-        if (instancesMin > 0) {
-            int maxNodes = Math.min(nodeTemplate.getTemplateInstanceCap(), nodeTemplate.getParent().getInstanceCap());
-            int runningNodesTotal = nodeTemplate.getOnlineNodes().size();
-            int idleNodesTotal = nodeTemplate.getIdleNodes().size();
-            int reusableBusyNodesTotal = nodeTemplate.getBusyReusableNodes().size();
-            if ((instancesMin >= (idleNodesTotal + reusableBusyNodesTotal - 1)) && (runningNodesTotal <= maxNodes)) {
-                return true;
-            }
-        }
-        return false;
-    }
 }

src/main/resources/org/jenkinsci/plugins/vSphereCloudSlaveTemplate/help-instancesMin.html

@@ -1,16 +1,12 @@
 <div>
     The number of VMs to be provisioned beforehand.<br>
     This allows to speed up CI runs by starting them immediately without waiting for a VM to get booted.<br>
-    <dt>If the number set to 0:</dt>
+    <dt>If the number is set to 0:</dt>
     <dd>No VMs provisioned in advance.</dd>
     <dt>If the number is bigger than 0:</dt>
     <dd>The plugin provisions new VMs to meet the value.</dd>
-    <dt>If Keep-Until-Idle retention strategy is used along with this option:</dt>
-    <dd>Busy VMs are counted as reuseable, so the plugin will not provision more VMs even if all of them are currently busy.</dd>
-    <dt>If Run-Once retention strategy is used along with this option:</dt>
-    <dd>Busy VMs are counted as non reusable, so the plugin will provision more VMs to meet the value.<br>
-        When at maximum capacity new VM will be added after an old VM is gone.</dd>
     <dt>If instances Min is bigger than instance Cap:</dt>
-    <dd>The plugin will provision max number of VMs specified in instance Cap (the smallest of cloud and template options).</dd>
+    <dd>The plugin provisions max number of VMs specified in instance Cap (the smallest of cloud and template options).</dd>
     The plugin checks the number of running VMs once in 2 minutes.
+    <dt>The plugin respects retention policies.</dt>
 </div>