-
Notifications
You must be signed in to change notification settings - Fork 53
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
Add Windows secondary IP mode configurable options for managing IP address allocation #443
Conversation
2286c6f
to
ba0f44a
Compare
ba0f44a
to
3598972
Compare
cc @jiechen0826 Please join the review |
@haouc @jiechen0826 I considered creating a validating webhook as part of this change to validate the configuration values and prevent invalid values from being set. I chose to leave it out to keep the change minimal and do a follow up change separately. Any thoughts on this? |
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.
I'm halfway through the changes, but just posting these comments first
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.
Reviewed all files. Ready to approve once the above comments are addressed.
3598972
to
aedc76d
Compare
aedc76d
to
04d8b58
Compare
04d8b58
to
677d4f5
Compare
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.
Just one more comment. Btw, there seems to be a unit test failure.
677d4f5
to
8d8540c
Compare
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 implementing the feature and addressing my comments!
8d8540c
to
9c7bc6e
Compare
9c7bc6e
to
1e9cb9b
Compare
@jiechen0826 I have added documentation and integration tests. Please take another look? The logic you reviewed has not changed. Note: New integration tests for secondary IP mode are all passing, currently investigating a few failures in PD tests. Once all integration tests are passing I will drop a note on the test output. In the meantime please proceed to review. |
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 adding the integration tests and doc update!
1e9cb9b
to
a05c40a
Compare
f2a537f
to
6bb794f
Compare
6bb794f
to
ae525f5
Compare
ae525f5
to
e466622
Compare
e466622
to
4bd2bce
Compare
lgtm, thanks. |
* add finalizer handler in v1.4 * fix an err variable * adding logs for mismatched CNINode * add metrics for mismatches * update EC2 instance types * Update aws-sdk-go and change way to get regional sts endpoint (#466) * Missing dependency update * Remove hard failure for not getting global STS endpoint (#467) * updating k8s manifest * chaning go to major.minor format (#477) * updating go version and controller-gen version (#464) * Add new target for building docker images with no tests (#415) * updating rbac * Add Windows secondary IP mode configurable options for managing IP address allocation (#443) * Add Windows secondary IP mode configurable options (#443) #443 * Various code fixes for PR feedback #443 * adding ctx in test * updating ec2 supported instance types (#475) --------- Co-authored-by: Hao Zhou <[email protected]> Co-authored-by: Jay Deokar <[email protected]> Co-authored-by: Jay Deokar <[email protected]> Co-authored-by: Tatenda Zifudzi <[email protected]>
Issue
Resolves #428
Description of changes
This change introduces changes to provide configurable options for managing IP address allocation on Windows nodes when using secondary IP mode. The VPC Resource Controller will now allow configuring of the
windows-minimum-ip-target
andwindows-warm-ip-target
parameters in theamazon-vpc-cni
ConfigMap. These parameters will control the minimum number of IP addresses to be allocated on each Windows node, and the number of 'warm' IP addresses to be pre-allocated respectively. The VPC RC will always ensure that based on the configurations, there will always be enough warm IPs to satisfy the configurations.Previously, the VPC Resource Controller utilized a fixed and hardcoded number of 3 warm secondary IP addresses on each Windows node. This approach, while beneficial for enabling faster pod startup times, wasn't ideal for the following example scenarios.
Scope
enable-windows-ipam
is set totrue
Notes for reviewers
Todo
Integration Test Output
Output as of Tue Jul 23 11:52AM PDT
Integration test runs