perf(rollingpush): Avoid unnecessarily searching for instance ids (#1633

)

If a `serverGroupName` or `asgName` is available on the stage context,
there is no need to lookup each instance id individually.

Also, `getServerGroup()` is more efficient than `getServerGroupFromCluster()`
when dealing with clusters containing multiple server groups.

orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/instance/TerminateInstanceAndDecrementServerGroupTask.groovy

@@ -49,7 +49,10 @@ class TerminateInstanceAndDecrementServerGroupTask extends AbstractCloudProvider
 
     List<TerminatingInstance> remainingInstances = instanceSupport.remainingInstances(stage)
 
+    def serverGroupName = stage.context.serverGroupName ?: stage.context.asgName
+
     trafficGuard.verifyInstanceTermination(
+      serverGroupName,
       [stage.context.instance] as List<String>,
       account,
       Location.region(stage.context.region as String),

orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/instance/TerminateInstancesTask.groovy

@@ -49,7 +49,10 @@ class TerminateInstancesTask extends AbstractCloudProviderAwareTask implements T
 
     List<TerminatingInstance> remainingInstances = instanceSupport.remainingInstances(stage)
 
+    def serverGroupName = stage.context.serverGroupName ?: stage.context.asgName
+
     trafficGuard.verifyInstanceTermination(
+      serverGroupName,
       stage.context.instanceIds as List<String>,
       account,
       Location.region(stage.context.region as String),

orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/utils/TrafficGuard.java

@@ -46,13 +46,24 @@ public TrafficGuard(OortHelper oortHelper, Optional<Front50Service> front50Servi
     this.front50Service = front50Service.orElse(null);
   }
 
-  public void verifyInstanceTermination(List<String> instanceIds, String account, Location location, String cloudProvider, String operationDescriptor) {
+  public void verifyInstanceTermination(String serverGroupNameFromStage,
+                                        List<String> instanceIds,
+                                        String account,
+                                        Location location,
+                                        String cloudProvider,
+                                        String operationDescriptor) {
     Map<String, List<String>> instancesPerServerGroup = new HashMap<>();
-    instanceIds.forEach(instanceId -> {
+    for (String instanceId : instanceIds) {
+      String serverGroupName = serverGroupNameFromStage;
+      if (serverGroupName == null) {
+        Optional<String> resolvedServerGroupName = resolveServerGroupNameForInstance(instanceId, account, location.getValue(), cloudProvider);
+        serverGroupName = resolvedServerGroupName.orElse(null);
+      }
 
-      Optional<String> resolvedServerGroupName = resolveServerGroupNameForInstance(instanceId, account, location.getValue(), cloudProvider);
-      resolvedServerGroupName.ifPresent(name -> instancesPerServerGroup.computeIfAbsent(name, serverGroup -> new ArrayList<>()).add(instanceId));
-    });
+      if (serverGroupName != null) {
+        instancesPerServerGroup.computeIfAbsent(serverGroupName, serverGroup -> new ArrayList<>()).add(instanceId);
+      }
+    }
 
     instancesPerServerGroup.entrySet().forEach(entry -> {
       String serverGroupName = entry.getKey();

orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/kato/tasks/DisableInstancesTask.groovy

@@ -42,7 +42,10 @@ class DisableInstancesTask implements CloudProviderAware, Task {
     String cloudProvider = getCloudProvider(stage)
     String account = getCredentials(stage)
 
+    def serverGroupName = stage.context.serverGroupName ?: stage.context.asgName
+
     trafficGuard.verifyInstanceTermination(
+      serverGroupName,
       stage.context.instanceIds as List<String>,
       account,
       Location.region(stage.context.region as String),

orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/kato/tasks/rollingpush/WaitForNewUpInstancesLaunchTask.groovy

@@ -42,7 +42,14 @@ class WaitForNewUpInstancesLaunchTask implements RetryableTask {
   @Override
   TaskResult execute(Stage stage) {
     StageData stageData = stage.mapTo(StageData)
-    def response = oortService.getServerGroupFromCluster(stageData.application, stageData.account, stageData.cluster, stage.context.asgName as String, stage.context.region as String, stageData.cloudProvider ?: 'aws' )
+
+    // similar check in `AbstractInstancesCheckTask`
+    def response = oortService.getServerGroup(
+      stageData.application,
+      stageData.account,
+      stage.context.region as String,
+      stage.context.asgName as String
+    )
 
     Map serverGroup = objectMapper.readValue(response.body.in(), Map)
 

orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/utils/TrafficGuardSpec.groovy

@@ -231,8 +231,9 @@ class TrafficGuardSpec extends Specification {
     given:
     addGuard([account: "test", location: "us-east-1", stack: "foo"])
     targetServerGroup.instances = [[name: "i-1", healthState: "Up"], [name: "i-2", healthState: "Down"]]
+
     when:
-    trafficGuard.verifyInstanceTermination(["i-1"], "test", location, "aws", "x")
+    trafficGuard.verifyInstanceTermination(null, ["i-1"], "test", location, "aws", "x")
 
     then:
     thrown(IllegalStateException)
@@ -249,8 +250,9 @@ class TrafficGuardSpec extends Specification {
     addGuard([account: "test", location: "us-east-1", stack: "foo"])
     targetServerGroup.instances = [[name: "i-1", healthState: "Up"], [name: "i-2", healthState: "Down"]]
     otherServerGroup.instances = [[name: "i-1", healthState: "Down"]]
+
     when:
-    trafficGuard.verifyInstanceTermination(["i-1"], "test", location, "aws", "x")
+    trafficGuard.verifyInstanceTermination(null, ["i-1"], "test", location, "aws", "x")
 
     then:
     thrown(IllegalStateException)
@@ -267,8 +269,9 @@ class TrafficGuardSpec extends Specification {
     addGuard([account: "test", location: "us-east-1", stack: "foo"])
     targetServerGroup.instances = [[name: "i-1", healthState: "Up"], [name: "i-2", healthState: "Down"]]
     otherServerGroup.instances = [[name: "i-1", healthState: "Up"]]
+
     when:
-    trafficGuard.verifyInstanceTermination(["i-1"], "test", location, "aws", "x")
+    trafficGuard.verifyInstanceTermination(null, ["i-1"], "test", location, "aws", "x")
 
     then:
     notThrown(IllegalStateException)
@@ -285,8 +288,9 @@ class TrafficGuardSpec extends Specification {
     addGuard([account: "test", location: "us-east-1", stack: "foo"])
     targetServerGroup.instances = [[name: "i-1", healthState: "Up"], [name: "i-2", healthState: "Up"]]
     otherServerGroup.instances = [[name: "i-1", healthState: "Down"]]
+
     when:
-    trafficGuard.verifyInstanceTermination(["i-1", "i-2"], "test", location, "aws", "x")
+    trafficGuard.verifyInstanceTermination(null, ["i-1", "i-2"], "test", location, "aws", "x")
 
     then:
     thrown(IllegalStateException)
@@ -303,8 +307,9 @@ class TrafficGuardSpec extends Specification {
     given:
     addGuard([account: "test", location: "us-east-1", stack: "foo"])
     targetServerGroup.instances = [[name: "i-1"]]
+
     when:
-    trafficGuard.verifyInstanceTermination(["i-1"], "test", location, "aws", "x")
+    trafficGuard.verifyInstanceTermination(null, ["i-1"], "test", location, "aws", "x")
 
     then:
     notThrown(IllegalStateException)
@@ -314,6 +319,21 @@ class TrafficGuardSpec extends Specification {
     0 * _
   }
 
+  void "should avoid searching for instance ids when server group provided"() {
+    given:
+    addGuard([account: "test", location: "us-east-1", stack: "foo"])
+    targetServerGroup.instances = [[name: "i-1"]]
+
+    when:
+    trafficGuard.verifyInstanceTermination("app-foo-v001", ["i-1"], "test", location, "aws", "x")
+
+    then:
+    notThrown(IllegalStateException)
+    1 * front50Service.get("app") >> application
+    1 * oortHelper.getTargetServerGroup("test", "app-foo-v001", location.value, "aws") >> (targetServerGroup as TargetServerGroup)
+    0 * _
+  }
+
   private void addGuard(Map guard) {
     applicationDetails.putIfAbsent("trafficGuards", [])
     applicationDetails.get("trafficGuards") << guard

orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/kato/tasks/rollingpush/WaitForNewUpInstancesLaunchTaskSpec.groovy

@@ -54,7 +54,7 @@ class WaitForNewUpInstancesLaunchTaskSpec extends Specification {
     def response = task.execute(stage)
 
     then:
-    1 * oortService.getServerGroupFromCluster(application, account, cluster, serverGroup, region, cloudProvider) >> oortResponse
+    1 * oortService.getServerGroup(application, account, region, serverGroup) >> oortResponse
     response.status == expectedStatus