Skip to content

Commit 785694c

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 785694c

File tree

4 files changed

+270
-32
lines changed

4 files changed

+270
-32
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: 60 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -323,14 +323,58 @@ local function routes_equal(old, new)
323323
end
324324

325325
local function apply_http(conf)
326-
local enabled = {}
326+
-- Stop old servers.
327+
for name, server in pairs(http_servers or {}) do
328+
local server_found = false
329+
330+
for _, node in pairs(conf) do
331+
if #(node.endpoints or {}) > 0 then
332+
local key
333+
334+
if node.server ~= nil then
335+
key = 'httpd_' .. node.server
336+
elseif node.listen ~= nil then
337+
local host, port, err = parse_listen(node.listen)
338+
if err ~= nil then
339+
error("failed to parse URI: " .. err, 2)
340+
end
341+
342+
key = 'listen_' .. host .. ':' .. tostring(port)
343+
else
344+
key = 'httpd_' .. httpd_role.DEFAULT_SERVER_NAME
345+
end
346+
347+
if name == key then
348+
server_found = true
349+
break
350+
end
351+
end
352+
end
353+
354+
355+
if not server_found then
356+
if server.httpd_name ~= nil then
357+
for path, _ in pairs(server.routes) do
358+
server.httpd:delete(path)
359+
end
360+
else
361+
if server.httpd.is_run == true then
362+
server.httpd:stop()
363+
end
364+
end
365+
366+
http_servers[name] = nil
367+
end
368+
end
369+
370+
-- Creating and editing existing ones.
327371
for _, node in ipairs(conf) do
328372
if #(node.endpoints or {}) > 0 then
329373
local host, port, target
330374
if node.server ~= nil then
331375
target = {
332-
value = node.server,
333-
is_httpd_role = true,
376+
value = 'httpd_' .. node.server,
377+
httpd_name = node.server,
334378
}
335379
elseif node.listen ~= nil then
336380
local err
@@ -339,23 +383,18 @@ local function apply_http(conf)
339383
error("failed to parse URI: " .. err, 2)
340384
end
341385
target = {
342-
value = node.listen,
343-
is_httpd_role = false,
386+
value = 'listen_' .. host .. ':' .. tostring(port),
344387
}
345388
else
346389
target = {
347-
value = httpd_role.DEFAULT_SERVER_NAME,
348-
is_httpd_role = true,
390+
value = 'httpd_' .. httpd_role.DEFAULT_SERVER_NAME,
391+
httpd_name = httpd_role.DEFAULT_SERVER_NAME,
349392
}
350393
end
351394

352395
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
357396

358-
if http_servers[tostring(target.value) .. tostring(target.is_httpd_role)] == nil then
397+
if http_servers[target.value] == nil then
359398
local httpd
360399
if node.listen ~= nil then
361400
httpd = http_server.new(host, port, {
@@ -368,21 +407,21 @@ local function apply_http(conf)
368407
})
369408
httpd:start()
370409
else
371-
httpd = httpd_role.get_server(target.value)
410+
httpd = httpd_role.get_server(target.httpd_name)
372411
if httpd == nil then
373412
error(('failed to get server by name %q, check that roles.httpd was' ..
374-
' already applied'):format(target.value))
413+
' already applied'):format(target.httpd_name))
375414
end
376415
end
377416

378-
http_servers[tostring(target.value) .. tostring(target.is_httpd_role)] = {
417+
http_servers[target.value] = {
379418
httpd = httpd,
380419
routes = {},
381-
is_httpd_role = target.is_httpd_role,
420+
httpd_name = target.httpd_name,
382421
}
383422
end
384423

385-
local server = http_servers[tostring(target.value) .. tostring(target.is_httpd_role)]
424+
local server = http_servers[target.value]
386425
local httpd = server.httpd
387426
local old_routes = server.routes
388427

@@ -421,29 +460,18 @@ local function apply_http(conf)
421460
server.routes = new_routes
422461
end
423462
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
437463
end
438464

439465
local function stop_http()
440466
for _, server in pairs(http_servers or {}) do
441-
if server.is_httpd_role then
467+
if server.httpd_name ~= nil then
442468
for path, _ in pairs(server.routes) do
443469
server.httpd:delete(path)
444470
end
445471
else
446-
server.httpd:stop()
472+
if server.httpd.is_run == true then
473+
server.httpd:stop()
474+
end
447475
end
448476
end
449477
http_servers = nil

‎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)