Fixing Issue with Orphaned Agents and configurable startup timeout (#1)…

… (#37) * Fixing Issue with Orphaned Agents and configurable startup timeout (#1) Adding Configurable Agent Startup Timeout and Fixing bug the resulted in Agents being orphaned on the Jenkins UI * refactor to enable editing the agent after it has started (#2) * refactor to enable editing the agent after it has started * downgrading so it runs in current version we are running * Update .gitignore * Delete config.yml * commiting fix to SECURITY-1058 * Update src/main/java/org/jenkinsci/plugins/nomad/NomadCloud.java Co-Authored-By: aphill70 <aphill70@gmail.com> * updating per code review * fixing messed of conflict resolution * had the wrong import
jenkinsci · May 28, 2019 · 93ea215 · 93ea215
1 parent fe2cc94
commit 93ea215
Show file tree

Hide file tree

Showing 14 changed files with 117 additions and 41 deletions.
diff --git a/.gitignore b/.gitignore
@@ -2,3 +2,11 @@
 target/
 work/
 nomad.iml
+.project
+.settings/
+.factorypath
+.classpath
+.vscode/
+.vagrant/
+Vagrantfile
+circleci/**
diff --git a/pom.xml b/pom.xml
@@ -37,6 +37,8 @@
     <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
     <java.level>8</java.level>
     <findbugs.failOnError>false</findbugs.failOnError>
+    <jenkins.version>2.121.1</jenkins.version>
+    <java.level>8</java.level>
   </properties>
 
   <developers>

diff --git a/src/main/java/org/jenkinsci/plugins/nomad/NomadCloud.java b/src/main/java/org/jenkinsci/plugins/nomad/NomadCloud.java
@@ -21,16 +21,20 @@
 import java.util.concurrent.*;
 import java.util.logging.Level;
 import java.util.logging.Logger;
-
 public class NomadCloud extends AbstractCloudImpl {
 
     private static final Logger LOGGER = Logger.getLogger(NomadCloud.class.getName());
+
     private final List<? extends NomadSlaveTemplate> templates;
+
     private final String nomadUrl;
     private String jenkinsUrl;
     private String jenkinsTunnel;
     private String slaveUrl;
+    private int workerTimeout = 1;
+
     private NomadApi nomad;
+
     private int pending = 0;
 
     @DataBoundConstructor
@@ -40,13 +44,16 @@ public NomadCloud(
             String jenkinsUrl,
             String jenkinsTunnel,
             String slaveUrl,
-            List<? extends NomadSlaveTemplate> templates) {
+            String workerTimeout,
+            List<? extends NomadSlaveTemplate> templates)
+    {
         super(name, null);
 
         this.nomadUrl = nomadUrl;
         this.jenkinsUrl = jenkinsUrl;
         this.jenkinsTunnel = jenkinsTunnel;
         this.slaveUrl = slaveUrl;
+        setWorkerTimeout(workerTimeout);
 
         if (templates == null) {
             this.templates = Collections.emptyList();
@@ -122,11 +129,9 @@ public ProvisioningCallback(String slaveName, NomadSlaveTemplate template, Nomad
         }
 
         public Node call() throws Exception {
-
             final NomadSlave slave = new NomadSlave(
-                    cloud,
                     slaveName,
-                    "Nomad Jenkins Slave",
+                    name,
                     template,
                     template.getLabels(),
                     new NomadRetentionStrategy(template.getIdleTerminationInMinutes()),
@@ -159,17 +164,18 @@ public Node call() throws Exception {
             ExecutorService executorService = Executors.newCachedThreadPool();
             Future<Boolean> future = executorService.submit(callableTask);
 
-            try {
-                future.get(5, TimeUnit.MINUTES);
+            try{
+                future.get(cloud.workerTimeout, TimeUnit.MINUTES);
                 LOGGER.log(Level.INFO, "Connection established");
             } catch (Exception ex) {
-                LOGGER.log(Level.SEVERE, "Slave computer did not come online within {0} minutes, terminating slave");
+                LOGGER.log(Level.SEVERE, "Slave computer did not come online within " + workerTimeout + " minutes, terminating slave"+ slave);
                 slave.terminate();
+                throw new RuntimeException("Timed out waiting for agent to start up. Timeout: " + workerTimeout + " minutes.");
             } finally {
                 future.cancel(true);
                 executorService.shutdown();
+                pending -= template.getNumExecutors();
             }
-            pending -= template.getNumExecutors();
             return slave;
         }
     }
@@ -221,7 +227,9 @@ public FormValidation doTestConnection(@QueryParameter("nomadUrl") String nomadU
             }
         }
 
+        @RequirePOST
         public FormValidation doCheckName(@QueryParameter String name) {
+            Objects.requireNonNull(Jenkins.getInstance()).checkPermission(Jenkins.ADMINISTER);
             if (Strings.isNullOrEmpty(name)) {
                 return FormValidation.error("Name must be set");
             } else {
@@ -231,7 +239,11 @@ public FormValidation doCheckName(@QueryParameter String name) {
     }
 
     // Getters
-    protected String getNomadUrl() {
+    public String getName() {
+        return name;
+    }
+
+    public String getNomadUrl() {
         return nomadUrl;
     }
 
@@ -243,6 +255,10 @@ public String getSlaveUrl() {
         return slaveUrl;
     }
 
+    public int getWorkerTimeout() {
+        return workerTimeout;
+    }
+
     public void setJenkinsUrl(String jenkinsUrl) {
         this.jenkinsUrl = jenkinsUrl;
     }
@@ -251,6 +267,14 @@ public void setSlaveUrl(String slaveUrl) {
         this.slaveUrl = slaveUrl;
     }
 
+    public void setWorkerTimeout(String workerTimeout) {
+        try {
+            this.workerTimeout = Integer.parseInt(workerTimeout);
+        } catch(NumberFormatException ex) {
+            LOGGER.log(Level.WARNING, "Failed to parse timeout defaulting to current value (default: 1 minute): " + workerTimeout + " minutes");
+        }
+    }
+
     public void setNomad(NomadApi nomad) {
         this.nomad = nomad;
     }

diff --git a/src/main/java/org/jenkinsci/plugins/nomad/NomadComputer.java b/src/main/java/org/jenkinsci/plugins/nomad/NomadComputer.java
@@ -11,14 +11,11 @@ public class NomadComputer extends AbstractCloudComputer<NomadSlave> {
 
     private static final Logger LOGGER = Logger.getLogger(NomadComputer.class.getName());
 
-    private final NomadCloud cloud;
-
     private final Boolean reusable;
 
     public NomadComputer(NomadSlave slave) {
         super(slave);
 
-        this.cloud = slave.getCloud();
         this.reusable = slave.getReusable();
     }
 

diff --git a/src/main/java/org/jenkinsci/plugins/nomad/NomadConstraintTemplate.java b/src/main/java/org/jenkinsci/plugins/nomad/NomadConstraintTemplate.java
@@ -3,17 +3,10 @@
 import hudson.Extension;
 import org.kohsuke.stapler.DataBoundConstructor;
 
-import hudson.Util;
 import hudson.model.Describable;
 import hudson.model.Descriptor;
 import jenkins.model.Jenkins;
 
-import javax.annotation.Nullable;
-import java.util.*;
-import java.util.logging.Logger;
-
-import org.json.JSONObject;
-
 public class NomadConstraintTemplate implements Describable<NomadConstraintTemplate> {
 
 	private final String ltarget;
@@ -57,7 +50,7 @@ public String getDisplayName() {
     public Descriptor<NomadConstraintTemplate> getDescriptor() {
         return Jenkins.getInstance().getDescriptor(getClass());
     }
-	
+
 	public String getLtarget() {
 		return ltarget;
 	}

diff --git a/src/main/java/org/jenkinsci/plugins/nomad/NomadProvisioningStrategy.java b/src/main/java/org/jenkinsci/plugins/nomad/NomadProvisioningStrategy.java
@@ -39,7 +39,7 @@ public NodeProvisioner.StrategyDecision apply(@Nonnull NodeProvisioner.StrategyS
                         new Object[]{snapshot.getAvailableExecutors(), snapshot.getConnectingExecutors(), strategyState.getAdditionalPlannedCapacity(),((NomadCloud)nomadCloud).getPending() });
                 int availableCapacity = snapshot.getAvailableExecutors() +
                         snapshot.getConnectingExecutors() +
-                        strategyState.getAdditionalPlannedCapacity() + 
+                        strategyState.getAdditionalPlannedCapacity() +
                         ((NomadCloud)nomadCloud).getPending();
 
                 int currentDemand = snapshot.getQueueLength();

diff --git a/src/main/java/org/jenkinsci/plugins/nomad/NomadRetentionStrategy.java b/src/main/java/org/jenkinsci/plugins/nomad/NomadRetentionStrategy.java
@@ -9,6 +9,10 @@ public NomadRetentionStrategy(int idleMinutes) {
         super(idleMinutes);
     }
 
+    public NomadRetentionStrategy(String idleMinutes) {
+        super(Integer.parseInt(idleMinutes));
+    }
+
     public static class DescriptorImpl extends Descriptor<hudson.slaves.RetentionStrategy<?>> {
         @Override
         public String getDisplayName() {

diff --git a/src/main/java/org/jenkinsci/plugins/nomad/NomadSlave.java b/src/main/java/org/jenkinsci/plugins/nomad/NomadSlave.java
@@ -2,35 +2,45 @@
 
 import hudson.Extension;
 import hudson.model.Descriptor;
+import hudson.model.Descriptor.FormException;
 import hudson.model.Node;
 import hudson.model.TaskListener;
 import hudson.slaves.*;
+import jenkins.model.Jenkins;
 
 import java.io.IOException;
+
+import java.util.Collections;
 import java.util.List;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
+import org.kohsuke.stapler.DataBoundConstructor;
+
 public class NomadSlave extends AbstractCloudSlave implements EphemeralNode {
 
     private static final Logger LOGGER = Logger.getLogger(NomadSlave.class.getName());
 
-    private final NomadCloud cloud;
-
     private final Boolean reusable;
 
+    private NomadSlaveTemplate template;
+
+    private static final String NODE_DESCRIPTION = "Nomad Jenkins Slave";
+
+    private final String cloudName;
+    private final int idleTerminationInMinutes;
+
     public NomadSlave(
-        NomadCloud cloud,
         String name,
-        String nodeDescription,
+        String cloudName,
         NomadSlaveTemplate template,
         String labelString,
-        hudson.slaves.RetentionStrategy retentionStrategy,
+        NomadRetentionStrategy retentionStrategy,
         List<? extends NodeProperty<?>> nodeProperties
     ) throws Descriptor.FormException, IOException {
         super(
             name,
-            nodeDescription,
+            NODE_DESCRIPTION,
             template.getRemoteFs(),
             template.getNumExecutors(),
             template.getMode(),
@@ -40,8 +50,21 @@ public NomadSlave(
             nodeProperties
         );
 
-        this.cloud = cloud;
+        this.cloudName = cloudName;
+
         this.reusable = template.getReusable();
+        this.idleTerminationInMinutes = template.getIdleTerminationInMinutes();
+    }
+
+    @DataBoundConstructor
+    // {"name":"jenkins-95266550a531","cloudName":"NomadTest","labelString":"test","mode":"NORMAL","remoteFS":"/","numExecutors":"1","idleTerminationInMinutes":"10","reusable":true}
+    public NomadSlave(String name, String cloudName, String remoteFS, String numExecutors, Mode mode, String labelString, String idleTerminationInMinutes, boolean reusable) throws FormException, IOException{
+        super(name, NODE_DESCRIPTION, remoteFS, numExecutors, mode, labelString, new JNLPLauncher(), new NomadRetentionStrategy(idleTerminationInMinutes), Collections.<NodeProperty<?>>emptyList());
+
+        this.cloudName = cloudName;
+        this.reusable = reusable;
+
+        this.idleTerminationInMinutes = Integer.parseInt(idleTerminationInMinutes);
     }
 
     @Override
@@ -51,6 +74,9 @@ public Node asNode() {
 
     @Extension
     public static class DescriptorImpl extends SlaveDescriptor {
+        public DescriptorImpl() {
+            load();
+        }
 
         @Override
         public String getDisplayName() {
@@ -67,21 +93,29 @@ public boolean isInstantiable() {
     }
 
     @Override
-    public AbstractCloudComputer createComputer() {
+    public AbstractCloudComputer<NomadSlave> createComputer() {
         return new NomadComputer(this);
     }
 
     @Override
     protected void _terminate(TaskListener listener)  {
         LOGGER.log(Level.INFO, "Asking Nomad to deregister slave '" + getNodeName() + "'");
-        cloud.Nomad().stopSlave(getNodeName());
+        getCloud().Nomad().stopSlave(getNodeName());
     }
 
     public NomadCloud getCloud() {
-        return cloud;
+        return (NomadCloud) Jenkins.getInstance().getCloud(cloudName);
+    }
+
+    public String getCloudName() {
+        return cloudName;
     }
 
     public Boolean getReusable() {
         return reusable;
     }
+
+    public int getIdleTerminationInMinutes() {
+        return this.idleTerminationInMinutes;
+    }
 }
diff --git a/src/main/java/org/jenkinsci/plugins/nomad/NomadSlaveTemplate.java b/src/main/java/org/jenkinsci/plugins/nomad/NomadSlaveTemplate.java
@@ -10,9 +10,7 @@
 import jenkins.model.Jenkins;
 import org.kohsuke.stapler.DataBoundConstructor;
 
-import java.util.Collections;
-import java.util.List;
-import java.util.Set;
+import java.util.*;
 import java.util.logging.Logger;
 
 public class NomadSlaveTemplate implements Describable<NomadSlaveTemplate> {

diff --git a/src/main/resources/org/jenkinsci/plugins/nomad/NomadCloud/config.jelly b/src/main/resources/org/jenkinsci/plugins/nomad/NomadCloud/config.jelly
@@ -8,7 +8,7 @@
 
   <f:entry title="Nomad URL" field="nomadUrl" description="Nomad API URL [hostname:port]">
     <f:textbox default="http://127.0.0.1:4646"/>
-  </f:entry>
+  </f:entry>j
 
   <f:entry title="Jenkins Base URL" field="jenkinsUrl" description="Jenkins base URL">
     <f:textbox default="${instance.getJenkinsUrl()}"/>
@@ -18,6 +18,10 @@
     <f:textbox default="${instance.getJenkinsTunnel()}"/>
   </f:entry>
 
+  <f:entry title="Worker Startup Timeout" field="workerTimeout" description="Worker Startup timeout in minutes">
+    <f:textbox default="1"/>
+  </f:entry>
+
   <f:entry title="Jenkins Slave URL" field="slaveUrl" description="Jenkins slave URL">
     <f:textbox default="${instance.getSlaveUrl()}"/>
   </f:entry>

diff --git a/src/main/resources/org/jenkinsci/plugins/nomad/NomadSlave/configure-entries.jelly b/src/main/resources/org/jenkinsci/plugins/nomad/NomadSlave/configure-entries.jelly
@@ -1,21 +1,31 @@
 <?jelly escape-by-default='true'?>
 
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
-
     <table width="100%">
+        <f:entry title="${%Cloud Name}" field="cloudName">
+          <f:readOnlyTextbox />
+        </f:entry>
 
-        <f:entry title="Labels" field="labelString">
+        <f:entry title="${%Labels}" field="labelString">
             <f:textbox/>
         </f:entry>
 
+        <f:slave-mode name="mode" node="${instance}" />
+
+        <f:entry title="${%Remote FS}" field="remoteFS">
+          <f:readOnlyTextbox />
+        </f:entry>
+
         <f:entry title="Number of executors" field="numExecutors">
             <f:textbox/>
         </f:entry>
 
-        <f:entry title="Idle termination time" field="idleTerminationInMinutes">
+        <f:entry title="Idle termination time (minutes)" field="idleTerminationInMinutes">
             <f:textbox/>
         </f:entry>
 
+        <f:entry title="Reusable">
+            <f:checkbox name="reusable" field="reusable" default="true" value="${instance.reusable}" />
+        </f:entry>
     </table>
-
 </j:jelly>