introduce a balancer interface #2543

ElvinEfendi · 2018-05-18T21:38:32Z

What this PR does / why we need it:
This PR refactors balancer logic to be more modular, extensible and testable. It also fixes bug with returning 503 by using rewrite_by_lua handler. Now balancer.lua has four public methods namely init_worker(), rewrite(), balance(), and log() which get called in their respective phase.

init_worker() function kicks in when Nginx worker starts. It calls private sync_backends() function that takes care of initializing, updating and deleting respective balancer instances for the backends. sync_backends() also gets called periodically in every 1 second to keep the backend balancers in sync for the given Nginx worker.

rewrite() function takes care of returning 503 when there's no balancer for the backend current request corresponds to.

balancer() gets the balancer instance for given request/backend and calls :balance() function which executes the respective load balancing algorithm and returns host, port pair. balancer() then takes care of calling Nginx API function to set the upstream peer.

log() function gets the balancer instance for given request/backend and calls :after_balance(). Currently only EWMA algorithm implements after_balance(). This is useful when the LB algorithm needs to store some stats related to i.e response times or response status code etc.

--

With this PR if someone wants to introduce a new load balancing algorithm they can create the respective Lua module in balancer/ folder and register it in balancer.lua. Every new load balancing module has to implement at least new(backend), sync(backend) and balance(). Optionally they can implement after_balance() as well.

--

Once this PR gets merged I'll add more unit tests which will most likely make some of the related e2e tests weak enough that we can just delete.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

codecov-io · 2018-05-26T04:28:48Z

Codecov Report

Merging #2543 into master will increase coverage by 0.13%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2543      +/-   ##
==========================================
+ Coverage   40.63%   40.77%   +0.13%     
==========================================
  Files          74       74              
  Lines        5094     5094              
==========================================
+ Hits         2070     2077       +7     
+ Misses       2743     2737       -6     
+ Partials      281      280       -1

Impacted Files	Coverage Δ
cmd/nginx/main.go	`29% <0%> (+5.34%)`	⬆️

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 a5958c0...5d12848. Read the comment docs.

ElvinEfendi · 2018-05-26T16:15:19Z

/hold

ElvinEfendi · 2018-05-26T16:15:33Z

/assign @aledbf

aledbf · 2018-05-26T16:21:35Z

@ElvinEfendi this looks really good. Just one question about this, can you add information about the ingress/service in the log messages? Not necessarily in this PR but I think we need more information than just IP/Port in case of failures. WDYT?

ElvinEfendi · 2018-05-26T16:22:28Z

@andrewlouis93 @csfrancis @ibawt would be great if some of you can take a look and give feedback as well.

ElvinEfendi · 2018-05-26T16:30:37Z

Just one question about this, can you add information about the ingress/service in the log messages? Not necessarily in this PR but I think we need more information than just IP/Port in case of failures. WDYT?

@aledbf $proxy_upstream_name is already part of Nginx log message by default, so we can know ingress, service and namespace by looking at proxy_upstream_name's value in Nginx log for the request. And other errors happening for example in sync_backends which is not in request path, are not about particular ingress/service. If I'm missing something please let me know.

Later on I'd like to make balancer.name part of log messages in request path though. Or maybe introduce a new Nginx variable and set it to balancer.name and make it part of Nginx access log.

aledbf · 2018-05-28T13:46:37Z

/lgtm
/approve

andrewloux · 2018-05-28T14:05:12Z

/assign @andrewlouis93

k8s-ci-robot · 2018-05-28T14:05:13Z

@andrewlouis93: GitHub didn't allow me to assign the following users: andrewlouis93.

Note that only kubernetes members and repo collaborators can be assigned.

In response to this:

/assign @andrewlouis93

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.

andrewloux · 2018-05-28T14:23:26Z

rootfs/etc/nginx/lua/balancer.lua

-  return backend
-end
+local function get_implementation(backend)
+  local name = backend["load-balance"] or DEFAULT_LB_ALG


Maybe out the scope of this PR - but could we have the DEFAULT_LB_ALG set per ingress?

If a user wanted to use a default lb algorithm it would currently require each app behind the ingress to update their load-balance attribute. WDYT?

@andrewlouis93 load-balance is also a configmap property so you can have a default value of it per ingress deployment and then customize further per app using the annotation.

Right, but currently this lua code is not reading the load-balance configmap property is it?

backend["load-balance"] is not necessary only load-balance annotation. It is whatever is set to the Backend struct in controller side. So yeah Lua will get custom load-balance algorithm when you set it in configmap.

