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

Pod ENI #924

Merged
merged 1 commit into from
Dec 9, 2021
Merged

Pod ENI #924

merged 1 commit into from
Dec 9, 2021

Conversation

bdwyertech
Copy link
Contributor

@bdwyertech bdwyertech commented Dec 6, 2021

1. Issue, if available:
#905

2. Description of changes:
This improves instance type selection to factor in Security Groups for Pods by ensuring only instance types capable of Pod ENI are launched if set in a pods resource request.

This leverages the lookup table in vpc-resource-controller to determine Pod ENI support.

3. 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 Dec 6, 2021

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

🔨 Explore the source changes: 5f43c70

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

@bdwyertech bdwyertech force-pushed the pod-eni branch 2 times, most recently from 52252b0 to a110bbf Compare December 6, 2021 04:53
@akestner akestner added networking Issues related to Karpenter and cluster networking and removed networking Issues related to Karpenter and cluster networking labels Dec 6, 2021
@@ -61,6 +61,7 @@ type InstanceType interface {
NvidiaGPUs() *resource.Quantity
AMDGPUs() *resource.Quantity
AWSNeurons() *resource.Quantity
AWSPodENI() *resource.Quantity
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit challenging. We've been talking about modeling this in a more vendor neutral way like:

Resources() map[resource.Quantity]

I'm willing to refactor this later, and accept this as is, though.

@ellistarn
Copy link
Contributor

This is a pretty cool WIP! I'm assuming that aws.amazon.com/pod-eni is already written onto nodes by the CNI controller and works w/ the kube scheduler?

@bdwyertech
Copy link
Contributor Author

bdwyertech commented Dec 7, 2021

This is a pretty cool WIP! I'm assuming that aws.amazon.com/pod-eni is already written onto nodes by the CNI controller and works w/ the kube scheduler?

Update:
So digging through the VPC CNI code a bit more, I now understand the logic a bit better. Apparently the label vpc.amazonaws.com/has-trunk-attached=false signals the VPC Resource Controller to attach an ENI.
https://github.com/aws/amazon-vpc-cni-k8s/blob/01654383404fe88f18a89508d9406ad52c6eec1f/pkg/ipamd/ipamd.go#L1194-L1195

There is also a lookup table -- this is ugly, but it is what should be used to determine Pod ENI support:
https://github.com/aws/amazon-vpc-resource-controller-k8s/blob/05e89ff9300a5cc0ebea705834cf27f0a7f3b509/pkg/aws/vpc/limits.go#L27

Also, it looks like there is a well-known limit vpc.amazonaws.com/pod-eni which is added to pods requiring a Pod ENI so the logic here should work fine:

VPC Resource Controller - Pod Webhook

VPC CNI - Handler

image

@bdwyertech bdwyertech force-pushed the pod-eni branch 2 times, most recently from 0f2fa68 to c71efdc Compare December 8, 2021 02:55
@bdwyertech bdwyertech changed the title WIP: Pod ENI Pod ENI Dec 8, 2021
@ellistarn
Copy link
Contributor

Awesome! This is shaping up really nicely!

@bdwyertech
Copy link
Contributor Author

bdwyertech commented Dec 8, 2021

I was kind of surprised by the hard-coded lookup table in the VPC controller and that the new m6i and m6a instances don't support this!

@ellistarn
Copy link
Contributor

Ready for review?

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.

One minor comment. Otherwise great job!

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.

Nicely done!

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.

3 participants