#48752 Page result search should return empty cookie if there is no returned entry
Closed: Fixed None Opened 3 years ago by tbordaz.

SSSD smart refresh uses PR search, it loops until the returned cookie is empty.

SSSD reported cases where SSSD start flowing DS with such requests

[19/Jan/2016:14:57:00 -0500] conn=117 op=7457 RESULT err=0 tag=101 nentries=0 etime=0 notes=P pr_idx=7448 
[19/Jan/2016:14:57:00 -0500] conn=117 op=7458 SRCH base="ou=SUDOers,<SUFFIX>" scope=2 filter="(&(&(objectClass=sudoRole)(entryusn>=2)(!(entryusn=2)))(|(!(sudoHost=*))(sudoHost=ALL)(sudoHost=<HOST>)...
[19/Jan/2016:14:57:00 -0500] conn=117 op=7458 RESULT err=0 tag=101 nentries=0 etime=0 notes=P pr_idx=7449 
[19/Jan/2016:14:57:00 -0500] conn=117 op=7459 SRCH base="ou=SUDOers,<SUFFIX>" scope=2 filter="(&(&(objectClass=sudoRole)(entryusn>=2)(!(entryusn=2)))(|(!(sudoHost=*))(sudoHost=ALL)(sudoHost=<HOST>)...
[19/Jan/2016:14:57:00 -0500] conn=117 op=7459 RESULT err=0 tag=101 nentries=0 etime=0 notes=P pr_idx=7450 
[19/Jan/2016:14:57:00 -0500] conn=117 op=7460 SRCH base="ou=SUDOers,<SUFFIX>" scope=2 filter="(&(&(objectClass=sudoRole)(entryusn>=2)(!(entryusn=2)))(|(!(sudoHost=*))(sudoHost=ALL)(sudoHost=<HOST>)...
[19/Jan/2016:14:57:00 -0500] conn=117 op=7460 RESULT err=0 tag=101 nentries=0 etime=0 notes=P pr_idx=7451

A possibility to explain that is that DS returns an non empty cookie, although the number of returned entry was 0.

RFC https://www.ietf.org/rfc/rfc2696.txt states (3 - Client-Server interaction:

The cookie MUST be set to an empty value if there are no more entries to return 
(i.e., the page of search results returned was the last), or, if there are more 
entries to return, to an octet string of the server's choosing,used to resume 
the search.

Hi Thierry,

