Dynamic reload first implementation #2152

valeriano-manassero · 2018-02-27T14:06:36Z

This PR adds a "dynamic-reload" mode.
If --dynamic-reload=true is added to the run command, NGINX will use LUA and will not reload if a server name or a backend (virtualhost) is added or deleted.

This is a first implementation based on concepts expressed here: #1905

k8s-reviewable · 2018-02-27T14:06:40Z

This change is

k8s-ci-robot · 2018-02-27T14:06:41Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

valeriano-manassero · 2018-02-27T14:09:53Z

/assign @aledbf

valeriano-manassero · 2018-02-27T14:38:13Z

CLA signed

aledbf · 2018-02-27T15:04:43Z

internal/ingress/controller/nginx.go

@@ -602,6 +615,31 @@ func (n *NGINXController) OnUpdate(ingressCfg ingress.Configuration) error {

 	cfg.SSLDHParam = sslDHParam

+	for _, server := range ingressCfg.Servers {


Let's do this feature in stages. Please remove this section. For this PR SSL should be handled by NGINX, not LUA.

aledbf · 2018-02-27T15:10:42Z

@valeriano-manassero first, thank you for doing this.

That said, the scope of this PR is huge. I propose to do this in stages, where the first one tackles only the upstream part, i.e. do not reload nginx when a new pod is added or removed from a service.

For this, your route template you should have only {{ $backends := .Backends }} as content and use the name of the backend as the key to knowing which upstream should handle the traffic.

k8s-ci-robot · 2018-02-27T17:04:44Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: valeriano-manassero
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: aledbf

Assign the PR to them by writing /assign @aledbf in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

aledbf · 2018-02-27T19:18:32Z

@valeriano-manassero please check the CI error (you need to fix the style of the go code)

oilbeater · 2018-02-28T08:41:24Z

As every request will go through lua code, performance will decrease. LuaJIT can provide better performance.

valeriano-manassero · 2018-02-28T09:21:34Z

@oilbeater using LuaJIT already:
ldd /usr/sbin/nginx | grep -i lua
returns
libluajit-5.1.so.2 => /usr/local/lib/libluajit-5.1.so.2 (0x00007feade334000)

valeriano-manassero · 2018-02-28T09:25:18Z

@aledbf regarding CI errors, I fixed code style. Now I'm getting error during TestStore function. For me it's confusing since I don't get this error while making coverage on my envirnonment.
Regarding tackling only the upstream part, I will answer you later. Thank you for your support.

codecov-io · 2018-02-28T11:56:45Z

Codecov Report

Merging #2152 into master will increase coverage by 0.3%.
The diff coverage is 43.8%.

@@            Coverage Diff            @@
##           master    #2152     +/-   ##
=========================================
+ Coverage   36.45%   36.75%   +0.3%     
=========================================
  Files          69       69             
  Lines        4861     4957     +96     
=========================================
+ Hits         1772     1822     +50     
- Misses       2819     2859     +40     
- Partials      270      276      +6

Impacted Files	Coverage Δ
internal/ingress/controller/config/config.go	`97.97% <ø> (ø)`	⬆️
internal/ingress/controller/nginx.go	`3.56% <0%> (-0.4%)`	⬇️
internal/ingress/controller/controller.go	`0% <0%> (ø)`	⬆️
cmd/nginx/flags.go	`83.82% <100%> (+0.24%)`	⬆️
cmd/nginx/main.go	`20.8% <20%> (-0.58%)`	⬇️
internal/file/bindata.go	`53.93% <84.31%> (+10.85%)`	⬆️
internal/ingress/controller/template/template.go	`64.75% <0%> (+2%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 164bb7b...c1b9f7f. Read the comment docs.

valeriano-manassero · 2018-02-28T12:01:19Z

@aledbf As you suggested, I implemented first stage (no reload only on the upstream part).
When dynamic mode is selected, the load balance algorithm is, actually, round robin.
As first implementation, it can be very helpful some testing by the community and, if is ok, I can implement least_conn and ip_hash too.

aledbf · 2018-02-28T14:10:00Z

rootfs/etc/nginx/template/nginx.tmpl

@@ -871,7 +906,6 @@ stream {
            {{ end }}

            {{/* Add any additional configuration defined */}}
-            {{ $location.ConfigurationSnippet }}


Why are you removing this line?

It's a mistake, I don't how it happened. Fixing in next commit.

aledbf · 2018-02-28T14:12:24Z

rootfs/etc/nginx/template/routes.tmpl

+    ]
+}
+{{ end }}
+}


Maybe we can serialize the content of $backends in Go and avoid the template part? (with this are sure the JSON is valid)

aledbf · 2018-02-28T14:19:13Z

@valeriano-manassero this looks really good. I just added two comments.

aledbf · 2018-02-28T14:31:19Z

@valeriano-manassero questions:

Can you implement some kind of hash to provide sticky sessions
https://github.com/kubernetes/ingress-nginx/blob/master/rootfs/etc/nginx/template/nginx.tmpl#L310
with something like openresty/lua-resty-balancer#4 (comment)

This is just to provide the same features we have now in the upstream section

Edit: this is a shorter version of the lua code https://github.com/canhnt/lua-sticky-session/blob/master/lua/sticky_session.lua#L43-L51

aledbf · 2018-02-28T14:33:06Z

As first implementation, it can be very helpful some testing by the community and, if is ok, I can implement least_conn and ip_hash too.

This would be really great to have so we provide the same features present in the upstream now
https://github.com/kubernetes/ingress-nginx/blob/master/docs/user-guide/configmap.md#load-balance

valeriano-manassero · 2018-03-01T07:19:33Z

@aledbf I'm on my way to improve code as you suggested.

@oilbeater sorry, reading again your suggestion I understand now what you mean. I will set the Dockerfile to generate bytecode of my custom scripts and I will use them in nginx conf.

Thank you all.

valeriano-manassero · 2018-03-01T13:59:55Z

Pushed all implementations as suggested.
Please remember this is an experimental feature disabled by default.

@aledbf I do not write the routes file anymore but I need the template to get a custom json to push in POST on /nginx_update

aledbf · 2018-03-03T02:11:10Z

@valeriano-manassero apologies for the delay but I had to solve some issues with the nginx image.

aledbf · 2018-03-03T02:17:14Z

For those interested in helping with this feature the image
quay.io/aledbf/nginx-ingress-controller:0.339 contains this PR.
The flag --dynamic-reload=true is required in the deployment to enable this feature.

aledbf · 2018-03-03T02:25:29Z

internal/ingress/controller/controller.go

-	if !n.isForceReload() && n.runningConfig.Equal(&pcfg) {
-		glog.V(3).Infof("skipping backend reload (no changes detected)")
-		return nil
+	if !n.cfg.DynamicReload {


The configuration could change, like an update to the configmap. This should be evaluated and reload NGINX even with the dynamic reload feature enabled.
To test this you can add/change enable-vts: "true" in the configuration configmap. You will see the diff of the nginx.conf (using the flag --v=2) but there's no NGINX reload.

aledbf · 2018-03-03T03:01:06Z

rootfs/etc/nginx/lua/balancer.lua

+end
+
+assert(b.set_current_peer(selected_endpoint.hostname, selected_endpoint.port))
+if (selected_endpoint.failtimeout ~= 0) then


Can you add some logs like ngx.log(ngx.DEBUG, using endpoint <host>:<port>) ?

aledbf · 2018-03-03T03:03:52Z

internal/ingress/controller/nginx.go

+					glog.Infof("NGINX dynamic update not ready\n")
+				} else {
+					updateOK = true
+					glog.Infof("NGINX dynamic update OK\n")


can you add something like

if glog.V(2) { glog.Infof("Updating nginx endpoints with\n%v\n",content) }

so with can see the content used by lua

ElvinEfendi · 2018-03-03T06:33:02Z

internal/ingress/controller/nginx.go

+				return fmt.Errorf("%v\n%v", err, string(o))
+			}
+		} else {
+			glog.Infof("NGINX reload not needed, executing live update only\n")


I might be reading the code wrong, but why is live updated needed in this case? It seems like the existing configuration is identical to the new one(referring to line 677).

Similarly if we are reloading at 683, why don't we break? What's the point of continuing with live update after reload?

If I'm right, there's the case where config file does not change, but live update needed to include new backends.

ElvinEfendi · 2018-03-03T06:35:39Z

rootfs/etc/nginx/lua/update.lua

+    ngx.req.read_body()
+    local backends_json = ngx.req.get_body_data()
+    local shared_memory = ngx.shared.shared_memory
+    shared_memory:set("CFGS", backends_json, 0)


what if setting to the dictionary fails? We still return 200 and controller will think everything went well.

ElvinEfendi · 2018-03-03T06:38:05Z

rootfs/etc/nginx/lua/balancer.lua

@@ -0,0 +1,112 @@
+local json = require "json"


Is this a plain Lua JSON lib? To my knowledge cjson is the recommended library to use for JSON encoding/decoding for performance reasons.

As far as I know cjson cannot be used with LuaJIT.

I have been using it with LuaJIT, also Openresty includes it by default https://openresty.org/en/lua-cjson-library.html

ElvinEfendi · 2018-03-04T20:47:06Z

IMHO instead of trying to match what LB algorithms Nginx supports we should focus on more general picture that is building strong foundation for dynamically configurable ingress-nginx. We can eventually get to a point where even new Ingress creation and removal would not require Nginx reload rather than only making endpoints and certificates dynamically reconfigurable. However this also means we will have more and more Lua middleware if we go down this path. Therefore I suggest following:

Decide how much do we want to rely on lua_ngx? Do we see this project implementing more and more Lua middleware in future?
Considering reloads caused by app deployments are the most common issue I suggest we first skip server reloads only for Endpoints update and fallback to existing approach for everything else
To be able to focus on stronger foundation rather than richer feature support focus on implementing a single LB algorithm
Decide how we are going to test these Lua middleware and setup CI accordingly

FWIW I have also been working on a similar feature at Shopify#16.

ElvinEfendi · 2018-03-04T20:57:37Z

rootfs/etc/nginx/lua/balancer.lua

+    ngx.status = 503
+    ngx.exit(ngx.status)
+end
+local cfgs = json.decode(cfgs_json)


this might have significant performance penalty, because it is being done for every request

True but I not found alternatives. Only shared dicts are visible worker wide.

Here https://github.com/Shopify/ingress/pull/16/files#diff-b00d77a6df9c8c05a483044b08e6bc50R87 is how I did it. I'm curious what do you think about that.

Basically periodically in every worker read the config from shared dictionary, decode it and store it locally.

Then the balancer algorithm will use the parsed/decoded config from local cache.

This is a very good implementation, congrats!

I understand proposed solution is not perfect but my intent was to add an experimental feature to be improved later.

Thanks @valeriano-manassero! I made a PR at #2174. If you agree with the ideas there and fine with the implementation I suggest we try to get it merged and continue adding the different LB algorithms you implemented here on top of it.

It already provides a mechanism to easily and cleanly add support for more LB algorithms but there's still room for improvement.

PR link was incorrect in my comment above, it is now pointing to the right PR.

aledbf · 2018-03-04T20:59:51Z

We can eventually get to a point where even new Ingress creation and removal would not require Nginx reload rather than only making endpoints and certificates dynamically reconfigurable.

Not sure about this. Right now the ingress controller is "just" nginx. You can see the nginx.conf file and it does not requires more knowledge than that. If we go full lua then the users must learn a new thing.

Decide how much do we want to rely on lua_ngx? Do we see this project implementing more and more Lua middleware in future?

This is not clear. One of the goals of this PR is to see exactly how this would work.

Considering reloads caused by app deployments are the most common issue I suggest we first skip server reloads only for Endpoints update and fallback to existing approach for everything else.

This is what this PR is doing, avoid reloads for endpoint changes.

Decide how we are going to test these Lua middleware and setup CI accordingly

This is one of the things why I need to see this PR working fully.

ElvinEfendi · 2018-03-04T21:00:46Z

rootfs/etc/nginx/lua/balancer.lua

+
+    if cookie_value == nil then
+        local endpoints_roundrobin = ngx.shared.endpoints_roundrobin
+        local ep_index = endpoints_roundrobin:get(http_host)


Should load balancing states not be scoped to backend/upstream instead? If I read this correctly you are load balancing per http_host rather than upstream. Which for example means given two domains and 5 endpoints, if I send a request to first domain and another request to the second domain they will both be proxied to the same endpoint.

aledbf · 2018-03-04T21:02:28Z

@ElvinEfendi I forgot to mention one important point. Even if the ingress controller switches to lua 100% this does not mean there could not be a disruption in the traffic (websockets), just fewer reloads. If the pod is removed before the new version of the deployment is serving traffic is not possible to provide real blue/green deployments.

ElvinEfendi · 2018-03-04T21:03:08Z

rootfs/etc/nginx/lua/balancer.lua

+        local ep_index = endpoints_roundrobin:get(http_host)
+        if ep_index == nil then
+            selected_endpoint = backend.endpoints[1]
+            endpoints_roundrobin:set(http_host, 1, 600)


Why do you reset the load balancing state after specifically 600s? Should not those be reset only when list of respective endpoints change?

ElvinEfendi · 2018-03-04T21:05:42Z

rootfs/etc/nginx/lua/balancer.lua

+                endpoints_roundrobin:set(http_host, 1, 600)
+            else
+                selected_endpoint = backend.endpoints[new_index]
+                endpoints_roundrobin:set(http_host, new_index, 600)


There can be race condition between Nginx workers accessing/writing to this shared dictionary. IMHO the correct way to implement this is to use some kind of locking mechanism.

valeriano-manassero · 2018-03-07T13:18:08Z

#2174 (comment)

pingles · 2018-05-10T12:15:52Z

If it helps others (or future work) we turned this on for our clusters today.

I've attached a screenshot from a Google Cloud Trace analysis report for one of our services below, it shows that <p50 times were increased a little but tail latencies have been improved quite a lot.

Thanks!

ElvinEfendi · 2018-05-10T18:04:57Z

@pingles are you using EWMA(nginx.ingress.kubernetes.io/load-balance=ewma)? If not that can potentially improve tail latencies even further.

The default load balancing algorithm in dynamic mode is Round Robin.

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 27, 2018

k8s-ci-robot assigned aledbf Feb 27, 2018

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 27, 2018

aledbf reviewed Feb 27, 2018

View reviewed changes

aledbf added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 27, 2018

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 28, 2018

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 28, 2018

aledbf reviewed Feb 28, 2018

View reviewed changes

aledbf mentioned this pull request Mar 3, 2018

enable setting upstreams dynamically via HTTP endpoint Shopify/ingress#16

Closed

aledbf reviewed Mar 3, 2018

View reviewed changes

ElvinEfendi reviewed Mar 3, 2018

View reviewed changes

ElvinEfendi reviewed Mar 4, 2018

View reviewed changes

ghost mentioned this pull request Mar 5, 2018

Turning off keepalive does not work as documented #2168

Closed

Dynamic reload first implementation

c1b9f7f

ElvinEfendi mentioned this pull request Mar 6, 2018

Live Nginx configuration update without reloading #2174

Merged

valeriano-manassero closed this Mar 14, 2018

valeriano-manassero deleted the dynamic_reload branch March 19, 2018 08:12

		@@ -602,6 +615,31 @@ func (n *NGINXController) OnUpdate(ingressCfg ingress.Configuration) error {

		cfg.SSLDHParam = sslDHParam

		for _, server := range ingressCfg.Servers {

Dynamic reload first implementation #2152

Dynamic reload first implementation #2152

Conversation

valeriano-manassero commented Feb 27, 2018 • edited Loading

k8s-reviewable commented Feb 27, 2018

k8s-ci-robot commented Feb 27, 2018

valeriano-manassero commented Feb 27, 2018

valeriano-manassero commented Feb 27, 2018

Choose a reason for hiding this comment

aledbf commented Feb 27, 2018

k8s-ci-robot commented Feb 27, 2018

aledbf commented Feb 27, 2018

oilbeater commented Feb 28, 2018

valeriano-manassero commented Feb 28, 2018 • edited Loading

valeriano-manassero commented Feb 28, 2018 • edited Loading

codecov-io commented Feb 28, 2018 • edited Loading

Codecov Report

valeriano-manassero commented Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aledbf commented Feb 28, 2018

aledbf commented Feb 28, 2018 • edited Loading

aledbf commented Feb 28, 2018

valeriano-manassero commented Mar 1, 2018

valeriano-manassero commented Mar 1, 2018

aledbf commented Mar 3, 2018

aledbf commented Mar 3, 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

ElvinEfendi Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvinEfendi Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvinEfendi commented Mar 4, 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

ElvinEfendi Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

ElvinEfendi Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

aledbf commented Mar 4, 2018

Choose a reason for hiding this comment

aledbf commented Mar 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valeriano-manassero commented Mar 7, 2018

pingles commented May 10, 2018

ElvinEfendi commented May 10, 2018 • edited Loading

valeriano-manassero commented Feb 27, 2018 •

edited

Loading

valeriano-manassero commented Feb 28, 2018 •

edited

Loading

valeriano-manassero commented Feb 28, 2018 •

edited

Loading

codecov-io commented Feb 28, 2018 •

edited

Loading

valeriano-manassero commented Feb 28, 2018 •

edited

Loading

aledbf commented Feb 28, 2018 •

edited

Loading

aledbf commented Mar 3, 2018 •

edited

Loading

ElvinEfendi Mar 3, 2018 •

edited

Loading

ElvinEfendi Mar 3, 2018 •

edited

Loading

ElvinEfendi Mar 6, 2018 •

edited

Loading

ElvinEfendi Mar 6, 2018 •

edited

Loading

ElvinEfendi commented May 10, 2018 •

edited

Loading