Issue #548 added enable/disable glob matching #550

galen0624 · 2018-09-17T18:48:46Z

Updated config files and table.go to enable/disable glob matching. Default is enabled. All the go tests have been updated to support enabled matching not disabled matching.

galen0624 · 2018-09-17T18:50:46Z

attached is the make test for this.
PR-550.txt

magiconair · 2018-09-19T17:31:09Z

config/default.go

@@ -77,4 +77,5 @@ var defaultConfig = &Config{
 		Color:  "light-green",
 		Access: "rw",
 	},
+	GlobMatching: true,


Since true is the default case but not the zero value of bool I'd prefer DisableGlobMatching since then most code and tests will not have to change.

This makes sense I'll update. The tests will still have to be updated to support passing config.Config into the Lookup func in main.go

Config: cfg.Proxy, Transport: newTransport(nil), InsecureTransport: newTransport(&tls.Config{InsecureSkipVerify: true}), Lookup: func(r *http.Request) *route.Target { t := route.GetTable().Lookup(r, r.Header.Get("trace"), pick, match, cfg) --SNIP

Can't you just use a different Lookup function?

Not passing the config.Config down into the route package would require doubling up the public Lookup func as well as the matchingHosts func as that is where the glob is compiled. If we keep the config being passed in we can apply the logic directly in the matchingHosts. This would keep a single Lookup func and a single matchingHosts func. Also we could add in some additional tests/bench_marks to test with/without glob matching. That would also help with future comparisons of compiled globs/non-compiled globs/non-globs.

I could also just set a local var and pass that along to the funcs to apply the logic but that would still require updated tests.

Thoughts?

Why not just pass a bool and default to false or also accept a nil config to simplify the tests?

fabio.properties

magiconair · 2018-09-19T17:33:45Z

route/table.go

@@ -295,13 +296,32 @@ func normalizeHost(host string, tls bool) string {

 // matchingHosts returns all keys (host name patterns) from the
 // routing table which match the normalized request hostname.
-func (t Table) matchingHosts(req *http.Request) (hosts []string) {
+func (t Table) matchingHosts(req *http.Request, cfg *config.Config) (hosts []string) {


I'd prefer if you factor this out into two separate functions. Also, I think you can cache the compiled globs if you limit the size of the cache and evict older entries. The following code should do the trick. Maybe you can add another option to configure the size of the glob cache. Set the default to 1000.

package route import ( "sync" "github.com/gobwas/glob" ) // GlobCache implements an LRU cache for compiled glob patterns. type GlobCache struct { mu sync.RWMutex // m maps patterns to compiled glob matchers. m map[string]glob.Glob // l contains the added patterns and serves as an LRU cache. // l has a fixed size and is initialized in the constructor. l []string // h is the first element in l. h int // n is the number of elements in l. n int } func NewGlobCache(size int) *GlobCache { return &GlobCache{ m: make(map[string]glob.Glob, size), l: make([]string, size), } } // Get returns the compiled glob pattern if it compiled without // error. Otherwise, the function returns nil. If the pattern // is not in the cache it will be added. func (c *GlobCache) Get(pattern string) glob.Glob { // fast path with read lock c.mu.RLock() g := c.m[pattern] c.mu.RUnlock() if g != nil { return g } // slow path with write lock c.mu.Lock() defer c.mu.Unlock() // check again to handle race condition g = c.m[pattern] if g != nil { return g } // try to compile pattern // todo(fs): can this fail and should we return err? g, err := glob.Compile(pattern) if err != nil { return nil } // if the LRU buffer is not full just append // the element to the buffer. if c.n < len(c.l) { c.m[pattern] = g c.l[c.n] = pattern c.n++ return g } // otherwise, remove the oldest element and move // the head. Note that once the buffer is full // (c.n == len(c.l)) it will never become smaller // again. delete(c.m, c.l[c.h]) c.m[pattern] = g c.l[c.h] = pattern c.h = (c.h + 1) % c.n return g }

package route import ( "reflect" "sort" "testing" ) func TestGlobCache(t *testing.T) { c := NewGlobCache(3) keys := func() []string { var kk []string for k := range c.m { kk = append(kk, k) } sort.Strings(kk) return kk } c.Get("a") if got, want := len(c.m), 1; got != want { t.Fatalf("got len %d want %d", got, want) } if got, want := keys(), []string{"a"}; !reflect.DeepEqual(got, want) { t.Fatalf("got %v want %v", got, want) } if got, want := c.l, []string{"a", "", ""}; !reflect.DeepEqual(got, want) { t.Fatalf("got %v want %v", got, want) } c.Get("b") if got, want := len(c.m), 2; got != want { t.Fatalf("got len %d want %d", got, want) } if got, want := keys(), []string{"a", "b"}; !reflect.DeepEqual(got, want) { t.Fatalf("got %v want %v", got, want) } if got, want := c.l, []string{"a", "b", ""}; !reflect.DeepEqual(got, want) { t.Fatalf("got %v want %v", got, want) } c.Get("c") if got, want := len(c.m), 3; got != want { t.Fatalf("got len %d want %d", got, want) } if got, want := keys(), []string{"a", "b", "c"}; !reflect.DeepEqual(got, want) { t.Fatalf("got %v want %v", got, want) } if got, want := c.l, []string{"a", "b", "c"}; !reflect.DeepEqual(got, want) { t.Fatalf("got %v want %v", got, want) } c.Get("d") if got, want := len(c.m), 3; got != want { t.Fatalf("got len %d want %d", got, want) } if got, want := keys(), []string{"b", "c", "d"}; !reflect.DeepEqual(got, want) { t.Fatalf("got %v want %v", got, want) } if got, want := c.l, []string{"d", "b", "c"}; !reflect.DeepEqual(got, want) { t.Fatalf("got %v want %v", got, want) } }

We are not looking to use Globs at all in the short term but we can spend some time testing this code and adding to a future PR to hedge against our requirements changing in the future.

I understand that. I'd appreciate it if you could embed this code to the glob path after splitting the function and run some tests.

Would it make more sense to add/delete the compiled glob at route add/route del?

Not sure what kind of performance impact that would have. Especially in our kind of ENV where we have with 1500+ services all adding/deleting routes constantly.

Since this is looping over the keys of t you could do this before the table is switched. However, my guess is that even with 1500 services your external hostnames are a bit more stable and you should have fewer.

My gut feel says that the LRU cache is probably easier to handle here.

galen0624 · 2018-09-20T15:01:18Z

I added a separate func with the latest commit for the globMatching disabled. Below is the recent load test we performed.
8Core, 8G VM, 4K TPS for each test. The first test was with glob.matching.disabled = false. We only could get to about 1200TPS and 90% CPU usage. The second test glob.matching.disabled = true shows the full 4K TPS and about 20% CPU usage.

aaronhurt · 2018-09-20T15:03:28Z

Very nice, thank you so much for the analysis and work.

magiconair · 2018-09-20T17:30:18Z

fabio.properties

@@ -743,6 +743,17 @@
 # registry.consul.checksRequired = one


+# glob.matching.disabled Disables glob matching on route lookups


s/Disables/disables/

magiconair · 2018-09-20T17:31:02Z

main.go

@@ -137,6 +137,7 @@ func main() {

 func newHTTPProxy(cfg *config.Config) http.Handler {
 	var w io.Writer
+	globDisabled := cfg.DisableGlobMatching


What's the purpose of the var?

Not needed. I'll update.

magiconair · 2018-09-20T17:34:11Z

proxy/http_integration_test.go

@@ -175,6 +175,8 @@ func TestProxyNoRouteStatus(t *testing.T) {
 }

 func TestProxyStripsPath(t *testing.T) {
+	//Glob Matching True


If you add this var to all tests you can also add some constants, e.g.

const ( // helper constants for the Lookup function globEnabled = false globDisabled = true )

magiconair · 2018-09-20T17:36:21Z

config/default.go

@@ -77,4 +77,5 @@ var defaultConfig = &Config{
 		Color:  "light-green",
 		Access: "rw",
 	},
+	DisableGlobMatching: false,


That's the default so you can remove it.

magiconair

Some small nitpicks. Other than that it looks ok.

galen0624 · 2018-09-20T17:49:01Z

Sounds good. I'll update and commit this afternoon. Also I have started testing the Glob Cache and we are running into performance issues. I would like to collaborate under a different branch/issue if you don't mind.

magiconair · 2018-09-20T17:50:54Z

That's fine. Lets get this working first so that this regression is fixed. That's more important.

magiconair · 2018-09-20T20:00:10Z

I've merged it and changed DisableGlobMatching to GlobMatchingDisabled. Didn't want you to do another roundtrip.

Thanks for your patience!

Issue fabiolb#548 added enable/disable glob matching

062a8ea

galen0624 added 2 commits September 19, 2018 11:24

updated docs for glob.matching.enabled

054be87

Updated Docs for glob.matching.enabled

eda6cf4

magiconair suggested changes Sep 19, 2018

View reviewed changes

galen0624 added 2 commits September 19, 2018 16:40

updated to pass in bool instead of config.Config to route package

ea75fb8

added separate func for no glob matching

1eb2e36

magiconair reviewed Sep 20, 2018

View reviewed changes

galen0624 added 2 commits September 20, 2018 14:43

updated test with CONST, var clean up

da67268

go fmt of files

8920153

magiconair approved these changes Sep 20, 2018

View reviewed changes

magiconair merged commit d9b3cde into fabiolb:master Sep 20, 2018

magiconair added this to the 1.5.10 milestone Sep 20, 2018

magiconair mentioned this pull request Sep 20, 2018

Performance issue - Glob matching with large number of services in consul #548

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #548 added enable/disable glob matching #550

Issue #548 added enable/disable glob matching #550

galen0624 commented Sep 17, 2018

galen0624 commented Sep 17, 2018

magiconair Sep 19, 2018

galen0624 Sep 19, 2018 •

edited

Loading

magiconair Sep 19, 2018

galen0624 Sep 19, 2018 •

edited

Loading

magiconair Sep 19, 2018

magiconair Sep 19, 2018

galen0624 Sep 19, 2018

magiconair Sep 19, 2018

galen0624 Sep 19, 2018 •

edited

Loading

magiconair Sep 19, 2018

magiconair Sep 19, 2018

galen0624 commented Sep 20, 2018 •

edited

Loading

aaronhurt commented Sep 20, 2018

magiconair Sep 20, 2018

magiconair Sep 20, 2018

galen0624 Sep 20, 2018

magiconair Sep 20, 2018

magiconair Sep 20, 2018

magiconair left a comment

galen0624 commented Sep 20, 2018

magiconair commented Sep 20, 2018

magiconair commented Sep 20, 2018

		@@ -743,6 +743,17 @@
		# registry.consul.checksRequired = one


		# glob.matching.disabled Disables glob matching on route lookups

Issue #548 added enable/disable glob matching #550

Issue #548 added enable/disable glob matching #550

Conversation

galen0624 commented Sep 17, 2018

galen0624 commented Sep 17, 2018

Choose a reason for hiding this comment

galen0624 Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galen0624 Sep 19, 2018 • 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

galen0624 Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galen0624 commented Sep 20, 2018 • edited Loading

aaronhurt commented Sep 20, 2018

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

magiconair left a comment

Choose a reason for hiding this comment

galen0624 commented Sep 20, 2018

magiconair commented Sep 20, 2018

magiconair commented Sep 20, 2018

galen0624 Sep 19, 2018 •

edited

Loading

galen0624 Sep 19, 2018 •

edited

Loading

galen0624 Sep 19, 2018 •

edited

Loading

galen0624 commented Sep 20, 2018 •

edited

Loading