Merge branch 'Issue-746-InitScriptRetry' of https://github.com/aledsage/jclouds

* 'Issue-746-InitScriptRetry' of https://github.com/aledsage/jclouds:
  Issue 746: add properties for initStatusInitialPeriod and initStatusMaxPeriod
  Issue 746: increasing retry times for init-script
  Issue 746: Improve RetryablePredicateTest; fix retry nextMaxInterval

compute/src/main/java/org/jclouds/compute/callables/BlockUntilInitScriptStatusIsZeroThenReturnOutput.java

@@ -28,8 +28,6 @@ import java.util.concurrent.TimeoutException;
 
 import javax.annotation.PostConstruct;
 import javax.annotation.Resource;
-import javax.inject.Inject;
-import javax.inject.Named;
 
 import org.jclouds.Constants;
 import org.jclouds.compute.domain.ExecResponse;
@@ -43,7 +41,9 @@ import org.jclouds.ssh.SshClient;
 import com.google.common.base.Predicate;
 import com.google.common.base.Throwables;
 import com.google.common.util.concurrent.AbstractFuture;
+import com.google.inject.Inject;
 import com.google.inject.assistedinject.Assisted;
+import com.google.inject.name.Named;
 
 /**
  * A future that works in tandem with a task that was invoked by {@link InitBuilder}
@@ -69,7 +69,13 @@ public class BlockUntilInitScriptStatusIsZeroThenReturnOutput extends AbstractFu
    @Inject
    public BlockUntilInitScriptStatusIsZeroThenReturnOutput(
             @Named(Constants.PROPERTY_USER_THREADS) ExecutorService userThreads,
+            ComputeServiceConstants.InitStatusProperties properties,
             final ScriptStatusReturnsZero stateRunning, @Assisted final SudoAwareInitManager commandRunner) {
+      
+      long retryMaxWait = TimeUnit.DAYS.toMillis(365); // arbitrarily high value, but Long.MAX_VALUE doesn't work!
+      long retryInitialPeriod = properties.initStatusInitialPeriod;
+      long retryMaxPeriod = properties.initStatusMaxPeriod;
+      
       this.commandRunner = checkNotNull(commandRunner, "commandRunner");
       this.userThreads = checkNotNull(userThreads, "userThreads");
       this.notRunningAnymore = new RetryablePredicate<String>(new Predicate<String>() {
@@ -78,8 +84,7 @@ public class BlockUntilInitScriptStatusIsZeroThenReturnOutput extends AbstractFu
          public boolean apply(String arg0) {
             return commandRunner.runAction(arg0).getOutput().trim().equals("");
          }
-         // arbitrarily high value, but Long.MAX_VALUE doesn't work!
+      }, retryMaxWait, retryInitialPeriod, retryMaxPeriod, TimeUnit.MILLISECONDS) {
-      }, TimeUnit.DAYS.toMillis(365)) {
          /**
           * make sure we stop the retry loop if someone cancelled the future, this keeps threads
           * from being consumed on dead tasks

compute/src/main/java/org/jclouds/compute/reference/ComputeServiceConstants.java

@@ -36,9 +36,13 @@ public interface ComputeServiceConstants {
    public static final String PROPERTY_TIMEOUT_NODE_SUSPENDED = "jclouds.compute.timeout.node-suspended";
    public static final String PROPERTY_TIMEOUT_SCRIPT_COMPLETE = "jclouds.compute.timeout.script-complete";
    public static final String PROPERTY_TIMEOUT_PORT_OPEN = "jclouds.compute.timeout.port-open";
+   
+   public static final String PROPERTY_INIT_STATUS_INITIAL_PERIOD = "jclouds.compute.init-status.initial-period";
+   public static final String PROPERTY_INIT_STATUS_MAX_PERIOD = "jclouds.compute.init-status.max-period";
+
    /**
-    * comma-separated nodes that we shouldn't attempt to list as they are dead in the provider for
+    * comma-separated nodes that we shouldn't attempt to list as they are dead
-    * some reason.
+    * in the provider for some reason.
     */
    public static final String PROPERTY_BLACKLIST_NODES = "jclouds.compute.blacklist-nodes";
 
