Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement support for handling instance termination protection #275

Merged
merged 5 commits into from
Sep 7, 2018

Conversation

cristim
Copy link
Member

@cristim cristim commented Jun 29, 2018

Issue Type

  • Feature Pull Request

Summary

This implements handling of the instance termination protection flag, requested as part of #267.

Current status:

  • it compiles :-)
  • hasn't been tested at all yet, didn't even try to run it against any infrastructure.
  • tests had to be created for the entire region.scanInstances() function, which wasn't tested before, and are currently failing. I could use some help on getting them pass, since this is a bit harder to mock because of the pagination.
  • It introduces lots of additional ec2.DescribeInstanceAttribute API calls, one per each instance, so it may need to be configurable for people who don't want to do all these calls. I think we should also do these calls only for on-demand instances running in ASGs, but this scope filtering is not implemented yet.

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.

@cristim
Copy link
Member Author

cristim commented Aug 17, 2018

I managed to write some tests and refactor the code a bit in the process, still needs to be tested against real AWS infrastructure.

@cristim cristim changed the title DRAFT: Implement support for handling instance termination protection Implement support for handling instance termination protection Aug 17, 2018
@coveralls
Copy link

coveralls commented Aug 17, 2018

Coverage Status

Coverage increased (+2.9%) to 83.774% when pulling 2866499 on feat/support-instance-termination-protection into fff582b on master.

- determine instance termination protection only when we are about to
  replace the on-demand instances, to avoid unnecessary API calls when
  scanning instances on all Lambda function runs
- write tests for instance termination protection and scale-in
  protection
- define instanceMap type as map[string]*instance
- fix tests after refactoring the instance scanning logic in a previous
  commit
@cristim cristim force-pushed the feat/support-instance-termination-protection branch from 6ea8662 to b29813a Compare August 31, 2018 20:48
@cristim
Copy link
Member Author

cristim commented Aug 31, 2018

I tested it against real infrastructure and it works pretty well.

@cristim
Copy link
Member Author

cristim commented Sep 5, 2018

Last chance to review this, I will merge it as it is on Friday unless I get any objections.

Copy link
Contributor

@lenucksi lenucksi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most minor nits, the rest looks good to me.
Refactorings look nice, instanceMap makes things more readable.

Also, gofmt had minor complaints, that you might want to address.

Edit:

  • thanks for review!
  • I couldn't reproduce the gofmt issues
$ make fmt-check
ok      all files passed go fmt

@@ -335,11 +335,11 @@ func (a *autoScalingGroup) process() {
logger.Println("No spot instances were found for ", a.name)

// find any given on-demand instance and try to replace it with a spot one
Copy link
Contributor

@lenucksi lenucksi Sep 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could consider adding 'unprotected' before 'on-demand'. The function below has a nice explaining name, so that could be enough to not need the more specific comment.
There is one more case of this throughout the PR.

Copy link
Member Author

@cristim cristim Sep 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!
I deleted the comments, they should only explain why are we doing some harder to grasp things, not mention once again what are we're doing, which should be seen clearly in the code itself.

@cristim
Copy link
Member Author

cristim commented Sep 6, 2018

@lenucksi thanks for the review, I'll try to address these issues and I hope I can finally merge this tomorrow.

@cristim cristim merged commit 5afd4c9 into master Sep 7, 2018
@cristim cristim deleted the feat/support-instance-termination-protection branch September 7, 2018 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants