Re: [PATCH] REGTEST/MINOR: skip seamless-reload test with abns socket on freebsd

[Willy Tarreau]

Hi Pieter,

On Sat, Dec 08, 2018 at 09:01:32PM +0100, PiBa-NL wrote:
> Hi List, Willy,
> 
> Added below to the reg-tests/seamless-reload/b0.vtc, I'm sure there are
> other targets to be excluded, but I'm not sure which. Or should i have
> listed 'all' targets except the 5 linux22 - linux2628 specific versions to
> be excluded? Only thing I know for sure it doesn't work on FreeBSD
> currently.. I can re-spin the patch with all non-linux targets listed if
> desired.
> 
> # expose-fd is available starting at version 1.8
> #REQUIRE_VERSION=1.8
> # abns@ sockets are not available on freebsd
> #EXCLUDE_TARGETS=freebsd

Thank you, that looks fine like this. Don't worry too much about
excluding targets you don't know. Few people will use these tests
anyway, they are mostly for developers and contributors. It's not
as if many of the other systems were used for development ;-) So
in the end if anyone faces issues, they'll quickly figure how to
blacklist their target and will submit an updated patch.

Thanks!
Willy

[PATCH] REGTEST/MINOR: skip seamless-reload test with abns socket on freebsd

[PiBa-NL]

Hi List, Willy,

Added below to the reg-tests/seamless-reload/b0.vtc, I'm sure there 
are other targets to be excluded, but I'm not sure which. Or should i 
have listed 'all' targets except the 5 linux22 - linux2628 specific 
versions to be excluded? Only thing I know for sure it doesn't work on 
FreeBSD currently.. I can re-spin the patch with all non-linux targets 
listed if desired.


# expose-fd is available starting at version 1.8
#REQUIRE_VERSION=1.8
# abns@ sockets are not available on freebsd
#EXCLUDE_TARGETS=freebsd

Regards,
PiBa-NL (Pieter)

From 3ef9f57b274b350b69d747f8f92fedfb8d283092 Mon Sep 17 00:00:00 2001
From: PiBa-NL 
Date: Sat, 8 Dec 2018 20:51:16 +0100
Subject: [PATCH] REGTEST/MINOR: skip seamless-reload test with abns socket on
 freebsd

abns sockets are not available on freebsd as such mark the test to skip
this OS and expose-fd was implemented first in 1.8 so require that
---
 reg-tests/seamless-reload/b0.vtc | 5 +
 1 file changed, 5 insertions(+)

diff --git a/reg-tests/seamless-reload/b0.vtc 
b/reg-tests/seamless-reload/b0.vtc
index 498e0c61..8f7acf64 100644
--- a/reg-tests/seamless-reload/b0.vtc
+++ b/reg-tests/seamless-reload/b0.vtc
@@ -11,6 +11,11 @@
 varnishtest "Seamless reload issue with abns sockets"
 feature ignore_unknown_macro
 
