Fix target group instances registration when TG created#302
Conversation
In case of creating a new TG removed return dropped off this function just before instance registration code. Daemon restart was needed to call ensureTargetGroup() again and reach out registration code (as targetGroup != nil, TG already created) Change-Id: I57717602624d9632049596e6928ac4671387905d
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
|
|
|
Welcome @abubyr! |
|
Hi @abubyr. Thanks for your PR. I'm waiting for a kubernetes 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. 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. |
|
|
||
| tg := result.TargetGroups[0] | ||
| return tg, nil | ||
| targetGroup = result.TargetGroups[0] |
There was a problem hiding this comment.
Seems like a simple fix. I wonder, @nckturner or @M00nF1sh, maybe this was coded this way originally because we wanted to sleep some period of time after creation of a TargetGroup before attempting to register expected instances in the TargetGroup below on original line 632?
There was a problem hiding this comment.
FYI: We've built an image with this patch if it helps speed up adoption: https://hub.docker.com/layers/189510904/freemangh/cloud-provider-aws/ada1f8a1-dirty/images/sha256-94271b14b2291cc577c36d5a3a300c1fa2036c856519359032ed3ec7c8905d61?context=repo
There was a problem hiding this comment.
I encountered the same issue with 1.23.0 today where the target group had no target registered.
Restarting the controller seems to have correctly updated the target group.
The return might be there to make a sleep, but I am not sure we are doing the reconcile loop again unless there is a change in the service.
I will try your version and see if it's happening again.
|
I just tried this fix using a cluster that is created every morning for multiple days in a row. It's correctly populating the target groups when first deploying the cloud controller \o/. |
|
/ok-to-test |
|
[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 |
|
Hello, This issue is still happening in 1.24.3 (causing a lot of issues and forcing us to move to CLB which the consequent reduction in the features like the possibility of assigning fixed IPs in the LBs). Any idea when this merge could get accepted as part of a following release? |
|
This PR has been released. Make sure you run cloud controller manager 1.24.1 or later |
|
@olemarkus There is no 1.24.1 in the Github Releases for this project, only 1.24.0 on 13 May. Is there another place to look for release info? Edit: Apologies for asking (un-) related project questions, but I see you're active on kOps as well... How does one instruct kOps 1.24.1 running Kubernetes 1.24.3 to use cloud-provider-aws 1.24.1? Wait for a kOps release? |
|
Try setting something like this I'll see to this being bumped in kOps shortly. |
…in-release-1.23 Automated cherry pick of #302: Fix target group instances registration when TG created
|
I recently came across this and I think I've found the original issue, https://github.com/kubernetes/cloud-provider-aws/pull/293/files#diff-70a4ebdf516a65404dc3c3b3ba9925fc62d0f85bd75e272a4f8ddb4b7b8ab852L620-L623 These lines were dropped, I'm guessing unintentionally and meant that the chunking work also got broken. IMO we should be reverting this PR and reinstating these lines, the end of this block should be like below, as it was originally |
|
I created #527 to restore the behaviour and re-separate the create and update behaviours. This will save us some API calls when creating services and re-distinguish the difference between create and update behaviour in the |
In case of creating a new TG removed return dropped off #this function
just before instance registration code. Daemon restart was needed to
call ensureTargetGroup() again and reach out registration code
(as targetGroup != nil, TG already created)
Change-Id: I57717602624d9632049596e6928ac4671387905d
What type of PR is this?
/kind bug
What this PR does / why we need it: fix of instances registration in AWS NLB target group during its creation
Which issue(s) this PR fixes:
Fixes #301
Special notes for your reviewer:
Does this PR introduce a user-facing change?: