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.
attachment ticket48266_test.py
Test case attached
Ticket has been cloned to Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1259949
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.
attachment 0001-Ticket-48266-Fractional-replication-evaluates-severa.patch
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.
attachment 0001-Ticket-48266-1-Fractional-replication-evaluates-severa.patch
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!
attachment 0001-Ticket-48266-2-Fractional-replication-evaluates-severa.patch
attachment 0001-Ticket-48266-test-case.patch
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
I opened https://fedorahosted.org/389/ticket/48278 to track this improvement
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: {{{
@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.
attachment 0001-Ticket-48266-3-Fractional-replication-evaluates-severa.patch
attachment 0001-Ticket-48266-2-test-case.patch
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
attachment 0001-Ticket-48266-4-Fractional-replication-evaluates-severa.patch
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
attachment 0001-Ticket-48266-test-case.2.patch
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.
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
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
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
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
Fix crash during online init of dedicated consumer 0001-Ticket-48266-Online-init-crashes-consumer.patch
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) }}}
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.
remove slapi_entry_free() 0001-Ticket-48266-do-not-free-repl-keep-alive-entry-on-er.patch
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
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.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Metadata Update from @spichugi: - Issue close_status updated to: wontfix (was: Fixed)
Login to comment on this ticket.