+# expose-fd is available starting at version 1.8
+#REQUIRE_VERSION=1.8
+# abns@ sockets are not available on freebsd
+#EXCLUDE_TARGETS=freebsd
+
 haproxy h1 -W -conf {
   global
 stats socket ${tmpdir}/h1/stats level admin expose-fd listeners
-- 
2.18.0.windows.1

Re: [PATCH] REGTEST/MINOR: remove double body specification for server txresp

[Willy Tarreau]

Hi Pieter,

On Sat, Dec 08, 2018 at 07:51:59PM +0100, PiBa-NL wrote:
> Hi List,
> 
> I'm getting a regtest failure for the h1.vtc test, seemingly because
> varnishtest doesn't like the definition.
> 
> Error (full log attached):
>  s1    0.0 http[23] |be-hdr-crc: 3634102538
>  s1    0.0 bodylen = 0
> **   s1    0.0 === txresp \
>  s1    0.0 Assert error in http_tx_parse_args(), vtc_http.c line 870: 
> Condition(body == nullbody) not true.
> 
> ***  h1    1.0 debug|:be.srvcls[000b:adfd]
> ***  h1    1.0 debug|:be.clicls[000b:adfd]
> 
> This comes from the following definition:
> 
>     txresp \
>       -status 234 \
>       -hdr "hdr1: val1" \
>       -hdr "hdr2:  val2a" \
>       -hdr "hdr2:    val2b" \
>       -hdr "hdr3:  val3a, val3b" \
>       -hdr "hdr4:" \
>       -bodylen 14 \
>       -body "This is a body"
> 
> Where both -bodylen and -body are defined, while it seems to me these 2
> settings are 'conflicting', as the '-bodylen' generates a kinda random body
> content. While '-body' defines the exact string to send as a body..

Ah interesting, I didn't have this issue here and thought the bodylen
was needed.

> Seems to me that the bodylen should be removed? Patch that does that
> attached.

It still works for me with your patch so I've applied it.

Thanks!
Willy

[PATCH] REGTEST/MINOR: remove double body specification for server txresp

[PiBa-NL]

Hi List,

I'm getting a regtest failure for the h1.vtc test, seemingly because 
varnishtest doesn't like the definition.


Error (full log attached):
 s1    0.0 http[23] |be-hdr-crc: 3634102538
 s1    0.0 bodylen = 0
**   s1    0.0 === txresp \
 s1    0.0 Assert error in http_tx_parse_args(), vtc_http.c line 
870:  Condition(body == nullbody) not true.


***  h1    1.0 debug|:be.srvcls[000b:adfd]
***  h1    1.0 debug|:be.clicls[000b:adfd]

This comes from the following definition:

    txresp \
      -status 234 \
      -hdr "hdr1: val1" \
      -hdr "hdr2:  val2a" \
      -hdr "hdr2:    val2b" \
      -hdr "hdr3:  val3a, val3b" \
      -hdr "hdr4:" \
      -bodylen 14 \
      -body "This is a body"

Where both -bodylen and -body are defined, while it seems to me these 2 
settings are 'conflicting', as the '-bodylen' generates a kinda random 
body content. While '-body' defines the exact string to send as a body..


Seems to me that the bodylen should be removed? Patch that does that 
attached.


Regards,

PiBa-NL

From e786be564e7dca1e3b347b6cc9e0af05c85e975b Mon Sep 17 00:00:00 2001
From: PiBa-NL 
Date: Sat, 8 Dec 2018 19:48:37 +0100
Subject: [PATCH] REGTEST/MINOR: remove double body specification for server
 txresp

fix http-rules/h0.vtc / http-rules/h0.vtc as both 'bodylen' and
'body' are specified, these settings conflict with each other as they
both generate/present the body to send.
---
 reg-tests/http-rules/h0.vtc | 1 -
 reg-tests/http-rules/h1.vtc | 1 -
 2 files changed, 2 deletions(-)

diff --git a/reg-tests/http-rules/h0.vtc b/reg-tests/http-rules/h0.vtc
index 25388f8a..aedb41ff 100644
--- a/reg-tests/http-rules/h0.vtc
+++ b/reg-tests/http-rules/h0.vtc
@@ -25,7 +25,6 @@ server s1 {
  -hdr "hdr2:val2b" \
  -hdr "hdr3:  val3a, val3b" \
  -hdr "hdr4:" \
- -bodylen 14 \
  -body "This is a body"
 
expect req.method == "GET"
diff --git a/reg-tests/http-rules/h1.vtc b/reg-tests/http-rules/h1.vtc
index 80522a1b..ca86f1b9 100644
--- a/reg-tests/http-rules/h1.vtc
+++ b/reg-tests/http-rules/h1.vtc
@@ -24,7 +24,6 @@ server s1 {
  -hdr "hdr2:val2b" \
  -hdr "hdr3:  val3a, val3b" \
  -hdr "hdr4:" \
- -bodylen 14 \
  -body "This is a body"
 
expect req.method == "GET"
-- 
2.18.0.windows.1

 top   0.0 extmacro def 
pwd=/usr/ports/net/haproxy-devel/work/haproxy-eb2bbba
 top   0.0 extmacro def localhost=127.0.0.1
 top   0.0 extmacro def bad_backend=127.0.0.1 28101
 top   0.0 extmacro def bad_ip=192.0.2.255
 top   0.0 macro def 
testdir=/usr/ports/net/haproxy-devel/work/haproxy-eb2bbba/./reg-tests/http-rules
 top   0.0 macro def 
tmpdir=/tmp/2018-12-08_19-21-06.FYVFcT/vtc.86314.04fe4024
*top   0.0 TEST ./reg-tests/http-rules/h1.vtc starting
**   top   0.0 === varnishtest "Composite HTTP manipulation test (H1 and H2 
cle...
*top   0.0 TEST Composite HTTP manipulation test (H1 and H2 clear to H1 
clear)
**   top   0.0 === feature ignore_unknown_macro
**   top   0.0 === server s1 {
**   s10.0 Starting server
 s10.0 macro def s1_addr=127.0.0.1
 s10.0 macro def s1_port=23305
 s10.0 macro def s1_sock=127.0.0.1 23305
*s10.0 Listen on 127.0.0.1 23305
**   top   0.0 === haproxy h1 -conf {
**   s10.0 Started on 127.0.0.1 23305
***  s10.0 Iteration 0
 h10.0 macro def h1_cli_sock=::1 23306
 h10.0 macro def h1_cli_addr=::1
 h10.0 macro def h1_cli_port=23306
 h10.0 setenv(cli, 5)
 h10.0 macro def h1_feh1_sock=::1 23307
 h10.0 macro def h1_feh1_addr=::1
 h10.0 macro def h1_feh1_port=23307
 h10.0 setenv(feh1, 6)
 h10.0 macro def h1_feh2_sock=::1 23308
 h10.0 macro def h1_feh2_addr=::1
 h10.0 macro def h1_feh2_port=23308
 h10.0 setenv(feh2, 7)
 h10.0 conf|global
 h10.0 conf|\tstats socket 
/tmp/2018-12-08_19-21-06.FYVFcT/vtc.86314.04fe4024/h1/stats.sock level admin 
mode 600
 h10.0 conf|stats socket "fd@${cli}" level admin
 h10.0 conf|
 h10.0 conf|defaults
 h10.0 conf|\tmode http
 h10.0 conf|\ttimeout connect 1s
 h10.0 conf|\ttimeout client  1s
 h10.0 conf|\ttimeout server  1s
 h10.0 conf|
 h10.0 conf|frontend fe
 h10.0 conf|\tbind "fd@${feh1}"
 h10.0 conf|\tbind "fd@${feh2}" proto h2
 h10.0 conf|
 h10.0 conf|\t requests
 h10.0 conf|\thttp-request set-var(req.method) method
 h10.0 conf|\thttp-request set-var(req.uri)url
 h10.0 conf|\thtt

Re: [PATCH] REGTEST/MINOR: script: add run-regtests.sh script

[Frederic Lecaille]

On 11/29/18 10:04 PM, Frederic Lecaille wrote:

On 11/29/18 5:36 AM, Willy Tarreau wrote:

Hi guys,

On Wed, Nov 28, 2018 at 11:17:22AM +0100, Frederic Lecaille wrote:

Perhaps we should "chmod +x" this script.


Good point, done here.

However I'm now seeing this when starting it :

## Starting varnishtest 
##

Testing with haproxy version: 1.8-dev0-3b0a6d-66
Assert error in start_test(), vtc_main.c line 283:
   Condition((mkdir(tmpdir, 0711)) == 0) not true.
   errno = 13 (Permission denied)
./scripts/run-regtests.sh: line 299: 21484 Aborted 
$VARNISHTEST_PROGRAM $varnishtestparams $verbose $jobcount -l -k -t 10 
$testlist
## Gathering failed results 
##

find: `/proc/tty/driver': Permission denied
find: `/proc/1/task/1/fd': Permission denied
find: `/proc/1/task/1/fdinfo': Permission denied
find: `/proc/1/task/1/ns': Permission denied
find: `/proc/1/fd': Permission denied
find: `/proc/1/map_files': Permission denied
(...)

Then I stopped it using Ctrl-C.

I'm seeing a few minor issues we must still address :

   - haproxy is started from the path, which means that on all systems
 where it's installed, it will be the one provided by the system and
 not the just built one which will be tested (as happened above),
 which is confusing. I think we shouldn't search for haproxy in the
 path but use $PWD/haproxy or ./haproxy or something like this.

   - it seems there are some issues in the way TMPDIR/TESTDIR are 
created,

 as it ended up with an empty TESTDIR. In my case, TMPDIR is not set,
 so TESTDIR was set to /tmp/varnishtest_haproxy. mkdir worked (I now
 have this directory), but a test is missing on this mkdir.

   - the way the sub-directory is created is problematic on shared
 development machines, as only the first user will own the directory
 and will thus prevent other users from creating their own overthere,
 thus it's probably preferable not to create an intermediary 
directory

 in the end.

   - in my case it's mktemp which failed :

 ++ date +%Y-%m-%d_%H-%M-%S
 + TESTRUNDATETIME=2018-11-29_05-03-43
 + TESTDIR=/tmp/varnishtest_haproxy
 + mkdir -p /tmp/varnishtest_haproxy
 ++ mktemp -d /tmp/varnishtest_haproxy/2018-11-29_05-03-43.
 mktemp: cannot make temp dir 
/tmp/varnishtest_haproxy/2018-11-29_05-03-43.: Invalid argument

 + TESTDIR=

 I haven't checked why yet, but we definitely need to test the status
 code for success as well.

   - in my opinion the script is still missing a number of quotes when 
using
 string variables, especially in the directory names. If someone 
is crazy

 enough to have TMPDIR="/tmp/Temp Dir", then he'll have some fun.

   - I'm seeing a linux-specific "rm -d" at the end, it's be better to
 replace it with rmdir.

   - there's "/usr/bin/env sh" at the top of the shell script. /bin/sh is
 the portable one, I've spent lots of time in the past editing 
scripts

 where env was forced to /usr/bin while it was placed in /bin on some
 systems, so I'm pretty certain that this one is not portable at 
all :-)


Here is a patch for haproxy (named 0001-REGTEST*) to fix these issues.

Note the other patch for varnishtest which is also required (sent to PHK).

Fred.


A remaining patch for varnishtest.

Fred.
>From bdcdde3744f26059dd2151f2c30ed2151500098e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?= 
Date: Fri, 30 Nov 2018 07:41:13 +0100
Subject: [PATCH 2/2] Support spaces in remaining filenames.

---
 bin/varnishtest/vtc_haproxy.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/bin/varnishtest/vtc_haproxy.c b/bin/varnishtest/vtc_haproxy.c
index e361aad78..ec3006505 100644
--- a/bin/varnishtest/vtc_haproxy.c
+++ b/bin/varnishtest/vtc_haproxy.c
@@ -477,7 +477,7 @@ haproxy_new(const char *name)
 	h->cli = haproxy_cli_new(h);
 	AN(h->cli);
 
-	bprintf(buf, "rm -rf %s ; mkdir -p \"%s\"", h->workdir, h->workdir);
+	bprintf(buf, "rm -rf \"%s\" ; mkdir -p \"%s\"", h->workdir, h->workdir);
 	AZ(system(buf));
 
 	VTAILQ_INSERT_TAIL(&haproxies, h, list);
@@ -498,7 +498,7 @@ haproxy_delete(struct haproxy *h)
 	vtc_logclose(h->vl);
 
 	if (!leave_temp) {
-		bprintf(buf, "rm -rf %s", h->workdir);
+		bprintf(buf, "rm -rf \"%s\"", h->workdir);
 		AZ(system(buf));
 	}
 
@@ -548,7 +548,7 @@ haproxy_start(struct haproxy *h)
 	vsb = VSB_new_auto();
 	AN(vsb);
 
-	VSB_printf(vsb, "exec %s", h->filename);
+	VSB_printf(vsb, "exec \"%s\"", h->filename);
 	if (h->opt_check_mode)
 		VSB_printf(vsb, " -c");
 	else if (h->opt_daemon)
@@ -567,7 +567,7 @@ haproxy_start(struct haproxy *h)
 		bprintf(buf, "%s/pid", h->workdir);
 		h->pid_fn = strdup(buf);
 		AN(h->pid_fn);
-		VSB_printf(vsb, " -p %s", h->pid_fn);
+		VSB_printf(vsb, " -p \"%s\"", h->pid_fn);
 	}
 
 	AZ(VSB_finish(vsb));
-- 
2.11.0

Re: [PATCH] REGTEST/MINOR: script: add run-regtests.sh script

[Willy Tarreau]

On Thu, Nov 29, 2018 at 10:04:29PM +0100, Frederic Lecaille wrote:
> Here is a patch for haproxy (named 0001-REGTEST*) to fix these issues.

Works like a charm, thank you Fred! Now applied.

Willy

Re: [PATCH] REGTEST/MINOR: script: add run-regtests.sh script

[Frederic Lecaille]

On 11/29/18 5:36 AM, Willy Tarreau wrote:

Hi guys,

On Wed, Nov 28, 2018 at 11:17:22AM +0100, Frederic Lecaille wrote:

Perhaps we should "chmod +x" this script.


Good point, done here.

However I'm now seeing this when starting it :

## Starting varnishtest ##
Testing with haproxy version: 1.8-dev0-3b0a6d-66
Assert error in start_test(), vtc_main.c line 283:
   Condition((mkdir(tmpdir, 0711)) == 0) not true.
   errno = 13 (Permission denied)
./scripts/run-regtests.sh: line 299: 21484 Aborted 
$VARNISHTEST_PROGRAM $varnishtestparams $verbose $jobcount -l -k -t 10 $testlist
## Gathering failed results ##
find: `/proc/tty/driver': Permission denied
find: `/proc/1/task/1/fd': Permission denied
find: `/proc/1/task/1/fdinfo': Permission denied
find: `/proc/1/task/1/ns': Permission denied
find: `/proc/1/fd': Permission denied
find: `/proc/1/map_files': Permission denied
(...)

Then I stopped it using Ctrl-C.

I'm seeing a few minor issues we must still address :

   - haproxy is started from the path, which means that on all systems
 where it's installed, it will be the one provided by the system and
 not the just built one which will be tested (as happened above),
 which is confusing. I think we shouldn't search for haproxy in the
 path but use $PWD/haproxy or ./haproxy or something like this.

   - it seems there are some issues in the way TMPDIR/TESTDIR are created,
 as it ended up with an empty TESTDIR. In my case, TMPDIR is not set,
 so TESTDIR was set to /tmp/varnishtest_haproxy. mkdir worked (I now
 have this directory), but a test is missing on this mkdir.

   - the way the sub-directory is created is problematic on shared
 development machines, as only the first user will own the directory
 and will thus prevent other users from creating their own overthere,
 thus it's probably preferable not to create an intermediary directory
 in the end.

   - in my case it's mktemp which failed :

 ++ date +%Y-%m-%d_%H-%M-%S
 + TESTRUNDATETIME=2018-11-29_05-03-43
 + TESTDIR=/tmp/varnishtest_haproxy
 + mkdir -p /tmp/varnishtest_haproxy
 ++ mktemp -d /tmp/varnishtest_haproxy/2018-11-29_05-03-43.
 mktemp: cannot make temp dir 
/tmp/varnishtest_haproxy/2018-11-29_05-03-43.: Invalid argument
 + TESTDIR=

 I haven't checked why yet, but we definitely need to test the status
 code for success as well.

   - in my opinion the script is still missing a number of quotes when using
 string variables, especially in the directory names. If someone is crazy
 enough to have TMPDIR="/tmp/Temp Dir", then he'll have some fun.

   - I'm seeing a linux-specific "rm -d" at the end, it's be better to
 replace it with rmdir.

   - there's "/usr/bin/env sh" at the top of the shell script. /bin/sh is
 the portable one, I've spent lots of time in the past editing scripts
 where env was forced to /usr/bin while it was placed in /bin on some
 systems, so I'm pretty certain that this one is not portable at all :-)


Here is a patch for haproxy (named 0001-REGTEST*) to fix these issues.

Note the other patch for varnishtest which is also required (sent to PHK).

Fred.
>From 7d316b74e65be4d75802feefe3d17b37afddf04e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?= 
Date: Thu, 29 Nov 2018 21:51:55 +0100
Subject: [PATCH] Add the support for spaces in filenames for the temporaty
 working directory.

---
 bin/varnishtest/vtc_haproxy.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/bin/varnishtest/vtc_haproxy.c b/bin/varnishtest/vtc_haproxy.c
index 9d12ed671..e361aad78 100644
--- a/bin/varnishtest/vtc_haproxy.c
+++ b/bin/varnishtest/vtc_haproxy.c
@@ -477,7 +477,7 @@ haproxy_new(const char *name)
 	h->cli = haproxy_cli_new(h);
 	AN(h->cli);
 
-	bprintf(buf, "rm -rf %s ; mkdir -p %s", h->workdir, h->workdir);
+	bprintf(buf, "rm -rf %s ; mkdir -p \"%s\"", h->workdir, h->workdir);
 	AZ(system(buf));
 
 	VTAILQ_INSERT_TAIL(&haproxies, h, list);
@@ -561,7 +561,7 @@ haproxy_start(struct haproxy *h)
 
 	VSB_printf(vsb, " %s", VSB_data(h->args));
 
-	VSB_printf(vsb, " -f %s ", h->cfg_fn);
+	VSB_printf(vsb, " -f \"%s\" ", h->cfg_fn);
 
 	if (h->opt_worker || h->opt_daemon) {
 		bprintf(buf, "%s/pid", h->workdir);
@@ -754,7 +754,7 @@ haproxy_write_conf(const struct haproxy *h, const char *cfg, int auto_be)
 	vsb2 = VSB_new_auto();
 	AN(vsb2);
 
-	VSB_printf(vsb, "global\n\tstats socket %s "
+	VSB_printf(vsb, "global\n\tstats socket \"%s\" "
 		   "level admin mode 600\n", h->cli_fn);
 	VSB_printf(vsb, "stats socket \"fd@${cli}\" level admin\n");
 	AZ(VSB_cat(vsb, cfg));
-- 
2.11.0

>From bd6bc4956f3bd005af7c27cf83e78c8cd14aea02 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?= 
Date: Thu, 29 Nov 2018 21:41:42 +0100
Subject: [PATCH] REGTEST: Fix s

Re: [PATCH] REGTEST/MINOR: script: add run-regtests.sh script

[PiBa-NL]

Hi Frederic,

Op 29-11-2018 om 19:18 schreef Frederic Lecaille:

On 11/29/18 8:47 AM, Willy Tarreau wrote:

On Thu, Nov 29, 2018 at 05:36:35AM +0100, Willy Tarreau wrote:
However I'm well aware that it's easier to work on improvements once 
the

script is merged, so what I've done now is to merge it and create a
temporary "reg-tests2" target in the makefile to use it without losing
the existing one. This way everyone can work in parallel, and once the
few issues seem reliably addressed, we can definitely replace the make
target.


Unfortunately ENOCOFFEE struck me this morning and I forgot to commit
my local changes so I merged the unmodified version which replaces the
"reg-test" target.

Thus now we're condemned to quickly fix these small issues :-)


Pieter,

I am having a look at all these issues.

Regards,

Fred.


If that means i don't have to do anything at this moment, thank you! (i 
suppose your turnaround time from issue>fix will also be shorter than 
waiting for my spare evening hour..)
Ill start checking some requirements of existing .vtc . And perhaps 
writing new one's.


Regards,

PiBa-NL (Pieter)

Re: [PATCH] REGTEST/MINOR: script: add run-regtests.sh script

[Frederic Lecaille]

On 11/29/18 8:47 AM, Willy Tarreau wrote:

On Thu, Nov 29, 2018 at 05:36:35AM +0100, Willy Tarreau wrote:

However I'm well aware that it's easier to work on improvements once the
script is merged, so what I've done now is to merge it and create a
temporary "reg-tests2" target in the makefile to use it without losing
the existing one. This way everyone can work in parallel, and once the
few issues seem reliably addressed, we can definitely replace the make
target.


Unfortunately ENOCOFFEE struck me this morning and I forgot to commit
my local changes so I merged the unmodified version which replaces the
"reg-test" target.

Thus now we're condemned to quickly fix these small issues :-)


Pieter,

I am having a look at all these issues.

Regards,

Fred.

Re: [PATCH] REGTEST/MINOR: script: add run-regtests.sh script

[Willy Tarreau]

On Thu, Nov 29, 2018 at 05:36:35AM +0100, Willy Tarreau wrote:
> However I'm well aware that it's easier to work on improvements once the
> script is merged, so what I've done now is to merge it and create a
> temporary "reg-tests2" target in the makefile to use it without losing
> the existing one. This way everyone can work in parallel, and once the
> few issues seem reliably addressed, we can definitely replace the make
> target.

Unfortunately ENOCOFFEE struck me this morning and I forgot to commit
my local changes so I merged the unmodified version which replaces the
"reg-test" target.

Thus now we're condemned to quickly fix these small issues :-)

I've found the problem related to mktemp, it uses "" as the suffix.
On my distro the man page says :

   The  mktemp  utility takes the given filename template and overwrites a
   portion of it to create a unique filename.  The  template  may  be  any
   filename   with   six   (6)   `Xs'   appended   to   it,   for  example
   /tmp/tfile.XX.

I've found that other systems demand at least 3 or at least 1. And the
glibc's mktemp() calll wants 6 anyway or returns EINVAL (the one I got).
Thus we should add 2 more "X" to make this work everywhere mktemp is
present (and test for the output as well).

Willy

Re: [PATCH] REGTEST/MINOR: script: add run-regtests.sh script

[Willy Tarreau]

Hi guys,

On Wed, Nov 28, 2018 at 11:17:22AM +0100, Frederic Lecaille wrote:
> Perhaps we should "chmod +x" this script.

Good point, done here.

However I'm now seeing this when starting it :

## Starting varnishtest ##
Testing with haproxy version: 1.8-dev0-3b0a6d-66
Assert error in start_test(), vtc_main.c line 283:
  Condition((mkdir(tmpdir, 0711)) == 0) not true.
  errno = 13 (Permission denied)
./scripts/run-regtests.sh: line 299: 21484 Aborted 
$VARNISHTEST_PROGRAM $varnishtestparams $verbose $jobcount -l -k -t 10 $testlist
## Gathering failed results ##
find: `/proc/tty/driver': Permission denied
find: `/proc/1/task/1/fd': Permission denied
find: `/proc/1/task/1/fdinfo': Permission denied
find: `/proc/1/task/1/ns': Permission denied
find: `/proc/1/fd': Permission denied
find: `/proc/1/map_files': Permission denied
(...)

Then I stopped it using Ctrl-C.

I'm seeing a few minor issues we must still address :

  - haproxy is started from the path, which means that on all systems
where it's installed, it will be the one provided by the system and
not the just built one which will be tested (as happened above),
which is confusing. I think we shouldn't search for haproxy in the
path but use $PWD/haproxy or ./haproxy or something like this.

  - it seems there are some issues in the way TMPDIR/TESTDIR are created,
as it ended up with an empty TESTDIR. In my case, TMPDIR is not set,
so TESTDIR was set to /tmp/varnishtest_haproxy. mkdir worked (I now
have this directory), but a test is missing on this mkdir.

  - the way the sub-directory is created is problematic on shared
development machines, as only the first user will own the directory
and will thus prevent other users from creating their own overthere,
thus it's probably preferable not to create an intermediary directory
in the end.

  - in my case it's mktemp which failed :

++ date +%Y-%m-%d_%H-%M-%S
+ TESTRUNDATETIME=2018-11-29_05-03-43
+ TESTDIR=/tmp/varnishtest_haproxy
+ mkdir -p /tmp/varnishtest_haproxy
++ mktemp -d /tmp/varnishtest_haproxy/2018-11-29_05-03-43.
mktemp: cannot make temp dir 
/tmp/varnishtest_haproxy/2018-11-29_05-03-43.: Invalid argument
+ TESTDIR=

I haven't checked why yet, but we definitely need to test the status
code for success as well.

  - in my opinion the script is still missing a number of quotes when using
string variables, especially in the directory names. If someone is crazy
enough to have TMPDIR="/tmp/Temp Dir", then he'll have some fun.

  - I'm seeing a linux-specific "rm -d" at the end, it's be better to
replace it with rmdir.

  - there's "/usr/bin/env sh" at the top of the shell script. /bin/sh is
the portable one, I've spent lots of time in the past editing scripts
where env was forced to /usr/bin while it was placed in /bin on some
systems, so I'm pretty certain that this one is not portable at all :-)

However I'm well aware that it's easier to work on improvements once the
script is merged, so what I've done now is to merge it and create a
temporary "reg-tests2" target in the makefile to use it without losing
the existing one. This way everyone can work in parallel, and once the
few issues seem reliably addressed, we can definitely replace the make
target.

Thanks!
Willy

Re: [PATCH] REGTEST/MINOR: script: add run-regtests.sh script

[Frederic Lecaille]

On 11/27/18 11:17 PM, PiBa-NL wrote:

Hi Frederic, Willy,

Op 27-11-2018 om 15:00 schreef Frederic Lecaille:

On 11/27/18 10:44 AM, Frederic Lecaille wrote:

On 11/27/18 9:52 AM, Willy Tarreau wrote:

Hi guys,

On Tue, Nov 27, 2018 at 09:45:25AM +0100, Frederic Lecaille wrote:

I put the script in the /reg-tests/ folder. Maybe it should have been
besides the Makefile in the / root ?


Yes I think it should be placed at the same level as the Makefile.


Well, we already have a "scripts" directory with the stuff used for
release and backport management. I think it perfectly has its place
there.

/scripts/ sounds good.


Note that the reg tests must be run from the Makefile with 
"reg-tests" target and possibly other arguments/variables.

Willy recently added REG_TEST_FILES variable.


I've changed the the script to include the LEVEL parameter almost the 
way the Makefile used it, changed behavior though so without the 
parameter it it runs all tests.


Ok.



I am sorry Pieter a remaining detail I should have mentioned before:

+  for i in $(find $TESTDIR/ -type d -name "vtc.*");
+  do
+    echo "## $(cat $i/INFO) ##"
+    echo "## test results in: $i"
+    grep --  $i/LOG
+
+    echo "## $(cat $i/INFO) ##" >> $TESTDIR/failedtests.log
+    echo "## test results in: $i" >> $TESTDIR/failedtests.log
+    grep --  $i/LOG >> $TESTDIR/failedtests.log
+    echo >> $TESTDIR/failedtests.log
+  done

may be shortened thanks to tee command like that:

 cat <<- EOF | tee $TESDIR/failedtests.log
 .
 .
 EOF

Removed some spaces for indentation which became part of the output.


To make <<- operator work (I think it is portable) we must
add TABS to align the code. And indeed the remaining spaces are taken
into an account. Let's forget that, it is ok with your modifications.



I have tested you script. For me it is OK. Good job!
Thank you a lot Pieter.


OK just let me know what to do with this, should I merge it as-is and
expect minor updates later, or do you or Pieter want to resend an
updated version ? I can adapt, let me know.


I have modified Pieter's patch for the modification mentioned above.
Seems to work ;)


Willy,

Here is a better patch which takes into an account the modification
above and yours (the script is added in "tests" directory).
I think Willy mentioned a 'scripts' directory? Changed patch to include 
that as well.


Aww yes, sorry.



You can merge it as-is.

Regards,

Fred


New path attached, which includes a LEVEL check.
And a modification of the Makefile to call the ./scripts/run-regtests.sh

Please can someone check it again before merging.?. Thanks guys :).


Perhaps we should "chmod +x" this script.

It is OK on my side.

Thank you Pieter.

Fred.

Re: [PATCH] REGTEST/MINOR: script: add run-regtests.sh script

[Willy Tarreau]

Hi Pieter,

On Tue, Nov 27, 2018 at 11:17:42PM +0100, PiBa-NL wrote:
> I've changed the the script to include the LEVEL parameter almost the way
> the Makefile used it, changed behavior though so without the parameter it it
> runs all tests.
(...)
> New path attached, which includes a LEVEL check.
> And a modification of the Makefile to call the ./scripts/run-regtests.sh

Thank you.

> Please can someone check it again before merging.?. Thanks guys :).

I've run a quick check and am mostly OK with it, though I'll wait for
Fred to re-check this morning before applying it.

I'll just perform a very minor modification below :

> diff --git a/Makefile b/Makefile
(...)
> + ./scripts/run-regtests.sh --LEVEL "$$LEVEL" $$REG_TEST_FILES

I'll reuse $(REG_TEST_FILES) instead of $$REG_TEST_FILES. The
difference is that the former is resolved by make and will take care
of makefile variables, while the latter is resolved by the shell and
will only see exported environment variables. This is fairly minor,
no need to respin a patch for this ;-)

Thanks,
Willy

Re: [PATCH] REGTEST/MINOR: script: add run-regtests.sh script

[PiBa-NL]

Hi Frederic, Willy,

Op 27-11-2018 om 15:00 schreef Frederic Lecaille:

On 11/27/18 10:44 AM, Frederic Lecaille wrote:

On 11/27/18 9:52 AM, Willy Tarreau wrote:

Hi guys,

On Tue, Nov 27, 2018 at 09:45:25AM +0100, Frederic Lecaille wrote:

I put the script in the /reg-tests/ folder. Maybe it should have been
besides the Makefile in the / root ?


Yes I think it should be placed at the same level as the Makefile.


Well, we already have a "scripts" directory with the stuff used for
release and backport management. I think it perfectly has its place
there.

/scripts/ sounds good.


Note that the reg tests must be run from the Makefile with 
"reg-tests" target and possibly other arguments/variables.

Willy recently added REG_TEST_FILES variable.


I've changed the the script to include the LEVEL parameter almost the 
way the Makefile used it, changed behavior though so without the 
parameter it it runs all tests.




I am sorry Pieter a remaining detail I should have mentioned before:

+  for i in $(find $TESTDIR/ -type d -name "vtc.*");
+  do
+    echo "## $(cat $i/INFO) ##"
+    echo "## test results in: $i"
+    grep --  $i/LOG
+
+    echo "## $(cat $i/INFO) ##" >> $TESTDIR/failedtests.log
+    echo "## test results in: $i" >> $TESTDIR/failedtests.log
+    grep --  $i/LOG >> $TESTDIR/failedtests.log
+    echo >> $TESTDIR/failedtests.log
+  done

may be shortened thanks to tee command like that:

 cat <<- EOF | tee $TESDIR/failedtests.log
 .
 .
 EOF

Removed some spaces for indentation which became part of the output.


I have tested you script. For me it is OK. Good job!
Thank you a lot Pieter.


OK just let me know what to do with this, should I merge it as-is and
expect minor updates later, or do you or Pieter want to resend an
updated version ? I can adapt, let me know.


I have modified Pieter's patch for the modification mentioned above.
Seems to work ;)


Willy,

Here is a better patch which takes into an account the modification
above and yours (the script is added in "tests" directory).
I think Willy mentioned a 'scripts' directory? Changed patch to include 
that as well.


You can merge it as-is.

Regards,

Fred


New path attached, which includes a LEVEL check.
And a modification of the Makefile to call the ./scripts/run-regtests.sh

Please can someone check it again before merging.?. Thanks guys :).

Regards,
PiBa-NL (Pieter)

From 989bf7ccbfd849deed450291121cdcc68796ba64 Mon Sep 17 00:00:00 2001
From: PiBa-NL 
Date: Tue, 27 Nov 2018 22:26:38 +0100
Subject: [PATCH] REGTEST/MINOR: script: add run-regtests.sh script

Some tests require a minimal haproxy version or compilation options to be
able to run successfully. This script allows to add 'requirements' to tests
to check so they will automatically be skipped if a requirement is not met.
The script supports several parameters to slightly modify its behavior
including the directories to search for tests.

Also some features are not available for certain OS's these can also
be 'excluded', this should allow for the complete set of test cases to be
run on any OS against any haproxy release without 'expected failures'.

The test .vtc files will need to be modified to include their 'requirements'
by listing including text options as shown below:
#EXCLUDE_TARGETS=dos,freebsd,windows
#REQUIRE_OPTIONS=ZLIB,OPENSSL,LUA
#REQUIRE_VERSION=0.0
#REQUIRE_VERSION_BELOW=99.9,
When excluding a OS by its TARGET, please do make a comment why the test
can not succeed on that TARGET.
---
 Makefile|  25 +---
 scripts/run-regtests.sh | 317 
 2 files changed, 318 insertions(+), 24 deletions(-)
 create mode 100644 scripts/run-regtests.sh

diff --git a/Makefile b/Makefile
index 6d7a0159..aa6d66b9 100644
--- a/Makefile
+++ b/Makefile
@@ -1094,28 +1094,5 @@ opts:
 # LEVEL 3 scripts are low interest scripts (prefixed with 'l' letter).
 # LEVEL 4 scripts are in relation with bugs they help to reproduce (prefixed 
with 'b' letter).
 reg-tests:
-   $(Q)if [ ! -x "$(VARNISHTEST_PROGRAM)" ]; then \
-   echo "Please make the VARNISHTEST_PROGRAM variable point to the 
location of the varnishtest program."; \
-   exit 1; \
-   fi
-   $(Q)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'; \
-   elif [ $$LEVEL = 4 ] ; then \
-  EXPR='b*.vtc'; \
-   fi ; \
-   if [ -n "$(REG_TEST_FILES)" ] ; then \
-  err=0; \
-  for n in $(REG_TEST_FILES); do \
- HAPROXY_P

Re: [PATCH] REGTEST/MINOR: script: add run-regtests.sh script

[Frederic Lecaille]

On 11/27/18 10:44 AM, Frederic Lecaille wrote:

On 11/27/18 9:52 AM, Willy Tarreau wrote:

Hi guys,

On Tue, Nov 27, 2018 at 09:45:25AM +0100, Frederic Lecaille wrote:

I put the script in the /reg-tests/ folder. Maybe it should have been
besides the Makefile in the / root ?


Yes I think it should be placed at the same level as the Makefile.


Well, we already have a "scripts" directory with the stuff used for
release and backport management. I think it perfectly has its place
there.


I am sorry Pieter a remaining detail I should have mentioned before:

+  for i in $(find $TESTDIR/ -type d -name "vtc.*");
+  do
+    echo "## $(cat $i/INFO) ##"
+    echo "## test results in: $i"
+    grep --  $i/LOG
+
+    echo "## $(cat $i/INFO) ##" >> $TESTDIR/failedtests.log
+    echo "## test results in: $i" >> $TESTDIR/failedtests.log
+    grep --  $i/LOG >> $TESTDIR/failedtests.log
+    echo >> $TESTDIR/failedtests.log
+  done

may be shortened thanks to tee command like that:

 cat <<- EOF | tee $TESDIR/failedtests.log
 .
 .
 EOF

I have tested you script. For me it is OK. Good job!
Thank you a lot Pieter.


OK just let me know what to do with this, should I merge it as-is and
expect minor updates later, or do you or Pieter want to resend an
updated version ? I can adapt, let me know.


I have modified Pieter's patch for the modification mentioned above.
Seems to work ;)


Willy,

Here is a better patch which takes into an account the modification
above and yours (the script is added in "tests" directory).

You can merge it as-is.

Regards,

Fred
>From 4432c10a0a822619c152aa187f18b2f6478ac565 Mon Sep 17 00:00:00 2001
From: PiBa-NL 
Date: Sun, 25 Nov 2018 16:46:44 +0100
Subject: [PATCH] REGTEST/MINOR: script: add run-regtests.sh script

Some tests require a minimal haproxy version or compilation options to be
able to run successfully. This script allows to add 'requirements' to tests
to check so they will automatically be skipped if a requirement is not met.
The script supports several parameters to slightly modify its behavior
including the directories to search for tests.

Also some features are not available for certain OS's these can also
be 'excluded', this should allow for the complete set of test cases to be
run on any OS against any haproxy release without 'expected failures'.

The test .vtc files will need to be modified to include their 'requirements'
by listing including text options as shown below:
#EXCLUDE_TARGETS=dos,freebsd,windows
#REQUIRE_OPTIONS=ZLIB,OPENSSL,LUA
#REQUIRE_VERSION=0.0
#REQUIRE_VERSION_BELOW=99.9,
When excluding a OS by its TARGET, please do make a comment why the test
can not succeed on that TARGET.
---
 tests/run-regtests.sh | 300 ++
 1 file changed, 300 insertions(+)
 create mode 100644 tests/run-regtests.sh

diff --git a/tests/run-regtests.sh b/tests/run-regtests.sh
new file mode 100644
index ..1094117f
--- /dev/null
+++ b/tests/run-regtests.sh
@@ -0,0 +1,300 @@
+#!/usr/bin/env sh
+
+if [ "$1" = "--help" ]; then
+  cat << EOF
+### run-regtests.sh ###
+  Running run-regtests.sh --help shows this information about how to use it
+
+  Run without parameters to run all tests in the current folder (including subfolders)
+run-regtests.sh
+
+  Provide paths to run tests from (including subfolders):
+run-regtests.sh ./tests1 ./tests2
+
+  Parameters:
+--j , To run varnishtest with multiple jobs / threads for a faster overall result
+  run-regtests.sh ./fasttest --j 16
+
+--v, to run verbose
+  run-regtests.sh --v, disables the default varnishtest 'quiet' parameter
+
+--varnishtestparams , passes custom ARGS to varnishtest
+  run-regtests.sh --varnishtestparams "-n 10"
+
+  Including text below into a .vtc file will check for its requirements 
+  related to haproxy's target and compilation options
+# Below targets are not capable of completing this test succesfully
+#EXCLUDE_TARGET=freebsd, abns sockets are not available on freebsd
+
+#EXCLUDE_TARGETS=dos,freebsd,windows
+
+# Below option is required to complete this test succesfully
+#REQUIRE_OPTION=OPENSSL, this test needs OPENSSL compiled in.
+ 
+#REQUIRE_OPTIONS=ZLIB,OPENSSL,LUA
+
+# To define a range of versions that a test can run with:
+#REQUIRE_VERSION=0.0
+#REQUIRE_VERSION_BELOW=99.9
+
+  Configure environment variables to set the haproxy and varnishtest binaries to use
+setenv HAPROXY_PROGRAM /usr/local/sbin/haproxy
+setenv VARNISHTEST_PROGRAM /usr/local/bin/varnishtest
+EOF
+  return
+fi
+
+_startswith() {
+  _str="$1"
+  _sub="$2"
+  echo "$_str" | grep "^$_sub" >/dev/null 2>&1
+}
+
+_findtests() {
+  set

Re: [PATCH] REGTEST/MINOR: script: add run-regtests.sh script

[Frederic Lecaille]

On 11/27/18 9:52 AM, Willy Tarreau wrote:

Hi guys,

On Tue, Nov 27, 2018 at 09:45:25AM +0100, Frederic Lecaille wrote:

I put the script in the /reg-tests/ folder. Maybe it should have been
besides the Makefile in the / root ?


Yes I think it should be placed at the same level as the Makefile.


Well, we already have a "scripts" directory with the stuff used for
release and backport management. I think it perfectly has its place
there.


I am sorry Pieter a remaining detail I should have mentioned before:

+  for i in $(find $TESTDIR/ -type d -name "vtc.*");
+  do
+echo "## $(cat $i/INFO) ##"
+echo "## test results in: $i"
+grep --  $i/LOG
+
+echo "## $(cat $i/INFO) ##" >> $TESTDIR/failedtests.log
+echo "## test results in: $i" >> $TESTDIR/failedtests.log
+grep --  $i/LOG >> $TESTDIR/failedtests.log
+echo >> $TESTDIR/failedtests.log
+  done

may be shortened thanks to tee command like that:

 cat <<- EOF | tee $TESDIR/failedtests.log
 .
 .
 EOF

I have tested you script. For me it is OK. Good job!
Thank you a lot Pieter.


OK just let me know what to do with this, should I merge it as-is and
expect minor updates later, or do you or Pieter want to resend an
updated version ? I can adapt, let me know.


I have modified Pieter's patch for the modification mentioned above.
Seems to work ;)

Fred.



>From 4432c10a0a822619c152aa187f18b2f6478ac565 Mon Sep 17 00:00:00 2001
From: PiBa-NL 
Date: Sun, 25 Nov 2018 16:46:44 +0100
Subject: [PATCH] REGTEST/MINOR: script: add run-regtests.sh script

Some tests require a minimal haproxy version or compilation options to be
able to run successfully. This script allows to add 'requirements' to tests
to check so they will automatically be skipped if a requirement is not met.
The script supports several parameters to slightly modify its behavior
including the directories to search for tests.

Also some features are not available for certain OS's these can also
be 'excluded', this should allow for the complete set of test cases to be
run on any OS against any haproxy release without 'expected failures'.

The test .vtc files will need to be modified to include their 'requirements'
by listing including text options as shown below:
#EXCLUDE_TARGETS=dos,freebsd,windows
#REQUIRE_OPTIONS=ZLIB,OPENSSL,LUA
#REQUIRE_VERSION=0.0
#REQUIRE_VERSION_BELOW=99.9,
When excluding a OS by its TARGET, please do make a comment why the test
can not succeed on that TARGET.
---
 reg-tests/run-regtests.sh | 300 ++
 1 file changed, 300 insertions(+)
 create mode 100644 reg-tests/run-regtests.sh

diff --git a/reg-tests/run-regtests.sh b/reg-tests/run-regtests.sh
new file mode 100644
index ..1094117f
--- /dev/null
+++ b/reg-tests/run-regtests.sh
@@ -0,0 +1,300 @@
+#!/usr/bin/env sh
+
+if [ "$1" = "--help" ]; then
+  cat << EOF
+### run-regtests.sh ###
+  Running run-regtests.sh --help shows this information about how to use it
+
+  Run without parameters to run all tests in the current folder (including subfolders)
+run-regtests.sh
+
+  Provide paths to run tests from (including subfolders):
+run-regtests.sh ./tests1 ./tests2
+
+  Parameters:
+--j , To run varnishtest with multiple jobs / threads for a faster overall result
+  run-regtests.sh ./fasttest --j 16
+
+--v, to run verbose
+  run-regtests.sh --v, disables the default varnishtest 'quiet' parameter
+
+--varnishtestparams , passes custom ARGS to varnishtest
+  run-regtests.sh --varnishtestparams "-n 10"
+
+  Including text below into a .vtc file will check for its requirements 
+  related to haproxy's target and compilation options
+# Below targets are not capable of completing this test succesfully
+#EXCLUDE_TARGET=freebsd, abns sockets are not available on freebsd
+
+#EXCLUDE_TARGETS=dos,freebsd,windows
+
+# Below option is required to complete this test succesfully
+#REQUIRE_OPTION=OPENSSL, this test needs OPENSSL compiled in.
+ 
+#REQUIRE_OPTIONS=ZLIB,OPENSSL,LUA
+
+# To define a range of versions that a test can run with:
+#REQUIRE_VERSION=0.0
+#REQUIRE_VERSION_BELOW=99.9
+
+  Configure environment variables to set the haproxy and varnishtest binaries to use
+setenv HAPROXY_PROGRAM /usr/local/sbin/haproxy
+setenv VARNISHTEST_PROGRAM /usr/local/bin/varnishtest
+EOF
+  return
+fi
+
+_startswith() {
+  _str="$1"
+  _sub="$2"
+  echo "$_str" | grep "^$_sub" >/dev/null 2>&1
+}
+
+_findtests() {
+  set -f
+  for i in $( find "$1" -name "*.vtc" ); do
+skiptest=
+require_version="$(grep "#REQUIRE_VERSION=" "$i" | sed -e 's/.*=//')"

Re: [PATCH] REGTEST/MINOR: script: add run-regtests.sh script

[Willy Tarreau]

Hi guys,

On Tue, Nov 27, 2018 at 09:45:25AM +0100, Frederic Lecaille wrote:
> > I put the script in the /reg-tests/ folder. Maybe it should have been
> > besides the Makefile in the / root ?
> 
> Yes I think it should be placed at the same level as the Makefile.

Well, we already have a "scripts" directory with the stuff used for
release and backport management. I think it perfectly has its place
there.

> I am sorry Pieter a remaining detail I should have mentioned before:
> 
> +  for i in $(find $TESTDIR/ -type d -name "vtc.*");
> +  do
> +echo "## $(cat $i/INFO) ##"
> +echo "## test results in: $i"
> +grep --  $i/LOG
> +
> +echo "## $(cat $i/INFO) ##" >> $TESTDIR/failedtests.log
> +echo "## test results in: $i" >> $TESTDIR/failedtests.log
> +grep --  $i/LOG >> $TESTDIR/failedtests.log
> +echo >> $TESTDIR/failedtests.log
> +  done
> 
> may be shortened thanks to tee command like that:
> 
> cat <<- EOF | tee $TESDIR/failedtests.log
> .
> .
> EOF
> 
> I have tested you script. For me it is OK. Good job!
> Thank you a lot Pieter.

OK just let me know what to do with this, should I merge it as-is and
expect minor updates later, or do you or Pieter want to resend an
updated version ? I can adapt, let me know.

thanks!
Willy

Re: [PATCH] REGTEST/MINOR: script: add run-regtests.sh script

[Frederic Lecaille]

On 11/25/18 4:59 PM, PiBa-NL wrote:

Hi Frederic, Willy,


Hi Pieter,


Added the varnishtest script we have been discussing as a .patch this time.

I put the script in the /reg-tests/ folder. Maybe it should have been 
besides the Makefile in the / root ?


Yes I think it should be placed at the same level as the Makefile.

Note that the reg tests must be run from the Makefile with "reg-tests" 
target and possibly other arguments/variables.


Willy recently added REG_TEST_FILES variable.


Also i put a bit of comments into the commit.

I hope it is okay like this? If not, feel free to comment on them or 
change them as required.


I am sorry Pieter a remaining detail I should have mentioned before:

+  for i in $(find $TESTDIR/ -type d -name "vtc.*");
+  do
+echo "## $(cat $i/INFO) ##"
+echo "## test results in: $i"
+grep --  $i/LOG
+
+echo "## $(cat $i/INFO) ##" >> $TESTDIR/failedtests.log
+echo "## test results in: $i" >> $TESTDIR/failedtests.log
+grep --  $i/LOG >> $TESTDIR/failedtests.log
+echo >> $TESTDIR/failedtests.log
+  done

may be shortened thanks to tee command like that:

cat <<- EOF | tee $TESDIR/failedtests.log
.
.
EOF

I have tested you script. For me it is OK. Good job!
Thank you a lot Pieter.

Once this one is 'accepted' ill create a few patches for the existing 
.vtc files to include their requirements. (at least the more obvious 
ones..)


Regards,
PiBa-NL (Pieter)

[PATCH] REGTEST/MINOR: script: add run-regtests.sh script

[PiBa-NL]

Hi Frederic, Willy,

Added the varnishtest script we have been discussing as a .patch this time.

I put the script in the /reg-tests/ folder. Maybe it should have been 
besides the Makefile in the / root ?


Also i put a bit of comments into the commit.

I hope it is okay like this? If not, feel free to comment on them or 
change them as required.


Once this one is 'accepted' ill create a few patches for the existing 
.vtc files to include their requirements. (at least the more obvious ones..)


Regards,
PiBa-NL (Pieter)

From 4432c10a0a822619c152aa187f18b2f6478ac565 Mon Sep 17 00:00:00 2001
From: PiBa-NL 
Date: Sun, 25 Nov 2018 16:46:44 +0100
Subject: [PATCH] REGTEST/MINOR: script: add run-regtests.sh script

Some tests require a minimal haproxy version or compilation options to be
able to run successfully. This script allows to add 'requirements' to tests
to check so they will automatically be skipped if a requirement is not met.
The script supports several parameters to slightly modify its behavior
including the directories to search for tests.

Also some features are not available for certain OS's these can also
be 'excluded', this should allow for the complete set of test cases to be
run on any OS against any haproxy release without 'expected failures'.

The test .vtc files will need to be modified to include their 'requirements'
by listing including text options as shown below:
#EXCLUDE_TARGETS=dos,freebsd,windows
#REQUIRE_OPTIONS=ZLIB,OPENSSL,LUA
#REQUIRE_VERSION=0.0
#REQUIRE_VERSION_BELOW=99.9,
When excluding a OS by its TARGET, please do make a comment why the test
can not succeed on that TARGET.
---
 reg-tests/run-regtests.sh | 303 ++
 1 file changed, 303 insertions(+)
 create mode 100644 reg-tests/run-regtests.sh

diff --git a/reg-tests/run-regtests.sh b/reg-tests/run-regtests.sh
new file mode 100644
index ..1094117f
--- /dev/null
+++ b/reg-tests/run-regtests.sh
@@ -0,0 +1,303 @@
+#!/usr/bin/env sh
+
+if [ "$1" = "--help" ]; then
+  cat << EOF
+### run-regtests.sh ###
+  Running run-regtests.sh --help shows this information about how to use it
+
+  Run without parameters to run all tests in the current folder (including 
subfolders)
+run-regtests.sh
+
+  Provide paths to run tests from (including subfolders):
+run-regtests.sh ./tests1 ./tests2
+
+  Parameters:
+--j , To run varnishtest with multiple jobs / threads for a faster 
overall result
+  run-regtests.sh ./fasttest --j 16
+
+--v, to run verbose
+  run-regtests.sh --v, disables the default varnishtest 'quiet' parameter
+
+--varnishtestparams , passes custom ARGS to varnishtest
+  run-regtests.sh --varnishtestparams "-n 10"
+
+  Including text below into a .vtc file will check for its requirements 
+  related to haproxy's target and compilation options
+# Below targets are not capable of completing this test succesfully
+#EXCLUDE_TARGET=freebsd, abns sockets are not available on freebsd
+
+#EXCLUDE_TARGETS=dos,freebsd,windows
+
+# Below option is required to complete this test succesfully
+#REQUIRE_OPTION=OPENSSL, this test needs OPENSSL compiled in.
+ 
+#REQUIRE_OPTIONS=ZLIB,OPENSSL,LUA
+
+# To define a range of versions that a test can run with:
+#REQUIRE_VERSION=0.0
+#REQUIRE_VERSION_BELOW=99.9
+
+  Configure environment variables to set the haproxy and varnishtest binaries 
to use
+setenv HAPROXY_PROGRAM /usr/local/sbin/haproxy
+setenv VARNISHTEST_PROGRAM /usr/local/bin/varnishtest
+EOF
+  return
+fi
+
+_startswith() {
+  _str="$1"
+  _sub="$2"
+  echo "$_str" | grep "^$_sub" >/dev/null 2>&1
+}
+
+_findtests() {
+  set -f
+  for i in $( find "$1" -name "*.vtc" ); do
+skiptest=
+require_version="$(grep "#REQUIRE_VERSION=" "$i" | sed -e 's/.*=//')"
+require_version_below="$(grep "#REQUIRE_VERSION_BELOW=" "$i" | sed -e 
's/.*=//')"
+require_options="$(grep "#REQUIRE_OPTIONS=" "$i" | sed -e 's/.*=//')"
+exclude_targets=",$(grep "#EXCLUDE_TARGETS=" "$i" | sed -e 's/.*=//'),"
+
+if [ -n "$require_version" ]; then
+  if [ $(_version "$HAPROXY_VERSION") -lt $(_version "$require_version") 
]; then
+echo "  Skip $i because option haproxy is version: $HAPROXY_VERSION"
+echo "REASON: this test requires at least version: 
$require_version"
+skiptest=1
+  fi
+fi
+if [ -n "$require_version_below" ]; then
+  if [ $(_version "$HAPROXY_VERSION") -ge $(_version 
"$require_version_below") ]; then
+echo "  Skip $i because option

Re: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters

[Willy Tarreau]

On Fri, Oct 12, 2018 at 02:29:51PM +0200, PiBa-NL wrote:
> Op 12-10-2018 om 10:53 schreef William Lallemand:
> > The attached patch should fix the issue.
> 
> The patch works for me, thanks.

Great, patch now merged.

Thanks!
Willy

Re: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters

[PiBa-NL]

Hi William,

Op 12-10-2018 om 10:53 schreef William Lallemand:

The attached patch should fix the issue.


The patch works for me, thanks.

Regards,

PiBa-NL (Pieter)

Re: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters

[William Lallemand]

On Thu, Oct 11, 2018 at 11:06:38PM +0200, William Lallemand wrote:
> On Thu, Oct 11, 2018 at 09:31:48PM +0200, PiBa-NL wrote:
> > Hi Willy, William,
> > 
> 
> Hi Peter,
> 
> Regarding this part:
>  
> > -Connection and request counters to low when ran as regtest from 
> > varnishtest (bug?)
> > It turns out that starting haproxy from varnishtest, and using -W 
> > master-worker mode, actually creates 2 processes that are handling 
> > traffic. That explains that a large part of connections isn't seen by 
> > the other haproxy instance and stats showing to low amounts of 
> > connections. Bisecting it seems to fail on this commit: b3f2be3 , 
> > perhaps @William can you take a look at it? Not really sure when this 
> > occurs in a 'real' environment, it doesn't seem to happen when manually 
> > running haproxy -W, but still its strange that when varnisttest is 
> > calling haproxy this occurs.
> > 
> 
> There was an exception in the master's code regarding the inherited FDs (fd@),
> which is the case with varnishtest. There is a flag which was forbidding the
> master to close the FD in 1.8.
> 
> Now there is a polling loop in the master and I think there is a side effect
> where the FD is registered in the master polling loop.
> 
> It's just a guess, I'll look at it tomorrow.
> 
> Cheers,
> 

The attached patch should fix the issue.

-- 
William Lallemand
>From 3f2c30a0f15e30b7941245d3c5b20cbd5561489f Mon Sep 17 00:00:00 2001
From: William Lallemand 
Date: Fri, 12 Oct 2018 10:39:54 +0200
Subject: [PATCH] BUG/MEDIUM: mworker: don't poll on LI_O_INHERITED listeners

The listeners with the LI_O_INHERITED flag were deleted but not unbound
which is a problem since we have a polling in the master.

This patch unbind every listeners which are not require for the master,
but does not close the FD of those that have a LI_O_INHERITED flag.
---
 src/haproxy.c | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/src/haproxy.c b/src/haproxy.c
index a7b07a267..82da86222 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -615,13 +615,17 @@ static void mworker_cleanlisteners()
 
 	for (curproxy = proxies_list; curproxy; curproxy = curproxy->next) {
 		list_for_each_entry_safe(l, l_next, &curproxy->conf.listeners, by_fe) {
-			/* does not close if the FD is inherited with fd@
-			 * from the parent process */
-			if (!(l->options & (LI_O_INHERITED|LI_O_MWORKER)))
-unbind_listener(l);
 			/* remove the listener, but not those we need in the master... */
-			if (!(l->options & LI_O_MWORKER))
+			if (!(l->options & LI_O_MWORKER)) {
+/* unbind the listener but does not close if
+   the FD is inherited with fd@ from the parent
+   process */
+if (l->options & LI_O_INHERITED)
+	unbind_listener_no_close(l);
+else
+	unbind_listener(l);
 delete_listener(l);
+			}
 		}
 	}
 }
-- 
2.16.4

Re: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters

[William Lallemand]

On Thu, Oct 11, 2018 at 09:31:48PM +0200, PiBa-NL wrote:
> Hi Willy, William,
> 

Hi Peter,

Regarding this part:
 
> -Connection and request counters to low when ran as regtest from 
> varnishtest (bug?)
> It turns out that starting haproxy from varnishtest, and using -W 
> master-worker mode, actually creates 2 processes that are handling 
> traffic. That explains that a large part of connections isn't seen by 
> the other haproxy instance and stats showing to low amounts of 
> connections. Bisecting it seems to fail on this commit: b3f2be3 , 
> perhaps @William can you take a look at it? Not really sure when this 
> occurs in a 'real' environment, it doesn't seem to happen when manually 
> running haproxy -W, but still its strange that when varnisttest is 
> calling haproxy this occurs.
> 

There was an exception in the master's code regarding the inherited FDs (fd@),
which is the case with varnishtest. There is a flag which was forbidding the
master to close the FD in 1.8.

Now there is a polling loop in the master and I think there is a side effect
where the FD is registered in the master polling loop.

It's just a guess, I'll look at it tomorrow.

Cheers,

-- 
William Lallemand

Re: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters

[PiBa-NL]

Hi Willy, William,


Op 2-10-2018 om 3:56 schreef Willy Tarreau:

it's important to cut this into pieces
to figure what it's showing.


A little update on this issue split in 2 pieces.

-Connection and request counters to low when ran as regtest from 
varnishtest (bug?)
It turns out that starting haproxy from varnishtest, and using -W 
master-worker mode, actually creates 2 processes that are handling 
traffic. That explains that a large part of connections isn't seen by 
the other haproxy instance and stats showing to low amounts of 
connections. Bisecting it seems to fail on this commit: b3f2be3 , 
perhaps @William can you take a look at it? Not really sure when this 
occurs in a 'real' environment, it doesn't seem to happen when manually 
running haproxy -W, but still its strange that when varnisttest is 
calling haproxy this occurs.


-Request counter to high (possibly a improvement request?)
The http_end_txn_clean_session(..) is called which increments the 
counter on a finished http response, and i was testing with 2 different 
methods for 1.8 and 1.9 due to missing length converter i used in my 1.9 
test, which makes the comparison unfair. Sorry i didn't realize this 
earlier i thought it did 'more or less' the same, that seems to have 
been 'less'. Together with that i found the numbers odd/unexpected i 
assumed a bigger problem that it actually seems to be, perhaps its not 
even that bad, haproxy is 'preparing' for a second request over the 
keep-alive connection if i understand correctly. Which eventually 
doesn't happen, but is counted. Maybe that is a point that can be 
improved in a future version if time permits.? Or would it even be 
expected to behave like that?


Regards,
PiBa-NL (Pieter)

Re: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters

[Willy Tarreau]

Hi Pieter,

On Thu, Oct 04, 2018 at 09:43:36PM +0200, PiBa-NL wrote:
> Hi Willy,
> 
> Op 2-10-2018 om 3:56 schreef Willy Tarreau:
> > it's important to cut this into pieces
> > to figure what it's showing.
> 
> Okay, the good thing i suppose we already have a reproduction sort-off..
> 
> What would be the best way to try and get to the bottom of this? Add debug
> output to haproxy code? Get some kind of trace? Anything in particular that
> would be of interest?

If you manage to figure a way to reproduce it with the "connection: close"
header, that's one different bug from what I observed. However to be honest,
given how much work we still have for the rework of the HTTP engine, for now
I'd rather continue to work on the stuff we still have to do than spend one
week figuring why this counter is sometimes wrong, so if you or anyone else
wants to dig this one down to the root cause, I would really appreciate it.

Thanks!
Willy

Re: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters

[PiBa-NL]

Hi Willy,

Op 2-10-2018 om 3:56 schreef Willy Tarreau:

it's important to cut this into pieces
to figure what it's showing.


Okay, the good thing i suppose we already have a reproduction sort-off..

What would be the best way to try and get to the bottom of this? Add 
debug output to haproxy code? Get some kind of trace? Anything in 
particular that would be of interest?


Regards,

PiBa-NL (Pieter)

Re: [PATCH] REGTEST/MINOR: compatibility: use unix@ instead of abns@

[Willy Tarreau]

Hi Pieter,

On Thu, Oct 04, 2018 at 12:02:11AM +0200, PiBa-NL wrote:
> Hi Frederic, Willy,
> 
> Attached a patch that will change /reg-tests/connection/b0.vtc to use
> unix@ sockets so it is compatible with FreeBSD and possibly other OS's.
(...)

applied, thank you!
Willy

[PATCH] REGTEST/MINOR: compatibility: use unix@ instead of abns@

[PiBa-NL]

Hi Frederic, Willy,

Attached a patch that will change /reg-tests/connection/b0.vtc to 
use unix@ sockets so it is compatible with FreeBSD and possibly other OS's.


As discussed in the other thread 
https://www.mail-archive.com/haproxy@formilux.org/msg31370.html.


Regards,
PiBa-NL (Pieter)

From 8c5ff12b4603e3525445d6f708f6239974003df4 Mon Sep 17 00:00:00 2001
From: PiBa-NL 
Date: Wed, 3 Oct 2018 23:54:49 +0200
Subject: [PATCH] REGTEST/MINOR: compatibility: use unix@ instead of abns@
 sockets

Changes the /reg-tests/connection/b0.vtc test to use unix@ instead of abns@ 
sockets.
This to allow the test to complete on other operating systems like FreeBSD that 
do not have 'namespaces'.
---
 reg-tests/connection/b0.vtc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/reg-tests/connection/b0.vtc b/reg-tests/connection/b0.vtc
index 3a873848..cbb8a7b0 100644
--- a/reg-tests/connection/b0.vtc
+++ b/reg-tests/connection/b0.vtc
@@ -36,14 +36,14 @@ haproxy h1 -conf {
 
 listen http
 bind-process 1
-bind abns@http accept-proxy name ssl-offload-http
+bind unix@${testdir}/http.socket accept-proxy name ssl-offload-http
 option forwardfor
 
 listen ssl-offload-http
 option httplog
 bind-process 2-4
 bind "fd@${ssl}" ssl crt ${testdir}/common.pem ssl no-sslv3 alpn 
h2,http/1.1
-server http abns@http send-proxy
+server http unix@${testdir}/http.socket send-proxy
 } -start
 
 
-- 
2.18.0.windows.1

Re: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters

[Willy Tarreau]

Hi Pieter,

On Mon, Oct 01, 2018 at 11:06:33PM +0200, PiBa-NL wrote:
> I have used the exact config above, and well it 'works fine' ?? Results
> below.. both types of nc requests send to 1 running haproxy result in a
> proper '21'..

OK.

> But even in this case the results would be 'predicable', while with my
> original test the outcome is 'randomized'..
> So despite the fact that i don't know whats really wrong, and the
> environment it runs on does seem to affect things. There is more going on
> that simply a counter incremented twice.. Also your results are 'higher'
> than expected, while mine are 'lower' than expected.. Perhaps this single
> testcase is already showing multiple issues beneath the carpet ;) ?.

It's very likely :-/ That's why it's important to cut this into pieces
to figure what it's showing.

Willy

Re: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters

[PiBa-NL]

Hi Frederic,

Op 1-10-2018 om 15:15 schreef Frederic Lecaille:

On 09/15/2018 02:03 AM, PiBa-NL wrote:

Hi List, Willy,


Hi Pieter,

Sorry for this late reply.

I am also sorry to tell you that -D option should not be used anymore 
because it as been recently broken. It adds an extra 2s delay on my PC.
There is a 'newer' version of the regtest in one of the other mails ( 
https://www.mail-archive.com/haproxy@formilux.org/msg31318.html ). it 
only runs 4x10 connections. and doesn't use -D anymore and well 
discussing with Willy some odd behavioral differences between his and my 
test results..


Without this option, on my PC the test fails but after having being 
run during about 300ms due to the regex which does not match. 
CummConns (resp. CumReq) never reaches 3001 (resp. 3002).


Note that as this script is highly verbose it may be required to 
increase the varnishtest internal buffer size (see -b varnishtest 
option).
Thanks didn't realize that one could have helped, probably not needed 
anymore with the changed regtest though.


I've created a regtest that checks that when concurrent connections 
are being handled that the connection counters are kept properly.


I think it could be committed as attached. It takes a few seconds to 
run. It currently fails on 1.9-dev2 (also fails on 1.8.13 with kqueue 
on FreeBSD, adding a 'nokqueue' on 1.8.13 makes it succeed though..).


I think it might be a good and reproducible test to run.?

Or does it need more tweaking? Thoughts appreciated :).


I have shorten the "haproxy -cli" section like that:

haproxy h1 -cli {
    send "show info"
    expect ~ "CumConns: 3001*\\nCumReq: 3002*"
} -wait
Thats a nice one, i did try a .* regex like wildcard check but found 
that didn't work, this will certainly help in case the test fails to 
produce less repeated/unneeded output and when it doesn't fail it will 
be a bit faster i guess, and allow for more checks on the same set of 
stats to take place with little effort.


Note that all the Slg_1 syslog specs are not completely run because 
you do not wait for its termination with "syslog Slg_1 -wait" command 
this is why it does not fails (but it should: the syslog server does 
not receive 15 messages).
I didn't intent to check for 15 messages as a valid outcome, but was 
looking more for 'some' syslog output when the testcase fails for some 
reason.. With the now fairly limited 40 connections it might be feasible 
to more properly check syslog output though. With 3000 connections it 
wasn't really nice when syslog was writing everything to the screen. Ill 
take another look at that one.


Regards,
PiBa-NL (Pieter)

Re: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters

[PiBa-NL]

Hi Willy,

Op 1-10-2018 om 4:00 schreef Willy Tarreau:

On Sun, Sep 30, 2018 at 10:16:55PM +0200, PiBa-NL wrote:

Hi Willy,
Op 30-9-2018 om 20:38 schreef Willy Tarreau:

On Sun, Sep 30, 2018 at 08:22:23PM +0200, Willy Tarreau wrote:

On Sun, Sep 30, 2018 at 07:59:34PM +0200, PiBa-NL wrote:

Indeed it works with 1.8, so in that regard i 'think' the test itself is
correct.. Also when disabling threads, or running only 1 client, it still
works.. Then both CumConns CumReq show 11 for the first stats result.

Hmmm for me it fails even without threads. That was the first thing I
tried when meeting the error in fact. But I need to dig deeper.

So I'm seeing that in fact the count is correct if the server connection
closes first, and wrong otherwise. In fact it fails similarly both for
1.6, 1.7, 1.8 and 1.9 with and without threads. I'm seeing that the
connection count is exactly 10 times the incoming connections while the
request count is exactly 20 times this count. I suspect that what happens
is that the request count is increased on each connection when preparing
to receive a new request. This even slightly reminds me something but
I don't know where I noticed something like this, I think I saw this
when reviewing the changes needed to be made to HTTP for the native
internal representation.

So I think it's a minor bug, but not a regression.

Thanks,
Willy

Not sure, only difference between 100x FAILED and 100x OK is the version
here. Command executed and result below.

Perhaps that's just because of the OS / Scheduler used though, i assume your
using some linux distro to test with, perhaps that explains part of the
differences between your and my results..

I find this strange. Your config contains a comment about the length
converter missing from 1.8 so I had to change it to use the deny part
on 1.8. It happens that using deny here precisely is what fixed the
problem for me the first time. I simplified it this way to run a
manual test :

   global
 stats socket /tmp/sock1 mode 666 level admin
 #nbthread 3
 log 127.0.0.1:5514 local0
 #nokqueue

   defaults
 mode http
 option dontlog-normal
 log global
 option httplog
 timeout connect 3s
 timeout client  4s
 timeout server  15s

   frontend fe1
 bind 127.0.0.1:8001
 acl donelooping hdr(TEST) -m len 10
 http-request set-header TEST "%[hdr(TEST)]x"
 use_backend b2 if donelooping
 default_backend b1

   backend b1
 server srv1 127.0.0.1:8001

   frontend fe2
 bind 127.0.0.1:8002
 default_backend b2

   backend b2
 server srv2 127.0.0.1:8000

Then I test it this way and got the same results on all versions :

   $ echo -e "GET / HTTP/1.1\r\n\r\n"|nc 0 8001
   $ echo show info | socat - /tmp/sock1 | grep Cum
   CumConns: 11
   CumReq: 21
   CumSslConns: 0

   $ echo -e "GET / HTTP/1.1\r\nconnection: close\r\n\r\n"|nc 0 8001
   $ echo show info | socat - /tmp/sock1 | grep Cum
   CumConns: 11
   CumReq: 11
   CumSslConns: 0

Regards,
Willy


I have used the exact config above, and well it 'works fine' ?? Results 
below.. both types of nc requests send to 1 running haproxy result in a 
proper '21'..


But even in this case the results would be 'predicable', while with my 
original test the outcome is 'randomized'..
So despite the fact that i don't know whats really wrong, and the 
environment it runs on does seem to affect things. There is more going 
on that simply a counter incremented twice.. Also your results are 
'higher' than expected, while mine are 'lower' than expected.. Perhaps 
this single testcase is already showing multiple issues beneath the 
carpet ;) ?.


Regards,
PiBa-NL (Pieter)

# haproxy -v
HA-Proxy version 1.8.14-52e4d43 2018/09/20
Copyright 2000-2018 Willy Tarreau 

# echo -e "GET / HTTP/1.1\r\n\r\n"|nc 0 8001
HTTP/1.0 503 Service Unavailable
Cache-Control: no-cache
Content-Type: text/html

503 Service Unavailable
No server is available to handle this request.

# echo -e "GET / HTTP/1.1\r\nconnection: close\r\n\r\n"|nc 0 8001
HTTP/1.0 503 Service Unavailable
Cache-Control: no-cache
Content-Type: text/html

503 Service Unavailable
No server is available to handle this request.

# echo show info | socat - /tmp/sock1 | grep Cum
CumConns: 21
CumReq: 21
CumSslConns: 0
#
#
# haproxy -v
HA-Proxy version 1.9-dev3-27010f0 2018/09/29
Copyright 2000-2018 Willy Tarreau 

# echo -e "GET / HTTP/1.1\r\n\r\n"|nc 0 8001
HTTP/1.0 503 Service Unavailable
Cache-Control: no-cache
Content-Type: text/html

503 Service Unavailable
No server is available to handle this request.

# echo -e "GET / HTTP/1.1\r\nconnection: close\r\n\r\n"|nc 0 8001
HTTP/1.0 503 Service Unavailable
Cache-Control: no-cache
Content-Type: text/html

503 Service Unavailable
No server is available to handle this request.

# echo show info | socat - /tmp/sock1 | grep Cum
CumConns: 21
CumReq: 21
CumSslConns: 0
#

Re: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters

[Frederic Lecaille]

On 09/15/2018 02:03 AM, PiBa-NL wrote:

Hi List, Willy,


Hi Pieter,

Sorry for this late reply.

I am also sorry to tell you that -D option should not be used anymore 
because it as been recently broken. It adds an extra 2s delay on my PC.


Without this option, on my PC the test fails but after having being run 
during about 300ms due to the regex which does not match. CummConns 
(resp. CumReq) never reaches 3001 (resp. 3002).


Note that as this script is highly verbose it may be required to 
increase the varnishtest internal buffer size (see -b varnishtest option).


I've created a regtest that checks that when concurrent connections are 
being handled that the connection counters are kept properly.


I think it could be committed as attached. It takes a few seconds to 
run. It currently fails on 1.9-dev2 (also fails on 1.8.13 with kqueue on 
FreeBSD, adding a 'nokqueue' on 1.8.13 makes it succeed though..).


I think it might be a good and reproducible test to run.?

Or does it need more tweaking? Thoughts appreciated :).


I have shorten the "haproxy -cli" section like that:

haproxy h1 -cli {
send "show info"
expect ~ "CumConns: 3001*\\nCumReq: 3002*"
} -wait

Note that all the Slg_1 syslog specs are not completely run because you 
do not wait for its termination with "syslog Slg_1 -wait" command this 
is why it does not fails (but it should: the syslog server does not 
receive 15 messages).



Regards,

PiBa-NL (Pieter)

Re: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters

[Willy Tarreau]

On Sun, Sep 30, 2018 at 10:16:55PM +0200, PiBa-NL wrote:
> Hi Willy,
> Op 30-9-2018 om 20:38 schreef Willy Tarreau:
> > On Sun, Sep 30, 2018 at 08:22:23PM +0200, Willy Tarreau wrote:
> > > On Sun, Sep 30, 2018 at 07:59:34PM +0200, PiBa-NL wrote:
> > > > Indeed it works with 1.8, so in that regard i 'think' the test itself is
> > > > correct.. Also when disabling threads, or running only 1 client, it 
> > > > still
> > > > works.. Then both CumConns CumReq show 11 for the first stats result.
> > > Hmmm for me it fails even without threads. That was the first thing I
> > > tried when meeting the error in fact. But I need to dig deeper.
> > So I'm seeing that in fact the count is correct if the server connection
> > closes first, and wrong otherwise. In fact it fails similarly both for
> > 1.6, 1.7, 1.8 and 1.9 with and without threads. I'm seeing that the
> > connection count is exactly 10 times the incoming connections while the
> > request count is exactly 20 times this count. I suspect that what happens
> > is that the request count is increased on each connection when preparing
> > to receive a new request. This even slightly reminds me something but
> > I don't know where I noticed something like this, I think I saw this
> > when reviewing the changes needed to be made to HTTP for the native
> > internal representation.
> > 
> > So I think it's a minor bug, but not a regression.
> > 
> > Thanks,
> > Willy
> 
> Not sure, only difference between 100x FAILED and 100x OK is the version
> here. Command executed and result below.
> 
> Perhaps that's just because of the OS / Scheduler used though, i assume your
> using some linux distro to test with, perhaps that explains part of the
> differences between your and my results..

I find this strange. Your config contains a comment about the length
converter missing from 1.8 so I had to change it to use the deny part
on 1.8. It happens that using deny here precisely is what fixed the
problem for me the first time. I simplified it this way to run a
manual test :

  global
stats socket /tmp/sock1 mode 666 level admin
#nbthread 3
log 127.0.0.1:5514 local0
#nokqueue

  defaults
mode http
option dontlog-normal
log global
option httplog
timeout connect 3s
timeout client  4s
timeout server  15s

  frontend fe1
bind 127.0.0.1:8001
acl donelooping hdr(TEST) -m len 10
http-request set-header TEST "%[hdr(TEST)]x"
use_backend b2 if donelooping
default_backend b1

  backend b1
server srv1 127.0.0.1:8001

  frontend fe2
bind 127.0.0.1:8002
default_backend b2

  backend b2
server srv2 127.0.0.1:8000

Then I test it this way and got the same results on all versions :

  $ echo -e "GET / HTTP/1.1\r\n\r\n"|nc 0 8001
  $ echo show info | socat - /tmp/sock1 | grep Cum
  CumConns: 11
  CumReq: 21
  CumSslConns: 0

  $ echo -e "GET / HTTP/1.1\r\nconnection: close\r\n\r\n"|nc 0 8001
  $ echo show info | socat - /tmp/sock1 | grep Cum
  CumConns: 11
  CumReq: 11
  CumSslConns: 0

Regards,
Willy

Re: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters

[PiBa-NL]

Hi Willy,
Op 30-9-2018 om 20:38 schreef Willy Tarreau:

On Sun, Sep 30, 2018 at 08:22:23PM +0200, Willy Tarreau wrote:

On Sun, Sep 30, 2018 at 07:59:34PM +0200, PiBa-NL wrote:

Indeed it works with 1.8, so in that regard i 'think' the test itself is
correct.. Also when disabling threads, or running only 1 client, it still
works.. Then both CumConns CumReq show 11 for the first stats result.

Hmmm for me it fails even without threads. That was the first thing I
tried when meeting the error in fact. But I need to dig deeper.

So I'm seeing that in fact the count is correct if the server connection
closes first, and wrong otherwise. In fact it fails similarly both for
1.6, 1.7, 1.8 and 1.9 with and without threads. I'm seeing that the
connection count is exactly 10 times the incoming connections while the
request count is exactly 20 times this count. I suspect that what happens
is that the request count is increased on each connection when preparing
to receive a new request. This even slightly reminds me something but
I don't know where I noticed something like this, I think I saw this
when reviewing the changes needed to be made to HTTP for the native
internal representation.

So I think it's a minor bug, but not a regression.

Thanks,
Willy


Not sure, only difference between 100x FAILED and 100x OK is the version 
here. Command executed and result below.


Perhaps that's just because of the OS / Scheduler used though, i assume 
your using some linux distro to test with, perhaps that explains part of 
the differences between your and my results.. In the end it doesn't 
matter much if its a bug or a regression still needs a fix ;). And well 
i don't know if its just the counter thats wrong, or there might be 
bigger consequences somewhere. if its just the counter then i guess it 
wouldn't hurt much to postpone a fix to a next (dev?) version.


Regards,

PiBa-NL (Pieter)

root@freebsd11:/usr/ports/net/haproxy-devel # varnishtest -q -n 100 -j 
16 -k ./haproxy_test_OK_20180831/loadtest/b0-loadtest.vtc

...
#    top  TEST ./haproxy_test_OK_20180831/loadtest/b0-loadtest.vtc 
FAILED (0.128) exit=2
#    top  TEST ./haproxy_test_OK_20180831/loadtest/b0-loadtest.vtc 
FAILED (0.135) exit=2

100 tests failed, 0 tests skipped, 0 tests passed
root@freebsd11:/usr/ports/net/haproxy-devel # haproxy -v
HA-Proxy version 1.9-dev3-27010f0 2018/09/29
Copyright 2000-2018 Willy Tarreau 

root@freebsd11:/usr/ports/net/haproxy-devel # pkg add -f 
haproxy-1.8.14-selfbuild-reg-tests-OK.txz

Installing haproxy-1.8...
package haproxy is already installed, forced install
Extracting haproxy-1.8: 100%
root@freebsd11:/usr/ports/net/haproxy-devel # varnishtest -q -n 100 -j 
16 -k ./haproxy_test_OK_20180831/loadtest/b0-loadtest.vtc

0 tests failed, 0 tests skipped, 100 tests passed
root@freebsd11:/usr/ports/net/haproxy-devel # haproxy -v
HA-Proxy version 1.8.14-52e4d43 2018/09/20
Copyright 2000-2018 Willy Tarreau 

Re: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters

[Willy Tarreau]

On Sun, Sep 30, 2018 at 08:22:23PM +0200, Willy Tarreau wrote:
> On Sun, Sep 30, 2018 at 07:59:34PM +0200, PiBa-NL wrote:
> > Indeed it works with 1.8, so in that regard i 'think' the test itself is
> > correct.. Also when disabling threads, or running only 1 client, it still
> > works.. Then both CumConns CumReq show 11 for the first stats result.
> 
> Hmmm for me it fails even without threads. That was the first thing I
> tried when meeting the error in fact. But I need to dig deeper.

So I'm seeing that in fact the count is correct if the server connection
closes first, and wrong otherwise. In fact it fails similarly both for
1.6, 1.7, 1.8 and 1.9 with and without threads. I'm seeing that the
connection count is exactly 10 times the incoming connections while the
request count is exactly 20 times this count. I suspect that what happens
is that the request count is increased on each connection when preparing
to receive a new request. This even slightly reminds me something but
I don't know where I noticed something like this, I think I saw this
when reviewing the changes needed to be made to HTTP for the native
internal representation.

So I think it's a minor bug, but not a regression.

Thanks,
Willy

Re: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters

[Willy Tarreau]

On Sun, Sep 30, 2018 at 07:59:34PM +0200, PiBa-NL wrote:
> Indeed it works with 1.8, so in that regard i 'think' the test itself is
> correct.. Also when disabling threads, or running only 1 client, it still
> works.. Then both CumConns CumReq show 11 for the first stats result.

Hmmm for me it fails even without threads. That was the first thing I
tried when meeting the error in fact. But I need to dig deeper.

> > However, I'd like to merge
> > the fix before merging the regtest otherwise it will kill the reg-test
> > feature until we manage to get the issue fixed!
> I'm not fully sure i agree on that.. While i understand that failing
> reg-tests can be a pita while developing (if you run them regulary) the fact
> is that currently existing tests can already already start to fail after
> some major redesign of the code, a few mails back (different mailthread) i
> tested like 10 commits in a row and they all suffered from different failing
> tests, that would imho not be a reason to remove those tests, and they didnt
> stop development.

The reason is that for now we have no way to let the tests fail gracefully
and report what is OK and what is not. So any error that's in the way will
lead to an absolutely certain behaviour from everyone : nobody will run the
tests anymore since the result will be known.

Don't get me wrong, I'm willing to get as many tests as we can, but 1) we
have to be sure these tests only fail for regressions and not for other
reasons, and 2) we must be sure that these tests do not prevent other ones
from being run nor make it impossible to observe the progress on other
ones. We're still at the beginning with reg tests, and as you can see we
have not even yet sorted out the requirements for some of them like threads
or Lua or whatever else.

I'm just asking that we don't create tests faster than we can sort them
out, that's all. This probably means that we really have to work on these
two main areas which are test prerequisites and synthetic reports of what
worked and what failed.

Ideas and proposals on this are welcome, but to be honest I can't spend
as much time as I'd want on this for now given how late we are on all
what remains to be done, so I really welcome discussions and help on the
subject between the various actors.

Thanks,
Willy

Re: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters

[Willy Tarreau]

On Sun, Sep 30, 2018 at 07:15:59PM +0200, PiBa-NL wrote:
> > on a simple config, the CummConns always matches the CumReq, and when
> > running this test I'm seeing random values there in the output, but I
> > also see that they are retrieved before all connections are closed
> But CurrConns is 0, so connections are (supposed to be?) closed? :
> 
>  h1    0.0 CLI recv|CurrConns: 0
>  h1    0.0 CLI recv|CumConns: 27
>  h1    0.0 CLI recv|CumReq: 27

You're totally right, I think I confused CumConns and CurrConns when
looking at the output. With that said I have no idea what's going on,
I'll have another look.

Thanks,
Willy

Re: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters

[PiBa-NL]

Hi Willy,

Op 30-9-2018 om 7:46 schreef Willy Tarreau:

Hi Pieter,

On Sun, Sep 30, 2018 at 12:05:14AM +0200, PiBa-NL wrote:

Hi Willy,

I thought lets give those reg-test another try :) as its easy to run and
dev3 just came out.
All tests pass on my FreeBSD system, except this one, new reg-test attached.

Pretty much the same test as previously send, but now with only 4 x 10
connections. Which should be fine for conntrack and sysctls (i hope..). It
seems those stats numbers are 'off', or is my expected value not as fixed as
i thought it would be?

Well, at least it works fine on 1.8 and not on 1.9-dev3 so I think you
spotted a regression that we have to analyse.
Indeed it works with 1.8, so in that regard i 'think' the test itself is 
correct.. Also when disabling threads, or running only 1 client, it 
still works.. Then both CumConns CumReq show 11 for the first stats result.

However, I'd like to merge
the fix before merging the regtest otherwise it will kill the reg-test
feature until we manage to get the issue fixed!
I'm not fully sure i agree on that.. While i understand that failing 
reg-tests can be a pita while developing (if you run them regulary) the 
fact is that currently existing tests can already already start to fail 
after some major redesign of the code, a few mails back (different 
mailthread) i tested like 10 commits in a row and they all suffered from 
different failing tests, that would imho not be a reason to remove those 
tests, and they didnt stop development.

I'm also seeing that you rely on threads, I think I noticed another test
involving threads. Probably that we should have a specific directory for
these ones that we can disable completely when threads are not enabled,
otherwise this will also destroy tests (and make them extremely slow due
to varnishtest waiting for the timeout if haproxy refuses to parse the
config).
A specific directory will imho not work. How should it be called? 
/threaded_lua_with_ssl_using_kqueue_scheduler_on_freebsd_without_absn_for_haproxy_1.9_and_higher/ 
?
Having varnishtest fail while waiting for a feature that was not 
compiled is indeed undesirable as well. So some 'smart' way of defining 
'requirements' for a test will be needed so they can gracefully skip if 
not applicable.. I'm not sure myself how that way should look though.. 
On one side i think the .vtc itself might be the place to define what 
requirements it has, on the other the other a separate list/script 
including logic of what tests to run could be nice.. But then who is 
going to maintain that one..

I think that we should think a bit forward based on these tests. We must
not let varnishtest stop on the first error but rather just log it.

varnishtest can continue on error with -k
Using this little mytest.sh script at the moment, this runs all tests 
and only failed tests produce a lot of logging..:

  haproxy -v
  varnishtest -j 16 -k -t 20 ./work/haproxy-*/reg-tests/*/*.vtc > 
./mytest-result.log 2>&1
  varnishtest -j 16 -k -t 20 ./haproxy_test_OK_20180831/*/*.vtc >> 
./mytest-result.log 2>&1

  cat ./mytest-result.log
  echo "" >> ./mytest-result.log
  haproxy -vv  >> ./mytest-result.log

There is also the -q parameter, but then it doesn't log anymore what 
tests passed and would only the failed tests will produce 1 log line.. 
(i do like to log what tests where executed though..)

  Then
at the end we could produce a report of successes and failures that would
be easy to diff from the previous (or expected) one. That will be
particularly useful when running the tests on older releases. As an
example, I had to run your test manually on 1.8 because for I-don't-know-
what-reason, the one about the proxy protocol now fails while it used to
work fine last week for the 1.8.14 release. That's a shame that we can't
complete tests just because one randomly fails.
You can continue tests. ( -k ) But better write it out to a logfile 
then, or perhaps combine with -l which leaves the /tmp/.vtc folder..

Thanks,
Willy


Regards,
PiBa-NL (Pieter)

Re: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters

[PiBa-NL]

Hi Willy,

Op 30-9-2018 om 7:56 schreef Willy Tarreau:

On Sun, Sep 30, 2018 at 07:46:24AM +0200, Willy Tarreau wrote:

Well, at least it works fine on 1.8 and not on 1.9-dev3 so I think you
spotted a regression that we have to analyse. However, I'd like to merge
the fix before merging the regtest otherwise it will kill the reg-test
feature until we manage to get the issue fixed!

By the way, could you please explain in simple words the issue you've
noticed ? I tried to reverse the vtc file but I don't understand the
details nor what it tries to achieve. When I'm running a simple test
on a simple config, the CummConns always matches the CumReq, and when
running this test I'm seeing random values there in the output, but I
also see that they are retrieved before all connections are closed

But CurrConns is 0, so connections are (supposed to be?) closed? :

 h1    0.0 CLI recv|CurrConns: 0
 h1    0.0 CLI recv|CumConns: 27
 h1    0.0 CLI recv|CumReq: 27


, so
I'm not even sure the test is correct :-/

Thanks,
Willy


What i'm trying to achieve is, well.. testing for regressions that are 
not yet known to exist on the current stable version.


So what this test does in short:
It makes 4 clients simultaneously send a request to a threaded haproxy, 
which in turn connects 10x backend to frontend and then sends the 
request to the s1 server. This with the intended purpose of having 
several connections started and broken up as fast as haproxy can process 
them while trying to have a high probability of adding/removing items 
from lists/counters from different threads thus possibly creating 
problems if some lock/sync isn't done correctly. After firing a few 
requests it also verifies the expected counts, and results where possible..


History:
Ive been bit a few times with older releases by corruption occurring 
inside the POST data when uploading large (500MB+) files to a server 
behind haproxy. After a few megabytes are passed correctly the resulting 
file would contain differences from their original when compared, the 
upload 'seemed' to succeed though. (this was then solved by installing a 
newer haproxy build..).. Also sometimes threads have locked up or 
crashed things. Or kqueue scheduler turned out to behave differently 
than others.. Ive been trying to test such things manually but found i 
always forget to run some test. This is why i really like the concept of 
having a set of defined tests that validate haproxy is working 
'properly', on the OS i run it on.. Also when some issue i ran into gets 
fixed i tend to run -dev builds on my production environment for a 
while, and well its nice to know that other functionality still works as 
it used to..


With writing this test i initially started with the idea of 
automatically testing a large file transfer through haproxy, but then 
thought where / how to leave such a file, so i thought of transferring a 
'large' header with increasing size 'might' trigger a similar 
condition.. Though in hindsight that might not actually test the same 
code paths..


I created that test with 1 byte growth in the header together with 4000 
connections didn't quite achieve that initial big file simulation, but 
still i thought it ended up to be a nice test. So submitted it a while 
back ;) .. Anyhow haproxy wasn't capable of doing much when dev2 was 
tagged so i wasnt to worried the test failed at that time.. And you 
announced dev2 as such as well, so that was okay. And perhaps the issue 
found then would solve itself when further fixes on top of dev2 were 
added ;).


Anyhow with dev3 i hoped all regressions would be fixed, and found this 
one still failed on 1.9dev3. So it tuned the numbers in the previous 
submitted regtest down a little to avoid conntrack/sysctl default 
limits, while still failing the test 'reliably'.. I'm not sure what 
exactly is going on, or how bad it is that these numbers don't match up 
anymore.. Maybe its only the counter thats not updated in a thread safe 
way, perhaps there is a bigger issue lurking with sync points and 
whatnot..? Either way the test should pass as i understand it, the 4 
defined varnish clients got their answer back and Currconns = 0, also 
adding a 3 second delay between waiting for the clients and checking the 
stats does not fix it... And as youve checked with 1.8 it does pass. 
Though that to could perhaps be a coincidence, maybe now things are 
processed even faster now but in different order so the test fails for 
the wrong reason.?.


Hope that makes some sense in my thought process :).

Regards,

PiBa-NL (Pieter)

Re: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters

[Willy Tarreau]

On Sun, Sep 30, 2018 at 07:46:24AM +0200, Willy Tarreau wrote:
> Well, at least it works fine on 1.8 and not on 1.9-dev3 so I think you
> spotted a regression that we have to analyse. However, I'd like to merge
> the fix before merging the regtest otherwise it will kill the reg-test
> feature until we manage to get the issue fixed!

By the way, could you please explain in simple words the issue you've
noticed ? I tried to reverse the vtc file but I don't understand the
details nor what it tries to achieve. When I'm running a simple test
on a simple config, the CummConns always matches the CumReq, and when
running this test I'm seeing random values there in the output, but I
also see that they are retrieved before all connections are closed, so
I'm not even sure the test is correct :-/

Thanks,
Willy

Re: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters

[Willy Tarreau]

Hi Pieter,

On Sun, Sep 30, 2018 at 12:05:14AM +0200, PiBa-NL wrote:
> Hi Willy,
> 
> I thought lets give those reg-test another try :) as its easy to run and
> dev3 just came out.
> All tests pass on my FreeBSD system, except this one, new reg-test attached.
> 
> Pretty much the same test as previously send, but now with only 4 x 10
> connections. Which should be fine for conntrack and sysctls (i hope..). It
> seems those stats numbers are 'off', or is my expected value not as fixed as
> i thought it would be?

Well, at least it works fine on 1.8 and not on 1.9-dev3 so I think you
spotted a regression that we have to analyse. However, I'd like to merge
the fix before merging the regtest otherwise it will kill the reg-test
feature until we manage to get the issue fixed!

I'm also seeing that you rely on threads, I think I noticed another test
involving threads. Probably that we should have a specific directory for
these ones that we can disable completely when threads are not enabled,
otherwise this will also destroy tests (and make them extremely slow due
to varnishtest waiting for the timeout if haproxy refuses to parse the
config).

I think that we should think a bit forward based on these tests. We must
not let varnishtest stop on the first error but rather just log it. Then
at the end we could produce a report of successes and failures that would
be easy to diff from the previous (or expected) one. That will be
particularly useful when running the tests on older releases. As an
example, I had to run your test manually on 1.8 because for I-don't-know-
what-reason, the one about the proxy protocol now fails while it used to
work fine last week for the 1.8.14 release. That's a shame that we can't
complete tests just because one randomly fails.

Thanks,
Willy

Re: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters

[PiBa-NL]

Hi Willy,

I thought lets give those reg-test another try :) as its easy to run and 
dev3 just came out.

All tests pass on my FreeBSD system, except this one, new reg-test attached.

Pretty much the same test as previously send, but now with only 4 x 10 
connections. Which should be fine for conntrack and sysctls (i hope..). 
It seems those stats numbers are 'off', or is my expected value not as 
fixed as i thought it would be?


Tested with:
HA-Proxy version 1.9-dev3-27010f0 2018/09/29
FreeBSD freebsd11 11.1-RELEASE

Results:
 h1    0.0 CLI recv|CumConns: 33
 h1    0.0 CLI recv|CumReq: 65
 h1    0.0 CLI expect failed ~ "CumConns: 41"

If my 'expect' is correct,  would the patch be suitable for inclusion 
with the other reg-tests this way?
If you want to rename loadtest, to heavytest, or any other tweaks please 
feel free to do so.


Regards,
PiBa-NL (Pieter)

Op 20-9-2018 om 22:25 schreef PiBa-NL:

Hi Willy,

Op 20-9-2018 om 13:56 schreef Willy Tarreau:

For me the test produces like 345 lines of output as attached. which seems
not to bad (if the test succeeds).

It's already far too much for a user.


Well those 345 lines are if it succeeds while in 'verbose' mode, in 
'normal' mode it only produces 1 line of output when successful. 
Pretty much all tests produce 100+ lines of 'logging' if they fail for 
some reason. From what ive seen varnishtest either produces a bulk of 
logging on a failure, or it only logs the failures. There isn't much 
in between.


As for all the rest of the email, thanks for your elaborate response :).

Regards,

PiBa-NL (Pieter)



From 28377ffe246ed1db0e0d898fa6263eccdc68c490 Mon Sep 17 00:00:00 2001
From: PiBa-NL 
Date: Sat, 15 Sep 2018 01:51:54 +0200
Subject: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters
 after running a 4 x 10 looping requests

---
 reg-tests/loadtest/b0-loadtest.vtc | 99 ++
 1 file changed, 99 insertions(+)
 create mode 100644 reg-tests/loadtest/b0-loadtest.vtc

diff --git a/reg-tests/loadtest/b0-loadtest.vtc 
b/reg-tests/loadtest/b0-loadtest.vtc
new file mode 100644
index ..590924e1
--- /dev/null
+++ b/reg-tests/loadtest/b0-loadtest.vtc
@@ -0,0 +1,99 @@
+# Checks that request and connection counters are properly kept
+
+varnishtest "Connection counters check"
+feature ignore_unknown_macro
+
+server s1 {
+rxreq
+expect req.http.TESTsize == 10
+txresp
+} -repeat 4 -start
+
+syslog Slg_1 -level notice {
+recv
+} -repeat 15 -start
+
+haproxy h1 -W -conf {
+  global
+nbthread 3
+log ${Slg_1_addr}:${Slg_1_port} local0
+#nokqueue
+
+  defaults
+mode http
+option dontlog-normal
+log global
+option httplog
+timeout connect 3s
+timeout client  4s
+timeout server  15s
+
+  frontend fe1
+bind "fd@${fe_1}"
+acl donelooping hdr(TEST) -m len 10
+http-request set-header TEST "%[hdr(TEST)]x"
+use_backend b2 if donelooping
+default_backend b1
+
+  backend b1
+server srv1 ${h1_fe_1_addr}:${h1_fe_1_port}
+
+  frontend fe2
+bind "fd@${fe_2}"
+default_backend b2
+
+  backend b2
+# haproxy 1.8 does not have the ,length converter.
+#acl OK hdr(TEST) -m len 10
+#http-request deny deny_status 200 if OK
+#http-request deny deny_status 400
+
+# haproxy 1.9 does have a ,length converter.
+http-request set-header TESTsize "%[hdr(TEST),length]"
+http-request del-header TEST
+server srv2 ${s1_addr}:${s1_port}
+
+} -start
+
+barrier b1 cond 4
+
+client c1 -connect ${h1_fe_1_sock} {
+  timeout 17
+   barrier b1 sync
+txreq -url "/"
+rxresp
+expect resp.status == 200
+} -start
+client c2 -connect ${h1_fe_1_sock} {
+  timeout 17
+   barrier b1 sync
+txreq -url "/"
+rxresp
+expect resp.status == 200
+} -start
+client c3 -connect ${h1_fe_1_sock} {
+  timeout 17
+   barrier b1 sync
+txreq -url "/"
+rxresp
+expect resp.status == 200
+} -start
+client c4 -connect ${h1_fe_1_sock} {
+  timeout 17
+   barrier b1 sync
+txreq -url "/"
+rxresp
+expect resp.status == 200
+} -start
+
+client c1 -wait
+client c2 -wait
+client c3 -wait
+client c4 -wait
+
+haproxy h1 -cli {
+send "show info"
+expect ~ "CumConns: 41"
+send "show info"
+expect ~ "CumReq: 42"
+}
-- 
2.18.0.windows.1

Re: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters

[PiBa-NL]

Hi Willy,

Op 20-9-2018 om 13:56 schreef Willy Tarreau:

For me the test produces like 345 lines of output as attached. which seems
not to bad (if the test succeeds).

It's already far too much for a user.


Well those 345 lines are if it succeeds while in 'verbose' mode, in 
'normal' mode it only produces 1 line of output when successful. Pretty 
much all tests produce 100+ lines of 'logging' if they fail for some 
reason. From what ive seen varnishtest either produces a bulk of logging 
on a failure, or it only logs the failures. There isn't much in between.


As for all the rest of the email, thanks for your elaborate response :).

Regards,

PiBa-NL (Pieter)

Re: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters

[Willy Tarreau]

Hi Pieter,

On Thu, Sep 20, 2018 at 12:48:35AM +0200, PiBa-NL wrote:
> Test takes like 5 seconds to run here, and while that is a bit long if you
> get a hundred more similar tests and want to continue tweaking developments
> while running tests in between. It wouldn't hurt to run such a (series) of
> longer tests before creating a patch and submitting it for inclusion on the
> official git repository in my opinion, or before a release.

Definitely, however those run before a release should be almost 100%
system-agnostic. Having to prepare the system and tune it properly for
the test not to fail is going to cause a headache every time there is
a failure because it will mean fixing the root cause and re-run the
whole suite, which precisely is what will make all this suite not to
be used at all anymore. This is why I'm really picky on the reliability
and the speed of these tests. It should be stupid-proof, with me being
the stupid (and believe me when it comes to doing repetitive stuff, I'm
among the stupidest persons you have ever met).

> My attempt was
> to test a bit differently than just looking for regressions of known fixed
> bugs, and putting a little load on haproxy so that threads and simultaneous
> actions 'might' get into conflicts/locks/stuff which might by chance, show
> up, which is why i choose to go a little higher on the number of round-trips
> with ever slightly increasing payload..

I really understand the point and I think it is valid to a certain extent.
But that's really not a thing to run by default. And I want to encourage us
(including me) to run reg tests from time to time. If you know that some of
them will take too long, you'll quickly end up avoiding all the ones you can
easily avoid using a single command (eg: playing with the LEVEL variable, or
not running it at all).

> For me the test produces like 345 lines of output as attached. which seems
> not to bad (if the test succeeds).

It's already far too much for a user. I should only know if it works
or not, otherwise it hides the output of all other ones (which is what
happened). We must not have to parse the output to know if we didn't
break anything, we just have to check that it looks normal. Here's what
make reg-tests gives me on 1.8 :

  willy@wtap:haproxy$ time sh make-reg-tests-1.8 
  #top  TEST reg-tests/lua/b2.vtc passed (0.159)
  #top  TEST reg-tests/lua/b1.vtc passed (0.122)
  #top  TEST reg-tests/lua/b0.vtc passed (0.110)
  #top  TEST reg-tests/lua/b3.vtc passed (0.137)
  #top  TEST reg-tests/connection/b0.vtc passed (0.172)
  #top  TEST reg-tests/server/b0.vtc passed (0.110)
  #top  TEST reg-tests/spoe/b0.vtc passed (0.008)
  #top  TEST reg-tests/ssl/b0.vtc passed (0.139)
  #top  TEST reg-tests/stick-table/b1.vtc passed (0.110)
  #top  TEST reg-tests/stick-table/b0.vtc passed (0.110)
  #top  TEST reg-tests/log/b0.vtc passed (0.125)
  #top  TEST reg-tests/seamless-reload/b0.vtc passed (0.123)
  
  real0m1.713s
  user0m0.316s
  sys 0m0.068s

As you can see there's no output to parse, it *visually* looks correct.

> Besides the 2 instances of cli output
> for stats, its seems not that much different from other tests..
> And with 1.8.13 on FreeBSD (without qkueue) it succeeds:  #    top TEST
> ./test/b0-loadtest.vtc passed (4.800

OK then you get a valid output there. It's here that it's ugly. But we
spend enough time analysing bugs, I really refuse to spend extra time
fixing bugs in tests supposed to help detect bugs, otherwise it becomes
recursive...

> Taking into account conntrack and ulimit, would that mean we can never
> 'reg-test' if haproxy can really handle like 1 connections without
> issue?

10k conns definitely is way beyond what you can expect from a non-root
user on a regular shell. I run most of my configs with "maxconn 400"
because that's less than 1024 FDs once you add the extra FDs for
listeners and checks. Anything beyond that will depend on the users'
setup and becomes tricky. And in this case it's more a matter of
stress-testing the system, and we can have stress-test procedures or
tools (just like we all do on our respective setups with different
tools). It's just that one has to know in advance that some preparation
is needed (typically killing a possible browser, unloading some modules,
checking that there's still memory left, maybe adding some addresses to
the loopback, etc). So it's a different approach and it definitely is
out of the scope of automatizing detection of potential regressions
during development.

> Or should the environment be configured by the test?? ,that seems
> very tricky at least and probably would be undesirable..

No definitely it would be even worse. For sure those used to run
"curl | sudo bash" will have no problem letting it configure their
system, but I'm not among such people and I appreciate a lot that
my machine works every morning when I want to wo

Re: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters

[PiBa-NL]

Hi Willy,

Op 19-9-2018 om 7:36 schreef Willy Tarreau:

Hi Pieter,

I took some time this morning to give it a test. For now it fails here,
after dumping 2200 lines of not really usable output that I didn't
investigate. From what I'm seeing it seems to moderately stress the
local machine so it has many reasons for failing (lack of source
ports, improperly tuned conntrack, ulimit, etc), and it takes far too
long a time to be usable as a default test, or this one alone will be
enough to discourage anyone from regularly running "make reg-tests".
Test takes like 5 seconds to run here, and while that is a bit long if 
you get a hundred more similar tests and want to continue tweaking 
developments while running tests in between. It wouldn't hurt to run 
such a (series) of longer tests before creating a patch and submitting 
it for inclusion on the official git repository in my opinion, or before 
a release.?. My attempt was to test a bit differently than just looking 
for regressions of known fixed bugs, and putting a little load on 
haproxy so that threads and simultaneous actions 'might' get into 
conflicts/locks/stuff which might by chance, show up, which is why i 
choose to go a little higher on the number of round-trips with ever 
slightly increasing payload..


For me the test produces like 345 lines of output as attached. which 
seems not to bad (if the test succeeds).. Besides the 2 instances of cli 
output for stats, its seems not that much different from other tests..
And with 1.8.13 on FreeBSD (without qkueue) it succeeds:  #    top TEST 
./test/b0-loadtest.vtc passed (4.800


Taking into account conntrack and ulimit, would that mean we can never 
'reg-test' if haproxy can really handle like 1 connections without 
issue? Or should the environment be configured by the test?? ,that seems 
very tricky at least and probably would be undesirable.. (i just today 
figured i could run reg-tests also on my production box to compare if a 
new build showed issues there that my test-box might not.. i wouldn't 
want system settings to changed by a reg-test run..)



I think we should create a distinct category for such tests
Agreed, which is why i used the currently non-existing '/loadtest/' 
folder. If '/heavy/' is better thats of course alright for me to.

, because
I see some value in it when it's made to reproduce a very specific
class of issues which is very unlikely to trigger unless someone is
working on it. In this case it is not a problem that it dumps a lot
of output, as it will be useful for the person knowing what to look
for there. Probably that such tests should be run by hand and dump
their log into a related file. Imagine for example that we would
have this :

  $ make reg-tests/heavy/conn-counter-3000-req.log
I'm not exactly sure..("make: don't know how to make reg-tests. Stop"). 
i would still like to have a way to run all 'applicable' tests with 1 
command, even if it takes a hour or so to verify haproxy is working 
'perfectly'. But like abns@ tests cant work on FreeBSD, but they should 
not 'fail', perhaps get skipped automatically though.?. Anyhow thats a 
question for my other mail-topic ( 
https://www.mail-archive.com/haproxy@formilux.org/msg31195.html )

It would run the test on reg-tests/heavy/conn-counter-3000-req.vtc and
would produce the log into reg-tests/heavy/conn-counter-3000-req.log.
We could use a similar thing to test for select/poll/epoll/kqueue, to
test for timeouts, race conditions (eg show sess in threads). This is
very likely something to brainstorm about. You might have other ideas
related to certain issues you faced in the past. Fred is unavailable
this week but I'd be very interested in his opinion on such things.

Thus for now I'm not applying your patch, but I'm interested in seeing
what can be done with it.
Okay no problem :) , ill keep running this particular test myself for 
the moment, it 'should' be able to pass normally..  (On my environment 
anyhow..)

Thanks,
Willy


Thanks for your comments, and thoughts.

I'm interested in Fred's and anyone elses opinion ;) , and well maybe 
this particular test-case could be replaced by something simpler/faster/ 
with more or less the same likelihood of catching yet unknown issues..? 
Looking forward to reactions :) .


Thanks and regards,

PiBa-NL (Pieter)

 top   0.0 extmacro def pwd=/usr/ports/net/haproxy-devel
 top   0.0 extmacro def localhost=127.0.0.1
 top   0.0 extmacro def bad_backend=127.0.0.1 58530
 top   0.0 extmacro def bad_ip=192.0.2.255
 top   0.0 macro def testdir=/usr/ports/net/haproxy-devel/./test
 top   0.0 macro def tmpdir=/tmp/vtc.35996.290f74a9
*top   0.0 TEST ./test/b0-loadtest.vtc starting
**   top   0.0 === varnishtest "Seamless reload issue with abns sockets"
*top   0.0 TEST Seamless reload issue with abns sockets
**   top   0.0 === feature ignore_unknown_macro
**   top   0.0 === server s1 {
**   s10.0 Starting server
 s10.0 macro def s1_addr=1

Re: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters

[Willy Tarreau]

Hi Pieter,

On Sat, Sep 15, 2018 at 02:03:45AM +0200, PiBa-NL wrote:
> Hi List, Willy,
> 
> I've created a regtest that checks that when concurrent connections are
> being handled that the connection counters are kept properly.
> 
> I think it could be committed as attached. It takes a few seconds to run. It
> currently fails on 1.9-dev2 (also fails on 1.8.13 with kqueue on FreeBSD,
> adding a 'nokqueue' on 1.8.13 makes it succeed though..).
> 
> I think it might be a good and reproducible test to run.?
> 
> Or does it need more tweaking? Thoughts appreciated :).

I took some time this morning to give it a test. For now it fails here,
after dumping 2200 lines of not really usable output that I didn't
investigate. From what I'm seeing it seems to moderately stress the
local machine so it has many reasons for failing (lack of source
ports, improperly tuned conntrack, ulimit, etc), and it takes far too
long a time to be usable as a default test, or this one alone will be
enough to discourage anyone from regularly running "make reg-tests".

I think we should create a distinct category for such tests, because
I see some value in it when it's made to reproduce a very specific
class of issues which is very unlikely to trigger unless someone is
working on it. In this case it is not a problem that it dumps a lot
of output, as it will be useful for the person knowing what to look
for there. Probably that such tests should be run by hand and dump
their log into a related file. Imagine for example that we would
have this :

 $ make reg-tests/heavy/conn-counter-3000-req.log

It would run the test on reg-tests/heavy/conn-counter-3000-req.vtc and
would produce the log into reg-tests/heavy/conn-counter-3000-req.log.
We could use a similar thing to test for select/poll/epoll/kqueue, to
test for timeouts, race conditions (eg show sess in threads). This is
very likely something to brainstorm about. You might have other ideas
related to certain issues you faced in the past. Fred is unavailable
this week but I'd be very interested in his opinion on such things.

Thus for now I'm not applying your patch, but I'm interested in seeing
what can be done with it.

Thanks,
Willy

[PATCH] REGTEST/MINOR: loadtest: add a test for connection counters

[PiBa-NL]

Hi List, Willy,

I've created a regtest that checks that when concurrent connections are 
being handled that the connection counters are kept properly.


I think it could be committed as attached. It takes a few seconds to 
run. It currently fails on 1.9-dev2 (also fails on 1.8.13 with kqueue on 
FreeBSD, adding a 'nokqueue' on 1.8.13 makes it succeed though..).


I think it might be a good and reproducible test to run.?

Or does it need more tweaking? Thoughts appreciated :).

Regards,

PiBa-NL (Pieter)

From 4b1af997e796e1bb2098c5f66ac24690841c72e8 Mon Sep 17 00:00:00 2001
From: PiBa-NL 
Date: Sat, 15 Sep 2018 01:51:54 +0200
Subject: [PATCH] REGTEST/MINOR: loadtest: add a test for connection counters
 after running 3000 requests in a loop

---
 reg-tests/loadtest/b0-loadtest.vtc | 94 ++
 1 file changed, 94 insertions(+)
 create mode 100644 reg-tests/loadtest/b0-loadtest.vtc

diff --git a/reg-tests/loadtest/b0-loadtest.vtc 
b/reg-tests/loadtest/b0-loadtest.vtc
new file mode 100644
index ..f66df5ee
--- /dev/null
+++ b/reg-tests/loadtest/b0-loadtest.vtc
@@ -0,0 +1,94 @@
+# Checks that request and connection counters are properly kept
+
+varnishtest "Seamless reload issue with abns sockets"
+feature ignore_unknown_macro
+
+server s1 {
+rxreq
+expect req.http.TESTsize == 1000
+txresp
+} -repeat 3 -start
+
+syslog Slg_1 -level notice {
+recv
+} -repeat 15 -start
+
+haproxy h1 -W -D -conf {
+  global
+nbthread 3
+log ${Slg_1_addr}:${Slg_1_port} local0
+maxconn 50
+#nokqueue
+
+  defaults
+mode http
+option dontlog-normal
+log global
+option httplog
+timeout connect 3s
+timeout client  4s
+timeout server  15s
+
+  frontend fe1
+maxconn 20001
+bind "fd@${fe_1}"
+acl donelooping hdr(TEST) -m len 1000
+http-request set-header TEST "%[hdr(TEST)]x"
+use_backend b2 if donelooping
+default_backend b1
+
+  backend b1
+fullconn 2
+server srv1 ${h1_fe_1_addr}:${h1_fe_1_port} maxconn 2
+
+  frontend fe2
+bind "fd@${fe_2}"
+default_backend b2
+
+  backend b2
+# haproxy 1.8 does not have the ,length converter.
+acl OK hdr(TEST) -m len 1000
+http-request deny deny_status 200 if OK
+http-request deny deny_status 400
+
+# haproxy 1.9 does have a ,length converter.
+#http-request set-header TESTsize "%[hdr(TEST),length]"
+#http-request del-header TEST
+#server srv2 ${s1_addr}:${s1_port}
+
+} -start
+
+barrier b1 cond 3
+
+client c1 -connect ${h1_fe_1_sock} {
+  timeout 17
+   barrier b1 sync
+txreq -url "/"
+rxresp
+expect resp.status == 200
+} -start
+client c2 -connect ${h1_fe_1_sock} {
+  timeout 17
+   barrier b1 sync
+txreq -url "/"
+rxresp
+expect resp.status == 200
+} -start
+client c3 -connect ${h1_fe_1_sock} {
+  timeout 17
+   barrier b1 sync
+txreq -url "/"
+rxresp
+expect resp.status == 200
+} -start
+
+client c1 -wait
+client c2 -wait
+client c3 -wait
+
+haproxy h1 -cli {
+send "show info"
+expect ~ "CumConns: 3001"
+send "show info"
+expect ~ "CumReq: 3002"
+}
-- 
2.18.0.windows.1

Re: [PATCH] REGTEST/MINOR: lua: Add reg testing files for 70d318c.

[Willy Tarreau]

On Tue, Sep 04, 2018 at 04:08:56PM +0200, Frederic Lecaille wrote:
> On 09/04/2018 04:01 PM, Frederic Lecaille wrote:
> > Another reg testing file for a LUA bug fixed by 70d318c commit.
> > 
> > Fred.
> > 
> 
> The same reg testing file but with indentation fixes.

Applied, thank you Fred.

Willy

Re: [PATCH] REGTEST/MINOR: lua: Add reg testing files for 70d318c.

[Frederic Lecaille]

On 09/04/2018 04:01 PM, Frederic Lecaille wrote:

Another reg testing file for a LUA bug fixed by 70d318c commit.

Fred.



The same reg testing file but with indentation fixes.


>From 7f01f387563564f1ee5ca718b4ad7562baa599b4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?= 
Date: Tue, 4 Sep 2018 15:58:14 +0200
Subject: [PATCH] REGTEST/MINOR: lua: Add reg testing files for 70d318c.

---
 reg-tests/lua/b3.lua |  1 +
 reg-tests/lua/b3.vtc | 52 
 2 files changed, 53 insertions(+)
 create mode 100644 reg-tests/lua/b3.lua
 create mode 100644 reg-tests/lua/b3.vtc

diff --git a/reg-tests/lua/b3.lua b/reg-tests/lua/b3.lua
new file mode 100644
index ..cc897e73
--- /dev/null
+++ b/reg-tests/lua/b3.lua
@@ -0,0 +1 @@
+core.register_service("donothing", "http", function(applet) end)
diff --git a/reg-tests/lua/b3.vtc b/reg-tests/lua/b3.vtc
new file mode 100644
index ..c43381a8
--- /dev/null
+++ b/reg-tests/lua/b3.vtc
@@ -0,0 +1,52 @@
+# commit 70d318c
+# BUG/MEDIUM: lua: possible CLOSE-WAIT state with '\n' headers
+#
+# The Lua parser doesn't takes in account end-of-headers containing
+# only '\n'. It expects always '\r\n'. If a '\n' is processes the Lua
+# parser considers it miss 1 byte, and wait indefinitely for new data.
+#
+# When the client reaches their timeout, it closes the connection.
+# This close is not detected and the connection keep in CLOSE-WAIT
+# state.
+#
+# I guess that this patch fix only a visible part of the problem.
+# If the Lua HTTP parser wait for data, the timeout server or the
+# connectio closed by the client may stop the applet.
+
+varnishtest "possible CLOSE-WAIT with '\n' headers"
+
+feature ignore_unknown_macro
+
+syslog Slog -level info -repeat 100 {
+recv info
+expect ~ "haproxy\\[${h1_pid}\\]: Ta=[0-9]* Tc=[0-9]* Td=-1 Th=[0-9]* Ti=[0-9]* Tq=[0-9]* TR=[0-9] Tr=-1 Tt=[0-9]* Tw=[0-9]*"
+} -start
+
+haproxy h1 -conf {
+defaults
+timeout client  1s
+timeout connect 1s
+
+global
+lua-load ${testdir}/b3.lua
+nbthread 4
+
+frontend frt
+log ${Slog_addr}:${Slog_port} local0 debug err
+log-format Ta=%Ta\ Tc=%Tc\ Td=%Td\ Th=%Th\ Ti=%Ti\ Tq=%Tq\ TR=%TR\ Tr=%Tr\ Tt=%Tt\ Tw=%Tw
+mode http
+bind "fd@${frt}"
+http-request use-service lua.donothing
+} -start
+
+
+client c1 -connect ${h1_frt_sock} -repeat 100  {
+send "GET / HTTP/1.1\n\n"
+} -run
+
+syslog Slog -wait
+
+shell {
+ss -pt | grep CLOSE-WAIT.*haproxy
+exit $((!$?))
+}
-- 
2.11.0

[PATCH] REGTEST/MINOR: lua: Add reg testing files for 70d318c.

[Frederic Lecaille]

Another reg testing file for a LUA bug fixed by 70d318c commit.

Fred.

>From ff46b36aa4d3ee0213ec0572f9ab3a7261233579 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?= 
Date: Tue, 4 Sep 2018 15:58:14 +0200
Subject: [PATCH] REGTEST/MINOR: lua: Add reg testing files for 70d318c.

---
 reg-tests/lua/b3.lua |  1 +
 reg-tests/lua/b3.vtc | 52 
 2 files changed, 53 insertions(+)
 create mode 100644 reg-tests/lua/b3.lua
 create mode 100644 reg-tests/lua/b3.vtc

diff --git a/reg-tests/lua/b3.lua b/reg-tests/lua/b3.lua
new file mode 100644
index ..cc897e73
--- /dev/null
+++ b/reg-tests/lua/b3.lua
@@ -0,0 +1 @@
+core.register_service("donothing", "http", function(applet) end)
diff --git a/reg-tests/lua/b3.vtc b/reg-tests/lua/b3.vtc
new file mode 100644
index ..d41efcf4
--- /dev/null
+++ b/reg-tests/lua/b3.vtc
@@ -0,0 +1,52 @@
+# commit 70d318c
+# BUG/MEDIUM: lua: possible CLOSE-WAIT state with '\n' headers
+#
+# The Lua parser doesn't takes in account end-of-headers containing
+# only '\n'. It expects always '\r\n'. If a '\n' is processes the Lua
+# parser considers it miss 1 byte, and wait indefinitely for new data.
+#
+# When the client reaches their timeout, it closes the connection.
+# This close is not detected and the connection keep in CLOSE-WAIT
+# state.
+#
+# I guess that this patch fix only a visible part of the problem.
+# If the Lua HTTP parser wait for data, the timeout server or the
+# connectio closed by the client may stop the applet.
+
+varnishtest "possible CLOSE-WAIT with '\n' headers"
+
+feature ignore_unknown_macro
+
+syslog Slog -level info -repeat 100 {
+	recv info
+expect ~ "haproxy\\[${h1_pid}\\]: Ta=[0-9]* Tc=[0-9]* Td=-1 Th=[0-9]* Ti=[0-9]* Tq=[0-9]* TR=[0-9] Tr=-1 Tt=[0-9]* Tw=[0-9]*"
+} -start
+
+haproxy h1 -conf {
+	defaults
+		timeout client  1s
+		timeout connect 1s
+
+	global
+		lua-load ${testdir}/b3.lua
+		nbthread 4
+
+	frontend frt
+	log ${Slog_addr}:${Slog_port} local0 debug err
+	log-format Ta=%Ta\ Tc=%Tc\ Td=%Td\ Th=%Th\ Ti=%Ti\ Tq=%Tq\ TR=%TR\ Tr=%Tr\ Tt=%Tt\ Tw=%Tw
+		mode http
+		bind "fd@${frt}"
+		http-request use-service lua.donothing
+} -start
+
+
+client c1 -connect ${h1_frt_sock} -repeat 100  {
+send "GET / HTTP/1.1\n\n"
+} -run
+
+syslog Slog -wait
+
+shell {
+	ss -pt | grep CLOSE-WAIT.*haproxy
+exit $((!$?))
+}
-- 
2.11.0

Re: [PATCH] REGTEST/MINOR

[Willy Tarreau]

On Thu, Aug 23, 2018 at 09:01:45AM +0200, Frederic Lecaille wrote:
> Hi ML,
> 
> Here are two patches for haproxy reg testing.
(...)

Applied, thanks Fred!
Willy

[PATCH] REGTEST/MINOR

[Frederic Lecaille]

Hi ML,

Here are two patches for haproxy reg testing.

Note that we have recently added an new feature to varnishtest so that 
to send commands to the CLI without running a shell, socat etc 
(https://varnish-cache.org/docs/trunk/reference/vtc.html#haproxy). This 
breaks reg-tests/spoe/h0.vtc reg test case (fixed by the first patch).


We can send commands to the CLI as follows:

haproxy h1 -conf {...}

haproxy h1 -cli {
send "show info"
expect ~ "something to expect"
} -wait


With the 2nd patch we changed the prefix of the VTC files from 'h' 
letter to 'b' (as "bug") and added a new LEVEL 4 to run all the VTC 
files prefixed with 'b' letter. This VTC files are in relation with bugs 
they help to reproduce (and prevent from coming back).


So, for now on, if you run reg-tests, as there is no more LEVEL 1 
(default LEVEL) VTC files, no VTC file will be run.


To run the LEVEL 4 (VTC files for already fixed bugs) you will have to 
use such a command:


$ 
VARNISHTEST_PROGRAM=~/src/varnish-cache-trunk/bin/varnishtest/varnishtest 
LEVEL=4 make reg-tests



Fred.
>From 35486065bf63a47d17ba65b94d7b42b08d958abb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?= 
Date: Wed, 22 Aug 2018 10:41:33 +0200
Subject: [PATCH 2/2] REGTEST/MINOR: Add a new class of regression testing
 files.

Add LEVEL #4 regression testing files which is dedicated to
VTC files in relation with bugs they help to reproduce.
At the date of this commit, all VTC files are LEVEL 4 VTC files.
---
 Makefile | 7 +++
 reg-tests/log/{h0.vtc => b0.vtc} | 0
 reg-tests/lua/{h0.lua => b0.lua} | 0
 reg-tests/lua/{h0.vtc => b0.vtc} | 2 +-
 reg-tests/seamless-reload/{h0.vtc => b0.vtc} | 0
 reg-tests/spoe/{h0.vtc => b0.vtc}| 0
 reg-tests/ssl/{h0.vtc => b0.vtc} | 0
 reg-tests/stick-table/{h0.vtc => b0.vtc} | 0
 8 files changed, 8 insertions(+), 1 deletion(-)
 rename reg-tests/log/{h0.vtc => b0.vtc} (100%)
 rename reg-tests/lua/{h0.lua => b0.lua} (100%)
 rename reg-tests/lua/{h0.vtc => b0.vtc} (97%)
 rename reg-tests/seamless-reload/{h0.vtc => b0.vtc} (100%)
 rename reg-tests/spoe/{h0.vtc => b0.vtc} (100%)
 rename reg-tests/ssl/{h0.vtc => b0.vtc} (100%)
 rename reg-tests/stick-table/{h0.vtc => b0.vtc} (100%)

diff --git a/Makefile b/Makefile
index 817161f7..c4923995 100644
--- a/Makefile
+++ b/Makefile
@@ -999,6 +999,11 @@ update-version:
 	echo "$(SUBVERS)" > SUBVERS
 	echo "$(VERDATE)" > VERDATE
 
+# Target to run the regression testing script files.
+# LEVEL 1 scripts are dedicated to pure haproxy compliance tests (prefixed with 'h' letter).
+# LEVEL 2 scripts are slow scripts (prefixed with 's' letter).
+# LEVEL 3 scripts are low interest scripts (prefixed with 'l' letter).
+# LEVEL 4 scripts are in relation with bugs they help to reproduce (prefixed with 'b' letter).
 reg-tests:
 	@if [ ! -x "$(VARNISHTEST_PROGRAM)" ]; then \
 		echo "Please make the VARNISHTEST_PROGRAM variable point to the location of the varnishtest program."; \
@@ -1011,6 +1016,8 @@ reg-tests:
 	   EXPR='s*.vtc'; \
 	elif [ $$LEVEL = 3 ] ; then \
 	   EXPR='l*.vtc'; \
+	elif [ $$LEVEL = 4 ] ; then \
+	   EXPR='b*.vtc'; \
 	fi ; \
 	if [ -n "$$EXPR" ] ; then \
 	   find reg-tests -type f -name "$$EXPR" -print0 | \
diff --git a/reg-tests/log/h0.vtc b/reg-tests/log/b0.vtc
similarity index 100%
rename from reg-tests/log/h0.vtc
rename to reg-tests/log/b0.vtc
diff --git a/reg-tests/lua/h0.lua b/reg-tests/lua/b0.lua
similarity index 100%
rename from reg-tests/lua/h0.lua
rename to reg-tests/lua/b0.lua
diff --git a/reg-tests/lua/h0.vtc b/reg-tests/lua/b0.vtc
similarity index 97%
rename from reg-tests/lua/h0.vtc
rename to reg-tests/lua/b0.vtc
index 2b2ffb0e..4229eeb0 100644
--- a/reg-tests/lua/h0.vtc
+++ b/reg-tests/lua/b0.vtc
@@ -40,7 +40,7 @@ server s1 -repeat 2 {
 
 haproxy h1 -conf {
 global
-lua-load ${testdir}/h0.lua
+lua-load ${testdir}/b0.lua
 
 frontend fe1
 mode http
diff --git a/reg-tests/seamless-reload/h0.vtc b/reg-tests/seamless-reload/b0.vtc
similarity index 100%
rename from reg-tests/seamless-reload/h0.vtc
rename to reg-tests/seamless-reload/b0.vtc
diff --git a/reg-tests/spoe/h0.vtc b/reg-tests/spoe/b0.vtc
similarity index 100%
rename from reg-tests/spoe/h0.vtc
rename to reg-tests/spoe/b0.vtc
diff --git a/reg-tests/ssl/h0.vtc b/reg-tests/ssl/b0.vtc
similarity index 100%
rename from reg-tests/ssl/h0.vtc
rename to reg-tests/ssl/b0.vtc
diff --git a/reg-tests/stick-table/h0.vtc b/reg-tests/stick-table/b0.vtc
similarity index 100%
rename from reg-tests/stick-table/h0.vtc
rename to reg-tests/stick-table/b0.vtc
-- 
2.11.0

>From 971de0c29c683d629e

Re: [PATCH] REGTEST/MINOR: Unexpected curl URL globling.

[Willy Tarreau]

On Fri, Jul 13, 2018 at 10:51:07AM +0200, Frederic Lecaille wrote:
> Another patch to fix a remaining issue with this boring VTC file reported by
> Ilya.

Applied, thanks Fred!

willy

[PATCH] REGTEST/MINOR: Unexpected curl URL globling.

[Frederic Lecaille]

On 07/12/2018 05:52 PM, Willy Tarreau wrote:

On Thu, Jul 12, 2018 at 11:05:30AM +0200, Frederic Lecaille wrote:

This is a patch to fix the issue reported by Ilya Shipitsin in this thread.


Applied, thank you Fred.

Willy



Another patch to fix a remaining issue with this boring VTC file 
reported by Ilya.


Fred.
>From 1f3c909b3a649059a41d7fa97b4bb4b7be056c53 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?= 
Date: Fri, 13 Jul 2018 10:44:12 +0200
Subject: [PATCH] REGTEST/MINOR: Unexpected curl URL globling.

With certain curl versions URLs which contain brackets may be interpreted
by the "URL globbing parser". This patch ensures that such brackets
are escaped.

Thank you to Ilya Shipitsin for having reported this issue.
---
 reg-tests/ssl/h0.vtc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/reg-tests/ssl/h0.vtc b/reg-tests/ssl/h0.vtc
index 819f385..d1b48fb 100644
--- a/reg-tests/ssl/h0.vtc
+++ b/reg-tests/ssl/h0.vtc
@@ -34,7 +34,7 @@ haproxy h1 -conf {
 shell {
 HOST=${h1_frt_addr}
 if [ "${h1_frt_addr}" = "::1" ] ; then
-HOST="[::1]"
+HOST="\[::1\]"
 fi
 for i in 1 2 3 4 5; do
 curl -i -k https://$HOST:${h1_frt_port} & pids="$pids $!"
-- 
2.1.4

Re: [PATCH] REGTEST/MINOR: Wrong URI syntax.

[Frederic Lecaille]

On 07/13/2018 10:17 AM, Илья Шипицин wrote:



пт, 13 июл. 2018 г. в 13:08, Frederic Lecaille >:


On 07/13/2018 09:53 AM, Илья Шипицин wrote:
 > sorry, I did not test it on centos 7
 >
 > https://gitlab.com/chipitsine/haproxy/-/jobs/81501288
 >
 >
 > (I could not find out what's that, error message is strange, I'll
try to
 > investigate on separate vm)
 >
 > чт, 12 июл. 2018 г. в 14:08, Frederic Lecaille
mailto:flecai...@haproxy.com>
 > >>:
 >
 >     This is a patch to fix the issue reported by Ilya Shipitsin
in this
 >     thread.
 >
 >     Fred.
 >

This is becoming boring. This is why we should not use external
programs
like curl. We will perhaps have to disable this vtc file.



once CI is up and running it will not be boring anymore.


Try one of these patches with a preference for reg-tests.2.diff.


I confirm test is green now



Thank you for your good job Ilya.

Regards.

Re: [PATCH] REGTEST/MINOR: Wrong URI syntax.

[Илья Шипицин]

пт, 13 июл. 2018 г. в 13:08, Frederic Lecaille :

> On 07/13/2018 09:53 AM, Илья Шипицин wrote:
> > sorry, I did not test it on centos 7
> >
> > https://gitlab.com/chipitsine/haproxy/-/jobs/81501288
> >
> >
> > (I could not find out what's that, error message is strange, I'll try to
> > investigate on separate vm)
> >
> > чт, 12 июл. 2018 г. в 14:08, Frederic Lecaille  > >:
> >
> > This is a patch to fix the issue reported by Ilya Shipitsin in this
> > thread.
> >
> > Fred.
> >
>
> This is becoming boring. This is why we should not use external programs
> like curl. We will perhaps have to disable this vtc file.
>


once CI is up and running it will not be boring anymore.


>
> Try one of these patches with a preference for reg-tests.2.diff.
>

I confirm test is green now


>
> They both work on my linux host with curl 7.38.0.
>
>

Re: [PATCH] REGTEST/MINOR: Wrong URI syntax.

[Frederic Lecaille]

On 07/13/2018 09:53 AM, Илья Шипицин wrote:

sorry, I did not test it on centos 7

https://gitlab.com/chipitsine/haproxy/-/jobs/81501288


(I could not find out what's that, error message is strange, I'll try to 
investigate on separate vm)


чт, 12 июл. 2018 г. в 14:08, Frederic Lecaille >:


This is a patch to fix the issue reported by Ilya Shipitsin in this
thread.

Fred.



This is becoming boring. This is why we should not use external programs 
like curl. We will perhaps have to disable this vtc file.


Try one of these patches with a preference for reg-tests.2.diff.

They both work on my linux host with curl 7.38.0.

diff --git a/reg-tests/ssl/h0.vtc b/reg-tests/ssl/h0.vtc
index 819f385..d1b48fb 100644
--- a/reg-tests/ssl/h0.vtc
+++ b/reg-tests/ssl/h0.vtc
@@ -34,7 +34,7 @@ haproxy h1 -conf {
 shell {
 HOST=${h1_frt_addr}
 if [ "${h1_frt_addr}" = "::1" ] ; then
-HOST="[::1]"
+HOST="\[::1\]"
 fi
 for i in 1 2 3 4 5; do
 curl -i -k https://$HOST:${h1_frt_port} & pids="$pids $!"
diff --git a/reg-tests/ssl/h0.vtc b/reg-tests/ssl/h0.vtc
index 819f385..5239c53 100644
--- a/reg-tests/ssl/h0.vtc
+++ b/reg-tests/ssl/h0.vtc
@@ -37,7 +37,7 @@ shell {
 HOST="[::1]"
 fi
 for i in 1 2 3 4 5; do
-curl -i -k https://$HOST:${h1_frt_port} & pids="$pids $!"
+curl -gi -k https://$HOST:${h1_frt_port} & pids="$pids $!"
 done
 wait $pids
 }

Re: [PATCH] REGTEST/MINOR: Wrong URI syntax.

[Илья Шипицин]

sorry, I did not test it on centos 7

https://gitlab.com/chipitsine/haproxy/-/jobs/81501288


(I could not find out what's that, error message is strange, I'll try to
investigate on separate vm)

чт, 12 июл. 2018 г. в 14:08, Frederic Lecaille :

> This is a patch to fix the issue reported by Ilya Shipitsin in this thread.
>
> Fred.
>

Re: [PATCH] REGTEST/MINOR: Wrong URI syntax.

[Willy Tarreau]

On Thu, Jul 12, 2018 at 11:05:30AM +0200, Frederic Lecaille wrote:
> This is a patch to fix the issue reported by Ilya Shipitsin in this thread.

Applied, thank you Fred.

Willy

[PATCH] REGTEST/MINOR: Wrong URI syntax.

[Frederic Lecaille]

This is a patch to fix the issue reported by Ilya Shipitsin in this thread.

Fred.
>From 47ca7696d0ccca5989929940db323e9e9255ae4a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?= 
Date: Thu, 12 Jul 2018 10:48:06 +0200
Subject: [PATCH] REGTEST/MINOR: Wrong URI syntax.

Ilya Shipitsin reported that with some curl versions this reg test
may fail due to a wrong URI syntax with ::1 ipv6 local address in
this varnishtest script. This patch fixes this syntax issue and
replaces the iteration of "procees" commands by a "shell" command
to start curl processes (must be faster).

Thanks to Ilya Shipitsin for having reported this VTC file bug.
---
 reg-tests/ssl/h0.vtc | 21 ++---
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/reg-tests/ssl/h0.vtc b/reg-tests/ssl/h0.vtc
index 0765cb4..819f385 100644
--- a/reg-tests/ssl/h0.vtc
+++ b/reg-tests/ssl/h0.vtc
@@ -31,14 +31,13 @@ haproxy h1 -conf {
 http-request redirect location /
 } -start
 
-process p1 "curl -i -k https://${h1_frt_addr}:${h1_frt_port}"; -start
-process p2 "curl -i -k https://${h1_frt_addr}:${h1_frt_port}"; -start
-process p3 "curl -i -k https://${h1_frt_addr}:${h1_frt_port}"; -start
-process p4 "curl -i -k https://${h1_frt_addr}:${h1_frt_port}"; -start
-process p5 "curl -i -k https://${h1_frt_addr}:${h1_frt_port}"; -start
-
-process p1 -wait
-process p2 -wait
-process p3 -wait
-process p4 -wait
-process p5 -wait
+shell {
+HOST=${h1_frt_addr}
+if [ "${h1_frt_addr}" = "::1" ] ; then
+HOST="[::1]"
+fi
+for i in 1 2 3 4 5; do
+curl -i -k https://$HOST:${h1_frt_port} & pids="$pids $!"
+done
+wait $pids
+}
-- 
2.1.4