URL: https://github.com/SSSD/sssd/pull/153 Author: celestian Title: #153: SYSDB: Changing dataExpireTimestamp in domain cache Action: opened
PR body: """ When a group/users are invalidated from sss cache, the group/user information in Domain (cache_LDAP.ldb) and timestamps cache are inconsistent with regard to dataExpireTimestamp attribute. This patch fixes it.
Resolves: https://fedorahosted.org/sssd/ticket/3164 """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/153/head:pr153 git checkout pr153
URL: https://github.com/SSSD/sssd/pull/153 Author: celestian Title: #153: SYSDB: Changing dataExpireTimestamp in domain cache Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/153/head:pr153 git checkout pr153
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: SYSDB: Changing dataExpireTimestamp in domain cache
lslebodn commented: """ On (13/02/17 06:26), celestian wrote:
When a group/users are invalidated from sss cache, the group/user information in Domain (cache_LDAP.ldb) and timestamps cache are inconsistent with regard to dataExpireTimestamp attribute. This patch fixes it.
Is this change related just to sss_cache utility?
If yes then it mightbe better to solve it in higher level and avoid writing to standard cache all the time. IMHO the current approach might decrease a performance benefit of timestamp cache.
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/153#issuecomment-279446947
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: SYSDB: Changing dataExpireTimestamp in domain cache
fidencio commented: """ @celestian, there are a few integration tests breaking after this patch:
test_ts_cache.py::test_group_2307bis_update_same_modstamp FAILED test_ts_cache.py::test_group_2307bis_update_same_attrs FAILED test_ts_cache.py::test_group_2307_update_same_modstamp FAILED test_ts_cache.py::test_group_2307_update_same_attrs FAILED test_ts_cache.py::test_user_update_same_modstamp FAILED test_ts_cache.py::test_user_update_same_attrs FAILED
I'm setting "Changes Request" till we're able to solve those issues. """
See the full comment at https://github.com/SSSD/sssd/pull/153#issuecomment-279490382
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: SYSDB: Changing dataExpireTimestamp in domain cache
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: SYSDB: Changing dataExpireTimestamp in domain cache
celestian commented: """ Thanks for comments. In my opinion it would be better to have the same value of dataExpireTimestamp only if we use sss_cache. The question is whether it could be confusing. """
See the full comment at https://github.com/SSSD/sssd/pull/153#issuecomment-279628799
URL: https://github.com/SSSD/sssd/pull/153 Author: celestian Title: #153: SYSDB: Changing dataExpireTimestamp in domain cache Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/153/head:pr153 git checkout pr153
URL: https://github.com/SSSD/sssd/pull/153 Author: celestian Title: #153: sss_cache: User/groups invalidation in domain cache Action: edited
Changed field: title Original value: """ SYSDB: Changing dataExpireTimestamp in domain cache """
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
celestian commented: """ I pushed new version (#2).
I addressed Lukáš's comment. Now it works only for sss_cache case. In detail, I added functions ``` sysdb_invalidate_user_cache_entry() sysdb_invalidate_group_cache_entry() ``` which invalidates the entries in domain cache. And it is added to sss_cache.
Unfortunately I am not able to test it in this moment (due to broken dnf repositories). But I would like if you look at this new solution. """
See the full comment at https://github.com/SSSD/sssd/pull/153#issuecomment-279680975
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
celestian commented: """ I pushed new version (#2).
I addressed Lukáš's comment. Now it works only for sss_cache case. In detail, I added functions ``` sysdb_invalidate_user_cache_entry() sysdb_invalidate_group_cache_entry() ``` which invalidates the entries in domain cache. And it is added to sss_cache.
Unfortunately I am not able to test it in this moment (due to broken dnf repositories). But I would like if you look at this new solution. """
See the full comment at https://github.com/SSSD/sssd/pull/153#issuecomment-279680975
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
fidencio commented: """ This approach seems less intrusive than the previous one and the integration tests passed locally (already asked @celestian to run our CI as well).
I'll do a proper review of the patches later Today/Tomorrow and I'm removing the "Changes requested" label for now. """
See the full comment at https://github.com/SSSD/sssd/pull/153#issuecomment-279684152
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
celestian commented: """ FYI http://sssd-ci.duckdns.org/logs/job/62/59/summary.html """
See the full comment at https://github.com/SSSD/sssd/pull/153#issuecomment-279693430
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
celestian commented: """ So, dnf repositories work again. I am able to test the functionality of my patch set. Unfortunately it doesnt work. I will fix it. """
See the full comment at https://github.com/SSSD/sssd/pull/153#issuecomment-279707593
URL: https://github.com/SSSD/sssd/pull/153 Author: celestian Title: #153: sss_cache: User/groups invalidation in domain cache Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/153/head:pr153 git checkout pr153
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
celestian commented: """ Fixed patch is pushed. I sent it to our CI and I will share the result. """
See the full comment at https://github.com/SSSD/sssd/pull/153#issuecomment-280025798
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
fidencio commented: """ @celestian: One thing that I forgot to mention, it's worth to write a test for this as well. Just checked with @lslebodn and he mentioned that an integration test is probably the way to go. """
See the full comment at https://github.com/SSSD/sssd/pull/153#issuecomment-280063001
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
celestian commented: """ The are results from our CI http://sssd-ci.duckdns.org/logs/job/62/90/summary.html How we can see this patch fails on test_ts_cache.py integration test, namely: ``` test_group_2307bis_update_same_modstamp FAILED test_group_2307bis_update_same_attrs FAILED test_group_2307_update_same_modstamp FAILED test_group_2307_update_same_attrs FAILED test_user_update_same_modstamp FAILED test_user_update_same_attrs FAILED ``` The reason is that there is used sss_cache internally on those tests.
I am not sure if request in https://fedorahosted.org/sssd/ticket/3164 is really good idea. The timestamp cache is important for high performance, so those tests cover essential part of this functionality. If we really would like to have "user/groups invalidation in domain cache" I would like discuss those broken tests with @jhrozek to be sure that I will not break the logic of tests. (I will talk to @jhrozek on Monday.) """
See the full comment at https://github.com/SSSD/sssd/pull/153#issuecomment-280316726
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
fidencio commented: """ @celestian:
Just ran the ci on your patch and it passed without issues (apart from the rhel6, but the failure is unrelated). http://sssd-ci.duckdns.org/logs/job/62/92/summary.html
Seems that you have tested the previous version of the patch.
Anyways, with the changes proposed the patch will be good to go. So, no need to wait for @jhrozek. """
See the full comment at https://github.com/SSSD/sssd/pull/153#issuecomment-280337573
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
fidencio commented: """ I take my comment back. I've submitted the wrong patch version to CI.
Yes, those tests are failing! """
See the full comment at https://github.com/SSSD/sssd/pull/153#issuecomment-280357058
URL: https://github.com/SSSD/sssd/pull/153 Author: celestian Title: #153: sss_cache: User/groups invalidation in domain cache Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/153/head:pr153 git checkout pr153
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
celestian commented: """ New version pushed, but CI said "NO": http://sssd-ci.duckdns.org/logs/job/62/98/summary.html
"""
See the full comment at https://github.com/SSSD/sssd/pull/153#issuecomment-281071243
URL: https://github.com/SSSD/sssd/pull/153 Author: celestian Title: #153: sss_cache: User/groups invalidation in domain cache Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/153/head:pr153 git checkout pr153
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
celestian commented: """ New version pushed. CI passed: http://sssd-ci.duckdns.org/logs/job/63/02/summary.html """
See the full comment at https://github.com/SSSD/sssd/pull/153#issuecomment-281270670
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
fidencio commented: """ Patch looks good and there's just two really minor coding style issues, IMO.
Whoever pushes this patch, please, squash https://fidencio.fedorapeople.org/pr153/pr153-squash.patch into this PR. """
See the full comment at https://github.com/SSSD/sssd/pull/153#issuecomment-282299100
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/153 Author: celestian Title: #153: sss_cache: User/groups invalidation in domain cache Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/153/head:pr153 git checkout pr153
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
celestian commented: """ I updated the patch. 1. The issue with enum is addressed. 1. I added function ```sysdb_invalidate_cache_entry()``` to sysdb API and removed the specific two functions provided earlier. It is needed because I use internal sysdb function, namely ```sysdb_set_cache_entry_attr```. """
See the full comment at https://github.com/SSSD/sssd/pull/153#issuecomment-283332627
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
jhrozek commented: """ I have one last trivial thing to add -- can you add comment to the new function that is always writes to the persistent cache and a similar comment to the call of this new function from the sss_cache tool? I would just like to avoid someone using this function from the deamon code, not realizing what it does and degrading performance. """
See the full comment at https://github.com/SSSD/sssd/pull/153#issuecomment-284663691
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
jhrozek commented: """ btw CI passed, so as soon as the comments are added, I'll push the patch: http://sssd-ci.duckdns.org/logs/job/63/98/summary.html """
See the full comment at https://github.com/SSSD/sssd/pull/153#issuecomment-284683019
URL: https://github.com/SSSD/sssd/pull/153 Author: celestian Title: #153: sss_cache: User/groups invalidation in domain cache Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/153/head:pr153 git checkout pr153
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
celestian commented: """ New version uploaded. Thanks for review. """
See the full comment at https://github.com/SSSD/sssd/pull/153#issuecomment-284703683
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
fidencio commented: """ @jhrozek re-reading those patches I do believe all your comments have been addressed in the latest version.
I'm adding a the "Accepted" label. """
See the full comment at https://github.com/SSSD/sssd/pull/153#issuecomment-284973494
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
jhrozek commented: """ * master: 57a924e71230ea360b19a88e0d5818cf01017161 """
See the full comment at https://github.com/SSSD/sssd/pull/153#issuecomment-285018846
URL: https://github.com/SSSD/sssd/pull/153 Author: celestian Title: #153: sss_cache: User/groups invalidation in domain cache Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/153/head:pr153 git checkout pr153
URL: https://github.com/SSSD/sssd/pull/153 Title: #153: sss_cache: User/groups invalidation in domain cache
Label: +Pushed
sssd-devel@lists.fedorahosted.org
-
celestian -
fidencio
-
jhrozek
-
lslebodn