From 640f5f5b74f11f12094d25cc7b07f0947ca72c39 Mon Sep 17 00:00:00 2001 From: John Gardiner Myers Date: Tue, 7 Jan 2020 23:13:25 -0800 Subject: [PATCH] Terminate AWS instances through EC2 instead of Autoscaling --- cloudmock/aws/mockautoscaling/BUILD.bazel | 3 ++ cloudmock/aws/mockautoscaling/ec2shim.go | 52 ++++++++++++++++++++ pkg/instancegroups/BUILD.bazel | 3 +- pkg/instancegroups/rollingupdate_test.go | 59 +++++++++++++---------- upup/pkg/fi/cloudup/awsup/aws_cloud.go | 7 ++- 5 files changed, 94 insertions(+), 30 deletions(-) create mode 100644 cloudmock/aws/mockautoscaling/ec2shim.go diff --git a/cloudmock/aws/mockautoscaling/BUILD.bazel b/cloudmock/aws/mockautoscaling/BUILD.bazel index 8d04c0dbf5..4623cd9d2d 100644 --- a/cloudmock/aws/mockautoscaling/BUILD.bazel +++ b/cloudmock/aws/mockautoscaling/BUILD.bazel @@ -5,6 +5,7 @@ go_library( srcs = [ "api.go", "attach.go", + "ec2shim.go", "group.go", "launchconfigurations.go", "tags.go", @@ -16,6 +17,8 @@ go_library( "//vendor/github.com/aws/aws-sdk-go/aws/request:go_default_library", "//vendor/github.com/aws/aws-sdk-go/service/autoscaling:go_default_library", "//vendor/github.com/aws/aws-sdk-go/service/autoscaling/autoscalingiface:go_default_library", + "//vendor/github.com/aws/aws-sdk-go/service/ec2:go_default_library", + "//vendor/github.com/aws/aws-sdk-go/service/ec2/ec2iface:go_default_library", "//vendor/k8s.io/klog:go_default_library", ], ) diff --git a/cloudmock/aws/mockautoscaling/ec2shim.go b/cloudmock/aws/mockautoscaling/ec2shim.go new file mode 100644 index 0000000000..97bdd09e95 --- /dev/null +++ b/cloudmock/aws/mockautoscaling/ec2shim.go @@ -0,0 +1,52 @@ +/* +Copyright 2020 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package mockautoscaling + +import ( + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/autoscaling" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/aws/aws-sdk-go/service/ec2/ec2iface" +) + +type ec2Shim struct { + ec2iface.EC2API + mockAutoscaling *MockAutoscaling +} + +func (m *MockAutoscaling) GetEC2Shim(e ec2iface.EC2API) ec2iface.EC2API { + return &ec2Shim{ + EC2API: e, + mockAutoscaling: m, + } +} + +func (e *ec2Shim) TerminateInstances(input *ec2.TerminateInstancesInput) (*ec2.TerminateInstancesOutput, error) { + if input.DryRun != nil && *input.DryRun { + return &ec2.TerminateInstancesOutput{}, nil + } + for _, id := range input.InstanceIds { + request := &autoscaling.TerminateInstanceInAutoScalingGroupInput{ + InstanceId: id, + ShouldDecrementDesiredCapacity: aws.Bool(false), + } + if _, err := e.mockAutoscaling.TerminateInstanceInAutoScalingGroup(request); err != nil { + return nil, err + } + } + return &ec2.TerminateInstancesOutput{}, nil +} diff --git a/pkg/instancegroups/BUILD.bazel b/pkg/instancegroups/BUILD.bazel index 520c15586c..51d2bd6009 100644 --- a/pkg/instancegroups/BUILD.bazel +++ b/pkg/instancegroups/BUILD.bazel @@ -44,7 +44,8 @@ go_test( "//upup/pkg/fi/cloudup/awsup:go_default_library", "//vendor/github.com/aws/aws-sdk-go/aws:go_default_library", "//vendor/github.com/aws/aws-sdk-go/service/autoscaling:go_default_library", - "//vendor/github.com/aws/aws-sdk-go/service/autoscaling/autoscalingiface:go_default_library", + "//vendor/github.com/aws/aws-sdk-go/service/ec2:go_default_library", + "//vendor/github.com/aws/aws-sdk-go/service/ec2/ec2iface:go_default_library", "//vendor/github.com/stretchr/testify/assert:go_default_library", "//vendor/k8s.io/api/core/v1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", diff --git a/pkg/instancegroups/rollingupdate_test.go b/pkg/instancegroups/rollingupdate_test.go index 6863b6b14d..14c51cb6ee 100644 --- a/pkg/instancegroups/rollingupdate_test.go +++ b/pkg/instancegroups/rollingupdate_test.go @@ -25,7 +25,8 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/autoscaling" - "github.com/aws/aws-sdk-go/service/autoscaling/autoscalingiface" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/aws/aws-sdk-go/service/ec2/ec2iface" "github.com/stretchr/testify/assert" v1 "k8s.io/api/core/v1" v1meta "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -49,7 +50,9 @@ func getTestSetup() (*RollingUpdateCluster, *awsup.MockAWSCloud, *kopsapi.Cluste k8sClient := fake.NewSimpleClientset() mockcloud := awsup.BuildMockAWSCloud("us-east-1", "abc") - mockcloud.MockAutoscaling = &mockautoscaling.MockAutoscaling{} + mockAutoscaling := &mockautoscaling.MockAutoscaling{} + mockcloud.MockAutoscaling = mockAutoscaling + mockcloud.MockEC2 = mockAutoscaling.GetEC2Shim(mockcloud.MockEC2) cluster := &kopsapi.Cluster{} cluster.Name = "test.k8s.local" @@ -672,7 +675,7 @@ func TestRollingUpdateDisabledCloudonly(t *testing.T) { // <-- validated type concurrentTest struct { - autoscalingiface.AutoScalingAPI + ec2iface.EC2API t *testing.T mutex sync.Mutex terminationRequestsLeft int @@ -727,29 +730,35 @@ func (c *concurrentTest) Validate() (*validation.ValidationCluster, error) { return &validation.ValidationCluster{}, nil } -func (c *concurrentTest) TerminateInstanceInAutoScalingGroup(input *autoscaling.TerminateInstanceInAutoScalingGroupInput) (*autoscaling.TerminateInstanceInAutoScalingGroupOutput, error) { +func (c *concurrentTest) TerminateInstances(input *ec2.TerminateInstancesInput) (*ec2.TerminateInstancesOutput, error) { + if input.DryRun != nil && *input.DryRun { + return &ec2.TerminateInstancesOutput{}, nil + } + c.mutex.Lock() defer c.mutex.Unlock() - terminationRequestsLeft := c.terminationRequestsLeft - c.terminationRequestsLeft-- - switch terminationRequestsLeft { - case 7, 2, 1: - assert.Equal(c.t, terminationRequestsLeft, c.previousValidation, "previous validation") - case 6, 4: - assert.Equal(c.t, terminationRequestsLeft, c.previousValidation, "previous validation") - c.mutex.Unlock() - select { - case <-c.terminationChan: - case <-time.After(1 * time.Second): - c.t.Error("timed out reading from terminationChan") + for range input.InstanceIds { + terminationRequestsLeft := c.terminationRequestsLeft + c.terminationRequestsLeft-- + switch terminationRequestsLeft { + case 7, 2, 1: + assert.Equal(c.t, terminationRequestsLeft, c.previousValidation, "previous validation") + case 6, 4: + assert.Equal(c.t, terminationRequestsLeft, c.previousValidation, "previous validation") + c.mutex.Unlock() + select { + case <-c.terminationChan: + case <-time.After(1 * time.Second): + c.t.Error("timed out reading from terminationChan") + } + c.mutex.Lock() + go c.delayThenWakeValidation() + case 5, 3: + assert.Equal(c.t, terminationRequestsLeft+1, c.previousValidation, "previous validation") } - c.mutex.Lock() - go c.delayThenWakeValidation() - case 5, 3: - assert.Equal(c.t, terminationRequestsLeft+1, c.previousValidation, "previous validation") } - return c.AutoScalingAPI.TerminateInstanceInAutoScalingGroup(input) + return c.EC2API.TerminateInstances(input) } func (c *concurrentTest) delayThenWakeValidation() { @@ -769,7 +778,7 @@ func (c *concurrentTest) AssertComplete() { func newConcurrentTest(t *testing.T, cloud *awsup.MockAWSCloud, allNeedUpdate bool) *concurrentTest { test := concurrentTest{ - AutoScalingAPI: cloud.MockAutoscaling, + EC2API: cloud.MockEC2, t: t, terminationRequestsLeft: 6, validationChan: make(chan bool), @@ -788,7 +797,7 @@ func TestRollingUpdateMaxUnavailableAllNeedUpdate(t *testing.T) { concurrentTest := newConcurrentTest(t, cloud, true) c.ValidateSuccessDuration = 0 c.ClusterValidator = concurrentTest - cloud.MockAutoscaling = concurrentTest + cloud.MockEC2 = concurrentTest two := intstr.FromInt(2) cluster.Spec.RollingUpdate = &kopsapi.RollingUpdate{ @@ -811,7 +820,7 @@ func TestRollingUpdateMaxUnavailableAllButOneNeedUpdate(t *testing.T) { concurrentTest := newConcurrentTest(t, cloud, false) c.ValidateSuccessDuration = 0 c.ClusterValidator = concurrentTest - cloud.MockAutoscaling = concurrentTest + cloud.MockEC2 = concurrentTest two := intstr.FromInt(2) cluster.Spec.RollingUpdate = &kopsapi.RollingUpdate{ @@ -833,7 +842,7 @@ func TestRollingUpdateMaxUnavailableAllNeedUpdateMaster(t *testing.T) { concurrentTest := newConcurrentTest(t, cloud, true) c.ValidateSuccessDuration = 0 c.ClusterValidator = concurrentTest - cloud.MockAutoscaling = concurrentTest + cloud.MockEC2 = concurrentTest two := intstr.FromInt(2) cluster.Spec.RollingUpdate = &kopsapi.RollingUpdate{ diff --git a/upup/pkg/fi/cloudup/awsup/aws_cloud.go b/upup/pkg/fi/cloudup/awsup/aws_cloud.go index 7aa22516e2..59b14458b4 100644 --- a/upup/pkg/fi/cloudup/awsup/aws_cloud.go +++ b/upup/pkg/fi/cloudup/awsup/aws_cloud.go @@ -414,12 +414,11 @@ func deleteInstance(c AWSCloud, i *cloudinstances.CloudInstanceGroupMember) erro return fmt.Errorf("id was not set on CloudInstanceGroupMember: %v", i) } - request := &autoscaling.TerminateInstanceInAutoScalingGroupInput{ - InstanceId: aws.String(id), - ShouldDecrementDesiredCapacity: aws.Bool(false), + request := &ec2.TerminateInstancesInput{ + InstanceIds: []*string{aws.String(id)}, } - if _, err := c.Autoscaling().TerminateInstanceInAutoScalingGroup(request); err != nil { + if _, err := c.EC2().TerminateInstances(request); err != nil { return fmt.Errorf("error deleting instance %q: %v", id, err) }