Re: [PATCH] REG-TEST: mailers: add new test for 'mailers' section
On Tue, Jan 22, 2019 at 12:36:41AM +0100, PiBa-NL wrote: > The regtest works for me as well with this patch. Without needing the > 'timeout mail' setting. > > I think we can call it fixed once committed. OK so I've now merged it in dev and 1.9. Thanks! Willy
Re: [PATCH] REG-TEST: mailers: add new test for 'mailers' section
Hi Christopher, Op 21-1-2019 om 15:28 schreef Christopher Faulet: Hi Pieter, About the timing issue, could you try the following patch please ? With it, I can run the regtest about email alerts without any error. Thanks, -- Christopher Faulet The regtest works for me as well with this patch. Without needing the 'timeout mail' setting. I think we can call it fixed once committed. Thanks, PiBa-NL (Pieter)
Re: [PATCH] REG-TEST: mailers: add new test for 'mailers' section
Le 23/12/2018 à 21:17, PiBa-NL a écrit :
Hi List,
Attached a new test to verify that the 'mailers' section is working
properly.
Currently with 1.9 the mailers sends thousands of mails for my setup...
As the test is rather slow i have marked it with a starting letter 's'.
Note that the test also fails on 1.6/1.7/1.8 but can be 'fixed' there by
adding a 'timeout mail 200ms'.. (except on 1.6 which doesn't have that
setting.)
I don't think that should be needed though if everything was working
properly?
If the test could be committed, and related issues exposed fixed that
would be neat ;)
Thanks in advance,
PiBa-NL (Pieter)
Hi Pieter,
About the timing issue, could you try the following patch please ? With
it, I can run the regtest about email alerts without any error.
Thanks,
--
Christopher Faulet
>From 8b7822ad2c7d9e4f9fe6b19f57d1d1ff01336a70 Mon Sep 17 00:00:00 2001
From: Christopher Faulet
Date: Mon, 21 Jan 2019 14:15:50 +0100
Subject: [PATCH] BUG/MINOR: check: Wake the check task if the check is
finished in wake_srv_chk()
With tcp-check, the result of the check is set by the function tcpcheck_main()
from the I/O layer. So it is important to wake up the check task to handle the
result and finish the check. Otherwise, we will wait the task timeout to handle
the result of a tcp-check, delaying the next check by as much.
This patch also fixes a problem about email alerts reported by PiBa-NL (Pieter)
on the ML [1] on all versions since the 1.6. So this patch must be backported
from 1.9 to 1.6.
[1] https://www.mail-archive.com/haproxy@formilux.org/msg32190.html
---
src/checks.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/checks.c b/src/checks.c
index 1a81ccdb3..b0de77088 100644
--- a/src/checks.c
+++ b/src/checks.c
@@ -1442,12 +1442,13 @@ static int wake_srv_chk(struct conn_stream *cs)
}
if (check->result != CHK_RES_UNKNOWN) {
- /* We're here because nobody wants to handle the error, so we
- * sure want to abort the hard way.
- */
+ /* Check complete or aborted. If connection not yet closed do it
+ * now and wake the check task up to be sure the result is
+ * handled ASAP. */
conn_sock_drain(conn);
cs_close(cs);
ret = -1;
+ task_wakeup(check->task, TASK_WOKEN_IO);
}
if (check->server)
--
2.20.1
Re: [PATCH] REG-TEST: mailers: add new test for 'mailers' section
On 1/11/19 12:35 AM, Cyril Bonté wrote: Hi all, Le 08/01/2019 à 10:06, Willy Tarreau a écrit : On Tue, Jan 08, 2019 at 09:31:22AM +0100, Frederic Lecaille wrote: Indeed this script could worked with a short mailer timeout before af4021e6 commit. Another git bisect shows that 53216e7d introduced the email bombing issue. Note that 33a09a5f refers to 53216e7d commit. I am not sure this can help. Sure it helps, at least to know whom to ping :-) Well, from what I've seen with a small test, I've a different conclusion about the commit which introduced the issue. It looks to have been introduced earlier with commit 0108bb3e4 "MEDIUM: mailers: Init alerts during conf parsing and refactor their processing" http://git.haproxy.org/?p=haproxy.git;a=commit;h=0108bb3e4 You are true. The feature was broken by af4021e6 fixed by 53216e7d which made the bug occur again. Thanks Cyril. Fred.
Re: [PATCH] REG-TEST: mailers: add new test for 'mailers' section
Hi Cyril, On Fri, Jan 11, 2019 at 12:35:12AM +0100, Cyril Bonté wrote: > Well, from what I've seen with a small test, I've a different conclusion > about the commit which introduced the issue. It looks to have been > introduced earlier with commit 0108bb3e4 "MEDIUM: mailers: Init alerts > during conf parsing and refactor their processing" > http://git.haproxy.org/?p=haproxy.git;a=commit;h=0108bb3e4 > > Hope this helps. Ah cool, sure it helps, I like it when we end up on one of Christopher's commits, usually he figures quickly where the problem is :-) Thanks! Willy
Re: [PATCH] REG-TEST: mailers: add new test for 'mailers' section
Hi all, Le 08/01/2019 à 10:06, Willy Tarreau a écrit : On Tue, Jan 08, 2019 at 09:31:22AM +0100, Frederic Lecaille wrote: Indeed this script could worked with a short mailer timeout before af4021e6 commit. Another git bisect shows that 53216e7d introduced the email bombing issue. Note that 33a09a5f refers to 53216e7d commit. I am not sure this can help. Sure it helps, at least to know whom to ping :-) Well, from what I've seen with a small test, I've a different conclusion about the commit which introduced the issue. It looks to have been introduced earlier with commit 0108bb3e4 "MEDIUM: mailers: Init alerts during conf parsing and refactor their processing" http://git.haproxy.org/?p=haproxy.git;a=commit;h=0108bb3e4 Hope this helps. -- Cyril Bonté
Re: [PATCH] REG-TEST: mailers: add new test for 'mailers' section
On Tue, Jan 08, 2019 at 09:31:22AM +0100, Frederic Lecaille wrote: > Indeed this script could worked with a short mailer timeout before af4021e6 > commit. Another git bisect shows that 53216e7d introduced the email bombing > issue. > > Note that 33a09a5f refers to 53216e7d commit. > > I am not sure this can help. Sure it helps, at least to know whom to ping :-) Willy
Re: [PATCH] REG-TEST: mailers: add new test for 'mailers' section
On 1/7/19 9:24 PM, PiBa-NL wrote: Hi Willy, Op 7-1-2019 om 15:25 schreef Willy Tarreau: Hi Pieter, On Sun, Jan 06, 2019 at 04:38:21PM +0100, PiBa-NL wrote: The 23654 mails received for a failed server is a bit much.. I agree. I really don't know much how the mails work to be honest, as I have never used them. I remember that we reused a part of the tcp-check infrastructure because by then it offered a convenient way to proceed with send/expect sequences. Maybe there's something excessive in the sequence there, such as a certain status code being expected at the end while the mail succeeds, I don't know. Given that this apparently has always been broken, For 1 part its always been broken (needing the short mailer timeout to send all expected mails), for the other part, at least until 1.8.14 it used to NOT send thousands of mails so that would be a regression in the current 1.9 version that should get fixed on a shorter term. Indeed this script could worked with a short mailer timeout before af4021e6 commit. Another git bisect shows that 53216e7d introduced the email bombing issue. Note that 33a09a5f refers to 53216e7d commit. I am not sure this can help. Fred.
Re: [PATCH] REG-TEST: mailers: add new test for 'mailers' section
Hi Pieter, On Mon, Jan 07, 2019 at 09:24:24PM +0100, PiBa-NL wrote: > For 1 part its always been broken (needing the short mailer timeout to send > all expected mails), for the other part, at least until 1.8.14 it used to > NOT send thousands of mails so that would be a regression in the current 1.9 > version that should get fixed on a shorter term. OK, thanks for clarifying. Indeed a fix is needed then. > > I'm hesitant between > > merging this in the slow category or the broken one. My goal with "broken" > > was to keep the scripts that trigger broken behaviours that need to be > > addressed, rather than keep broken scripts. > Indeed keeping broken scripts wouldn't be help-full in the long run, unless > there is still the intent to fix them. It isn't what the makefile says about > 'LEVEL 5' though. It says its for 'broken scripts' and to quickly disable > them, not as you write here for scripts that show 'broken haproxy behavior'. I know :-) For me the "broken" scripts should be the experimental ones, those for known broken haproxy behavior definitely are worth keeping because once the issue is fixed they can be renamed and included in the standard test series. It's just that until a bug is fixed, reminding in loops about it is counter-productive. Especially when these ones cause time-outs or whatever issue some of our bugs used to cause! > > My goal is to make sure we > > never consider it normal to have failures in the regular test suite, > > otherwise you know how it becomes, just like compiler warnings, people > > say "oh I didn't notice this new error in the middle of all other ones". > Agreed, though i will likely fall into repeat some day, apology in advance > ;).. No, you're welcome! > I guess we could 'fix' the regtest by specifying the 'timeout mail > 200', that would fix it for 1.7 and 1.8.. And might help for 1.9 > regressiontests and to get it fixed to at least not send thousands of mails. > We might forget about the short time requirement then though, which seems > strange as well. And the test wouldn't be 1.6 compatible as it doesn't have > that setting at all. I honestly have no idea. My goal is that this bug gets fixed at least if it's a regression. > > Thus probably the best thing to do is to use it at level 5 so that it's > > easier to work on the bug without triggering false positives when doing > > regression testing. > > > > What's your opinion ? > > With a changed description for 'level 5' being 'shows broken haproxy > behavior, to be fixed in a future release' i think it would fit in there > nicely. Can you change the starting letter of the .vtc test (and the .lua > and reference to that) to 'k' during committing? Or shall i re-send it? I can do it, don't worry. > p.s. What do you think about the 'naming' of the test? > 'k_healthcheckmail.vtc' or 'k0.vtc' personally i don't think the > 'numbering' of tests makes them easier to use.?. I've been thinking about such things as well. As I mentioned earlier, I definitely think that our reg-testing suite's organisation will continue to evolve. It's something new for this project and we're discovering a number of unexpected side effects that progressively make us adapt, and as long as we can continue to issue "make regtest", I think everyone will be OK. Cheers, Willy
Re: [PATCH] REG-TEST: mailers: add new test for 'mailers' section
Hi Willy, Op 7-1-2019 om 15:25 schreef Willy Tarreau: Hi Pieter, On Sun, Jan 06, 2019 at 04:38:21PM +0100, PiBa-NL wrote: The 23654 mails received for a failed server is a bit much.. I agree. I really don't know much how the mails work to be honest, as I have never used them. I remember that we reused a part of the tcp-check infrastructure because by then it offered a convenient way to proceed with send/expect sequences. Maybe there's something excessive in the sequence there, such as a certain status code being expected at the end while the mail succeeds, I don't know. Given that this apparently has always been broken, For 1 part its always been broken (needing the short mailer timeout to send all expected mails), for the other part, at least until 1.8.14 it used to NOT send thousands of mails so that would be a regression in the current 1.9 version that should get fixed on a shorter term. I'm hesitant between merging this in the slow category or the broken one. My goal with "broken" was to keep the scripts that trigger broken behaviours that need to be addressed, rather than keep broken scripts. Indeed keeping broken scripts wouldn't be help-full in the long run, unless there is still the intent to fix them. It isn't what the makefile says about 'LEVEL 5' though. It says its for 'broken scripts' and to quickly disable them, not as you write here for scripts that show 'broken haproxy behavior'. My goal is to make sure we never consider it normal to have failures in the regular test suite, otherwise you know how it becomes, just like compiler warnings, people say "oh I didn't notice this new error in the middle of all other ones". Agreed, though i will likely fall into repeat some day, apology in advance ;).. I guess we could 'fix' the regtest by specifying the 'timeout mail 200', that would fix it for 1.7 and 1.8.. And might help for 1.9 regressiontests and to get it fixed to at least not send thousands of mails. We might forget about the short time requirement then though, which seems strange as well. And the test wouldn't be 1.6 compatible as it doesn't have that setting at all. Thus probably the best thing to do is to use it at level 5 so that it's easier to work on the bug without triggering false positives when doing regression testing. What's your opinion ? With a changed description for 'level 5' being 'shows broken haproxy behavior, to be fixed in a future release' i think it would fit in there nicely. Can you change the starting letter of the .vtc test (and the .lua and reference to that) to 'k' during committing? Or shall i re-send it? p.s. What do you think about the 'naming' of the test? 'k_healthcheckmail.vtc' or 'k0.vtc' personally i don't think the 'numbering' of tests makes them easier to use.?. thanks, Willy Regards, PiBa-NL (Pieter)
Re: [PATCH] REG-TEST: mailers: add new test for 'mailers' section
Hi Pieter, On Sun, Jan 06, 2019 at 04:38:21PM +0100, PiBa-NL wrote: > Hi, > 2 weeks passed without reply, so a little hereby a little 'bump'.. I know > everyone has been busy, but would be nice to get test added or at least the > biggest issue of the 'mailbomb' fixed before next release. If its > 'scheduled' to get looked at later thats okay. Just making sure it aint > forgotten about :). Sure, and sorry for the silence, but as you guessed, I also think everyone got busy. > The 23654 mails received for a failed server is a bit much.. I agree. I really don't know much how the mails work to be honest, as I have never used them. I remember that we reused a part of the tcp-check infrastructure because by then it offered a convenient way to proceed with send/expect sequences. Maybe there's something excessive in the sequence there, such as a certain status code being expected at the end while the mail succeeds, I don't know. Given that this apparently has always been broken, I'm hesitant between merging this in the slow category or the broken one. My goal with "broken" was to keep the scripts that trigger broken behaviours that need to be addressed, rather than keep broken scripts. My goal is to make sure we never consider it normal to have failures in the regular test suite, otherwise you know how it becomes, just like compiler warnings, people say "oh I didn't notice this new error in the middle of all other ones". Thus probably the best thing to do is to use it at level 5 so that it's easier to work on the bug without triggering false positives when doing regression testing. What's your opinion ? thanks, Willy
Re: [PATCH] REG-TEST: mailers: add new test for 'mailers' section
Hi, 2 weeks passed without reply, so a little hereby a little 'bump'.. I know everyone has been busy, but would be nice to get test added or at least the biggest issue of the 'mailbomb' fixed before next release. If its 'scheduled' to get looked at later thats okay. Just making sure it aint forgotten about :). The 23654 mails received for a failed server is a bit much.. c2 7.5 EXPECT resp.http.mailsreceived (23654) == "16" failed Regards, PiBa-NL (Pieter) Op 23-12-2018 om 23:37 schreef PiBa-NL: Changed subject of patch requirement to 'REGTEST'. Op 23-12-2018 om 21:17 schreef PiBa-NL: Hi List, Attached a new test to verify that the 'mailers' section is working properly. Currently with 1.9 the mailers sends thousands of mails for my setup... As the test is rather slow i have marked it with a starting letter 's'. Note that the test also fails on 1.6/1.7/1.8 but can be 'fixed' there by adding a 'timeout mail 200ms'.. (except on 1.6 which doesn't have that setting.) I don't think that should be needed though if everything was working properly? If the test could be committed, and related issues exposed fixed that would be neat ;) Thanks in advance, PiBa-NL (Pieter)
Re: [PATCH] REG-TEST: mailers: add new test for 'mailers' section
Changed subject of patch requirement to 'REGTEST'.
Op 23-12-2018 om 21:17 schreef PiBa-NL:
Hi List,
Attached a new test to verify that the 'mailers' section is working
properly.
Currently with 1.9 the mailers sends thousands of mails for my setup...
As the test is rather slow i have marked it with a starting letter 's'.
Note that the test also fails on 1.6/1.7/1.8 but can be 'fixed' there
by adding a 'timeout mail 200ms'.. (except on 1.6 which doesn't have
that setting.)
I don't think that should be needed though if everything was working
properly?
If the test could be committed, and related issues exposed fixed that
would be neat ;)
Thanks in advance,
PiBa-NL (Pieter)
From 8d63f5a39a9b4b326b636e42ccafcf0c2173d752 Mon Sep 17 00:00:00 2001
From: PiBa-NL
Date: Sun, 23 Dec 2018 21:06:31 +0100
Subject: [PATCH] REGTEST: mailers: add new test for 'mailers' section
This test verifies the mailers section works properly by checking that it sends
the proper amount of mails when health-checks are changing and or marking a
server up/down
The test currently fails on all versions of haproxy i tried with varying
results.
1.9.0 produces thousands of mails..
1.8.14 only sends 1 mail, needs a 200ms 'timeout mail' to succeed
1.7.11 only sends 1 mail, needs a 200ms 'timeout mail' to succeed
1.6 only sends 1 mail, (does not have the 'timeout mail' setting implemented)
---
reg-tests/mailers/shealthcheckmail.lua | 105 +
reg-tests/mailers/shealthcheckmail.vtc | 75 ++
2 files changed, 180 insertions(+)
create mode 100644 reg-tests/mailers/shealthcheckmail.lua
create mode 100644 reg-tests/mailers/shealthcheckmail.vtc
diff --git a/reg-tests/mailers/shealthcheckmail.lua
b/reg-tests/mailers/shealthcheckmail.lua
new file mode 100644
index ..9c75877b
--- /dev/null
+++ b/reg-tests/mailers/shealthcheckmail.lua
@@ -0,0 +1,105 @@
+
+local vtc_port1 = 0
+local mailsreceived = 0
+local mailconnectionsmade = 0
+local healthcheckcounter = 0
+
+core.register_action("bug", { "http-res" }, function(txn)
+ data = txn:get_priv()
+ if not data then
+ data = 0
+ end
+ data = data + 1
+ print(string.format("set to %d", data))
+ txn.http:res_set_status(200 + data)
+ txn:set_priv(data)
+end)
+
+core.register_service("luahttpservice", "http", function(applet)
+ local response = "?"
+ local responsestatus = 200
+ if applet.path == "/setport" then
+ vtc_port1 = applet.headers["vtcport1"][0]
+ response = "OK"
+ end
+ if applet.path == "/svr_healthcheck" then
+ healthcheckcounter = healthcheckcounter + 1
+ if healthcheckcounter < 2 or healthcheckcounter > 6 then
+ responsestatus = 403
+ end
+ end
+
+ applet:set_status(responsestatus)
+ if applet.path == "/checkMailCounters" then
+ response = "MailCounters"
+ applet:add_header("mailsreceived", mailsreceived)
+ applet:add_header("mailconnectionsmade", mailconnectionsmade)
+ end
+ applet:start_response()
+ applet:send(response)
+end)
+
+core.register_service("fakeserv", "http", function(applet)
+ applet:set_status(200)
+ applet:start_response()
+end)
+
+function RecieveAndCheck(applet, expect)
+ data = applet:getline()
+ if data:sub(1,expect:len()) ~= expect then
+ core.Info("Expected: "..expect.." but
got:"..data:sub(1,expect:len()))
+ applet:send("Expected: "..expect.." but got:"..data.."\r\n")
+ return false
+ end
+ return true
+end
+
+core.register_service("mailservice", "tcp", function(applet)
+ core.Info("# Mailservice Called #")
+ mailconnectionsmade = mailconnectionsmade + 1
+ applet:send("220 Welcome\r\n")
+ local data
+
+ if RecieveAndCheck(applet, "EHLO") == false then
+ return
+ end
+ applet:send("250 OK\r\n")
+ if RecieveAndCheck(applet, "MAIL FROM:") == false then
+ return
+ end
+ applet:send("250 OK\r\n")
+ if RecieveAndCheck(applet, "RCPT TO:") == false then
+ return
+ end
+ applet:send("250 OK\r\n")
+ if RecieveAndCheck(applet, "DATA") == false then
+ return
+ end
+ applet:send("354 OK\r\n")
+ core.Info(" Send your mailbody")
+ local endofmail = false
+ local subject = ""
+ while endofmail ~= true do
+ data = applet:getline() -- BODY CONTENT
+ --core.Info(data)
+ if data:sub(1, 9) == "Subject: " then
+ subject = data
+ end
+ if (data == "\r\n") then
+ data = applet:getline() -- BODY CONTENT
+ core.Info(data)
+ if (data ==
