Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support weighting #7

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,17 @@

[![Build Status](https://travis-ci.org/xytis/lua-consul-balancer.svg?branch=master)](https://travis-ci.org/xytis/lua-consul-balancer)

Consul enabled upstream balancer. Does exactly what is advertised -- enables nginx to use consul service discovery to forward requests to dynamic upstreams.
Consul enabled upstream balancer. Does exactly what is advertised -- enables nginx to use consul service discovery to forward requests to dynamic upstreams, and upstream servers support weighting(consul version must >= 1.2.3).

## Usage

Each nginx worker must initialize the library:

lua_shared_dict consul_balancer 4m;
lua_package_path '/path/to/lua-consul-balancer/lib/?.lua;/path/to/lua-resty-http/lib/?.lua;/path/to/lua-resty-balancer/lib/?.lua;;';
lua_package_cpath '/path/to/lua-resty-balancer/?.so;;';

init_worker_by_lua_block {
local consul_balancer = require "n4l.consul_balancer"
consul_balancer.set_shared_dict_name("consul_balancer") # name of shared dictionary to keep cache in
consul_balancer.watch("http://127.0.0.1:8500", {"foo", "bar"})
}

Expand Down
70 changes: 29 additions & 41 deletions lib/n4l/consul_balancer.lua
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@
--
--
--
local next = next

-- Dependencies
local http = require "resty.http"
local balancer = require "ngx.balancer"
local json = require "cjson"
local resty_roundrobin = require "resty.roundrobin"

local WATCH_RETRY_TIMER = 0.5

Expand All @@ -18,7 +20,7 @@ end
local _M = new_tab(0, 5) -- Change the second number.

_M.VERSION = "0.04"
_M._cache = {}
_M._cache = {} -- To save the "service" object

local function _sanitize_uri(consul_uri)
-- TODO: Ensure that uri has <proto>://<host>[:<port>] scheme
Expand Down Expand Up @@ -64,28 +66,35 @@ local function _parse_service(response)
if passing then
local s = v["Service"]
local na = v["Node"]["Address"]
table.insert(service.upstreams, {
address = s["Address"] ~= "" and s["Address"] or na,
port = s["Port"],
})
local address = s["Address"] ~= "" and s["Address"] or na
local port = s["Port"]
-- If the weight parameter fails to be obtained or is passed incorrectly, set it to 1
local weight = s["Weights"]["Passing"] or 1
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lookup may unwind the stack if response structure does not contain 'Weight' key.

I could not determine which consul version added this field into the API response.
If this key aways exist, then the error handling part of the following code is unnecessary.
If this key exists only after some Consul version, then all clients using earlier version will stop functioning permanently.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to solve the consul issues #1088 and 4198, the Weights field was added in cosnul 1.2.3 version,See consul-1.2.3 AgentWeights and #4468.

Also, When I tested with Consul v1.4.0, the response will always have the Weights field by default, whether or not I passed in the Weights field when I signed up for the service.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then just as a precaution, slap in Readme that this new code only works with Consul >1.2.3 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll add dependencies later.

if type(weight) == "number" and weight > 0 then
service.upstreams[address .. ":" .. port] = weight
ngx.log(ngx.INFO, "consul.balancer: add " .. address .. ":" .. port .. " to upstreams, weight " .. weight)
else
ngx.log(ngx.ALERT, "consul.balancer: upstream " .. address .. ":" .. port .. " weight set error:" .. weight)
end
end
end
if service.upstreams ==nil or next(service.upstreams) ==nil then
ngx.log(ngx.ERR, "[FATAL] consul.balancer: upstream list is nil")
return nil, "upstream list is nil"
end
local rr_up = resty_roundrobin:new(service.upstreams)
service.rr_up = rr_up
return service
end

local function _persist(service_name, service)
if _M.shared_cache then
_M.shared_cache:set(service_name, json.encode(service))
return
end
-- Shared cache requires encode & decode to store data. The "find" method of rr_up objects will be lost.
-- Therefore, the rr_up objects are stored directly using table.
_M._cache[service_name] = service
end

local function _aquire(service_name)
if _M.shared_cache then
local service_json = _M.shared_cache:get(service_name)
return service_json and json.decode(service_json) or nil
end
-- When using table to store an rr_up object, simply return it
return _M._cache[service_name]
end

Expand Down Expand Up @@ -162,7 +171,7 @@ local function _watch(premature, service_descriptor)
hc:set_timeout(360000) -- consul api has a default of 5 minutes for tcp long poll
local service_index = 0
ngx.log(ngx.NOTICE, "consul.balancer: started watching for changes in ", service_descriptor.name)
while true do
while not ngx.worker.exiting() do
local uri = _build_service_uri(service_descriptor, service_index)
local service, err = _refresh(hc, uri)
if service == nil then
Expand All @@ -179,10 +188,7 @@ local function _watch(premature, service_descriptor)
end

function _M.watch(consul_uri, service_list)
-- start watching on first worker only, skip for others (if shared storage provided)
if _M.shared_cache and ngx.worker.id() > 0 then
return
end
-- Each worker process is independent and independently watches consul upstream changes
-- TODO: Reconsider scope for this variable.
_M._consul_uri = _sanitize_uri(consul_uri)
for k,v in pairs(service_list) do
Expand All @@ -196,35 +202,17 @@ function _M.round_robin(service_name)
ngx.log(ngx.ERR, "consul.balancer: no entry found for service: ", service_name)
return ngx.exit(500)
end
if service.upstreams == nil or #service.upstreams == 0 then
ngx.log(ngx.ERR, "consul.balancer: no peers for service: ", service_name)
local rr_up = service.rr_up
if rr_up == nil then
ngx.log(ngx.ERR, "consul.balancer: no roundrobin object for service: ", service_name)
return ngx.exit(500)
end
if service.state == nil or service.state > #service.upstreams then
service.state = 1
end
-- TODO: https://github.com/openresty/lua-resty-core/blob/master/lib/ngx/balancer.md#get_last_failure
-- set max tries only at first attempt
if not balancer.get_last_failure() then
balancer.set_more_tries(#service.upstreams - 1)
end
-- Picking next upstream
local upstream = service.upstreams[service.state]
service.state = service.state + 1
_persist(service_name, service)
local ok, err = balancer.set_current_peer(upstream["address"], upstream["port"])
local server = rr_up:find()
local ok, err = balancer.set_current_peer(server)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not find firm proof that this function behaves correctly if passed a string containing ':' separated host/port pair.

I checked here: https://github.com/openresty/lua-resty-core/blob/master/lib/ngx/balancer.md#set_current_peer
and here: https://github.com/openresty/lua-resty-core/blob/master/lib/ngx/balancer.lua#L109

Please verify this by testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The find() method does return a string of ip:port, and as you can see from the lua-resty-balancer example, the ip:port string can be passed directly to the set_current_peer() method as an argument.

After testing, it is feasible, it is possible that the set_current_peer() method is internally compatible.

if not ok then
ngx.log(ngx.ERR, "consul.balancer: failed to set the current peer: ", err)
return ngx.exit(500)
end
end

function _M.set_shared_dict_name(dict_name)
_M.shared_cache = ngx.shared[dict_name]
if not _M.shared_cache then
ngx.log(ngx.ERR, "consul.balancer: unable to access shared dict ", dict_name)
return ngx.exit(ngx.ERROR)
end
end

return _M
4 changes: 2 additions & 2 deletions t/test_shared.t
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ our $HttpConfig = qq{
lua_package_path "$pwd/lib/?.lua;/usr/local/lib/lua/?.lua;;";
error_log logs/error.log debug;

lua_shared_dict consul_balancer 16k;
#lua_shared_dict consul_balancer 16k;

init_by_lua_block {
if $ENV{TEST_COVERAGE} == 1 then
Expand All @@ -23,7 +23,7 @@ our $HttpConfig = qq{

init_worker_by_lua_block {
local consul_balancer = require "n4l.consul_balancer"
consul_balancer.set_shared_dict_name("consul_balancer")
--consul_balancer.set_shared_dict_name("consul_balancer")
consul_balancer.watch("http://127.0.0.1:8500", {
"foo",
{ name = "bar", service = "bar" },
Expand Down