[PATCH] Better cleanup in TLS tests for -13beta2

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Better cleanup in TLS tests for -13beta2

Felix Lechner
Hi,

This patch removes two temporary files that are not removed. In
Debian, repeated builds fail. We do not allow builds from modified
sources.

The first file was clearly an oversight. It was created separately. I
am not sure why the loop over @keys did not remove the second.

For the record, the error message from Debian is below. Lines 25-27
show the files that were left behind.

Kind regards,
Felix Lechner

lechner@4bba56c5a8a8:~/postgresql$ debuild
 dpkg-buildpackage -us -uc -ui
dpkg-buildpackage: info: source package postgresql-13
dpkg-buildpackage: info: source version 13~beta2-1
dpkg-buildpackage: info: source distribution experimental
dpkg-buildpackage: info: source changed by Christoph Berg <[hidden email]>
 dpkg-source --before-build .
dpkg-buildpackage: info: host architecture amd64
 fakeroot debian/rules clean
dh clean
   debian/rules override_dh_auto_clean
make[1]: Entering directory '/home/lechner/postgresql'
rm -rf build
make[1]: Leaving directory '/home/lechner/postgresql'
   dh_autoreconf_clean
   dh_clean
 dpkg-source -b .
dpkg-source: info: using source format '3.0 (quilt)'
dpkg-source: info: building postgresql-13 using existing
./postgresql-13_13~beta2.orig.tar.bz2
dpkg-source: info: using patch list from debian/patches/series
dpkg-source: warning: ignoring deletion of file configure, use
--include-removal to override
dpkg-source: warning: ignoring deletion of file
src/include/pg_config.h.in, use --include-removal to override
dpkg-source: warning: ignoring deletion of file
doc/src/sgml/man-stamp, use --include-removal to override
dpkg-source: warning: ignoring deletion of file
doc/src/sgml/html-stamp, use --include-removal to override
dpkg-source: info: local changes detected, the modified files are:
 postgresql/src/test/ssl/ssl/client_tmp.key
 postgresql/src/test/ssl/ssl/client_wrongperms_tmp.key
dpkg-source: error: aborting due to unexpected upstream changes, see
/tmp/postgresql-13_13~beta2-1.diff.gy3ajb
dpkg-source: info: you can integrate the local changes with dpkg-source --commit
dpkg-buildpackage: error: dpkg-source -b . subprocess returned exit status 2
debuild: fatal error at line 1182:
dpkg-buildpackage -us -uc -ui failed

