Added upstream zone directive annotation #629

vrrs · 2019-07-22T22:49:31Z

Proposed changes

These changes enable the zone directive on upstream block with upstream name equal to zone name and zone size equals to the annotation value. This follow the same convention as nginx-plus ingress config. For instance, nginx.org/upstream-zone-size: "32k" in your ingress.yaml will generate a config such as

upstream myService-ingress-prod-myService.production.ingress.net-myService-prod-80 {
   zone myService-ingress-prod-myService.production.ingress.net-myService-prod-80 32k;
   server 125.255.255.255:8888 max_fails=3 fail_timeout=10 max_conns=6;
}

If the annotation is not present then the zone directive will not be present on the config.

Nginx Documentation for upstream zone


Syntax:	zone name [size];
Default:	N/A
Context:	upstream

Motivation

In multi worker nginx setup(worker_processes > 1), the zone directive fixes the least_conn lb method as well as max_fails, max_conns parameters. From the nginx document on load balancing:

For example, if the configuration of a group is not shared, each worker process maintains its own counter for failed attempts to pass a request to a server (set by the max_fails parameter). In this case, each request gets to only one worker process. When the worker process that is selected to process a request fails to transmit the request to a server, other worker processes don’t know anything about it. While some worker process can consider a server unavailable, others might still send requests to this server. For a server to be definitively considered unavailable, the number of failed attempts during the timeframe set by the fail_timeout parameter must equal max_fails multiplied by the number of worker processes. On the other hand, the zone directive guarantees the expected behavior.
Similarly, the Least Connections load‑balancing method might not work as expected without the zone directive, at least under low load.

Use case

A properly working least_conn load balancer is very useful in balancing long lived connections and limiting load on the destination servers; particularly when upstream servers are streaming web apps.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works
I have checked that all unit tests pass after adding my changes
I have updated necessary documentation
I have rebased my branch onto master
I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

pleshakov · 2019-07-23T11:22:38Z

@vrrs thanks for the PR! We'll review it shortly

pleshakov · 2019-07-24T15:37:34Z

@vrrs
Thanks for bringing this PR! Having zones in the config for enabling passive health checks and the least conn algorithm to work across multiple workers for NGINX OSS makes sense. We should've had it implemented before.

I wonder though if it makes sense to have a configmap key upstream-zone-size instead of the suggestion annotation. It seems that it is a more common case for the zone size to be configured/enabled per all Ingress resources than an per individual Ingress resource. For example, if the Ingress Controller is configured with multiple NGINX workers, why not to enable zones for all Ingresses? Does that apply for your requirements as well?

Additionally, having zones enabled by default also makes sense, so that the mentioned benefits are enabled by default. The default size of 256K will work for the case of more than 100 endpoints in a single upstream. We use the same size in the upstreams of NGINX Plus. Having a special value 0 to remove the zones from upstreams is reasonable to me.

Let me know what you think.

vrrs · 2019-07-24T19:59:34Z

@pleshakov Our use case requires individual resources to specify max_conn and zone since our ingress nginx is shared. Different set of upstream servers might require different zone sizes
or no zones at all. In addition, this change is backward compatible. We have already deployments without upstream zone and adding a default zone might have an impact on performance.
However, We can add a configmap key with a special value(0) as you suggested and set it to 0 as default. This way it covers both use cases.

In summary, We should add a configmap key along with the annotation defaulted to 0 which will enable both use cases of having all ingresses with one zone(using configmap key) or having individual ingresses with specific zones(with annotation).
Let me know what you think.

pleshakov

@vrrs

In addition, this change is backward compatible. We have already deployments without upstream zone and adding a default zone might have an impact on performance.

Thanks for thinking about backward compatibility! However, we do not expect any significant impact on performance. At the same time, we will document this change in the release notes appropriately, so that during upgrade people can set the configmap key to 0 to disable zones.

In summary, We should add a configmap key along with the annotation defaulted to 0 which will enable both use cases of having all ingresses with one zone(using configmap key) or having individual ingresses with specific zones(with annotation).

Sounds good! Just a small note -- the annotation should default to the ConfigMap key.
If you could make the default 256K and treat 0 as a special value to turn off the zones (both for the configmap key and the annotation), that be great!

I added suggestions about the doc (which assumes the configmap key, the default and the special value to turn off zones) and a small consistency suggestion for the template.

docs/configmap-and-annotations.md

pleshakov · 2019-07-25T16:02:04Z

internal/configs/version1/nginx.ingress.tmpl

@@ -2,6 +2,7 @@

 {{range $upstream := .Upstreams}}
 upstream {{$upstream.Name}} {
+	{{if $upstream.UpstreamZoneSize }}zone {{$upstream.Name}} {{$upstream.UpstreamZoneSize}};{{end}}


Suggested change

{{if $upstream.UpstreamZoneSize }}zone {{$upstream.Name}} {{$upstream.UpstreamZoneSize}};{{end}}

{{if $upstream.UpstreamZoneSize}}zone {{$upstream.Name}} {{$upstream.UpstreamZoneSize}};{{end}}

vrrs · 2019-07-25T21:45:24Z

@pleshakov Performed the changes

vrrs · 2019-07-25T22:02:53Z

@Rulox @Dean-Coakley 👀 👀 --^

Dean-Coakley

Looks good!

Lgtm once: #629 (comment) is applied. Looks like it was missed.

Dean-Coakley · 2019-07-26T09:15:38Z

docs/configmap-and-annotations.md

@@ -166,6 +166,7 @@ spec:
 | `nginx.org/websocket-services` | N/A | Enables WebSocket for services. | N/A | [WebSocket support](../examples/websocket). |
 | `nginx.org/max-fails` | `max-fails` | Sets the value of the [max_fails](https://nginx.org/en/docs/http/ngx_http_upstream_module.html#max_fails) parameter of the `server` directive. | `1` | |
 | `nginx.org/max-conns` | N\A | Sets the value of the [max_conns](https://nginx.org/en/docs/http/ngx_http_upstream_module.html#max_conns) parameter of the `server` directive. | `0` | |
+| `nginx.org/upstream-zone-size` | N\A | Sets the size of the shared memory [zone](https://nginx.org/en/docs/http/ngx_http_upstream_module.html?#zone). If this annotation is not present then the zone directive will not be set. | `N\A` | |


Looks like you missed #629 (comment) unless I'm mistaken?

Dean-Coakley · 2019-07-26T09:16:31Z

internal/configs/config_params.go

@@ -109,6 +110,7 @@ func NewDefaultConfigParams() *ConfigParams {
 		SSLPorts:                   []int{443},
 		MaxFails:                   1,
 		MaxConns:                   0,
+		UpstreamZoneSize: 			"256k",


Looks like this is missing a run of gofmt, not gonna block the pr for it, but would be nice.

go fmt should be run for consistency before merging

vrrs · 2019-07-26T19:57:35Z

@Dean-Coakley Modified the documentation.

Dean-Coakley · 2019-07-26T20:23:50Z

@vrrs The format of config_params.go still isn't quite right.
Please run $ gofmt -w internal/configs/config_params.go

vrrs · 2019-07-26T21:41:13Z

Ohh i see, check now @Dean-Coakley

Dean-Coakley

Perfect! Thanks @vrrs

Rulox · 2019-07-29T14:13:25Z

@vrrs Looks great, just 2 small things before I can merge:

It seems this Added upstream zone directive annotation #629 (comment) was missed? (it's just removing the space there)
Make sure you follow the (style guides)[https://github.com/nginxinc/kubernetes-ingress/blob/master/CONTRIBUTING.md#git-style-guide]. You need to rebase against master and also squash all your commits into one, using a present tense commit message like Add upstream zone directive annotation/configmap.

Both changes are just to follow the rules and consistency with the rest of the code/repo.

Once that's done I will merge it, thanks again for your time!

vrrs · 2019-07-29T17:15:06Z

@Rulox

Done. FYI, the lbmethod upstream line is not following this standard which is the one i was following.
Regarding squashed commits, when merging you can select squash and merge. I always rebase before submitting PRs and using one commit(From the guideline: Keep a clean, concise and meaningful git commit history on your branch, rebasing locally and squashing before submitting a PR). The extra commits you see were as a consequence of the review. This branch has no merge conflicts. Am i suppose to close this PR and create a new one ?

Rulox · 2019-07-30T09:20:51Z

@vrrs

Yes I noticed when reviewing, it's a mistake, I'll fix it.
No worries, I'll squash them

👍

victor added 2 commits July 22, 2019 18:12

SERV-3377 Added upstream zone directive size

5118fae

Fixed unit tests

1d89339

pleshakov added the enhancement Pull requests for new features/feature enhancements label Jul 23, 2019

pleshakov reviewed Jul 25, 2019

View reviewed changes

Added configmap key for upstream zone

faf7fa2

vrrs force-pushed the upstream-zone branch from 8c79b19 to faf7fa2 Compare July 25, 2019 21:33

Dean-Coakley suggested changes Jul 26, 2019

View reviewed changes

victor added 2 commits July 26, 2019 15:46

Added changes to documentation

97157ac

Modified format of config_params

1714ffa

reformatted config_params

2c4e728

Dean-Coakley approved these changes Jul 26, 2019

View reviewed changes

Remove space between snippet and brackets in nginx.ingreess go template

ec35186

Rulox approved these changes Jul 30, 2019

View reviewed changes

Rulox merged commit 9739593 into nginx:master Jul 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added upstream zone directive annotation #629

Added upstream zone directive annotation #629

vrrs commented Jul 22, 2019 •

edited

Loading

pleshakov commented Jul 23, 2019

pleshakov commented Jul 24, 2019

vrrs commented Jul 24, 2019

pleshakov left a comment

pleshakov Jul 25, 2019

vrrs commented Jul 25, 2019

vrrs commented Jul 25, 2019

Dean-Coakley left a comment

Dean-Coakley Jul 26, 2019

Dean-Coakley Jul 26, 2019

Rulox Jul 26, 2019

vrrs commented Jul 26, 2019

Dean-Coakley commented Jul 26, 2019

vrrs commented Jul 26, 2019

Dean-Coakley left a comment

Rulox commented Jul 29, 2019

vrrs commented Jul 29, 2019

Rulox commented Jul 30, 2019

	{{if $upstream.UpstreamZoneSize }}zone {{$upstream.Name}} {{$upstream.UpstreamZoneSize}};{{end}}
	{{if $upstream.UpstreamZoneSize}}zone {{$upstream.Name}} {{$upstream.UpstreamZoneSize}};{{end}}

Added upstream zone directive annotation #629

Added upstream zone directive annotation #629

Conversation

vrrs commented Jul 22, 2019 • edited Loading

Proposed changes

Motivation

Use case

Checklist

pleshakov commented Jul 23, 2019

pleshakov commented Jul 24, 2019

vrrs commented Jul 24, 2019

pleshakov left a comment

Choose a reason for hiding this comment

pleshakov Jul 25, 2019

Choose a reason for hiding this comment

vrrs commented Jul 25, 2019

vrrs commented Jul 25, 2019

Dean-Coakley left a comment

Choose a reason for hiding this comment

Dean-Coakley Jul 26, 2019

Choose a reason for hiding this comment

Dean-Coakley Jul 26, 2019

Choose a reason for hiding this comment

Rulox Jul 26, 2019

Choose a reason for hiding this comment

vrrs commented Jul 26, 2019

Dean-Coakley commented Jul 26, 2019

vrrs commented Jul 26, 2019

Dean-Coakley left a comment

Choose a reason for hiding this comment

Rulox commented Jul 29, 2019

vrrs commented Jul 29, 2019

Rulox commented Jul 30, 2019

vrrs commented Jul 22, 2019 •

edited

Loading