From 55bc2b147c1bd72d2b0feed59820aa8fdcbcfc25 Mon Sep 17 00:00:00 2001 From: Thierry Bordaz Date: Tue, 28 Nov 2023 18:26:13 +0100 Subject: [PATCH] Issue 5944 - Reversion of the entry cache should be limited to BETXN plugin failures 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: --- dirsrvtests/tests/suites/betxns/betxn_test.py | 88 +++++++++++++++++++ ldap/servers/slapd/back-ldbm/cache.c | 10 +++ ldap/servers/slapd/back-ldbm/ldbm_add.c | 16 +++- ldap/servers/slapd/back-ldbm/ldbm_delete.c | 18 +++- ldap/servers/slapd/back-ldbm/ldbm_modify.c | 12 ++- ldap/servers/slapd/back-ldbm/ldbm_modrdn.c | 11 ++- 6 files changed, 150 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 6d6b6864be..604c0d77d9 100644 --- a/ldap/servers/slapd/back-ldbm/cache.c +++ b/ldap/servers/slapd/back-ldbm/cache.c @@ -556,7 +556,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++) { @@ -638,6 +643,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 27e9be748c..ed0c126f83 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; } @@ -1326,9 +1334,15 @@ ldbm_back_add(Slapi_PBlock *pb) slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, &opreturn); } slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message); + if (retval) { + betxn_callback_fails = 1; + } } /* the repl postop needs to be called for aborted operations */ retval = plugin_call_mmr_plugin_postop(pb, NULL,SLAPI_PLUGIN_BE_TXN_POST_ADD_FN); + if (retval) { + betxn_callback_fails = 1; + } if (addingentry) { if (inst && cache_is_in_cache(&inst->inst_cache, addingentry)) { CACHE_REMOVE(&inst->inst_cache, addingentry); @@ -1366,7 +1380,7 @@ ldbm_back_add(Slapi_PBlock *pb) } /* 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_delete.c b/ldap/servers/slapd/back-ldbm/ldbm_delete.c index 2542ff7eb0..5f07121ab4 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; } @@ -1416,8 +1428,12 @@ ldbm_back_delete(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; } retval = plugin_call_mmr_plugin_postop(pb, NULL,SLAPI_PLUGIN_BE_TXN_POST_DELETE_FN); + if (retval) { + betxn_callback_fails = 1; + } /* Release SERIAL LOCK */ dblayer_txn_abort(be, &txn); /* abort crashes in case disk full */ @@ -1437,7 +1453,7 @@ ldbm_back_delete(Slapi_PBlock *pb) } /* 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 4d86472237..1b5c1fecdc 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; } @@ -1083,8 +1089,12 @@ ldbm_back_modify(Slapi_PBlock *pb) if (!opreturn) { slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, ldap_result_code ? &ldap_result_code : &retval); } + betxn_callback_fails = 1; } retval = plugin_call_mmr_plugin_postop(pb, NULL,SLAPI_PLUGIN_BE_TXN_POST_MODIFY_FN); + if (retval) { + betxn_callback_fails = 1; + } /* It is safer not to abort when the transaction is not started. */ /* Release SERIAL LOCK */ @@ -1096,7 +1106,7 @@ ldbm_back_modify(Slapi_PBlock *pb) rc = SLAPI_FAIL_GENERAL; } /* 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 c5e9be4aa6..c98059ff27 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; } @@ -1250,10 +1254,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; } @@ -1319,7 +1325,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,9 +1406,10 @@ 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; /* Revert the caches if this is the parent operation */ - if (parent_op) { + if (parent_op && betxn_callback_fails) { revert_cache(inst, &parent_time); } }