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

Added enviroment variable to allow ipamd to manage the ENIs on a non schedulable node #2296

Merged
merged 5 commits into from
Mar 7, 2023

Conversation

rajeeshckr
Copy link
Contributor

What type of PR is this?
feature

Which issue does this PR fix:
#2265

What does this PR do / Why do we need it:
TLDR, we need the ipamd to allocate pod IP from non primary ENI on a scheduling disabled node to run a key cluster daemonset component.
Details regarding the bug can be found under the issue

Testing done on this change:
Tested with unit test TestIncreaseIPPoolCustomENIOnNonSchedulableNode.
image

Automation added to e2e:
No

Will this PR introduce any new dependencies?:
No

Will this break upgrades or downgrades. Has updating a running cluster been tested?:
No

Does this change require updates to the CNI daemonset config files to work?:
No

Does this PR introduce any user-facing change?:
No

ipamd can manage ENIs on nonschedulable node when `AWS_VPC_K8S_CNI_MANAGE_ENIS_ON_NON_SCHEDULABLE_NODE` is set to `true`

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

@rajeeshckr rajeeshckr requested a review from a team as a code owner March 1, 2023 04:44
Copy link
Contributor

@jdn5126 jdn5126 left a comment

Choose a reason for hiding this comment

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

Change overall looks good to me, just a few nits. Can you also add the environment variable to the README.md? You can use version v1.13.0+.

And did you confirm that this fixes your issue? Like you build your own image, installed it, and this fixed things?

pkg/ipamd/ipamd.go Outdated Show resolved Hide resolved
pkg/ipamd/ipamd.go Outdated Show resolved Hide resolved
pkg/ipamd/ipamd.go Outdated Show resolved Hide resolved
@jdn5126
Copy link
Contributor

jdn5126 commented Mar 3, 2023

@rajeeshckr looks like some bad merge conflict resolution here. Can you fix the diff so that it only shows your changes?

@rajeeshckr
Copy link
Contributor Author

@rajeeshckr looks like some bad merge conflict resolution here. Can you fix the diff so that it only shows your changes?

@jdn5126, I have tested this building and running with the new image. Looks good to me! Thanks for helping with the review!

@rajeeshckr
Copy link
Contributor Author

I have updated the README.md, we are using v1.12.0. Is there a chance of cherry-picking this to that release?

Copy link
Contributor

@jdn5126 jdn5126 left a comment

Choose a reason for hiding this comment

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

Thanks for making these changes! Just two last nits and then we can kick off an integration run against this change

README.md Outdated Show resolved Hide resolved
pkg/ipamd/ipamd_test.go Show resolved Hide resolved
@jdn5126
Copy link
Contributor

jdn5126 commented Mar 7, 2023

I have updated the README.md, we are using v1.12.0. Is there a chance of cherry-picking this to that release?

Thanks for cleaning this up and adding additional test coverage! And yep, this will end up in the next 1.12 release, so v1.12.6, which we are working on now

@jdn5126
Copy link
Contributor

jdn5126 commented Mar 7, 2023

Integration test run status is here: https://github.com/aws/amazon-vpc-cni-k8s/actions/runs/4349737321

@jdn5126
Copy link
Contributor

jdn5126 commented Mar 7, 2023

Integration test passed, so merging now. Thanks for the help, @rajeeshckr !

@jdn5126 jdn5126 merged commit bdbd79f into aws:master Mar 7, 2023
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