From e2ed3561d3367b2506b5329c1820c98841e0ff81 Mon Sep 17 00:00:00 2001 From: tbordaz Date: Wed, 29 Nov 2023 13:46:57 +0100 Subject: [PATCH] Issue 5944 - Reversion of the entry cache should be limited to BETXN plugin failures (#5994) Bug description: During an update if an BETXN plugin fails the full TXN is aborted and the DB returns to the previous state. However potential internal updates, done by BETXN plugins, are also applied on the entry cache. Some entries in the entry cache are left in a state that does not reflect the DB state. To prevent this mismatch, upon BETXN failure, the fix https://pagure.io/389-ds-base/issue/50260 reverts some entries in the entry cache . The problem is that reversion is not limited to the cases of BETXN failures that was the initial goal. So a "regular" error like schema violation could trigger revert_cache Fix description: The fix flags if the failure is due to BETXN failures and trigger revert_cache only in that case relates: #5944 Reviewed by: Pierre Rogier (Thanks!) --- dirsrvtests/tests/suites/betxns/betxn_test.py | 88 +++++++++++++++++++ ldap/servers/slapd/back-ldbm/cache.c | 10 +++ ldap/servers/slapd/back-ldbm/ldbm_add.c | 10 ++- ldap/servers/slapd/back-ldbm/ldbm_delete.c | 14 ++- ldap/servers/slapd/back-ldbm/ldbm_modify.c | 8 +- ldap/servers/slapd/back-ldbm/ldbm_modrdn.c | 12 ++- 6 files changed, 137 insertions(+), 5 deletions(-) diff --git a/dirsrvtests/tests/suites/betxns/betxn_test.py b/dirsrvtests/tests/suites/betxns/betxn_test.py index b5ca01047f..d40fbafd73 100644 --- a/dirsrvtests/tests/suites/betxns/betxn_test.py +++ b/dirsrvtests/tests/suites/betxns/betxn_test.py @@ -8,6 +8,7 @@ # import pytest import ldap +import os from lib389.tasks import * from lib389.utils import * from lib389.topologies import topology_st @@ -15,6 +16,7 @@ MemberOfPlugin, ManagedEntriesPlugin, ReferentialIntegrityPlugin, MEPTemplates, MEPConfigs) +from lib389.plugins import MemberOfPlugin from lib389.idm.user import UserAccounts, TEST_USER_PROPERTIES from lib389.idm.organizationalunit import OrganizationalUnits from lib389.idm.group import Groups, Group @@ -356,6 +358,92 @@ def test_ri_and_mep_cache_corruption(topology_st): # Success log.info("Test PASSED") +def test_revert_cache(topology_st, request): + """Tests that reversion of the entry cache does not occur + during 'normal' failure (like schema violation) but only + when a plugin fails + + :id: a2361285-b939-4da0-aa80-7fc54d12c981 + + :setup: Standalone instance + + :steps: + 1. Create a user account (with a homeDirectory) + 2. Remove the error log file + 3. Add a second value of homeDirectory + 4. Check that error log does not contain entry cache reversion + 5. Configure memberof to trigger a failure + 6. Do a update the will fail in memberof plugin + 7. Check that error log does contain entry cache reversion + + :expectedresults: + 1. Succeeds + 2. Fails with OBJECT_CLASS_VIOLATION + 3. Succeeds + 4. Succeeds + 5. Succeeds + 6. Fails with OBJECT_CLASS_VIOLATION + 7. Succeeds + """ + # Create an test user entry + log.info('Create a user without nsMemberOF objectclass') + try: + users = UserAccounts(topology_st.standalone, DEFAULT_SUFFIX, rdn='ou=people') + user = users.create_test_user() + user.replace('objectclass', ['top', 'account', 'person', 'inetorgperson', 'posixAccount', 'organizationalPerson', 'nsAccount']) + except ldap.LDAPError as e: + log.fatal('Failed to create test user: error ' + e.args[0]['desc']) + assert False + + # Remove the current error log file + topology_st.standalone.stop() + lpath = topology_st.standalone.ds_error_log._get_log_path() + os.unlink(lpath) + topology_st.standalone.start() + + # + # Add a second value to 'homeDirectory' + # leads to ldap.OBJECT_CLASS_VIOLATION + # And check that the entry cache was not reverted + try: + topology_st.standalone.modify_s(user.dn, [(ldap.MOD_ADD, 'homeDirectory', ensure_bytes('/home/user_new'))]) + assert False + except ldap.OBJECT_CLASS_VIOLATION: + pass + assert not topology_st.standalone.ds_error_log.match('.*WARN - flush_hash - Upon BETXN callback failure, entry cache is flushed during.*') + + # Prepare memberof so that it will fail during a next update + # If memberof plugin can not add 'memberof' to the + # member entry, it retries after adding + # 'memberOfAutoAddOC' objectclass to the member. + # If it fails again the plugin fails with 'object + # violation' + # To trigger this failure, set 'memberOfAutoAddOC' + # to a value that does *not* allow 'memberof' attribute + memberof = MemberOfPlugin(topology_st.standalone) + memberof.enable() + memberof.replace('memberOfAutoAddOC', 'account') + memberof.replace('memberofentryscope', DEFAULT_SUFFIX) + topology_st.standalone.restart() + + # Try to add the user to demo_group + # It should fail because user entry has not 'nsmemberof' objectclass + # As this is a BETXN plugin that fails it should revert the entry cache + try: + GROUP_DN = "cn=demo_group,ou=groups," + DEFAULT_SUFFIX + topology_st.standalone.modify_s(GROUP_DN, + [(ldap.MOD_REPLACE, 'member', ensure_bytes(user.dn))]) + assert False + except ldap.OBJECT_CLASS_VIOLATION: + pass + assert topology_st.standalone.ds_error_log.match('.*WARN - flush_hash - Upon BETXN callback failure, entry cache is flushed during.*') + + def fin(): + user.delete() + memberof = MemberOfPlugin(topology_st.standalone) + memberof.replace('memberOfAutoAddOC', 'nsmemberof') + + request.addfinalizer(fin) if __name__ == '__main__': # Run isolated diff --git a/ldap/servers/slapd/back-ldbm/cache.c b/ldap/servers/slapd/back-ldbm/cache.c index b68a0cce85..b62bab0af3 100644 --- a/ldap/servers/slapd/back-ldbm/cache.c +++ b/ldap/servers/slapd/back-ldbm/cache.c @@ -531,7 +531,12 @@ flush_hash(struct cache *cache, struct timespec *start_time, int32_t type) { Hashtable *ht = cache->c_idtable; /* start with the ID table as it's in both ENTRY and DN caches */ void *e, *laste = NULL; + char flush_etime[ETIME_BUFSIZ] = {0}; + struct timespec duration; + struct timespec flush_start; + struct timespec flush_end; + clock_gettime(CLOCK_MONOTONIC, &flush_start); cache_lock(cache); for (size_t i = 0; i < ht->size; i++) { @@ -609,6 +614,11 @@ flush_hash(struct cache *cache, struct timespec *start_time, int32_t type) } cache_unlock(cache); + + clock_gettime(CLOCK_MONOTONIC, &flush_end); + slapi_timespec_diff(&flush_end, &flush_start, &duration); + snprintf(flush_etime, ETIME_BUFSIZ, "%" PRId64 ".%.09" PRId64 "", (int64_t)duration.tv_sec, (int64_t)duration.tv_nsec); + slapi_log_err(SLAPI_LOG_WARNING, "flush_hash", "Upon BETXN callback failure, entry cache is flushed during %s\n", flush_etime); } void diff --git a/ldap/servers/slapd/back-ldbm/ldbm_add.c b/ldap/servers/slapd/back-ldbm/ldbm_add.c index e3d18bab3b..11064b7f3b 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_add.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_add.c @@ -98,6 +98,7 @@ ldbm_back_add(Slapi_PBlock *pb) int op_id; int result_sent = 0; int32_t parent_op = 0; + int32_t betxn_callback_fails = 0; /* if a BETXN fails we need to revert entry cache */ struct timespec parent_time; if (slapi_pblock_get(pb, SLAPI_CONN_ID, &conn_id) < 0) { @@ -919,6 +920,9 @@ ldbm_back_add(Slapi_PBlock *pb) slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message); slapi_log_err(SLAPI_LOG_DEBUG, "ldbm_back_add", "SLAPI_PLUGIN_BE_TXN_PRE_ADD_FN plugin failed: %d\n", ldap_result_code ? ldap_result_code : retval); + if (retval) { + betxn_callback_fails = 1; + } goto error_return; } } @@ -1229,11 +1233,15 @@ ldbm_back_add(Slapi_PBlock *pb) slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, ldap_result_code ? &ldap_result_code : &retval); } slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message); + if (retval) { + betxn_callback_fails = 1; + } goto error_return; } retval = plugin_call_mmr_plugin_postop(pb, NULL,SLAPI_PLUGIN_BE_TXN_POST_ADD_FN); if (retval) { + betxn_callback_fails = 1; ldbm_set_error(pb, retval, &ldap_result_code, &ldap_result_message); goto error_return; } @@ -1257,7 +1265,7 @@ ldbm_back_add(Slapi_PBlock *pb) error_return: /* Revert the caches if this is the parent operation */ - if (parent_op) { + if (parent_op && betxn_callback_fails) { revert_cache(inst, &parent_time); } if (addingentry_id_assigned) { diff --git a/ldap/servers/slapd/back-ldbm/ldbm_delete.c b/ldap/servers/slapd/back-ldbm/ldbm_delete.c index e187e91253..a50e874c4b 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_delete.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_delete.c @@ -80,6 +80,7 @@ ldbm_back_delete(Slapi_PBlock *pb) int result_sent = 0; Connection *pb_conn; int32_t parent_op = 0; + int32_t betxn_callback_fails = 0; /* if a BETXN fails we need to revert entry cache */ struct timespec parent_time; if (slapi_pblock_get(pb, SLAPI_CONN_ID, &conn_id) < 0) { @@ -434,6 +435,9 @@ ldbm_back_delete(Slapi_PBlock *pb) &ldap_result_code : &retval ); } slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message); + if (retval) { + betxn_callback_fails = 1; + } goto error_return; } } @@ -717,6 +721,9 @@ ldbm_back_delete(Slapi_PBlock *pb) ldap_result_code ? &ldap_result_code : &retval); } slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message); + if (retval) { + betxn_callback_fails = 1; + } goto error_return; } } @@ -746,6 +753,9 @@ ldbm_back_delete(Slapi_PBlock *pb) } /* retval is -1 */ slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message); + if (rc) { + betxn_callback_fails = 1; + } goto error_return; } slapi_pblock_set(pb, SLAPI_DELETE_BEPREOP_ENTRY, orig_entry); @@ -1266,6 +1276,7 @@ ldbm_back_delete(Slapi_PBlock *pb) slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, &retval); } slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message); + betxn_callback_fails = 1; goto error_return; } if (parent_found) { @@ -1281,6 +1292,7 @@ ldbm_back_delete(Slapi_PBlock *pb) retval = plugin_call_mmr_plugin_postop(pb, NULL,SLAPI_PLUGIN_BE_TXN_POST_DELETE_FN); if (retval) { + betxn_callback_fails = 1; ldbm_set_error(pb, retval, &ldap_result_code, &ldap_result_message); goto error_return; } @@ -1353,7 +1365,7 @@ ldbm_back_delete(Slapi_PBlock *pb) error_return: /* Revert the caches if this is the parent operation */ - if (parent_op) { + if (parent_op && betxn_callback_fails) { revert_cache(inst, &parent_time); } diff --git a/ldap/servers/slapd/back-ldbm/ldbm_modify.c b/ldap/servers/slapd/back-ldbm/ldbm_modify.c index 3fd5733377..58c66b486c 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_modify.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_modify.c @@ -522,6 +522,7 @@ ldbm_back_modify(Slapi_PBlock *pb) int ec_locked = 0; int result_sent = 0; int32_t parent_op = 0; + int32_t betxn_callback_fails = 0; /* if a BETXN fails we need to revert entry cache */ struct timespec parent_time; Slapi_Mods *smods_add_rdn = NULL; @@ -817,6 +818,9 @@ ldbm_back_modify(Slapi_PBlock *pb) slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, ldap_result_code ? &ldap_result_code : &retval); } slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message); + if (retval) { + betxn_callback_fails = 1; + } goto error_return; } @@ -1017,10 +1021,12 @@ ldbm_back_modify(Slapi_PBlock *pb) slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, ldap_result_code ? &ldap_result_code : &retval); } slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message); + betxn_callback_fails = 1; goto error_return; } retval = plugin_call_mmr_plugin_postop(pb, NULL,SLAPI_PLUGIN_BE_TXN_POST_MODIFY_FN); if (retval) { + betxn_callback_fails = 1; ldbm_set_error(pb, retval, &ldap_result_code, &ldap_result_message); goto error_return; } @@ -1041,7 +1047,7 @@ ldbm_back_modify(Slapi_PBlock *pb) error_return: /* Revert the caches if this is the parent operation */ - if (parent_op) { + if (parent_op && betxn_callback_fails) { revert_cache(inst, &parent_time); } diff --git a/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c b/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c index 32a8cd6578..a3294e3887 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_modrdn.c @@ -101,6 +101,7 @@ ldbm_back_modrdn(Slapi_PBlock *pb) int result_sent = 0; Connection *pb_conn = NULL; int32_t parent_op = 0; + int32_t betxn_callback_fails = 0; /* if a BETXN fails we need to revert entry cache */ struct timespec parent_time; Slapi_Mods *smods_add_rdn = NULL; @@ -999,6 +1000,9 @@ ldbm_back_modrdn(Slapi_PBlock *pb) slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, ldap_result_code ? &ldap_result_code : &retval); } slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message); + if (retval) { + betxn_callback_fails = 1; + } goto error_return; } @@ -1249,10 +1253,12 @@ ldbm_back_modrdn(Slapi_PBlock *pb) slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, ldap_result_code ? &ldap_result_code : &retval); } slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message); + betxn_callback_fails = 1; goto error_return; } retval = plugin_call_mmr_plugin_postop(pb, NULL,SLAPI_PLUGIN_BE_TXN_POST_MODRDN_FN); if (retval) { + betxn_callback_fails = 1; ldbm_set_error(pb, retval, &ldap_result_code, &ldap_result_message); goto error_return; } @@ -1318,7 +1324,7 @@ ldbm_back_modrdn(Slapi_PBlock *pb) error_return: /* Revert the caches if this is the parent operation */ - if (parent_op) { + if (parent_op && betxn_callback_fails) { revert_cache(inst, &parent_time); } @@ -1400,7 +1406,9 @@ ldbm_back_modrdn(Slapi_PBlock *pb) } slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message); - /* Revert the caches if this is the parent operation */ + /* As it is a BETXN plugin failure then + * revert the caches if this is the parent operation + */ if (parent_op) { revert_cache(inst, &parent_time); }