Netflix · alfasin · Mar 11, 2014 · Mar 7, 2014 · Mar 7, 2014 · Mar 11, 2014
diff --git a/src/test/java/com/netflix/simianarmy/TestUtils.java b/src/test/java/com/netflix/simianarmy/TestUtils.java
@@ -0,0 +1,42 @@
+package com.netflix.simianarmy;
+
+import static org.joda.time.DateTimeConstants.MILLIS_PER_DAY;
+
+import org.joda.time.DateTime;
+import org.testng.Assert;
+
+/** Utility class for test cases.
+ * @author mgeis
+ *
+ */
+public final class TestUtils {
+
+    private TestUtils() {
+        //this should never be called
+        //if called internally, throw an error
+        throw new InstantiationError("Instantiation of TestUtils utility class prohibited.");
+    }
+
+    /** Verify that the termination date is roughly retentionDays from now
+     * By 'roughly' we mean within one day.  There are times (twice per year)
+     * when certain tests execute and the Daylight Savings cutover makes it not
+     * a precisely rounded day amount (for example, a termination policy of 4 days
+     * will really be about 3.95 days, or 95 hours, because one hour is lost as
+     * the clocks "spring ahead").
+     *
+     * A more precise, but complicated logic could be written to make sure that "roughly"
+     * means not more than an hour before and not more than an hour after the anticipated
+     * cutoff, but that makes the test much less readable.
+     *
+     * By just making sure that the difference between the actual and proposed dates
+     * is less than one day, we get a rough idea of whether the termination time was correct.
+     * @param resource The AWS Resource to be checked
+     * @param retentionDays number of days it should be kept around
+     * @param timeOfCheck The time the check is run
+     */
+    public static void verifyTerminationTimeRough(Resource resource, int retentionDays, DateTime timeOfCheck) {
+        long days = (resource.getExpectedTerminationTime().getTime() - timeOfCheck.getMillis()) / MILLIS_PER_DAY;
+        Assert.assertTrue(Math.abs(days - retentionDays) <= 1);
+    }
+
+}
diff --git a/src/test/java/com/netflix/simianarmy/aws/janitor/rule/asg/TestOldEmptyASGRule.java b/src/test/java/com/netflix/simianarmy/aws/janitor/rule/asg/TestOldEmptyASGRule.java
@@ -28,6 +28,7 @@
 
 import com.netflix.simianarmy.MonkeyCalendar;
 import com.netflix.simianarmy.Resource;
+import com.netflix.simianarmy.TestUtils;
 import com.netflix.simianarmy.aws.AWSResource;
 import com.netflix.simianarmy.aws.AWSResourceType;
 import com.netflix.simianarmy.aws.janitor.crawler.ASGJanitorCrawler;
@@ -49,7 +50,7 @@ public void testEmptyASGWithObsoleteLaunchConfig() {
         OldEmptyASGRule rule = new OldEmptyASGRule(calendar, launchConfiguAgeThreshold, retentionDays,
                 new DummyASGInstanceValidator());
         Assert.assertFalse(rule.isValid(resource));
-        verifyTerminationTime(resource, retentionDays, now);
+        TestUtils.verifyTerminationTimeRough(resource, retentionDays, now);
     }
 
     @Test
@@ -115,7 +116,7 @@ public void testEmptyASGWithoutLaunchConfig() {
         OldEmptyASGRule rule = new OldEmptyASGRule(calendar, launchConfiguAgeThreshold, retentionDays,
                 new DummyASGInstanceValidator());
         Assert.assertFalse(rule.isValid(resource));
-        verifyTerminationTime(resource, retentionDays, now);
+        TestUtils.verifyTerminationTimeRough(resource, retentionDays, now);
     }
 
     @Test
@@ -185,9 +186,4 @@ public void testNonASGResource() {
         Assert.assertNull(resource.getExpectedTerminationTime());
     }
 
-    /** Verify that the termination date is roughly rentionDays from now **/
-    private void verifyTerminationTime(Resource resource, int retentionDays, DateTime now) {
-        long days = (resource.getExpectedTerminationTime().getTime() - now.getMillis()) / (24 * 60 * 60 * 1000);
-        Assert.assertEquals(days, retentionDays);
-    }
 }
diff --git a/src/test/java/com/netflix/simianarmy/aws/janitor/rule/asg/TestSuspendedASGRule.java b/src/test/java/com/netflix/simianarmy/aws/janitor/rule/asg/TestSuspendedASGRule.java
@@ -28,6 +28,7 @@
 
 import com.netflix.simianarmy.MonkeyCalendar;
 import com.netflix.simianarmy.Resource;
