From 98b1ebf974403ae1b3965d53cd143a7c0b6ef9c1 Mon Sep 17 00:00:00 2001 From: Adam Jordens Date: Wed, 20 Sep 2017 08:16:32 -0700 Subject: [PATCH] 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. --- ...InstanceAndDecrementServerGroupTask.groovy | 3 ++ .../instance/TerminateInstancesTask.groovy | 3 ++ .../orca/clouddriver/utils/TrafficGuard.java | 21 +++++++++---- .../kato/tasks/DisableInstancesTask.groovy | 3 ++ .../WaitForNewUpInstancesLaunchTask.groovy | 9 +++++- .../clouddriver/utils/TrafficGuardSpec.groovy | 30 +++++++++++++++---- ...WaitForNewUpInstancesLaunchTaskSpec.groovy | 2 +- 7 files changed, 59 insertions(+), 12 deletions(-) diff --git a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/instance/TerminateInstanceAndDecrementServerGroupTask.groovy b/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/instance/TerminateInstanceAndDecrementServerGroupTask.groovy index 4e3eea1afe..a4693fdd8f 100644 --- a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/instance/TerminateInstanceAndDecrementServerGroupTask.groovy +++ b/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/instance/TerminateInstanceAndDecrementServerGroupTask.groovy @@ -49,7 +49,10 @@ class TerminateInstanceAndDecrementServerGroupTask extends AbstractCloudProvider List remainingInstances = instanceSupport.remainingInstances(stage) + def serverGroupName = stage.context.serverGroupName ?: stage.context.asgName + trafficGuard.verifyInstanceTermination( + serverGroupName, [stage.context.instance] as List, account, Location.region(stage.context.region as String), diff --git a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/instance/TerminateInstancesTask.groovy b/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/instance/TerminateInstancesTask.groovy index 5409371ad8..8e60a970eb 100644 --- a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/instance/TerminateInstancesTask.groovy +++ b/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 remainingInstances = instanceSupport.remainingInstances(stage) + def serverGroupName = stage.context.serverGroupName ?: stage.context.asgName + trafficGuard.verifyInstanceTermination( + serverGroupName, stage.context.instanceIds as List, account, Location.region(stage.context.region as String), diff --git a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/utils/TrafficGuard.java b/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/utils/TrafficGuard.java index f4d7f8397a..c617887038 100644 --- a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/utils/TrafficGuard.java +++ b/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/utils/TrafficGuard.java @@ -46,13 +46,24 @@ public TrafficGuard(OortHelper oortHelper, Optional front50Servi this.front50Service = front50Service.orElse(null); } - public void verifyInstanceTermination(List instanceIds, String account, Location location, String cloudProvider, String operationDescriptor) { + public void verifyInstanceTermination(String serverGroupNameFromStage, + List instanceIds, + String account, + Location location, + String cloudProvider, + String operationDescriptor) { Map> instancesPerServerGroup = new HashMap<>(); - instanceIds.forEach(instanceId -> { + for (String instanceId : instanceIds) { + String serverGroupName = serverGroupNameFromStage; + if (serverGroupName == null) { + Optional resolvedServerGroupName = resolveServerGroupNameForInstance(instanceId, account, location.getValue(), cloudProvider); + serverGroupName = resolvedServerGroupName.orElse(null); + } - Optional 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(); diff --git a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/kato/tasks/DisableInstancesTask.groovy b/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/kato/tasks/DisableInstancesTask.groovy index a1803fccf5..cff3aafdd7 100644 --- a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/kato/tasks/DisableInstancesTask.groovy +++ b/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, account, Location.region(stage.context.region as String), diff --git a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/kato/tasks/rollingpush/WaitForNewUpInstancesLaunchTask.groovy b/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/kato/tasks/rollingpush/WaitForNewUpInstancesLaunchTask.groovy index ff6a186fc0..835c2de7e7 100644 --- a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/kato/tasks/rollingpush/WaitForNewUpInstancesLaunchTask.groovy +++ b/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) diff --git a/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/utils/TrafficGuardSpec.groovy b/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/utils/TrafficGuardSpec.groovy index 94f50459c8..7485a9fa97 100644 --- a/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/utils/TrafficGuardSpec.groovy +++ b/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 diff --git a/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/kato/tasks/rollingpush/WaitForNewUpInstancesLaunchTaskSpec.groovy b/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/kato/tasks/rollingpush/WaitForNewUpInstancesLaunchTaskSpec.groovy index cec6990a4b..116eb0decf 100644 --- a/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/kato/tasks/rollingpush/WaitForNewUpInstancesLaunchTaskSpec.groovy +++ b/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