URL:
https://github.com/SSSD/sssd/pull/289
Title: #289: Check the timestamp cache during the users/groups cleanup
fidencio commented:
"""
On Wed, Jun 7, 2017 at 2:04 PM, Jakub Hrozek <notifications(a)github.com>
wrote:
On Wed, Jun 07, 2017 at 01:58:26AM -0700, fidencio wrote:
> @jhrozek:
>
> So, I've implemented most of your suggestion but now I'm facing an issue
about making sysdb_search_users() and sysdb_search_groups() looking at the
timestamp cache.
>
> What I've done in the ldap_cleanup() code was:
> ```
> in case a timestamp cache exists:
> check the timestamp cache
> search in the normal cache for the results gotten in the timestamp cache
> otherwise
> check the normal cache
> merge the results with the timestamp cache
> ```
I /thought/ (but I was wrong, see below) we wanted to do:
1) search the timestamp cache using the provided filter. This returns
a set of results
2) for each DN from the timestamp results, search the DN in the
persistent cache
3) always search the persistent cache
4) merge in the timestamp attributes into results from 3)
But as you pointed out, 3 is flawed because we are searching based on
the attributes that are not in the same cache..oops..
>
> The very same logic cannot be applied in the sysdb_search_users() and
> sysdb_search_groups() as there are some places in the code using those
> functions filtering by "attributes" that are not part of the timestamp
> cache. By doing this, the timestamp cache returns an ENOENT and we do
*not*
> want to go an search the cache in this case. The reason we don't want to
> do that is because timestamps may not be up-to-date in the normal cache,
> so we would search in the timestamp cache, get an ENOENT as result,
search
> in the normal cache and find users/groups (which is exactly what we're
> trying to solve).
Hmm, I didn't think about this. I'm sorry for leading you down the wrong
path.
That's no problem at all. The most part of the work related to this patch
set can (and most likely) will be used upstream.
So, it's just a matter of which patches we're going to backport.
>
> Considering this limitation I'd like to check with you what you think
about these two possible approaches::
> - Introduce a new function that only will be used by the ldap_cleanup()
for now. The changes already done for the work you suggested could still be
merged, but for sure not backported.
Yes, this is the fastest way to fix the bug. Would you like me to
continue with the first round of patches?
Nops. I'd prefer to add a new function to sysdb that does exactly what I've
done in ldap_cleanup(),
Something like: sysdb_search_{users,groups}_by_timestamp() (feel free to
suggest a better naming) and then we can simply use it in ldap_cleanup().
It doesn't look intrusive and still looks like a better solution than my
first patchset (which exposes somethings from the timestamp cache).
Is this okay for you?
> - Introduce a way to check, in the timestamp cache methods, that we're
not searching for something that's not supported (and return a new error
when it happens) so we for sure know that we can search in the normal cache
on these cases.
Yes, the safest way would be to explode the filter ourselves at least to
learn during development that the search might be not precise.
>
> The first approach seems way saner and less hacky than the second one,
IMO.
>
> I'd really like to hear some suggestions from you (and from other
developers as well) about these 2 approaches or even about something else
that I've missed/wasn't able to come up with.
>
> --
> You are receiving this because you were mentioned.
> Reply to this email directly or view it on GitHub:
>
https://github.com/SSSD/sssd/pull/289#issuecomment-306734209
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<
https://github.com/SSSD/sssd/pull/289#issuecomment-306774398>, or mute
the thread
<
https://github.com/notifications/unsubscribe-auth/AAG4ekLCbrVgHJIulucTX1b...
.
"""
See the full comment at
https://github.com/SSSD/sssd/pull/289#issuecomment-306776441