En/fix 1988 1 #2151
En/fix 1988 1 #2151
Conversation
enuribekov-tempesta
force-pushed
the
en/fix-1988-1
branch
from
70af0cd
to
7768f06
Compare
enuribekov-tempesta
force-pushed
the
en/fix-1988-1
branch
from
1f24659
to
d901ee9
Compare
There was a problem hiding this comment.
Nothing serious, but bunch of cleanups is required. Probably it's more serious to fix the tests in tempesta-tech/tempesta-test#637 w/o using the warning
|
|
||
| if(tfw_apm_hm_srv_alive(resp, srv)) { | ||
| T_DBG2("Mark server alive"); | ||
| tfw_srv_mark_alive(srv); |
There was a problem hiding this comment.
The function must be renamed or commented here: it's very confusing to see a function call with name 'mark_alive' right after thec all 'try_suspend'
There was a problem hiding this comment.
tfw_http_hm_try_suspend() is the conditional function and has following comment:
/*
* Try to mark server as suspended.
* In case of HM is active do it, otherwise left unchanged.
*/
tfw_srv_mark_alive() is just inline for clearing flag
There was a problem hiding this comment.
The logic is not clear for me. We trying to suspend server using tfw_http_hm_try_suspend even in case if server was suspended in previous call we checking is it alive tfw_apm_hm_srv_alive. Why?
enuribekov-tempesta
force-pushed
the
en/fix-1988-1
branch
from
d818c1e
to
d901ee9
Compare
fw/http.c
Outdated
| old_flags = cmpxchg(&srv->flags, flags, | ||
| flags | TFW_SRV_F_SUSPEND); | ||
| flags | TFW_SRV_F_SUSPEND); |
There was a problem hiding this comment.
Previous alignment was good
| * Total response statistics is counted permanently regardless | ||
| * of the state of the health monitor. | ||
| */ | ||
| bool lim_exceeded = tfw_apm_hm_srv_limit(resp->status, srv->apmref); |
There was a problem hiding this comment.
Why do we calc the limit before checking flag srv->flags & TFW_SRV_F_HMONITOR, seems we calc the limit even for servers doesn't have health monitor. Or am I wrong?
There was a problem hiding this comment.
Yes, because these counters used for statistics too.
Perhaps this can be done much more elegantly, but I have kept the existing logic because statistics go beyond that.
|
|
||
| if(tfw_apm_hm_srv_alive(resp, srv)) { | ||
| T_DBG2("Mark server alive"); | ||
| tfw_srv_mark_alive(srv); |
There was a problem hiding this comment.
The logic is not clear for me. We trying to suspend server using tfw_http_hm_try_suspend even in case if server was suspended in previous call we checking is it alive tfw_apm_hm_srv_alive. Why?
enuribekov-tempesta
force-pushed
the
en/fix-1988-1
branch
from
cc2698f
to
8bcaf12
Compare
Request string should not contain quotes, because all of the symbols will be send on the wire as is. Also fixed HTTTP string.
Original issue to investigate was a suspection that automatic CRC calculation will never works properly. After investigation it turned out that response never achieves this place. This commit contains fixes, light refactoring for better readability and removing unused function arguments. Test cases for checking this changes added to tempesta-test.
In case of HM off, TfwApmHM is not allocated and following access to it lead to crash. So we should return from tfw_http_hm_control() immediately after updating statistics.
Warning informed that auto CRC generated and used by tests. Now tests will be just check CRC regardless of it origin.
enuribekov-tempesta
force-pushed
the
en/fix-1988-1
branch
from
8bcaf12
to
90b37ec
Compare
New pull request for #1988 made from the scratch.