replication conflicts are created mostly because of failures in manually applying updates on different servers or client applications. But once they are created many clients, applications and users have problems to understand and handle them properly. In most cases the "original" and "conflict" entries are identical, but there exist scenarios where both entries could have (different) children before the conflict is created, so they cannot be completely hidden or ignored.
There are several proposals to handle this, the latest: http://www.port389.org/docs/389ds/design/managing-repl-conflict-entries.html
which also discusses teh other proposals and open issues with conflict entries.
This ticket should be used to evaluate the suggested solutions and implement one, making the otehr tickets obsolete.
attachment 0001-ticket-49043-initial-fix.patch
Metadata Update from @lkrispen: - Issue set to the milestone: 1.3.6.0
<img alt="0001-Ticket-49050-make-objectclass-ldapsubentry-effective.patch" src="/389-ds-base/issue/raw/files/5aee460b095691a2007e620f88657e25c8da87c8f2d93a23d5dd1ecbef742521-0001-Ticket-49050-make-objectclass-ldapsubentry-effective.patch" />
Metadata Update from @lkrispen: - Custom field component reset - Custom field reviewstatus adjusted to new - Issue close_status updated to: None
<img alt="0002-fix-incorrect-behaviour-update-resolution.patch" src="/389-ds-base/issue/raw/files/47ac39a1387c4e623f72dd4a0921ad5c64f187af33fd5f68986d9192a998b7d3-0002-fix-incorrect-behaviour-update-resolution.patch" />
<img alt="0003-ticket-49043-manage-replication-conflicts.patch" src="/389-ds-base/issue/raw/files/88465a2f65f773e96aed88d11f3f1ffc28f5e4174655757d5bc8721be88e07f6-0003-ticket-49043-manage-replication-conflicts.patch" />
<img alt="0004-hide-cenotaphs-from-retro-cl.patch" src="/389-ds-base/issue/raw/files/71bc51b42891bf00e0168717562206b60e6a710486252eaa86b4d999f8fa93a1-0004-hide-cenotaphs-from-retro-cl.patch" />
I attached a set of patches reflecting the current state of work, The CI test will follow and an updated design doc as well.
With these patches conflict management hides conflcit entries, produces consistent result for valid and conflict entries and works for memberof, managed entries plugins.
It hides the cenotpaphs fro the retor cl, but the conflict entries and operations on them are still seen in the retro cl and plugins are still called for the internal operations managing the conflict entries
internal bz created:
https://bugzilla.redhat.com/show_bug.cgi?id=1437887
here is a complete new set of patches
Metadata Update from @lkrispen: - Custom field reviewstatus reset (from new)
<img alt="0004-ticket-49043-part4-fix-errors-and-leaks-reported-by-.patch" src="/389-ds-base/issue/raw/77b134c6f3595ceff13f15100cfad7268eeaecdb41250bb2539595079bba8886-0004-ticket-49043-part4-fix-errors-and-leaks-reported-by-.patch" />
<img alt="0001-ticket-49043-part1-manage-replication-conflicts.patch" src="/389-ds-base/issue/raw/909764b4533245bfed8f7620ba4df67b761c3ec8f2ce36b4801b04eec582567f-0001-ticket-49043-part1-manage-replication-conflicts.patch" />
<img alt="0002-ticket-49043-part2-factor-out-mmr-repl-plugin-from-g.patch" src="/389-ds-base/issue/raw/d89d2dcb9d7e2bf1af832de22c0db43a0bc5551d5315119600a4aa76c8c6d47f-0002-ticket-49043-part2-factor-out-mmr-repl-plugin-from-g.patch" />
<img alt="0003-ticket-49043-part3-hanle-complex-scenarios.patch" src="/389-ds-base/issue/raw/158ac556e45e5f44a08d6f2d7e3a650cd3865b0bd02605a4bbaa416ea20cef3f-0003-ticket-49043-part3-hanle-complex-scenarios.patch" />
<img alt="ticket49043_1_test.py" src="/389-ds-base/issue/raw/00571921ea43decbc7d643449783871aefe323fd3c314427161fb228dc22a463-ticket49043_1_test.py" />
<img alt="ticket49043_2_test.py" src="/389-ds-base/issue/raw/d67802efd3d3623d2ec5cda2de79b395c4905c85aab6ff4ff78b4d876d20ccfc-ticket49043_2_test.py" />
<img alt="check-db.sh" src="/389-ds-base/issue/raw/885ee5b2aa7c5bb54dcbb831bf312e8274d1636c59d261c9fec62fa91d1d4ec7-check-db.sh" />
The design document is a great reading. Thanks !!!
Looking at the case where an entry with children becomes a conflict. Don't you think that urp_rename_conflict_children should search search (|(objectclass=ldapsubentry)(objectclass=*)), in case some children are subentries.
I understand how an existing entry becomes a conflict and its children are moved to the new added valid entry. When this is the opposite, a new added entry becomes a conflict. Where are added its children, under the old valid entry ? Is it the reason why there is no specific code to handle that case ?
Continuing the review...
The fix looks good, but it's a lot to digest. :-) One thing I noticed is that now it appears we issue an internal search operation for every update (via urp). Could this have an impact on replication/update performance? Perhaps we run run some of the replication tests, and do timing comparisons from before and after the fix is applied. We should also run the CI test suite to make sure there are no regressions around the entry cache, and replication.
Also, mainly in the urp code, you added a bunch of logging that uses SLAPI_LOG_ERR, but it would appear that it should probably be SLAPI_LOG_REPL.
As previously mentioned there is a ton of indentation issues, trailing white space, mixing spaces & tabs, spelling errors, etc.
There are also a lot of compiler warnings that should be fixed:
../389-ds-base/ldap/servers/slapd/plugin_mmr.c: In function ‘plugin_call_mmr_plugin_postop’: ../389-ds-base/ldap/servers/slapd/plugin_mmr.c:52:8: warning: unused variable ‘mmrplugin_initialized’ [-Wunused-variable] int mmrplugin_initialized = 0; ^~~~~~~~~~~~~~~~~~~~~ ../389-ds-base/ldap/servers/plugins/replication/urp.c: In function ‘urp_modrdn_operation’: ../389-ds-base/ldap/servers/plugins/replication/urp.c:345:50: warning: passing argument 3 of ‘tombstone_to_glue’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] op_result = tombstone_to_glue (pb, sessionid, target_entry, ^~~~~~~~~~~~ In file included from ../389-ds-base/ldap/servers/plugins/replication/urp.c:22:0: ../389-ds-base/ldap/servers/plugins/replication/urp.h:56:5: note: expected ‘Slapi_Entry * {aka struct slapi_entry *}’ but argument is of type ‘const Slapi_Entry * {aka const struct slapi_entry *}’ int tombstone_to_glue(Slapi_PBlock *pb, char *sessionid, Slapi_Entry *entry, const Slapi_DN *parentdn, const char *reason, CSN *opcsn, Slapi_DN **newparentdn); ^~~~~~~~~~~~~~~~~ ../389-ds-base/ldap/servers/plugins/replication/urp.c: In function ‘urp_delete_operation’: ../389-ds-base/ldap/servers/plugins/replication/urp.c:620:52: warning: passing argument 2 of ‘urp_delete_check_conflict’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] } else if (urp_delete_check_conflict (sessionid, deleteentry, opcsn)) { ^~~~~~~~~~~ ../389-ds-base/ldap/servers/plugins/replication/urp.c:28:12: note: expected ‘Slapi_Entry * {aka struct slapi_entry *}’ but argument is of type ‘const Slapi_Entry * {aka const struct slapi_entry *}’ static int urp_delete_check_conflict (char *sessionid, Slapi_Entry *tombstone_entry, CSN *opcsn); ^~~~~~~~~~~~~~~~~~~~~~~~~ ../389-ds-base/ldap/servers/plugins/replication/urp.c:658:57: warning: passing argument 1 of ‘slapi_entry_get_sdn’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] if (0 == slapi_sdn_compare(sdn, slapi_entry_get_sdn(deleteentry))) { ^~~~~~~~~~~ In file included from ../389-ds-base/ldap/servers/plugins/replication/urp.c:19:0: ../389-ds-base/ldap/servers/slapd/slapi-plugin.h:1340:11: note: expected ‘Slapi_Entry * {aka struct slapi_entry *}’ but argument is of type ‘const Slapi_Entry * {aka const struct slapi_entry *}’ Slapi_DN *slapi_entry_get_sdn( Slapi_Entry *e ); ^~~~~~~~~~~~~~~~~~~ ../389-ds-base/ldap/servers/plugins/replication/urp.c:674:36: warning: passing argument 2 of ‘is_renamed_entry’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] } else if (is_renamed_entry(pb, deleteentry, opcsn)) { ^~~~~~~~~~~ ../389-ds-base/ldap/servers/plugins/replication/urp.c:40:12: note: expected ‘Slapi_Entry * {aka struct slapi_entry *}’ but argument is of type ‘const Slapi_Entry * {aka const struct slapi_entry *}’ static int is_renamed_entry ( Slapi_PBlock *pb, Slapi_Entry *entry, CSN *opcsn); ^~~~~~~~~~~~~~~~ ../389-ds-base/ldap/servers/plugins/replication/urp.c:696:34: warning: passing argument 1 of ‘urp_rename_conflict_children’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] urp_rename_conflict_children(slapi_entry_get_dn_const(deleteentry), ^~~~~~~~~~~~~~~~~~~~~~~~ ../389-ds-base/ldap/servers/plugins/replication/urp.c:45:12: note: expected ‘char *’ but argument is of type ‘const char *’ static int urp_rename_conflict_children(char *old_parent, const Slapi_DN *new_parent); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../389-ds-base/ldap/servers/plugins/replication/urp.c: In function ‘urp_get_min_naming_conflict_entry’: ../389-ds-base/ldap/servers/plugins/replication/urp.c:1956:10: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] basedn = collisiondn; ^ At top level: ../389-ds-base/ldap/servers/plugins/replication/urp.c:1806:1: warning: ‘urp_get_conflict_dn_for_nsuniqueid’ defined but not used [-Wunused-function] urp_get_conflict_dn_for_nsuniqueid ( char *nsuniqueid, Slapi_DN *parentdn ) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../389-ds-base/ldap/servers/plugins/replication/urp_tombstone.c: In function ‘tombstone_to_conflict_check_parent’: ../389-ds-base/ldap/servers/plugins/replication/urp_tombstone.c:124:20: warning: initialization discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] char *newrdn = slapi_rdn_get_rdn(rdn); ^~~~~~~~~~~~~~~~~
Rather than int, try to use int32_t or int64_t. Because int is an undefined size type, in the future to be stricter we should be using the fixed width specifiers. In some cases you may consider int_fast32_t, which is an int that holds up to 32bits, but is the platforms "fastest" available type (ie on x86_64 it's an int64_t.). Better yet, just use int64_t.
As mark said, there is some whitespace, issues, but I think I would add, for all "new" code, just use soft spaces.
740 +» for (int i = 0; entries && (entries[i] != NULL); i++) {
Platforms index arrays by size_t, not int. So please check these locations and update it.
In urp_annotate_dn, you have:
if (conflict_dn) *conflict_dn = NULL;
I'm assuming this doesn't leak memory today, but later you have an "if conflict_dn". Wouldn't this always be false since you force it to NULL?
urp_get_valid_parent_nsuniqueid you don't need
981 +» newpb = NULL;
Because the function is abotu to return, setting this to NULL is a no-op. You might want to check for similar un-neccesarry NULLing.
1152 +is_conflict_entry(const Slapi_Entry *entry)
This seems like a candidate for entry.c, where you have a "slapi_entry_is_conflict(Slapi_Entry *e)"
This way, you save on the alloc, copy, and free, when you really just need to check an attr internal to the entries content.
116 +» rc = urp_add_check_tombstone(pb, sessionid, addentry, opcsn);
Rather than havinc rc == 1, or rc == 2, you could consider an enum for this return code. I have some examples of this in the code.
Otherwise, I think that it looks pretty good. Your document covers many of the places that I had questions, and it makes it clearer to see what's happening in URP. I think while we have the current code in a plugin it will always be a challenge to really make it flow nicely.
Metadata Update from @mreynolds: - Issue set to the milestone: 1.3.7.0 (was: 1.3.6.0)
<img alt="0001-Ticket-49043-make-replication-conflicts-transparent-.patch" src="/389-ds-base/issue/raw/files/10df6642b83e2cfc3f82543e5de1c69ce076b3fa71650c7ab0935f794f85d47e-0001-Ticket-49043-make-replication-conflicts-transparent-.patch" />
Metadata Update from @lkrispen: - Custom field component adjusted to None - Custom field reviewstatus adjusted to None
This latest patch is the cumulative patch of previous patches. It contains the changes required by reviewers. I will commit it upstream to get more exposure to harden it for release in 7.5
committed to master
To ssh://git@pagure.io/389-ds-base.git 5d82589..f63949d master -> master
Metadata Update from @lkrispen: - Issue close_status updated to: fixed - Issue status updated to: Closed (was: Open)
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/2102
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.