Default instance type to m5.large

heptio / aws-quickstart

AWS Kubernetes cluster via CloudFormation and kubeadm

Apache License 2.0

223 stars 134 forks source link

Default instance type to m5.large #248

Closed matthewfischer closed 4 years ago

matthewfischer commented 5 years ago

m5 should be cheaper in all regions and offers better performance as well.

heptibot commented 5 years ago

Can one of the admins verify this patch?

matthewfischer commented 5 years ago

Unsure why but the master fails to create using m5.large - so for now, bailing on this

detiber commented 5 years ago

Odd, I had tested with m5 instance types roughly 1 year ago or so, but ended up deferring then because they were not available in many regions at that point. @johnSchnake any ideas on why m5 instances wouldn't work with the current state?

matthewfischer commented 5 years ago

The stack failed to create (master node failed to create) and as my goal was using it, I didn't debug. I tried several times. I don't know why it shouldn't work.

On Wed, Feb 20, 2019 at 8:46 AM Jason DeTiberus notifications@github.com wrote:

Odd, I had tested with m5 instance types roughly 1 year ago or so, but ended up deferring then because they were not available in many regions at that point. @johnSchnake https://github.com/johnSchnake any ideas on why m5 instances wouldn't work with the current state?

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/heptio/aws-quickstart/pull/248#issuecomment-465632935, or mute the thread https://github.com/notifications/unsubscribe-auth/ABm7FW50uqECPvWiHah4OLuY72gULHEdks5vPW3ngaJpZM4bD_7H .

johnSchnake commented 5 years ago

No idea why something like that would fail; I'll try it out and see what the errors indicate. AWS did push/publicize that m5 was just better than m4 and even just reading the base stats/price it does seem to be a better choice. https://aws.amazon.com/blogs/aws/m5-the-next-generation-of-general-purpose-ec2-instances/

matthewfischer commented 5 years ago

I'll try it again right now and see if I can get a more useful error from cfn.

johnSchnake commented 5 years ago

I made these changes locally and did the boot-from-local script and everything went fine.

@matthewfischer let me know what the error is you get; otherwise I say we can reopen this and move it forward.

I did also (locally) change the other template though to use m5.large as well; the other ...with_new_vpc.template template passes its instanceType parameter to the cluster template. So I think you'd still end up with m4 if only this one line was changed.

matthewfischer commented 5 years ago

It also worked for me just now. I'm willing to chalk it up to me making another mistake. But I have an m5 based cluster up and running.

johnSchnake commented 5 years ago

@matthewfischer glad that it is working for both of us now.

I do think you needed to adjust both templates though. Once that is done and you sign your commit then I can merge.

Thanks!

johnSchnake commented 4 years ago

Closing based on #261 which marks this repo as deprecated and no longer under development.