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

Pulumi shouldn't fetch the latest AMI by default #88

Closed
ggilmore opened this issue Mar 28, 2019 · 9 comments · Fixed by #114
Closed

Pulumi shouldn't fetch the latest AMI by default #88

ggilmore opened this issue Mar 28, 2019 · 9 comments · Fixed by #114
Assignees
Labels
customer/feedback Feedback from customers
Milestone

Comments

@ggilmore
Copy link

ggilmore commented Mar 28, 2019

When using pulumi update with an EKS cluster, pulumi's default behavior is to always fetch the latest version of the EKS AMI if nodeAmiId isn't specified.

While this behavior can be convenient, unexpected upgrades to the EKS AMI can cause unexpected downtime or break the cluster entirely.

For production situations, it's strongly recommended that you explicitly pass the nodeAmiId parameter so that you have full control over when your nodes upgrade.

pulumi-eks's default behavior should enforce best practices. I propose the following:

  1. Make nodeAmiId a required parameter (force users to make a conscious decision about the AMI that they want to use).
  2. Provide the existing fetch behavior only as a convenience function for users that don't need fine control over when their cluster upgrades.
  3. Warn people in the documentation for the above helper function about not using this in production.

See https://pulumi-community.slack.com/archives/C84L4E3N1/p1553791785005000 for more discussion.

@metral metral self-assigned this Mar 28, 2019
@metral metral added this to the 0.22 milestone Mar 28, 2019
@metral metral added the customer/feedback Feedback from customers label Mar 28, 2019
@lbogdan
Copy link

lbogdan commented Mar 28, 2019

  1. Have some kind of initial default, which will only get set on the first run, and be remembered for the subsequent runs.

@lukehoban
Copy link
Member

Have some kind of initial default, which will only get set on the first run, and be remembered for the subsequent runs.

Indeed - this is something we want to enable as part of the ignoreChanges feature we are working on this milestone (pulumi/pulumi#2277). I'm not sure yet whether it's the right solution here - but it's the one that most immediately felt like a good balance to me.

@lbogdan
Copy link

lbogdan commented Apr 1, 2019

@lukehoban If I correctly understood that feature's behavior, it's not really what we need, as we should still be able to change the AMI by explicitly specifying it.

@lukehoban lukehoban modified the milestones: 0.22, 0.23 Apr 12, 2019
lukehoban added a commit that referenced this issue Apr 24, 2019
When we use the "smart default" to pick the most recent EKS-optimized AMI, we now also mark the image as `ignoreChanges` to ensure that we do not pick up new "smart defaults" later.  Users can still explicitly specify an `amiID`, and if they do this will also remove the `ignoreChanges` annotation so that the change will be applied.

Fixes #88.
@lukehoban
Copy link
Member

I opened #114 with my proposal for this. I believe it addresses @lbogdan's concern about whether it is possible to "unignore" this so that an explicit AMI can be provided. My feeling is this provides a nice balance of still having a "smart default" at creation time, and allowing full explicit customization, but without any surprises during updates. @ggilmore @lbogdan @metral Would appreciate your feedback on whether this sounds like a good solution.

@lbogdan
Copy link

lbogdan commented Apr 25, 2019

That seems to be exactly what we need, thanks!

@ggilmore
Copy link
Author

This sounds like a good compromise.

lukehoban added a commit that referenced this issue May 1, 2019
When we use the "smart default" to pick the most recent EKS-optimized AMI, we now also mark the image as `ignoreChanges` to ensure that we do not pick up new "smart defaults" later.  Users can still explicitly specify an `amiID`, and if they do this will also remove the `ignoreChanges` annotation so that the change will be applied.

Fixes #88.
lukehoban added a commit that referenced this issue May 1, 2019
When we use the "smart default" to pick the most recent EKS-optimized AMI, we now also mark the image as `ignoreChanges` to ensure that we do not pick up new "smart defaults" later.  Users can still explicitly specify an `amiID`, and if they do this will also remove the `ignoreChanges` annotation so that the change will be applied.

Fixes #88.
@lukehoban lukehoban assigned lukehoban and unassigned metral May 1, 2019
@gitfool
Copy link

gitfool commented Sep 11, 2019

Falling back to requiring AMI Id is not so friendly. It would be better to be able to specify an exact k8s version, e.g. 1.14.6, or an exact AMI name, e.g. amazon-eks-node-1.14-v20190906, and still have the AMI Id looked up, since the optimal AMI Id will vary depending on region and so should match the region the Pulumi stack is in.

@metral
Copy link
Contributor

metral commented Sep 19, 2019

@gitfool The new AWS SSM Parameter Store API was just announced yesterday. #258 is tracking the work to leverage it.

@liveandletbri
Copy link

It looks like, with the merging of #366 and closing of #258, the ignoreChanges that was added in #114 was removed. This appears to mean the AMI is dynamically selected again, is that correct?

My team just experienced the issue described here, where the AMI updated and the services running in our cluster experienced downtime. I am not clear about what the solution is, given that ingoreChanges is gone.

Another thought, which may warrant the creation of a new issue: when this happens, ideally the new nodes would be spun up, and Pulumi would wait until they're healthy before swapping over to them and then shutting the old nodes. Is there a way to make that happen? Should I open a new issue for it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer/feedback Feedback from customers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants