[SSSD] [sssd PR#956][comment] tests: fix race confition in enumeration tests
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#956][comment] tests: fix race confition in enumeration tests
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#956][comment] tests: fix race confition in enumeration tests
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#956][comment] tests: fix race confition in enumeration tests
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#956][comment] tests: fix race confition in enumeration tests
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#956][comment] tests: fix race confition in enumeration tests
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#956][comment] tests: fix race confition in enumeration tests
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#956][comment] tests: fix race confition in enumeration tests
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#956][comment] tests: fix race confition in enumeration tests
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#956][comment] tests: fix race confition in enumeration tests
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#956][comment] tests: fix race confition in enumeration tests
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#956][comment] tests: fix race confition in enumeration tests
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#956][comment] tests: fix race confition in enumeration tests
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#956][comment] tests: fix race confition in enumeration tests
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#956][comment] tests: fix race confition in enumeration tests
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#956][comment] tests: fix race confition in enumeration tests
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
