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 ENI support for nodes(for Fargate nodes) #223

Merged
merged 1 commit into from
Jun 23, 2021
Merged

Add ENI support for nodes(for Fargate nodes) #223

merged 1 commit into from
Jun 23, 2021

Conversation

SaranBalaji90
Copy link
Contributor

@SaranBalaji90 SaranBalaji90 commented May 23, 2021 •
edited
Loading

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR adds support for adding Fargate worker nodes to Kubernetes cluster. To add Fargate worker nodes, node providerID should have the prefix "fargate-" followed by ENI IP address..

Special notes for your reviewer:
This code doesn't change anything for EC2 worker nodes and only performs Describe interface for Fargate worker nodes.

Does this PR introduce a user-facing change?:

Adds support for worker nodes based on ENI

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 23, 2021
@k8s-ci-robot k8s-ci-robot requested review from ayberk and nckturner May 23, 2021 18:27
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 23, 2021
nckturner
nckturner reviewed Jun 23, 2021
View reviewed changes
@SaranBalaji90 SaranBalaji90 force-pushed the eni-support branch 2 times, most recently from 81732b1 to d352941 Compare June 23, 2021 15:25
nckturner
nckturner reviewed Jun 23, 2021
View reviewed changes
pkg/providers/v1/aws.go
@@ -1732,6 +1777,10 @@ func (c *Cloud) InstanceTypeByProviderID(ctx context.Context, providerID string)
return "", err
}

if isFargateNode(string(instanceID)) {
return "", nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason we didn't create a custom "fargate" instance type? Not saying we should but just wondering if we explored that option. It might be nice from an API consumer's perspective to not have to check the provider ID to determine why an instance type is empty. Also it could be useful as fargate evolves to be able to have different instance types, if there is any information we want to expose about the instance. Of course, that can come later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point but we don't want to populate this information as Fargate abstracts the instance types. But should be easy to add our own definition for instance-type as "fargate" if we need to. But in future it might affect if Fargate has some plan to expose the task type as "compute-optimized", "memory-optimized", etc. My suggestion is to keep it simple for now and add the type as "fargate" later if needed. Do you agree as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, exposing the task type would be be a good future reason to fill this in. In that case, setting it to "fargate" shouldn't affect anything negatively, but we can keep it simple for now and wait until someone asks for something like that.

@nckturner
Copy link
Contributor

Did you test the node controller/node addresses with the kubelet cloud provider set to external?

@SaranBalaji90
Copy link
Contributor Author

Did you test the node controller/node addresses with the kubelet cloud provider set to external?

Yes we tested on EKS Fargate enabled cluster :)

@nckturner
Copy link
Contributor

Thanks. Looks like you need to fix the gofmt error, then I'll lgtm.

@nckturner
Copy link
Contributor

/cc @wongma7

@k8s-ci-robot k8s-ci-robot requested a review from wongma7 June 23, 2021 17:51
@nckturner
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 23, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nckturner, SaranBalaji90

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 23, 2021
@k8s-ci-robot k8s-ci-robot merged commit 7d6f102 into kubernetes:master Jun 23, 2021
@saurav-agarwalla saurav-agarwalla mentioned this pull request May 25, 2022
Merged
k8s-ci-robot added a commit that referenced this pull request May 25, 2022
@k8s-ci-robot
Merge pull request #392 from saurav-agarwalla/automated-cherry-pick-o…
4d9ce2f 
…f-#223-#387-upstream-release-1.20

Automated cherry pick of #223: Add ENI support for nodes(for Fargate nodes)
#387: Fix issues in tagging controller
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nckturner nckturner nckturner left review comments

@ayberk ayberk Awaiting requested review from ayberk

@wongma7 wongma7 Awaiting requested review from wongma7

Assignees

@nckturner nckturner

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SaranBalaji90 @nckturner @k8s-ci-robot