fix(balancer) async initialization of balancer objects #3187

hishamhm · 2018-01-29T19:32:16Z

When upstreams include targets using hostnames instead of IPs, the initialization of balancer objects needs to perform DNS resolution, which involves opening a socket, which is not available in the init_worker phase.

~~This patch delays balancer initialization to the access phase.~~

Updated: This patch performs balancer initialization in a timer, which makes initialization asynchronous to worker initialization. For this reason, it adds a locking system to create_balancer to avoid race conditions when requests happen while the balancers are being initialized.

The locking system uses a simplified version of the resty.lock logic which does
not use the shm, since balancers are per-worker and we are concerned here only
with intra-worker concurrency control.

Tieske · 2018-01-29T22:45:51Z

this test runs on every request. Would it be possible to use a timer, started from init_worker to do the initialization?

hishamhm · 2018-01-30T12:29:46Z

@Tieske can one ensure that the timer will be serviced before the first access? I don't want to risk it ever running in an inconsistent state.

Tieske · 2018-01-31T22:02:44Z

if the timer is set at 0, it will run immediately

thibaultcha

Agreed with the approach in general, just left a few concerns!

thibaultcha · 2018-02-07T03:56:42Z

kong/core/handler.lua

+      if not balancer_initialized then
+        local ok, err = balancer.init()
+        if not ok then
+          log(ngx.ERR, err)


we should add some context to this error message, like "failed to initialize balancers: " ...

Also, it might be worth to bump this to an EMERG message (instead of ERR)? Error generally is used for something that impacts the execution of a particular request. An error such as this one has much wider implications to many of the subsequent requests that would be made.

thibaultcha · 2018-02-07T03:58:59Z

kong/core/balancer.lua

@@ -616,11 +637,16 @@ local function init()
    if ok ~= nil then
      oks = oks + 1
    else
-      log(ngx.STDERR, "failed creating balancer for ", name, ": ", err)
+      log(ngx.ERR, "failed creating balancer for ", name, ": ", err)


I believe my below comment was the reason why this was an STDERR level (probably best to use CRIT though)

thibaultcha · 2018-02-07T04:00:31Z

kong/core/balancer.lua

@@ -27,6 +28,7 @@ local _load_targets_into_memory

 -- table holding our balancer objects, indexed by upstream name
 local balancers = {}
+local balancers_initialized = nil


It would be sweet to declare this in a do end block wrapping the below init function?

thibaultcha · 2018-02-07T04:01:45Z

kong/core/balancer.lua

+    return true
+  end
+
+  local lock, err = resty_lock:new("kong", { timeout = 3600 })


Isn't this timeout a bit excessive? Something like 30 might be safer? Is there a particular intent why it is so high? Anyway, it doesn't make much sense for it to be higher than the default exptime (which is 30s), and actually, it would be modified to 30 right now, see https://github.com/openresty/lua-resty-lock/blob/master/lib/resty/lock.lua#L104

thibaultcha

The overall approach looks good to me - I am of the opinion that we propose this patch to @aaronhmiller so that he can give it a try! :)

thibaultcha · 2018-02-14T04:12:12Z

kong/core/balancer.lua

@@ -470,7 +517,7 @@ local function get_balancer(target, no_create)
      return nil, "balancer not found"
    else
      log(ERR, "balancer not found for ", upstream.name, ", will create it")
-      return create_balancer(upstream)
+      return create_balancer(upstream), upstream


dammit, seems like this already was an issue?

it wasn't -- I mean, it was with regard to the function signature but it didn't happen because the get_balancer -> create_balancer code path was never hit when all balancers were created at startup. Now that it is async and get_balancer can run before init finishes running, then this code path can happen.

My one concern here is that this can be confusing for users of get_balancer and increase cognitive load: create_balancer can return 1 or 2 values, making get_balancer return 1, 2, or 3 values. Usually, we deal with this by catching errors like:

local balancer, err = create_balancer(upstream) if not balancer then return nil, err end return balancer, upstream -- still somewhat annoying to me, I prefer `balancer, nil, upstream` but up to you!

thibaultcha · 2018-02-14T04:14:00Z

kong/core/balancer.lua

+      if ok then
+        return balancers[upstream.id]
+      else
+        return nil, "timeout waiting for balancer for " .. upstream.id


style: this goes against our code style which instructs to avoid subsequent branches when the previous one already returns :/

ah, ok, I had read the style doc differently! my vague recollection is that it instructed to return early on the ground of performing validations and avoiding unneeded computation -- since there's no difference in computation here I thought it didn't make a difference here (I tend to find this more readable because it reads as "or else, fail!" instead of "the normal code flow is to fail" -- I'll just invert the test condition)

will change it, no probs! :)

thibaultcha · 2018-02-14T04:15:06Z

kong/core/balancer.lua

@@ -289,9 +293,50 @@ do
    end
  end

+  local creating = {}


should we scope this and other create_balancer dependencies under a do ... end block? It'd be a little bit saner/easier to skim through imho

