Restore chunked behaviour of target group creation in LBv2#527
Conversation
|
@JoelSpeed: This issue is currently awaiting triage. If cloud-provider-aws contributors determine this is a relevant issue, they will accept it by applying the The DetailsInstructions 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/test-infra repository. |
|
Thanks for looking at this. The cyclomatic complexity of that function is off the charts ... apologies for introducing bugs. /lgtm |
Yeah it's pretty bad! No worries! |
|
any ETA of merging this fix? I went into the same case where instances were not registered in the respective target groups |
|
/assign @andrewsykim Are you able to approve Andrew? |
|
I will have a look. I'm doing the 1.26 release in not too long and should get this in before that. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: olemarkus The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
So do we know when the fix will be rolled out to EKS. Will it be in the form of another EKS platform version? Thanks. |
What type of PR is this?
What this PR does / why we need it:
When kubernetes/kubernetes##101592 introduced the target group registration chunking, this is how the target groups were being registered. Then we would return. The rest of this function was intended to handle the case where the load balancer already exists.
#293, I believe, accidentally removed the target registration from the "create" part of this function, creating the bug #301. #302 resolved this by allowing the function to fall through to the "update" section of the code.
Since we don't need to run the checks for existing and dirty health checks on create, I think we should restore the original behaviour to prevent those unnecessary API calls.
I've manually tested this myself by building a cluster and checking that when I create an NLB backed service, the targets are registered correctly on the first instance.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: