Skip to content

Bump EC2 instance type (SC-924)#188

Merged
holmanb merged 1 commit into
mainfrom
ec2-ipv6
Apr 13, 2022
Merged

Bump EC2 instance type (SC-924)#188
holmanb merged 1 commit into
mainfrom
ec2-ipv6

Conversation

@TheRealFalcon
Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon commented Apr 11, 2022

Proposed commit message

Bump EC2 instance type

Updates the default ec2 instance type to t3.micro as it supports
IPv6 metadata while also being faster and cheaper than t2.

@TheRealFalcon TheRealFalcon changed the title Enable IPv6 metadata URL on ec2 Enable IPv6 metadata URL on ec2 (SC-924) Apr 11, 2022
Copy link
Copy Markdown
Collaborator

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

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

Thanks @TheRealFalcon. If you have a use-case for why we want to set this up after initial instance creation, then I think we can keep this method as defined. But, my initial thought was we'd really like to do this just as instance launches, which would either require passing kwargs into the create_instances call or even letting pycloudlib attempt to setup create_launch_templates for ipv4, dual-stack and ipv6-only metadata which we can use at create_instances call.

Comment thread pycloudlib/ec2/instance.py Outdated
if wait:
self.wait_for_delete()

def enable_ipv6_instance_metadata(self):
Copy link
Copy Markdown
Collaborator

@blackboxsw blackboxsw Apr 12, 2022

Choose a reason for hiding this comment

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

I don't think we want to typically run this after the instance launches because there could be a race with the instance coming up earlier than our IPv6 IMDS metadata makes it through AWS backplane, so cloud-init may have already passed the point where early IMDS detection passed on an initially IPv4-only IMDS.

I think we actually want this at launch which looks like we can provide via something like create_instances kwargs MetadataOptions={'HttpProtocolIpv6': 'enabled'}.
I'm testing this suggestion now. That said, we might want an easy way to provide a simple param during ec2 instance launch to specify IMDS 'ipv4'|'ipv6'|'dual-stack' at instance launch time so we don't have to digup and remember these options at every call-site.

@TheRealFalcon
Copy link
Copy Markdown
Contributor Author

If you have a use-case for why we want to set this up after initial instance creation, then I think we can keep this method as defined.

I don't. I just didn't see another way of way of doing it, but now I see it can also be passed to run_instances, so agreed, that would be preferred.

@blackboxsw
Copy link
Copy Markdown
Collaborator

If you have a use-case for why we want to set this up after initial instance creation, then I think we can keep this method as defined.

I don't. I just didn't see another way of way of doing it, but now I see it can also be passed to run_instances, so agreed, that would be preferred.

OK, let's adapt this PR then to minimally update default instance_type to t3.micro, add a comment about support of IPv6 IMDS (as it's also required for IPv6-only Nitro instance support too).

@TheRealFalcon
Copy link
Copy Markdown
Contributor Author

TheRealFalcon commented Apr 12, 2022

Sounds good. I'm going to leave this branch up as-is until we can have the cloud-init integration test work without it. Then I'll remove that method.

@TheRealFalcon TheRealFalcon changed the title Enable IPv6 metadata URL on ec2 (SC-924) Bump EC2 instance type (SC-924) Apr 13, 2022
Updates the default ec2 instance type to t3.micro as it supports
IPv6 metadata while also being faster and cheaper than t2.
Copy link
Copy Markdown
Member

@holmanb holmanb left a comment

Choose a reason for hiding this comment

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

LGTM

@holmanb holmanb merged commit a2fde24 into main Apr 13, 2022
@TheRealFalcon TheRealFalcon deleted the ec2-ipv6 branch April 13, 2022 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants