#48266 Fractional replication evaluates several times the same CSN
Closed: wontfix None Opened 8 years ago by tbordaz.

In MMR topology with Fractional replication, the updates are evaluated by the RA to determine if they should be skipped or sent.

During a replication session, if from the starting CSN all the CSNs will be skipped, then the next session will start at the same starting point and will evaluate the same set of already skipped CSN.
This does not prevent fractional replication to work, because the next update to send will increase the consumer RUV (starting point).
But this behaviour should be improve.

However it can create problem, if the set of skipped CSN is large. In that case the supplier takes time to evaluate all of them. During that time it prevent others suppliers to acquire the consumer replica and to send their own updates.
The increasing delay of the backoff timer of the others supplier, will give them less and less chance to send their updates.

Some cases reported delayed replication by more than 1h.


Workaround:

Under extreme condition (if all updates are skipped and the list of them is large) replication can appear to be broken. A workaround is to apply periodic dummy updates on all servers.
Those updates need to be replicated ones (not updating filtered attributes). Periodicity could be 1/h or less, depending on the update rate.

Thierry, thank you for working hard to solve the sticky problem...

I've reviewed your patch. I have a couple of requests...

1) In the patch descition, you wrote the threshold is 100.

Fix Description:
The fix introduces a subentry under the suffix: 'cn=repl keep alive,$SUFFIX'
During an incremental replication session, if the session only contains skipped updates
and the number of them overpass a threshold (100), it triggers an update on that subentry.

It is indeed 10 in the source code. I guess the macro is correct?
1679 #define FRACTIONAL_SKIPPED_THRESHOLD 10

2) The repl keep alive entry: cn=repl keep alive,$SUFFIX
I'm a little afraid that if there is one repl keep alive entry per suffix, there could be a chance that it is updated by its multiple supliers at one time and could cause a conflict on the repl keep alive entry. To avoid the ugly situation, can we have one keep alive entry per supplier? Probably, you could include the replica ID in the DN?

3) A minor thing, but could you please adjust the indentation? ;)

Thanks!!

{{{

429     dn = slapi_ch_smprintf("cn=%s,%s", KEEP_ALIVE_ENTRY, slapi_sdn_get_dn(repl_root)); 
430     PR_snprintf(entry_string, sizeof (entry_string), "dn: %s\nobjectclass: top\nobjectclass: ldapsubentry\nobjectclass: extensibleObject\ncn: %s", 
431             dn, KEEP_ALIVE_ENTRY);

}}}
If you're going to use malloc/formatting at all here, you might as well combine them into one operation:
{{{
entry_string = slapi_ch_smprintf("dn: cn=%s,%s\nobjectclass: top\nobjectclass: ldapsubentry\nobjectclass: extensibleObject\ncn: %s",
KEEP_ALIVE_ENTRY, slapi_sdn_get_dn(repl_root), KEEP_ALIVE_ENTRY);
}}}
Fixed size buffers should be avoided because if someone created a ridiculously long suffix, this would overflow. If it turns out that there is a performance problem with malloc here, we can revisit. You can use slapi_entry_get_dn_const(e) to get the DN for error messages.

If it turns out to cause performance or other problems, you can get rid of doing the internal search every time - just try the modify - if you get err=32, then add the entry.

Thank you for updating the patch, Thierry. I verified my requests are all taken care.

But I still see this PR_snprintf in 0001-Ticket-48266-1-Fractional-replication-evaluates-severa.patch​ pointed out by Rich in #comment:11.
{{{
432 dn = slapi_ch_smprintf(KEEP_ALIVE_DN_FORMAT, KEEP_ALIVE_ENTRY, rid, slapi_sdn_get_dn(repl_root));
433 PR_snprintf(entry_string, sizeof (entry_string), "dn: %s\nobjectclass: top\nobjectclass: ldapsubentry\nobjectclass: extensibleObject\ncn: %s",
434 dn, KEEP_ALIVE_ENTRY);
}}}
Could you replace the 2 calls into one slapi_ch_smprintf and free entry_string instead of dn at the end of the call?

Thanks!

A very minor thing, but since the cn value in the dn and the one in the attributes do not match, the entry would have "cn: KEEP_ALIVE_ENTRY rid" in addition to "cn: KEEP_ALIVE_ENTRY".
{{{
431 entry_string = slapi_ch_smprintf("dn: cn=%s %d,%s\nobjectclass: top\nobjectclass: ldapsubentry\nobjectclass: extensibleObject\ncn: %s",
432 KEEP_ALIVE_ENTRY, rid, slapi_sdn_get_dn(repl_root), KEEP_ALIVE_ENTRY);
}}}

Another minor thing... You could call slapi_ch_free_string(&entry_string) to avoid a cast that we'd like to avoid...
{{{
464 slapi_ch_free((void**)&entry_string);
}}}

Thank you!!

if there is a keep alive entry per RID, waht happens if the replica is removed ?
replica removal is a common scenario in IPA, maybe cleanallruv should be enhanced to to also remove these entries, could be a separate ticket

Hello Ludwig,

Thanks for looking at this patch.
Currently the subentry is kept in the DIT. No more replica will update it and this entry will be useless for fractional replication. Now I do not see how that entry could create any problem.
In fact I like the idea of CLEANALLRUV to just remove that subentry with a direct ldap delete. It will create a tombstone on all replicas and it will be later purge.
I would prefer to open a separated as the testing of cleanallruv is quite different from that current patch.

thanks

I agree, it is not urgent (and not to delay this fix) and I don't see real problems, except accumulation of unneeded entries. even the reuse of a replicaId should not be problematic.

But cleaning it up when a replica is removed would be nice

Hi Thierry, did you have a chance to see my comment https://fedorahosted.org/389/ticket/48266#comment:13? The 2 requests are minor, but could you please consider them in your patch?
Thanks!

Hi Thierry,

Thank you for the test suite. :)

About some things we can improve. Not really big ones, but logical important.

On the line 150 you have:
{{{
def test_ticket48266_init(topology):
# add dummy entries in the staging DIT
}}}

As this is not an actual test case, it's better to wrap it with fixture and put to the tests.
For example, like that:
{{{

If use scope="module",

then it will be executed only one time per test suite

@pytest.fixture(scope="module")
def entries(topology):
# add dummy entries in the staging DIT
...

def text_ticket48266_count_csn_evaluation(topology, entries):
...
}}}

On the line 485:
{{{
def test_ticket48266(topology):
}}}
I think, we don't need it at all, it does nothing except:
{{{
log.info('Test complete')
}}}
But this work is made by pytest itself. He tells you, when test is finished.

Thanks,
Simon

P.S. can you please avoid line length like at "line 449"?
It's 171 char length and I think it's a little bit too much. :) Preferable length is 80-100, so it fits well on the screen, when you edit two files with vim.

Hi Thierry,

unfortunatly, your patch wouldn't work, because your fixture is never activated.

Please, add your fixture to every test case where you need it. Like this:
{{{
def text_ticket48266_count_csn_evaluation(topology, entries):
...
}}}

Also, you can read more about fixture there: [[https://pytest.org/latest/fixture.html]].
Really succinct and comprehensive article. :)

Thank you,
Simon

Thank you soooooo much, Thierry!

Hi Simon,

Thanks for your reviews and tips. Unfortunately I am a bit lost with py.test because I use to run my test 'isolated' (py.test is not working well on my laptop)

The test case 'text_ticket48266_count_csn_evaluation' was an old test case that is now useless. so it can be safely dropped.

Basically the test cases I would like to run are (in this order) 'entries', 'test_ticket48266_fractional', 'test_ticket48266_check_repl_desc' then 'test_ticket48266_count_csn_evaluation'.
Do you mind to give me a hand to make these test cases 'py.test' compatible ?

thanks
thierry

Thank you Noriko for your continuous support and reviews !!

== Master ==

git merge ticket48266
Updating a35ef55..71ccbfa
Fast-forward
dirsrvtests/tickets/ticket48266_test.py | 446 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
ldap/servers/plugins/replication/repl5.h | 2 +
ldap/servers/plugins/replication/repl5_inc_protocol.c | 39 ++++++
ldap/servers/plugins/replication/repl5_replica.c | 156 ++++++++++++++++++++++++
ldap/servers/plugins/replication/repl5_tot_protocol.c | 14 ++-
5 files changed, 655 insertions(+), 2 deletions(-)
create mode 100644 dirsrvtests/tickets/ticket48266_test.py

git push origin master
Counting objects: 19, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (10/10), done.
Writing objects: 100% (10/10), 3.61 KiB, done.
Total 10 (delta 8), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
a35ef55..1a58bf8 master -> master

commit 1a58bf8
Author: Thierry Bordaz tbordaz@redhat.com
Date: Fri Sep 11 18:56:53 2015 +0200

== 1.3.4 ==

git cherry-pick 1a58bf8
[389-ds-base-1.3.4 6343e4c] Ticket 48266: Fractional replication evaluates several times the same CSN
4 files changed, 209 insertions(+), 2 deletions(-)

git push origin 389-ds-base-1.3.4
Counting objects: 19, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (10/10), done.
Writing objects: 100% (10/10), 3.61 KiB, done.
Total 10 (delta 8), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
29611d8..6343e4c 389-ds-base-1.3.4 -> 389-ds-base-1.3.4

commit 6343e4c
Author: Thierry Bordaz tbordaz@redhat.com
Date: Fri Sep 11 18:56:53 2015 +0200

== 1.3.3 ==

git push origin 389-ds-base-1.3.3
Counting objects: 19, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (10/10), done.
Writing objects: 100% (10/10), 3.65 KiB, done.
Total 10 (delta 8), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
2fecc39..38e0d75 389-ds-base-1.3.3 -> 389-ds-base-1.3.3

commit 38e0d75
Author: Thierry Bordaz tbordaz@redhat.com
Date: Fri Sep 11 18:56:53 2015 +0200

== 1.2.11 ==

git push origin 389-ds-base-1.2.11
Counting objects: 19, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (10/10), done.
Writing objects: 100% (10/10), 3.64 KiB, done.
Total 10 (delta 8), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
d2b69d5..f04f4c0 389-ds-base-1.2.11 -> 389-ds-base-1.2.11

commit f04f4c0
Author: Thierry Bordaz tbordaz@redhat.com
Date: Fri Sep 11 18:56:53 2015 +0200

Hi Thierry,

I think according pytest, your patch should look like that.
Please, if you have any question, ask me via email, I'll help you as I can.

Thank you,
Simon

Fixing coverity issue

== master ==

git push origin master
Counting objects: 13, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (7/7), done.
Writing objects: 100% (7/7), 608 bytes, done.
Total 7 (delta 5), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
1a58bf8..a8130ab master -> master

commit a8130ab
Author: Thierry Bordaz tbordaz@redhat.com
Date: Fri Sep 18 18:38:19 2015 +0200

== 1.3.4 ==

git push origin 389-ds-base-1.3.4
Counting objects: 13, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (7/7), done.
Writing objects: 100% (7/7), 613 bytes, done.
Total 7 (delta 5), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
6343e4c..8cd4f45 389-ds-base-1.3.4 -> 389-ds-base-1.3.4

commit 8cd4f45
Author: Thierry Bordaz tbordaz@redhat.com
Date: Fri Sep 18 18:38:19 2015 +0200

== 1.3.3 ==

git push origin 389-ds-base-1.3.3
Counting objects: 13, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (7/7), done.
Writing objects: 100% (7/7), 614 bytes, done.
Total 7 (delta 5), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
38e0d75..25cbb79 389-ds-base-1.3.3 -> 389-ds-base-1.3.3

commit 25cbb79
Author: Thierry Bordaz tbordaz@redhat.com
Date: Fri Sep 18 18:38:19 2015 +0200

== 1.2.11 ==

git push origin 389-ds-base-1.2.11
Counting objects: 13, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (7/7), done.
Writing objects: 100% (7/7), 617 bytes, done.
Total 7 (delta 5), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
f04f4c0..8d4e08a 389-ds-base-1.2.11 -> 389-ds-base-1.2.11

commit 8d4e08a
Author: Thierry Bordaz tbordaz@redhat.com
Date: Fri Sep 18 18:38:19 2015 +0200

Hi Mark,

Thanks for catching this double free.
The fix looks good. ACK.

Note: I was looking when the entry is freed.
As soon as 'op_shared_add' is called, the entry in the pblock will be freed.
And 'op_shared_add' is not called if:
* LDAP_PARAM_ERROR in add_internal_pb
* LDAP_UNWILLING_TO_PERFORM in slapi_add_internal_pb

The problem is that 'op_shared_add' may also return those result values, so we can not rely on those values (LDAP_PARAM_ERROR and LDAP_UNWILLING_TO_PERFOMR) to free the entry.

8212a89..5538bac master -> master
commit 5538bac
Author: Mark Reynolds mreynolds@redhat.com
Date: Tue Sep 22 09:49:12 2015 -0400

a215c00..1c127b4 389-ds-base-1.3.4 -> 389-ds-base-1.3.4
commit 1c127b4

25cbb79..c28b52f 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit c28b52f

8d4e08a..3896e68 389-ds-base-1.2.11 -> 389-ds-base-1.2.11
commit 3896e68

Replying to [comment:27 tbordaz]:

Hi Mark,

Note: I was looking when the entry is freed.
As soon as 'op_shared_add' is called, the entry in the pblock will be freed.
And 'op_shared_add' is not called if:
* LDAP_PARAM_ERROR in add_internal_pb
* LDAP_UNWILLING_TO_PERFORM in slapi_add_internal_pb

The problem is that 'op_shared_add' may also return those result values, so we can not rely on those values (LDAP_PARAM_ERROR and LDAP_UNWILLING_TO_PERFOMR) to free the entry.

So perhaps we need a new ticket so that op_shared_add() frees the entry when these errors occur? The comments in the code say that the entry is supposed to be consumed. The caller should not have to worry about freeing the entry:

{{{
/ Note: Passed entry e is going to be consumed. /
/ Initialize a pblock for a call to slapi_add_internal_pb() /
void slapi_add_entry_internal_set_pb (Slapi_PBlock pb, Slapi_Entry e, LDAPControl
controls, Slapi_ComponentId plugin_identity,
int operation_flags)
}}}

Replying to [comment:27 tbordaz]:

Note: I was looking when the entry is freed.
As soon as 'op_shared_add' is called, the entry in the pblock will be freed.
And 'op_shared_add' is not called if:
* LDAP_PARAM_ERROR in add_internal_pb

This only happens when e is already NULL -so no free is needed

  • LDAP_UNWILLING_TO_PERFORM in slapi_add_internal_pb

This seems to be the only case where the free was missing. I'm going to handle this missing free in ticket: https://fedorahosted.org/389/ticket/48284, then I'll adjust this ticket to remove the slapi_entry_free() call all together.

622be8b..e5d9b0c master -> master
commit e5d9b0c
Author: Mark Reynolds mreynolds@redhat.com
Date: Tue Sep 22 13:58:38 2015 -0400

99dbfb7..f95e73f 389-ds-base-1.3.4 -> 389-ds-base-1.3.4
commit f95e73f

61f3a05..77c8001 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit 77c8001

82b4347..17834f9 389-ds-base-1.2.11 -> 389-ds-base-1.2.11
commit 17834f9

git patch file (1.2.11 only) - setting replica_object in prp_total
0001-Ticket-48266-1.2.11-only-Fractional-replication-eval.patch

Hi Noriko,

Thank you soooo much for catching this issue. I was still failing to reproduce with valgrind.
The fix is good and I also tested it with the freeipa use case.

ACK

Reviewed by Thierry (Thank you!!)

Pushed to 389-ds-base-1.2.11:
fb94767..1e7e99d 389-ds-base-1.2.11 -> 389-ds-base-1.2.11
commit 1e7e99d

== Master (CI test case) ==

git push origin master
Counting objects: 8, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (5/5), done.
Writing objects: 100% (5/5), 3.91 KiB, done.
Total 5 (delta 3), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
5b33c78..9c84b93 master -> master

Hi,

seems this bug has been fixed for replication agreements but not for sync agreements where we are seeing the same issue.

In ipa windows sync is also defined as fractional replication filtering same attrs.

nsDS5ReplicatedAttributeList: (objectclass=*) $ EXCLUDE memberof idnssoaserial
entryusn krblastsuccessfulauth krblastfailedauth krbloginfailedcount

It could be nice to fix this also in windows. We are seeing the skipping messages. Could we check that but is also hit in sync agreements ?

I agree the same issue can exist DS --> AD. if winsync agreeemnt is skipping many updates (because of fractional filter) a same CSN will be evaluated several times.

Note that if an other agreement exist DS --> DS and hits the same issue (skipping updates), it will update the keep alive entry and that should also fix the problem on DS --> AD. The first RA that updates the keep alive entry, helps all the others RA.

It would be good to open a separated ticket to "port" this fix on WinSync and to provide some replication logs to confirm it also happens in front of AD.

Hi. I agree that only is reproducible in DS -> AD with no more replicas than these two ones.

I have opened internal bz: 1337630

And I will create a new upstream bug.

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

7 years ago

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/1597

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.

Metadata Update from @spichugi:
- Issue close_status updated to: wontfix (was: Fixed)

3 years ago

Login to comment on this ticket.

Metadata