-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
wait for instances to drain from classic LB #12902
Conversation
Hi @heybronson. 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. 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/test-infra repository. |
/ok-to-test |
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.
Ideally we could cache the DescribeAutoScalingGroups response since it isn't likely to change between deleteInstance calls on instances from the same InstanceGroup, but the frequency that the Describe call is being made (once every few minutes) shouldn't have a material impact on AWS API call volume.
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.
So this is only load balancers that are associated with the ASG, so probably only the ones directly created by kOps. Not the ones created by KCM/LBC. Unfortunate there doesn't seem to be a call to enumerate the load balancers that an instance is registered with.
And this only helps with rolling update, not cluster-autoscaler scale-down. I'm still hoping for someone to implement graceful node shutdown.
/assign @johngmyers |
friendly bump |
d012cc4
to
0ccb8b1
Compare
/assign @olemarkus |
/lgtm Will approve unless someone objects in the next day or so. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: johngmyers, olemarkus 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 |
/test pull-kops-verify-golangci-lint |
Now that we've enabled connection draining by default for all classic LBs (#12881), we need to ensure that we're waiting for connectionDraining to complete before terminating the instance.
Deleting the instance without waiting doesn't allow for connections to drain. We need to call
DeregisterInstancesFromLoadBalancer
which will ensure that there aren't any ongoing connections to the instance before we terminate.Gathering the corresponding loadBalancers by describing the underlying ASG.
Confirmed that rolling-updates continue to behave as expected and 0% of requests are dropped from corresponding load balancers.