this is already inside a do-block and far down so it's only visible to the two relevant functions that need it. Can we leave this one as-is?

Hmm I missed it; quite large for do block, but that's fine!

thibaultcha · 2018-02-14T04:18:38Z

kong/core/balancer.lua

  local upstreams, err = get_all_upstreams()
  if not upstreams then
-    log(ngx.STDERR, "failed loading initial list of upstreams: ", err)
-    return
+    return nil, "failed loading initial list of upstreams: " .. err


It seems we don't log this returned error in our balancer.init() call in init_worker? Maybe we should keep this as a CRIT log + early return?

When upstreams include targets using hostnames instead of IPs, the initialization of balancer objects needs to perform DNS resolution, which involves opening a socket, which is not available in the init_worker phase. This patch performs balancer initialization in a timer, which makes initialization asynchronous to worker initialization. For this reason, it adds a locking system to create_balancer to avoid race conditions when requests happen while the balancers are being initialized. The locking system uses a simplified version of `resty.lock` which does not use the shm, since balancers are per-worker and we are concerned only with intra-worker concurrency control.

thibaultcha · 2018-02-14T18:53:08Z

kong/core/balancer.lua

  local upstreams, err = get_all_upstreams()
  if not upstreams then
-    log(ngx.STDERR, "failed loading initial list of upstreams: ", err)
+    log(CRIT, "failed loading initial list of upstreams: " .. err)


Minor, but better not make use of Lua-land string concatenation in those error messages, and rely on the variadic arguments of ngx.log instead

thibaultcha · 2018-02-14T19:00:00Z

kong/core/balancer.lua

@@ -470,7 +517,7 @@ local function get_balancer(target, no_create)
      return nil, "balancer not found"
    else
      log(ERR, "balancer not found for ", upstream.name, ", will create it")
-      return create_balancer(upstream)
+      return create_balancer(upstream), upstream


My one concern here is that this can be confusing for users of get_balancer and increase cognitive load: create_balancer can return 1 or 2 values, making get_balancer return 1, 2, or 3 values. Usually, we deal with this by catching errors like:

local balancer, err = create_balancer(upstream) if not balancer then return nil, err end return balancer, upstream -- still somewhat annoying to me, I prefer `balancer, nil, upstream` but up to you!

hishamhm force-pushed the fix/balancer-initialization branch from b4efc00 to 096c7b5 Compare February 6, 2018 18:27

thibaultcha reviewed Feb 7, 2018

View reviewed changes

hishamhm force-pushed the fix/balancer-initialization branch 4 times, most recently from 4c672d5 to b0346d6 Compare February 8, 2018 17:32

hishamhm changed the title ~~fix(balancer) delay initialization of balancer objects~~ fix(balancer) async initialization of balancer objects Feb 8, 2018

hishamhm force-pushed the fix/balancer-initialization branch 2 times, most recently from f413715 to 7c65baa Compare February 8, 2018 18:15

tests(balancer) active healthchecks run before any proxy traffic

332d105

hishamhm force-pushed the fix/balancer-initialization branch from 7c65baa to 4afa415 Compare February 8, 2018 18:41

thibaultcha reviewed Feb 14, 2018

View reviewed changes

hishamhm force-pushed the fix/balancer-initialization branch from 4afa415 to 1d5a10e Compare February 14, 2018 13:33

kikito approved these changes Feb 14, 2018

View reviewed changes

thibaultcha approved these changes Feb 14, 2018

View reviewed changes

hishamhm merged commit 1d63abe into master Feb 14, 2018

hishamhm deleted the fix/balancer-initialization branch February 14, 2018 19:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(balancer) async initialization of balancer objects #3187

fix(balancer) async initialization of balancer objects #3187

hishamhm commented Jan 29, 2018 •

edited

Loading

Tieske commented Jan 29, 2018

hishamhm commented Jan 30, 2018

Tieske commented Jan 31, 2018

thibaultcha left a comment

thibaultcha Feb 7, 2018

thibaultcha Feb 7, 2018

thibaultcha Feb 7, 2018

thibaultcha Feb 7, 2018

thibaultcha Feb 7, 2018

thibaultcha left a comment

thibaultcha Feb 14, 2018

hishamhm Feb 14, 2018

thibaultcha Feb 14, 2018

thibaultcha Feb 14, 2018

hishamhm Feb 14, 2018

thibaultcha Feb 14, 2018

hishamhm Feb 14, 2018

thibaultcha Feb 14, 2018

thibaultcha Feb 14, 2018

hishamhm Feb 14, 2018

thibaultcha Feb 14, 2018

thibaultcha Feb 14, 2018

fix(balancer) async initialization of balancer objects #3187

fix(balancer) async initialization of balancer objects #3187

Conversation

hishamhm commented Jan 29, 2018 • edited Loading

Tieske commented Jan 29, 2018

hishamhm commented Jan 30, 2018

Tieske commented Jan 31, 2018

thibaultcha 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

thibaultcha 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hishamhm commented Jan 29, 2018 •

edited

Loading