[THREESCALE-2194] Ensure locks are released on configuration_loader. #1050

eloycoto · 2019-05-29T09:22:02Z

This commits add a new function run to the synchronization module,
that function ensures that a given function is going to be run in a safe
task and make sure that the release of the lock always happens, so the
lock is not going to be blocked if something goes wrong.

Fix #1010
Fix THREESCALE-2194

davidor · 2019-05-29T10:06:59Z

t/configuration-loading-lazy.t

@@ -75,7 +76,7 @@ env THREESCALE_PORTAL_ENDPOINT=http://127.0.0.1:$TEST_NGINX_SERVER_PORT;
 GET /t?user_key=fake
 --- error_code: 200
 --- user_files eval
-[
+[ 


just empty whitespace on the end of the line. I think that it's good to have it clean :-)

davidor · 2019-05-29T10:20:51Z

gateway/src/resty/synchronization.lua

+-- @tparam int timeout: timeout for getting the lock before raise an error.
+-- @tparam function callback: function to execute if the lock is adquired correctly
+function _M:run(key, timeout, callback, ...)
+  local sema = self:acquire(key)


acquire can return an error.

davidor · 2019-05-29T10:21:23Z

gateway/src/resty/synchronization.lua

+--- run a new function, callback using locks on the given key
+-- @tparam string key: key for the semaphore
+-- @tparam int timeout: timeout for getting the lock before raise an error.
+-- @tparam function callback: function to execute if the lock is adquired correctly


typo acquired

davidor · 2019-05-29T10:23:04Z

gateway/src/resty/synchronization.lua

+--- run a new function, callback using locks on the given key
+-- @tparam string key: key for the semaphore
+-- @tparam int timeout: timeout for getting the lock before raise an error.
+-- @tparam function callback: function to execute if the lock is adquired correctly


Maybe we should also add ... to the documentation of the function. I can't remember the syntax now.

davidor · 2019-05-29T10:25:44Z

gateway/src/resty/synchronization.lua

+  local task = safe_task.new(callback)
+  local ret, result = task:execute(...)
+
+  if ok then


Can we name this lock_acquired or similar? I think that would make it perfectly clear what this refers to.

davidor · 2019-05-29T10:36:00Z

gateway/src/resty/synchronization.lua

+    sema:post()
+  end
+
+  return ret, result


Are 2 enough? According to the docs of the function, execute returns 2 or 3 params depending on whether there was an error or not.

When there's an error, execute returns 3 params and the error msg is the third one, so I think we'll lose it.

APIcast/gateway/src/resty/concurrent/safe_task_executor.lua

Line 39 in 6b42baa

return false, nil, ret[1]

davidor · 2019-05-29T14:52:20Z

t/configuration-loading-lazy.t

@@ -95,3 +96,30 @@ GET /t?user_key=fake
  | ]
 ]

+=== TEST 4: load invalid json
+To validate that process does not died with invalid config


I'm not sure this test is what we need. I checked that it passes without applying the patch. Also, this description seems to contradict the one in TEST 2.

test 2 validates that the test returns a 404, but the json.decode of an empty string is not going to fail. But json decode of an invalid json will break.

Made this test, looks more legitimate and it's fails:

=== TEST 5: Invalid json structure To validate that process does not died if the server return an invalid json configuration --- main_config env THREESCALE_PORTAL_ENDPOINT=http://127.0.0.1:$TEST_NGINX_SERVER_PORT/; env APICAST_CONFIGURATION_LOADER=lazy; --- http_config include $TEST_NGINX_UPSTREAM_CONFIG; lua_package_path "$TEST_NGINX_LUA_PATH"; --- config include $TEST_NGINX_APICAST_CONFIG; include $TEST_NGINX_BACKEND_CONFIG; location = /admin/api/nginx/spec.json { try_files /config.json =404; } location /api/ { echo "all ok"; } --- request GET /t?user_key=fake --- error_code: 404 --- user_files >>> config.json { "services": [{ "id": 1, "backend_version": 1, "proxy": { "api_backend": ["test1", "test2"], "proxy_rules": [ { "pattern": "/t", "http_method": "GET", "metric_system_name": "test" } ] } }] }

davidor · 2019-05-29T14:52:36Z

spec/synchronization.lua

+    local ret, result = synchronization:run(key, 10, callback)
+    assert.same(ret, true)
+    assert.same(result, {1})
+    assert.spy(callback).was.called()


o we need the spies? I think that checking the results of run might be enough.

It's just to make the test completed and easy to understand if someone review the code in future.

davidor · 2019-05-29T14:52:45Z

gateway/src/apicast/configuration_loader.lua

-    sema:post()
-  else
-    ngx.log(ngx.WARN, 'failed to acquire lock to lazy load: ', host, ' error: ', err)
+  local ret, result = synchronization:run(host, 15, lazy.load_config, configuration, host)


Can we define a constant with this 15? I know it's not a problem introduced in this PR, but looks like a good opportunity to fix this.

davidor · 2019-05-29T14:52:57Z

gateway/src/apicast/configuration_loader.lua

@@ -196,33 +196,30 @@ function lazy.init(configuration)
  configuration.configured = true
 end

+function lazy.load_config(configuration, host)


This method is exposed. It should be private, right?

Just checking. Did you forget to change this or decided not to?

davidor · 2019-05-29T14:53:36Z

gateway/src/apicast/configuration_loader.lua

