From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on sa.local.altlinux.org X-Spam-Level: X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00 autolearn=unavailable autolearn_force=no version=3.4.1 To: Ivan Zakharyaschev References: From: Aleksei Nikiforov Message-ID: <614161b6-33a0-2acb-3aa3-ad785616661a@altlinux.org> Date: Tue, 18 Feb 2020 16:17:11 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: ru Content-Transfer-Encoding: 8bit Cc: devel@lists.altlinux.org, ldv@altlinux.org Subject: Re: [devel] [APT PATCH] test/integration/framework/test-apt-method-http*: use nginx as a forking daemon X-BeenThere: devel@lists.altlinux.org X-Mailman-Version: 2.1.12 Precedence: list Reply-To: ALT Linux Team development discussions List-Id: ALT Linux Team development discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 18 Feb 2020 13:17:16 -0000 Archived-At: List-Archive: List-Post: 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. >