fix(entitytags): correctly fetch prior asg details without depending …

…on forceCacheRefresh (#2928)
spinnaker · May 20, 2019 · cb5e947 · cb5e947
1 parent dac2670
commit cb5e947
Show file tree

Hide file tree

Showing 2 changed files with 56 additions and 39 deletions.
diff --git a/...pinnaker/orca/clouddriver/tasks/servergroup/SpinnakerMetadataServerGroupTagGenerator.java b/...pinnaker/orca/clouddriver/tasks/servergroup/SpinnakerMetadataServerGroupTagGenerator.java
@@ -113,30 +113,31 @@ Map<String, Object> getPreviousServerGroupFromCluster(
       String cloudProvider,
       String location,
       String createdServerGroup) {
-    if (cloudProvider.equals("titus")) {
-      // titus does not (currently!) force cache refresh to it's possible that `NEWEST` is actually
-      // the `ANCESTOR` to
-      // the just created server group
-      Map<String, Object> newestServerGroup =
-          retrySupport.retry(
-              () -> {
-                return getPreviousServerGroupFromClusterByTarget(
-                    application, account, cluster, cloudProvider, location, "NEWEST");
-              },
-              10,
-              3000,
-              false); // retry for up to 30 seconds
-
-      if (newestServerGroup == null) {
-        // cluster has no enabled server groups
-        return null;
-      }
 
-      if (!newestServerGroup.get("name").equals(createdServerGroup)) {
-        // if the `NEWEST` server group is _NOT_ what was just created, we've found our `ANCESTOR`
-        // if not, fall through to an explicit `ANCESTOR` lookup
-        return newestServerGroup;
-      }
+    // When forceCacheRefresh is disabled (i.e. via the
+    // stages.createServerGroupStage.forceCacheRefresh.enabled
+    // property), or unsupported by the cloudProvider (true for titus), NEWEST may reflect the
+    // ancestor and not
+    // the serverGroup just created. If not, an explicit ANCESTOR lookup is attempted.
+    Map<String, Object> newestServerGroup =
+        retrySupport.retry(
+            () -> {
+              return getPreviousServerGroupFromClusterByTarget(
+                  application, account, cluster, cloudProvider, location, "NEWEST");
+            },
+            10,
+            3000,
+            false); // retry for up to 30 seconds
+
+    if (newestServerGroup == null) {
+      // cluster has no enabled server groups
+      return null;
+    }
+
+    if (!newestServerGroup.get("name").equals(createdServerGroup)) {
+      // if the `NEWEST` server group is _NOT_ what was just created, we've found our `ANCESTOR`
+      // if not, fall through to an explicit `ANCESTOR` lookup
+      return newestServerGroup;
     }
 
     return retrySupport.retry(

diff --git a/...er/orca/clouddriver/tasks/servergroup/SpinnakerMetadataServerGroupTagGeneratorSpec.groovy b/...er/orca/clouddriver/tasks/servergroup/SpinnakerMetadataServerGroupTagGeneratorSpec.groovy
@@ -46,7 +46,11 @@ class SpinnakerMetadataServerGroupTagGeneratorSpec extends Specification {
   def "should build spinnaker:metadata tag for pipeline"() {
     given:
     def tagGenerator = Spy(SpinnakerMetadataServerGroupTagGenerator, constructorArgs: [oortService, retrySupport]) {
-      1 * getPreviousServerGroupFromClusterByTarget(_, _, _, _, _, "ANCESTOR") >> { return previousServerGroup }
+      1 * getPreviousServerGroupFromClusterByTarget(_, _, _, _, _, "NEWEST") >> { return newestServerGroup }
+
+      if (newestServerGroup != null && newestServerGroup != previousServerGroup) {
+        1 * getPreviousServerGroupFromClusterByTarget(_, _, _, _, _, "ANCESTOR") >> { return previousServerGroup }
+      }
     }
 
     def pipeline = pipeline {
@@ -66,9 +70,9 @@ class SpinnakerMetadataServerGroupTagGeneratorSpec extends Specification {
 
     then:
     tags == [[
-               name : "spinnaker:metadata",
+               name     : "spinnaker:metadata",
                namespace: "spinnaker",
-               value: [
+               value    : [
                  executionId     : pipeline.id,
                  pipelineConfigId: "configId",
                  application     : "application",
@@ -81,17 +85,21 @@ class SpinnakerMetadataServerGroupTagGeneratorSpec extends Specification {
              ]]
 
     where:
-    previousServerGroup                   | authenticatedUser || _
-    null                                  | null              || _    // metadata tag should NOT include `previousServerGroup`
-    null                                  | "username"        || _    // include user if non-null
-    [serverGroupName: "application-v001"] | null              || _
+    newestServerGroup          | previousServerGroup        | authenticatedUser || _
+    null                       | null                       | null              || _    // metadata tag should NOT include `previousServerGroup`
+    null                       | null                       | "username"        || _    // include user if non-null
+    [name: "application-v002"] | [name: "application-v001"] | null              || _    // NEWEST is checked first, falling back to ANCESTOR
+    [name: "application-v001"] | [name: "application-v001"] | null              || _    // NEWEST is still cached as the ANCESTOR, no fallback
   }
 
   @Unroll
   def "should build spinnaker:metadata tag for orchestration"() {
     given:
     def tagGenerator = Spy(SpinnakerMetadataServerGroupTagGenerator, constructorArgs: [oortService, retrySupport]) {
-      1 * getPreviousServerGroupFromClusterByTarget(_, _, _, _, _, "ANCESTOR") >> { return previousServerGroup }
+      1 * getPreviousServerGroupFromClusterByTarget(_, _, _, _, _, "NEWEST") >> { return newestServerGroup }
+      if (newestServerGroup != null && newestServerGroup != previousServerGroup) {
+        1 * getPreviousServerGroupFromClusterByTarget(_, _, _, _, _, "ANCESTOR") >> { return previousServerGroup }
+      }
     }
 
     def orchestration = orchestration {
@@ -110,7 +118,7 @@ class SpinnakerMetadataServerGroupTagGeneratorSpec extends Specification {
     tags == [[
                name     : "spinnaker:metadata",
                namespace: "spinnaker",
-               value: [
+               value    : [
                  executionId  : orchestration.id,
                  application  : "application",
                  executionType: "orchestration",
@@ -121,10 +129,11 @@ class SpinnakerMetadataServerGroupTagGeneratorSpec extends Specification {
              ]]
 
     where:
-    previousServerGroup                   | authenticatedUser || _
-    null                                  | null              || _    // metadata tag should NOT include `previousServerGroup`
-    null                                  | "username"        || _    // include user if non-null
-    [serverGroupName: "application-v001"] | null              || _
+    newestServerGroup          | previousServerGroup        | authenticatedUser || _
+    null                       | null                       | null              || _    // metadata tag should NOT include `previousServerGroup`
+    null                       | null                       | "username"        || _    // include user if non-null
+    [name: "application-v002"] | [name: "application-v001"] | null              || _    // NEWEST is queried, then falls back to ANCESTOR
+    [name: "application-v001"] | [name: "application-v001"] | null              || _    // NEWEST is still cached as the ANCESTOR, no fallback
   }
 
   def "should construct previous server group metadata when present"() {
@@ -137,6 +146,13 @@ class SpinnakerMetadataServerGroupTagGeneratorSpec extends Specification {
     )
 
     then: "metadata should be returned"
+    1 * oortService.getServerGroupSummary("application", "account", "cluster", "aws", "us-west-2", "NEWEST", "image", "true") >> {
+      return [
+        serverGroupName: "application-v002",
+        imageId        : "ami-f234567",
+        imageName      : "my_image"
+      ]
+    }
     1 * oortService.getServerGroupSummary("application", "account", "cluster", "aws", "us-west-2", "ANCESTOR", "image", "true") >> {
       return [
         serverGroupName: "application-v001",
@@ -158,14 +174,14 @@ class SpinnakerMetadataServerGroupTagGeneratorSpec extends Specification {
     )
 
     then: "no metadata should be returned"
-    1 * oortService.getServerGroupSummary("application", "account", "cluster", "aws", "us-west-2", "ANCESTOR", "image", "true") >> {
+    1 * oortService.getServerGroupSummary("application", "account", "cluster", "aws", "us-west-2", "NEWEST", "image", "true") >> {
       throw notFoundError
     }
-    0 * oortService._
+    0 * oortService._ // when NEWEST is not found, no fallback is made to query for ANCESTOR
     previousServerGroupMetadata == null
   }
 
-  def "should check NEWEST and ANCESTOR when constructing previous server group metadata for titus"() {
+  def "should check NEWEST and ANCESTOR when constructing previous server group metadata"() {
     given:
     def tagGenerator = new SpinnakerMetadataServerGroupTagGenerator(oortService, retrySupport)