Gotcha, makes sense :)

andrewloux · 2018-05-28T14:44:57Z

rootfs/etc/nginx/lua/balancer.lua

-  if not backend then
-    return nil
+  if backend["sessionAffinityConfig"] and backend["sessionAffinityConfig"]["name"] == "cookie" then
+    name = "sticky"


If a backend load-balance algorithm is set, and is overriden here since sessionAffinity is required - should we emit a log message here indicating that the load-balance implementation is being ignored?

This is a default behaviour in non dynamic mode as well. We can maybe document better how this annotation overrides load-balance annotation and config, but I don't see how logging here would add any value.

That being said we can do INFO logging if you think it is valuable. But I'd rather see this becoming necessity in practice first and then add the logging.

That being said we can do INFO logging if you think it is valuable. But I'd rather see this becoming necessity in practice first and then add the logging.

💯

ibawt · 2018-05-28T15:03:36Z

code LGTM @ElvinEfendi did you any specific worries you'd like me to go over in depth?

andrewloux · 2018-05-28T15:25:27Z

rootfs/etc/nginx/lua/balancer.lua

+function _M.balance()
+  local balancer = get_balancer()
+  if not balancer then
+    return


Do we want to log something if not balancer here?

@andrewlouis93 https://github.com/kubernetes/ingress-nginx/pull/2543/files#diff-b00d77a6df9c8c05a483044b08e6bc50R103 short-circuits when balancer does not exist and this code does not even get executed. And we get 503 response code

Right I thought about that - but consider the case when a sync_backends that results in balancers = {} occurs between a request's rewrite and balance phase?

andrewloux · 2018-05-28T15:25:56Z

rootfs/etc/nginx/lua/balancer.lua

  end
 end

+function _M.log()
+  local balancer = get_balancer()
+  if not balancer then


Same question here

https://github.com/kubernetes/ingress-nginx/pull/2543/files#diff-b00d77a6df9c8c05a483044b08e6bc50R103 takes care of this

ElvinEfendi · 2018-05-28T16:52:51Z

@ibawt thanks for looking into, reviewing general design approach is enough for now!

csfrancis

I've only really looked at balancer.lua, but I mostly approve of the interface. I question if rewrite is necessary, but everything else looks good.

csfrancis · 2018-05-28T18:23:58Z

rootfs/etc/nginx/lua/balancer.lua

+    return
+  end
+
+  balancer:after_balance()


I think this should be checking if the function exists, since it can be optional.

csfrancis · 2018-05-28T18:25:22Z

rootfs/etc/nginx/lua/balancer.lua

  end

-  local host, port = balance()
+  local host, port = balancer:balance()
  if not host then


Should you be checking that port is set as well?

csfrancis · 2018-05-28T18:26:17Z

rootfs/etc/nginx/lua/balancer.lua

  if not host then
-    ngx.status = ngx.HTTP_SERVICE_UNAVAILABLE
-    return ngx.exit(ngx.status)
+    ngx.log(ngx.WARN, "no host returned, balancer: " .. balancer.name)


I'm curious why you got rid of the 503 here? To me it seems like it would be better to be more explicit about this error case.

as mentioned at https://github.com/kubernetes/ingress-nginx/pull/2543/files#r191262645 it is not possible to modify status at this phase.

csfrancis · 2018-05-28T18:26:48Z

rootfs/etc/nginx/lua/balancer.lua

  end

  ngx_balancer.set_more_tries(1)

  local ok, err = ngx_balancer.set_current_peer(host, port)
  if not ok then
-    ngx.log(ngx.ERR, string.format("error while setting current upstream peer to %s", tostring(err)))
+    ngx.log(ngx.ERR, "error while setting current upstream peer to " .. tostring(err))


Similar comment here ... should this return a 503?

https://github.com/kubernetes/ingress-nginx/pull/2543/files#r191262997

csfrancis · 2018-05-28T18:30:20Z

rootfs/etc/nginx/lua/balancer.lua

+  local balancer = get_balancer()
+  if not balancer then
+    ngx.status = ngx.HTTP_SERVICE_UNAVAILABLE
+    return ngx.exit(ngx.status)


I'm not super familiar with the ingress-nginx config, but I'm guessing this means that every configured upstream will be configured to use Lua load balancing? Likewise, will rewrite_by_lua be called for every configured site? Why not move this logic into balance instead?

@csfrancis this was in balance() before. But it turns out modifying ngx.status is not allowed in balance phase.

