[pynfs RFC PATCH] nfs4.0/testserver.py: don't return an error when tests fail

[pynfs RFC PATCH] nfs4.0/testserver.py: don't return an error when tests fail

[Jeff Layton]

This script was originally changed in eb3ba0b60055 ("Have testserver.py
have non-zero exit code if any tests fail"), but the same change wasn't
made to the 4.1 testserver.py script.

There also wasn't much explanation for it, and it makes it difficult to
tell whether the test harness itself failed, or whether there was a
failure in a requested test.

Stop the 4.0 testserver.py from exiting with an error code when a test
fails, so that a successful return means only that the test harness
itself worked, not that every requested test passed.

Cc: Frank Filz <ffilzlnx@mindspring.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 nfs4.0/testserver.py | 2 --
 1 file changed, 2 deletions(-)

I'm not sure about this one. I've worked around this in kdevops for now,
but it would really be preferable if it worked this way, imo. If this
isn't acceptable, maybe we can add a new option that enables this
behavior?

Frank, what was the original rationale for eb3ba0b60055 ?

diff --git a/nfs4.0/testserver.py b/nfs4.0/testserver.py
index f2c41568e5c7..4f4286daa657 100755
--- a/nfs4.0/testserver.py
+++ b/nfs4.0/testserver.py
@@ -387,8 +387,6 @@ def main():
 
     if nfail < 0:
         sys.exit(3)
-    if nfail > 0:
-        sys.exit(2)
 
 if __name__ == "__main__":
     main()
-- 
2.39.2

Re: [pynfs RFC PATCH] nfs4.0/testserver.py: don't return an error when tests fail

[J. Bruce Fields]

I orginally thought I'd continue maintaining pynfs on a volunteer basis,
but I haven't been.  These all look like reasonable changes, but someone
else probably needs to step in to make sure they're handled in a
reasonable amount of time.

--b.

On Wed, Feb 22, 2023 at 08:49:52AM -0500, Jeff Layton wrote:
> This script was originally changed in eb3ba0b60055 ("Have testserver.py
> have non-zero exit code if any tests fail"), but the same change wasn't
> made to the 4.1 testserver.py script.
> 
> There also wasn't much explanation for it, and it makes it difficult to
> tell whether the test harness itself failed, or whether there was a
> failure in a requested test.
> 
> Stop the 4.0 testserver.py from exiting with an error code when a test
> fails, so that a successful return means only that the test harness
> itself worked, not that every requested test passed.
> 
> Cc: Frank Filz <ffilzlnx@mindspring.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  nfs4.0/testserver.py | 2 --
>  1 file changed, 2 deletions(-)
> 
> I'm not sure about this one. I've worked around this in kdevops for now,
> but it would really be preferable if it worked this way, imo. If this
> isn't acceptable, maybe we can add a new option that enables this
> behavior?
> 
> Frank, what was the original rationale for eb3ba0b60055 ?
> 
> diff --git a/nfs4.0/testserver.py b/nfs4.0/testserver.py
> index f2c41568e5c7..4f4286daa657 100755
> --- a/nfs4.0/testserver.py
> +++ b/nfs4.0/testserver.py
> @@ -387,8 +387,6 @@ def main():
>  
>      if nfail < 0:
>          sys.exit(3)
> -    if nfail > 0:
> -        sys.exit(2)
>  
>  if __name__ == "__main__":
>      main()
> -- 
> 2.39.2

Re: [pynfs RFC PATCH] nfs4.0/testserver.py: don't return an error when tests fail

[Mkrtchyan, Tigran]

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


----- Original Message -----
> From: "J. Bruce Fields" <bfields@fieldses.org>
> To: "Jeff Layton" <jlayton@kernel.org>
> Cc: "Dai Ngo" <dai.ngo@oracle.com>, "linux-nfs" <linux-nfs@vger.kernel.org>, "Frank Filz" <ffilzlnx@mindspring.com>
> Sent: Thursday, 23 February, 2023 16:11:32
> Subject: Re: [pynfs RFC PATCH] nfs4.0/testserver.py: don't return an error when tests fail

> I orginally thought I'd continue maintaining pynfs on a volunteer basis,
> but I haven't been.  These all look like reasonable changes, but someone
> else probably needs to step in to make sure they're handled in a
> reasonable amount of time.
> 

Well, I already have a fork in github that is used by others. Thus I can try to
pick the patches from the mailing list and try to keep the tree up-to-date. 

Tigran.
> --b.
> 
> On Wed, Feb 22, 2023 at 08:49:52AM -0500, Jeff Layton wrote:
>> This script was originally changed in eb3ba0b60055 ("Have testserver.py
>> have non-zero exit code if any tests fail"), but the same change wasn't
>> made to the 4.1 testserver.py script.
>> 
>> There also wasn't much explanation for it, and it makes it difficult to
>> tell whether the test harness itself failed, or whether there was a
>> failure in a requested test.
>> 
>> Stop the 4.0 testserver.py from exiting with an error code when a test
>> fails, so that a successful return means only that the test harness
>> itself worked, not that every requested test passed.
>> 
>> Cc: Frank Filz <ffilzlnx@mindspring.com>
>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>> ---
>>  nfs4.0/testserver.py | 2 --
>>  1 file changed, 2 deletions(-)
>> 
>> I'm not sure about this one. I've worked around this in kdevops for now,
>> but it would really be preferable if it worked this way, imo. If this
>> isn't acceptable, maybe we can add a new option that enables this
>> behavior?
>> 
>> Frank, what was the original rationale for eb3ba0b60055 ?
>> 
>> diff --git a/nfs4.0/testserver.py b/nfs4.0/testserver.py
>> index f2c41568e5c7..4f4286daa657 100755
>> --- a/nfs4.0/testserver.py
>> +++ b/nfs4.0/testserver.py
>> @@ -387,8 +387,6 @@ def main():
>>  
>>      if nfail < 0:
>>          sys.exit(3)
>> -    if nfail > 0:
>> -        sys.exit(2)
>>  
>>  if __name__ == "__main__":
>>      main()
>> --
> > 2.39.2

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 2208 bytes --]

Re: [pynfs RFC PATCH] nfs4.0/testserver.py: don't return an error when tests fail

[Calum Mackay]

[-- Attachment #1.1: Type: text/plain, Size: 2658 bytes --]

On 25/02/2023 11:45 am, Mkrtchyan, Tigran wrote:
> 
> ----- Original Message -----
>> From: "J. Bruce Fields" <bfields@fieldses.org>
>> To: "Jeff Layton" <jlayton@kernel.org>
>> Cc: "Dai Ngo" <dai.ngo@oracle.com>, "linux-nfs" <linux-nfs@vger.kernel.org>, "Frank Filz" <ffilzlnx@mindspring.com>
>> Sent: Thursday, 23 February, 2023 16:11:32
>> Subject: Re: [pynfs RFC PATCH] nfs4.0/testserver.py: don't return an error when tests fail
> 
>> I orginally thought I'd continue maintaining pynfs on a volunteer basis,
>> but I haven't been.  These all look like reasonable changes, but someone
>> else probably needs to step in to make sure they're handled in a
>> reasonable amount of time.
>>
> 
> Well, I already have a fork in github that is used by others. Thus I can try to
> pick the patches from the mailing list and try to keep the tree up-to-date.

hi Tigran, I was going to take it over from Bruce, unless you'd prefer 
to, which is fine?

cheers,
calum.

> 
> Tigran.
>> --b.
>>
>> On Wed, Feb 22, 2023 at 08:49:52AM -0500, Jeff Layton wrote:
>>> This script was originally changed in eb3ba0b60055 ("Have testserver.py
>>> have non-zero exit code if any tests fail"), but the same change wasn't
>>> made to the 4.1 testserver.py script.
>>>
>>> There also wasn't much explanation for it, and it makes it difficult to
>>> tell whether the test harness itself failed, or whether there was a
>>> failure in a requested test.
>>>
>>> Stop the 4.0 testserver.py from exiting with an error code when a test
>>> fails, so that a successful return means only that the test harness
>>> itself worked, not that every requested test passed.
>>>
>>> Cc: Frank Filz <ffilzlnx@mindspring.com>
>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>> ---
>>>   nfs4.0/testserver.py | 2 --
>>>   1 file changed, 2 deletions(-)
>>>
>>> I'm not sure about this one. I've worked around this in kdevops for now,
>>> but it would really be preferable if it worked this way, imo. If this
>>> isn't acceptable, maybe we can add a new option that enables this
>>> behavior?
>>>
>>> Frank, what was the original rationale for eb3ba0b60055 ?
>>>
>>> diff --git a/nfs4.0/testserver.py b/nfs4.0/testserver.py
>>> index f2c41568e5c7..4f4286daa657 100755
>>> --- a/nfs4.0/testserver.py
>>> +++ b/nfs4.0/testserver.py
>>> @@ -387,8 +387,6 @@ def main():
>>>   
>>>       if nfail < 0:
>>>           sys.exit(3)
>>> -    if nfail > 0:
>>> -        sys.exit(2)
>>>   
>>>   if __name__ == "__main__":
>>>       main()
>>> --
>>> 2.39.2

-- 
Calum Mackay
Linux Kernel Engineering
Oracle Linux and Virtualisation

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [pynfs RFC PATCH] nfs4.0/testserver.py: don't return an error when tests fail

[Mkrtchyan, Tigran]

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



----- Original Message -----
> From: "Calum Mackay" <calum.mackay@oracle.com>
> To: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de>, "J. Bruce Fields" <bfields@fieldses.org>
> Cc: "Calum Mackay" <calum.mackay@oracle.com>, "Jeff Layton" <jlayton@kernel.org>, "Dai Ngo" <dai.ngo@oracle.com>,
> "linux-nfs" <linux-nfs@vger.kernel.org>, "Frank Filz" <ffilzlnx@mindspring.com>
> Sent: Saturday, 25 February, 2023 16:57:12
> Subject: Re: [pynfs RFC PATCH] nfs4.0/testserver.py: don't return an error when tests fail

> On 25/02/2023 11:45 am, Mkrtchyan, Tigran wrote:
>> 
>> ----- Original Message -----
>>> From: "J. Bruce Fields" <bfields@fieldses.org>
>>> To: "Jeff Layton" <jlayton@kernel.org>
>>> Cc: "Dai Ngo" <dai.ngo@oracle.com>, "linux-nfs" <linux-nfs@vger.kernel.org>,
>>> "Frank Filz" <ffilzlnx@mindspring.com>
>>> Sent: Thursday, 23 February, 2023 16:11:32
>>> Subject: Re: [pynfs RFC PATCH] nfs4.0/testserver.py: don't return an error when
>>> tests fail
>> 
>>> I orginally thought I'd continue maintaining pynfs on a volunteer basis,
>>> but I haven't been.  These all look like reasonable changes, but someone
>>> else probably needs to step in to make sure they're handled in a
>>> reasonable amount of time.
>>>
>> 
>> Well, I already have a fork in github that is used by others. Thus I can try to
>> pick the patches from the mailing list and try to keep the tree up-to-date.
> 
> hi Tigran, I was going to take it over from Bruce, unless you'd prefer
> to, which is fine?

Hi Calum,

just go ahead!

Thanks for the effort,
   Tigran.

> 
> cheers,
> calum.
> 
>> 
>> Tigran.
>>> --b.
>>>
>>> On Wed, Feb 22, 2023 at 08:49:52AM -0500, Jeff Layton wrote:
>>>> This script was originally changed in eb3ba0b60055 ("Have testserver.py
>>>> have non-zero exit code if any tests fail"), but the same change wasn't
>>>> made to the 4.1 testserver.py script.
>>>>
>>>> There also wasn't much explanation for it, and it makes it difficult to
>>>> tell whether the test harness itself failed, or whether there was a
>>>> failure in a requested test.
>>>>
>>>> Stop the 4.0 testserver.py from exiting with an error code when a test
>>>> fails, so that a successful return means only that the test harness
>>>> itself worked, not that every requested test passed.
>>>>
>>>> Cc: Frank Filz <ffilzlnx@mindspring.com>
>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>>> ---
>>>>   nfs4.0/testserver.py | 2 --
>>>>   1 file changed, 2 deletions(-)
>>>>
>>>> I'm not sure about this one. I've worked around this in kdevops for now,
>>>> but it would really be preferable if it worked this way, imo. If this
>>>> isn't acceptable, maybe we can add a new option that enables this
>>>> behavior?
>>>>
>>>> Frank, what was the original rationale for eb3ba0b60055 ?
>>>>
>>>> diff --git a/nfs4.0/testserver.py b/nfs4.0/testserver.py
>>>> index f2c41568e5c7..4f4286daa657 100755
>>>> --- a/nfs4.0/testserver.py
>>>> +++ b/nfs4.0/testserver.py
>>>> @@ -387,8 +387,6 @@ def main():
>>>>   
>>>>       if nfail < 0:
>>>>           sys.exit(3)
>>>> -    if nfail > 0:
>>>> -        sys.exit(2)
>>>>   
>>>>   if __name__ == "__main__":
>>>>       main()
>>>> --
>>>> 2.39.2
> 
> --
> Calum Mackay
> Linux Kernel Engineering
> Oracle Linux and Virtualisation

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 2208 bytes --]

RE: [pynfs RFC PATCH] nfs4.0/testserver.py: don't return an error when tests fail

[Frank Filz]

> From: Jeff Layton [mailto:jlayton@kernel.org]
 
> This script was originally changed in eb3ba0b60055 ("Have testserver.py
have
> non-zero exit code if any tests fail"), but the same change wasn't made to
the
> 4.1 testserver.py script.
> 
> There also wasn't much explanation for it, and it makes it difficult to
tell
> whether the test harness itself failed, or whether there was a failure in
a
> requested test.
> 
> Stop the 4.0 testserver.py from exiting with an error code when a test
fails, so
> that a successful return means only that the test harness itself worked,
not that
> every requested test passed.
> 
> Cc: Frank Filz <ffilzlnx@mindspring.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  nfs4.0/testserver.py | 2 --
>  1 file changed, 2 deletions(-)
> 
> I'm not sure about this one. I've worked around this in kdevops for now,
but it
> would really be preferable if it worked this way, imo. If this isn't
acceptable,
> maybe we can add a new option that enables this behavior?
> 
> Frank, what was the original rationale for eb3ba0b60055 ?

We needed a way for CI to easily detect failure of pynfs. I'm not sure how
helpful it is since Ganesha does fail some tests...

It might be helpful to have some helpers for CI to use, or an option that
causes pynfs to report in a way that's much easier for CI to determine if
pynfs succeeded or not.

Hmm, one thing that would help is to be able to flag a set of tests that
should not constitute a CI failure (known errors) but we want to keep
running them because of what they exercise, or to more readily detect that
they have been fixed.

> diff --git a/nfs4.0/testserver.py b/nfs4.0/testserver.py index
> f2c41568e5c7..4f4286daa657 100755
> --- a/nfs4.0/testserver.py
> +++ b/nfs4.0/testserver.py
> @@ -387,8 +387,6 @@ def main():
> 
>      if nfail < 0:
>          sys.exit(3)
> -    if nfail > 0:
> -        sys.exit(2)
> 
>  if __name__ == "__main__":
>      main()
> --
> 2.39.2

Re: [pynfs RFC PATCH] nfs4.0/testserver.py: don't return an error when tests fail

[Jeff Layton]

On Thu, 2023-02-23 at 08:22 -0800, Frank Filz wrote:
> > From: Jeff Layton [mailto:jlayton@kernel.org]
>  
> > This script was originally changed in eb3ba0b60055 ("Have
> > testserver.py
> have
> > non-zero exit code if any tests fail"), but the same change wasn't
> > made to
> the
> > 4.1 testserver.py script.
> > 
> > There also wasn't much explanation for it, and it makes it difficult
> > to
> tell
> > whether the test harness itself failed, or whether there was a
> > failure in
> a
> > requested test.
> > 
> > Stop the 4.0 testserver.py from exiting with an error code when a
> > test
> fails, so
> > that a successful return means only that the test harness itself
> > worked,
> not that
> > every requested test passed.
> > 
> > Cc: Frank Filz <ffilzlnx@mindspring.com>
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  nfs4.0/testserver.py | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > I'm not sure about this one. I've worked around this in kdevops for
> > now,
> but it
> > would really be preferable if it worked this way, imo. If this isn't
> acceptable,
> > maybe we can add a new option that enables this behavior?
> > 
> > Frank, what was the original rationale for eb3ba0b60055 ?
> 
> We needed a way for CI to easily detect failure of pynfs. I'm not sure
> how
> helpful it is since Ganesha does fail some tests...
> 
> It might be helpful to have some helpers for CI to use, or an option
> that
> causes pynfs to report in a way that's much easier for CI to determine
> if
> pynfs succeeded or not.
> 

That's exactly the issue I had when working with this for kdevops. It
runs testserver.py via ansible, and when it gets a non-zero exit code,
the run aborts without doing anything further. I have it ignoring the
return code from testserver.py for now, but that's not ideal since I
can't catch actual problems with the test harness.

I have testserver.py output the result to JSON, and then analyze that to
see if anything failed. That also gives us what you were asking for in
your other email -- the ability to filter out known failures. Here's
what I have so far, but I'd like to expand it to highlight other
behavior changes:
 
https://github.com/linux-kdevops/kdevops/blob/master/scripts/workflows/pynfs/check_pynfs_results.py

It may make sense to move that script into pynfs itself.

If there is CI that depends on this behavior, then I'd be interested to
hear how they are dealing with failed tests. Do they just not run the
tests that always fail?

> Hmm, one thing that would help is to be able to flag a set of tests
> that
> should not constitute a CI failure (known errors) but we want to keep
> running them because of what they exercise, or to more readily detect
> that
> they have been fixed.
> 

The right way to do that is the same way that xfstests works. You test
for the conditions being favorable for a test and then just skip it if
they aren't.

> > diff --git a/nfs4.0/testserver.py b/nfs4.0/testserver.py index
> > f2c41568e5c7..4f4286daa657 100755
> > --- a/nfs4.0/testserver.py
> > +++ b/nfs4.0/testserver.py
> > @@ -387,8 +387,6 @@ def main():
> > 
> >      if nfail < 0:
> >          sys.exit(3)
> > -    if nfail > 0:
> > -        sys.exit(2)
> > 
> >  if __name__ == "__main__":
> >      main()
> > --
> > 2.39.2
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [pynfs RFC PATCH] nfs4.0/testserver.py: don't return an error when tests fail

[Mkrtchyan, Tigran]

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



----- Original Message -----
> From: "Jeff Layton" <jlayton@kernel.org>
> To: "Frank Filz" <ffilzlnx@mindspring.com>, "J. Bruce Fields" <bfields@fieldses.org>, "Dai Ngo" <dai.ngo@oracle.com>
> Cc: "linux-nfs" <linux-nfs@vger.kernel.org>
> Sent: Thursday, 23 February, 2023 18:08:19
> Subject: Re: [pynfs RFC PATCH] nfs4.0/testserver.py: don't return an error when tests fail

> On Thu, 2023-02-23 at 08:22 -0800, Frank Filz wrote:
>> > From: Jeff Layton [mailto:jlayton@kernel.org]
>>  
>> > This script was originally changed in eb3ba0b60055 ("Have
>> > testserver.py
>> have
>> > non-zero exit code if any tests fail"), but the same change wasn't
>> > made to
>> the
>> > 4.1 testserver.py script.
>> > 
>> > There also wasn't much explanation for it, and it makes it difficult
>> > to
>> tell
>> > whether the test harness itself failed, or whether there was a
>> > failure in
>> a
>> > requested test.
>> > 
>> > Stop the 4.0 testserver.py from exiting with an error code when a
>> > test
>> fails, so
>> > that a successful return means only that the test harness itself
>> > worked,
>> not that
>> > every requested test passed.
>> > 
>> > Cc: Frank Filz <ffilzlnx@mindspring.com>
>> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
>> > ---
>> >  nfs4.0/testserver.py | 2 --
>> >  1 file changed, 2 deletions(-)
>> > 
>> > I'm not sure about this one. I've worked around this in kdevops for
>> > now,
>> but it
>> > would really be preferable if it worked this way, imo. If this isn't
>> acceptable,
>> > maybe we can add a new option that enables this behavior?
>> > 
>> > Frank, what was the original rationale for eb3ba0b60055 ?
>> 
>> We needed a way for CI to easily detect failure of pynfs. I'm not sure
>> how
>> helpful it is since Ganesha does fail some tests...
>> 
>> It might be helpful to have some helpers for CI to use, or an option
>> that
>> causes pynfs to report in a way that's much easier for CI to determine
>> if
>> pynfs succeeded or not.
>> 
> 
> That's exactly the issue I had when working with this for kdevops. It
> runs testserver.py via ansible, and when it gets a non-zero exit code,
> the run aborts without doing anything further. I have it ignoring the
> return code from testserver.py for now, but that's not ideal since I
> can't catch actual problems with the test harness.
> 
> I have testserver.py output the result to JSON, and then analyze that to
> see if anything failed. That also gives us what you were asking for in
> your other email -- the ability to filter out known failures. Here's
> what I have so far, but I'd like to expand it to highlight other
> behavior changes:
> 
> https://github.com/linux-kdevops/kdevops/blob/master/scripts/workflows/pynfs/check_pynfs_results.py
> 
> It may make sense to move that script into pynfs itself.
> 
> If there is CI that depends on this behavior, then I'd be interested to
> hear how they are dealing with failed tests. Do they just not run the
> tests that always fail?


Same here... The test generates for us xunit report, thus error code is in the
reporting and we always have to run it as:

```
./testserver.py -v --noinit --xml="${WORKSPACE}/xunit-report-v41.xml" ${SUT}:${TEST_PATH} all $NFS41_INCLUDES $NFS41_EXCLUDES_OPTION || true
```

> 
>> Hmm, one thing that would help is to be able to flag a set of tests
>> that
>> should not constitute a CI failure (known errors) but we want to keep
>> running them because of what they exercise, or to more readily detect
>> that
>> they have been fixed.

yeah, an option might do the job.

Tigran.

>> 
> 
> The right way to do that is the same way that xfstests works. You test
> for the conditions being favorable for a test and then just skip it if
> they aren't.
> 
>> > diff --git a/nfs4.0/testserver.py b/nfs4.0/testserver.py index
>> > f2c41568e5c7..4f4286daa657 100755
>> > --- a/nfs4.0/testserver.py
>> > +++ b/nfs4.0/testserver.py
>> > @@ -387,8 +387,6 @@ def main():
>> > 
>> >      if nfail < 0:
>> >          sys.exit(3)
>> > -    if nfail > 0:
>> > -        sys.exit(2)
>> > 
>> >  if __name__ == "__main__":
>> >      main()
>> > --
>> > 2.39.2
>> 
> 
> --
> Jeff Layton <jlayton@kernel.org>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 2208 bytes --]

RE: [pynfs RFC PATCH] nfs4.0/testserver.py: don't return an error when tests fail

[Frank Filz]

> -----Original Message-----
> From: Mkrtchyan, Tigran [mailto:tigran.mkrtchyan@desy.de]
> Sent: Thursday, February 23, 2023 11:38 AM
> To: Jeff Layton <jlayton@kernel.org>
> Cc: Frank Filz <ffilzlnx@mindspring.com>; J. Bruce Fields <bfields@fieldses.org>;
> Dai Ngo <dai.ngo@oracle.com>; linux-nfs <linux-nfs@vger.kernel.org>
> Subject: Re: [pynfs RFC PATCH] nfs4.0/testserver.py: don't return an error when
> tests fail
> 
> 
> 
> ----- Original Message -----
> > From: "Jeff Layton" <jlayton@kernel.org>
> > To: "Frank Filz" <ffilzlnx@mindspring.com>, "J. Bruce Fields"
> <bfields@fieldses.org>, "Dai Ngo" <dai.ngo@oracle.com>
> > Cc: "linux-nfs" <linux-nfs@vger.kernel.org>
> > Sent: Thursday, 23 February, 2023 18:08:19
> > Subject: Re: [pynfs RFC PATCH] nfs4.0/testserver.py: don't return an error
> when tests fail
> 
> > On Thu, 2023-02-23 at 08:22 -0800, Frank Filz wrote:
> >> > From: Jeff Layton [mailto:jlayton@kernel.org]
> >>
> >> > This script was originally changed in eb3ba0b60055 ("Have
> >> > testserver.py
> >> have
> >> > non-zero exit code if any tests fail"), but the same change wasn't
> >> > made to
> >> the
> >> > 4.1 testserver.py script.
> >> >
> >> > There also wasn't much explanation for it, and it makes it difficult
> >> > to
> >> tell
> >> > whether the test harness itself failed, or whether there was a
> >> > failure in
> >> a
> >> > requested test.
> >> >
> >> > Stop the 4.0 testserver.py from exiting with an error code when a
> >> > test
> >> fails, so
> >> > that a successful return means only that the test harness itself
> >> > worked,
> >> not that
> >> > every requested test passed.
> >> >
> >> > Cc: Frank Filz <ffilzlnx@mindspring.com>
> >> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> >> > ---
> >> >  nfs4.0/testserver.py | 2 --
> >> >  1 file changed, 2 deletions(-)
> >> >
> >> > I'm not sure about this one. I've worked around this in kdevops for
> >> > now,
> >> but it
> >> > would really be preferable if it worked this way, imo. If this isn't
> >> acceptable,
> >> > maybe we can add a new option that enables this behavior?
> >> >
> >> > Frank, what was the original rationale for eb3ba0b60055 ?
> >>
> >> We needed a way for CI to easily detect failure of pynfs. I'm not sure
> >> how
> >> helpful it is since Ganesha does fail some tests...
> >>
> >> It might be helpful to have some helpers for CI to use, or an option
> >> that
> >> causes pynfs to report in a way that's much easier for CI to determine
> >> if
> >> pynfs succeeded or not.
> >>
> >
> > That's exactly the issue I had when working with this for kdevops. It
> > runs testserver.py via ansible, and when it gets a non-zero exit code,
> > the run aborts without doing anything further. I have it ignoring the
> > return code from testserver.py for now, but that's not ideal since I
> > can't catch actual problems with the test harness.
> >
> > I have testserver.py output the result to JSON, and then analyze that to
> > see if anything failed. That also gives us what you were asking for in
> > your other email -- the ability to filter out known failures. Here's
> > what I have so far, but I'd like to expand it to highlight other
> > behavior changes:
> >
> > https://github.com/linux-
> kdevops/kdevops/blob/master/scripts/workflows/pynfs/check_pynfs_results.py
> >
> > It may make sense to move that script into pynfs itself.
> >
> > If there is CI that depends on this behavior, then I'd be interested to
> > hear how they are dealing with failed tests. Do they just not run the
> > tests that always fail?
> 
> 
> Same here... The test generates for us xunit report, thus error code is in the
> reporting and we always have to run it as:
> 
> ```
> ./testserver.py -v --noinit --xml="${WORKSPACE}/xunit-report-v41.xml"
> ${SUT}:${TEST_PATH} all $NFS41_INCLUDES $NFS41_EXCLUDES_OPTION ||
> true
> ```

OK, maybe we just need to revert this behavior. Honestly, I'm not sure that the Ganesha CI are in good shape so if this reversion prompts examination of why things aren't working as expected, well, then we just need to revisit those things.

I still would argue against changing the meaning of "all"...

Frank

> >
> >> Hmm, one thing that would help is to be able to flag a set of tests
> >> that
> >> should not constitute a CI failure (known errors) but we want to keep
> >> running them because of what they exercise, or to more readily detect
> >> that
> >> they have been fixed.
> 
> yeah, an option might do the job.
> 
> Tigran.
> 
> >>
> >
> > The right way to do that is the same way that xfstests works. You test
> > for the conditions being favorable for a test and then just skip it if
> > they aren't.
> >
> >> > diff --git a/nfs4.0/testserver.py b/nfs4.0/testserver.py index
> >> > f2c41568e5c7..4f4286daa657 100755
> >> > --- a/nfs4.0/testserver.py
> >> > +++ b/nfs4.0/testserver.py
> >> > @@ -387,8 +387,6 @@ def main():
> >> >
> >> >      if nfail < 0:
> >> >          sys.exit(3)
> >> > -    if nfail > 0:
> >> > -        sys.exit(2)
> >> >
> >> >  if __name__ == "__main__":
> >> >      main()
> >> > --
> >> > 2.39.2
> >>
> >
> > --
> > Jeff Layton <jlayton@kernel.org>