+import com.netflix.simianarmy.TestUtils;
 import com.netflix.simianarmy.aws.AWSResource;
 import com.netflix.simianarmy.aws.AWSResourceType;
 import com.netflix.simianarmy.aws.janitor.crawler.ASGJanitorCrawler;
@@ -49,7 +50,7 @@ public void testEmptyASGSuspendedMoreThanThreshold() {
         SuspendedASGRule rule = new SuspendedASGRule(calendar, suspensionAgeThreshold, retentionDays,
                 new DummyASGInstanceValidator());
         Assert.assertFalse(rule.isValid(resource));
-        verifyTerminationTime(resource, retentionDays, now);
+        TestUtils.verifyTerminationTimeRough(resource, retentionDays, now);
     }
 
     @Test
@@ -186,9 +187,4 @@ public void testNullCalendar() {
         new SuspendedASGRule(null, 3, 2, null);
     }
 
-    /** Verify that the termination date is roughly rentionDays from now **/
-    private void verifyTerminationTime(Resource resource, int retentionDays, DateTime now) {
-        long days = (resource.getExpectedTerminationTime().getTime() - now.getMillis()) / (24 * 60 * 60 * 1000);
-        Assert.assertEquals(days, retentionDays);
-    }
 }
diff --git a/src/test/java/com/netflix/simianarmy/aws/janitor/rule/instance/TestOrphanedInstanceRule.java b/src/test/java/com/netflix/simianarmy/aws/janitor/rule/instance/TestOrphanedInstanceRule.java
@@ -27,6 +27,7 @@
 import org.testng.annotations.Test;
 
 import com.netflix.simianarmy.Resource;
+import com.netflix.simianarmy.TestUtils;
 import com.netflix.simianarmy.aws.AWSResource;
 import com.netflix.simianarmy.aws.AWSResourceType;
 import com.netflix.simianarmy.aws.janitor.crawler.InstanceJanitorCrawler;
@@ -47,7 +48,7 @@ public void testOrphanedInstancesWithOwner() {
         OrphanedInstanceRule rule = new OrphanedInstanceRule(new TestMonkeyCalendar(),
                 ageThreshold, retentionDaysWithOwner, retentionDaysWithoutOwner);
         Assert.assertFalse(rule.isValid(resource));
-        verifyTerminationTime(resource, retentionDaysWithOwner, now);
+        TestUtils.verifyTerminationTimeRough(resource, retentionDaysWithOwner, now);
     }
 
     @Test
@@ -62,7 +63,7 @@ public void testOrphanedInstancesWithoutOwner() {
         OrphanedInstanceRule rule = new OrphanedInstanceRule(new TestMonkeyCalendar(),
                 ageThreshold, retentionDaysWithOwner, retentionDaysWithoutOwner);
         Assert.assertFalse(rule.isValid(resource));
-        verifyTerminationTime(resource, retentionDaysWithoutOwner, now);
+        TestUtils.verifyTerminationTimeRough(resource, retentionDaysWithoutOwner, now);
     }
 
     @Test
@@ -171,10 +172,4 @@ public void testNonRunningInstance() {
         Assert.assertNull(resource.getExpectedTerminationTime());
     }
 
-    /** Verify that the termination date is roughly rentionDays from now **/
-    private void verifyTerminationTime(Resource resource, int retentionDays, DateTime now) {
-        int hours = (int) (resource.getExpectedTerminationTime().getTime() - now.getMillis()) / (60 * 60 * 1000);
-        // There could be a 1-hour difference if the time passes the daylight saving time change
-        Assert.assertTrue(hours >= retentionDays * 24 - 1 && hours <= retentionDays * 24 + 1);
-    }
 }
diff --git a/...a/com/netflix/simianarmy/aws/janitor/rule/launchconfig/TestOldUnusedLaunchConfigRule.java b/...a/com/netflix/simianarmy/aws/janitor/rule/launchconfig/TestOldUnusedLaunchConfigRule.java
@@ -22,6 +22,7 @@
 
 import com.netflix.simianarmy.MonkeyCalendar;
 import com.netflix.simianarmy.Resource;
+import com.netflix.simianarmy.TestUtils;
 import com.netflix.simianarmy.aws.AWSResource;
 import com.netflix.simianarmy.aws.AWSResourceType;
 import com.netflix.simianarmy.aws.janitor.crawler.LaunchConfigJanitorCrawler;
@@ -46,7 +47,7 @@ public void testOldUnsedLaunchConfig() {
         int retentionDays = 3;
         OldUnusedLaunchConfigRule rule = new OldUnusedLaunchConfigRule(calendar, ageThreshold, retentionDays);
         Assert.assertFalse(rule.isValid(resource));
-        verifyTerminationTime(resource, retentionDays, now);
+        TestUtils.verifyTerminationTimeRough(resource, retentionDays, now);
     }
 
     @Test
