diff --git a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/servergroup/SpinnakerMetadataServerGroupTagGenerator.java b/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/servergroup/SpinnakerMetadataServerGroupTagGenerator.java index 4fa9d030e2..a2e5a441fe 100644 --- a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/servergroup/SpinnakerMetadataServerGroupTagGenerator.java +++ b/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/servergroup/SpinnakerMetadataServerGroupTagGenerator.java @@ -45,7 +45,11 @@ public SpinnakerMetadataServerGroupTagGenerator(OortService oortService, RetrySu } @Override - public Collection> generateTags(Stage stage, String serverGroup, String account, String location, String cloudProvider) { + public Collection> generateTags(Stage stage, + String serverGroup, + String account, + String location, + String cloudProvider) { Execution execution = stage.getExecution(); Map context = stage.getContext(); @@ -77,12 +81,13 @@ public Collection> generateTags(Stage stage, String serverGr try { cluster = Names.parseName(serverGroup).getCluster(); - Map previousServerGroup = getPreviousServerGroup( + Map previousServerGroup = getPreviousServerGroupFromCluster( execution.getApplication(), account, cluster, cloudProvider, - location + location, + serverGroup ); if (previousServerGroup != null) { @@ -100,48 +105,76 @@ public Collection> generateTags(Stage stage, String serverGr return Collections.singletonList(tag); } - Map getPreviousServerGroup(String application, - String account, - String cluster, - String cloudProvider, - String location) { + Map getPreviousServerGroupFromCluster(String application, + String account, + String cluster, + String cloudProvider, + String location, + String createdServerGroup) { if (cloudProvider.equals("titus")) { - // TODO-AJ titus does not force cache refresh so `ANCESTOR` will return inconsistent results - return null; + // titus does not (currently!) force cache refresh to it's possible that `NEWEST` is actually the `ANCESTOR` to + // the just created server group + Map newestServerGroup = retrySupport.retry(() -> { + return getPreviousServerGroupFromClusterByTarget( + application, account, cluster, cloudProvider, location, "NEWEST" + ); + }, 12, 5000, false); // retry for up to one minute + + 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(() -> { - try { - Map targetServerGroup = oortService.getServerGroupSummary( - application, - account, - cluster, - cloudProvider, - location, - "ANCESTOR", - "image", - "true" - ); + return getPreviousServerGroupFromClusterByTarget( + application, account, cluster, cloudProvider, location, "ANCESTOR" + ); + }, 12, 5000, false); // retry for up to one minute + } - Map previousServerGroup = new HashMap<>(); - previousServerGroup.put("name", targetServerGroup.get("serverGroupName")); - previousServerGroup.put("imageName", targetServerGroup.get("imageName")); - previousServerGroup.put("imageId", targetServerGroup.get("imageId")); - previousServerGroup.put("cloudProvider", cloudProvider); + Map getPreviousServerGroupFromClusterByTarget(String application, + String account, + String cluster, + String cloudProvider, + String location, + String target) { + try { + Map targetServerGroup = oortService.getServerGroupSummary( + application, + account, + cluster, + cloudProvider, + location, + target, + "image", + "true" + ); - if (targetServerGroup.containsKey("buildInfo")) { - previousServerGroup.put("buildInfo", targetServerGroup.get("buildInfo")); - } + Map previousServerGroup = new HashMap<>(); + previousServerGroup.put("name", targetServerGroup.get("serverGroupName")); + previousServerGroup.put("imageName", targetServerGroup.get("imageName")); + previousServerGroup.put("imageId", targetServerGroup.get("imageId")); + previousServerGroup.put("cloudProvider", cloudProvider); - return previousServerGroup; - } catch (RetrofitError e) { - if (e.getKind() == RetrofitError.Kind.HTTP && e.getResponse().getStatus() == 404) { - // it's ok if the previous server group does not exist - return null; - } + if (targetServerGroup.containsKey("buildInfo")) { + previousServerGroup.put("buildInfo", targetServerGroup.get("buildInfo")); + } - throw e; + return previousServerGroup; + } catch (RetrofitError e) { + if (e.getKind() == RetrofitError.Kind.HTTP && e.getResponse().getStatus() == 404) { + // it's ok if the previous server group does not exist + return null; } - }, 12, 5000, false); // retry for up to one minute + + throw e; + } } } diff --git a/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/servergroup/SpinnakerMetadataServerGroupTagGeneratorSpec.groovy b/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/servergroup/SpinnakerMetadataServerGroupTagGeneratorSpec.groovy index c5ee097ad9..787381294a 100644 --- a/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/servergroup/SpinnakerMetadataServerGroupTagGeneratorSpec.groovy +++ b/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/servergroup/SpinnakerMetadataServerGroupTagGeneratorSpec.groovy @@ -49,7 +49,7 @@ class SpinnakerMetadataServerGroupTagGeneratorSpec extends Specification { def "should build spinnaker:metadata tag for pipeline"() { given: def tagGenerator = Spy(SpinnakerMetadataServerGroupTagGenerator, constructorArgs: [oortService, retrySupport]) { - 1 * getPreviousServerGroup(_, _, _, _, _) >> { return previousServerGroup } + 1 * getPreviousServerGroupFromClusterByTarget(_, _, _, _, _, "ANCESTOR") >> { return previousServerGroup } } def pipeline = pipeline { @@ -93,7 +93,7 @@ class SpinnakerMetadataServerGroupTagGeneratorSpec extends Specification { def "should build spinnaker:metadata tag for orchestration"() { given: def tagGenerator = Spy(SpinnakerMetadataServerGroupTagGenerator, constructorArgs: [oortService, retrySupport]) { - 1 * getPreviousServerGroup(_, _, _, _, _) >> { return previousServerGroup } + 1 * getPreviousServerGroupFromClusterByTarget(_, _, _, _, _, "ANCESTOR") >> { return previousServerGroup } } def orchestration = orchestration { @@ -133,8 +133,8 @@ class SpinnakerMetadataServerGroupTagGeneratorSpec extends Specification { def tagGenerator = new SpinnakerMetadataServerGroupTagGenerator(oortService, retrySupport) when: "previous server does exist" - def previousServerGroupMetadata = tagGenerator.getPreviousServerGroup( - "application", "account", "cluster", "aws", "us-west-2" + def previousServerGroupMetadata = tagGenerator.getPreviousServerGroupFromCluster( + "application", "account", "cluster", "aws", "us-west-2", "application-v002" ) then: "metadata should be returned" @@ -145,6 +145,7 @@ class SpinnakerMetadataServerGroupTagGeneratorSpec extends Specification { imageName : "my_image" ] } + 0 * oortService._ previousServerGroupMetadata == [ name : "application-v001", imageId : "ami-1234567", @@ -153,14 +154,69 @@ class SpinnakerMetadataServerGroupTagGeneratorSpec extends Specification { ] when: "previous server group does NOT exist" - previousServerGroupMetadata = tagGenerator.getPreviousServerGroup( - "application", "account", "cluster", "aws", "us-west-2" + previousServerGroupMetadata = tagGenerator.getPreviousServerGroupFromCluster( + "application", "account", "cluster", "aws", "us-west-2", "application-v002" ) then: "no metadata should be returned" 1 * oortService.getServerGroupSummary("application", "account", "cluster", "aws", "us-west-2", "ANCESTOR", "image", "true") >> { throw notFoundError } + 0 * oortService._ previousServerGroupMetadata == null } + + def "should check NEWEST and ANCESTOR when constructing previous server group metadata for titus"() { + given: + def tagGenerator = new SpinnakerMetadataServerGroupTagGenerator(oortService, retrySupport) + + when: "NEWEST != just created server group" + def previousServerGroupMetadata = tagGenerator.getPreviousServerGroupFromCluster( + "application", "account", "cluster", "titus", "us-west-2", "application-v002" + ) + + then: "previous server group == NEWEST" + 1 * oortService.getServerGroupSummary("application", "account", "cluster", "titus", "us-west-2", "NEWEST", "image", "true") >> { + return [ + serverGroupName: "application-v001", + imageId : "1234567", + imageName : "my_image" + ] + } + 0 * oortService._ + previousServerGroupMetadata == [ + name : "application-v001", + imageId : "1234567", + imageName : "my_image", + cloudProvider: "titus" + ] + + when: "NEWEST == just created server group" + previousServerGroupMetadata = tagGenerator.getPreviousServerGroupFromCluster( + "application", "account", "cluster", "titus", "us-west-2", "application-v002" + ) + + then: "previous server group == ANCESTOR" + 1 * oortService.getServerGroupSummary("application", "account", "cluster", "titus", "us-west-2", "NEWEST", "image", "true") >> { + return [ + serverGroupName: "application-v002", + imageId : "1234567", + imageName : "my_image" + ] + } + 1 * oortService.getServerGroupSummary("application", "account", "cluster", "titus", "us-west-2", "ANCESTOR", "image", "true") >> { + return [ + serverGroupName: "application-v001", + imageId : "1234567", + imageName : "my_image" + ] + } + 0 * oortService._ + previousServerGroupMetadata == [ + name : "application-v001", + imageId : "1234567", + imageName : "my_image", + cloudProvider: "titus" + ] + } }