Implement event-based instance replacement

[cristim]

Context

This change attempts to replace on-demand instances with spot as soon as they
are launched (triggered by the on-demand instance entering the running state),
in order to avoid running expensive on-demand capacity in parallel with the
newly launched spot instances. This should cover new groups out of the box, as
well as scaling out or replacement of terminated spot instances.

It has a few nice side effects concerning cost, since on-demand instances are
likely to run for just a few seconds, as well as potentially tackling some
known issues with ElasticBeanstalk(#343, #344) and deletion of CloudFormation
stacks(#332).

In addition, it implements support for handling the start lifecycle hooks(#296) by
similarly intercepting the lifecycle hook completion API failure event received
from Cloudtrail, and will immediately replace any scaled out instances, partially
addressing #284.

The current/legacy replacement logic will still kick in when enabling
AutoSpotting for the first time on an existing group, as well as when spot
instances fail to be launched so the group must failover to on-demand capacity
in order to avoid downtime.

Once the legacy replacement logic is done on the current group, the group is
tagegd with a configuration tag that enables the new event-based logic. This tag
is also set for all groups newer than an hour so their instances are automatically
replaced by the event-based replacement logic, and can be used to manually revert
a group back to the legacy logic in case of problems with the event-based implementation.

Current status

• changed regional CloudFormation stack to intercept EC2 instances
  as soon as entering the 'pending' and 'running' state'
• implement handling code for these events, that launches a replacement spot
  instance for each on-demand instance currently in pending state and performs
  the usual instance swapping logic when a spot instance enters the running
  state, without waiting for it to exit its grace period.
• largely kept the initial instance replacement logic as it is (waiting for
  the grace period) to avoid introducing regressions, and also in case the
  events are not properly propagated like in case of misconfigurations.
• implemented a configuration override that allows users to disable the event-based logic on a per-group level by tagging the group.
• this code was executed against against real infrastructure and is
  working as expected for a couple of groups I tested it against.
• still missing test coverage for the new code and some existing tests were
  crashing and had to be disabled for now
• implemented support for handling the AutoScaling Lifecycle hooks when
  replacing new on-demand instances in event-based execution mode based on
  CloudTrail events. As a caveat the lifecycle hook handling will be simulated with
  a delay of up to 15 minutes, because of the CloudTrail event propagation latency.
• added support for running it locally against custom JSON event data in
  CloudFront Event format: https://docs.aws.amazon.com/AmazonCloudWatch/latest/events/EventTypes.html#ec2_event_type

ToDo

• test against more infrastructure
• iron out any potential crashes
• write automated tests for the new code and re-enable older test cases so
  that coverage doesn't drop
• port regional CloudFormation stack changes to the main template so
  it can also run from a StackSet
• update the documentation to mention this new behavior

Issue Type

• Feature Pull Request
• Bugfix Pull Request
• Documentation Pull Request

Summary

Code contribution checklist

1. The contribution fixes a single existing github issue, and it is linked
   to it.
2. The code is as simple as possible, readable and follows the idiomatic Go
   guidelines.
3. All new functionality is covered by automated test cases so the overall
   test coverage doesn't decrease.
4. No issues are reported when running make full-test.
5. Functionality not applicable to all users should be configurable.
6. Configurations should be exposed through Lambda function environment
   variables which are also passed as parameters to the
   CloudFormation
   and
   Terraform
   stacks defined as infrastructure code.
7. Global configurations set from the infrastructure stack level should also
   support per-group overrides using tags.
8. Tags names and expected values should be similar to the other existing
   configurations.
9. Both global and tag-based configuration mechanisms should be tested and
   proven to work using log output from various test runs.
10. The logs should be kept as clean as possible (use log levels as
    appropriate) and formatted consistently to the existing log output.
11. The documentation is updated to cover the new behavior, as well as the
    new configuration options for both stack parameters and tag overrides.
12. A code reviewer reproduced the problem and can confirm the code
    contribution actually resolves it.

[codeclimate]

Function GetPendingReplaceableOnDemandInstance has 5 return statements (exceeds 4 allowed).

[codeclimate]

Instance has 27 methods (exceeds 20 allowed). Consider refactoring.

[codeclimate]

Method Instance.createRunInstancesInput has 69 lines of code (exceeds 50 allowed). Consider refactoring.

[golangcibot]

Error return value of a.terminateInstanceInAutoScalingGroup is not checked (from errcheck)

[golangcibot]

Error return value of a.replaceOnDemandInstanceWithSpot is not checked (from errcheck)

[golangcibot]

Error return value of a.region.scanInstance is not checked (from errcheck)

[golangcibot]

Error return value of a.attachSpotInstance is not checked (from errcheck)

[golangcibot]

Error return value of a.setAutoScalingMaxSize is not checked (from errcheck)

[golangcibot]

S1012: should use time.Since instead of time.Now().Sub (from gosimple)

[golangcibot]

U1000: func (*autoScalingGroup).isEnabled is unused (from unused)

[golangcibot]

U1000: field wantASG is unused (from unused)

[golangcibot]

U1000: func (*Config).disableLogging is unused (from unused)

[cristim]

For anyone brave enough to be a guinea pig, to install the latest build from this branch just click on this URL:
https://console.aws.amazon.com/cloudformation/home?region=us-east-1#/stacks/create/template?stackName=AutoSpotting&templateURL=https://s3.amazonaws.com/cloudprowess/custom/template_build_1439509.yaml

[codeclimate]

Method autoScalingGroup.processCronEvent has 8 return statements (exceeds 4 allowed).

[codeclimate]

Method instance.swapWithGroupMember has 9 return statements (exceeds 4 allowed).

[codeclimate]

Method AutoSpotting.EventHandler has 5 return statements (exceeds 4 allowed).

[codeclimate]

Method AutoSpotting.handleNewInstanceLaunch has 8 return statements (exceeds 4 allowed).

[codeclimate]

Method instance.swapWithGroupMember has 56 lines of code (exceeds 50 allowed). Consider refactoring.

[golangcibot]

Error return value of i.terminate is not checked (from errcheck)

[codeclimate]

Method AutoSpotting.handleLifecycleHookEvent has 11 return statements (exceeds 4 allowed).

[codeclimate]

Method instance.swapWithGroupMember has 8 return statements (exceeds 4 allowed).

[codeclimate]

Method AutoSpotting.handleNewInstanceLaunch has 77 lines of code (exceeds 50 allowed). Consider refactoring.

[codeclimate]

Method AutoSpotting.handleNewInstanceLaunch has 11 return statements (exceeds 4 allowed).

[codeclimate]

Function parseCommandLineFlags has 67 lines of code (exceeds 50 allowed). Consider refactoring.

[golangcibot]

Error return value of a.handleLifecycleHookEvent is not checked (from errcheck)

[golangcibot]

Error return value of asg.terminateRandomSpotInstanceIfHavingEnough is not checked (from errcheck)

[golangcibot]

Error return value of asg.terminateRandomSpotInstanceIfHavingEnough is not checked (from errcheck)

[golangcibot]

Error return value of spotInstance.terminate is not checked (from errcheck)

[golangcibot]

Error return value of asg.replaceOnDemandInstanceWithSpot is not checked (from errcheck)

[golangcibot]

S1023: redundant return statement (from gosimple)

[golangcibot]

U1000: type fields is unused (from unused)

[mello7tre]

Hi, i begin porting regional CloudFormation stack changes to the main template so new event-based replacement can be deployed using StackSet too.

As soon i found some time to test it i will notify you.
(I will try to test not only stackset creation but the whole new event-based replacement.)

[golangcibot]

Error return value of a.suspendResumeProcess is not checked (from errcheck)

[golangcibot]

Error return value of a.suspendResumeProcess is not checked (from errcheck)

[golangcibot]

Error return value of a.changeAutoScalingMaxSize is not checked (from errcheck)

[golangcibot]

Error return value of asg.suspendResumeProcess is not checked (from errcheck)

[golangcibot]

Error return value of asg.suspendResumeProcess is not checked (from errcheck)

[golangcibot]

copylocks: handleNewSpotInstanceLaunch passes lock by value: github.com/AutoSpotting/AutoSpotting/core.region contains sync.WaitGroup contains sync.noCopy (from govet)

[golangcibot]

func (*autoScalingGroup).temporarilySuspendTerminations is unused (from unused)

[golangcibot]

func (*autoScalingGroup).isTerminationSuspended is unused (from unused)

[golangcibot]

func (*autoScalingGroup).resumeTerminationProcess is unused (from unused)

[golangcibot]

func (*autoScalingGroup).suspendTerminationProcess is unused (from unused)

[codeclimate]

The PR diff size of 7401 lines exceeds the maximum allowed for the inline comments feature.

[mello7tre]

i know that this PR and the comment related to it are already a lot, but i found a bug and didn't want to open an issue as this is not have been yet merged in the master branch.
So i will explain it here, and then when i will be sure that the solution that i implemented is fine i will wil open a specific PR.

The problem is difficult to explain, but is revelant so i will try:

Setup

Consider an ASG with a Desired = 10 and a AS minOnDemand = 1
Over time we will arrive at the following state:
9 Spot and 1 OnDemand
Now if the ASG can scale based on load Autoscaling can terminate an instance and reduce the Desired to 9.
Let's take the case where the terminated one is the only one OnDemand.
We will have 9 Spot and 0 OnDemand.
On the next run AS (AutoSpotting)will terminate a spot, so that a new onDemand is launched, to obey the condition of minOnDemand = 1.
Buy if the ASG have a lifecyclehook triggered on instance terminating and that lifecyclehook take more time than AS scheduled frequency, a big issue can arise.

Issue

AS terminate spot instance(minOnDemand), instance is not terminated (LifeCycleHook) immediately, but Autoscaling Launch a new instance (onDemand).
On the next scheduled run, AS (cronEventAction):

                if need, total := a.needReplaceOnDemandInstances(); !need {
                        logger.Printf("Not allowed to replace any more of the running OD instances in %s", a.name)
                        return terminateSpotInstance{target{asg: a, totalInstances: total}}
                }

look if new onDemand must be replaced, but as onDemandRunning == a.minOnDemand need = false and he execute:
terminateSpotInstance that finally execute terminateRandomSpotInstanceIfHavingEnough

func (a *autoScalingGroup) terminateRandomSpotInstanceIfHavingEnough(totalRunning int64, wait bool) error {

        if totalRunning == 1 {
                logger.Println("Warning: blocking replacement of very last instance - consider raising ASG to >= 2")
                return nil
        }

        if allInstancesAreRunning, onDemandRunning := a.allInstancesRunning(); allInstancesAreRunning {
                if a.instances.count64() == *a.DesiredCapacity && onDemandRunning == a.minOnDemand {
                        logger.Println("Currently Spot running equals to the required number, skipping termination")
                        return nil
                }

                if a.instances.count64() < *a.DesiredCapacity {
                        logger.Println("Not enough capacity in the group")
                        return nil
                }
        }

        randomSpot := a.getAnySpotInstance()
        if randomSpot == nil {
                logger.Println("Couldn't pick a random spot instance")
                return nil
        }

        logger.Println("Terminating randomly-selected spot instance",
                *randomSpot.Instance.InstanceId)

        switch a.config.TerminationMethod {
        case DetachTerminationMethod:
                return randomSpot.terminate()
        default:
                return a.terminateInstanceInAutoScalingGroup(randomSpot.Instance.InstanceId, wait, false)
        }
}

the root problem is that the terminating instance is still in state running!
So if a.instances.count64() == *a.DesiredCapacity && onDemandRunning == a.minOnDemand { is not true and AS terminate another spot instance (can even happen that try to terminate the same already terminating!)
Instance begin terminating (LifeCycleHook) and Autoscaling launch a new instance, but on the next run the same problem arise, so we can end in a loop of launching and then terminating instances.
(i have observed all this in realtime)

Solution/Fix

When executing terminateRandomSpotInstanceIfHavingEnough we need to consider not the instance State but the Instance Autoscaling LifeCycle state.
I have added to autoScalingGroup struct two new properties:

        instancesOnDemandInService map[string]*instance
        instancesSpotInService     map[string]*instance

then created a func populateASGInstancesInService that create the previous map with the id of the instance inService as index and the instance herself as value.
I have then changed terminateRandomSpotInstanceIfHavingEnough:

func (a *autoScalingGroup) terminateRandomSpotInstanceIfHavingEnough(wait bool) error {
        onDemandRunning := int64(len(a.instancesOnDemandInService))
        totalRunning := int64(len(a.instancesOnDemandInService) + len(a.instancesSpotInService))

        if totalRunning == 1 {
                logger.Println("Warning: blocking replacement of very last instance - consider raising ASG to >= 2")
                return nil
        }

        if totalRunning < *a.DesiredCapacity {
                logger.Println("Not enough capacity in the group, skipping termination")
                return nil
        }

        if totalRunning == *a.DesiredCapacity && onDemandRunning == a.minOnDemand {
                logger.Println("Currently Spot running equals to the required number, skipping termination")
                return nil
        }
.....
......
......

And finally to avoid terminating a terminating in progress instance i changed getAnySptInstance:

func (a *autoScalingGroup) getAnySpotInstance() *instance {
        for _, i := range a.instancesSpotInService {
                return i
        }
        return nil
}

I have an AS version builded with this code running, and so far found no problem.
I will notify you if i found any.
If all goes well, if you like, i can open a PR with the changes/fixes.

Regards, Alberto.

[mello7tre]

Thinking again on this:
as the problem can arise even when terminating onDemand instance, maybe a better solution can be to exclude instances if their ASG lifecycle state begin with Terminating directly in scanInstances func ....

[mello7tre]

made some test, the right solution is to exclude instances when their ASG lifecycle state begin with Terminating.
I will open a PR

[codeclimate]

The PR diff size of 7405 lines exceeds the maximum allowed for the inline comments feature.

[codeclimate]

The PR diff size of 7404 lines exceeds the maximum allowed for the inline comments feature.

[codeclimate]

The PR diff size of 5305 lines exceeds the maximum allowed for the inline comments feature.

[codeclimate]

The PR diff size of 5306 lines exceeds the maximum allowed for the inline comments feature.

[codeclimate]

The PR diff size of 5394 lines exceeds the maximum allowed for the inline comments feature.

[codeclimate]

The PR diff size of 5391 lines exceeds the maximum allowed for the inline comments feature.

[codeclimate]

The PR diff size of 5413 lines exceeds the maximum allowed for the inline comments feature.

[codeclimate]

Code Climate has analyzed commit 7c92d9d and detected 12 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	11
Bug Risk	1

View more on Code Climate.