Add check for per-host maximum connections #664

Merged
merged 3 commits into from Mar 14, 2016

Projects

None yet

5 participants

@jupiter
Collaborator
jupiter commented Mar 8, 2016

As per my comment #613 (comment), checking along with the other per-server checks makes for fewer code added, but with somewhat confusing method name.

The alternative is adding a Full() method on the host, with CheckFull() function on the upstream, and do do the Full() check in basically all the same places are where the Down() check is done.

@CLAassistant

CLA assistant check
All committers have accepted the CLA.

@abiosoft abiosoft and 1 other commented on an outdated diff Mar 8, 2016
middleware/proxy/upstream.go
@@ -78,6 +80,10 @@ func NewStaticUpstreams(c parse.Dispenser) ([]Upstream, error) {
ExtraHeaders: upstream.proxyHeaders,
CheckDown: func(upstream *staticUpstream) UpstreamHostDownFunc {
return func(uh *UpstreamHost) bool {
+ if upstream.MaxConns != 0 &&
+ uh.Conns == upstream.MaxConns {
@abiosoft
abiosoft Mar 8, 2016 Collaborator

Shouldn't make any difference but I kinda feel more comfortable with >=.

@jupiter
jupiter Mar 8, 2016 Collaborator

I agree, actually.

@abiosoft abiosoft commented on an outdated diff Mar 8, 2016
middleware/proxy/upstream.go
@@ -147,6 +153,15 @@ func parseBlock(c *parse.Dispenser, u *staticUpstream) error {
return err
}
u.MaxFails = int32(n)
+ case "max_conns":
+ if !c.NextArg() {
+ return c.ArgErr()
+ }
+ n, err := strconv.Atoi(c.Val())
+ if err != nil {
+ return err
+ }
+ u.MaxConns = int64(n)
@abiosoft
abiosoft Mar 8, 2016 Collaborator

since MaxConns is int64. I'd go with strconv.ParseInt(c.Val(), 10, 64) instead.

@jupiter Pieter Raubenheimer Add check for per-host maximum connections
ce8ee83
@jupiter
Collaborator
jupiter commented Mar 9, 2016

So shall we take this approach or do we add the connections check on the host with a similar inversion of control? Note that there are currently no tests that cover the CheckDown method, and we would probably need to break up the method to be able to write tests for it.

@mholt
Owner
mholt commented Mar 9, 2016

Hey @jupiter! Sorry, been swamped with some other work the last couple days. Give me a moment to look at it.

@mholt mholt added the under review label Mar 9, 2016
@mholt
Owner
mholt commented Mar 9, 2016

@jupiter This looks great. Perhaps we should consider Down and Available as separate ideas? In other words, instead of simply reporting a backend as "Unavailable" we could report it as "Down" or "Unavailable" -- do you think that distinction is valuable? A down upstream might need attention; an unavailable one is just busy.

jupiter added some commits Mar 10, 2016
@jupiter jupiter Add common method for checking host availability a7766c9
@jupiter jupiter Add test for UpstreamHost defaults
1f7d8d8
@jupiter
Collaborator
jupiter commented Mar 10, 2016

I've refactored the check into its own method Full() on UpstreamHost and added a method Available() for general use.

@mholt
Owner
mholt commented Mar 13, 2016

Hmm, a down host is technically unavailable. Do you want to rename them to Down and Full instead? If not, I can make the change after merging.

@jupiter
Collaborator
jupiter commented Mar 14, 2016

@mholt Not sure what you mean, but I think I did what you suggest. There are now 3 methods: Down(), Full(), and Available().

@mholt mholt removed the under review label Mar 14, 2016
@mholt
Owner
mholt commented Mar 14, 2016

This is awesome - thank you!

@mholt mholt merged commit b79ff74 into mholt:master Mar 14, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@@ -147,6 +160,15 @@ func parseBlock(c *parse.Dispenser, u *staticUpstream) error {
return err
}
u.MaxFails = int32(n)
+ case "max_conns":
@augustoroman
augustoroman Feb 14, 2017 Collaborator

@mholt This is an awesome feature but not mentioned in the docs: https://caddyserver.com/docs/proxy

How do are the docs updated?

@mholt
mholt Feb 14, 2017 Owner

@augustoroman Oops. Submit a PR to https://github.com/caddyserver/caddyserver.com - that would be great! :)

@augustoroman
augustoroman Feb 14, 2017 Collaborator

will do, thanks

@@ -57,6 +58,16 @@ func (uh *UpstreamHost) Down() bool {
return uh.CheckDown(uh)
}
+// Full checks whether the upstream host has reached its maximum connections
+func (uh *UpstreamHost) Full() bool {
+ return uh.MaxConns > 0 && uh.Conns >= uh.MaxConns
@augustoroman
augustoroman Feb 14, 2017 Collaborator

This should be using atomic to read the value.

@mholt
mholt Feb 14, 2017 Owner

Which value?

@augustoroman
augustoroman Feb 15, 2017 Collaborator

sorry, uh.Conns, since it's modified atomically as requests come and go in various goroutines. I'll submit a PR shortly.

@augustoroman
augustoroman Feb 15, 2017 Collaborator

That was trickier than I thought to get a good test written, but here you go: #1438

@augustoroman augustoroman added a commit to augustoroman/caddyserver.com that referenced this pull request Feb 14, 2017
@augustoroman augustoroman Docs (Proxy directive): add max_conns param
Adding docs for max_conns per mholt/caddy#664/
(see related issue mholt/caddy#613)

As far as I can tell, the 502 response code is what actually happens when Select() returns nil.
baf2942
@augustoroman augustoroman referenced this pull request in caddyserver/caddyserver.com Feb 14, 2017
Merged

Docs (Proxy directive): add max_conns param #112

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment