Skip to content

Commit f916e64

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 f916e64

File tree

4 files changed

+259
-36
lines changed

4 files changed

+259
-36
lines changed

‎CHANGELOG.md

+2
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

+49-36
Original file line numberDiff line numberDiff line change
@@ -322,15 +322,35 @@ local function routes_equal(old, new)
322322
return true
323323
end
324324

325+
local function disable_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+
local listen_servers_to_start = {}
345+
local applied_servers = {}
346+
327347
for _, node in ipairs(conf) do
328348
if #(node.endpoints or {}) > 0 then
329349
local host, port, target
330350
if node.server ~= nil then
331351
target = {
332-
value = node.server,
333-
is_httpd_role = true,
352+
value = 'httpd_' .. node.server,
353+
httpd_name = node.server,
334354
}
335355
elseif node.listen ~= nil then
336356
local err
@@ -339,23 +359,19 @@ local function apply_http(conf)
339359
error("failed to parse URI: " .. err, 2)
340360
end
341361
target = {
342-
value = node.listen,
343-
is_httpd_role = false,
362+
value = 'listen_' .. host .. ':' .. tostring(port),
344363
}
345364
else
346365
target = {
347-
value = httpd_role.DEFAULT_SERVER_NAME,
348-
is_httpd_role = true,
366+
value = 'httpd_' .. httpd_role.DEFAULT_SERVER_NAME,
367+
httpd_name = httpd_role.DEFAULT_SERVER_NAME,
349368
}
350369
end
351370

352371
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
372+
applied_servers[target.value] = {}
357373

358-
if http_servers[tostring(target.value) .. tostring(target.is_httpd_role)] == nil then
374+
if http_servers[target.value] == nil then
359375
local httpd
360376
if node.listen ~= nil then
361377
httpd = http_server.new(host, port, {
@@ -366,23 +382,29 @@ local function apply_http(conf)
366382
ssl_password = node.ssl_password,
367383
ssl_password_file = node.ssl_password_file
368384
})
369-
httpd:start()
370385
else
371-
httpd = httpd_role.get_server(target.value)
386+
httpd = httpd_role.get_server(target.httpd_name)
372387
if httpd == nil then
373388
error(('failed to get server by name %q, check that roles.httpd was' ..
374-
' already applied'):format(target.value))
389+
' already applied'):format(target.httpd_name))
375390
end
376391
end
377392

378-
http_servers[tostring(target.value) .. tostring(target.is_httpd_role)] = {
393+
http_servers[target.value] = {
379394
httpd = httpd,
380395
routes = {},
381-
is_httpd_role = target.is_httpd_role,
396+
httpd_name = target.httpd_name,
382397
}
398+
399+
if node.listen ~= nil then
400+
-- Defer starting a server with `listen` key to not trigger
401+
-- "already in use" error of running servers that should be stopped
402+
-- if it isn't in applying config.
403+
table.insert(listen_servers_to_start, http_servers[target.value])
404+
end
383405
end
384406

385-
local server = http_servers[tostring(target.value) .. tostring(target.is_httpd_role)]
407+
local server = http_servers[target.value]
386408
local httpd = server.httpd
387409
local old_routes = server.routes
388410

@@ -422,29 +444,20 @@ local function apply_http(conf)
422444
end
423445
end
424446

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
447+
for name in pairs(http_servers or {}) do
448+
if applied_servers[name] == nil then
449+
disable_server(name)
435450
end
436451
end
452+
453+
for _, server in pairs(listen_servers_to_start) do
454+
server.httpd:start()
455+
end
437456
end
438457

439458
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
459+
for name in pairs(http_servers or {}) do
460+
disable_server(name)
448461
end
449462
http_servers = nil
450463
end

‎test/integration/reload_config_test.lua

+102
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

+106
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)