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

Manage vpc-cni, coredns and kube-proxy via eks addons #276

Merged
merged 4 commits into from
Dec 1, 2021
Merged

Conversation

aidy
Copy link
Contributor

@aidy aidy commented Nov 24, 2021

No description provided.

@aidy aidy force-pushed the aidy/eks-addons branch 2 times, most recently from 5c19db2 to 989ef33 Compare November 25, 2021 14:57
@aidy aidy marked this pull request as ready for review November 25, 2021 16:37
@aidy aidy requested review from a team and ettiee and removed request for a team November 25, 2021 16:37
@aidy aidy changed the title Aidy/eks addons Manage vpc-cni, coredns and kube-proxy via eks addons Nov 25, 2021
@aidy aidy requested a review from errm November 25, 2021 16:37
Copy link
Contributor

@ettiee ettiee left a comment

Choose a reason for hiding this comment

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

lgtm - could you update the PR description before merging?

@ettiee
Copy link
Contributor

ettiee commented Nov 29, 2021

989ef33 removes a variable so probably worth documenting in UPGRADING.md

@aidy
Copy link
Contributor Author

aidy commented Nov 29, 2021

lgtm - could you update the PR description before merging?

hmm, so - my normal approach is to have sane, atomic, commits - and then to merge the commits, rather than squash.

Perhaps it's worth rewording 821d95f to have some reasoning for the motivation for this change. Would that work for you?

@ettiee
Copy link
Contributor

ettiee commented Nov 29, 2021

Perhaps it's worth rewording 821d95f to have some reasoning for the motivation for this change. Would that work for you?

Yes that sounds good 👍
And also link the related issues in the PR? (#224 and #256) for easy reference from the issues?

Copy link
Contributor

@ettiee ettiee left a comment

Choose a reason for hiding this comment

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

lgtm - be good to include #276 (comment)

@aidy
Copy link
Contributor Author

aidy commented Dec 1, 2021

lgtm - be good to include #276 (comment)

Oh, yes.

(That should probably have been a whole different PR - but we're here now)

errm and others added 4 commits December 1, 2021 12:12
For the switch to using eks_addons, we'll need to ensure that nodes are
up and running before trying to provision them.

Otherwise we end up in a race condition and get errors of the kind:
```
Error: unexpected EKS Add-On (terraform-aws-eks-testing-jPfg0J:coredns)
state returned during creation: unexpected state 'DEGRADED', wanted
target 'ACTIVE'. last error: %!!(MISSING)s(<nil>)
```
https://docs.aws.amazon.com/eks/latest/userguide/eks-add-ons.html

This makes a start on our reconsideration of addons - #224
Where available, we'd prefer to use eks addons rather than set them up
ourselves.
EKS and the node AMIs are using the same logic to calculate it.
@aidy aidy merged commit 1faaeba into main Dec 1, 2021
@errm errm deleted the aidy/eks-addons branch July 20, 2022 13:24
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.

None yet

3 participants