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

ec2_instance: Allow auto selection of ebs optimized #61222

Open
wants to merge 7 commits into
base: devel
from

Conversation

@Shaps
Copy link
Contributor

commented Aug 23, 2019

SUMMARY

Allow for ebs_optimized to be selected automatically

Fixes #55912

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

ec2_instance.py

ADDITIONAL INFORMATION

See #55912

Shaps added 2 commits Aug 23, 2019

@Shaps Shaps changed the title Ec2 instance auto ebs optimized ec2_instance: Allow auto selection of ebs optimized Aug 23, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/modules/cloud/amazon/ec2_instance.py:1087:61: bad-whitespace Exactly one space required after comma     ebs_optimized_types = ['a1', 'c4', 'c5', 'd2', 'f1', 'g3','h1', 'i3', 'm4', 'm5', 'p2', 'p3', 'r4', 'r5', 't3', 'u-', 'x1','z1']                                                              ^
lib/ansible/modules/cloud/amazon/ec2_instance.py:1087:126: bad-whitespace Exactly one space required after comma     ebs_optimized_types = ['a1', 'c4', 'c5', 'd2', 'f1', 'g3','h1', 'i3', 'm4', 'm5', 'p2', 'p3', 'r4', 'r5', 't3', 'u-', 'x1','z1']                                                                                                                               ^

The test ansible-test sanity --test pep8 [explain] failed with 4 errors:

lib/ansible/modules/cloud/amazon/ec2_instance.py:1086:1: E302 expected 2 blank lines, found 1
lib/ansible/modules/cloud/amazon/ec2_instance.py:1087:62: E231 missing whitespace after ','
lib/ansible/modules/cloud/amazon/ec2_instance.py:1087:127: E231 missing whitespace after ','
lib/ansible/modules/cloud/amazon/ec2_instance.py:1092:1: E302 expected 2 blank lines, found 1

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/ec2_instance.py:0:0: E326 Argument 'ebs_optimized' in argument_spec defines choices as (['true', 'false', 'auto', 'True', 'False']) but documentation defines choices as (['true', 'false', 'auto'])

click here for bot help

@@ -1084,6 +1086,13 @@ def discover_security_groups(group, groups, parent_vpc_id=None, subnet_id=None,
return list(dict((g['GroupId'], g) for g in found_groups).values())


def _supports_ebs_optimized(instance_type):
ebs_optimized_types = ['a1', 'c4', 'c5', 'd2', 'f1', 'g3', 'h1', 'i3', 'm4', 'm5', 'p2', 'p3', 'r4', 'r5', 't3', 'u-', 'x1', 'z1']

This comment has been minimized.

Copy link
@willthames

willthames Sep 2, 2019

Contributor

This doesn't seem hugely future proof, but the alternative might be a bit harder.

I think the pricing api might be able to help but with significantly more work - ec2instances.info seems to use the API to reasonable effect: https://github.com/powdahound/ec2instances.info/blob/master/ec2.py

Not a blocker though

This comment has been minimized.

Copy link
@willthames

willthames Sep 2, 2019

Contributor

Looking at this again, I can see no simple way of getting this info from the pricing info.

It seems to be: (set of instance types with dedicatedEbsThroughput set) - (set of instance types with ebsOptimized set) - {'r2','t3'}

aws pricing get-products --service-code=AmazonEC2 --region us-east-1  --filters \
Type=TERM_MATCH,Field=operatingSystem,Value=RHEL \
Type=TERM_MATCH,Field=capacitystatus,Value=AllocatedCapacityReservation \
Type=TERM_MATCH,Field=location,Value="US East (N. Virginia)" \
--query PriceList[]  --output text | sed 's/  /^M/g' | grep dedicatedEbsThroughput | \
while read line; do echo $line | jq -r .product.attributes.instanceType ; done | \
awk -F. '{print $1}' | sort -u
aws pricing get-products --service-code=AmazonEC2 --region us-east-1 --filters \
Type=TERM_MATCH,Field=ebsOptimized,Value=Yes \
--query PriceList[] --output text | sed 's/  /^M/g' | \
while read line; do echo $line | jq -r .product.attributes.instanceType ; done | \
awk -F. '{print $1}' | sort -u

This comment has been minimized.

Copy link
@jillr

jillr Sep 3, 2019

Contributor

I'd also prefer not to hardcode this list, but if there's no reasonable alternative I'd be ok with it.

This comment has been minimized.

Copy link
@Shaps

Shaps Sep 4, 2019

Author Contributor

One thing to consider with using the pricing API ( or any other API, really ), is that we'd be adding additional API calls to each instance, potentially impacting the performance of the module.
( Thanks @s-hertel for pointing it out )

@ansibot ansibot removed the needs_triage label Sep 2, 2019

@s-hertel s-hertel requested review from s-hertel and jillr Sep 3, 2019

lib/ansible/modules/cloud/amazon/ec2_instance.py Outdated Show resolved Hide resolved
@@ -1084,6 +1086,13 @@ def discover_security_groups(group, groups, parent_vpc_id=None, subnet_id=None,
return list(dict((g['GroupId'], g) for g in found_groups).values())


def _supports_ebs_optimized(instance_type):
ebs_optimized_types = ['a1', 'c4', 'c5', 'd2', 'f1', 'g3', 'h1', 'i3', 'm4', 'm5', 'p2', 'p3', 'r4', 'r5', 't3', 'u-', 'x1', 'z1']

This comment has been minimized.

Copy link
@jillr

jillr Sep 3, 2019

Contributor

I'd also prefer not to hardcode this list, but if there's no reasonable alternative I'd be ok with it.

@@ -1587,7 +1600,7 @@ def main():
instance_type=dict(default='t2.micro', type='str'),
user_data=dict(type='str'),
tower_callback=dict(type='dict'),
ebs_optimized=dict(type='bool'),
ebs_optimized=dict(type='raw'),

This comment has been minimized.

Copy link
@s-hertel

s-hertel Sep 9, 2019

Contributor

If someone passes something other than a boolean or 'auto', boolean() will cause a TypeError. Using choices with a list containing 'auto' and all the possible booleans for choices is one solution, or you could use https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/common/validation.py#L436 and if the value isn't 'auto' or a valid boolean fail at the beginning of the module for invalid type.

LGTM besides that. The hardcoding is unfortunate but seems like the best choice for now (once this module can spin up multiple instances at once we could reconsider the try/except approach).

@flowerysong

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

This change is pointless in its current form; the hardcoded list of instance types are the instance types for which EBS Optimization is enabled by default and cannot be disabled, not the older instance sizes where this flag actually has an effect (e.g. c1.xlarge; see the small "EBS Optimization Supported" table at https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/EBSOptimized.html)

@Shaps

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

@flowerysong Brilliant, thanks for the comment! I'll update the list ( and check ) to the list of instances in the table for the time being, will look into making the list dynamic at a later stage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.