Add Sidecar MaxConn support without other functionality changes #8

mihaitodor · 2017-04-27T23:56:01Z

Fix resource leak (missing resp.Body.Close() in Sidecar.fetchState())
Add maxconn functionality
- allow the user to configure MaxConns via the TOML settings file
- extend TestSidecarMakeBackends to test this new functionality
Restructure tests and remove useless repetition (couldn't help myself, sorry)
- use custom http.Client instances which we can reset to
  http.DefaultClient in the unit tests, because httpmock can't hook
  into (multiple) non-default HTTP clients and we need to have two
  of them (one for the watcher endpoint and one for fetching
  state.json)
- adjust TestSidecarWatcher to actually test that
  loadSidecarConfig is being invoked when adding new service entries
  to the mock Sidecar watcher response
Fix linter suggestions

Update 09.05.2017:

Used a pool to keep track of all goroutines that are spawned by the Sidecar provider. The functionality from safe/routine.go allows us to keep track and stop all child goroutines.
Remove the "Frontend" field from the Sidecar provider and use the default "Filename" field from the embedded BaseProvider struct.
Added MaxConn as an optional field to the frontends defined in the configuration TOML.

TODO in separate PRs:

use the IP address for the port from the Sidecar payload as discussed here (see Use service server IPs from Sidecar #11)
refactor tr.CancelRequest(req) as agreed here (see Refactor deprecated Transport.CancelRequest method #12)

(cc @bparli, @relistan)

relistan · 2017-04-28T11:48:54Z

configuration.go

@@ -334,6 +334,7 @@ func NewTraefikDefaultPointersConfiguration() *TraefikConfiguration {
 	defaultSidecar.Endpoint = "http://127.0.0.1:7777"
 	defaultSidecar.Frontend = "sidecar.toml"
 	defaultSidecar.RefreshConn = flaeg.Duration(60 * time.Second)
+	defaultSidecar.MaxConns = 300


This is a super low default. HAproxy defaults to 2000 for example:

Also, when is set to large values, it is possible that the servers
are not sized to accept such loads, and for this reason it is generally wise
to assign them some reasonable connection limits.

By default, this value is set to 2000.

relistan · 2017-04-28T11:50:59Z

provider/sidecar.go

-	}
-	return nil
+
+	return provider.loadSidecarConfig(states.ByService())


As mentioned in #7, I'd just pass states here and loop over them with states.EachService() later. Saves allocation for a biggish map and lots less CPU time.

This might not work out easily, because the Sidecar watch endpoint returns state.ByService(), which is a map[string][]*service.Service.

Given this, you mentioned submitting a PR to Sidecar to add a parameter to this endpoint that allows the consumer to specify whether they want to get the state "ByService" or not. I'll proceed to make the required changes in Sidecar for this to work.

@relistan Changes pushed to address this... Hopefully, it will fit nicely with the Sidecar watcher additions :)

relistan · 2017-04-28T11:51:35Z

provider/sidecar.go

 		safe.Go(func() { catalog.DecodeStream(resp.Body, provider.callbackLoader) })

 		//wait on refresh connection timer.  If this expires we haven't seen an update in a
 		//while and should cancel the request, reset the time, and reconnect just in case
 		<-provider.connTimer.C
 		provider.connTimer.Reset(time.Duration(provider.RefreshConn))
-		tr.CancelRequest(req)
+
+		//TODO: Deprecated method. Refactor this to use a context.


relistan · 2017-04-28T11:53:38Z

provider/sidecar_test.go

+}
+
+func TestSidecarFetchState(t *testing.T) {
+	setup()


This is working against the grain for goconvey. Have a read here: https://github.com/smartystreets/goconvey/wiki/Execution-order

relistan · 2017-04-28T11:55:29Z

provider/sidecar_test.go

-		configMsg, _ := <-configurationChan
+
+		// We have no way to shut down the provider when it's running in watch mode,
+		// so just let the unit test close it at the end


That's a smell. There are solutions. This is one: https://github.com/relistan/go-director

@relistan I have pushed 4635181 to address this comment. Please let me know if that change fully addresses this code smell.

relistan · 2017-04-28T11:57:24Z

provider/sidecar.go

+	method               = "wrr"
+	weight               = 0
+	sticky               = false
+	maxConnExtractorFunc = "request.host"


I'm struggling to see where this is used other than in New and tests

That's just there until we finally agree to have this setting configurable via TOML along with the actual limit. In our case "request.host" makes sense, but other people might want to change it. It's used here and it can be set to "client.ip" or "request.host" or "request.header.<header_name>". I think it's best if we allow users to change it via the config file.

If you guys really think we need this level of control with the max connections per backend (as opposed to one global setting), then I agree with Mihai's implementation. Simplest would be to expose it in the toml config file.

relistan · 2017-04-28T11:58:02Z

Some comments inline above. But generally 👍

bparli · 2017-04-28T21:01:46Z

provider/sidecar.go

+
+	sidecarHTTPClient = &http.Client{
+		Timeout: 5 * time.Second,
+	}


👍 this is much cleaner

bparli · 2017-04-28T21:04:45Z

Just one comment from me re: the max connections setting, but I'll defer to you guys on that point. Thanks for doing this @mihaitodor 👍

mihaitodor · 2017-05-09T14:07:55Z

@relistan @bparli I have managed to add the MaxConn field as optional on the frontends defined in the TOML via commit c06c529. Please note that it will default to Amount: 2000 / ExtractorFunc: "request.host" if no MaxConn field is defined for a specific frontend in the TOML file.

This approach was discussed here and I assumed silent approval :)

- restructure tests and remove useless repetition - use custom http.Client instances which we can reset to http.DefaultClient in the unit tests, because httpmock can't hook into (multiple) non-default HTTP clients and we need to have two of them (one for the watcher endpoint and one for fetching state.json) - adjust TestSidecarWatcher to actually test that loadSidecarConfig is being invoked when adding new service entries to the mock Sidecar watcher response

- Allow the user to configure MaxConns via the TOML settings file - Extend TestSidecarMakeBackends to test this new functionality

Discussion: #8 (comment)

I decided to not add a dependency on go-director, since Traefik already has similar functionality builtin (safe/routine.go). Discussion: #7 (comment) #8 (comment)

Remove the "Frontend" field from the Sidecar provider and use the default "Filename" field from the embedded BaseProvider struct. Note: this will require updating the configuration files in erector. Discussion: https://github.com/Nitro/traefik/pull/7/files#r113908566

decodeStream should terminate when the input stream is closed. Discussion: #8 (comment)

- This commit enables adding a MaxConn field on the frontends defined for Sidecar via the TOML specified in the Filename field. - Design discussed here: #7 (comment) and here: #8 (comment)

bparli · 2017-05-15T20:22:53Z

provider/testdata/sidecar_testdata.toml

        [frontends.web.routes.test_1]
-        rule = "Host: some-aws-host"
+            rule = "Host: some-aws-host"
+        [frontends.web.maxconn]


Should there be another test without the frontends.web.maxconn to verify the default Maxconn setting?

It's tested for the other backend defined in this file. See here.

I was considering to create a separate TOML for that specific case, but I think it wouldn't add much value.

mihaitodor mentioned this pull request Apr 28, 2017

Add Sidecar maxconn support - DO NOT MERGE #7

Closed

4 tasks

relistan reviewed Apr 28, 2017

View reviewed changes

bparli reviewed Apr 28, 2017

View reviewed changes

provider/sidecar.go

sidecarHTTPClient = &http.Client{

Timeout: 5 * time.Second,

}

Copy link

bparli Apr 28, 2017

Choose a reason for hiding this comment

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

👍 this is much cleaner

mihaitodor force-pushed the add-sidecar-maxconn-support-no-extras branch from 2b7a4cc to 138c9ff Compare May 9, 2017 11:35

mihaitodor added 13 commits May 9, 2017 16:10

Fix resource leak

d9ecea8

Fix Sidecar test naming scheme

c5487bb

Add maxconn functionality

ade3531

- Allow the user to configure MaxConns via the TOML settings file - Extend TestSidecarMakeBackends to test this new functionality

Fix linter suggestions

4afc4dd

Change MaxConns default value to match HAProxy

4227eb9

Get the entire state from the Sidecar watcher

70f924d

Discussion: #8 (comment)

Do proper setup/teardown with Convey

5827a15

Use the recommended version of httpmock

8f5c465

Use a pool for every goroutine

027ce69

I decided to not add a dependency on go-director, since Traefik already has similar functionality builtin (safe/routine.go). Discussion: #7 (comment) #8 (comment)

Revert using the goroutine pool for decodeStream

dbdbb90

decodeStream should terminate when the input stream is closed. Discussion: #8 (comment)

Add MaxConn field for the frontends config TOML

726472c

- This commit enables adding a MaxConn field on the frontends defined for Sidecar via the TOML specified in the Filename field. - Design discussed here: #7 (comment) and here: #8 (comment)

mihaitodor force-pushed the add-sidecar-maxconn-support-no-extras branch from c06c529 to 726472c Compare May 9, 2017 15:11

This was referenced May 12, 2017

Add backend conn stats JSON REST API and UI support #10

Merged

Use service server IPs from Sidecar #11

Merged

Refactor deprecated Transport.CancelRequest method #12

Open

bparli reviewed May 15, 2017

View reviewed changes

bparli approved these changes May 15, 2017

View reviewed changes

mihaitodor merged commit 9280d68 into master May 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Sidecar MaxConn support without other functionality changes #8

Add Sidecar MaxConn support without other functionality changes #8

mihaitodor commented Apr 27, 2017 •

edited

Loading

relistan Apr 28, 2017

relistan Apr 28, 2017

mihaitodor Apr 28, 2017

mihaitodor May 3, 2017

relistan Apr 28, 2017

relistan Apr 28, 2017

relistan Apr 28, 2017

mihaitodor May 9, 2017

relistan Apr 28, 2017

mihaitodor Apr 28, 2017

bparli Apr 28, 2017

relistan commented Apr 28, 2017

bparli Apr 28, 2017

bparli commented Apr 28, 2017

mihaitodor commented May 9, 2017

bparli May 15, 2017

mihaitodor May 15, 2017

Add Sidecar MaxConn support without other functionality changes #8

Add Sidecar MaxConn support without other functionality changes #8

Conversation

mihaitodor commented Apr 27, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

relistan commented Apr 28, 2017

Choose a reason for hiding this comment

bparli commented Apr 28, 2017

mihaitodor commented May 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihaitodor commented Apr 27, 2017 •

edited

Loading