-
Notifications
You must be signed in to change notification settings - Fork 848
Add EC2 metadata-labeler metadata source #2591
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
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @sylviah23. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/ok-to-test |
Code Coverage Diff
|
/test pull-aws-ebs-csi-driver-image-test |
/test pull-aws-ebs-csi-driver-image-test Fixed issue with ECR repo being full. |
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.
First review round: I have not manually tested yet, intend to do that in my next review. I also don't think I was as in depth with looking at some parts (in particular the tests) as I should have been, so I might have missed some bugs lurking there.
The comments about the leader election are probably the most critical comments in this review - as written, this code will keep running after the leader election is switched away, causing two or more instances of the driver to simultaneously run the patcher and race each other.
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.
Thank you for the contribution and adding comprehensive code coverage on the first revision @sylviah23 . Overall, this is looking solid 🙂.
The leader election code needs some work, as Connor suggested #2591 (comment), leveraging the CSI lib-utils wrapper will be a nice improvement. That code is battle tested and already addresses some of the core challenges we need to solve implementing this enhancement.
b433a86
to
c0a4312
Compare
/retest |
4302445
to
6d90aa7
Compare
09dc542
to
5ccdb9f
Compare
/unhold I will make any other last small changes from personal computer and personal AWS account |
5ccdb9f
to
5ec7434
Compare
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What type of PR is this?
/kind feature
What is this PR about? / Why do we need it?
This PR gets the Volume and ENI count of each node via the EC2 API. It then adds this information to the metadata labels of each node in the cluster, so Kubernetes can get this information without IMDS access.
How was this change tested?
Unit testing and E2E testing and manually testing with EKS cluster by adding a new node.
Does this PR introduce a user-facing change?