Circuit breaker #362

aryszka · 2017-07-03T10:24:09Z

OP:

In the simplest case, the proxy gets a breaker for the current backend host from the registry, if configured. The breakers are always bound to a backend host, even if they share their settings. There are two breaker types, one opens after N consecutive failures, the other opens when the failure rate crosses a threshold within a sliding window (numeric window, not time based). The idle breakers are automatically dropped from the registry when new breakers need to be created, e.g. when some backend hosts were removed from the configuration but new ones were added.

CONFIG:

it is possible to define global breaker settings (common settings for each backend host breaker)
it is possible to override or disable the global settings for each host individually
it is possible to override the global or host based settings for each route individually, using standard filters

DOCS:
https://github.com/zalando/skipper/blob/circuit-breaker/circuit/doc.go

DeTeam

Few notes on docs.

DeTeam · 2017-07-10T14:45:09Z

circuit/doc.go

+The Host field indicates to which backend host should the current set of settings be applied. Leaving it empty
+indicates global settings.
+
+Command line name: host.


Is glob supported (*)?

* The answer is no, but mb it's a featue worth considering..

how would it work? currently if the host is not set, that means global settings (still with individual breaker instances for the hosts)

DeTeam · 2017-07-10T14:45:43Z

circuit/doc.go

+
+The window value sets the size of the sliding counter window of the failure rate breaker.
+
+Command line name: window. Possible command line values: any positive integer.


Are there any defaults worth mentioning here?

there are.. but currently they are used from sony/gobreaker and may be replaced in a later wave with https://github.com/ideahitme/circuit-breaker, so considered not declaring the defaults in the docs atm. Basically, 5 consecutive failures, 1 minute timeout, 1 half-open request. The default IdleTTL is documented.

DeTeam · 2017-07-10T14:45:50Z

circuit/doc.go

+
+The failures value sets the max failure count for both the "consecutive" and "rate" breakers.
+
+Command line name: failures. Possible command line values: any positive integer.


Are there any defaults worth mentioning here?

DeTeam · 2017-07-10T16:04:59Z

Related issues in gobreaker:

aryszka · 2017-07-10T16:05:02Z

sorry, didn't mean to delete those comments 🤕

DeTeam · 2017-07-10T16:12:58Z

👍

mo-gr · 2017-07-11T08:32:19Z

circuit/list.go

+// It can return the first consecutive items that match a
+// condition (in our case, the ones that were inactive for a
+// while).
+type list struct {


I think this is the first time ever since university, that I see a linked list implementation :-)

Anyway. Why not just use an array? This seems like premature optimisation and just adds fuckup-surface. Please consider just using an array. Yes, it might be slower, no it won't matter

i also been waiting long for the use case ;)

i think it is simply optimization, but not premature, since it is already in the PR. Using an array would be like not using anything, because then we could also just iterate over the timestamps to see which breaker is idle. The reason for the optimization here is that this is in a synchronized scope, happening on every request, and blocking every concurrent request. Why do you think that it won't matter?

now that thinking about it.. maybe there is a way to bind the breakers to each route. Will play with that as a separate effort.

mo-gr · 2017-07-11T08:41:58Z

proxy/proxy.go

+	}
+
+	settings, _ := c.stateBag[circuitfilters.RouteSettingsKey].(circuit.BreakerSettings)
+	settings.Host = c.outgoingHost


Can I set breakers for <shunt> or <loopback>? Is that desirable behaviour? Or should we check and early escape in those cases?

we cannot set breakers for <shunt> and <loopback> . I think the "shunt", by its name, already breaks the circuit by itself. For the loopback, not sure, but i believe it is enough to see where it loops to.

mo-gr · 2017-07-11T08:43:46Z

👍

mo-gr · 2017-07-11T08:44:38Z

A general question, maybe more like a feature request: If a circuit breaker flips open, is it possible to purge the DNS cache for that host? Seems like a nice-to-have feature...

mo-gr · 2017-07-11T08:47:19Z

proxy/proxy.go