-    ngx.log(ngx.WARN, 'failed to acquire lock to lazy load: ', host, ' error: ', err)
+  local ret, result = synchronization:run(host, 15, lazy.load_config, configuration, host)
+  if not ret then
+    ngx.log(ngx.WARN, 'failed to acquire lock to lazy load: ', host, ' error: ', result)


this error message is also shown in the synchronization module. Maybe here we should show a more specific error.

davidor · 2019-05-29T14:54:29Z

spec/synchronization.lua

+    assert.spy(callback).was_called()
+  end)
+
+  it("Validates that run does not breaks if a execption on the callback", function()


typos: break exception.

davidor · 2019-06-05T13:08:38Z

gateway/src/apicast/configuration_loader.lua

@@ -9,7 +9,7 @@ local oidc_loader = require 'apicast.configuration_loader.oidc'
 local util = require 'apicast.util'
 local env = require('resty.env')
 local resty_url = require('resty.url')
-local synchronization = require('resty.synchronization').new(1)
+local synchronization = require('resty.synchronization').new(nil,1)


Why did this change?

davidor · 2019-06-05T13:24:54Z

gateway/src/resty/synchronization.lua

+  local sema, err = self:acquire(key)
+  if err ~= key then
+    ngx.log(ngx.WARN, 'failed to acquire lock on key: ', key, ' error: ', err)
+    return false, nil


There's no need for the , nil

davidor · 2019-06-06T09:30:26Z

spec/synchronization.lua

+    assert.spy(callback).was_called()
+  end)
+
+  it("Validates that run does not break if a exception on the callback", function()


but there's no exception in this test, right?

davidor · 2019-06-06T09:46:42Z

spec/synchronization.lua

@@ -0,0 +1,78 @@
+describe("Synchronization", function()


this file needs to be named sychronization_spec.lua. Without the _spec our Makefile will not run the test in this file.

This commits add a new function `run` to the synchronization module, that function ensures that a given function is going to be run in a safe task and make sure that the release of the lock always happens, so the lock is not going to be blocked if something goes wrong. Fix 3scale#1010 Fix THREESCALE-2194 Signed-off-by: Eloy Coto <eloy.coto@gmail.com>

Load an invalid json to make sure that the invalid config does not break the thread. Signed-off-by: Eloy Coto <eloy.coto@gmail.com>

Return a error if the url is not valid for oidc_endpoint and make sure that process does not died due invalid url Signed-off-by: Eloy Coto <eloy.coto@gmail.com>

eloycoto requested a review from a team as a code owner May 29, 2019 09:22

eloycoto force-pushed the Issue-1010 branch 2 times, most recently from df8243f to 4c4573f Compare May 29, 2019 09:41

davidor reviewed May 29, 2019

View reviewed changes

eloycoto force-pushed the Issue-1010 branch 4 times, most recently from fdc7f23 to 890dfa2 Compare May 31, 2019 13:36

eloycoto requested a review from davidor June 3, 2019 16:03

davidor reviewed Jun 5, 2019

View reviewed changes

eloycoto force-pushed the Issue-1010 branch 3 times, most recently from 8788a7b to 704f55b Compare June 6, 2019 09:21

eloycoto requested a review from davidor June 6, 2019 09:23

davidor reviewed Jun 6, 2019

View reviewed changes

eloycoto force-pushed the Issue-1010 branch 2 times, most recently from b1a881d to 89cfb7c Compare June 6, 2019 09:58

eloycoto force-pushed the Issue-1010 branch 2 times, most recently from 8cbde7f to 2c03d0c Compare June 6, 2019 11:03

eloycoto added 3 commits June 6, 2019 13:09

[THREESCALE-2194] Lazy configuration integration test.

e14ae25

Load an invalid json to make sure that the invalid config does not break the thread. Signed-off-by: Eloy Coto <eloy.coto@gmail.com>

[Configuration] Fix break when oidc is not a valid target.

1536c4f

Return a error if the url is not valid for oidc_endpoint and make sure that process does not died due invalid url Signed-off-by: Eloy Coto <eloy.coto@gmail.com>

eloycoto force-pushed the Issue-1010 branch from 2c03d0c to 1536c4f Compare June 6, 2019 11:09

davidor approved these changes Jun 6, 2019

View reviewed changes

davidor merged commit d797002 into 3scale:master Jun 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[THREESCALE-2194] Ensure locks are released on configuration_loader. #1050

[THREESCALE-2194] Ensure locks are released on configuration_loader. #1050

eloycoto commented May 29, 2019

davidor May 29, 2019

eloycoto May 29, 2019

davidor May 29, 2019

davidor May 29, 2019

davidor May 29, 2019

davidor May 29, 2019

davidor May 29, 2019

davidor Jun 5, 2019

davidor May 29, 2019

eloycoto May 29, 2019

eloycoto May 29, 2019

davidor May 29, 2019

eloycoto May 29, 2019

davidor Jun 5, 2019

davidor May 29, 2019

davidor May 29, 2019

davidor Jun 5, 2019

davidor May 29, 2019 •

edited

Loading

davidor May 29, 2019

davidor Jun 5, 2019

davidor Jun 5, 2019

davidor Jun 6, 2019

davidor Jun 6, 2019

[THREESCALE-2194] Ensure locks are released on configuration_loader. #1050

[THREESCALE-2194] Ensure locks are released on configuration_loader. #1050

Conversation

eloycoto commented May 29, 2019

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

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

davidor May 29, 2019 • 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

Choose a reason for hiding this comment

davidor May 29, 2019 •

edited

Loading