Wrong calculation for ServerNameHashMaxSize?

[ankon]

https://github.com/kubernetes/ingress/blob/master/controllers/nginx/pkg/cmd/controller/nginx.go#L328 tries to auto-size the hash for the server names, and it uses the total number of bytes for all server names to determine ServerNameHashMaxSize.

But, according to http://nginx.org/en/docs/http/server_names.html#optimization that seems wrong, and instead the number of server names should be considered:

If a large number of server names are defined [...] try to set server_names_hash_max_size to a number close to the number of server names.

Am I missing something here?

[aledbf]

@ankon this is the length of the key in the hash tables not the length of the table.
The calculation is here

[ankon]

	var serverNames int
	for _, srv := range ingressCfg.Servers {
		serverNames += len([]byte(srv.Hostname))
		if longestName < len(srv.Hostname) {
			longestName = len(srv.Hostname)
		}
	}

At the end we have serverNames as sum of the (byte) lengths of the Hostnames, right?

	serverNameHashMaxSize := nextPowerOf2(serverNames)
	if serverNameHashMaxSize > cfg.ServerNameHashMaxSize {
		glog.V(3).Infof("adjusting ServerNameHashMaxSize variable from %v to %v",
			cfg.ServerNameHashMaxSize, serverNameHashMaxSize)
		cfg.ServerNameHashMaxSize = serverNameHashMaxSize
	}

So here we end up with serverNameHashMaxSize being the first power of 2 > serverNames -- which was a sum, not a number/count. The documentation I quoted seems to indicate that server_names_hash_max_size should be calculated by counting the server names, and server_names_hash_bucket_size should be calculated as "longer than the length of the longest name" (which is what the calculation for serverNameHashBucketSize does based on longestName).

So this calculation seems to "overestimate" the needed space in the table.

[aledbf]

So this calculation seems to "overestimate" the needed space in the table.

Right. What alternative you propose?

If we leave this as a parameter (until 0.8.3) every time you add a new ingress you could end with a non working nginx configuration. Honestly I prefer overestimate the size of the table.

[ankon]

Being able to configure things is definitely helpful ... in fact the reason I started to look at this code in the first place is that I hit the other limit and didn't know there is a way to configure it :)

Rough idea would be this though:

diff --git a/controllers/nginx/pkg/cmd/controller/nginx.go b/controllers/nginx/pkg/cmd/controller/nginx.go
index a57df8d..be30b67 100644
--- a/controllers/nginx/pkg/cmd/controller/nginx.go
+++ b/controllers/nginx/pkg/cmd/controller/nginx.go
@@ -327,9 +327,7 @@ func (n *NGINXController) SetListers(lister ingress.StoreLister) {
 // if an error is returned means requeue the update
 func (n *NGINXController) OnUpdate(ingressCfg ingress.Configuration) ([]byte, error) {
 	var longestName int
-	var serverNames int
 	for _, srv := range ingressCfg.Servers {
-		serverNames += len([]byte(srv.Hostname))
 		if longestName < len(srv.Hostname) {
 			longestName = len(srv.Hostname)
 		}
@@ -358,7 +356,7 @@ func (n *NGINXController) OnUpdate(ingressCfg ingress.Configuration) ([]byte, er
 			cfg.ServerNameHashBucketSize, nameHashBucketSize)
 		cfg.ServerNameHashBucketSize = nameHashBucketSize
 	}
-	serverNameHashMaxSize := nextPowerOf2(serverNames)
+	serverNameHashMaxSize := nextPowerOf2(len(ingressCfg.Servers))
 	if serverNameHashMaxSize > cfg.ServerNameHashMaxSize {
 		glog.V(3).Infof("adjusting ServerNameHashMaxSize variable from %v to %v",
 			cfg.ServerNameHashMaxSize, serverNameHashMaxSize)

[aledbf]

@ankon I think we can change the default ServerNameHashMaxSize to -1 and use the calculated serverNameHashMaxSize value or use the value you configured in the configmap

if cfg.ServerNameHashMaxSize == -1 {
  // use the serverNameHashMaxSize value
}

[aledbf]

@ankon friendly ping

[kylebyerly-hp]

With 0.9-beta.4 and 8 ingresses each doing 4 urls (of various length 26-47 chars) I start getting this error.
I agree with @aledbf on the over-estimation.

-------------------------------------------------------------------------------
Error: exit status 1
2017/04/14 19:07:51 [emerg] 43#43: could not build server_names_hash, you should increase server_names_hash_bucket_size: 64
nginx: [emerg] could not build server_names_hash, you should increase server_names_hash_bucket_size: 64
nginx: configuration file /tmp/nginx-cfg017454066 test failed

-------------------------------------------------------------------------------