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

[Aleksei Nikiforov <darktemplar@altlinux.org>]

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.
>