From bb207f69abc03f0f06573dbfc08434c179539841 Mon Sep 17 00:00:00 2001 From: Thijs Schreijer Date: Fri, 28 Jul 2017 23:06:39 +0200 Subject: [PATCH] refactor(balancer) drop the orderlist property --- kong-0.11.0-0.rockspec | 2 +- kong/core/balancer.lua | 2 - kong/dao/migrations/cassandra.lua | 7 + kong/dao/migrations/postgres.lua | 7 + kong/dao/schemas/upstreams.lua | 60 ------- kong/init.lua | 26 +-- spec/01-unit/007-entities_schemas_spec.lua | 89 --------- .../04-admin_api/07-upstreams_routes_spec.lua | 170 ++++-------------- .../04-admin_api/08-targets_routes_spec.lua | 2 - .../05-proxy/09-balancer_spec.lua | 24 ++- 10 files changed, 91 insertions(+), 298 deletions(-) diff --git a/kong-0.11.0-0.rockspec b/kong-0.11.0-0.rockspec index 2d820b37704..133a0264de9 100644 --- a/kong-0.11.0-0.rockspec +++ b/kong-0.11.0-0.rockspec @@ -27,7 +27,7 @@ dependencies = { "luacrypto == 0.3.2", "luasyslog == 1.0.0", "lua_pack == 1.0.5", - "lua-resty-dns-client == 0.6.0", + "lua-resty-dns-client == 0.6.2", "lua-resty-worker-events == 0.3.0", "lua-resty-mediador == 0.1.2", } diff --git a/kong/core/balancer.lua b/kong/core/balancer.lua index ae57205ed82..47edf076bdf 100644 --- a/kong/core/balancer.lua +++ b/kong/core/balancer.lua @@ -176,7 +176,6 @@ local get_balancer = function(target) -- no balancer yet (or invalidated) so create a new one balancer, err = ring_balancer.new({ wheelSize = upstream.slots, - order = upstream.orderlist, dns = dns_client, }) @@ -222,7 +221,6 @@ local get_balancer = function(target) -- for now; create a new balancer from scratch balancer, err = ring_balancer.new({ wheelSize = upstream.slots, - order = upstream.orderlist, dns = dns_client, }) if not balancer then diff --git a/kong/dao/migrations/cassandra.lua b/kong/dao/migrations/cassandra.lua index 717f57b5673..6b7a6f1f388 100644 --- a/kong/dao/migrations/cassandra.lua +++ b/kong/dao/migrations/cassandra.lua @@ -474,4 +474,11 @@ return { DROP TABLE nodes; ]], }, + { + name = "2017-07-28-225000_balancer_orderlist_remove", + up = [[ + ALTER TABLE upstreams DROP orderlist; + ]], + down = function(_, _, dao) end -- not implemented + }, } diff --git a/kong/dao/migrations/postgres.lua b/kong/dao/migrations/postgres.lua index cebc99f8377..a30b3bef708 100644 --- a/kong/dao/migrations/postgres.lua +++ b/kong/dao/migrations/postgres.lua @@ -526,4 +526,11 @@ return { DROP INDEX ttls_primary_uuid_value_idx; ]] }, + { + name = "2017-07-28-225000_balancer_orderlist_remove", + up = [[ + ALTER TABLE upstreams DROP COLUMN IF EXISTS orderlist; + ]], + down = function(_, _, dao) end -- not implemented + }, } diff --git a/kong/dao/schemas/upstreams.lua b/kong/dao/schemas/upstreams.lua index 8059db905f1..c40123665ee 100644 --- a/kong/dao/schemas/upstreams.lua +++ b/kong/dao/schemas/upstreams.lua @@ -31,13 +31,6 @@ return { type = "number", default = DEFAULT_SLOTS, }, - orderlist = { - -- a list of sequential, but randomly ordered, integer numbers. In the datastore - -- because all Kong nodes need the exact-same 'randomness'. If changed, consistency is lost. - -- must have exactly `slots` number of entries. - type = "array", - default = {}, - } }, self_check = function(schema, config, dao, is_updating) @@ -58,59 +51,6 @@ return { return false, Errors.schema(SLOTS_MSG) end - -- check the order array - local order = config.orderlist - if #order == config.slots then - -- array size unchanged, check consistency - - local t = utils.shallow_copy(order) - table.sort(t) - local count, max = 0, 0 - for i, v in pairs(t) do - if i ~= v then - return false, Errors.schema("invalid orderlist") - end - - count = count + 1 - if i > max then - max = i - end - end - - if count ~= config.slots or max ~= config.slots then - return false, Errors.schema("invalid orderlist") - end - - else - -- size mismatch - if #order > 0 then - -- size given, but doesn't match the size of the also given orderlist - return false, Errors.schema("size mismatch between 'slots' and 'orderlist'") - end - - -- No list given, generate order array - local t = {} - for i = 1, config.slots do - t[i] = { - id = i, - order = math.random(1, config.slots), - } - end - - -- sort the array (we don't check for -accidental- duplicates as the - -- id field is used for the order and that one is always unique) - table.sort(t, function(a,b) - return a.order < b.order - end) - - -- replace the created 'record' with only the id - for i, v in ipairs(t) do - t[i] = v.id - end - - config.orderlist = t - end - return true end, } diff --git a/kong/init.lua b/kong/init.lua index 8440d01c378..ea075947aa1 100644 --- a/kong/init.lua +++ b/kong/init.lua @@ -62,6 +62,10 @@ local kong_error_handlers = require "kong.core.error_handlers" local ngx = ngx local header = ngx.header +local ngx_log = ngx.log +local ngx_ERR = ngx.ERR +local ngx_CRIT = ngx.CRIT +local ngx_DEBUG = ngx.DEBUG local ipairs = ipairs local assert = assert local tostring = tostring @@ -73,7 +77,7 @@ local set_more_tries = ngx_balancer.set_more_tries local function load_plugins(kong_conf, dao) local in_db_plugins, sorted_plugins = {}, {} - ngx.log(ngx.DEBUG, "Discovering used plugins") + ngx_log(ngx_DEBUG, "Discovering used plugins") local rows, err_t = dao.plugins:find_all() if not rows then @@ -101,7 +105,7 @@ local function load_plugins(kong_conf, dao) return nil, "no configuration schema found for plugin: " .. plugin end - ngx.log(ngx.DEBUG, "Loading plugin: " .. plugin) + ngx_log(ngx_DEBUG, "Loading plugin: " .. plugin) sorted_plugins[#sorted_plugins+1] = { name = plugin, @@ -171,7 +175,7 @@ function Kong.init_worker() local ok, err = singletons.dao:init_worker() if not ok then - ngx.log(ngx.CRIT, "could not init DB: ", err) + ngx_log(ngx_CRIT, "could not init DB: ", err) return end @@ -191,7 +195,7 @@ function Kong.init_worker() wait_max = 0.5, -- max wait time before discarding event } if not ok then - ngx.log(ngx.CRIT, "could not start inter-worker events: ", err) + ngx_log(ngx_CRIT, "could not start inter-worker events: ", err) return end @@ -209,7 +213,7 @@ function Kong.init_worker() poll_offset = configuration.db_update_propagation, } if not cluster_events then - ngx.log(ngx.CRIT, "could not create cluster_events: ", err) + ngx_log(ngx_CRIT, "could not create cluster_events: ", err) return end @@ -229,7 +233,7 @@ function Kong.init_worker() }, } if not cache then - ngx.log(ngx.CRIT, "could not create kong cache: ", err) + ngx_log(ngx_CRIT, "could not create kong cache: ", err) return end @@ -237,7 +241,7 @@ function Kong.init_worker() return "init" end) if not ok then - ngx.log(ngx.CRIT, "could not set router version in cache: ", err) + ngx_log(ngx_CRIT, "could not set router version in cache: ", err) return end @@ -291,7 +295,7 @@ function Kong.balancer() local ok, err = balancer_execute(addr) if not ok then - ngx.log(ngx.ERR, "failed to retry the dns/balancer resolver for ", + ngx_log(ngx_ERR, "failed to retry the dns/balancer resolver for ", addr.upstream.host, "' with: ", tostring(err)) return responses.send(500) @@ -309,9 +313,11 @@ function Kong.balancer() current_try.port = addr.port -- set the targets as resolved + ngx_log(ngx_DEBUG, "setting address (try ", addr.try_count, "): ", + addr.ip, ":", addr.port) local ok, err = set_current_peer(addr.ip, addr.port) if not ok then - ngx.log(ngx.ERR, "failed to set the current peer (address: ", + ngx_log(ngx_ERR, "failed to set the current peer (address: ", tostring(addr.ip), " port: ", tostring(addr.port),"): ", tostring(err)) @@ -322,7 +328,7 @@ function Kong.balancer() addr.send_timeout / 1000, addr.read_timeout / 1000) if not ok then - ngx.log(ngx.ERR, "could not set upstream timeouts: ", err) + ngx_log(ngx_ERR, "could not set upstream timeouts: ", err) end core.balancer.after() diff --git a/spec/01-unit/007-entities_schemas_spec.lua b/spec/01-unit/007-entities_schemas_spec.lua index 93f7eb6939c..b6998730b1f 100644 --- a/spec/01-unit/007-entities_schemas_spec.lua +++ b/spec/01-unit/007-entities_schemas_spec.lua @@ -5,7 +5,6 @@ local targets_schema = require "kong.dao.schemas.targets" local upstreams_schema = require "kong.dao.schemas.upstreams" local validations = require "kong.dao.schemas_validation" local validate_entity = validations.validate_entity -local utils = require "kong.tools.utils" describe("Entities Schemas", function() @@ -775,94 +774,6 @@ describe("Entities Schemas", function() end end) - it("should require (optional) orderlist to be a proper list", function() - local data, valid, errors, check - local function validate_order(list, size) - assert(type(list) == "table", "expected list table, got " .. type(list)) - assert(next(list), "table is empty") - assert(type(size) == "number", "expected size number, got " .. type(size)) - assert(size > 0, "expected size to be > 0") - local c = {} - local max = 0 - for i,v in pairs(list) do --> note: pairs, not ipairs!! - if i > max then max = i end - c[i] = v - end - assert(max == size, "highest key is not equal to the size") - table.sort(c) - max = 0 - for i, v in ipairs(c) do - assert(i == v, "expected sorted table to have equal keys and values") - if i>max then max = i end - end - assert(max == size, "expected array, but got list with holes") - end - - for _ = 1, 20 do -- have Kong generate 20 random sized arrays and verify them - data = { - name = "valid.host.name", - slots = math.random(slots_min, slots_max) - } - valid, errors, check = validate_entity(data, upstreams_schema) - assert.is_true(valid) - assert.is_nil(errors) - assert.is_nil(check) - validate_order(data.orderlist, data.slots) - end - - local lst = { 9,7,5,3,1,2,4,6,8,10 } -- a valid list - data = { - name = "valid.host.name", - slots = 10, - orderlist = utils.shallow_copy(lst) - } - valid, errors, check = validate_entity(data, upstreams_schema) - assert.is_true(valid) - assert.is_nil(errors) - assert.is_nil(check) - assert.same(lst, data.orderlist) - - data = { - name = "valid.host.name", - slots = 10, - orderlist = { 9,7,5,3,1,2,4,6,8 } -- too short (9) - } - valid, errors, check = validate_entity(data, upstreams_schema) - assert.is_false(valid) - assert.is_nil(errors) - assert.are.equal("size mismatch between 'slots' and 'orderlist'",check.message) - - data = { - name = "valid.host.name", - slots = 10, - orderlist = { 9,7,5,3,1,2,4,6,8,10,11 } -- too long (11) - } - valid, errors, check = validate_entity(data, upstreams_schema) - assert.is_false(valid) - assert.is_nil(errors) - assert.are.equal("size mismatch between 'slots' and 'orderlist'",check.message) - - data = { - name = "valid.host.name", - slots = 10, - orderlist = { 9,7,5,3,1,2,4,6,8,8 } -- a double value (2x 8, no 10) - } - valid, errors, check = validate_entity(data, upstreams_schema) - assert.is_false(valid) - assert.is_nil(errors) - assert.are.equal("invalid orderlist",check.message) - - data = { - name = "valid.host.name", - slots = 10, - orderlist = { 9,7,5,3,1,2,4,6,8,11 } -- a hole (10 missing) - } - valid, errors, check = validate_entity(data, upstreams_schema) - assert.is_false(valid) - assert.is_nil(errors) - assert.are.equal("invalid orderlist",check.message) - end) - end) -- diff --git a/spec/02-integration/04-admin_api/07-upstreams_routes_spec.lua b/spec/02-integration/04-admin_api/07-upstreams_routes_spec.lua index 52e1692e1ba..37ac76557d1 100644 --- a/spec/02-integration/04-admin_api/07-upstreams_routes_spec.lua +++ b/spec/02-integration/04-admin_api/07-upstreams_routes_spec.lua @@ -12,27 +12,6 @@ local function it_content_types(title, fn) it(title .. " with application/json", test_json) end -local function validate_order(list, size) - assert(type(list) == "table", "expected list table, got " .. type(list)) - assert(next(list), "table is empty") - assert(type(size) == "number", "expected size number, got " .. type(size)) - assert(size > 0, "expected size to be > 0") - local c = {} - local max = 0 - for i,v in pairs(list) do --> note: pairs, not ipairs!! - if i > max then max = i end - c[i] = v - end - assert(max == size, "highest key is not equal to the size") - table.sort(c) - max = 0 - for i, v in ipairs(c) do - assert(i == v, "expected sorted table to have equal keys and values") - if i>max then max = i end - end - assert(max == size, "expected array, but got list with holes") -end - dao_helpers.for_each_dao(function(kong_config) describe("Admin API: #" .. kong_config.database, function() @@ -41,6 +20,7 @@ describe("Admin API: #" .. kong_config.database, function() setup(function() dao = assert(DAOFactory.new(kong_config)) + helpers.run_migrations(dao) helpers.run_migrations(dao) assert(helpers.start_kong{ @@ -75,67 +55,45 @@ describe("Admin API: #" .. kong_config.database, function() assert.is_number(json.created_at) assert.is_string(json.id) assert.are.equal(slots_default, json.slots) - validate_order(json.orderlist, json.slots) end end) - it("creates an upstream without defaults with application/json", function() - local res = assert(client:send { - method = "POST", - path = "/upstreams", - body = { - name = "my.upstream", - slots = 10, - orderlist = { 10,9,8,7,6,5,4,3,2,1 }, - }, - headers = {["Content-Type"] = "application/json"} - }) - assert.response(res).has.status(201) - local json = assert.response(res).has.jsonbody() - assert.equal("my.upstream", json.name) - assert.is_number(json.created_at) - assert.is_string(json.id) - assert.are.equal(10, json.slots) - validate_order(json.orderlist, json.slots) - assert.are.same({ 10,9,8,7,6,5,4,3,2,1 }, json.orderlist) - end) - pending("creates an upstream without defaults with application/www-form-urlencoded", function() --- pending due to inability to pass array --- see also the todo's below - local res = assert(client:send { - method = "POST", - path = "/upstreams", - body = "name=my.upstream&slots=10&" .. - "orderlist[]=10&orderlist[]=9&orderlist[]=8&orderlist[]=7&" .. - "orderlist[]=6&orderlist[]=5&orderlist[]=4&orderlist[]=3&" .. - "orderlist[]=2&orderlist[]=1", - headers = {["Content-Type"] = "application/www-form-urlencoded"} - }) - assert.response(res).has.status(201) - local json = assert.response(res).has.jsonbody() - assert.equal("my.upstream", json.name) - assert.is_number(json.created_at) - assert.is_string(json.id) - assert.are.equal(10, json.slots) - validate_order(json.orderlist, json.slots) - assert.are.same({ 10,9,8,7,6,5,4,3,2,1 }, json.orderlist) + it_content_types("creates an upstream without defaults with application/json", function(content_type) + return function() + local res = assert(client:send { + method = "POST", + path = "/upstreams", + body = { + name = "my.upstream", + slots = 10, + }, + headers = {["Content-Type"] = content_type} + }) + assert.response(res).has.status(201) + local json = assert.response(res).has.jsonbody() + assert.equal("my.upstream", json.name) + assert.is_number(json.created_at) + assert.is_string(json.id) + assert.are.equal(10, json.slots) + end end) - it("creates an upstream with " .. slots_max .. " slots", function(content_type) - local res = assert(client:send { - method = "POST", - path = "/upstreams", - body = { - name = "my.upstream", - slots = slots_max, - }, - headers = {["Content-Type"] = "application/json"} - }) - assert.response(res).has.status(201) - local json = assert.response(res).has.jsonbody() - assert.equal("my.upstream", json.name) - assert.is_number(json.created_at) - assert.is_string(json.id) - assert.are.equal(slots_max, json.slots) - validate_order(json.orderlist, json.slots) + it_content_types("creates an upstream with " .. slots_max .. " slots", function(content_type) + return function() + local res = assert(client:send { + method = "POST", + path = "/upstreams", + body = { + name = "my.upstream", + slots = slots_max, + }, + headers = {["Content-Type"] = content_type} + }) + assert.response(res).has.status(201) + local json = assert.response(res).has.jsonbody() + assert.equal("my.upstream", json.name) + assert.is_number(json.created_at) + assert.is_string(json.id) + assert.are.equal(slots_max, json.slots) + end end) describe("errors", function() it("handles malformed JSON body", function() @@ -190,54 +148,6 @@ describe("Admin API: #" .. kong_config.database, function() assert.same({ message = "number of slots must be between 10 and 65536" }, json) end end) - it_content_types("handles invalid input - orderlist", function(content_type) - return function() ---TODO: line below disables the test for urlencoded, because the orderlist array isn't passed/received properly -if content_type == "application/x-www-form-urlencoded" then return end - -- non-integers - local res = assert(client:send { - method = "POST", - path = "/upstreams", - body = { - name = "my.upstream", - slots = 10, - orderlist = { "one","two","three","four","five","six","seven","eight","nine","ten" }, - }, - headers = {["Content-Type"] = content_type} - }) - local body = assert.res_status(400, res) - local json = cjson.decode(body) - assert.same({ message = "invalid orderlist" }, json) - -- non-consecutive - res = assert(client:send { - method = "POST", - path = "/upstreams", - body = { - name = "my.upstream", - slots = 10, - orderlist = { 1,2,3,4,5,6,7,8,9,11 }, -- 10 is missing - }, - headers = {["Content-Type"] = content_type} - }) - body = assert.res_status(400, res) - local json = cjson.decode(body) - assert.same({ message = "invalid orderlist" }, json) - -- doubles - res = assert(client:send { - method = "POST", - path = "/upstreams", - body = { - name = "my.upstream", - slots = 10, - orderlist = { 1,2,3,4,5,1,2,3,4,5 }, - }, - headers = {["Content-Type"] = content_type} - }) - body = assert.res_status(400, res) - local json = cjson.decode(body) - assert.same({ message = "invalid orderlist" }, json) - end - end) it_content_types("returns 409 on conflict", function(content_type) return function() local res = assert(client:send { @@ -288,12 +198,9 @@ if content_type == "application/x-www-form-urlencoded" then return end assert.is_number(json.created_at) assert.is_string(json.id) assert.is_number(json.slots) - assert.is_table(json.orderlist) end end) - --it_content_types("replaces if exists", function(content_type) - pending("replaces if exists", function(content_type) ---TODO: no idea why this fails in an odd manner... + it_content_types("replaces if exists", function(content_type) return function() local res = assert(client:send { method = "POST", @@ -323,7 +230,6 @@ if content_type == "application/x-www-form-urlencoded" then return end assert.equal("my-new-upstream", updated_json.name) assert.equal(123, updated_json.slots) assert.equal(json.id, updated_json.id) - assert.equal(json.created_at, updated_json.created_at) end end) describe("errors", function() diff --git a/spec/02-integration/04-admin_api/08-targets_routes_spec.lua b/spec/02-integration/04-admin_api/08-targets_routes_spec.lua index 3a6ac35f9ba..fed8bc4b9da 100644 --- a/spec/02-integration/04-admin_api/08-targets_routes_spec.lua +++ b/spec/02-integration/04-admin_api/08-targets_routes_spec.lua @@ -24,7 +24,6 @@ describe("Admin API", function() upstream = assert(helpers.dao.upstreams:insert { name = upstream_name, slots = 10, - orderlist = { 1,2,3,4,5,6,7,8,9,10 } }) end) @@ -250,7 +249,6 @@ describe("Admin API", function() assert(helpers.dao.upstreams:insert { name = upstream_name2, slots = 10, - orderlist = { 1,2,3,4,5,6,7,8,9,10 } }) end) diff --git a/spec/02-integration/05-proxy/09-balancer_spec.lua b/spec/02-integration/05-proxy/09-balancer_spec.lua index 6bc94631f17..c38a4f083e0 100644 --- a/spec/02-integration/05-proxy/09-balancer_spec.lua +++ b/spec/02-integration/05-proxy/09-balancer_spec.lua @@ -5,13 +5,28 @@ local helpers = require "spec.helpers" local dao_helpers = require "spec.02-integration.03-dao.helpers" local PORT = 21000 +local TEST_LOG = false -- extra verbose logging of test server + -- modified http-server. Accepts (sequentially) a number of incoming -- connections, and returns the number of succesful ones. -- Also features a timeout setting. local function http_server(timeout, count, port, ...) local threads = require "llthreads2.ex" local thread = threads.new({ - function(timeout, count, port) + function(timeout, count, port, TEST_LOG) + + local function test_log(...) + if not TEST_LOG then + return + end + + local t = { n = select( "#", ...), ...} + for i, v in ipairs(t) do + t[i] = tostring(v) + end + print(table.concat(t)) + end + local socket = require "socket" local server = assert(socket.tcp()) assert(server:setoption('reuseaddr', true)) @@ -20,6 +35,7 @@ local function http_server(timeout, count, port, ...) local expire = socket.gettime() + timeout assert(server:settimeout(0.1)) + test_log("test http server on port ", port, " started") local success = 0 while count > 0 do @@ -58,13 +74,16 @@ local function http_server(timeout, count, port, ...) if s then success = success + 1 end + test_log("test http server on port ", port, ": ", success, "/", + (success + count)," requests handled") end end server:close() + test_log("test http server on port ", port, " closed") return success end - }, timeout, count, port) + }, timeout, count, port, TEST_LOG) local server = thread:start(...) ngx.sleep(0.2) -- attempt to make sure server is started for failing CI tests @@ -80,6 +99,7 @@ dao_helpers.for_each_dao(function(kong_config) helpers.run_migrations() config_db = helpers.test_conf.database helpers.test_conf.database = kong_config.database + helpers.run_migrations() end) teardown(function() helpers.test_conf.database = config_db