@@ -150,9 +151,4 @@ public void testNonLaunchConfigResource() {
         Assert.assertNull(resource.getExpectedTerminationTime());
     }
 
-    /** Verify that the termination date is roughly rentionDays from now **/
-    private void verifyTerminationTime(Resource resource, int retentionDays, DateTime now) {
-        long days = (resource.getExpectedTerminationTime().getTime() - now.getMillis()) / (24 * 60 * 60 * 1000);
-        Assert.assertEquals(days, retentionDays);
-    }
 }
diff --git a/src/test/java/com/netflix/simianarmy/aws/janitor/rule/snapshot/TestNoGeneratedAMIRule.java b/src/test/java/com/netflix/simianarmy/aws/janitor/rule/snapshot/TestNoGeneratedAMIRule.java
@@ -27,6 +27,7 @@
 import org.testng.annotations.Test;
 
 import com.netflix.simianarmy.Resource;
+import com.netflix.simianarmy.TestUtils;
 import com.netflix.simianarmy.aws.AWSResource;
 import com.netflix.simianarmy.aws.AWSResourceType;
 import com.netflix.simianarmy.aws.janitor.crawler.EBSSnapshotJanitorCrawler;
@@ -97,7 +98,7 @@ public void testOldSnapshotWithoutAMI() {
         NoGeneratedAMIRule rule = new NoGeneratedAMIRule(new TestMonkeyCalendar(),
                 ageThreshold, retentionDays);
         Assert.assertFalse(rule.isValid(resource));
-        verifyTerminationTime(resource, retentionDays, now);
+        TestUtils.verifyTerminationTimeRough(resource, retentionDays, now);
     }
 
     @Test
@@ -182,9 +183,4 @@ public void testNullCalendar() {
         new NoGeneratedAMIRule(null, 5, 4);
     }
 
-    /** Verify that the termination date is roughly rentionDays from now **/
-    private void verifyTerminationTime(Resource resource, int retentionDays, DateTime now) {
-        long days = (resource.getExpectedTerminationTime().getTime() - now.getMillis()) / (24 * 60 * 60 * 1000);
-        Assert.assertEquals(days, retentionDays);
-    }
 }
diff --git a/src/test/java/com/netflix/simianarmy/aws/janitor/rule/volume/TestOldDetachedVolumeRule.java b/src/test/java/com/netflix/simianarmy/aws/janitor/rule/volume/TestOldDetachedVolumeRule.java
@@ -20,19 +20,29 @@
 
 package com.netflix.simianarmy.aws.janitor.rule.volume;
 
+import java.util.Calendar;
 import java.util.Date;
+import java.util.TimeZone;
 
 import org.joda.time.DateTime;
+import org.joda.time.DateTimeZone;
 import org.testng.Assert;
 import org.testng.annotations.Test;
 
+import com.netflix.simianarmy.MonkeyCalendar;
 import com.netflix.simianarmy.Resource;
+import com.netflix.simianarmy.TestUtils;
 import com.netflix.simianarmy.aws.AWSResource;
 import com.netflix.simianarmy.aws.AWSResourceType;
 import com.netflix.simianarmy.aws.janitor.VolumeTaggingMonkey;
 import com.netflix.simianarmy.aws.janitor.rule.TestMonkeyCalendar;
 import com.netflix.simianarmy.janitor.JanitorMonkey;
 
