From 830c95ffba0342f3f2ab53c6ff9d70c631de28bd Mon Sep 17 00:00:00 2001 From: Robert Paprocki Date: Fri, 24 Mar 2017 15:14:04 -0700 Subject: [PATCH] feat(admin) sugar method to delete upstream target --- kong/api/routes/upstreams.lua | 82 ++++++------------- kong/core/balancer.lua | 66 +++++++++++++++ .../03-admin_api/09-targets_routes_spec.lua | 53 ++++++++++++ 3 files changed, 142 insertions(+), 59 deletions(-) diff --git a/kong/api/routes/upstreams.lua b/kong/api/routes/upstreams.lua index 3346bb88757..6bc0f103bc7 100644 --- a/kong/api/routes/upstreams.lua +++ b/kong/api/routes/upstreams.lua @@ -1,6 +1,7 @@ local crud = require "kong.api.crud_helpers" local app_helpers = require "lapis.application" local responses = require "kong.tools.responses" +local balancer = require "kong.core.balancer" return { ["/upstreams/"] = { @@ -46,65 +47,7 @@ return { end, POST = function(self, dao_factory, helpers) - -- when to cleanup: invalid-entries > (valid-ones * cleanup_factor) - local cleanup_factor = 10 - - --cleaning up history, check if it's necessary... - local target_history = dao_factory.targets:find_all( - { upstream_id = self.params.upstream_id }) - - if target_history then --ignoring errors here, will be caught when posting below - -- sort the targets - for _,target in ipairs(target_history) do - target.order = target.created_at..":"..target.id - end - - -- sort table in reverse order - table.sort(target_history, function(a,b) return a.order>b.order end) - -- do clean up - local cleaned = {} - local delete = {} - - for _, entry in ipairs(target_history) do - if cleaned[entry.target] then - -- we got a newer entry for this target than this, so this one can go - delete[#delete+1] = entry - - else - -- haven't got this one, so this is the last one for this target - cleaned[entry.target] = true - cleaned[#cleaned+1] = entry - if entry.weight == 0 then - delete[#delete+1] = entry - end - end - end - - -- do we need to cleanup? - -- either nothing left, or when 10x more outdated than active entries - if (#cleaned == 0 and #delete > 0) or - (#delete >= (math.max(#cleaned,1)*cleanup_factor)) then - - ngx.log(ngx.INFO, "[admin api] Starting cleanup of target table for upstream ", - tostring(self.params.upstream_id)) - local cnt = 0 - for _, entry in ipairs(delete) do - -- not sending update events, one event at the end, based on the - -- post of the new entry should suffice to reload only once - dao_factory.targets:delete( - { id = entry.id }, - { quiet = true } - ) - -- ignoring errors here, deleted by id, so should not matter - -- in case another kong-node does the same cleanup simultaneously - cnt = cnt + 1 - end - - ngx.log(ngx.INFO, "[admin api] Finished cleanup of target table", - " for upstream ", tostring(self.params.upstream_id), - " removed ", tostring(cnt), " target entries") - end - end + balancer.clean_history(self.params.upstream_id, dao_factory) crud.post(self.params, dao_factory.targets) end, @@ -160,4 +103,25 @@ return { end }, + ["/upstreams/:name_or_id/targets/:target"] = { + before = function(self, dao_factory, helpers) + crud.find_upstream_by_name_or_id(self, dao_factory, helpers) + self.params.upstream_id = self.upstream.id + end, + + DELETE = function(self, dao_factory) + balancer.clean_history(self.params.upstream_id, dao_factory) + + -- this is just a wrapper around POSTing a new target with weight=0 + local target = self.params + target.weight = 0 + + local data, err = dao_factory.targets:insert(target) + if err then + return app_helpers.yield_error(err) + end + + return responses.send_HTTP_NO_CONTENT() + end + } } diff --git a/kong/core/balancer.lua b/kong/core/balancer.lua index 30915ae4642..6bc9e343fe9 100644 --- a/kong/core/balancer.lua +++ b/kong/core/balancer.lua @@ -9,6 +9,7 @@ local toip = dns_client.toip local log = ngx.log local ERROR = ngx.ERR +local INFO = ngx.INFO local DEBUG = ngx.DEBUG local EMPTY_T = pl_tablex.readonly {} @@ -58,6 +59,70 @@ local function invalidate_balancer(upstream_name) balancers[upstream_name] = nil end +-- clean the target history for a given upstream +local function clean_history(upstream_id, dao_factory) + -- when to cleanup: invalid-entries > (valid-ones * cleanup_factor) + local cleanup_factor = 10 + + --cleaning up history, check if it's necessary... + local target_history = dao_factory.targets:find_all({ + upstream_id = upstream_id + }) + + if target_history then + -- sort the targets + for _,target in ipairs(target_history) do + target.order = target.created_at..":"..target.id + end + + -- sort table in reverse order + table.sort(target_history, function(a,b) return a.order>b.order end) + -- do clean up + local cleaned = {} + local delete = {} + + for _, entry in ipairs(target_history) do + if cleaned[entry.target] then + -- we got a newer entry for this target than this, so this one can go + delete[#delete+1] = entry + + else + -- haven't got this one, so this is the last one for this target + cleaned[entry.target] = true + cleaned[#cleaned+1] = entry + if entry.weight == 0 then + delete[#delete+1] = entry + end + end + end + + -- do we need to cleanup? + -- either nothing left, or when 10x more outdated than active entries + if (#cleaned == 0 and #delete > 0) or + (#delete >= (math.max(#cleaned,1)*cleanup_factor)) then + + log(INFO, "[admin api] Starting cleanup of target table for upstream ", + tostring(upstream_id)) + local cnt = 0 + for _, entry in ipairs(delete) do + -- not sending update events, one event at the end, based on the + -- post of the new entry should suffice to reload only once + dao_factory.targets:delete( + { id = entry.id }, + { quiet = true } + ) + -- ignoring errors here, deleted by id, so should not matter + -- in case another kong-node does the same cleanup simultaneously + cnt = cnt + 1 + end + + log(INFO, "[admin api] Finished cleanup of target table", + " for upstream ", tostring(upstream_id), + " removed ", tostring(cnt), " target entries") + end + end +end + -- loads a single upstream entity local function load_upstream_into_memory(upstream_id) log(DEBUG, "fetching upstream: ", tostring(upstream_id)) @@ -315,6 +380,7 @@ local function execute(target) end return { + clean_history = clean_history, execute = execute, invalidate_balancer = invalidate_balancer, diff --git a/spec/02-integration/03-admin_api/09-targets_routes_spec.lua b/spec/02-integration/03-admin_api/09-targets_routes_spec.lua index 41f780907b4..18806d8aa28 100644 --- a/spec/02-integration/03-admin_api/09-targets_routes_spec.lua +++ b/spec/02-integration/03-admin_api/09-targets_routes_spec.lua @@ -320,4 +320,57 @@ describe("Admin API", function() end) end) end) + + describe("/upstreams/{upstream}/targets/{target}", function() + describe("DELETE", function() + local target + local upstream_name4 = "example4.com" + + before_each(function() + local upstream4 = assert(helpers.dao.upstreams:insert { + name = upstream_name4, + }) + + assert(helpers.dao.targets:insert { + target = "api-1:80", + weight = 10, + upstream_id = upstream4.id, + }) + + -- predefine the target to mock delete + target = assert(helpers.dao.targets:insert { + target = "api-2:80", + weight = 10, + upstream_id = upstream4.id, + }) + end) + + it("acts as a sugar method to POST a target with 0 weight", function() + local res = assert(client:send { + method = "DELETE", + path = "/upstreams/" .. upstream_name4 .. "/targets/" .. target.target + }) + assert.response(res).has.status(204) + + local targets = assert(client:send { + method = "GET", + path = "/upstreams/" .. upstream_name4 .. "/targets/", + }) + assert.response(targets).has.status(200) + local json = assert.response(targets).has.jsonbody() + assert.equal(3, #json.data) + assert.equal(3, json.total) + + local active = assert(client:send { + method = "GET", + path = "/upstreams/" .. upstream_name4 .. "/targets/active/", + }) + assert.response(active).has.status(200) + json = assert.response(active).has.jsonbody() + assert.equal(1, #json.data) + assert.equal(1, json.total) + assert.equal("api-1:80", json.data[1].target) + end) + end) + end) end)