Skip to content
This repository has been archived by the owner on Jun 29, 2022. It is now read-only.

packet: Use c3.small.x86 by default #612

Merged
merged 2 commits into from
Jul 6, 2020

Conversation

johananl
Copy link
Member

@johananl johananl commented Jun 10, 2020

General

t1.small.x86 machines are becoming EOL and are no longer available in new Packet projects. While c3.small.x86 is significantly larger (and more expensive) than t1.small.x86, it is currently the smallest and cheapest Packet device type in general availability.

Release notes

This change modifies the default machine type from t1.small.x86 to c3.small.x86 on Packet clusters. If you haven't explicitly defined the controller_type and/or worker_pool.node_type configuration options, upgrading to this release will replace your controller and/or worker nodes with c3.small.x86 machines. To avoid this, set these configuration options to the desired values.

invidian
invidian previously approved these changes Jun 10, 2020
@johananl johananl force-pushed the johananl/packet-default-dev-type branch 2 times, most recently from a69c174 to 3f4599d Compare June 10, 2020 18:05
surajssd
surajssd previously approved these changes Jun 11, 2020
ipochi
ipochi previously approved these changes Jun 11, 2020
Copy link
Member

@ipochi ipochi left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @johananl

Should we provide a link to the Packet documentation (https://www.packet.com/cloud/servers/c3-small/) regarding the pricing so that the user is aware of the costs they may incur ? I understand the difference in pricing between t1.small.x86 and c3.small.x86 is not trivial.

@johananl johananl dismissed stale reviews from ipochi and surajssd via a18a0cd June 11, 2020 10:58
@johananl johananl force-pushed the johananl/packet-default-dev-type branch from a18a0cd to f0b8201 Compare June 11, 2020 10:59
@johananl
Copy link
Member Author

Thanks for the PR @johananl

Should we provide a link to the Packet documentation (https://www.packet.com/cloud/servers/c3-small/) regarding the pricing so that the user is aware of the costs they may incur ? I understand the difference in pricing between t1.small.x86 and c3.small.x86 is not trivial.

I've added a note with a link. Thanks.

ipochi
ipochi previously approved these changes Jun 11, 2020
Copy link
Member

@ipochi ipochi left a comment

Choose a reason for hiding this comment

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

lgtm 🎉

invidian
invidian previously approved these changes Jun 11, 2020
Copy link
Contributor

@iaguis iaguis left a comment

Choose a reason for hiding this comment

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

This seems to break existing clusters that didn't have a node type explicitly defined. Let's give it more thought.

Kudos to @ipochi for finding out. Awesome catch!

@ipochi
Copy link
Member

ipochi commented Jun 18, 2020

Upgrade concern:

What happens in case of existing clusters where machine type is not provided and the previous default (t1.small.x86) is used. I assume the nodes will get deleted and cluster recreated.

In one scenario, the cluster was lost due to the Packet facility not having provisioning capacity for c3.small.x86 servers. Old nodes were deleted.

@ipochi ipochi dismissed their stale review June 18, 2020 13:31

Breaks existing clusters where default machine type is used.

@invidian
Copy link
Member

IMO this should be resolved the same way as #642 (comment).

@iaguis
Copy link
Contributor

iaguis commented Jun 25, 2020

IMO this should be resolved the same way as #642 (comment).

I agree with this, we can update the default and mention something in the release notes about users needing to define their instance type explicitly if they were using the default.

What do others think? @surajssd @johananl @rata?

@rata
Copy link
Member

rata commented Jun 25, 2020

@iaguis @invidian I strongly agree, a note in the changelog is enough IMHO and we should be able to change defaults (at least in these cases where the user can have the old setup working adding a line)

It should be loud and clear, though. Like a "changes required" section or something like that, so it is very visible in the changelog.

@iaguis
Copy link
Contributor

iaguis commented Jun 26, 2020

Can we get this rebased?

And please provide a "Release Notes" text to be included in the next release.

Thanks!

t1.small.x86 machines are becoming EOL and are no longer available in
new Packet projects. While c3.small.x86 is significantly larger (and
more expensive) than t1.small.x86, it is currently the smallest and
cheapest Packet device type in general availability.
Users may not be aware of the available Packet device types and/or
their pricing.
@johananl
Copy link
Member Author

Rebased and included release notes text in the description.

@johananl johananl requested a review from iaguis July 6, 2020 11:56
Copy link
Contributor

@iaguis iaguis left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

I'd do s/This change changes/This changes/g but the idea is to help with releases so it doesn't have to be perfect :)

@johananl
Copy link
Member Author

johananl commented Jul 6, 2020

Thanks! LGTM

I'd do s/This change changes/This changes/g but the idea is to help with releases so it doesn't have to be perfect :)

This is a strange phrasing indeed. Not sure why I wrote it this way. Fixed :-)

@johananl
Copy link
Member Author

johananl commented Jul 6, 2020

@iaguis I'm going to merge this without a green ARM build. The build failed due to Packet capacity problems and the change doesn't affect ARM AFAICT.

@iaguis
Copy link
Contributor

iaguis commented Jul 6, 2020

Sounds good.

@johananl johananl merged commit 7225aa9 into master Jul 6, 2020
@johananl johananl deleted the johananl/packet-default-dev-type branch July 6, 2020 12:54
@ipochi ipochi mentioned this pull request Jul 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants