[devel] [APT PATCH] test/integration/framework/test-apt-method-http*: use nginx as a forking daemon

[devel] [APT PATCH] test/integration/framework/test-apt-method-http*: use nginx as a forking daemon

[Ivan Zakharyaschev]

[-- Attachment #1: Type: text/plain, Size: 4898 bytes --]

This is needed not to send requests to nginx before it is ready to
listen: when the main process forks and exits, nginx is considered to
be ready.

Without this change, I observed a failure in the http test, but not
the https one, probably, because the delay in the https test was
larger due to an extra package being built:

Run Testcase (22/29) test-apt-method-http
Building package: simple-package
Test for successful execution of apt-get update …
Err http://localhost x86_64 release
  Could not connect to localhost:8080 (127.0.0.1). - connect (111 Connection refused)

However:

Run Testcase (23/29) test-apt-method-https
Building package: simple-package
Building package: conflicting-package-one
Test for successful execution of apt-get update … PASS
Test that package(s) are not installed with rpm -q simple-package … PASS
Test for successful execution of apt-get install simple-package … PASS
Test that package(s) are installed with rpm -q simple-package … PASS
Pinning invalid key in apt
Test for failure in execution of apt-get update … PASS
Test that package(s) are not installed with rpm -q conflicting-package-one … PASS
Test for failure in execution of apt-get install conflicting-package-one … PASS
Test that package(s) are not installed with rpm -q conflicting-package-one … PASS
---
 test/integration/framework                                   | 2 --
 test/integration/test-apt-method-http                        | 5 +++--
 test/integration/test-apt-method-https                       | 5 +++--
 test/integration/test-apt-method-https-invalid-cert-hostname | 5 +++--
 4 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/test/integration/framework b/test/integration/framework
index 5f0f58eda..eccfb174e 100644
--- a/test/integration/framework
+++ b/test/integration/framework
@@ -560,7 +560,6 @@ nginxsetuphttp() {
 cat >> $TMPWORKINGDIRECTORY/nginx/nginx.conf << ENDCONFIG
 worker_processes 1;
 error_log $TMPWORKINGDIRECTORY/nginx/error.log;
-daemon off;
 pid $TMPWORKINGDIRECTORY/nginx/nginx.pid;
 
 events {
@@ -606,7 +605,6 @@ nginxsetuphttps() {
 	cat >> $TMPWORKINGDIRECTORY/nginx/nginx.conf << ENDCONFIG
 worker_processes 1;
 error_log $TMPWORKINGDIRECTORY/nginx/error.log;
-daemon off;
 pid $TMPWORKINGDIRECTORY/nginx/nginx.pid;
 
 events {
diff --git a/test/integration/test-apt-method-http b/test/integration/test-apt-method-http
index 0872c99d4..d463a0d14 100755
--- a/test/integration/test-apt-method-http
+++ b/test/integration/test-apt-method-http
@@ -17,8 +17,9 @@ rpm http://localhost:8080/ $(getarchitecture) apt-tests
 rpm http://localhost:8080/ noarch apt-tests
 END
 
-/usr/sbin/nginx -c $TMPWORKINGDIRECTORY/nginx/nginx.conf -p $TMPWORKINGDIRECTORY &>> $TMPWORKINGDIRECTORY/nginx/process-stderr.log &
-NGINXPID=$!
+/usr/sbin/nginx -c $TMPWORKINGDIRECTORY/nginx/nginx.conf -p $TMPWORKINGDIRECTORY &>> $TMPWORKINGDIRECTORY/nginx/process-stderr.log
+NGINXPID="$(cat $TMPWORKINGDIRECTORY/nginx/nginx.pid)"
+[ -n "$NGINXPID" ]
 
 addtrap 'prefix' "kill -SIGTERM $NGINXPID; [ \"$EXIT_CODE\" = '0' ] || cat $TMPWORKINGDIRECTORY/nginx/process-stderr.log;"
 
diff --git a/test/integration/test-apt-method-https b/test/integration/test-apt-method-https
index 29e4d2d2b..4b2463a4c 100755
--- a/test/integration/test-apt-method-https
+++ b/test/integration/test-apt-method-https
@@ -30,8 +30,9 @@ rpm https://localhost:8080/ $(getarchitecture) apt-tests
 rpm https://localhost:8080/ noarch apt-tests
 END
 
-/usr/sbin/nginx -c $TMPWORKINGDIRECTORY/nginx/nginx.conf -p $TMPWORKINGDIRECTORY &>> $TMPWORKINGDIRECTORY/nginx/process-stderr.log &
-NGINXPID=$!
+/usr/sbin/nginx -c $TMPWORKINGDIRECTORY/nginx/nginx.conf -p $TMPWORKINGDIRECTORY &>> $TMPWORKINGDIRECTORY/nginx/process-stderr.log
+NGINXPID="$(cat $TMPWORKINGDIRECTORY/nginx/nginx.pid)"
+[ -n "$NGINXPID" ]
 
 addtrap 'prefix' "kill -SIGTERM $NGINXPID; [ \"$EXIT_CODE\" = '0' ] || cat $TMPWORKINGDIRECTORY/nginx/process-stderr.log;"
 
diff --git a/test/integration/test-apt-method-https-invalid-cert-hostname b/test/integration/test-apt-method-https-invalid-cert-hostname
index 33e80b613..b7b036bbd 100755
--- a/test/integration/test-apt-method-https-invalid-cert-hostname
+++ b/test/integration/test-apt-method-https-invalid-cert-hostname
@@ -29,8 +29,9 @@ rpm https://localhost:8080/ $(getarchitecture) apt-tests
 rpm https://localhost:8080/ noarch apt-tests
 END
 
-/usr/sbin/nginx -c $TMPWORKINGDIRECTORY/nginx/nginx.conf -p $TMPWORKINGDIRECTORY &>> $TMPWORKINGDIRECTORY/nginx/process-stderr.log &
-NGINXPID=$!
+/usr/sbin/nginx -c $TMPWORKINGDIRECTORY/nginx/nginx.conf -p $TMPWORKINGDIRECTORY &>> $TMPWORKINGDIRECTORY/nginx/process-stderr.log
+NGINXPID="$(cat $TMPWORKINGDIRECTORY/nginx/nginx.pid)"
+[ -n "$NGINXPID" ]
 
 addtrap 'prefix' "kill -SIGTERM $NGINXPID; [ \"$EXIT_CODE\" = '0' ] || cat $TMPWORKINGDIRECTORY/nginx/process-stderr.log;"
 
-- 
2.24.1


-- 
Best regards,
Ivan

Re: [devel] [APT PATCH] test/integration/framework/test-apt-method-http*: use nginx as a forking daemon

[Aleksei Nikiforov]

18.02.2020 06:39, Ivan Zakharyaschev пишет:
> This is needed not to send requests to nginx before it is ready to
> listen: when the main process forks and exits, nginx is considered to
> be ready.
> 
> Without this change, I observed a failure in the http test, but not
> the https one, probably, because the delay in the https test was
> larger due to an extra package being built:
> 
> Run Testcase (22/29) test-apt-method-http
> Building package: simple-package
> Test for successful execution of apt-get update …
> Err http://localhost x86_64 release
>    Could not connect to localhost:8080 (127.0.0.1). - connect (111 Connection refused)
> 
> However:
> 
> Run Testcase (23/29) test-apt-method-https
> Building package: simple-package
> Building package: conflicting-package-one
> Test for successful execution of apt-get update … PASS
> Test that package(s) are not installed with rpm -q simple-package … PASS
> Test for successful execution of apt-get install simple-package … PASS
> Test that package(s) are installed with rpm -q simple-package … PASS
> Pinning invalid key in apt
> Test for failure in execution of apt-get update … PASS
> Test that package(s) are not installed with rpm -q conflicting-package-one … PASS
> Test for failure in execution of apt-get install conflicting-package-one … PASS
> Test that package(s) are not installed with rpm -q conflicting-package-one … PASS
> ---
>   test/integration/framework                                   | 2 --
>   test/integration/test-apt-method-http                        | 5 +++--
>   test/integration/test-apt-method-https                       | 5 +++--
>   test/integration/test-apt-method-https-invalid-cert-hostname | 5 +++--
>   4 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/test/integration/framework b/test/integration/framework
> index 5f0f58eda..eccfb174e 100644
> --- a/test/integration/framework
> +++ b/test/integration/framework
> @@ -560,7 +560,6 @@ nginxsetuphttp() {
>   cat >> $TMPWORKINGDIRECTORY/nginx/nginx.conf << ENDCONFIG
>   worker_processes 1;
>   error_log $TMPWORKINGDIRECTORY/nginx/error.log;
> -daemon off;
>   pid $TMPWORKINGDIRECTORY/nginx/nginx.pid;
>   
>   events {
> @@ -606,7 +605,6 @@ nginxsetuphttps() {
>   	cat >> $TMPWORKINGDIRECTORY/nginx/nginx.conf << ENDCONFIG
>   worker_processes 1;
>   error_log $TMPWORKINGDIRECTORY/nginx/error.log;
> -daemon off;
>   pid $TMPWORKINGDIRECTORY/nginx/nginx.pid;
>   
>   events {
> diff --git a/test/integration/test-apt-method-http b/test/integration/test-apt-method-http
> index 0872c99d4..d463a0d14 100755
> --- a/test/integration/test-apt-method-http
> +++ b/test/integration/test-apt-method-http
> @@ -17,8 +17,9 @@ rpm http://localhost:8080/ $(getarchitecture) apt-tests
>   rpm http://localhost:8080/ noarch apt-tests
>   END
>   
> -/usr/sbin/nginx -c $TMPWORKINGDIRECTORY/nginx/nginx.conf -p $TMPWORKINGDIRECTORY &>> $TMPWORKINGDIRECTORY/nginx/process-stderr.log &
> -NGINXPID=$!
> +/usr/sbin/nginx -c $TMPWORKINGDIRECTORY/nginx/nginx.conf -p $TMPWORKINGDIRECTORY &>> $TMPWORKINGDIRECTORY/nginx/process-stderr.log
> +NGINXPID="$(cat $TMPWORKINGDIRECTORY/nginx/nginx.pid)"
> +[ -n "$NGINXPID" ]
>   
>   addtrap 'prefix' "kill -SIGTERM $NGINXPID; [ \"$EXIT_CODE\" = '0' ] || cat $TMPWORKINGDIRECTORY/nginx/process-stderr.log;"
>   
> diff --git a/test/integration/test-apt-method-https b/test/integration/test-apt-method-https
> index 29e4d2d2b..4b2463a4c 100755
> --- a/test/integration/test-apt-method-https
> +++ b/test/integration/test-apt-method-https
> @@ -30,8 +30,9 @@ rpm https://localhost:8080/ $(getarchitecture) apt-tests
>   rpm https://localhost:8080/ noarch apt-tests
>   END
>   
> -/usr/sbin/nginx -c $TMPWORKINGDIRECTORY/nginx/nginx.conf -p $TMPWORKINGDIRECTORY &>> $TMPWORKINGDIRECTORY/nginx/process-stderr.log &
> -NGINXPID=$!
> +/usr/sbin/nginx -c $TMPWORKINGDIRECTORY/nginx/nginx.conf -p $TMPWORKINGDIRECTORY &>> $TMPWORKINGDIRECTORY/nginx/process-stderr.log
> +NGINXPID="$(cat $TMPWORKINGDIRECTORY/nginx/nginx.pid)"
> +[ -n "$NGINXPID" ]
>   
>   addtrap 'prefix' "kill -SIGTERM $NGINXPID; [ \"$EXIT_CODE\" = '0' ] || cat $TMPWORKINGDIRECTORY/nginx/process-stderr.log;"
>   
> diff --git a/test/integration/test-apt-method-https-invalid-cert-hostname b/test/integration/test-apt-method-https-invalid-cert-hostname
> index 33e80b613..b7b036bbd 100755
> --- a/test/integration/test-apt-method-https-invalid-cert-hostname
> +++ b/test/integration/test-apt-method-https-invalid-cert-hostname
> @@ -29,8 +29,9 @@ rpm https://localhost:8080/ $(getarchitecture) apt-tests
>   rpm https://localhost:8080/ noarch apt-tests
>   END
>   
> -/usr/sbin/nginx -c $TMPWORKINGDIRECTORY/nginx/nginx.conf -p $TMPWORKINGDIRECTORY &>> $TMPWORKINGDIRECTORY/nginx/process-stderr.log &
> -NGINXPID=$!
> +/usr/sbin/nginx -c $TMPWORKINGDIRECTORY/nginx/nginx.conf -p $TMPWORKINGDIRECTORY &>> $TMPWORKINGDIRECTORY/nginx/process-stderr.log
> +NGINXPID="$(cat $TMPWORKINGDIRECTORY/nginx/nginx.pid)"
> +[ -n "$NGINXPID" ]
>   
>   addtrap 'prefix' "kill -SIGTERM $NGINXPID; [ \"$EXIT_CODE\" = '0' ] || cat $TMPWORKINGDIRECTORY/nginx/process-stderr.log;"
>   
> 

Does nginx always finish starting before actual test runs with these 
changes? Or does it sometimes still fail to start in time but produces 
different error? And I'd prefer to have a verbose error message like 
'Nginx failed start in time' or something else instead of default one.

Re: [devel] [APT PATCH] test/integration/framework/test-apt-method-http*: use nginx as a forking daemon

[Ivan Zakharyaschev]

[-- Attachment #1: Type: text/plain, Size: 8043 bytes --]

Hello!

On Tue, 18 Feb 2020, Aleksei Nikiforov wrote:

> 18.02.2020 06:39, Ivan Zakharyaschev пишет:
> > This is needed not to send requests to nginx before it is ready to
> > listen: when the main process forks and exits, nginx is considered to
> > be ready.
> > 
> > Without this change, I observed a failure in the http test, but not
> > the https one, probably, because the delay in the https test was
> > larger due to an extra package being built:
> > 
> > Run Testcase (22/29) test-apt-method-http
> > Building package: simple-package
> > Test for successful execution of apt-get update …
> > Err http://localhost x86_64 release
> >    Could not connect to localhost:8080 (127.0.0.1). - connect (111
> >    Connection refused)
> > 
> > However:
> > 
> > Run Testcase (23/29) test-apt-method-https
> > Building package: simple-package
> > Building package: conflicting-package-one
> > Test for successful execution of apt-get update … PASS
> > Test that package(s) are not installed with rpm -q simple-package … PASS
> > Test for successful execution of apt-get install simple-package … PASS
> > Test that package(s) are installed with rpm -q simple-package … PASS
> > Pinning invalid key in apt
> > Test for failure in execution of apt-get update … PASS
> > Test that package(s) are not installed with rpm -q conflicting-package-one …
> > PASS
> > Test for failure in execution of apt-get install conflicting-package-one …
> > PASS
> > Test that package(s) are not installed with rpm -q conflicting-package-one …
> > PASS
> > ---
> >   test/integration/framework                                   | 2 --
> >   test/integration/test-apt-method-http                        | 5 +++--
> >   test/integration/test-apt-method-https                       | 5 +++--
> >   test/integration/test-apt-method-https-invalid-cert-hostname | 5 +++--
> >   4 files changed, 9 insertions(+), 8 deletions(-)
> > 
> > diff --git a/test/integration/framework b/test/integration/framework
> > index 5f0f58eda..eccfb174e 100644
> > --- a/test/integration/framework
> > +++ b/test/integration/framework
> > @@ -560,7 +560,6 @@ nginxsetuphttp() {
> >   cat >> $TMPWORKINGDIRECTORY/nginx/nginx.conf << ENDCONFIG
> >   worker_processes 1;
> >   error_log $TMPWORKINGDIRECTORY/nginx/error.log;
> > -daemon off;
> >   pid $TMPWORKINGDIRECTORY/nginx/nginx.pid;
> >   
> >   events {
> > @@ -606,7 +605,6 @@ nginxsetuphttps() {
> > cat > > $TMPWORKINGDIRECTORY/nginx/nginx.conf << ENDCONFIG
> >   worker_processes 1;
> >   error_log $TMPWORKINGDIRECTORY/nginx/error.log;
> > -daemon off;
> >   pid $TMPWORKINGDIRECTORY/nginx/nginx.pid;
> >   
> >   events {
> > diff --git a/test/integration/test-apt-method-http
> > b/test/integration/test-apt-method-http
> > index 0872c99d4..d463a0d14 100755
> > --- a/test/integration/test-apt-method-http
> > +++ b/test/integration/test-apt-method-http
> > @@ -17,8 +17,9 @@ rpm http://localhost:8080/ $(getarchitecture) apt-tests
> >   rpm http://localhost:8080/ noarch apt-tests
> >   END
> >   
> > -/usr/sbin/nginx -c $TMPWORKINGDIRECTORY/nginx/nginx.conf -p
> > $TMPWORKINGDIRECTORY &>> $TMPWORKINGDIRECTORY/nginx/process-stderr.log &
> > -NGINXPID=$!
> > +/usr/sbin/nginx -c $TMPWORKINGDIRECTORY/nginx/nginx.conf -p
> > $TMPWORKINGDIRECTORY &>> $TMPWORKINGDIRECTORY/nginx/process-stderr.log
> > +NGINXPID="$(cat $TMPWORKINGDIRECTORY/nginx/nginx.pid)"
> > +[ -n "$NGINXPID" ]
> >   
> >   addtrap 'prefix' "kill -SIGTERM $NGINXPID; [ \"$EXIT_CODE\" = '0' ] || cat
> >   $TMPWORKINGDIRECTORY/nginx/process-stderr.log;"

> Does nginx always finish starting before actual test runs with these changes?
> Or does it sometimes still fail to start in time but produces different error?

In my ca. 10 runs, this never happened. (In my 3-4 runs of the previous 
revision, it always happened.)

In my solution, I relied on the description of the behavior of a 
"traditional UNIX forking daemon" given in man systemd.service:

	    If set to forking it is expected that the process
	    configured with ExecStart= will call fork() as part of
	    its start-up. The parent process is expected to exit
	    when start-up is complete and all communication channels
	    set up. The child continues to run as the main daemon
	    process. This is the behavior of traditional UNIX
	    daemons. If this setting is used, it is recommended to
	    also use the PIDFile= option, so that systemd can
	    identify the main process of the daemon. systemd will
	    proceed starting follow-up units as soon as the parent
	    process exits.

I believe that nginx conforms to this specification without further 
explorations. However, I did practical experiments as stated above.

> And I'd prefer to have a verbose error message like 'Nginx failed start in
> time' or something else instead of default one.

Let's look what the error messages look like now and before my changes.
To model it, I did: chmod a-x /usr/sbin/nginx

In my revision:

Run Testcase (22/29) test-apt-method-http
Building package: simple-package
E: Looks like the testcases ended prematurely with exitcode: 126
test-apt-method-http ... FAIL

Quit short and clear IMHO.

In the previous revision, the reason for failure is not caught as early 
and not reported clearly. The messages are long:

Run Testcase (22/29) test-apt-method-http
Building package: simple-package
Test for successful execution of apt-get update … 
Err http://localhost x86_64 release
  Could not connect to localhost:8080 (127.0.0.1). - connect (111 
Connection refused)
Err http://localhost noarch release
  Could not connect to localhost:8080 (127.0.0.1). - connect (111 
Connection refused)
Err http://localhost x86_64/apt-tests pkglist
  Could not connect to localhost:8080 (127.0.0.1). - connect (111 
Connection refused)
Err http://localhost x86_64/apt-tests release
  Could not connect to localhost:8080 (127.0.0.1). - connect (111 
Connection refused)
Err http://localhost noarch/apt-tests pkglist
  Could not connect to localhost:8080 (127.0.0.1). - connect (111 
Connection refused)
Err http://localhost noarch/apt-tests release
  Could not connect to localhost:8080 (127.0.0.1). - connect (111 
Connection refused)
Failed to fetch http://localhost:8080/x86_64/base/release  Could not 
connect to localhost:8080 (127.0.0.1). - connect (111 Connection refused)
Failed to fetch http://localhost:8080/noarch/base/release  Could not 
connect to localhost:8080 (127.0.0.1). - connect (111 Connection refused)
Failed to fetch http://localhost:8080/x86_64/base/pkglist.apt-tests  Could 
not connect to localhost:8080 (127.0.0.1). - connect (111 Connection 
refused)
Failed to fetch http://localhost:8080/x86_64/base/release.apt-tests  Could 
not connect to localhost:8080 (127.0.0.1). - connect (111 Connection 
refused)
Failed to fetch http://localhost:8080/noarch/base/pkglist.apt-tests  Could 
not connect to localhost:8080 (127.0.0.1). - connect (111 Connection 
refused)
Failed to fetch http://localhost:8080/noarch/base/release.apt-tests  Could 
not connect to localhost:8080 (127.0.0.1). - connect (111 Connection 
refused)
Reading Package Lists...
Building Dependency Tree...
W: Release files for some repositories could not be retrieved or 
authenticated. Such repositories are being ignored.
W: You may want to run apt-get update to correct these problems
E: Some index files failed to download, they have been ignored, or old 
ones used instead.
FAIL
Test that package(s) are not installed with rpm -q simple-package … PASS
Test for successful execution of apt-get install simple-package … 
Reading Package Lists...
Building Dependency Tree...
E: Couldn't find package simple-package
FAIL
Test that package(s) are installed with rpm -q simple-package … FAIL
./test-apt-method-http: line 1: kill: (30573) - No such process
test-apt-method-http ... FAIL


So, I think, after my changes the clearness of the failure messages 
improved. I'd suggest you can make an additional patch if you'd like to 
improve the error messages somehow.

-- 
Best regards,
Ivan

Re: [devel] [APT PATCH] test/integration/framework/test-apt-method-http*: use nginx as a forking daemon

[Aleksei Nikiforov]

18.02.2020 15:44, Ivan Zakharyaschev пишет:
> Hello!
> 
> On Tue, 18 Feb 2020, Aleksei Nikiforov wrote:
> 
>> 18.02.2020 06:39, Ivan Zakharyaschev пишет:
>>> This is needed not to send requests to nginx before it is ready to
>>> listen: when the main process forks and exits, nginx is considered to
>>> be ready.
>>>
>>> Without this change, I observed a failure in the http test, but not
>>> the https one, probably, because the delay in the https test was
>>> larger due to an extra package being built:
>>>
>>> Run Testcase (22/29) test-apt-method-http
>>> Building package: simple-package
>>> Test for successful execution of apt-get update …
>>> Err http://localhost x86_64 release
>>>     Could not connect to localhost:8080 (127.0.0.1). - connect (111
>>>     Connection refused)
>>>
>>> However:
>>>
>>> Run Testcase (23/29) test-apt-method-https
>>> Building package: simple-package
>>> Building package: conflicting-package-one
>>> Test for successful execution of apt-get update … PASS
>>> Test that package(s) are not installed with rpm -q simple-package … PASS
>>> Test for successful execution of apt-get install simple-package … PASS
>>> Test that package(s) are installed with rpm -q simple-package … PASS
>>> Pinning invalid key in apt
>>> Test for failure in execution of apt-get update … PASS
>>> Test that package(s) are not installed with rpm -q conflicting-package-one …
>>> PASS
>>> Test for failure in execution of apt-get install conflicting-package-one …
>>> PASS
>>> Test that package(s) are not installed with rpm -q conflicting-package-one …
>>> PASS
>>> ---
>>>    test/integration/framework                                   | 2 --
>>>    test/integration/test-apt-method-http                        | 5 +++--
>>>    test/integration/test-apt-method-https                       | 5 +++--
>>>    test/integration/test-apt-method-https-invalid-cert-hostname | 5 +++--
>>>    4 files changed, 9 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/test/integration/framework b/test/integration/framework
>>> index 5f0f58eda..eccfb174e 100644
>>> --- a/test/integration/framework
>>> +++ b/test/integration/framework
>>> @@ -560,7 +560,6 @@ nginxsetuphttp() {
>>>    cat >> $TMPWORKINGDIRECTORY/nginx/nginx.conf << ENDCONFIG
>>>    worker_processes 1;
>>>    error_log $TMPWORKINGDIRECTORY/nginx/error.log;
>>> -daemon off;
>>>    pid $TMPWORKINGDIRECTORY/nginx/nginx.pid;
>>>    
>>>    events {
>>> @@ -606,7 +605,6 @@ nginxsetuphttps() {
>>> cat > > $TMPWORKINGDIRECTORY/nginx/nginx.conf << ENDCONFIG
>>>    worker_processes 1;
>>>    error_log $TMPWORKINGDIRECTORY/nginx/error.log;
>>> -daemon off;
>>>    pid $TMPWORKINGDIRECTORY/nginx/nginx.pid;
>>>    
>>>    events {
>>> diff --git a/test/integration/test-apt-method-http
>>> b/test/integration/test-apt-method-http
>>> index 0872c99d4..d463a0d14 100755
>>> --- a/test/integration/test-apt-method-http
>>> +++ b/test/integration/test-apt-method-http
>>> @@ -17,8 +17,9 @@ rpm http://localhost:8080/ $(getarchitecture) apt-tests
>>>    rpm http://localhost:8080/ noarch apt-tests
>>>    END
>>>    
>>> -/usr/sbin/nginx -c $TMPWORKINGDIRECTORY/nginx/nginx.conf -p
>>> $TMPWORKINGDIRECTORY &>> $TMPWORKINGDIRECTORY/nginx/process-stderr.log &
>>> -NGINXPID=$!
>>> +/usr/sbin/nginx -c $TMPWORKINGDIRECTORY/nginx/nginx.conf -p
>>> $TMPWORKINGDIRECTORY &>> $TMPWORKINGDIRECTORY/nginx/process-stderr.log
>>> +NGINXPID="$(cat $TMPWORKINGDIRECTORY/nginx/nginx.pid)"
>>> +[ -n "$NGINXPID" ]
>>>    
>>>    addtrap 'prefix' "kill -SIGTERM $NGINXPID; [ \"$EXIT_CODE\" = '0' ] || cat
>>>    $TMPWORKINGDIRECTORY/nginx/process-stderr.log;"
> 
>> Does nginx always finish starting before actual test runs with these changes?
>> Or does it sometimes still fail to start in time but produces different error?
> 
> In my ca. 10 runs, this never happened. (In my 3-4 runs of the previous
> revision, it always happened.)
> 
> In my solution, I relied on the description of the behavior of a
> "traditional UNIX forking daemon" given in man systemd.service:
> 
> 	    If set to forking it is expected that the process
> 	    configured with ExecStart= will call fork() as part of
> 	    its start-up. The parent process is expected to exit
> 	    when start-up is complete and all communication channels
> 	    set up. The child continues to run as the main daemon
> 	    process. This is the behavior of traditional UNIX
> 	    daemons. If this setting is used, it is recommended to
> 	    also use the PIDFile= option, so that systemd can
> 	    identify the main process of the daemon. systemd will
> 	    proceed starting follow-up units as soon as the parent
> 	    process exits.
> 
> I believe that nginx conforms to this specification without further
> explorations. However, I did practical experiments as stated above.
> 
>> And I'd prefer to have a verbose error message like 'Nginx failed start in
>> time' or something else instead of default one.
> 
> Let's look what the error messages look like now and before my changes.
> To model it, I did: chmod a-x /usr/sbin/nginx
> 
> In my revision:
> 
> Run Testcase (22/29) test-apt-method-http
> Building package: simple-package
> E: Looks like the testcases ended prematurely with exitcode: 126
> test-apt-method-http ... FAIL
> 
> Quit short and clear IMHO.
> 

It's definitely shorter, but I don't agree with 'clear'. There is no 
reason stated for prematurely end and no location of this exit is 
specified either.

> In the previous revision, the reason for failure is not caught as early
> and not reported clearly. The messages are long:
> 
> Run Testcase (22/29) test-apt-method-http
> Building package: simple-package
> Test for successful execution of apt-get update …
> Err http://localhost x86_64 release
>    Could not connect to localhost:8080 (127.0.0.1). - connect (111
> Connection refused)
> Err http://localhost noarch release
>    Could not connect to localhost:8080 (127.0.0.1). - connect (111
> Connection refused)
> Err http://localhost x86_64/apt-tests pkglist
>    Could not connect to localhost:8080 (127.0.0.1). - connect (111
> Connection refused)
> Err http://localhost x86_64/apt-tests release
>    Could not connect to localhost:8080 (127.0.0.1). - connect (111
> Connection refused)
> Err http://localhost noarch/apt-tests pkglist
>    Could not connect to localhost:8080 (127.0.0.1). - connect (111
> Connection refused)
> Err http://localhost noarch/apt-tests release
>    Could not connect to localhost:8080 (127.0.0.1). - connect (111
> Connection refused)
> Failed to fetch http://localhost:8080/x86_64/base/release  Could not
> connect to localhost:8080 (127.0.0.1). - connect (111 Connection refused)
> Failed to fetch http://localhost:8080/noarch/base/release  Could not
> connect to localhost:8080 (127.0.0.1). - connect (111 Connection refused)
> Failed to fetch http://localhost:8080/x86_64/base/pkglist.apt-tests  Could
> not connect to localhost:8080 (127.0.0.1). - connect (111 Connection
> refused)
> Failed to fetch http://localhost:8080/x86_64/base/release.apt-tests  Could
> not connect to localhost:8080 (127.0.0.1). - connect (111 Connection
> refused)
> Failed to fetch http://localhost:8080/noarch/base/pkglist.apt-tests  Could
> not connect to localhost:8080 (127.0.0.1). - connect (111 Connection
> refused)
> Failed to fetch http://localhost:8080/noarch/base/release.apt-tests  Could
> not connect to localhost:8080 (127.0.0.1). - connect (111 Connection
> refused)
> Reading Package Lists...
> Building Dependency Tree...
> W: Release files for some repositories could not be retrieved or
> authenticated. Such repositories are being ignored.
> W: You may want to run apt-get update to correct these problems
> E: Some index files failed to download, they have been ignored, or old
> ones used instead.
> FAIL
> Test that package(s) are not installed with rpm -q simple-package … PASS
> Test for successful execution of apt-get install simple-package …
> Reading Package Lists...
> Building Dependency Tree...
> E: Couldn't find package simple-package
> FAIL
> Test that package(s) are installed with rpm -q simple-package … FAIL
> ./test-apt-method-http: line 1: kill: (30573) - No such process
> test-apt-method-http ... FAIL
> 
> 
> So, I think, after my changes the clearness of the failure messages
> improved. I'd suggest you can make an additional patch if you'd like to
> improve the error messages somehow.
> 

Re: [devel] [APT PATCH] test/integration/framework/test-apt-method-http*: use nginx as a forking daemon

[Dmitry V. Levin]

On Tue, Feb 18, 2020 at 06:39:01AM +0300, Ivan Zakharyaschev wrote:
> This is needed not to send requests to nginx before it is ready to
> listen: when the main process forks and exits, nginx is considered to
> be ready.
> 
> Without this change, I observed a failure in the http test, but not
> the https one, probably, because the delay in the https test was
> larger due to an extra package being built:
> 
> Run Testcase (22/29) test-apt-method-http
> Building package: simple-package
> Test for successful execution of apt-get update …
> Err http://localhost x86_64 release
>   Could not connect to localhost:8080 (127.0.0.1). - connect (111 Connection refused)
> 
> However:
> 
> Run Testcase (23/29) test-apt-method-https
> Building package: simple-package
> Building package: conflicting-package-one
> Test for successful execution of apt-get update … PASS
> Test that package(s) are not installed with rpm -q simple-package … PASS
> Test for successful execution of apt-get install simple-package … PASS
> Test that package(s) are installed with rpm -q simple-package … PASS
> Pinning invalid key in apt
> Test for failure in execution of apt-get update … PASS
> Test that package(s) are not installed with rpm -q conflicting-package-one … PASS
> Test for failure in execution of apt-get install conflicting-package-one … PASS
> Test that package(s) are not installed with rpm -q conflicting-package-one … PASS

Apparently, this fix is insufficient, I experience sporadic failures
in these nginx-based tests.  Something racy is still there.


-- 
ldv

Re: [devel] [APT PATCH] test/integration/framework/test-apt-method-http*: use nginx as a forking daemon

[Ivan Zakharyaschev]

[-- Attachment #1: Type: text/plain, Size: 1852 bytes --]

On Tue, 18 Feb 2020, Dmitry V. Levin wrote:

> On Tue, Feb 18, 2020 at 06:39:01AM +0300, Ivan Zakharyaschev wrote:
> > This is needed not to send requests to nginx before it is ready to
> > listen: when the main process forks and exits, nginx is considered to
> > be ready.
> > 
> > Without this change, I observed a failure in the http test, but not
> > the https one, probably, because the delay in the https test was
> > larger due to an extra package being built:
> > 
> > Run Testcase (22/29) test-apt-method-http
> > Building package: simple-package
> > Test for successful execution of apt-get update …
> > Err http://localhost x86_64 release
> >   Could not connect to localhost:8080 (127.0.0.1). - connect (111 Connection refused)
> > 
> > However:
> > 
> > Run Testcase (23/29) test-apt-method-https
> > Building package: simple-package
> > Building package: conflicting-package-one
> > Test for successful execution of apt-get update … PASS
> > Test that package(s) are not installed with rpm -q simple-package … PASS
> > Test for successful execution of apt-get install simple-package … PASS
> > Test that package(s) are installed with rpm -q simple-package … PASS
> > Pinning invalid key in apt
> > Test for failure in execution of apt-get update … PASS
> > Test that package(s) are not installed with rpm -q conflicting-package-one … PASS
> > Test for failure in execution of apt-get install conflicting-package-one … PASS
> > Test that package(s) are not installed with rpm -q conflicting-package-one … PASS
> 
> Apparently, this fix is insufficient, I experience sporadic failures
> in these nginx-based tests.  Something racy is still there.

It's a bit difficult for me to help to find out the reason, because I 
don't know how to reproduce any other failures than those fixed by my 
patch...

-- 
Best regards,
Ivan

Re: [devel] [APT PATCH] test/integration/framework/test-apt-method-http*: use nginx as a forking daemon

[Dmitry V. Levin]

On Tue, Feb 18, 2020 at 05:12:56PM +0300, Ivan Zakharyaschev wrote:
> On Tue, 18 Feb 2020, Dmitry V. Levin wrote:
> > On Tue, Feb 18, 2020 at 06:39:01AM +0300, Ivan Zakharyaschev wrote:
> > > This is needed not to send requests to nginx before it is ready to
> > > listen: when the main process forks and exits, nginx is considered to
> > > be ready.
> > > 
> > > Without this change, I observed a failure in the http test, but not
> > > the https one, probably, because the delay in the https test was
> > > larger due to an extra package being built:
> > > 
> > > Run Testcase (22/29) test-apt-method-http
> > > Building package: simple-package
> > > Test for successful execution of apt-get update …
> > > Err http://localhost x86_64 release
> > >   Could not connect to localhost:8080 (127.0.0.1). - connect (111 Connection refused)
> > > 
> > > However:
> > > 
> > > Run Testcase (23/29) test-apt-method-https
> > > Building package: simple-package
> > > Building package: conflicting-package-one
> > > Test for successful execution of apt-get update … PASS
> > > Test that package(s) are not installed with rpm -q simple-package … PASS
> > > Test for successful execution of apt-get install simple-package … PASS
> > > Test that package(s) are installed with rpm -q simple-package … PASS
> > > Pinning invalid key in apt
> > > Test for failure in execution of apt-get update … PASS
> > > Test that package(s) are not installed with rpm -q conflicting-package-one … PASS
> > > Test for failure in execution of apt-get install conflicting-package-one … PASS
> > > Test that package(s) are not installed with rpm -q conflicting-package-one … PASS
> > 
> > Apparently, this fix is insufficient, I experience sporadic failures
> > in these nginx-based tests.  Something racy is still there.
> 
> It's a bit difficult for me to help to find out the reason, because I 
> don't know how to reproduce any other failures than those fixed by my 
> patch...

It's extremely difficult to go any further while these tests are so flaky.
I'm postponing apt patches processing until the test suite is thoroughly fixed.


-- 
ldv

Re: [devel] [APT PATCH] test/integration/framework/test-apt-method-http*: use nginx as a forking daemon

[Aleksei Nikiforov]

18.02.2020 17:28, Dmitry V. Levin пишет:
> On Tue, Feb 18, 2020 at 05:12:56PM +0300, Ivan Zakharyaschev wrote:
>> On Tue, 18 Feb 2020, Dmitry V. Levin wrote:
>>> On Tue, Feb 18, 2020 at 06:39:01AM +0300, Ivan Zakharyaschev wrote:
>>>> This is needed not to send requests to nginx before it is ready to
>>>> listen: when the main process forks and exits, nginx is considered to
>>>> be ready.
>>>>
>>>> Without this change, I observed a failure in the http test, but not
>>>> the https one, probably, because the delay in the https test was
>>>> larger due to an extra package being built:
>>>>
>>>> Run Testcase (22/29) test-apt-method-http
>>>> Building package: simple-package
>>>> Test for successful execution of apt-get update …
>>>> Err http://localhost x86_64 release
>>>>    Could not connect to localhost:8080 (127.0.0.1). - connect (111 Connection refused)
>>>>
>>>> However:
>>>>
>>>> Run Testcase (23/29) test-apt-method-https
>>>> Building package: simple-package
>>>> Building package: conflicting-package-one
>>>> Test for successful execution of apt-get update … PASS
>>>> Test that package(s) are not installed with rpm -q simple-package … PASS
>>>> Test for successful execution of apt-get install simple-package … PASS
>>>> Test that package(s) are installed with rpm -q simple-package … PASS
>>>> Pinning invalid key in apt
>>>> Test for failure in execution of apt-get update … PASS
>>>> Test that package(s) are not installed with rpm -q conflicting-package-one … PASS
>>>> Test for failure in execution of apt-get install conflicting-package-one … PASS
>>>> Test that package(s) are not installed with rpm -q conflicting-package-one … PASS
>>>
>>> Apparently, this fix is insufficient, I experience sporadic failures
>>> in these nginx-based tests.  Something racy is still there.
>>
>> It's a bit difficult for me to help to find out the reason, because I
>> don't know how to reproduce any other failures than those fixed by my
>> patch...
> 
> It's extremely difficult to go any further while these tests are so flaky.
> I'm postponing apt patches processing until the test suite is thoroughly fixed.
> 
> 

Is it extremely difficult to disable 3 tests until better fix is 
implemented? Or just not pick them up since they are presented in 
separate commit? I guess it's just an extremely convenient excuse for 
lazyness.

Re: [devel] [APT PATCH] test/integration/framework/test-apt-method-http*: use nginx as a forking daemon

[Dmitry V. Levin]

On Tue, Feb 18, 2020 at 05:47:06PM +0300, Aleksei Nikiforov wrote:
> 18.02.2020 17:28, Dmitry V. Levin пишет:
> > On Tue, Feb 18, 2020 at 05:12:56PM +0300, Ivan Zakharyaschev wrote:
> >> On Tue, 18 Feb 2020, Dmitry V. Levin wrote:
> >>> On Tue, Feb 18, 2020 at 06:39:01AM +0300, Ivan Zakharyaschev wrote:
> >>>> This is needed not to send requests to nginx before it is ready to
> >>>> listen: when the main process forks and exits, nginx is considered to
> >>>> be ready.
> >>>>
> >>>> Without this change, I observed a failure in the http test, but not
> >>>> the https one, probably, because the delay in the https test was
> >>>> larger due to an extra package being built:
> >>>>
> >>>> Run Testcase (22/29) test-apt-method-http
> >>>> Building package: simple-package
> >>>> Test for successful execution of apt-get update …
> >>>> Err http://localhost x86_64 release
> >>>>    Could not connect to localhost:8080 (127.0.0.1). - connect (111 Connection refused)
> >>>>
> >>>> However:
> >>>>
> >>>> Run Testcase (23/29) test-apt-method-https
> >>>> Building package: simple-package
> >>>> Building package: conflicting-package-one
> >>>> Test for successful execution of apt-get update … PASS
> >>>> Test that package(s) are not installed with rpm -q simple-package … PASS
> >>>> Test for successful execution of apt-get install simple-package … PASS
> >>>> Test that package(s) are installed with rpm -q simple-package … PASS
> >>>> Pinning invalid key in apt
> >>>> Test for failure in execution of apt-get update … PASS
> >>>> Test that package(s) are not installed with rpm -q conflicting-package-one … PASS
> >>>> Test for failure in execution of apt-get install conflicting-package-one … PASS
> >>>> Test that package(s) are not installed with rpm -q conflicting-package-one … PASS
> >>>
> >>> Apparently, this fix is insufficient, I experience sporadic failures
> >>> in these nginx-based tests.  Something racy is still there.
> >>
> >> It's a bit difficult for me to help to find out the reason, because I
> >> don't know how to reproduce any other failures than those fixed by my
> >> patch...
> > 
> > It's extremely difficult to go any further while these tests are so flaky.
> > I'm postponing apt patches processing until the test suite is thoroughly fixed.
> 
> Is it extremely difficult to disable 3 tests until better fix is 
> implemented? Or just not pick them up since they are presented in 
> separate commit? I guess it's just an extremely convenient excuse for 
> lazyness.

It's extremely easy to call people names instead of fixing your own bugs.


-- 
ldv