Instead of checking the search result count in pagedresults_set_response_control as you proposed:
{{{
ldap/servers/slapd/pagedresults.c

diff --git a/ldap/servers/slapd/pagedresults.c b/ldap/servers/slapd/pagedresults.c
index d394dab..c6d6276 100644

a b pagedresults_set_response_control( Slapi_PBlock pb, int iscritical,
244 244 }
245 245
246 246 /
begin sequence, payload, end sequence /
247 if (current_search_count < 0) {
247 if (current_search_count <= 0) {
248 248 cookie_str = slapi_ch_strdup("");
249 slapi_pblock_set ( pb, SLAPI_PAGED_RESULTS_COOKIE, 0 );
249 250 } else {
250 251 cookie_str = slapi_ch_smprintf("%d", index);
252 slapi_pblock_set ( pb, SLAPI_PAGED_RESULTS_COOKIE, index );
251 253 }
252 254 ber_printf ( ber, "{io}", estimate, cookie_str, strlen(cookie_str) );
253 255 if ( ber_flatten ( ber, &berval ) != LDAP_SUCCESS )
}}}
how about checking in the caller side who has the next_be info like this?
{{{
diff --git a/ldap/servers/slapd/opshared.c b/ldap/servers/slapd/opshared.c
index 41a1b37..d870747 100644
--- a/ldap/servers/slapd/opshared.c
+++ b/ldap/servers/slapd/opshared.c
@@ -699,7 +699,7 @@ op_shared_search (Slapi_PBlock
pb, int send_result)
}
pagedresults_unlock(pb->pb_conn, pr_idx);

  • if (PAGEDRESULTS_SEARCH_END == pr_stat) {
  • if ((PAGEDRESULTS_SEARCH_END == pr_stat) || (0 == pnentries)) {
    / no more entries to send in the backend /
    if (NULL == next_be) {
    / no more entries && no more backends /
    @@ -869,7 +869,7 @@ op_shared_search (Slapi_PBlock *pb, int send_result)
         curr_search_count = pnentries;
         slapi_pblock_get(pb, SLAPI_SEARCH_RESULT_SET, &sr);
    
    • if (PAGEDRESULTS_SEARCH_END == pr_stat) {
    • if ((PAGEDRESULTS_SEARCH_END == pr_stat) || (0 == pnentries)) {
      / no more entries, but at least another backend /
      PR_EnterMonitor(pb->pb_conn->c_mutex);
      pagedresults_set_search_result(pb->pb_conn, operation, NULL, 1, pr_idx);
      }}}
      This way if the search result count is 0 AND there's no more backend, -1 current_search_count is passed to pagedresults_set_response_control?

adding reference to the Red Hat bz 1326077

Hi Noriko,

The fix https://fedorahosted.org/389/ticket/48752#comment:5 looks more like a final fix than the one proposed https://fedorahosted.org/389/attachment/ticket/48752/0002-Ticket-48752-Page-result-search-should-return-empty-.patch.

In fact https://fedorahosted.org/389/attachment/ticket/48752/0002-Ticket-48752-Page-result-search-should-return-empty-.patch did not take into consideration PR over multiple backend. It was just proposed for a test with only one backend.

For information, we managed to reproduce twice a week. And proposed a test hotfix with https://fedorahosted.org/389/attachment/ticket/48752/0002-Ticket-48752-Page-result-search-should-return-empty-.patch. It will be applied and run over a week to check if it fixes the problem and then check with the additional logs that we can have condition where there is 0 returned entry but not empty cookie.

Replying to [comment:7 tbordaz]:

For information, we managed to reproduce twice a week. And proposed a test hotfix with https://fedorahosted.org/389/attachment/ticket/48752/0002-Ticket-48752-Page-result-search-should-return-empty-.patch. It will be applied and run over a week to check if it fixes the problem and then check with the additional logs that we can have condition where there is 0 returned entry but not empty cookie.

Thank you for the latest status, Thierry! If your patch fixes the problem, the concept is proved correct. I hope we can come to the conclusion and close this case... ;)

I'm happy with the look of the code for this fix. How did the test go? Is this ready to be merged do you think?

Replying to [comment:9 firstyear]:

I'm happy with the look of the code for this fix. How did the test go? Is this ready to be merged do you think?

Hi William,

The ticket comes from SSSD cascades of requests.
SSSD team was unable to reproduce and I was also unable to reproduce with direct PR searches.

Now reviewing SSSD code, it looks the only case it could cascade is that we return a valid cookie (non empty). As in the logs we can see that when SSSD cascade, the search returned no entry we just "guess" we could end in such situation: no returned entry + valid cookie.

From time to time, some SSSD users are hitting this issue but so far I have been enable to let them test a hotfix with that fix.

If the fix looks ok, we may decide to push it even if we are unable to verify it addresses the SSSD cascade. Or we may decide to wait for an other user being able to verify it fixes the cascades.

Hi Thierry,

Did you have a chance to try the diff in the comment (https://fedorahosted.org/389/ticket/48752#comment:5)? I think the patch you attached to this ticket is not a finalized one (since it is not supporting the multiple backends). Could it be possible to try the one in #comment:5? (Or I'd better come up with the patch?)

Thanks!

git patch file (master) -- reviewed 0002-Ticket-48752-Page-result-search-should-return-empty-.patch and applied the proposal in #comment:5 as well as fixed compiler warnings.
0001-Ticket-48752-Page-result-search-should-return-empty-.2.patch

Per discussion at the scrum, pushing the patch on behalf of Thierry (Thanks!)

Pushed to master:
ba3b844..f215fb6 master -> master
commit f215fb6

Pushed to 389-ds-base-1.3.4:
14bc6ce..ca50d1c 389-ds-base-1.3.4 -> 389-ds-base-1.3.4
commit ca50d1c

Small, but important change to the test case (try-finally block + wrap the generator).
Please, review the second patch.

To ssh://git.fedorahosted.org/git/389/ds.git

Pushed to master:
0532a63..fc97900 master -> master
commit fc97900
Author: Simon Pichugin spichugi@redhat.com
Date: Thu Jul 14 14:14:12 2016 +0200

Pushed to 389-ds-base-1.3.4:

42fcf97..7738a00 389-ds-base-1.3.4 -> 389-ds-base-1.3.4
commit 7738a00
Author: Simon Pichugin spichugi@redhat.com
Date: Thu Jul 14 14:14:12 2016 +0200

Metadata Update from @tbordaz:
- Issue assigned to tbordaz
- Issue set to the milestone: 1.3.4.10

2 years ago

Login to comment on this ticket.

Metadata