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!)
389ds · Dec 11, 2023 · e2ed356 · e2ed356
1 parent 5eb779b
commit e2ed356
Show file tree

Hide file tree

Showing 6 changed files with 137 additions and 5 deletions.
diff --git a/dirsrvtests/tests/suites/betxns/betxn_test.py b/dirsrvtests/tests/suites/betxns/betxn_test.py
@@ -8,13 +8,15 @@
 #
 import pytest
 import ldap
+import os
 from lib389.tasks import *
 from lib389.utils import *
 from lib389.topologies import topology_st
 from lib389.plugins import (SevenBitCheckPlugin, AttributeUniquenessPlugin,
                             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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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);
                 }