+import static org.joda.time.DateTimeConstants.MILLIS_PER_DAY;
+
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.when;
+
 
 public class TestOldDetachedVolumeRule {
 
@@ -118,7 +128,59 @@ public void testOldDetachedVolume() {
         OldDetachedVolumeRule rule = new OldDetachedVolumeRule(new TestMonkeyCalendar(),
                 ageThreshold, retentionDays);
         Assert.assertFalse(rule.isValid(resource));
-        verifyTerminationTime(resource, retentionDays, now);
+        TestUtils.verifyTerminationTimeRough(resource, retentionDays, now);
+    }
+
+    /** This test exists to check logic on a utility method.
+     * The tagging rule for resource expiry uses a variable nubmer of days.
+     * However, JodaTime date arithmetic for DAYS uses calendar days.  It does NOT
+     * treat a day as 24 hours in this case (HOUR arithmetic, however, does).
+     * Therefore, a termination policy of 4 days (96 hours) will actually occur in
+     * 95 hours if the resource is tagged with that rule within 4 days of the DST
+     * cutover.
+     *
+     * We experienced test case failures around the 2014 spring DST cutover that
+     * prevented us from getting green builds.  So, the assertion logic was loosened
+     * to check that we were within a day of the expected date.  For the test case,
+     * all but 4 days of the year this problem never shows up.  To verify that our
+     * fix was correct, this test case explicitly sets the date.  The other tests
+     * that use a DateTime of "DateTime.now()" are not true unit tests, because the
+     * test does not isolate the date.  They are actually a partial integration test,
+     * as they leave the date up to the system where the test executes.
+     *
+     * We have to mock the call to MonkeyCalendar.now() because the constructor
+     * for that class uses Calendar.getInstance() internally.
+     *
+     */
+    @Test
+    public void testOldDetachedVolumeBeforeDaylightSavingsCutover() {
+        int ageThreshold = 5;
+        //here we set the create date to a few days before a known DST cutover, where
+        //we observed DST failures
+        DateTime closeToSpringAheadDst = new DateTime(2014, 3, 7, 0, 0, DateTimeZone.forID("America/Los_Angeles"));
+        Resource resource = new AWSResource().withId("vol-123").withResourceType(AWSResourceType.EBS_VOLUME)
+            .withLaunchTime(new Date(closeToSpringAheadDst.minusDays(ageThreshold + 1).getMillis()));
+        ((AWSResource) resource).setAWSResourceState("available");
+        Date lastDetachTime = new Date(closeToSpringAheadDst.minusDays(ageThreshold + 1).getMillis());
+        String metaTag = VolumeTaggingMonkey.makeMetaTag(null, null, lastDetachTime);
+        resource.setTag(JanitorMonkey.JANITOR_META_TAG, metaTag);
+        int retentionDays = 4;
+
+        //set the "now" to the fixed execution date for this rule and create a partial mock
+        Calendar fixed = Calendar.getInstance(TimeZone.getTimeZone("America/Los_Angeles"));
+        fixed.setTimeInMillis(closeToSpringAheadDst.getMillis());
+        MonkeyCalendar monkeyCalendar = new TestMonkeyCalendar();
+        MonkeyCalendar spyCalendar = spy(monkeyCalendar);
+        when(spyCalendar.now()).thenReturn(fixed);
+
+        //use the partial mock for the OldDetachedVolumeRule
+        OldDetachedVolumeRule rule = new OldDetachedVolumeRule(spyCalendar, ageThreshold, retentionDays);
+        Assert.assertFalse(rule.isValid(resource)); //this volume should be seen as invalid
+        //now verify that the difference between "now" and the cutoff is slightly under the intended
+        //retention limit, as the DST cutover makes us lose one hour
+        verifyDSTCutoverHappened(resource, retentionDays, closeToSpringAheadDst);
+        //now verify that our projected termination time is within one day of what was asked for
+        TestUtils.verifyTerminationTimeRough(resource, retentionDays, closeToSpringAheadDst);
     }
 
     @Test
@@ -139,7 +201,7 @@ public void testDetachedVolumeNotOld() {
     }
 
     @Test
-    public void testAchedVolume() {
+    public void testAttachedVolume() {
         int ageThreshold = 5;
         DateTime now = DateTime.now();
         Resource resource = new AWSResource().withId("vol-123").withResourceType(AWSResourceType.EBS_VOLUME)
@@ -198,9 +260,19 @@ public void testNullCalendar() {
         new OldDetachedVolumeRule(null, 5, 4);
     }
 
-    /** Verify that the termination date is roughly rentionDays from now **/
-    private void verifyTerminationTime(Resource resource, int retentionDays, DateTime now) {
-        long days = (resource.getExpectedTerminationTime().getTime() - now.getMillis()) / (24 * 60 * 60 * 1000);
-        Assert.assertEquals(days, retentionDays);
+    /** Verify that a test conditioned to run across the spring DST cutover actually did
+     * cross that threshold.  The real difference will be about 0.05 days less than
+     * the retentionDays parameter.
+     * @param resource The AWS resource being tested
+     * @param retentionDays Number of days the resource should be kept around
+     * @param timeOfCheck When the check is executed
+     */
+    private void verifyDSTCutoverHappened(Resource resource, int retentionDays, DateTime timeOfCheck) {
+        double realDays = (double) (resource.getExpectedTerminationTime().getTime() - timeOfCheck.getMillis())
+            / (double) MILLIS_PER_DAY;
+        long days = (resource.getExpectedTerminationTime().getTime() - timeOfCheck.getMillis()) / MILLIS_PER_DAY;
+        Assert.assertTrue(realDays < (double) retentionDays);
+        Assert.assertNotEquals(days, retentionDays);
     }
+
 }