@@ -53,6 +57,17 @@ public interface ComputeServiceConstants {
     */
    public static final String PROPERTY_OS_VERSION_MAP_JSON = "jclouds.compute.os-version-map-json";
 
+   @Singleton
+   public static class InitStatusProperties {
+      @Inject(optional = true)
+      @Named(PROPERTY_INIT_STATUS_INITIAL_PERIOD)
+      public long initStatusInitialPeriod = 500;
+
+      @Inject(optional = true)
+      @Named(PROPERTY_INIT_STATUS_MAX_PERIOD)
+      public long initStatusMaxPeriod = 5000;
+   }
+
    @Singleton
    public static class ReferenceData {
       @Inject(optional = true)

core/src/main/java/org/jclouds/predicates/RetryablePredicate.java

@@ -80,6 +80,7 @@ public class RetryablePredicate<T> implements Predicate<T> {
          }
       } catch (InterruptedException e) {
          logger.warn(e, "predicate %s on %s interrupted, returning false", input, predicate);
+         Thread.currentThread().interrupt();
       } catch (RuntimeException e) {
          if (getFirstThrowableOfType(e, ExecutionException.class) != null) {
             logger.warn(e, "predicate %s on %s errored [%s], returning false", input, predicate, e.getMessage());
@@ -97,8 +98,9 @@ public class RetryablePredicate<T> implements Predicate<T> {
    }
 
    protected long nextMaxInterval(long attempt, Date end) {
-      // FIXME i think this should be pow(1.5, attempt) -- or alternatively newInterval = oldInterval*1.5
+      // Interval increases exponentially, at a rate of nextInterval *= 1.5
-      long interval = (period * (long) Math.pow(attempt, 1.5));
+      // Note that attempt starts counting at 1
+      long interval = (long) (period * Math.pow(1.5, (attempt-1)));
       interval = interval > maxPeriod ? maxPeriod : interval;
       long max = end.getTime() - System.currentTimeMillis();
       return (interval > max) ? max : interval;

core/src/test/java/org/jclouds/predicates/RetryablePredicateTest.java

@@ -18,25 +18,36 @@
  */
 package org.jclouds.predicates;
 
-import java.util.Date;
+import java.util.ArrayList;
+import java.util.List;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 
+import org.testng.Assert;
+import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.Test;
 
 import com.google.common.base.Predicate;
 import com.google.common.base.Predicates;
+import com.google.common.base.Stopwatch;
 import com.google.common.base.Supplier;
 
 /**
  * 
  * @author Adrian Cole
- * 
  */
-@Test(groups = "unit", sequential = true)
+@Test(groups = "unit", singleThreaded = true)
 public class RetryablePredicateTest {
-   public static int SLOW_BUILD_SERVER_GRACE = 100;
+   // Grace must be reasonably big; Thread.sleep can take a bit longer to wake up sometimes...
+   public static int SLOW_BUILD_SERVER_GRACE = 250;
+   
+   private Stopwatch stopwatch;
+
+   @BeforeMethod
+   public void setUp() {
+      stopwatch = new Stopwatch();
+   }
    
    @Test
    void testFalseOnIllegalStateExeception() {
@@ -74,7 +85,8 @@ public class RetryablePredicateTest {
                   }
 
                }, 3, 1, TimeUnit.SECONDS);
-      Date startPlusThird = new Date(System.currentTimeMillis() + 1000);
+      
+      stopwatch.start();
       assert !predicate.apply(new Supplier<String>() {
 
          @Override
@@ -83,76 +95,94 @@ public class RetryablePredicateTest {
          }
 
       });
-      Date now = new Date();
+      long duration = stopwatch.elapsedMillis();
-      assert now.compareTo(startPlusThird) < 0 : String.format("%s should be less than %s", now.getTime(),
+      assertOrdered(duration, SLOW_BUILD_SERVER_GRACE);
-               startPlusThird.getTime());
    }
 
    @Test
    void testAlwaysTrue() {
+      // will call once immediately
       RetryablePredicate<String> predicate = new RetryablePredicate<String>(Predicates.<String> alwaysTrue(), 3, 1,
                TimeUnit.SECONDS);
-      Date startPlusThird = new Date(System.currentTimeMillis() + 1000);
+      stopwatch.start();
       predicate.apply("");
-      Date now = new Date();
+      long duration = stopwatch.elapsedMillis();
-      assert now.compareTo(startPlusThird) < 0 : String.format("%s should be less than %s", now.getTime(),
+      assertOrdered(duration, SLOW_BUILD_SERVER_GRACE);
-               startPlusThird.getTime());
    }
 
    @Test
    void testAlwaysFalseMillis() {
+      // maxWait=3; period=1; maxPeriod defaults to 1*10
+      // will call at 0, 1, 1+(1*1.5), 3
       RetryablePredicate<String> predicate = new RetryablePredicate<String>(Predicates.<String> alwaysFalse(), 3, 1,
                TimeUnit.SECONDS);
-      Date startPlus3Seconds = new Date(System.currentTimeMillis() + 3000);
+      stopwatch.start();
-      Date startPlus4Seconds = new Date(System.currentTimeMillis() + 4000 + SLOW_BUILD_SERVER_GRACE);
       predicate.apply("");
-      Date now = new Date();
+      long duration = stopwatch.elapsedMillis();
-      assert now.compareTo(startPlus3Seconds) >= 0 : String.format("%s should be less than %s", startPlus3Seconds
+      assertOrdered(3000, duration, 3000+SLOW_BUILD_SERVER_GRACE);
-               .getTime(), now.getTime());
-      assert now.compareTo(startPlus4Seconds) <= 0 : String.format("%s should be greater than %s", startPlus4Seconds
-               .getTime(), now.getTime());
-
-   }
-
-   private static class ThirdTimeTrue implements Predicate<String> {
-
-      private int count = 0;
-
-      @Override
-      public boolean apply(String input) {
-         return count++ == 2;
-      }
-
    }
 
    @Test
    void testThirdTimeTrue() {
-      RetryablePredicate<String> predicate = new RetryablePredicate<String>(new ThirdTimeTrue(), 3, 1, TimeUnit.SECONDS);
+      // maxWait=4; period=1; maxPeriod defaults to 1*10
-
+      // will call at 0, 1, 1+(1*1.5)
-      Date startPlus = new Date(System.currentTimeMillis() + 1000);
+      RepeatedAttemptsPredicate rawPredicate = new RepeatedAttemptsPredicate(2);
-      Date startPlus3 = new Date(System.currentTimeMillis() + 3000 + SLOW_BUILD_SERVER_GRACE);
+      RetryablePredicate<String> predicate = new RetryablePredicate<String>(rawPredicate, 4, 1, TimeUnit.SECONDS);
 
+      stopwatch.start();
       predicate.apply("");
-      Date now = new Date();
+      long duration = stopwatch.elapsedMillis();
-      assert now.compareTo(startPlus) >= 0 : String.format("%s should be greater than %s", now.getTime(), startPlus
+      
-               .getTime());
+      assertOrdered(2500, duration, 2500+SLOW_BUILD_SERVER_GRACE);
-      assert now.compareTo(startPlus3) <= 0 : String.format("%s should be greater than %s", startPlus3.getTime(), now
+      assertCallFrequency(rawPredicate.callTimes, 0, 1000, 1000+1500);
-               .getTime());
    }
 
    @Test
    void testThirdTimeTrueLimitedMaxInterval() {
-      RetryablePredicate<String> predicate = new RetryablePredicate<String>(new ThirdTimeTrue(), 3, 1, 1,
+      // maxWait=3; period=1; maxPeriod=1
+      // will call at 0, 1, 1+1
+      RepeatedAttemptsPredicate rawPredicate = new RepeatedAttemptsPredicate(2);
+      RetryablePredicate<String> predicate = new RetryablePredicate<String>(rawPredicate, 3, 1, 1,
                TimeUnit.SECONDS);
 
-      Date startPlus = new Date(System.currentTimeMillis() + 1000);
+      stopwatch.start();
-      Date startPlus2 = new Date(System.currentTimeMillis() + 2000 + SLOW_BUILD_SERVER_GRACE);
-
       predicate.apply("");
-      Date now = new Date();
+      long duration = stopwatch.elapsedMillis();
-      assert now.compareTo(startPlus) >= 0 : String.format("%s should be greater than %s", now.getTime(), startPlus
+      
-               .getTime());
+      assertOrdered(2000, duration, 2000+SLOW_BUILD_SERVER_GRACE);
-      assert now.compareTo(startPlus2) <= 0 : String.format("%s should be greater than %s", startPlus2.getTime(), now
+      assertCallFrequency(rawPredicate.callTimes, 0, 1000, 2000);
-               .getTime());
+   }
+   
+   private static class RepeatedAttemptsPredicate implements Predicate<String> {
+      final List<Long> callTimes = new ArrayList<Long>();
+      private final int succeedOnAttempt;
+      private final Stopwatch stopwatch;
+      private int count = 0;
+      
+      RepeatedAttemptsPredicate(int succeedOnAttempt) {
+         this.succeedOnAttempt = succeedOnAttempt;
+         this.stopwatch = new Stopwatch();
+         stopwatch.start();
+      }
+      @Override
+      public boolean apply(String input) {
+         callTimes.add(stopwatch.elapsedMillis());
+         return count++ == succeedOnAttempt;
+      }
+   }
+   
+   private void assertCallFrequency(List<Long> actual, Integer... expected) {
+      Assert.assertEquals(actual.size(), expected.length);
+      for (int i = 0; i < expected.length; i++) {
+         long callTime = actual.get(i);
+         assertOrdered(expected[i], callTime, expected[i]+SLOW_BUILD_SERVER_GRACE);
+      }
+   }
+   
+   private void assertOrdered(long... vals) {
+      long prevVal = vals[0];
+      for (long val : vals) {
+         assert val >= prevVal : String.format("%s should be ordered", vals);
+      }
    }
 }