[autobackport: sssd-2-9] Improve the performance when using enumeration#8558
Draft
sssd-bot wants to merge 9 commits intoSSSD:sssd-2-9from
Draft
[autobackport: sssd-2-9] Improve the performance when using enumeration#8558sssd-bot wants to merge 9 commits intoSSSD:sssd-2-9from
sssd-bot wants to merge 9 commits intoSSSD:sssd-2-9from
Conversation
Function sysdb_enumpwent() is not used. It was replaced by sysdb_enumpwent_filter(). Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com> (cherry picked from commit b89f9b6)
Added a case that was not checked before. It is the case when `attr`, `attr_name` and `addtl_filter` are all `NULL`. Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com> (cherry picked from commit 70e78f1)
Create the filter to retrieve only the requested entries. Do not create a new filter and search for matches if there is no results from the previous search. The called functions handle this case correctly but why wasting time calling them? Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com> (cherry picked from commit 5284ea6)
Function cache_req_user_by_filter_lookup() will set or not the recent filter depending on whether data->name.attr is set or not. As mentioned in the comment, it should be done base on whether the refernced attribute is name or not. Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com> (cherry picked from commit 670db53)
The message said that sysdb_enumpwent() had failed, but it was actually sysdb_enumpwent_filter() which failed. Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com> (cherry picked from commit 55e3a30)
The "name" attribute was not being added to the TS cache, even though that it is part of the DN (ldb doesn't enforce it). Adding this attribute requires that the DB version is incremented for the TS cache to be regenerated with the missing attribute. This made the if-block in sysdb_enumpwent_filter() rather useless. In addition, once this if-block is executed, the fuction leaves without further processing. Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com> (cherry picked from commit 11a15c2)
Although ts_res.count is set to 0 when sysdb_search_ts_users() return ERR_NO_TS, before using it we make an extra check to verify that the returned code is EOK. Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com> (cherry picked from commit 0a739f8)
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the system database to version 0.26, primarily to include the object name in the timestamp cache and refactor user enumeration logic by removing the sysdb_enumpwent function in favor of sysdb_enumpwent_filter. A critical issue was identified in src/responder/common/cache_req/plugins/cache_req_user_by_filter.c, which contains unresolved merge conflict markers that will prevent the code from compiling.
Comment on lines
+93
to
+97
| <<<<<<< HEAD | ||
| if (is_files_provider(domain) || data->name.attr != NULL) { | ||
| ======= | ||
| if (strcmp(attr, SYSDB_NAME) != 0) { | ||
| >>>>>>> 670db53b1 (NSS: Be coherent when using a lastUpdate filter) |
There was a problem hiding this comment.
This file contains unresolved merge conflict markers. This will cause compilation to fail. Please resolve the conflict before merging. Based on the original pull request, the intended logic appears to be what is suggested below.
if (strcmp(attr, SYSDB_NAME) != 0) {
alexey-tikhonov
added
no-backport
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is an automatic backport of PR#8395 Improve the performance when using enumeration to branch sssd-2-9, created by @aplopez.
Caution
@aplopez The patches did not apply cleanly. It is necessary to resolve conflicts before merging this pull request. Commits that introduced conflict are marked with
CONFLICT!.You can push changes to this pull request
Original commits
b89f9b6 - SYSDB: Remove unused function
5b5d1ff - NSS: Reduce a possibly extremely long log message
e91c10a - NSS: Fix wrong condition invalidating an optimization
70e78f1 - TESTS: Improve test_sysdb_enumpwent_filter
5284ea6 - NSS: Some optimizations.
670db53 - NSS: Be coherent when using a lastUpdate filter
55e3a30 - NSS: Fix the logged function name
11a15c2 - NSS: Fix sysdb_enumpwent_filter()
0a739f8 - NSS: Better handle ERR_NO_TS in sysdb_enumpwent_filter()
Backported commits
Conflicting Files Information (check for deleted and re-added files)
Original Pull Request Body
This PR includes:
Enumeration, specially when there are 15,000+ users, is slow. This fix helps, but it doesn't work miracles.
In my test environment, the enumeration went from 8 minutes to about 1.
It is important to know that, with such an amount of users, many operations time out. It is necessary to increment the
timeoutin[nss]and for the domain, but also set large values forldap_enumeration_refresh_timeoutandldap_search_timeoutin the domain. I used these values to avoid any timeout (YMMV):