Skip to content

Commit 0181f24

Browse files
themilchenkothemilchenko
committed
api: fix address already in use error
There was a bug when user wants to change server's `listen` address and reload config on the fly - error `address already in use` occures. This patch reworks server's address handling: now it checks if the new config contains existing servers, and stops unused ones if not. Closes #34
1 parent 3396bc0 commit 0181f24

File tree

4 files changed

+276
-38
lines changed

4 files changed

+276
-38
lines changed

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1111

1212
### Fixed
1313

14+
- `Address already in use` error on change role's config on the fly (#34).
15+
1416
### Changed
1517

1618
- Unclear error message when `roles.httpd` config is not applied yet (#33).

‎roles/metrics-export.lua‎

Lines changed: 66 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -322,15 +322,67 @@ local function routes_equal(old, new)
322322
return true
323323
end
324324

325+
local function stop_server(name)
326+
local server = (http_servers or {})[name]
327+
328+
if server ~= nil then
329+
if server.httpd_name ~= nil then
330+
for path, _ in pairs(server.routes) do
331+
server.httpd:delete(path)
332+
end
333+
else
334+
if server.httpd.is_run == true then
335+
server.httpd:stop()
336+
end
337+
end
338+
339+
http_servers[name] = nil
340+
end
341+
end
342+
325343
local function apply_http(conf)
326-
local enabled = {}
344+
-- Stop old servers.
345+
for name in pairs(http_servers or {}) do
346+
local server_found = false
347+
348+
for _, node in pairs(conf) do
349+
if #(node.endpoints or {}) > 0 then
350+
local key
351+
352+
if node.server ~= nil then
353+
key = 'httpd_' .. node.server
354+
elseif node.listen ~= nil then
355+
local host, port, err = parse_listen(node.listen)
356+
if err ~= nil then
357+
error("failed to parse URI: " .. err, 2)
358+
end
359+
360+
key = 'listen_' .. host .. ':' .. tostring(port)
361+
else
362+
key = 'httpd_' .. httpd_role.DEFAULT_SERVER_NAME
363+
end
364+
365+
if name == key then
366+
server_found = true
367+
break
368+
end
369+
end
370+
end
371+
372+
373+
if not server_found then
374+
stop_server(name)
375+
end
376+
end
377+
378+
-- Creating and editing existing ones.
327379
for _, node in ipairs(conf) do
328380
if #(node.endpoints or {}) > 0 then
329381
local host, port, target
330382
if node.server ~= nil then
331383
target = {
332-
value = node.server,
333-
is_httpd_role = true,
384+
value = 'httpd_' .. node.server,
385+
httpd_name = node.server,
334386
}
335387
elseif node.listen ~= nil then
336388
local err
@@ -339,23 +391,18 @@ local function apply_http(conf)
339391
error("failed to parse URI: " .. err, 2)
340392
end
341393
target = {
342-
value = node.listen,
343-
is_httpd_role = false,
394+
value = 'listen_' .. host .. ':' .. tostring(port),
344395
}
345396
else
346397
target = {
347-
value = httpd_role.DEFAULT_SERVER_NAME,
348-
is_httpd_role = true,
398+
value = 'httpd_' .. httpd_role.DEFAULT_SERVER_NAME,
399+
httpd_name = httpd_role.DEFAULT_SERVER_NAME,
349400
}
350401
end
351402

352403
http_servers = http_servers or {}
353-
-- Since the 'listen' and 'server' names of other servers in the config may be
354-
-- the same, we create a unique string concatenating the key name and information
355-
-- about whether it is an httpd key or not.
356-
enabled[tostring(target.value) .. tostring(target.is_httpd_role)] = true
357404

358-
if http_servers[tostring(target.value) .. tostring(target.is_httpd_role)] == nil then
405+
if http_servers[target.value] == nil then
359406
local httpd
360407
if node.listen ~= nil then
361408
httpd = http_server.new(host, port, {
@@ -368,21 +415,21 @@ local function apply_http(conf)
368415
})
369416
httpd:start()
370417
else
371-
httpd = httpd_role.get_server(target.value)
418+
httpd = httpd_role.get_server(target.httpd_name)
372419
if httpd == nil then
373420
error(('failed to get server by name %q, check that roles.httpd was' ..
374-
' already applied'):format(target.value))
421+
' already applied'):format(target.httpd_name))
375422
end
376423
end
377424

378-
http_servers[tostring(target.value) .. tostring(target.is_httpd_role)] = {
425+
http_servers[target.value] = {
379426
httpd = httpd,
380427
routes = {},
381-
is_httpd_role = target.is_httpd_role,
428+
httpd_name = target.httpd_name,
382429
}
383430
end
384431

385-
local server = http_servers[tostring(target.value) .. tostring(target.is_httpd_role)]
432+
local server = http_servers[target.value]
386433
local httpd = server.httpd
387434
local old_routes = server.routes
388435

@@ -421,30 +468,11 @@ local function apply_http(conf)
421468
server.routes = new_routes
422469
end
423470
end
424-
425-
for target, server in pairs(http_servers) do
426-
if not enabled[target] then
427-
if server.is_httpd_role then
428-
for path, _ in pairs(server.routes) do
429-
server.httpd:delete(path)
430-
end
431-
else
432-
server.httpd:stop()
433-
end
434-
http_servers[target] = nil
435-
end
436-
end
437471
end
438472

439473
local function stop_http()
440-
for _, server in pairs(http_servers or {}) do
441-
if server.is_httpd_role then
442-
for path, _ in pairs(server.routes) do
443-
server.httpd:delete(path)
444-
end
445-
else
446-
server.httpd:stop()
447-
end
474+
for name in pairs(http_servers or {}) do
475+
stop_server(name)
448476
end
449477
http_servers = nil
450478
end

‎test/integration/reload_config_test.lua‎

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
local fio = require('fio')
2+
local yaml = require('yaml')
3+
local socket = require('socket')
4+
local helpers = require('test.helpers')
5+
local server = require('test.helpers.server')
6+
7+
local t = require('luatest')
8+
local g = t.group()
9+
10+
g.before_all(function()
11+
helpers.skip_if_unsupported()
12+
end)
13+
14+
g.before_each(function(cg)
15+
cg.workdir = fio.tempdir()
16+
fio.mktree(cg.workdir)
17+
18+
fio.copytree(".rocks", fio.pathjoin(cg.workdir, ".rocks"))
19+
fio.copytree("roles", fio.pathjoin(cg.workdir, "roles"))
20+
fio.copytree(fio.pathjoin("test", "ssl_data"), fio.pathjoin(cg.workdir, "ssl_data"))
21+
fio.copyfile(fio.pathjoin('test', 'entrypoint', 'config.yaml'), cg.workdir)
22+
end)
23+
24+
g.after_each(function(cg)
25+
cg.server:stop()
26+
fio.rmtree(cg.workdir)
27+
end)
28+
29+
local function is_tcp_connect(host, port)
30+
local tcp = socket.tcp()
31+
tcp:settimeout(0.3)
32+
local ok, _ = tcp:connect(host, port)
33+
tcp:close()
34+
35+
return ok
36+
end
37+
38+
local function change_listen_target_in_config(cg, old_addr, new_addr)
39+
local file = fio.open(fio.pathjoin(cg.workdir, 'config.yaml'), {'O_RDONLY'})
40+
t.assert(file ~= nil)
41+
42+
local cfg = file:read()
43+
file:close()
44+
45+
cfg = yaml.decode(cfg)
46+
local export_instances = cfg.groups['group-001'].replicasets['replicaset-001'].
47+
instances.master.roles_cfg['roles.metrics-export'].http
48+
49+
for i, v in pairs(export_instances) do
50+
if v.listen ~= nil and v.listen == old_addr then
51+
export_instances[i].listen = new_addr
52+
end
53+
end
54+
55+
file = fio.open(fio.pathjoin(cg.workdir, 'config.yaml'), {'O_CREAT', 'O_WRONLY', 'O_TRUNC'}, tonumber('644', 8))
56+
file:write(yaml.encode(cfg))
57+
file:close()
58+
end
59+
60+
g.test_reload_config_update_addr = function(cg)
61+
cg.server = server:new({
62+
config_file = fio.pathjoin(cg.workdir, 'config.yaml'),
63+
chdir = cg.workdir,
64+
alias = 'master',
65+
workdir = cg.workdir,
66+
})
67+
68+
cg.server:start({wait_until_ready = true})
69+
70+
t.assert(is_tcp_connect('127.0.0.1', 8082))
71+
t.assert_not(is_tcp_connect('127.0.0.2', 8082))
72+
73+
change_listen_target_in_config(cg, '127.0.0.1:8082', '0.0.0.0:8082')
74+
cg.server:eval("require('config'):reload()")
75+
76+
t.assert(is_tcp_connect('127.0.0.1', 8082))
77+
t.assert(is_tcp_connect('127.0.0.2', 8082))
78+
t.assert(is_tcp_connect('127.1.2.3', 8082))
79+
80+
change_listen_target_in_config(cg, '0.0.0.0:8082', '127.0.0.1:8082')
81+
cg.server:eval("require('config'):reload()")
82+
83+
t.assert_not(is_tcp_connect('127.0.0.2', 8082))
84+
t.assert(is_tcp_connect('127.0.0.1', 8082))
85+
end
86+
87+
g.test_reload_config_global_addr_conflict = function(cg)
88+
cg.server = server:new({
89+
config_file = fio.pathjoin(cg.workdir, 'config.yaml'),
90+
chdir = cg.workdir,
91+
alias = 'master',
92+
workdir = cg.workdir,
93+
})
94+
95+
cg.server:start({wait_until_ready = true})
96+
97+
change_listen_target_in_config(cg, 8081, '0.0.0.0:8082')
98+
t.assert_error_msg_content_equals(
99+
"Can't create tcp_server: Address already in use",
100+
function() cg.server:eval("require('config'):reload()") end
101+
)
102+
end

‎test/unit/http_test.lua‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -699,6 +699,57 @@ local test_reapply_delete_cases = {
699699
},
700700
},
701701
},
702+
["listen_global_addr"] = {
703+
apply_cases = {
704+
{
705+
cfg = {
706+
http = {
707+
{
708+
listen = '0.0.0.0:8081',
709+
endpoints = {
710+
{
711+
path = "/metrics1",
712+
format = "prometheus",
713+
},
714+
},
715+
},
716+
},
717+
},
718+
expected_json_urls = {},
719+
expected_prometheus_urls = {
720+
"http://127.0.0.1:8081/metrics1",
721+
"http://127.1.2.3:8081/metrics1"
722+
},
723+
expected_none_urls = {},
724+
},
725+
{
726+
cfg = {
727+
http = {
728+
{
729+
listen = '127.0.0.1:8081',
730+
endpoints = {
731+
{
732+
path = "/metrics1",
733+
format = "json",
734+
},
735+
{
736+
path = "/metrics2",
737+
format = "prometheus",
738+
},
739+
},
740+
},
741+
},
742+
},
743+
expected_json_urls = {
744+
"http://127.0.0.1:8081/metrics1",
745+
},
746+
expected_prometheus_urls = {
747+
"http://127.0.0.1:8081/metrics2",
748+
},
749+
expected_none_urls = {},
750+
},
751+
},
752+
},
702753
}
703754

704755
for name, case in pairs(test_reapply_delete_cases) do
@@ -858,6 +909,61 @@ local test_reapply_add_cases = {
858909
},
859910
},
860911
},
912+
["listen_global_addr"] = {
913+
apply_cases = {
914+
{
915+
cfg = {
916+
http = {
917+
{
918+
listen = '127.0.0.1:8081',
919+
endpoints = {
920+
{
921+
path = "/metrics1",
922+
format = "prometheus",
923+
},
924+
},
925+
},
926+
},
927+
},
928+
expected_json_urls = {},
929+
expected_prometheus_urls = {
930+
"http://127.0.0.1:8081/metrics1"
931+
},
932+
expected_none_urls = {
933+
"http://127.0.0.1:8081/metrics2",
934+
"http://127.0.0.1:8082/metrics/1",
935+
},
936+
},
937+
{
938+
cfg = {
939+
http = {
940+
{
941+
listen = '0.0.0.0:8081',
942+
endpoints = {
943+
{
944+
path = "/metrics1",
945+
format = "json",
946+
},
947+
{
948+
path = "/metrics2",
949+
format = "prometheus",
950+
},
951+
},
952+
},
953+
},
954+
},
955+
expected_json_urls = {
956+
"http://127.0.0.1:8081/metrics1",
957+
"http://127.1.2.3:8081/metrics1",
958+
},
959+
expected_prometheus_urls = {
960+
"http://127.0.0.1:8081/metrics2",
961+
"http://127.1.2.3:8081/metrics2",
962+
},
963+
expected_none_urls = {},
964+
},
965+
},
966+
},
861967
}
862968

863969
for name, case in pairs(test_reapply_add_cases) do

0 commit comments

Comments
 (0)