Skip to content
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

withdraw ClusterIP bgp route for externalTrafficPolicy=Local when there are no local endpoints #347

Merged
merged 4 commits into from
Mar 21, 2018

Conversation

TvL2386
Copy link
Contributor

@TvL2386 TvL2386 commented Mar 21, 2018

Hi guys,

I noticed that when using a Service object with type: Loadbalancer and externalTrafficPolicy: Local, the routes are not withdrawn. On my external BGP peer I could still see the route which caused for dropped packets when that route was used.

This pull request withdraws routes for this externalTrafficPolicy when there are no endpoints.

@SEJeff
Copy link
Contributor

SEJeff commented Mar 21, 2018

Refs #347

err := nrc.bgpServer.DeletePath([]byte(nil), 0, "", pathList)

if err != nil {
return fmt.Errorf(err.Error())
Copy link
Collaborator

Choose a reason for hiding this comment

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

just

return err 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you, but the AdvertiseClusterIp method above it should be refactored as well then, since the methods are structured the same

Copy link
Collaborator

Choose a reason for hiding this comment

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

feel free to fix the method above too :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alrighty then :)

@andrewsykim
Copy link
Collaborator

andrewsykim commented Mar 21, 2018

Tested this patch and it works as we expect. However, the convergence time in kube-router is taking a bit longer than I would like once the route is removed. I think this is expected if you use the default sync period (60s)

@andrewsykim
Copy link
Collaborator

Might be a good time to refactor to use informers so we can get the update events for endpoints right away, as opposed to listing all endpoints on each sync loop

@andrewsykim
Copy link
Collaborator

cc @murali-reddy

@TvL2386
Copy link
Contributor Author

TvL2386 commented Mar 21, 2018

I agree that the convergence is not really acceptable. Ideally it should be ran immediately when there are no endpoints left on the node.

@andrewsykim
Copy link
Collaborator

LGTM, thanks for the PR @TvL2386

I'll try to do some work in the upcoming week to shorten the convergence time.

@andrewsykim andrewsykim merged commit 035a9a8 into cloudnativelabs:master Mar 21, 2018
@TvL2386 TvL2386 deleted the delete_path branch March 28, 2018 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants