Support launching spot nodes of recent instance types

[cristim]

Issue Type

• Bugfix Pull Request

Summary

This partially fixes #294, allowing to launch spot nodes of instance types that lack on-demand pricing data.

Previously we would skip adding the type info when missing the on-demand pricing data.

For the use case of launching spot instances we can just use the spot market prices which we already have and use the rest of the instance type information (mainly hardware specs) which we ignored so far.

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.

[coveralls]

Coverage decreased (-0.08%) to 83.698% when pulling 6df46ed on feat/support-recent-instance-types into 5afd4c9 on master.

[xlr-8]

Why switching this one to Printf but leaving the other as Println ?

[cristim]

I had a larger local patch in which I wanted to print on the same line the reason why the comparison failed: CPU, memory, storage, etc.

I failed to do that cleanly for now, so I reverted most of that but this hunk was missed.

[xlr-8]

I'm aware it's unlikely to happen, but maybe switching to <= 0 would be safer as even if the value is negative the code would warn us.

Also according to the previous method's behaviour, shouldn't this be followed by an else afterwards? Or a return on the if itself if no else is desired?

[cristim]

This should never be negative, the check is for the zero value in case we're missing the data.

I'll revisit the if logic on Friday.

[lenucksi]

I agree with @xlr-8's question and suggestion.

[lenucksi]

Very minor comments. Fix them and I'll be happy to give thumbs up.

[lenucksi]

I agree with @xlr-8's question and suggestion.

[gabegorelick]

Does this fix #376 (AutoSpotting ignoring spot instances that it incorrectly thinks are $0)?

[cristim]

Not sure if this would help you but I can explain what's going on:

This covers a situation in which existing instance types are later added to new regions.

In this case we may have instance type information for some other regions but on demand price is zero for the current region because at the time we compiled the instance type information that particular instance type wasn't available there. If the price is zero we currently just skip the instance type in this region.

Later, once the instance becomes available we will get spot price information for it, which is all we need to launch the spot instances.

This change allows us to launch the spot instances if we have spot price information about them even if we're lacking the on demand price information

[codeclimate]

Code Climate has analyzed commit e4b072f and detected 0 issues on this pull request.

View more on Code Climate.