cleanup-certificates.patch (422 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Better cleanup in TLS tests for -13beta2

Daniel Gustafsson
> On 29 Jun 2020, at 17:52, Felix Lechner <[hidden email]> wrote:

> This patch removes two temporary files that are not removed. In
> Debian, repeated builds fail. We do not allow builds from modified
> sources.

Aha, nice catch!

> The first file was clearly an oversight. It was created separately. I
> am not sure why the loop over @keys did not remove the second.

That's because it's created in the 002_scram.pl testsuite as well, but not
cleaned up there.

The proposed patch admittedly seems a bit like a hack, and the client_tmo.key
handling is wrong as mentioned above.  I propose that we instead add the key to
the @keys array and have clean up handle all files uniformly.  The attached
fixes both of the files.

cheers ./daniel





tls_tests_cleanup.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Better cleanup in TLS tests for -13beta2

Tom Lane-2
Daniel Gustafsson <[hidden email]> writes:
> The proposed patch admittedly seems a bit like a hack, and the client_tmo.key
> handling is wrong as mentioned above.  I propose that we instead add the key to
> the @keys array and have clean up handle all files uniformly.  The attached
> fixes both of the files.

Hmm ... so I guess my reaction to both of these is "what guarantees
that we get to the part of the script that does the unlinks?".
I've certainly seen lots of TAP tests fail to complete.  Could we
do the cleanup in an END block or the like?  (I'm a poor enough
Perl programmer to be uncertain what's the best way, but I know
Perl has constructs like that.)

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Better cleanup in TLS tests for -13beta2

Daniel Gustafsson
> On 29 Jun 2020, at 21:27, Tom Lane <[hidden email]> wrote:

> Hmm ... so I guess my reaction to both of these is "what guarantees
> that we get to the part of the script that does the unlinks?".
> I've certainly seen lots of TAP tests fail to complete.  Could we
> do the cleanup in an END block or the like?  (I'm a poor enough
> Perl programmer to be uncertain what's the best way, but I know
> Perl has constructs like that.)

If execution calls die() during testing, then we wont reach the clean up
portion at the end but we would if we did it as part of END which is (unless my
memory is too fogged) guaranteed to be the last code to run before the
interpreter exits.

That being said, we do retain temporary files on such failures on purpose in
our TestLib since 88802e068017bee8cea7a5502a712794e761c7b5 and a few follow-up
commits since, should these be handled differently?  They are admittedly less
"unknown" as compared to other files as they are copies, but famous last words
have been spoken about bugs that can never happen.

cheers ./daniel

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Better cleanup in TLS tests for -13beta2

Tom Lane-2
Daniel Gustafsson <[hidden email]> writes:
>> On 29 Jun 2020, at 21:27, Tom Lane <[hidden email]> wrote:
>> Hmm ... so I guess my reaction to both of these is "what guarantees
>> that we get to the part of the script that does the unlinks?".

> That being said, we do retain temporary files on such failures on purpose in
> our TestLib since 88802e068017bee8cea7a5502a712794e761c7b5 and a few follow-up
> commits since, should these be handled differently?  They are admittedly less
> "unknown" as compared to other files as they are copies, but famous last words
> have been spoken about bugs that can never happen.

Oh, good point.  Objection withdrawn.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Better cleanup in TLS tests for -13beta2

Michael Paquier-2
On Mon, Jun 29, 2020 at 03:51:48PM -0400, Tom Lane wrote:
> Daniel Gustafsson <[hidden email]> writes:
>> That being said, we do retain temporary files on such failures on purpose in
>> our TestLib since 88802e068017bee8cea7a5502a712794e761c7b5 and a few follow-up
>> commits since, should these be handled differently?  They are admittedly less
>> "unknown" as compared to other files as they are copies, but famous last words
>> have been spoken about bugs that can never happen.
>
> Oh, good point.  Objection withdrawn.

I looked at the patch, and can confirm that client_wrongperms_tmp.key
remains around after running 001_ssltests.pl, and client_tmp.key after
running 002_scram.pl.  The way the patch does its cleanup looks fine
to me, so I'll apply and backpatch where necessary, if there are no
objections of course.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Better cleanup in TLS tests for -13beta2

Michael Paquier-2
On Tue, Jun 30, 2020 at 01:13:39PM +0900, Michael Paquier wrote:
> I looked at the patch, and can confirm that client_wrongperms_tmp.key
> remains around after running 001_ssltests.pl, and client_tmp.key after
> running 002_scram.pl.  The way the patch does its cleanup looks fine
> to me, so I'll apply and backpatch where necessary, if there are no
> objections of course.

I found one problem when testing with parallel jobs once we apply this
patch (say PROVE_FLAGS="-j 4"): the tests of 001 and 002 had the idea
to use the same file name client_tmp.key, so it was possible to easily
fail the tests if for example 002 removes the temporary client key
copy that 001 needs, or vice-versa.  001 takes longer than 002, so the
removal would likely be done by the latter, not the former.  And it
was even logically possible to fail in the case where 001 removes the
file and 002 needs it, though very unlikely because 002 needs this
file for a very short amount of time and one test case.  I have fixed
this issue by just making 002 use a different file name, as we do in
001 for the case of the wrong permissions, and applied the patch down
to 13.
--
Michael

signature.asc (849 bytes) Download Attachment