Skip to content

Add option to also deregister the terminating instance from all ELBs#5

Open
piontec wants to merge 5 commits intopusher:masterfrom
trilogy-group:master
Open

Add option to also deregister the terminating instance from all ELBs#5
piontec wants to merge 5 commits intopusher:masterfrom
trilogy-group:master

Conversation

@piontec
Copy link
Copy Markdown

@piontec piontec commented Oct 18, 2018

This becomes essential when instances are exposed with ELB: after getting the termination signal, the instance is drained and soon will be terminated. Still, kube-proxy keeps running on the instance, so it's not removed from ELB. WHen isntance stops, ELB must detect failure on its own, which introduces a delay of at least 10s. With this PR terminating instances can deregister themselfs from all ELBs before they will be reclaimed by AWS.

Copy link
Copy Markdown
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just got a few minor questions on this but am fairly happy with the PR in general.

Would you be able to add something to the ReadMe on this new feature?

Out of interest, have you tried externalTrafficPolicy: local on your Kubernetes services, we don't see this problem and I believe it's because we have the external policy set. It means that kube-proxy only responds to requests if the service target exists on the node, so draining the node causes ELB health check failures.

Comment thread README.md
## Usage

### Deploy to Kubernetes
A docker image is available at `quay.io/pusher/k8s-spot-termination-handler`.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of interest, why are you removing this comment?

Comment thread deploy/clusterrole.yaml
verbs:
- get
- update
- patch
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see any reason to add this extra verb unless you've run into errors with RBAC using this helper?

Comment thread docker-entrypoint.py
print('Node Drain successful')
else:
print('Node drain failed, will retry')
continue
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this continue necessary?

@pusher-ci
Copy link
Copy Markdown

@piontec: PR needs rebase.

Details

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants