URL: https://github.com/SSSD/sssd/pull/453 Author: jhrozek Title: #453: Speed up by-ID lookups with the help of the Global Catalog Action: opened
PR body: """ These patches implement the RFE requested in https://pagure.io/SSSD/sssd/issue/3468
The design page was sent to sssd-devel for review as: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.o... but so far was not merged.
Please see the design page for explanation of the general flow, but also feel free to point out if more comments should be added to the code so that developers don't have to go back to design pages.
There are still some things I'm working on, but I think at least the cache_req part should be ready for review.
Here's what I think is not finished yet: - I don't think the locate domain request works for MPG domains - unit tests should be added for the negcache API additions as well as the DP interface to not decrease the code coverage - I still haven't made up my mind if we should support also the object-by-ID request. I think it would be nice for completeness but I also think it's not strictly required - I only ran several downstream tests, not all, so I don't know if there are some regressions or not """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/453/head:pr453 git checkout pr453
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-345555344
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-345555413
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
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
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
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
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
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
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
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
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
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
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
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
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
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 +0000, 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
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
URL: https://github.com/SSSD/sssd/pull/453 Title: #453: Speed up by-ID lookups with the help of the Global Catalog
Label: +Accepted
sssd-devel@lists.fedorahosted.org
-
jhrozek -
pbrezina
-
sumit-bose