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.

Sign up for GitHub

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

Jump to bottom

Add BlockDeviceMappings to the AWS cloudprovider #1420

Merged
merged 5 commits into from
Mar 10, 2022
Merged

Add BlockDeviceMappings to the AWS cloudprovider #1420

merged 5 commits into from
Mar 10, 2022

Conversation

bwagner5
Copy link
Contributor

@bwagner5 bwagner5 commented Feb 25, 2022 •
edited
Loading

1. Issue, if available:
#939
#1415

2. Description of changes:

  • Create Launch Template Resolver abstraction that is able to model dynamic LT parameters that could vary with AMIFamily or something other attribute. This abstraction helps to contain the logic in AMI resolution, userdata scripts (bootstrappers), and block device mappings to a small set of AMIFamilies that implement the Options interface.
  • Add a BlockDeviceMappings struct to the AWS Cloud Provider provisioner spec
    • BlockDeviceMappings allows configuration of the volume size, type, IOPS, throughput, encryption, encryption key, and deleteOnTermination flag.
  • Remove Persistent Defaulting of AMIFamily and instead runtime default to AL2

3. How was this change tested?

  • Functional Tests
  • Manual Tests on EKS
    • Tested the runtime defaulting w/ a provisioner w/o BlockDeviceMappings configured (created provisioner before installing the updated version of karpenter and then scaling out)
    • Tested the defaulting logic by creating a new provisioner which correctly picked up the default BlockDeviceMappings of a 20GiB volume.
    • Varied the BlockDeviceMapping by setting a volume type of io2 and IOPS to 10000. Observed the proper volume configuration.

4. Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: link to issue
  • No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@netlify
Copy link

netlify bot commented Feb 25, 2022 •
edited
Loading

✔️ Deploy Preview for karpenter-docs-prod canceled.

🔨 Explore the source changes: b2e285f

🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/622a4b8be84a0a00094931a5

@bwagner5 bwagner5 changed the title Add rootVolumeOptions to the AWS cloudprovider [WIP] Add rootVolumeOptions to the AWS cloudprovider Feb 26, 2022
@bwagner5 bwagner5 force-pushed the volume-config branch 3 times, most recently from 43cc783 to 35077a3 Compare February 26, 2022 18:27
@bwagner5 bwagner5 changed the title [WIP] Add rootVolumeOptions to the AWS cloudprovider [WIP] Add BlockDeviceMappings to the AWS cloudprovider Feb 26, 2022
@bwagner5 bwagner5 changed the title [WIP] Add BlockDeviceMappings to the AWS cloudprovider Add BlockDeviceMappings to the AWS cloudprovider Feb 26, 2022
@bwagner5 bwagner5 requested a review from ellistarn February 28, 2022 21:35
@bwagner5 bwagner5 force-pushed the volume-config branch 2 times, most recently from 49bcc72 to 89bcc71 Compare March 2, 2022 23:28
@bwagner5 bwagner5 requested a review from ellistarn March 2, 2022 23:47
@bwagner5 bwagner5 force-pushed the volume-config branch 4 times, most recently from 10b66de to b3eb44c Compare March 9, 2022 19:25
@bwagner5 bwagner5 changed the title [WIP] Add BlockDeviceMappings to the AWS cloudprovider Add BlockDeviceMappings to the AWS cloudprovider Mar 9, 2022
suket22
suket22 previously approved these changes Mar 9, 2022
View reviewed changes
pkg/cloudprovider/aws/ltresolver/resolver.go Outdated
BlockDeviceMappings []*v1alpha1.BlockDeviceMapping
MetadataOptions *v1alpha1.MetadataOptions
AMIID string
InstanceTypes []cloudprovider.InstanceType `hash:"ignore"`
Copy link
Contributor

Choose a reason for hiding this comment

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

InstanceTypes aren't being set within the launch template right? I think the naming ResolvedTemplate threw me off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct, they are set in the instance provider (within the fleet request). They're in the resolved template because they're kind of a dynamic parameter that is dependent on the AMI selected.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm super confused by this astraction.

pkg/cloudprovider/aws/securitygroups.go Show resolved Hide resolved
@bwagner5 bwagner5 mentioned this pull request Mar 10, 2022
Closed
ellistarn
ellistarn reviewed Mar 10, 2022
View reviewed changes
Copy link
Contributor

@ellistarn ellistarn left a comment

Choose a reason for hiding this comment

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

Great changes, a couple naming nits.

@ellistarn ellistarn merged commit c1d08c6 into aws:main Mar 10, 2022
@bwagner5 bwagner5 mentioned this pull request Mar 11, 2022
Closed
3 tasks
@spring1843 spring1843 mentioned this pull request Mar 15, 2022
Merged
3 tasks
@bwagner5 bwagner5 mentioned this pull request Mar 17, 2022
Closed
3 tasks
@walkley walkley mentioned this pull request Mar 18, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tzneal tzneal tzneal left review comments

@ellistarn ellistarn ellistarn approved these changes

@suket22 suket22 suket22 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bwagner5 @tzneal @ellistarn @suket22