Skip to content

Conversation

sylviah23
Copy link

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?

Add new metadata source relying on EC2.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Jul 31, 2025
Copy link

linux-foundation-easycla bot commented Jul 31, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: sylviah23 / name: Sylvia Han (2938b0c)
  • ✅ login: AndrewSirenko / name: Drew Sirenko (5ec7434)

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jul 31, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign jsafrane for approval. For more information see the Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot
Copy link
Contributor

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 31, 2025
@AndrewSirenko
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 31, 2025
Copy link

github-actions bot commented Jul 31, 2025

Code Coverage Diff

File Old Coverage New Coverage Delta
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/cloud/cloud.go 87.1% 84.7% -2.4
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/cloud/metadata/k8s.go 58.3% 72.3% 14.0
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/cloud/metadata/labels.go Does not exist 57.6%
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/cloud/metadata/metadata.go 84.8% 78.5% -6.3

@mdzraf
Copy link
Member

mdzraf commented Jul 31, 2025

/test pull-aws-ebs-csi-driver-image-test

@mdzraf
Copy link
Member

mdzraf commented Aug 1, 2025

/test pull-aws-ebs-csi-driver-image-test

Fixed issue with ECR repo being full.

Copy link
Contributor

@ConnorJC3 ConnorJC3 left a 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.

Copy link
Member

@torredil torredil left a 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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 7, 2025
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 3, 2025
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 3, 2025
@AndrewSirenko
Copy link
Contributor

/retest

@AndrewSirenko AndrewSirenko force-pushed the patcher branch 3 times, most recently from 4302445 to 6d90aa7 Compare September 5, 2025 18:01
@AndrewSirenko AndrewSirenko force-pushed the patcher branch 5 times, most recently from 09dc542 to 5ccdb9f Compare September 8, 2025 22:55
@AndrewSirenko
Copy link
Contributor

/unhold

I will make any other last small changes from personal computer and personal AWS account

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 8, 2025
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2025
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 23, 2025
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 24, 2025
@k8s-ci-robot
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants