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

fix: Karpenter 1.0 installation fails to create default EC2NodeClass #250

Closed
wants to merge 0 commits into from

Conversation

JoeNorth
Copy link
Contributor

@JoeNorth JoeNorth commented Sep 9, 2024

Issue #249

Description of changes:

  • Fixed misspelled function names for getting EKS AMIs.

  • Updated the logic for getting EKS AMIs for the default EC2NodeClass.

With the release of Karpenter v1.0 spec.amiSelectorTerms is now a required field for EC2NodeClass. This posed an issue as the previous logic did not create this field for Bottlerocket AMIs and thus caused the default EC2NodeClass creation to fail.

Starting with Karpenter v1.0, spec.amiSelectorTerms.alias is now available. The proposed changes simplify this logic so that only the amiFamily and latest AMI release version need to be known and Karpenter will figure out the AMIs to be used.

The alias field is now passed to the default EC2NodeClass in the format ami-family@ami-version. For example:

amiSelectorTerms:
  - alias: al2@v20240904

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@aaroniscode
Copy link
Contributor

aaroniscode commented Sep 10, 2024

Hi @JoeNorth

I'm aiming for the install karpenter to match the Getting Started Guide as close as possible and in Step 5. Create NodePool the example uses amiSelectorTerms with specific AMI IDs which is best practice.

Your PR removes the AMI ID's list and replaces with an alias. I'm OK with having an option flag to set an alias, though I'd like the default to match the Getting Started Guide. I think this needs to just add a lookup for Bottlerocket AMI's.

If you aren't familiar with this code I'm happy to add it to your PR, just let me know.

@JoeNorth
Copy link
Contributor Author

Thanks for the feedback @aaroniscode. I can see where sticking with the resolved AMI IDs would be a good default.

For now I'll remove the alias support and keep this PR to just fixing the immediate issue. Support for the optional alias param can be a separate feature PR down the road.

@JoeNorth JoeNorth closed this Sep 18, 2024
@JoeNorth JoeNorth force-pushed the bug/karpenter-nodeclass-alias branch from 8f6ef3a to 9ce9c2e Compare September 18, 2024 18:18
@JoeNorth JoeNorth deleted the bug/karpenter-nodeclass-alias branch September 18, 2024 18:19
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.

2 participants