Re: [PATCH] REGTEST: stick-tables: Test expiration when used with table_*
On Fri, Aug 31, 2018 at 11:14:00AM +0200, Frederic Lecaille wrote: > Hi, > > I am coming back to this reg testing sent by Daniel which was very > interesting because very complete and because there was a little > detail to fix. (...) applied, thanks Fred. Willy
Re: [PATCH] REGTEST: stick-tables: Test expiration when used with table_*
Hi,
I am coming back to this reg testing sent by Daniel which was very
interesting because very complete and because there was a little
detail to fix.
On 06/21/2018 04:53 AM, Willy Tarreau wrote:
Hi Daniel,
On Wed, Jun 20, 2018 at 10:28:43AM -0400, Daniel Corbett wrote:
+shell -expect "used:0" {
+echo "show table http1" |socat ${tmpdir}/h1/stats.sock -
As mentioned in previous mails, but after the first version of
this reg testing file, we can now use -cli argument to send commands
to the CLI which is automatically created for each script without
having to mention it in the configuration file (with -conf argument).
So the shell command above may be replaced by this section:
haproxy h1 -cli {
send "show table http1"
expect ~ "used: 0"
} -wait
The little detail to fix is that on my PC the CLI expect command may
sometimes fail because the unique stick-table entry created during
this script may sometime still be present when we dump the stick-table.
So here we expect to have no entry at all, or an entry with a null "use"
counter (use=0).
A correct regex would be something like 'used: (0|1\n.*use=0.*)\n$'.
Find attached to this mail Daniel's reg testing file with
the fixes mentioned above.
Fred.
>From 18692c4dd350ff6ef3609a56bd0162517590f2f4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?=
Date: Fri, 31 Aug 2018 11:05:39 +0200
Subject: [PATCH] REGTEST/MINOR: Add a reg testing file for 3e60b11.
Thank you to Daniel for having worked on this one.
---
reg-tests/stick-table/b1.vtc | 70
1 file changed, 70 insertions(+)
create mode 100644 reg-tests/stick-table/b1.vtc
diff --git a/reg-tests/stick-table/b1.vtc b/reg-tests/stick-table/b1.vtc
new file mode 100644
index ..43e25eb2
--- /dev/null
+++ b/reg-tests/stick-table/b1.vtc
@@ -0,0 +1,70 @@
+# commit 3e60b11
+# BUG/MEDIUM: stick-tables: Decrement ref_cnt in table_* converters
+#
+# When using table_* converters ref_cnt was incremented
+# and never decremented causing entries to not expire.
+#
+# The root cause appears to be that stktable_lookup_key()
+# was called within all sample_conv_table_* functions which was
+# incrementing ref_cnt and not decrementing after completion.
+#
+# Added stktable_release() to the end of each sample_conv_table_*
+# function and reworked the end logic to ensure that ref_cnt is
+# always decremented after use.
+#
+# This should be backported to 1.8
+
+varnishtest "stick-tables: Test expirations when used with table_*"
+
+# As some macros for haproxy are used in this file, this line is mandatory.
+feature ignore_unknown_macro
+
+# Do nothing.
+server s1 {
+} -start
+
+haproxy h1 -conf {
+# Configuration file of 'h1' haproxy instance.
+defaults
+mode http
+timeout connect 5s
+timeout server 30s
+timeout client 30s
+
+frontend http1
+bind "fd@${my_frontend_fd}"
+stick-table size 1k expire 1ms type ip store conn_rate(10s),http_req_cnt,http_err_cnt,http_req_rate(10s),http_err_rate(10s),gpc0,gpc0_rate(10s),gpt0
+http-request track-sc0 req.hdr(X-Forwarded-For)
+http-request redirect location https://${s1_addr}:${s1_port}/ if { req.hdr(X-Forwarded-For),table_http_req_cnt(http1) -m int lt 0 }
+http-request redirect location https://${s1_addr}:${s1_port}/ if { req.hdr(X-Forwarded-For),table_trackers(http1) -m int lt 0 }
+http-request redirect location https://${s1_addr}:${s1_port}/ if { req.hdr(X-Forwarded-For),in_table(http1) -m int lt 0 }
+http-request redirect location https://${s1_addr}:${s1_port}/ if { req.hdr(X-Forwarded-For),table_bytes_in_rate(http1) -m int lt 0 }
+http-request redirect location https://${s1_addr}:${s1_port}/ if { req.hdr(X-Forwarded-For),table_bytes_out_rate(http1) -m int lt 0 }
+http-request redirect location https://${s1_addr}:${s1_port}/ if { req.hdr(X-Forwarded-For),table_conn_cnt(http1) -m int lt 0 }
+http-request redirect location https://${s1_addr}:${s1_port}/ if { req.hdr(X-Forwarded-For),table_conn_cur(http1) -m int lt 0 }
+http-request redirect location https://${s1_addr}:${s1_port}/ if { req.hdr(X-Forwarded-For),table_conn_rate(http1) -m int lt 0 }
+http-request redirect location https://${s1_addr}:${s1_port}/ if { req.hdr(X-Forwarded-For),table_gpt0(http1) -m int lt 0 }
+http-request redirect location https://${s1_addr}:${s1_port}/ if { req.hdr(X-Forwarded-For),table_gpc0(http1) -m int lt 0 }
+http-request redirect location https://${s1_addr}:${s1_port}/ if { req.hdr(X-Forwarded-For),table_gpc0_rate(http1) -m int lt 0 }
+http-request redirect location https://${s1_addr}:${s1_port}/ if { req.hdr(X-Forwarded-For),table_http_err_cnt(http1) -m int lt 0 }
+http-request redirect location https://${s1_addr}:${s1_port}/ if { req.hdr(X-Forwarded-For),table_http_err_r
Re: [PATCH] REGTEST: stick-tables: Test expiration when used with table_*
Hi Fred, On Mon, Jun 25, 2018 at 11:45:31AM +0200, Frederic Lecaille wrote: > I have attached #0003 patch for that in addition to these ones: > > #0001 : as would say Olivier "Ooops, I'am an idiot etc". > > reg-tests/ssl/h0.vtc did not run any https request. > > #0002 : set the default value of HAPROXY_PROGRAM environment variable. > > We will have to change the class of the already existing reg test files. Sorry I missed this series today, it's now merged. Thank you! Willy
Re: [PATCH] REGTEST: stick-tables: Test expiration when used with table_*
On 06/21/2018 04:53 AM, Willy Tarreau wrote:
Hi Daniel,
On Wed, Jun 20, 2018 at 10:28:43AM -0400, Daniel Corbett wrote:
+shell -expect "used:0" {
+echo "show table http1" |socat ${tmpdir}/h1/stats.sock -
^
This is the point where it will start to require that we organize the
reg tests better. First, socat is rarely installed by default so we'll
have to mention that it's required. Second, socat introduces half a
second delay before quitting, making it impractical for the quick
automated tests that we expect developers to run frequently.
The dependency on socat makes me think we could probably put all of such
tests in a specific sub-directory. However, I predict that we will also
create a number of other ones which will be slower than average and which
will be unrelated to the CLI.
Maybe we could simply introduce levels :
- level 1 (the default) would contain only the immediate tests that cover
the internal state machine and HTTP compliance (the things we break the
most often by side effets when fixing a bug in the same area). Basically
we should expect to be able to run 100 tests in a second there and there
should be zero excuse for not running them before committing a patch
affecting a sensitive area.
- level 2 would cover some extra parts requiring a bit more time (eg:
CLI commands, horrible stuff involving tcploop) and would probably
be needed only when trying to ensure that a fix doesn't break
something unexpected.
- level 3 would be the painful one that we already know nobody will dare
to run. They would cover timeouts, health checks, etc. All the stuff
that takes multiple seconds per test would be there. They may occasionally
be run by a dev during lunch time, or at night by automated bots.
Then we could issue "make reg-tests" to run level 1 by default or
"make reg-tests LEVEL=" for the other ones. The idea is that I would
*really* like to encourage developers to run some basic tests before sending
patches, and we all know that none of us will accept to run them if they take
more time than what is needed to divert us (ie if you have time to switch to
reading your mails while the test runs, we won't run them because this will
create distraction).
I have attached #0003 patch for that in addition to these ones:
#0001 : as would say Olivier "Ooops, I'am an idiot etc".
reg-tests/ssl/h0.vtc did not run any https request.
#0002 : set the default value of HAPROXY_PROGRAM environment variable.
We will have to change the class of the already existing reg test files.
>From bbbef595937170b87d0707780968d031c95ca9e4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?=
Date: Mon, 25 Jun 2018 11:15:43 +0200
Subject: [PATCH 3/3] REGTEST/MINOR: Add levels to reg-tests target.
With this patch we can provide LEVEL environment variable when
running reg-tests Makefile targe (reg testing) to set the execution
level of the reg-tests make target to run.
LEVEL default value is 1.
LEVEL=1 is to run all h*.vtc files which are the most important
reg testing files (to test haproxy core, HTTP compliance etc).
LEVEL=2 is to run all s*.vtc files which are a bit slow tests,
for instance tests requiring external programs (curl, socat etc).
LEVEL=3 is to run all l*.vtc files which are test files with again
more slow or with little interest.
---
Makefile | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/Makefile b/Makefile
index 9ea3e80..817161f 100644
--- a/Makefile
+++ b/Makefile
@@ -1004,6 +1004,16 @@ reg-tests:
echo "Please make the VARNISHTEST_PROGRAM variable point to the location of the varnishtest program."; \
exit 1; \
fi
- @find reg-tests -type f -name "*.vtc" -print0 | \
- HAPROXY_PROGRAM=$${HAPROXY_PROGRAM:-$$PWD/haproxy} xargs -0 $(VARNISHTEST_PROGRAM) -l -t5
+ @export LEVEL=$${LEVEL:-1}; \
+ if [ $$LEVEL = 1 ] ; then \
+ EXPR='h*.vtc'; \
+ elif [ $$LEVEL = 2 ] ; then \
+ EXPR='s*.vtc'; \
+ elif [ $$LEVEL = 3 ] ; then \
+ EXPR='l*.vtc'; \
+ fi ; \
+ if [ -n "$$EXPR" ] ; then \
+ find reg-tests -type f -name "$$EXPR" -print0 | \
+ HAPROXY_PROGRAM=$${HAPROXY_PROGRAM:-$$PWD/haproxy} xargs -r -0 $(VARNISHTEST_PROGRAM) -l -t5 ; \
+ fi
.PHONY: reg-tests
--
2.1.4
>From 820602fa642c3143c3884056775a7077bf99c2eb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?=
Date: Mon, 25 Jun 2018 10:24:37 +0200
Subject: [PATCH 2/3] REGTEST/MINOR: Set HAPROXY_PROGRAM default value.
With this patch, we set HAPROXY_PROGRAM environment variable
default value to the haproxy executable of the current working directory.
So, if the current directory is the haproxy sources directory,
the reg-tests Makefile target may be run with this shorter command:
$ VARNISTEST_PROGRAM=<...> make reg-tests
in place of
$ VARNISTEST_PROGRAM=<...> HAPROXY_PROGRAM=<...> make reg-tests
---
Makefile | 2 +-
1 file
Re: [PATCH] REGTEST: stick-tables: Test expiration when used with table_*
On Thu, Jun 21, 2018 at 09:41:43PM +0100, Jonathan Matthews wrote: > On Thu, 21 Jun 2018 at 19:45, Willy Tarreau wrote: > > > Oh indeed I didn't even notice! The correct solution is to use the > > example.com domain for this, as explained in RFC2606/6761. No other > > domain possibly pointing to a valid location now or in the future > > should appear in test nor example files > > > [Gmail on mobile; forgive any formatting fubar] > > Example\.com resolves. There's a "you can use this domain in documentation" > site there. In fact it depends where : $ host example.com example.com has address 127.0.0.1 $ host example.com 8.8.8.8 Using domain server: Name: 8.8.8.8 Address: 8.8.8.8#53 Aliases: example.com has address 93.184.216.34 example.com has IPv6 address 2606:2800:220:1:248:1893:25c8:1946 > *Someone* is absorbing the traffic to that domain - I suggest > not putting it in .vtc files :-) It's hosted by IANA. But it's made exactly for this purpose in fact, because you know that people copy-pasting examples in documentation do whatever with them and sometimes even forget to replace certain lines. > I think the same RFC reserves .invalid as a TLD. Perhaps > missing.haproxy.invalid for when a DNS entry needs not to exist, and ... > something else for when it needs a real backend? I'm out of ideas on that > 2nd use case ... I'd suggest to indeed use .invalid when we really want a name to always fail to resolve, and example.com when we'd prefer it to resolve though we don't care if any service works or not. All other cases where you need a working service have to be provided with the embedded server in vtc and must not rely on any external service. Also that reminds me that if we have to provide example addresses for servers, there's a 192.2 network for this purpose which is listed in RFCs as well and that we should use instead of anything else. Cheers, Willy
Re: [PATCH] REGTEST: stick-tables: Test expiration when used with table_*
On Thu, 21 Jun 2018 at 19:45, Willy Tarreau wrote: > Oh indeed I didn't even notice! The correct solution is to use the > example.com domain for this, as explained in RFC2606/6761. No other > domain possibly pointing to a valid location now or in the future > should appear in test nor example files [Gmail on mobile; forgive any formatting fubar] Example\.com resolves. There's a "you can use this domain in documentation" site there. *Someone* is absorbing the traffic to that domain - I suggest not putting it in .vtc files :-) I think the same RFC reserves .invalid as a TLD. Perhaps missing.haproxy.invalid for when a DNS entry needs not to exist, and ... something else for when it needs a real backend? I'm out of ideas on that 2nd use case ... J > -- Jonathan Matthews London, UK http://www.jpluscplusm.com/contact.html
Re: [PATCH] REGTEST: stick-tables: Test expiration when used with table_*
On Thu, Jun 21, 2018 at 09:37:54AM +0200, Frederic Lecaille wrote: > On 06/20/2018 04:28 PM, Daniel Corbett wrote: > > Hello, > > > > Thanks for adding this integration Fred. Great job! > > > > Attached is a new regression test to check for stick-tables expiration when > > they are used with table_* converters as noted in commit id: > > 3e60b11100cbc812b77029ca142b83ac7a314db1 > > > > > > Thanks, > > > > -- Daniel > > > > Hello Daniel, > > Thank you for this test file. > > But as Christopher has just noticed, would it be possible to not use http > redirections to https://www.haproxy.com? > > I think you could use redirections to a varnishtest server. Oh indeed I didn't even notice! The correct solution is to use the example.com domain for this, as explained in RFC2606/6761. No other domain possibly pointing to a valid location now or in the future should appear in test nor example files, as you definitely don't want the domain admin to have to deal with misconfigured CI systems running your tests in loops, nor do you want that anyone copy-pastes your example configs for other purposes and accidently causes harm. Cheers, Willy
Re: [PATCH] REGTEST: stick-tables: Test expiration when used with table_*
On 06/20/2018 04:28 PM, Daniel Corbett wrote: Hello, Thanks for adding this integration Fred. Great job! Attached is a new regression test to check for stick-tables expiration when they are used with table_* converters as noted in commit id: 3e60b11100cbc812b77029ca142b83ac7a314db1 Thanks, -- Daniel Hello Daniel, Thank you for this test file. But as Christopher has just noticed, would it be possible to not use http redirections to https://www.haproxy.com? I think you could use redirections to a varnishtest server. Fred.
Re: [PATCH] REGTEST: stick-tables: Test expiration when used with table_*
Hi Daniel,
On Wed, Jun 20, 2018 at 10:28:43AM -0400, Daniel Corbett wrote:
> +shell -expect "used:0" {
> +echo "show table http1" |socat ${tmpdir}/h1/stats.sock -
^
This is the point where it will start to require that we organize the
reg tests better. First, socat is rarely installed by default so we'll
have to mention that it's required. Second, socat introduces half a
second delay before quitting, making it impractical for the quick
automated tests that we expect developers to run frequently.
The dependency on socat makes me think we could probably put all of such
tests in a specific sub-directory. However, I predict that we will also
create a number of other ones which will be slower than average and which
will be unrelated to the CLI.
Maybe we could simply introduce levels :
- level 1 (the default) would contain only the immediate tests that cover
the internal state machine and HTTP compliance (the things we break the
most often by side effets when fixing a bug in the same area). Basically
we should expect to be able to run 100 tests in a second there and there
should be zero excuse for not running them before committing a patch
affecting a sensitive area.
- level 2 would cover some extra parts requiring a bit more time (eg:
CLI commands, horrible stuff involving tcploop) and would probably
be needed only when trying to ensure that a fix doesn't break
something unexpected.
- level 3 would be the painful one that we already know nobody will dare
to run. They would cover timeouts, health checks, etc. All the stuff
that takes multiple seconds per test would be there. They may occasionally
be run by a dev during lunch time, or at night by automated bots.
Then we could issue "make reg-tests" to run level 1 by default or
"make reg-tests LEVEL=" for the other ones. The idea is that I would
*really* like to encourage developers to run some basic tests before sending
patches, and we all know that none of us will accept to run them if they take
more time than what is needed to divert us (ie if you have time to switch to
reading your mails while the test runs, we won't run them because this will
create distraction).
Also an important point since I'm seeing that the first tests focus on
known bugs (and I know it's often the point of regression testing) :
we've never reintroduced a bug that was already fixed, and it's likely
the same in most other projects. The reason is human nature consisting
in learning from our mistakes. So in practice, focusing on testing that
a very specific bug doesn't exist anymore is pointless because there is
zero chance that the exact same bug will re-appear, thus too precise a
test will just waste development time without bringing value.
HOWEVER, each such bugs belong to a *class* of bugs which are very
likely to be hit many times in various ways. This means two things :
1) each test must be written in a way that tries to cover the widest
spectrum of causes for a single class ;
2) when writing such a test, if other very closely related variants
are imagined, it's worth adding all these variants at once even
if they cover bugs that have not been met yet.
In your case, the test covers large parts (ie case 1 above), but only
focuses on "used:0". I think it's worth checking what else in the output
could possibly break. Given the number of entries tracked, there are very
likely a number of values set at once which deserve being matched because
we know what to expect there. By making the check cover them as well, we
increase the likeliness that this test will detect a regression affecting
stick tables, thus the usefulness of the test, hence the likeliness that
a developer will want to run it.
Then maybe you'll find that the request made doesn't trigger certain fields
and that we should slightly improve it to increase the coverage. This will
result in a test that still detects the bug you wanted to cover, but as part
of a class of bugs and not just this specific one. I suspect that for this
it will make sense to implement the server part to provide a valid response
in order to get more fields filled.
Just my two cents,
Willy
