Refactor the code to address comments

* Use native logic that takes into account state of the nodes

Author: nkorabli

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

@@ -23,8 +23,6 @@
 import org.jenkinsci.plugins.folder.FolderVSphereCloudProperty;
 import org.jenkinsci.plugins.vsphere.VSphereConnectionConfig;
 import org.jenkinsci.plugins.vsphere.tools.*;
-import org.kohsuke.accmod.Restricted;
-import org.kohsuke.accmod.restrictions.NoExternalUse;
 import org.kohsuke.stapler.DataBoundConstructor;
 import org.kohsuke.stapler.QueryParameter;
 import org.kohsuke.stapler.Stapler;
@@ -217,8 +215,7 @@ public List<? extends vSphereCloudSlaveTemplate> getTemplates() {
         return this.templates;
     }
 
-    @Restricted(NoExternalUse.class)
-    vSphereCloudSlaveTemplate getTemplateForVM(final String vmName) {
+    private vSphereCloudSlaveTemplate getTemplateForVM(final String vmName) {
         if (this.templates == null || vmName == null)
             return null;
         for (final vSphereCloudSlaveTemplate t : this.templates) {
@@ -355,22 +352,15 @@ public Collection<PlannedNode> provision(final Label label, int excessWorkload)
             final List<PlannedNode> plannedNodes = new ArrayList<PlannedNode>();
             synchronized (templateState) {
                 templateState.pruneUnwantedRecords();
-                Integer maxSlavesToProvisionBeforeCloudCapHit = calculateMaxAdditionalSlavesPermitted();
-                if (maxSlavesToProvisionBeforeCloudCapHit != null && maxSlavesToProvisionBeforeCloudCapHit <= 0) {
+                if (!cloudHasCapacity()) {
                     return Collections.emptySet(); // no capacity due to cloud instance cap
                 }
                 final List<vSphereCloudSlaveTemplate> templates = getTemplates(label);
                 final List<CloudProvisioningRecord> whatWeCouldUse = templateState.calculateProvisionableTemplates(templates);
                 VSLOG.log(Level.INFO, methodCallDescription + ": " + numberOfvSphereCloudSlaves + " existing slaves (="
                         + numberOfvSphereCloudSlaveExecutors + " executors), templates available are " + whatWeCouldUse);
                 while (excessWorkloadSoFar > 0) {
-                    if (maxSlavesToProvisionBeforeCloudCapHit != null) {
-                        final int intValue = maxSlavesToProvisionBeforeCloudCapHit.intValue();
-                        if (intValue <= 0) {
-                            break; // out of capacity due to cloud instance cap
-                        }
-                        maxSlavesToProvisionBeforeCloudCapHit = Integer.valueOf(intValue - 1);
-                    }
+                    if (!cloudHasCapacity()) break;
                     final CloudProvisioningRecord whatWeShouldSpinUp = CloudProvisioningAlgorithm.findTemplateWithMostFreeCapacity(whatWeCouldUse);
                     if (whatWeShouldSpinUp == null) {
                         break; // out of capacity due to template instance cap
@@ -390,6 +380,46 @@ public Collection<PlannedNode> provision(final Label label, int excessWorkload)
         }
     }
 
+    /**
+     * Pre-provisions nodes per template to save time on a VM boot.
+     *
+     * @param template
+     */
+    public void preProvisionNodes(vSphereCloudSlaveTemplate template) {
+        final String methodCallDescription = "preProvisionNodesForTemplate(" + template.getLabelString() + ")";
+        try {
+            synchronized (this) {
+                ensureLists();
+            }
+            synchronized (templateState) {
+                templateState.pruneUnwantedRecords();
+                final CloudProvisioningRecord provisionable = templateState.getOrCreateRecord(template);
+                int nodesToProvision = CloudProvisioningAlgorithm.shouldPreProvisionNodes(provisionable);
+                VSLOG.log(Level.INFO, methodCallDescription + ": should pre-provision " + nodesToProvision + " nodes");
+                while (nodesToProvision > 0) {
+                    if (!cloudHasCapacity()) break;
+                    final String nodeName = CloudProvisioningAlgorithm.findUnusedName(provisionable);
+                    VSpherePlannedNode.createInstance(templateState, nodeName, provisionable);
+                    nodesToProvision -= 1;
+                }
+            }
+        } catch (Exception ex) {
+            VSLOG.log(Level.WARNING, methodCallDescription + ": Failed.", ex);
+        }
+    }
+
+    /**
+     * Check if at least one additional node can be provisioned.
+     */
+    private boolean cloudHasCapacity(){
+        Integer maxSlavesToProvisionBeforeCloudCapHit = calculateMaxAdditionalSlavesPermitted();
+        if (maxSlavesToProvisionBeforeCloudCapHit != null && maxSlavesToProvisionBeforeCloudCapHit <= 0) {
+            VSLOG.info("The cloud is at max capacity. Can not provison more nodes.");
+            return false;
+        }
+        return true;
+    }
+
     /**
      * Has another go at deleting VMs we failed to delete earlier. It's possible
      * that we were unable to talk to vSphere (or some other failure happened)

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

@@ -2,13 +2,8 @@
 
 import java.io.PrintWriter;
 import java.io.StringWriter;
-import java.util.ArrayList;
-import java.util.List;
 
-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;
@@ -19,10 +14,8 @@
 import com.vmware.vim25.mo.ManagedEntity;
 import com.vmware.vim25.mo.VirtualMachine;
 
-import hudson.model.Computer;
 import hudson.slaves.AbstractCloudComputer;
 import hudson.slaves.AbstractCloudSlave;
-import jenkins.model.Jenkins;
 
 public class vSphereCloudSlaveComputer extends AbstractCloudComputer {
     private final vSphereCloudSlave vSlave;
@@ -84,19 +77,6 @@ public String getVmInformationError() {
         return getVMInformation().errorEncounteredWhenDataWasRead;
     }
 
-    /**
-     * Get all vsphere computers.
-     */
-    @Restricted(NoExternalUse.class)
-    static @Nonnull List<vSphereCloudSlaveComputer> getAll() {
-        ArrayList<vSphereCloudSlaveComputer> out = new ArrayList<>();
-        for (final Computer c : Jenkins.get().getComputers()) {
-            if (!(c instanceof vSphereCloudSlaveComputer)) continue;
-            out.add((vSphereCloudSlaveComputer) c);
-        }
-        return out;
-    }
-
     /** 10 seconds */
     private static final long NANOSECONDS_TO_CACHE_VMINFORMATION = 10L * 1000000000L;
 

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

@@ -325,37 +325,6 @@ public RetentionStrategy<?> getRetentionStrategy() {
         return this.retentionStrategy;
     }
 
-    /**
-     * Return a list of running nodes provisioned using this template.
-     */
-    @Restricted(NoExternalUse.class)
-    public List<vSphereCloudSlaveComputer> getOnlineNodes() {
-        return getNodes(false);
-    }
-
-    /**
-     * Return a list of idle nodes provisioned using this template.
-     */
-    @Restricted(NoExternalUse.class)
-    public List<vSphereCloudSlaveComputer> getIdleNodes() {
-        return getNodes(true);
-    }
-
-    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;
-            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;
-            nodes.add(node);
-        }
-        return nodes;
-    }
-
     protected Object readResolve() {
         this.labelSet = Label.parse(labelString);
         if(this.templateInstanceCap == 0) {

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

@@ -0,0 +1,52 @@
+package org.jenkinsci.plugins.vsphere;
+
+import hudson.Extension;
+import hudson.Functions;
+import hudson.model.TaskListener;
+import hudson.model.AsyncPeriodicWork;
+import hudson.slaves.Cloud;
+import jenkins.model.Jenkins;
+import org.jenkinsci.plugins.vSphereCloud;
+import org.jenkinsci.plugins.vSphereCloudSlaveTemplate;
+
+import java.util.logging.Level;
+import java.util.logging.Logger;
+
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.NoExternalUse;
+
+/**
+ * A {@link AsyncPeriodicWork} that pre-provisions nodes to meet insntanceMin template value.
+ * <p>
+ * The async work will check the number of active nodes 
+ * and provision additional ones to meet template values. 
+ * 
+ * The check is happening every 2 minutes.
+ */
+@Extension
+@Restricted(NoExternalUse.class)
+public final class VSpherePreProvisonWork extends AsyncPeriodicWork {
+    private static final Logger LOGGER = Logger.getLogger(VSpherePreProvisonWork.class.getName());
+
+    public VSpherePreProvisonWork() {
+        super("Vsphere pre-provision check");
+    }
+
+    @Override
+    public long getRecurrencePeriod() {
+        return Functions.getIsUnitTest() ? Long.MAX_VALUE : MIN * 2;
+    }
+
+    @Override
+    public void execute(TaskListener listener) {
+        for (Cloud cloud : Jenkins.getActiveInstance().clouds) {
+            if (!(cloud instanceof vSphereCloud)) continue;
+            for (vSphereCloudSlaveTemplate template : ((vSphereCloud) cloud).getTemplates()) {
+                if (template.getInstancesMin() > 0) {
+                    LOGGER.log(Level.INFO, "Check if template (label=" + template.getLabelString() + ") has enough active nodes to meet instances Min value");
+                    ((vSphereCloud) cloud).preProvisionNodes(template);
+                }
+            }
+        }
+    }
+}

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

@@ -81,6 +81,23 @@ public static String findUnusedName(CloudProvisioningRecord record) {
                 + ", even after " + maxAttempts + " attempts.");
     }
 
+    /**
+     * Compares sum of provisioned and planned nodes for the template.
+     *
+     * If the sum is less than instanceMin template value we should provision more nodes,
+     * otherwise the value is satisfied and we should not add any more nodes yet.
+     *
+     * @param record
+     *            Our record regarding the template the agent will be created
+     *            from.
+     * @return A number of nodes to be provisioned.
+     */
+    public static int shouldPreProvisionNodes(CloudProvisioningRecord record) {
+        int provisionedNodes = record.getCurrentlyProvisioned().size() + record.getCurrentlyPlanned().size();
+        int requiredNodes = record.getTemplate().getInstancesMin();
+        return requiredNodes - provisionedNodes;
+    }
+
     private static String calcSequentialSuffix(final int attempt) {
         final int slaveNumber = attempt + 1;
         final String suffix = Integer.toString(slaveNumber);

src/main/java/org/jenkinsci/plugins/vsphere/tools/CloudProvisioningAlgorithm.java

@@ -8,5 +8,5 @@
     <dt>If instances Min is bigger than instance Cap:</dt>
     <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>
+    <dt>Pre-provisoned VMs will be deleted based on the retention policy.</dt>
 </div>