URL: https://github.com/SSSD/sssd/pull/956 Author: pbrezina Title: #956: tests: fix race confition in enumeration tests Action: opened
PR body: """ Resubmitting Fabiano's PR since the tests kept failing for last two years with no reason. This needs to be fixed.
---
INTG: Increase the sleep() time so the changes are reflected on SSSD
Those tests have been failing a lot recently and it does happen becase the time to reflect the changes on SSSD is not enough for the machine where the tests are running.
There's no reasonable explanation in the code why 4 seconds is used as INTERACTIVE_TIMEOUT, neither a reasonable explanation why 2 seconds is used as the time waited in order to have those changes reflected on SSSD (neither in the code nor in the commit messages).
This patch uses the most simple empiric way to determine a better value for this timeout, which was "run the tests a considerable amount of time and check that there were no failures".
So, in order to avoid failures and our tests giving us more reliable information, let's give more time so the changes are reflected on SSSD.
Resolves: https://pagure.io/SSSD/sssd/issue/3463 """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/956/head:pr956 git checkout pr956
URL: https://github.com/SSSD/sssd/pull/956 Title: #956: tests: fix race confition in enumeration tests
mzidek-rh commented: """ I agree with the reasoning for this patch. ACK. """
See the full comment at https://github.com/SSSD/sssd/pull/956#issuecomment-562150939
URL: https://github.com/SSSD/sssd/pull/956 Title: #956: tests: fix race confition in enumeration tests
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/956 Title: #956: tests: fix race confition in enumeration tests
alexey-tikhonov commented: """ Hi @mzidek-rh,
I agree with the reasoning for this patch. ACK.
Do you by any chance have an explanation for the this `time.sleep(INTERACTIVE_TIMEOUT/2)` (replaced with `time.sleep(INTERACTIVE_TIMEOUT)` in this patch)?
Commit message is talking about "the time we should wait in order to have the changes reflected" but that's not the case for this sleep, IMO. """
See the full comment at https://github.com/SSSD/sssd/pull/956#issuecomment-562154615
URL: https://github.com/SSSD/sssd/pull/956 Title: #956: tests: fix race confition in enumeration tests
alexey-tikhonov commented: """ Hi @mzidek-rh,
I agree with the reasoning for this patch. ACK.
Do you by any chance have an explanation for the this `time.sleep(INTERACTIVE_TIMEOUT/2)` (replaced with `time.sleep(INTERACTIVE_TIMEOUT)` in this patch)? What is its purpose?
Commit message is talking about "the time we should wait in order to have the changes reflected" but that's not the case for this sleep, IMO. """
See the full comment at https://github.com/SSSD/sssd/pull/956#issuecomment-562154615
URL: https://github.com/SSSD/sssd/pull/956 Title: #956: tests: fix race confition in enumeration tests
alexey-tikhonov commented: """ Hi @mzidek-rh,
I agree with the reasoning for this patch. ACK.
Do you by any chance have an explanation for the this `time.sleep(INTERACTIVE_TIMEOUT/2)` (replaced with `time.sleep(INTERACTIVE_TIMEOUT)` in this patch)? What is its purpose?
Commit message is talking about "the time we should wait in order to have the changes reflected" but that's not the case for this sleep, IMO.
Or maybe it is, but I do not see it. Those timeouts (changed by this patch) are awaited *before* `ldap_conn.add_s` is called. What is asserted after those timeouts is that `ent` is empty. Probably there is a reason to do so, probably there are leftovers in 'ent' or something like that and tests need to wait previous enumeration to complete to have it clean. But not explanations is really given, IMO. """
See the full comment at https://github.com/SSSD/sssd/pull/956#issuecomment-562154615
URL: https://github.com/SSSD/sssd/pull/956 Title: #956: tests: fix race confition in enumeration tests
alexey-tikhonov commented: """ Hi @mzidek-rh,
I agree with the reasoning for this patch. ACK.
Do you by any chance have an explanation for the this `time.sleep(INTERACTIVE_TIMEOUT/2)` (replaced with `time.sleep(INTERACTIVE_TIMEOUT)` in this patch)? What is its purpose?
Commit message is talking about "the time we should wait in order to have the changes reflected" but that's not the case for this sleep, IMO.
Or maybe it is, but I do not understand it. Those timeouts (changed by this patch) are awaited *before* `ldap_conn.add_s` is called. What is asserted after those timeouts is that `ent` is empty. Probably there is a reason to do so, probably there are leftovers in 'ent' or something like that and tests need to wait previous enumeration to complete to have it clean. But not explanations is really given, IMO. """
See the full comment at https://github.com/SSSD/sssd/pull/956#issuecomment-562154615
URL: https://github.com/SSSD/sssd/pull/956 Title: #956: tests: fix race confition in enumeration tests
alexey-tikhonov commented: """ Hi @mzidek-rh,
I agree with the reasoning for this patch. ACK.
Do you by any chance have an explanation for the this `time.sleep(INTERACTIVE_TIMEOUT/2)` (replaced with `time.sleep(INTERACTIVE_TIMEOUT)` in this patch)? What is its purpose?
Commit message is talking about "the time we should wait in order to have the changes reflected" but that's not the case for this sleep, IMO.
Or maybe it is, but I do not understand it. Those timeouts (changed by this patch) are awaited *before* `ldap_conn.add_s` is called. What is asserted after those timeouts is that `ent` is empty. Probably there is a reason to do so, probably there are leftovers in 'ent' or something like that and tests need to wait previous enumeration to complete to have it clean. But no explanations is really given, IMO. """
See the full comment at https://github.com/SSSD/sssd/pull/956#issuecomment-562154615
URL: https://github.com/SSSD/sssd/pull/956 Title: #956: tests: fix race confition in enumeration tests
alexey-tikhonov commented: """ Hi @mzidek-rh,
I agree with the reasoning for this patch. ACK.
Do you by any chance have an explanation for this `time.sleep(INTERACTIVE_TIMEOUT/2)` (replaced with `time.sleep(INTERACTIVE_TIMEOUT)` in this patch)? What is its purpose?
Commit message is talking about "the time we should wait in order to have the changes reflected" but that's not the case for this sleep, IMO.
Or maybe it is, but I do not understand it. Those timeouts (changed by this patch) are awaited *before* `ldap_conn.add_s` is called. What is asserted after those timeouts is that `ent` is empty. Probably there is a reason to do so, probably there are leftovers in 'ent' or something like that and tests need to wait previous enumeration to complete to have it clean. But no explanations is really given, IMO. """
See the full comment at https://github.com/SSSD/sssd/pull/956#issuecomment-562154615
URL: https://github.com/SSSD/sssd/pull/956 Title: #956: tests: fix race confition in enumeration tests
alexey-tikhonov commented: """ Please, take a note, I do not remove 'accepted' label. What I want is to clarify test design so asking if you understand it. If no, well, I will have to figure it on my own. """
See the full comment at https://github.com/SSSD/sssd/pull/956#issuecomment-562171933
URL: https://github.com/SSSD/sssd/pull/956 Title: #956: tests: fix race confition in enumeration tests
alexey-tikhonov commented: """ For detailed discussion see #947 """
See the full comment at https://github.com/SSSD/sssd/pull/956#issuecomment-562181499
URL: https://github.com/SSSD/sssd/pull/956 Title: #956: tests: fix race confition in enumeration tests
mzidek-rh commented: """ The problem with these tests is that ldap_conn has scope=module and the fixture that fills the ldap contents before the test has scope=function (default), so there can be leftovers from previous test run in the passwd result (enumeration) after the fixture with function scope was executed, but the enumeration results are still cached in SSSD. I really do not know why we decided to go with INTERACTIVE_TIMEOUT/2 and I wonder if it was supposed to be INTERACTIVE_TIMEOUT*2 instead, which would make much more sense to me. However INTERACTIVE_TIMEOUT is probably ok as well, unless something slows SSSD really a lot. Now, of course we can speculate what number would be actually good... if this function fails in the future again, we can change it to INTERACTIVE_TIMEOUT * 2, but again, I do not have good explanation or justification other then "maybe sometimes SSSD will need more time for the cached enumeration result to actualy time out, despite the timeout being INTERACTIVE_TIMEOUT in the sssd.conf".
Also the way we picked the INTERACTIVE_TIMEOUT was kind of "this seems to work" method. IIRC in the first version it was 2, but that turned out to not be enough in some cases and caused the tests to fail "sometimes". """
See the full comment at https://github.com/SSSD/sssd/pull/956#issuecomment-562197555
URL: https://github.com/SSSD/sssd/pull/956 Title: #956: tests: fix race confition in enumeration tests
mzidek-rh commented: """ In case we do clean the cache (rm) before each test run, the problem may be that the changes in LDAP were kind of slow to propagate after the fixture with function scope made them... so we need to wait for some time for the changes to take effect before we start requesting something from LDAP again. """
See the full comment at https://github.com/SSSD/sssd/pull/956#issuecomment-562199694
URL: https://github.com/SSSD/sssd/pull/956 Title: #956: tests: fix race confition in enumeration tests
alexey-tikhonov commented: """
there can be leftovers from previous test run in the passwd result (enumeration) after the fixture with function scope was executed, but the enumeration results are still cached in SSSD.
So this first `sleep(INTERACTIVE_TIMEOUT[/2])` is to ensure enumeration task was run and cleared sysdb cache, right?
But as was explained [here](https://github.com/SSSD/sssd/pull/947#issuecomment-559440211) test needs to have sleep_timeout > enumeration_timeout to ensure enumeration was run.
I really do not know why we decided to go with INTERACTIVE_TIMEOUT/2 and I wonder if it was supposed to be INTERACTIVE_TIMEOUT*2 instead, which would make much more sense to me.
Agree. """
See the full comment at https://github.com/SSSD/sssd/pull/956#issuecomment-562209567
URL: https://github.com/SSSD/sssd/pull/956 Title: #956: tests: fix race confition in enumeration tests
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/956 Title: #956: tests: fix race confition in enumeration tests
mzidek-rh commented: """ In other words, you think it is safer to have INTERACTIVE_TIMEOUT*2 where the previous /2 was? I think it makes sense. As I mentioned previously, I think that this may have been the original intention, even though practically INTERACTIVE_TIMEOUT may be good enough as well, but it is harder to justify.
Removing the accepted label and adding changes requested.
@pbrezina Will you please change the /2 to *2 (instead of just removing the /2) and re-run the tests a few times? """
See the full comment at https://github.com/SSSD/sssd/pull/956#issuecomment-562243930
URL: https://github.com/SSSD/sssd/pull/956 Title: #956: tests: fix race confition in enumeration tests
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/956 Title: #956: tests: fix race confition in enumeration tests
alexey-tikhonov commented: """
In other words, you think it is safer to have INTERACTIVE_TIMEOUT*2 where the previous /2 was?
Actually I liked initial Pavel's approach (INTERACTIVE_TIMEOUT=2*ENUMERATION_TIMEOUT) more. Because we need to have all sleep_timeouts > enumeration_timeout. Not just in the beginnings of a test. What was missing: replacement INTERACTIVE_TIMEOUT/2 with INTERACTIVE_TIMEOUT in the beginnings (this is what Fabiano's patch does). Those patches combined plus your explanation of reason behind first sleep() seems reasonable to me. """
See the full comment at https://github.com/SSSD/sssd/pull/956#issuecomment-562256253
URL: https://github.com/SSSD/sssd/pull/956 Title: #956: tests: fix race confition in enumeration tests
mzidek-rh commented: """ Ok. So, let's go with the INTERACTIVE_TIMEOUT=2*ENUMERATION_TIMEOUT it is then. @pbrezina , can you make the change? """
See the full comment at https://github.com/SSSD/sssd/pull/956#issuecomment-563229817
URL: https://github.com/SSSD/sssd/pull/956 Title: #956: tests: fix race confition in enumeration tests
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/956 Title: #956: tests: fix race confition in enumeration tests
Label: +superseded
URL: https://github.com/SSSD/sssd/pull/956 Title: #956: tests: fix race confition in enumeration tests
mzidek-rh commented: """ Superseeded in favor of #959 """
See the full comment at https://github.com/SSSD/sssd/pull/956#issuecomment-564093224
URL: https://github.com/SSSD/sssd/pull/956 Author: pbrezina Title: #956: tests: fix race confition in enumeration tests Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/956/head:pr956 git checkout pr956
sssd-devel@lists.fedorahosted.org
-
alexey-tikhonov -
mzidek-rh
-
pbrezina