On (04/12/15 12:11), Jakub Hrozek wrote:
On Fri, Dec 04, 2015 at 11:42:00AM +0100, Lukas Slebodnik wrote:
> On (03/12/15 20:22), Jakub Hrozek wrote:
> >On Wed, Dec 02, 2015 at 05:59:07PM +0100, Lukas Slebodnik wrote:
> >> On (02/12/15 17:10), Michal Židek wrote:
> >> >Hi!
> >> >
> >> >I saw some integration tests failures recently,
> >> >and I think there is a race condition between the
> >> >enumeration refresh timeout and the sleeps
> >> >after some operations that wait for this timeout.
> >> >SSSD fails to populate changes from LDAP in time
> >> >and some asserts can fail because of this.
> >> >
> >> >So far I saw 4 tests to fail like this, which
> >> >is already quite a lot.
> >> >
> >> >The attached patch modifies the timeout values
> >> >and hopefully removes the issue.
> >> >
> >> >Michal
> >>
> >> >From b724db15ce0c1593cfdd7b4da8e0c39e97942e8c Mon Sep 17 00:00:00 2001
> >> >From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com>
> >> >Date: Wed, 2 Dec 2015 16:44:48 +0100
> >> >Subject: [PATCH] ldap_test.py: Modify enum cache timeouts
> >> >
> >> >There is a race condation between ldap
> >> >enumeration refresh timeout and the sleeps
> >> >that wait for the ldap changes to populate
> >> >to SSSD if the timeout and the sleeps have
> >> >the same value.
> >> >---
> >> > src/tests/intg/ldap_test.py | 30 +++++++++++++++++-------------
> >> > 1 file changed, 17 insertions(+), 13 deletions(-)
> >> >
> >> >diff --git a/src/tests/intg/ldap_test.py b/src/tests/intg/ldap_test.py
> >> >index 757ee20..8ec8dbe 100644
> >> >--- a/src/tests/intg/ldap_test.py
> >> >+++ b/src/tests/intg/ldap_test.py
> >> >@@ -33,7 +33,11 @@ import ldap_ent
> >> > from util import *
> >> >
> >> > LDAP_BASE_DN = "dc=example,dc=com"
> >> >-INTERACTIVE_TIMEOUT = 4
> >> >+INTERACTIVE_TIMEOUT = 2
> >> >+
> >> >+
> >> >+def wait_for_ldap_enum_refresh():
> >> >+ time.sleep(INTERACTIVE_TIMEOUT + 4)
> >> Why does it need to be INTERACTIVE_TIMEOUT + 4
> >>
> >> Could it be INTERACTIVE_TIMEOUT + 3 or + 5
> >>
> >
> >Regardless of the value we choose, can we move this patch forward? I see
> >the related failure quite often in SSSD.
> Adding timeout without real explanation is not a solution.
>
> The main problem is with empiric values.
> If they are very high then test are slow.
> And there still can be slow/fast machine where lower values caused troubles.
>
> The ideal solution would be to get rid of enumeration
> in ldap tests.
Enumeration is a codepath that is different from non-enumeration, so it
should be tested. Not as priority, not as the only ldap tests, but it's
a valid case, so it should be there.
> If we want to test enumeration than it should be in separate
> test.
Maybe, but we do have enumeration tests and we should fix them.
Adding sleep is not a fix. It's just a workaround
because all sleep timeout are just an empiric values.
and we should fix test and not adding workaround/hacks.
If we cannot fix the test and don't want te rewrite test without enumeration
then we should remove/revert problematic tests.
LS