[sssd PR#345][opened] Improve the situation of recurrent failing CI tests

[fidencio]

   URL: https://github.com/SSSD/sssd/pull/345
Author: fidencio
 Title: #345: Improve the situation of recurrent failing CI tests
Action: opened

PR body:
"""
Taking a look in our CI the latest issues we had were related to not waiting enough time
in some sleep() that are around the tests code.

For a more detailed explanation, please, take a look on each patch of this small series.

Last 6 CI runs were successful (all triggered with these patches). There were still 5 runs
to go, but those jobs were aborted without any kind of notice. :-\
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/345/head:pr345
git checkout pr345

[sssd PR#345][comment] Improve the situation of recurrent failing CI tests

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/345
Title: #345: Improve the situation of recurrent failing CI tests

lslebodn commented:
"""
Changing hardcoded timeout without explanation "why" is not sufficient solution.
(because you might hide a bug). And purpose of tests is to find bugs.

Therefer NACK to 1st patch.

NACK to 2nd patch because 15 seconds is enough as it is already explained in commit
message.


"""

See the full comment at https://github.com/SSSD/sssd/pull/345#issuecomment-321232993

[sssd PR#345][+Changes requested] Improve the situation of recurrent failing CI tests

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/345
Title: #345: Improve the situation of recurrent failing CI tests

Label: +Changes requested

[sssd PR#345][comment] Improve the situation of recurrent failing CI tests

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/345
Title: #345: Improve the situation of recurrent failing CI tests

lslebodn commented:
"""
Changing hardcoded timeout without explanation "why" is not sufficient solution.
(because you might hide a bug). And purpose of tests is to find bugs.

Therefore NACK to 1st patch.

NACK to 2nd patch because 15 seconds is enough as it is already explained in commit
message.


"""

See the full comment at https://github.com/SSSD/sssd/pull/345#issuecomment-321232993

[sssd PR#345][comment] Improve the situation of recurrent failing CI tests

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/345
Title: #345: Improve the situation of recurrent failing CI tests

lslebodn commented:
"""
Changing hardcoded timeout without explanation "why" is not sufficient solution.
(because you might hide a bug). And purpose of tests is to find bugs.

Therefore NACK to 1st patch.

NACK to 2nd patch because 15 seconds is enough as it is already explained in the
comment.


"""

See the full comment at https://github.com/SSSD/sssd/pull/345#issuecomment-321232993

[sssd PR#345][comment] Improve the situation of recurrent failing CI tests

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/345
Title: #345: Improve the situation of recurrent failing CI tests

lslebodn commented:
"""
FYI: here is an example why you cannot blindly increase timeout in test.

Lets assume that we have a failing test which should test midpoint refresh for some entry
in cache.
Let say that default cache timeout is 2000. I would expect that entry will be
automatically refreshed after 1000 seconds. But test is failing. Therefore we decide to
double sleep from 1000 -> 2000 and test will pass. However you will not test midpoint
refresh of entry. Because entry as not refreshed bit "midpoint automatic task"
in advance but it was refreshed because it was expired.

And your commit does not explain why it pass on fedora but does not pass on debian. In
another word; it is almost the same as disabling test.

  
"""

See the full comment at https://github.com/SSSD/sssd/pull/345#issuecomment-321237851

[sssd PR#345][comment] Improve the situation of recurrent failing CI tests

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/345
Title: #345: Improve the situation of recurrent failing CI tests

jhrozek commented:
"""
So are the issues because enumeration hasn't ran yet or because we are in the middle
of enumeration when the test checks?

In general I would expect the enumeration code to run each period as specified, but there
might be a bit of processing after the period has concluded and before the enumeration
results are processed.

Also, I wonder if it was more systematic to instead poll for the sysdb changes instead of
waiting, maybe that would make it possible to finish the test sooner? (I forget if we
store the lastUSN in sysdb, but perhaps checking for that could be used).

About the second patch, did we have problems with the 15 seconds timeout at all? I did see
quite a few  issues with smaller timeouts during development, but not with the 15 seconds
one..
"""

See the full comment at https://github.com/SSSD/sssd/pull/345#issuecomment-321237929

[sssd PR#345][comment] Improve the situation of recurrent failing CI tests

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/345
Title: #345: Improve the situation of recurrent failing CI tests

fidencio commented:
"""
@jhrozek: about the 15 seconds timeout, please, take a look at:
http://vm-058-233.$ABC/logs/job/72/75/debian_testing/ci-build-debug/ci-ma...
and http://vm-058-233.$ABC/logs/job/72/59/fedora23/ci-build-debug/ci-make-int...
are some examples of this test failing.
"""

See the full comment at https://github.com/SSSD/sssd/pull/345#issuecomment-321239855

[sssd PR#345][comment] Improve the situation of recurrent failing CI tests

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/345
Title: #345: Improve the situation of recurrent failing CI tests

lslebodn commented:
"""
On (09/08/17 12:24), fidencio wrote:
> @jhrozek: about the 15 seconds timeout, please, take a look at:
> http://vm-058-233.$ABC/logs/job/72/75/debian_testing/ci-build-debug/ci-ma...
> and http://vm-058-233.$ABC/logs/job/72/59/fedora23/ci-build-debug/ci-make-int...
> are some examples of this test failing.
>  
That is not a sufficient justification for increasing sleep in test.
Are you sure it is not a bug in sssd/tevent?

Unfortunately, we cannot see sssd_secrets.log in dir
http://vm-058-233.$ABC/logs/job/72/75/debian_testing/ci-build-debug/intg/...
And anyway; `debug_level` was very likely not increased in
`test_idle_timeout` unless you use extra patch on top of git master.

LS

"""

See the full comment at https://github.com/SSSD/sssd/pull/345#issuecomment-321258216

[sssd PR#345][comment] Improve the situation of recurrent failing CI tests

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/345
Title: #345: Improve the situation of recurrent failing CI tests

fidencio commented:
"""
@lslebodn, I was just pointing to @jhrozek the tests that have been failing (as he
asked).
"""

See the full comment at https://github.com/SSSD/sssd/pull/345#issuecomment-321266231

[sssd PR#345][comment] Improve the situation of recurrent failing CI tests

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/345
Title: #345: Improve the situation of recurrent failing CI tests

fidencio commented:
"""
Anyways, I do not have any motivation to work further in this PR (and that's the
reason I did not assigned the bug to myself).

I'm closing the PR as rejected.
"""

See the full comment at https://github.com/SSSD/sssd/pull/345#issuecomment-321266553

[sssd PR#345][-Changes requested] Improve the situation of recurrent failing CI tests

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/345
Title: #345: Improve the situation of recurrent failing CI tests

Label: -Changes requested

[sssd PR#345][+Rejected] Improve the situation of recurrent failing CI tests

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/345
Title: #345: Improve the situation of recurrent failing CI tests

Label: +Rejected

[sssd PR#345][closed] Improve the situation of recurrent failing CI tests

[fidencio]

   URL: https://github.com/SSSD/sssd/pull/345
Author: fidencio
 Title: #345: Improve the situation of recurrent failing CI tests
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/345/head:pr345
git checkout pr345

[sssd PR#345][comment] Improve the situation of recurrent failing CI tests

[mzidek-rh]

  URL: https://github.com/SSSD/sssd/pull/345
Title: #345: Improve the situation of recurrent failing CI tests

mzidek-rh commented:
"""
I think the fact that the increased timeouts helped stabilize the tests is helpful
information. I did not look into the issue itself yet, but it may help pinpointing the
real cause.

So I am linking this PR to the issue as an attempt to fix it with possible value as
fallback once we learn more about the issue and what is causing it.
"""

See the full comment at https://github.com/SSSD/sssd/pull/345#issuecomment-321305533

[sssd PR#345][comment] Improve the situation of recurrent failing CI tests

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/345
Title: #345: Improve the situation of recurrent failing CI tests

lslebodn commented:
"""
> I think the fact that the increased timeouts helped stabilize the
> tests is helpful information. I did not look into the issue itself yet, but it may help
> pinpointing the real cause.
>  
@mzidek It is not helpful because it does not explain why it failed. And it still might
fail in future. Failures in test will be a HUUUUUUUUUUUGE problem in modularity[1].
Because if test fails then module cannot be released and it would also block rebuilding of
other modules which will depend on sssd (freeIPA)  

I know that sleeps in tests are terrible (they hide bugs) downstream tests use sleep on
many places and therefore didn't catch bug fixed by commit
https://pagure.io/SSSD/sssd/c/d4063e9a21a4e203bee7e0a0144fa8cabb14cc46

So we need to understand why test failed and explain it either in commit message or it
comment.
e.g. https://pagure.io/SSSD/sssd/c/b3074dca3acebd91437ef13d3329d6d65d655215

> Anyways, I do not have any motivation to work further in this PR (and
> that's the reason I did not assigned the bug to myself).
>  
@fidencio  Me neither, I have enough fighting/debbuging downstream tests. But all members
need to be familiar with upstream tests.
 
[1] https://docs.pagure.org/modularity/
"""

See the full comment at https://github.com/SSSD/sssd/pull/345#issuecomment-321314040