[SSSD] [sssd PR#453][comment] Speed up by-ID lookups with the help of the Global Catalog
URL: https://github.com/SSSD/sssd/pull/453 Title: #453: Speed up by-ID lookups with the help of the Global Catalog sumit-bose commented: """ ok, I have no further comments as well, so ACK. Since @pbrezina already gave his ACK I'll set the Accepted flag. """ See the full comment at https://github.com/SSSD/sssd/pull/453#issuecomment-349756756 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#453][comment] Speed up by-ID lookups with the help of the Global Catalog
URL: https://github.com/SSSD/sssd/pull/453 Title: #453: Speed up by-ID lookups with the help of the Global Catalog jhrozek commented: """ On Wed, Dec 06, 2017 at 12:55:22PM +, sumit-bose wrote: > The code works well for me and I didn't run into issues either. I left two > minor comments which do not affect the code, so feel free to ignore them. Hopefully all issues were fixed. I renamed the request in a separate patch, because otherwise I would have to touch several patches just to rename a function.. """ See the full comment at https://github.com/SSSD/sssd/pull/453#issuecomment-349731831 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#453][comment] Speed up by-ID lookups with the help of the Global Catalog
URL: https://github.com/SSSD/sssd/pull/453 Title: #453: Speed up by-ID lookups with the help of the Global Catalog sumit-bose commented: """ The code works well for me and I didn't run into issues either. I left two minor comments which do not affect the code, so feel free to ignore them. """ See the full comment at https://github.com/SSSD/sssd/pull/453#issuecomment-349631553 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#453][comment] Speed up by-ID lookups with the help of the Global Catalog
URL: https://github.com/SSSD/sssd/pull/453 Title: #453: Speed up by-ID lookups with the help of the Global Catalog pbrezina commented: """ Thank you, this code is much better. I ran some test and it seems to be OK. Ack. """ See the full comment at https://github.com/SSSD/sssd/pull/453#issuecomment-349604008 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#453][comment] Speed up by-ID lookups with the help of the Global Catalog
URL: https://github.com/SSSD/sssd/pull/453 Title: #453: Speed up by-ID lookups with the help of the Global Catalog jhrozek commented: """ downstream tests: * AD tests "passed" - 2182302 - there are some child tests that failed, but Lukas tells me he has at least a workaround for the tests in the meantime. Anyway, these failures are known * LDAP-ID-LDAP-AUTH: 2182300: passed * Tests against openldap: 2182299: passed """ See the full comment at https://github.com/SSSD/sssd/pull/453#issuecomment-349428303 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#453][comment] Speed up by-ID lookups with the help of the Global Catalog
URL: https://github.com/SSSD/sssd/pull/453 Title: #453: Speed up by-ID lookups with the help of the Global Catalog jhrozek commented: """ new patches that fix the Coverity warnings were pushed """ See the full comment at https://github.com/SSSD/sssd/pull/453#issuecomment-349283744 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#453][comment] Speed up by-ID lookups with the help of the Global Catalog
URL: https://github.com/SSSD/sssd/pull/453 Title: #453: Speed up by-ID lookups with the help of the Global Catalog jhrozek commented: """ btw Coverity found two issues in the latest patches; I've fixed them both locally and I'm re-running all the tests now.. """ See the full comment at https://github.com/SSSD/sssd/pull/453#issuecomment-349274703 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#453][comment] Speed up by-ID lookups with the help of the Global Catalog
URL: https://github.com/SSSD/sssd/pull/453 Title: #453: Speed up by-ID lookups with the help of the Global Catalog jhrozek commented: """ I pushed new patches. The changes include: - the domain locator logic in `cache_req.c` is now in a separate request as @pbrezina suggested - several fixes in the AD domain locator code, mostly issues found by downstream tests such as that the request didn't disable the GC when it found that no POSIX attributes are present in the Global Catalog These patches more-or-less pass all downstream tests. I say more or less because I still see some intermittent failures in some of the AD-related tests, but I can't put my finger on them. The child join test always fails, but it also always fails without my patches. With my patches, I've also seen an auth failure against a child domain, but only once and I also saw an enumeration test failure, but also only once. I've re-started all the tests again to see if any of them fail again, but for now I think the patches are ready for another round of review! """ See the full comment at https://github.com/SSSD/sssd/pull/453#issuecomment-349098830 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#453][comment] Speed up by-ID lookups with the help of the Global Catalog
URL: https://github.com/SSSD/sssd/pull/453 Title: #453: Speed up by-ID lookups with the help of the Global Catalog pbrezina commented: """ Yes, turning it into a separate request would be better. """ See the full comment at https://github.com/SSSD/sssd/pull/453#issuecomment-348442274 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#453][comment] Speed up by-ID lookups with the help of the Global Catalog
URL: https://github.com/SSSD/sssd/pull/453 Title: #453: Speed up by-ID lookups with the help of the Global Catalog jhrozek commented: """ @pbrezina sorry for the delay, but I was working on other PR reviews but also on fixing some issues these patches had caused in AD downstream tests (new tests are running at the moment). About moving the logic to a separate request..I'm not sure this can be done easily, because you can mix domains that support the locator and those that don't in the cr_domains list, which means that running the locator before the lookups themselves would be a bit wasteful (consider id_provider=files and id_provider=ad, you don't want to search the AD server for every files provider lookup). The other option, which I in fact considered initially was to move the whole logic into cache_req_search, but there you're searching only in the context of a single domain (the one in the cache_req structure), but the locator needs to set negative cache of other domains from the same forest/from the same main sssd domain based on results of the locator. So what can be done is to turn `cache_req_search_domains_locate` into a tevent request, at least then `cache_req_search_locate_cache_done` and `cache_req_search_locate_dp_done` would be moved into this new request. Maybe, if it's not too much of a hack, we could even pass the cr_domain list into this new request so that it can even set the negative cache inside it. Would this look nicer to you? """ See the full comment at https://github.com/SSSD/sssd/pull/453#issuecomment-348170332 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#453][comment] Speed up by-ID lookups with the help of the Global Catalog
URL: https://github.com/SSSD/sssd/pull/453 Title: #453: Speed up by-ID lookups with the help of the Global Catalog jhrozek commented: """ I'll try to work on that during the next couple of days. """ See the full comment at https://github.com/SSSD/sssd/pull/453#issuecomment-346682291 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#453][comment] Speed up by-ID lookups with the help of the Global Catalog
URL: https://github.com/SSSD/sssd/pull/453 Title: #453: Speed up by-ID lookups with the help of the Global Catalog pbrezina commented: """ Is it possible to make the cache req code more straightforward instead of mixing logic of locating domain and normal domain search? I.e. create a request, say `cache_req_locate_domain` which would locate the domain if possible and decide what domain(s) to search. In other words, is it possible for the code to read like this? ```c cache_req_search_domains_send() -> cache_req_locate_domain_send() <- got single domain, or multiple domains -> cache_req_search_domains_next() and proceed as usual ``` """ See the full comment at https://github.com/SSSD/sssd/pull/453#issuecomment-346335962 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#453][comment] Speed up by-ID lookups with the help of the Global Catalog
URL: https://github.com/SSSD/sssd/pull/453 Title: #453: Speed up by-ID lookups with the help of the Global Catalog jhrozek commented: """ The test failures are not so bad after all, but there are some failures in parts of code I touched. but the earliest I will be able to debug the failing tests is Friday. """ See the full comment at https://github.com/SSSD/sssd/pull/453#issuecomment-346172574 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#453][comment] Speed up by-ID lookups with the help of the Global Catalog
URL: https://github.com/SSSD/sssd/pull/453 Title: #453: Speed up by-ID lookups with the help of the Global Catalog jhrozek commented: """ quick test update: All the generic ldap, kerberos and proxy tests succeeded, the AD tests failed in a grandiose way. I'll investigate why later. """ See the full comment at https://github.com/SSSD/sssd/pull/453#issuecomment-345993408 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#453][comment] Speed up by-ID lookups with the help of the Global Catalog
URL: https://github.com/SSSD/sssd/pull/453 Title: #453: Speed up by-ID lookups with the help of the Global Catalog jhrozek commented: """ I fixed the multidomain bug and another bug in the AD locator request which could have caused a loop in an MPG domain. I still haven't ran any downstream tests, though, sorry. """ See the full comment at https://github.com/SSSD/sssd/pull/453#issuecomment-345851714 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#453][comment] Speed up by-ID lookups with the help of the Global Catalog
URL: https://github.com/SSSD/sssd/pull/453 Title: #453: Speed up by-ID lookups with the help of the Global Catalog jhrozek commented: """ Lastly, I realized that get-by-SID requests could theoretically be supported as well, but I also don't think it's urgent. Maybe we should have a ticket for completeness. """ See the full comment at https://github.com/SSSD/sssd/pull/453#issuecomment-34413 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#453][comment] Speed up by-ID lookups with the help of the Global Catalog
URL: https://github.com/SSSD/sssd/pull/453 Title: #453: Speed up by-ID lookups with the help of the Global Catalog jhrozek commented: """ I pushed new version of patches: - MPG domains now work fine with this request - unit tests were added for the negcache additions - I didn't add tests for the DP additions, because the existing cmocka DP tests test the inner workings of DP, not the methods and the methods will be better tested by downstream tests anyway - I also added support for object-by-ID requests along with tests What is still on the todo list (but should not block review in general) - I didn't re-run all the downstream tests with this new iteration of the patches - I found out that there is a bug in a multidomain scenario where the first domain doesn't support the locator, but the second one does (for example, a combination of id_provider=files followed by id_provider=ad). I'm going to fix this promptly. """ See the full comment at https://github.com/SSSD/sssd/pull/453#issuecomment-34344 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