@@ -515,13 +547,26 @@ func (p *Proxy) do(ctx *context) error {
 		ctx.outgoingDebugRequest = debugReq
 		ctx.setResponse(&http.Response{Header: make(http.Header)}, p.flags.PreserveOriginal())
 	} else {
+		done, allow := p.checkBreaker(ctx)
+		if !allow {
+			return errCircuitBreakerOpen


Do I read this correct, that we will only log that a circuit breaker is open, but not which one? If a breaker flips open (or more general flips state), I would like to see a log line stating clearly, which breaker for which host goes from which old state into which new state.

Is this not there or am I missing something?

valid point. It should be there.

Besides the logs, the plan is to add an endpoint to the "metrics" -> "support" listener that reports all the breaker states. After #364 gets finished.

aryszka · 2017-07-11T08:53:33Z

purging the DNS cache. Will look into it.

mo-gr · 2017-07-11T16:09:15Z

👍

aryszka · 2017-07-13T13:37:08Z

👍

DeTeam · 2017-07-13T13:41:10Z

👍

aryszka added 10 commits July 2, 2017 23:32

circuit breaker: consecutive and experimental rate breaker

b3b6f49

route specific breaker settings

3cbe423

WIP notes

403270a

Merge branch 'master' into circuit-breaker

8ea54e0

refactor the circuit breaker package

0dea759

optional individual ttl for each breaker

5cbca8a

test host and route specific settings

b3571a2

breaker configuration from command line

60ce060

add documentation

427bd2e

docs reviewed

fe2f786

mo-gr requested review from evangelion1204 and removed request for evangelion1204 July 10, 2017 11:37

DeTeam reviewed Jul 10, 2017

View reviewed changes

zalando deleted a comment from DeTeam Jul 10, 2017

mo-gr reviewed Jul 11, 2017

View reviewed changes

mo-gr approved these changes Jul 11, 2017

View reviewed changes

mo-gr reviewed Jul 11, 2017

View reviewed changes

aryszka added 2 commits July 11, 2017 17:18

remove sorted access list

a451c1a

log open and close events

761d569

drop unused fields

9da7acf

aryszka merged commit a6f2131 into master Jul 13, 2017

aryszka deleted the circuit-breaker branch July 13, 2017 13:41

szuecs mentioned this pull request Jul 17, 2017

add dependency which was not added in the PR #371

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Circuit breaker #362

Circuit breaker #362

aryszka commented Jul 3, 2017 •

edited

Loading

DeTeam left a comment

DeTeam Jul 10, 2017

DeTeam Jul 10, 2017

aryszka Jul 10, 2017

DeTeam Jul 10, 2017

aryszka Jul 10, 2017

DeTeam Jul 10, 2017

aryszka Jul 10, 2017

DeTeam commented Jul 10, 2017

aryszka commented Jul 10, 2017

DeTeam commented Jul 10, 2017

mo-gr Jul 11, 2017

aryszka Jul 11, 2017

aryszka Jul 11, 2017

mo-gr Jul 11, 2017

aryszka Jul 11, 2017 •

edited

Loading

mo-gr commented Jul 11, 2017

mo-gr commented Jul 11, 2017

mo-gr Jul 11, 2017

aryszka Jul 11, 2017

aryszka commented Jul 11, 2017

mo-gr commented Jul 11, 2017

aryszka commented Jul 13, 2017

DeTeam commented Jul 13, 2017


		The window value sets the size of the sliding counter window of the failure rate breaker.

		Command line name: window. Possible command line values: any positive integer.


		The failures value sets the max failure count for both the "consecutive" and "rate" breakers.

		Command line name: failures. Possible command line values: any positive integer.

Circuit breaker #362

Circuit breaker #362

Conversation

aryszka commented Jul 3, 2017 • edited Loading

DeTeam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DeTeam commented Jul 10, 2017

aryszka commented Jul 10, 2017

DeTeam commented Jul 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aryszka Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

mo-gr commented Jul 11, 2017

mo-gr commented Jul 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aryszka commented Jul 11, 2017

mo-gr commented Jul 11, 2017

aryszka commented Jul 13, 2017

DeTeam commented Jul 13, 2017

aryszka commented Jul 3, 2017 •

edited

Loading

aryszka Jul 11, 2017 •

edited

Loading