-
Notifications
You must be signed in to change notification settings - Fork 598
🐛 awsmachine: only register machine to LB when it's running #5040
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
Conversation
Hi @r4f4. 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. |
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
@mtulio: changing LGTM is restricted to collaborators In response to this:
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. |
No regressions found in Openshift e2e CI tests: openshift/installer#8678 |
/ok-to-test |
/hold |
The AWS docs [1] state that to register an instance with the Load Balancer target groups, the instance must be running. Currently, CAPA tries to register it while it's still pending, causing the following error: ``` 0314 22:24:18.512176 701419 awsmachine_controller.go:605] "failed to reconcile LB attachment" err=< [could not register machine to load balancer: could not register control plane instance "i-039bb22b1e8df7c99" with load balancer: failed to register instance with target group 'mrb-capa-67-d2pfx-int-22623': InvalidTarget: The following targets are not in a running state and cannot be registered: 'i-039bb22b1e8df7c99' status code: 400, request id: 17514354-77ac-42b1-a882-489760563bbd, could not register machine to load balancer: could not register control plane instance "i-039bb22b1e8df7c99" with load balancer: failed to register instance with target group 'mrb-capa-67-d2pfx-ext-6443': InvalidTarget: The following targets are not in a running state and cannot be registered: 'i-039bb22b1e8df7c99' status code: 400, request id: 84e1849c-abb7-4af9-9220-8791fdc1a3fb] > I0314 22:24:18.512325 701419 recorder.go:104] "events: Failed to register control plane instance \"i-039bb22b1e8df7c99\" with load balancer: failed to register instance with target group 'mrb-capa-67-d2pfx-ext-6443': InvalidTarget: The following targets are not in a running state and cannot be registered: 'i-039bb22b1e8df7c99'\n\tstatus code: 400, request id: 84e1849c-abb7-4af9-9220-8791fdc1a3fb" type="Warning" object={"kind":"AWSMachine","namespace":"openshift-cluster-api-guests","name":"mrb-capa-67-d2pfx-bootstrap","uid":"58af1162-380b-4f3e-93fe-c0e81401070e","apiVersion":"infrastructure.cluster.x-k8s.io/v1beta2","resourceVersion":"562"} reason="FailedAttachControlPlaneELB" ``` Even though this doesn't stop the install from succeeding, let's wait for the instance state to be "running" and with that avoid unnecessary AWS API calls. [1] https://docs.aws.amazon.com/elasticloadbalancing/latest/application/target-group-register-targets.html#register-instances
With the updated version:
/test pull-cluster-api-provider-aws-e2e-blocking |
/test pull-cluster-api-provider-aws-e2e |
/hold cancel I don't see any regressions on Openshift e2e tests. |
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
/test pull-cluster-api-provider-aws-e2e |
/milestone v2.6.0 |
/lgtm also |
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.
/approve |
Its already approved but also: /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AndiDog, richardcase The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@r4f4 does this need backporting? |
@damdo It's not a serious issue but I think it'd be nice to backport. |
@richardcase wdyt? |
This bump includes the following fix to only register instances to the LB when they are in a running state: * kubernetes-sigs/cluster-api-provider-aws#5040 This should avoid unnecessary AWS API calls and red herring error messages in the log output.
This bump includes the following fix to only register instances to the LB when they are in a running state: * kubernetes-sigs/cluster-api-provider-aws#5040 This should avoid unnecessary AWS API calls and red herring error messages in the log output.
This bump includes the following fix to only register instances to the LB when they are in a running state: * kubernetes-sigs/cluster-api-provider-aws#5040 This should avoid unnecessary AWS API calls and red herring error messages in the log output.
What type of PR is this?
/kind bug
What this PR does / why we need it:
The AWS docs [1] state that to register an instance with the Load Balancer target groups, the instance must be running. Currently, CAPA tries to register it while it's still pending, causing the following error:
Even though this doesn't stop the install from succeeding, let's wait for the instance state to be "running" and with that avoid unnecessary AWS API calls.
[1] https://docs.aws.amazon.com/elasticloadbalancing/latest/application/target-group-register-targets.html#register-instances
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #5037
Special notes for your reviewer:
Checklist:
Release note: