Skip to content
This repository has been archived by the owner on Apr 17, 2019. It is now read-only.

Resilience fixes for the Ingress controller #652

Merged
merged 5 commits into from
Apr 4, 2016
Merged

Resilience fixes for the Ingress controller #652

merged 5 commits into from
Apr 4, 2016

Conversation

bprashanth
Copy link

Reasoning for each commit:

  1. UrlMap updating logic was too complicated, because it accounted for "joining" which we don't care about right now.
  2. Errors at the beginning of the sync() function would prevent cloud urlmaps from getting cleaned up
  3. The controller would silently forget about resources if it was restarted, and while down someone modified an Ingress
  4. Retry creating GCE client since it can fail if we don't resolve metadata server

@bprashanth bprashanth force-pushed the ing_resilience branch 2 times, most recently from 6b0366c to fc525de Compare March 29, 2016 18:52
freehan
freehan reviewed Apr 4, 2016
View reviewed changes
ingress/controllers/gce/controller/cluster_manager.go
// If this controller is scheduled on a node without compute/rw
// it won't be allowed to list backends. We can assume that the
// user has no need for Ingress in this case. If they grant
// permissions to the node they will have to restart the controller
Copy link
Contributor

Choose a reason for hiding this comment

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

FMOK, does user have to restart controller? why?

Copy link
Author

Choose a reason for hiding this comment

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

If I loop around here continuously re-creating the client till ListBackendServices succeedes, the liveness probe will fail at some point and cause a crashloop. I want to not crash if scheduled on a r/o node.

Another way to handle this is to keep re-creating the client object in the liveness probe, and if it suddenly starts succeeding ListbackendServices calls, somehow re-notify everyone I've given the old client to refresh their clients. Didn't think this level of effort was worth it, since we can just doc that the controller needs permissions and leave it at that.

@bprashanth
Copy link
Author

@freehan PTAL

@freehan
Copy link
Contributor

freehan commented Apr 4, 2016

LGTM, except the formatting nit...

@freehan freehan added the lgtm Indicates that a PR is ready to be merged. label Apr 4, 2016
@bprashanth
Copy link
Author

Fixed nit, inheriting lgtm and merging on green

@bprashanth bprashanth merged commit 693beab into kubernetes-retired:master Apr 4, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bprashanth @freehan @googlebot