2:57 a.m.
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
6:47 a.m.
New subject: [sssd PR#812][synchronized] 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: 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
6:49 a.m.
New subject: [sssd PR#812][comment] Implement background refresh for IPA and AD domains and subdomains
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
7:38 a.m.
New subject: [sssd PR#812][comment] Implement background refresh for IPA and AD domains and subdomains
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
6:21 a.m.
New subject: [sssd PR#812][synchronized] 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: 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
6:23 a.m.
New subject: [sssd PR#812][comment] Implement background refresh for IPA and AD domains and subdomains
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
7:37 a.m.
New subject: [sssd PR#812][synchronized] 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: 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
7:37 a.m.
New subject: [sssd PR#812][comment] Implement background refresh for IPA and AD domains and subdomains
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
8:50 a.m.
New subject: [sssd PR#812][comment] Implement background refresh for IPA and AD domains and subdomains
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
2:28 p.m.
New subject: [sssd PR#812][comment] Implement background refresh for IPA and AD domains and subdomains
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
2:28 p.m.
New subject: [sssd PR#812][+Changes requested] Implement background refresh for IPA and AD domains and subdomains
URL: https://github.com/SSSD/sssd/pull/812
Title: #812: Implement background refresh for IPA and AD domains and subdomains
Label: +Changes requested
8:15 a.m.
New subject: [sssd PR#812][synchronized] 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: 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
8:16 a.m.
New subject: [sssd PR#812][comment] Implement background refresh for IPA and AD domains and subdomains
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
8:16 a.m.
New subject: [sssd PR#812][-Changes requested] Implement background refresh for IPA and AD domains and subdomains
URL: https://github.com/SSSD/sssd/pull/812
Title: #812: Implement background refresh for IPA and AD domains and subdomains
Label: -Changes requested
3:47 a.m.
New subject: [sssd PR#812][comment] Implement background refresh for IPA and AD domains and subdomains
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
3:59 a.m.
New subject: [sssd PR#812][comment] Implement background refresh for IPA and AD domains and subdomains
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
4:42 a.m.
New subject: [sssd PR#812][comment] Implement background refresh for IPA and AD domains and subdomains
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
8:02 a.m.
New subject: [sssd PR#812][synchronized] 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: 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
8:03 a.m.
New subject: [sssd PR#812][comment] Implement background refresh for IPA and AD domains and subdomains
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
8:03 a.m.
New subject: [sssd PR#812][synchronized] 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: 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
3:47 p.m.
New subject: [sssd PR#812][synchronized] 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: 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
3:49 p.m.
New subject: [sssd PR#812][comment] Implement background refresh for IPA and AD domains and subdomains
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
5:47 a.m.
New subject: [sssd PR#812][synchronized] 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: 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
5:48 a.m.
New subject: [sssd PR#812][comment] Implement background refresh for IPA and AD domains and subdomains
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
5:48 a.m.
New subject: [sssd PR#812][comment] Implement background refresh for IPA and AD domains and subdomains
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
5:54 a.m.
New subject: [sssd PR#812][comment] Implement background refresh for IPA and AD domains and subdomains
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
5:19 a.m.
New subject: [sssd PR#812][comment] Implement background refresh for IPA and AD domains and subdomains
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
5:44 a.m.
New subject: [sssd PR#812][comment] Implement background refresh for IPA and AD domains and subdomains
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
5:46 a.m.
New subject: [sssd PR#812][comment] Implement background refresh for IPA and AD domains and subdomains
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
5:55 a.m.
New subject: [sssd PR#812][comment] Implement background refresh for IPA and AD domains and subdomains
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
7:28 a.m.
New subject: [sssd PR#812][synchronized] 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: 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
7:30 a.m.
New subject: [sssd PR#812][comment] Implement background refresh for IPA and AD domains and subdomains
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
5:33 a.m.
New subject: [sssd PR#812][comment] Implement background refresh for IPA and AD domains and subdomains
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
8:45 a.m.
New subject: [sssd PR#812][comment] Implement background refresh for IPA and AD domains and subdomains
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
9:23 a.m.
New subject: [sssd PR#812][synchronized] 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: 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
9:24 a.m.
New subject: [sssd PR#812][comment] Implement background refresh for IPA and AD domains and subdomains
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
9:55 a.m.
New subject: [sssd PR#812][comment] Implement background refresh for IPA and AD domains and subdomains
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
9:55 a.m.
New subject: [sssd PR#812][+Accepted] Implement background refresh for IPA and AD domains and subdomains
URL: https://github.com/SSSD/sssd/pull/812
Title: #812: Implement background refresh for IPA and AD domains and subdomains
Label: +Accepted
5:36 a.m.
New subject: [sssd PR#812][comment] Implement background refresh for IPA and AD domains and subdomains
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
5:37 a.m.
New subject: [sssd PR#812][closed] 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: 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
5:37 a.m.
New subject: [sssd PR#812][+Pushed] Implement background refresh for IPA and AD domains and subdomains
URL: https://github.com/SSSD/sssd/pull/812
Title: #812: Implement background refresh for IPA and AD domains and subdomains
Label: +Pushed
1779
days inactive
1836
days old
sssd-devel@lists.fedorahosted.org
40 comments
2 participants
participants (2)
-
jhrozek -
sumit-bose