[sssd PR#812][opened] Implement background refresh for IPA and AD domains and subdomains
URL: https://github.com/SSSD/sssd/pull/812 Author: jhrozek Title: #812: Implement background refresh for IPA and AD domains and subdomains Action: opened
PR body: """ This PR refactors the existing background refresh task to be more extendable, splits out the account handlers of IPA and AD providers into tevent requests which are finally reused by new ipa_refresh and ad_refresh modules.
The refreshes are done in batches so that we don't starve out other requests or even invoke the watchdog. """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/812/head:pr812 git checkout pr812
URL: https://github.com/SSSD/sssd/pull/812 Author: jhrozek Title: #812: Implement background refresh for IPA and AD domains and subdomains Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/812/head:pr812 git checkout pr812
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
jhrozek commented: """ I just pushed fixes for some minor issues that Coverity found. """
See the full comment at https://github.com/SSSD/sssd/pull/812#issuecomment-490871040
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
sumit-bose commented: """ Hi,
I know you plan to update this PR, but jfyi I had to add the attached changes ti make sure MAX defines do not collide.
bye, Sumit [pr812.diff.gz](https://github.com/SSSD/sssd/files/3187061/pr812.diff.gz)
"""
See the full comment at https://github.com/SSSD/sssd/pull/812#issuecomment-493050078
URL: https://github.com/SSSD/sssd/pull/812 Author: jhrozek Title: #812: Implement background refresh for IPA and AD domains and subdomains Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/812/head:pr812 git checkout pr812
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
jhrozek commented: """ Thank you for the diff, I squashed it into my patches and added updates by SID for users and groups from AD domains. It's only pure AD domains as IPA-AD trusts always resolve the groups to find out the group type, therefore we do have the name at least. """
See the full comment at https://github.com/SSSD/sssd/pull/812#issuecomment-494350299
URL: https://github.com/SSSD/sssd/pull/812 Author: jhrozek Title: #812: Implement background refresh for IPA and AD domains and subdomains Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/812/head:pr812 git checkout pr812
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
jhrozek commented: """ The last version just fixes a coverity warning. """
See the full comment at https://github.com/SSSD/sssd/pull/812#issuecomment-494373153
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
sumit-bose commented: """ Hi,
my first tests went quite well and I like the batch feature. But `be_refresh_get_values_ex()` use `sysdb_search_entry()` which applies the search filter with `dataExpireTimestamp` to the data cache, but the attribute itself is only updated in the timestamp cache. So it looks like we need a sysdb call with calls `sysdb_search_ts_entry()` first and then merges the result with the request attributes from the data cache object.
bye, Sumit """
See the full comment at https://github.com/SSSD/sssd/pull/812#issuecomment-494811145
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
jhrozek commented: """ Thank you for the review, I'll set changes requested """
See the full comment at https://github.com/SSSD/sssd/pull/812#issuecomment-494922113
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/812 Author: jhrozek Title: #812: Implement background refresh for IPA and AD domains and subdomains Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/812/head:pr812 git checkout pr812
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
jhrozek commented: """ Thank you for the review, the first two patches change the sysdb lookups to check the timestamp cache. """
See the full comment at https://github.com/SSSD/sssd/pull/812#issuecomment-496511796
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
jhrozek commented: """ We talked about this PR with Sumit on the phone, I'm adding some notes so I don't forget: - there is some code duplication between the the refresh modules - the next refresh is not scheduled from the end of the run, but from the start of the previous run - initgroups are not refreshed. Maybe we could add a new refresh type (and maybe even make it configurable what refreshes are ran) to refresh user's groups if they already had the initgroups operation ran on them """
See the full comment at https://github.com/SSSD/sssd/pull/812#issuecomment-497630270
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
jhrozek commented: """ I also wrote a stupid test program to see if creating the compound filter is better or worse than doing base-searches in a loop. I found a bug in that program so I need to check again, but Sumit also suggested to add objects that do not match to the program. """
See the full comment at https://github.com/SSSD/sssd/pull/812#issuecomment-497634073
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
jhrozek commented: """ OK, I'm back here after having to spend time with some other work. The first observation is that the compound filter is massively slower that a loop of base searches.. A quick look at callgrind shows that a lot of the time is spent in ldb_match_message and ldb_dn_compare. """
See the full comment at https://github.com/SSSD/sssd/pull/812#issuecomment-503030279
URL: https://github.com/SSSD/sssd/pull/812 Author: jhrozek Title: #812: Implement background refresh for IPA and AD domains and subdomains Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/812/head:pr812 git checkout pr812
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
jhrozek commented: """ I pushed new patches that just address the review comments and search with base searches instead of the huge sysdb filter. The other issues still need to be implemented. """
See the full comment at https://github.com/SSSD/sssd/pull/812#issuecomment-503125677
URL: https://github.com/SSSD/sssd/pull/812 Author: jhrozek Title: #812: Implement background refresh for IPA and AD domains and subdomains Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/812/head:pr812 git checkout pr812
URL: https://github.com/SSSD/sssd/pull/812 Author: jhrozek Title: #812: Implement background refresh for IPA and AD domains and subdomains Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/812/head:pr812 git checkout pr812
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
jhrozek commented: """ The current version fixes the periodical task to start relative to the last task's finish time, not start time. Still todo:
- [ ] Add the initgroups refresh - [ ] Add some warning if the periodical task is too slow and might result in expired entries? - [ ] Look at reducing the code duplication
btw I'd like to have the whole PR reviewed and merged before me and Sumit go for a summer vacation.. """
See the full comment at https://github.com/SSSD/sssd/pull/812#issuecomment-503741016
URL: https://github.com/SSSD/sssd/pull/812 Author: jhrozek Title: #812: Implement background refresh for IPA and AD domains and subdomains Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/812/head:pr812 git checkout pr812
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
jhrozek commented: """ The current version fixes the periodical task to start relative to the last task's finish time, not start time. Still todo:
- [x] Add the initgroups refresh - [ ] Add some warning if the periodical task is too slow and might result in expired entries? - [ ] Look at reducing the code duplication
btw I'd like to have the whole PR reviewed and merged before me and Sumit go for a summer vacation.. """
See the full comment at https://github.com/SSSD/sssd/pull/812#issuecomment-503741016
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
jhrozek commented: """ The current version fixes the periodical task to start relative to the last task's finish time, not start time. Still todo:
- [x] Add the initgroups refresh - [ ] Add some warning if the periodical task is too slow and might result in expired entries? - [x] Look at reducing the code duplication
btw I'd like to have the whole PR reviewed and merged before me and Sumit go for a summer vacation.. """
See the full comment at https://github.com/SSSD/sssd/pull/812#issuecomment-503741016
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
jhrozek commented: """ The current PR is more-or-less finished, I think. It contains the initgroups refresh for entries that had initgrExpireTimestamp at any time and decreases the code duplication in the refresh modules. I generated some code with macros, which I also in general don't like, but I think here it might be OK because the code would have been completely cut-and-paste anyway.
I added the patches atop the existing ones. Maybe the code duplication reduction could have been squashed before the patches that add the IPA and AD refreshes, but I didn't want to destroy my history in case you didn't like this approach. Just let me know if you would prefer to have the patches squashed.
There are some more potential improvements I can think about, but I also didn't want to just implement them without having some second opinion: - group refreshes don't take group nesting into account. It is possible that refreshing parentgroup might also refresh childgroup. Maybe the refresh code could do another cache search (as long as it's a BASE-scoped search, it would be quite cheap) to check if a group should be refreshed or not. - I still haven't implemented warning if the refresh takes longer than the cache timeout. I can do it, though. - One thing we discussed on the phone was the ability to specify only certain entry types to be refreshed. It would be possible to add another domain struct member that would be a flag and enable only those refreshes. But to be honest I forgot the reason why we talked about this enhancement :-) """
See the full comment at https://github.com/SSSD/sssd/pull/812#issuecomment-505825200
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
sumit-bose commented: """ Hi Jakub,
while checking with the AD provider it looks like although the initgroups refresh is run the related timestamp in the cache object is not updated. Can you check if you see this as well or if I have to check my test environment?
bye, Sumit """
See the full comment at https://github.com/SSSD/sssd/pull/812#issuecomment-507206231
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
jhrozek commented: """ Yes, I can see it as well.. """
See the full comment at https://github.com/SSSD/sssd/pull/812#issuecomment-507213297
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
jhrozek commented: """ btw I don't see this behaviour when I run id from the command line..weird. Anyway, thank you for reporting, I will take a look. """
See the full comment at https://github.com/SSSD/sssd/pull/812#issuecomment-507213964
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
jhrozek commented: """ Ah, OK, the attribute is set at the DP level (`set_initgroups_expire_attribute()`). It was even me who moved it outside the provider..oops.. """
See the full comment at https://github.com/SSSD/sssd/pull/812#issuecomment-507216631
URL: https://github.com/SSSD/sssd/pull/812 Author: jhrozek Title: #812: Implement background refresh for IPA and AD domains and subdomains Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/812/head:pr812 git checkout pr812
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
jhrozek commented: """ I added two new patches, one should IMO stay as a separate patch, the other I'll squash into the initgroups support patches, but having both the patches separate for now makes things easier to review I guess.
btw the code duplication between the provider-specific requests is still quite bad, I'll look into decreasing it even more, but for now, I hope the patches are good for review again. """
See the full comment at https://github.com/SSSD/sssd/pull/812#issuecomment-507244334
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
sumit-bose commented: """ retest this please """
See the full comment at https://github.com/SSSD/sssd/pull/812#issuecomment-507619059
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
sumit-bose commented: """ Hi,
I tested the patches on AD client, IPA client and IPA servers and the worked as expected. Coverity and the internal CI didn't find any issues, not sure why the CI here got stuck in 'Build in progress'?
If you can extend the commit message of the last two patches I would ACK the whole set. I do not think that anything has to be squashed into other patches, all stand for themself.
bye, Sumit """
See the full comment at https://github.com/SSSD/sssd/pull/812#issuecomment-507684107
URL: https://github.com/SSSD/sssd/pull/812 Author: jhrozek Title: #812: Implement background refresh for IPA and AD domains and subdomains Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/812/head:pr812 git checkout pr812
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
jhrozek commented: """ OK, done, thank you for the careful review. """
See the full comment at https://github.com/SSSD/sssd/pull/812#issuecomment-507700738
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
sumit-bose commented: """ ACK """
See the full comment at https://github.com/SSSD/sssd/pull/812#issuecomment-507713737
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
jhrozek commented: """ * master: * cdc44a05d11ae614eb55f219f70150d241cd850f * 7a08d1dea8cb9148ba1afe13f4d4567229c9b381 * 039384b8851bb6a2513af83dba0df318432e0c63 * 60c876aefe2efc5a67929f9b3890b627cea7c549 * 792235097b9b63593dc717440aab48e8671fbf12 * 1d0e75e9c5db0acf946f82705a4640063ea5aea9 * d76756ef472da9593c691f94186d09226bb49916 * b72adfcc332b13489931483201bcc4c7ecf9ecb6 * 576f3691a2d22322b08fb55fe74899d2ea4975d6 * 0fbc317ac7f1fe13cd41364c67db7d7a19d7d546 * 7443498cc074c323e3b307f47ed49d59a5001f64 * 2cb294e6d5782aa725a2e9d7892a9e0c62e0b3a9 * ac72bb4ab1a8d3d13f0d459efe5f23cf010c2790 * 41305ef5a0ef2f4796e322190ffcc12331151643 * d1eb0a70de3c98ca9dc03a0b79287f4ce6ee4855 * 9d49c90ceb7388333c8682f4cbd6842ec236b9de * bb0bd61ac54dca429b6562e808755152d4c90ce7 * 1a08b53defa7f921a9b0f9e839ca90f91b5f86d2 * f27955297603dd7bcbab2569394853d5d9ca90ea * db99504a5295ae1f9bc5166133c8f21e4510c676 """
See the full comment at https://github.com/SSSD/sssd/pull/812#issuecomment-508715722
URL: https://github.com/SSSD/sssd/pull/812 Author: jhrozek Title: #812: Implement background refresh for IPA and AD domains and subdomains Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/812/head:pr812 git checkout pr812
URL: https://github.com/SSSD/sssd/pull/812 Title: #812: Implement background refresh for IPA and AD domains and subdomains
Label: +Pushed
sssd-devel@lists.fedorahosted.org
-
jhrozek -
sumit-bose