but I'm guessing this means that every configured upstream will be configured to use Lua load balancing

that's correct it's either all or none

csfrancis · 2018-05-28T18:54:46Z

rootfs/etc/nginx/lua/balancer.lua

+    return
+  end
+
+  if getmetatable(balancer) ~= implementation then


This is weird .. why are you calling getmetatable here?

@csfrancis because of the way OOP implemented in Lua. Another option would be to compare .name but IMO this is more elegant.

If you notice new() function in implementations return a new Lua table and sets its metatable to self.

So this logic is to detect the situation where an app starts with let's say EWMA, then in next deploy switches to Round Robin. This condition will detect that and replace EWMA instance with new RR instance for the backend.

Weird .. that was not immediately obvious to me. More elegant, but I feel like .name is more clear. But that's just my opinion, man.

Maybe a comment would make this clearer?

@csfrancis I wrote this with .name initially - another reason why I switched to the current version was that if we use .name for this then we have to make that field mandatory for every implementation. Currently .name is more for logging purposes and optional.

I'll extract this into a function with a meaningful name :)

or maybe just add comment for now

add inline comment to make LB algorithm change detection logic clearer also require port in addition to host

ElvinEfendi · 2018-05-28T21:01:08Z

/hold cancel

aledbf · 2018-05-28T21:04:40Z

/lgtm

k8s-ci-robot · 2018-05-28T21:04:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, ElvinEfendi

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~~ [aledbf]

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

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 18, 2018

ElvinEfendi changed the title ~~[ci-skip] [WIP] introduce a balancer interface~~ [WIP] introduce a balancer interface May 18, 2018

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 18, 2018

ElvinEfendi force-pushed the lua-balancer-refactoring branch from 84d0006 to 8e42ddd Compare May 26, 2018 00:08

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 26, 2018

ElvinEfendi force-pushed the lua-balancer-refactoring branch 2 times, most recently from c711cfa to 422ef31 Compare May 26, 2018 00:11

ElvinEfendi changed the title ~~[WIP] introduce a balancer interface~~ introduce a balancer interface May 26, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 26, 2018

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 26, 2018

k8s-ci-robot assigned aledbf May 26, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 28, 2018

andrewloux reviewed May 28, 2018

View reviewed changes

csfrancis reviewed May 28, 2018

View reviewed changes

ElvinEfendi added 4 commits May 28, 2018 15:51

refactor balancer into more testable and extensible interface

e9dc275

make sure balancer gets deleted when ther is no backend

da3a876

fix ewma.balance and add unit tests for it

04b7356

make sure after_balance is actually otional

b4e6513

add inline comment to make LB algorithm change detection logic clearer also require port in addition to host

ElvinEfendi force-pushed the lua-balancer-refactoring branch from 5d12848 to b4e6513 Compare May 28, 2018 20:28

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 28, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 28, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 28, 2018

k8s-ci-robot merged commit f099d37 into kubernetes:master May 28, 2018

ElvinEfendi deleted the lua-balancer-refactoring branch May 28, 2018 21:13

introduce a balancer interface #2543

introduce a balancer interface #2543

Conversation

ElvinEfendi commented May 18, 2018 • edited Loading

codecov-io commented May 26, 2018

Codecov Report

ElvinEfendi commented May 26, 2018

ElvinEfendi commented May 26, 2018

aledbf commented May 26, 2018

ElvinEfendi commented May 26, 2018

ElvinEfendi commented May 26, 2018 • edited Loading

aledbf commented May 28, 2018

andrewloux commented May 28, 2018

k8s-ci-robot commented May 28, 2018

andrewloux May 28, 2018 • edited Loading

Choose a reason for hiding this comment

ElvinEfendi May 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvinEfendi May 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibawt commented May 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewloux May 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

ElvinEfendi commented May 28, 2018

csfrancis left a 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

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

Choose a reason for hiding this comment

ElvinEfendi May 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvinEfendi commented May 28, 2018

aledbf commented May 28, 2018

k8s-ci-robot commented May 28, 2018

ElvinEfendi commented May 18, 2018 •

edited

Loading

ElvinEfendi commented May 26, 2018 •

edited

Loading

andrewloux May 28, 2018 •

edited

Loading

ElvinEfendi May 28, 2018 •

edited

Loading

ElvinEfendi May 28, 2018 •

edited

Loading

andrewloux May 28, 2018 •

edited

Loading

ElvinEfendi May 28, 2018 •

edited

Loading