-
Notifications
You must be signed in to change notification settings - Fork 742
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
Respect existing eniConfig label if set on node. #1596
Conversation
Thanks for the PR. We will review it. |
Might need to run "make format" for the unit test to pass. |
Signed-off-by: Jonah Back <[email protected]>
660954b
to
55f8965
Compare
@jayanthvn ran the make format - needs an approval to allow running workflows. |
hey @jayanthvn - anything needed here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the PR and UTs.
Cool! Is it fair to expect this to land in the 1.9.2 release? |
@jayanthvn any ideas if/when this will land in master? |
Sorry for the delay, we will finish the review by early next week. |
No worries - just wanted to make sure it wasn't lost / forgotten :) |
hey @jayanthvn - just wanted to nudge here again. Would love to see this in the next patch release! |
Hi, Sorry for the delay, can you please run "make format"? Rest of the code LGTM. /cc @achevuru |
@jayanthvn formatted the code and resolved merge conflicts. |
Nit: We need to update readme. Lgtm too. |
Closes #1593
Signed-off-by: Jonah Back [email protected]
What type of PR is this?
Bug
Which issue does this PR fix:
#1593
What does this PR do / Why do we need it:
This PR makes the CNI respect existing eniConfig labels if they exist. This is useful for users who may want to setup custom ENIConfigurations, but want to manage it through node labels (which notably can be passed to kubelet at startup) instead of through the override annotation.
If an issue # is not available please add repro steps and logs from IPAMD/CNI showing the issue:
Testing done on this change:
Manual testing on an EKS cluster to ensure that the CNI was properly choosing the right ENIConfig.
Automation added to e2e:
Will this break upgrades or downgrades. Has updating a running cluster been tested?:
It could be considered a breaking change if a user is currently setting these labels, since it would start using their specified ENIConfig instead of the one that the CNI chooses.
Does this change require updates to the CNI daemonset config files to work?:
No
Does this PR introduce any user-facing change?:
No
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.