-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Refactor default backend handling and add better events #21
Merged
bprashanth
merged 5 commits into
kubernetes:master
from
bprashanth:default_backend_retry
Dec 7, 2016
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
6260eee
Re-order default backend deletion calls
bprashanth b789b78
Log urlmap
bprashanth d7fb15d
Better events and timeouts for health checks
bprashanth d231222
Don't retry on instance group not found
bprashanth 8c4d951
Bump up glbc version
bprashanth File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -88,19 +88,8 @@ func NewLoadBalancerPool( | |
} | ||
|
||
func (l *L7s) create(ri *L7RuntimeInfo) (*L7, error) { | ||
// Lazily create a default backend so we don't tax users who don't care | ||
// about Ingress by consuming 1 of their 3 GCE BackendServices. This | ||
// BackendService is deleted when there are no more Ingresses, either | ||
// through Sync or Shutdown. | ||
if l.glbcDefaultBackend == nil { | ||
err := l.defaultBackendPool.Add(l.defaultBackendNodePort) | ||
if err != nil { | ||
return nil, err | ||
} | ||
l.glbcDefaultBackend, err = l.defaultBackendPool.Get(l.defaultBackendNodePort) | ||
if err != nil { | ||
return nil, err | ||
} | ||
glog.Warningf("Creating l7 without a default backend") | ||
} | ||
return &L7{ | ||
runtimeInfo: ri, | ||
|
@@ -175,24 +164,24 @@ func (l *L7s) Delete(name string) error { | |
func (l *L7s) Sync(lbs []*L7RuntimeInfo) error { | ||
glog.V(3).Infof("Creating loadbalancers %+v", lbs) | ||
|
||
// The default backend is completely managed by the l7 pool. | ||
// This includes recreating it if it's deleted, or fixing broken links. | ||
if err := l.defaultBackendPool.Add(l.defaultBackendNodePort); err != nil { | ||
return err | ||
} | ||
// create new loadbalancers, perform an edge hop for existing | ||
for _, ri := range lbs { | ||
if err := l.Add(ri); err != nil { | ||
if len(lbs) != 0 { | ||
// Lazily create a default backend so we don't tax users who don't care | ||
// about Ingress by consuming 1 of their 3 GCE BackendServices. This | ||
// BackendService is GC'd when there are no more Ingresses. | ||
if err := l.defaultBackendPool.Add(l.defaultBackendNodePort); err != nil { | ||
return err | ||
} | ||
defaultBackend, err := l.defaultBackendPool.Get(l.defaultBackendNodePort) | ||
if err != nil { | ||
return err | ||
} | ||
l.glbcDefaultBackend = defaultBackend | ||
} | ||
// Tear down the default backend when there are no more loadbalancers | ||
// because the cluster could go down anytime and we'd leak it otherwise. | ||
if len(lbs) == 0 { | ||
if err := l.defaultBackendPool.Delete(l.defaultBackendNodePort); err != nil { | ||
// create new loadbalancers, validate existing | ||
for _, ri := range lbs { | ||
if err := l.Add(ri); err != nil { | ||
return err | ||
} | ||
l.glbcDefaultBackend = nil | ||
} | ||
return nil | ||
} | ||
|
@@ -215,6 +204,15 @@ func (l *L7s) GC(names []string) error { | |
return err | ||
} | ||
} | ||
// Tear down the default backend when there are no more loadbalancers. | ||
// This needs to happen after we've deleted all url-maps that might be | ||
// using it. | ||
if len(names) == 0 { | ||
if err := l.defaultBackendPool.Delete(l.defaultBackendNodePort); err != nil { | ||
return err | ||
} | ||
l.glbcDefaultBackend = nil | ||
} | ||
return nil | ||
} | ||
|
||
|
@@ -586,7 +584,7 @@ func (l *L7) edgeHop() error { | |
} | ||
} | ||
if l.runtimeInfo.TLS != nil { | ||
glog.V(3).Infof("Edge hopping https for %v", l.Name) | ||
glog.V(3).Infof("validating https for %v", l.Name) | ||
if err := l.edgeHopHttps(); err != nil { | ||
return err | ||
} | ||
|
@@ -696,7 +694,7 @@ func (l *L7) UpdateUrlMap(ingressRules utils.GCEURLMap) error { | |
} else { | ||
l.um.DefaultService = l.glbcDefaultBackend.SelfLink | ||
} | ||
glog.V(3).Infof("Updating url map %+v", ingressRules) | ||
glog.Infof("Updating url map:\n%+v", ingressRules) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The default level is 0 right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. V(2) is default |
||
|
||
// Every update replaces the entire urlmap. | ||
// TODO: when we have multiple loadbalancers point to a single gce url map | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I noticed this before when I played with Ingress. Will this incur a lag for the default backend?
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.
we were always creating it as part of sync: https://github.com/kubernetes/contrib/blob/master/ingress/controllers/gce/loadbalancers/loadbalancers.go#L180, so this was just a redundant check. But we were also deleting it as part of sync, when there are no more lbs: https://github.com/kubernetes/contrib/blob/master/ingress/controllers/gce/loadbalancers/loadbalancers.go#L191
That's not the right time to try and delete it, because a url-map could still be referencing it (the delete would fail saying: "backend already in use", then we'd requeue, come back in, delete the url map, and now the backend delete would pass).
So I moved the delete to GC(), which happens after url map deletion.
https://github.com/kubernetes/ingress/pull/21/files#diff-247e0d4a5e00d30d32c0e454986614cdR210