Merge pull request #70 from spotify/rculbertson/temp-jobs-default-expire

Set the expires value each time we create a TempJob

Author: rculbertson

helios-testing/pom.xml

@@ -26,6 +26,11 @@
       <artifactId>junit</artifactId>
       <version>4.11</version>
     </dependency>
+    <dependency>
+      <groupId>joda-time</groupId>
+      <artifactId>joda-time</artifactId>
+      <version>2.3</version>
+    </dependency>
 
     <!--test deps-->
     <dependency>

helios-testing/src/main/java/com/spotify/helios/testing/TemporaryJobBuilder.java

@@ -33,11 +33,14 @@
 import com.spotify.helios.common.descriptors.ServiceEndpoint;
 import com.spotify.helios.common.descriptors.ServicePorts;
 
+import org.joda.time.DateTime;
+
 import java.io.File;
 import java.io.IOException;
 import java.net.MalformedURLException;
 import java.net.URL;
 import java.nio.file.Path;
+import java.util.Date;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -56,6 +59,7 @@
 public class TemporaryJobBuilder {
 
   private static final Pattern JOB_NAME_FORBIDDEN_CHARS = Pattern.compile("[^0-9a-zA-Z-_.]+");
+  private static final int DEFAULT_EXPIRES_MINUTES = 30;
 
   private final List<String> hosts = Lists.newArrayList();
   private final Job.Builder builder = Job.newBuilder();
@@ -149,6 +153,19 @@ public TemporaryJobBuilder hostFilter(final String hostFilter) {
     return this;
   }
 
+  /**
+   * The Helios master will undeploy and delete the job at the specified date, if it has not
+   * already been removed. If not set, jobs will be removed after 30 minutes. This is for the
+   * case when a TemporaryJob is not cleaned up properly, perhaps because the process terminated
+   * prematurely.
+   * @param expires the Date when the job should be removed
+   * @return the TemporaryJobBuilder
+   */
+  public TemporaryJobBuilder expires(final Date expires) {
+    this.builder.setExpires(expires);
+    return this;
+  }
+
   /**
    * Deploys the job to the specified hosts. If no hosts are specified, a host will be chosen at
    * random from the current Helios cluster. If the HELIOS_HOST_FILTER environment variable is set,
@@ -181,6 +198,12 @@ public TemporaryJob deploy(final List<String> hosts) {
         builder.setVersion(randomVersion());
       }
 
+      // Set job to expires value, if it's not already set. This ensures temporary jobs which
+      // aren't cleaned up properly by the test will be removed by the master.
+      if (builder.getExpires() == null) {
+        builder.setExpires(new DateTime().plusMinutes(DEFAULT_EXPIRES_MINUTES).toDate());
+      }
+
       if (this.hosts.isEmpty()) {
         if (isNullOrEmpty(hostFilter)) {
           hostFilter = getenv("HELIOS_HOST_FILTER");

helios-testing/src/test/java/com/spotify/helios/testing/TemporaryJobsTest.java

@@ -27,23 +27,27 @@
 import com.spotify.helios.common.descriptors.JobStatus;
 import com.spotify.helios.system.SystemTestBase;
 
+import org.joda.time.DateTime;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 
 import java.io.File;
 import java.net.Socket;
 import java.nio.file.Path;
+import java.util.Date;
 import java.util.Map;
 
 import static com.google.common.base.Charsets.UTF_8;
 import static com.google.common.collect.Iterables.getOnlyElement;
 import static com.spotify.helios.common.descriptors.HostStatus.Status.UP;
 import static java.util.concurrent.TimeUnit.MINUTES;
 import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasKey;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.notNullValue;
 import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -202,17 +206,45 @@ public static class JobNamePrefixTest {
         .prefixDirectory(prefixDirectory.toString())
         .build();
 
+    private final Date expires = new DateTime().plusHours(1).toDate();
+
     private TemporaryJob job1;
+    private TemporaryJob job2;
 
     @Before
     public void setup() {
       job1 = temporaryJobs.job()
           .image(BUSYBOX)
           .command(IDLE_COMMAND)
           .deploy(testHost);
+
+      job2 = temporaryJobs.job()
+          .image(BUSYBOX)
+          .command(IDLE_COMMAND)
+          .expires(expires)
+          .deploy(testHost);
     }
 
     @Test public void testJobPrefixFile() throws Exception {
+      // Verify a default expires values was set on job1
+      assertThat(job1.job().getExpires(), is(notNullValue()));
+
+      // Verify expires was set correctly on job2
+      assertThat(job2.job().getExpires(), is(equalTo(expires)));
+
+      // Get all jobs from master to make sure values are set correctly there
+      final Map<JobId, Job> jobs = client.jobs().get();
+
+      // Verify job1 was set correctly on master
+      final Job remoteJob1 = jobs.get(job1.job().getId());
+      assertThat(remoteJob1, is(notNullValue()));
+      assertThat(remoteJob1.getExpires(), is(equalTo(job1.job().getExpires())));
+
+      // Verify job2 was set correctly on master
+      final Job remoteJob2 = jobs.get(job2.job().getId());
+      assertThat(remoteJob2, is(notNullValue()));
+      assertThat(remoteJob2.getExpires(), equalTo(expires));
+
       // Set jobPrefixFile so we can verify it was deleted after test completed
       jobPrefixFile = temporaryJobs.jobPrefixFile();
     }