[devel] [APT PATCH] test/integration/framework/test-apt-method-http*: use nginx as a forking daemon
Aleksei Nikiforov
darktemplar на altlinux.org
Вт Фев 18 16:17:11 MSK 2020
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.
>
Подробная информация о списке рассылки Devel