#50286 Ticket 50260 - Invalid cache flushing improvements
Closed 3 years ago by spichugi. Opened 5 years ago by mreynolds.
mreynolds/389-ds-base ticket50260  into  master

@@ -57,24 +57,20 @@ 

      user = users.create(properties=TEST_USER_PROPERTIES)

  

      # Attempt a modrdn, this should fail

- 

-     try:

+     with pytest.raises(ldap.LDAPError):

          user.rename(BAD_RDN)

-         log.fatal('test_betxt_7bit: Modrdn operation incorrectly succeeded')

-         assert False

-     except ldap.LDAPError as e:

-         log.info('Modrdn failed as expected: error %s' % str(e))

  

      # Make sure the operation did not succeed, attempt to search for the new RDN

+     with pytest.raises(ldap.LDAPError):

+         users.get(u'Fu\u00c4\u00e8')

  

+     # Make sure original entry is present

      user_check = users.get("testuser")

- 

      assert user_check.dn.lower() == user.dn.lower()

  

-     #

      # Cleanup - remove the user

-     #

      user.delete()

+ 

      log.info('test_betxt_7bit: PASSED')

  

  
@@ -153,17 +149,11 @@ 

      memberof = MemberOfPlugin(topology_st.standalone)

      memberof.enable()

      memberof.set_autoaddoc('referral')

-     # memberof.add_groupattr('member') # This is already the default.

      topology_st.standalone.restart()

  

      groups = Groups(topology_st.standalone, DEFAULT_SUFFIX)

-     group1 = groups.create(properties={

-         'cn': 'group1',

-     })

- 

-     group2 = groups.create(properties={

-         'cn': 'group2',

-     })

+     group1 = groups.create(properties={'cn': 'group1'})

+     group2 = groups.create(properties={'cn': 'group2'})

  

      # We may need to mod groups to not have nsMemberOf ... ?

      if not ds_is_older('1.3.7'):
@@ -171,21 +161,18 @@ 

          group2.remove('objectClass', 'nsMemberOf')

  

      # Add group2 to group1 - it should fail with objectclass violation

-     try:

+     with pytest.raises(ldap.OBJECT_CLASS_VIOLATION):

          group1.add_member(group2.dn)

-         log.fatal('test_betxn_memberof: Group2 was incorrectly allowed to be added to group1')

-         assert False

-     except ldap.LDAPError as e:

-         log.info('test_betxn_memberof: Group2 was correctly rejected (mod add): error: ' + str(e))

  

-     #

+     # verify entry cache reflects the current/correct state of group1

+     assert not group1.is_member(group2.dn)

+ 

      # Done

-     #

      log.info('test_betxn_memberof: PASSED')

  

  

  def test_betxn_modrdn_memberof_cache_corruption(topology_st):

-     """Test modrdn operations and memberOf

+     """Test modrdn operations and memberOf be txn post op failures

  

      :id: 70d0b96e-b693-4bf7-bbf5-102a66ac5994

  
@@ -234,9 +221,7 @@ 

      with pytest.raises(ldap.OBJECT_CLASS_VIOLATION):

          group.rename('cn=group_to_people', newsuperior=peoplebase)

  

-     #

      # Done

-     #

      log.info('test_betxn_modrdn_memberof: PASSED')

  

  
@@ -311,15 +296,23 @@ 

          log.fatal("MEP group was not created for the user")

          assert False

  

+     # Test MEP be txn pre op failure does not corrupt entry cache

+     # Should get the same exception for both rename attempts

+     with pytest.raises(ldap.UNWILLING_TO_PERFORM):

+         mep_group.rename("cn=modrdn group")

+ 

+     with pytest.raises(ldap.UNWILLING_TO_PERFORM):

+         mep_group.rename("cn=modrdn group")

+ 

      # Mess with MEP so it fails

      mep_plugin.disable()

      mep_group.delete()

      mep_plugin.enable()

  

-     # Add another group for verify entry cache is not corrupted

+     # Add another group to verify entry cache is not corrupted

      test_group = groups.create(properties={'cn': 'test_group'})

  

-     # Delete user, should fail, and user should still be a member

+     # Delete user, should fail in MEP be txn post op, and user should still be a member

      with pytest.raises(ldap.NO_SUCH_OBJECT):

          user.delete()

  

@@ -517,7 +517,8 @@ 

  /*

   * Flush all the cache entries that were added after the "start time"

   * This is called when a backend transaction plugin fails, and we need

-  * to remove all the possible invalid entries in the cache.

+  * to remove all the possible invalid entries in the cache.  We need

+  * to check both the ID and DN hashtables when checking the entry cache.

   *

   * If the ref count is 0, we can straight up remove it from the cache, but

   * if the ref count is greater than 1, then the entry is currently in use.
@@ -528,8 +529,8 @@ 

  static void

  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;

-     Hashtable *ht = cache->c_idtable;

  

      cache_lock(cache);

  
@@ -570,6 +571,43 @@ 

          }

      }

  

+     if (type == ENTRY_CACHE) {

+         /* Also check the DN hashtable */

+         ht = cache->c_dntable;

+ 

+         for (size_t i = 0; i < ht->size; i++) {

+             e = ht->slot[i];

+             while (e) {

+                 struct backcommon *entry = (struct backcommon *)e;

+                 uint64_t remove_it = 0;

+                 if (flush_remove_entry(&entry->ep_create_time, start_time)) {

+                     /* Mark the entry to be removed */

+                     slapi_log_err(SLAPI_LOG_CACHE, "flush_hash", "[ENTRY CACHE] Removing entry id (%d)\n",

+                             entry->ep_id);

+                     remove_it = 1;

+                 }

+                 laste = e;

+                 e = HASH_NEXT(ht, e);

+ 

+                 if (remove_it) {

+                     /* since we have the cache lock we know we can trust refcnt */

+                     entry->ep_state |= ENTRY_STATE_INVALID;

+                     if (entry->ep_refcnt == 0) {

+                         entry->ep_refcnt++;

+                         lru_delete(cache, laste);

+                         entrycache_remove_int(cache, laste);

+                         entrycache_return(cache, (struct backentry **)&laste);

+                     } else {

+                         /* Entry flagged for removal */

+                         slapi_log_err(SLAPI_LOG_CACHE, "flush_hash",

+                                 "[ENTRY CACHE] Flagging entry to be removed later: id (%d) refcnt: %d\n",

+                                 entry->ep_id, entry->ep_refcnt);

+                     }

+                 }

+             }

+         }

+     }

+ 

      cache_unlock(cache);

  }

  

@@ -1221,11 +1221,6 @@ 

              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);

- 

-         /* Revert the caches if this is the parent operation */

-         if (parent_op) {

-             revert_cache(inst, &parent_time);

-         }

          goto error_return;

      }

  
@@ -1253,6 +1248,10 @@ 

      goto common_return;

  

  error_return:

+     /* Revert the caches if this is the parent operation */

+     if (parent_op) {

+         revert_cache(inst, &parent_time);

+     }

I agree with the testing in preop but are you sure we hit a cache corruption during a plugin preop ?
Which plugin updates an entry on a preop while it does not know the result of the operation ?

      if (addingentry_id_assigned) {

          next_id_return(be, addingentry->ep_id);

      }

@@ -1279,11 +1279,6 @@ 

              slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, &retval);

          }

          slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message);

- 

-         /* Revert the caches if this is the parent operation */

-         if (parent_op) {

-             revert_cache(inst, &parent_time);

-         }

          goto error_return;

      }

      if (parent_found) {
@@ -1370,6 +1365,11 @@ 

      goto common_return;

  

  error_return:

+     /* Revert the caches if this is the parent operation */

+     if (parent_op) {

+         revert_cache(inst, &parent_time);

+     }

+ 

      if (tombstone) {

          if (cache_is_in_cache(&inst->inst_cache, tombstone)) {

              tomb_ep_id = tombstone->ep_id; /* Otherwise, tombstone might have been freed. */
@@ -1388,6 +1388,7 @@ 

          CACHE_RETURN(&inst->inst_cache, &tombstone);

          tombstone = NULL;

      }

+ 

      if (retval == DB_RUNRECOVERY) {

          dblayer_remember_disk_filled(li);

          ldbm_nasty("ldbm_back_delete", "Delete", 79, retval);

@@ -873,11 +873,6 @@ 

              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);

- 

-         /* Revert the caches if this is the parent operation */

-         if (parent_op) {

-             revert_cache(inst, &parent_time);

-         }

          goto error_return;

      }

      retval = plugin_call_mmr_plugin_postop(pb, NULL,SLAPI_PLUGIN_BE_TXN_POST_MODIFY_FN);
@@ -901,6 +896,11 @@ 

      goto common_return;

  

  error_return:

+     /* Revert the caches if this is the parent operation */

+     if (parent_op) {

+         revert_cache(inst, &parent_time);

+     }

+ 

      if (postentry != NULL) {

          slapi_entry_free(postentry);

          postentry = NULL;

@@ -1217,11 +1217,6 @@ 

              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);

- 

-         /* Revert the caches if this is the parent operation */

-         if (parent_op) {

-             revert_cache(inst, &parent_time);

-         }

          goto error_return;

      }

  	retval = plugin_call_mmr_plugin_postop(pb, NULL,SLAPI_PLUGIN_BE_TXN_POST_MODRDN_FN);
@@ -1290,6 +1285,11 @@ 

      goto common_return;

  

  error_return:

+     /* Revert the caches if this is the parent operation */

+     if (parent_op) {

+         revert_cache(inst, &parent_time);

+     }

+ 

      /* result already sent above - just free stuff */

      if (postentry) {

          slapi_entry_free(postentry);

Description:
The original version of the fix only checked if backend
transaction "post" operation plugins failed, but it did
not check for errors from the backend transaction "pre"
operation plugin. To address this we flush invalid
entries whenever any error occurs.

          We were also not flushing invalid cache entries when
          modrdn errors occurred.  Modrdns only make changes to
          the DN hashtable inside the entry cache, but we were only
          checking the ID hashtable.  So we also need to check the
          DN hashtable in the entry cache for invalid entries.

https://pagure.io/389-ds-base/issue/50260

rebased onto 423907e056b183631fc8e8e9e56040995ef5348c

5 years ago

rebased onto 8e9f0c74edbf52e21156d25d2a95099c9ce1f53a

5 years ago

I agree with the testing in preop but are you sure we hit a cache corruption during a plugin preop ?
Which plugin updates an entry on a preop while it does not know the result of the operation ?

I think remove_it is not the appropriate flag. It is used for each entries in the previous loop. So if the last evaluated entry is "older" than the txn remove_it is set to 0 although the loop may have remove others entries.

Sorry I do not understand why this loop fix the new issue :(

The patch is about ENTRY cache. In entry cache, idtable and dntable hashtables are updated together and an backcommon entry is either present in both tables or missing in both. If in the previous loop the backcommon was older that the txn start time it will still be older in that loop. as far as I understand entrycache_remove/return will never be called.

Sorry I do not understand why this loop fix the new issue :(
The patch is about ENTRY cache. In entry cache, idtable and dntable hashtables are updated together and an backcommon entry is either present in both tables or missing in both. If in the previous loop the backcommon was older that the txn start time it will still be older in that loop. as far as I understand entrycache_remove/return will never be called.

The issue is when we check for entries to be removed... Previously we only checked the ID table, and not the DN table. But in the case of a failed modrdn in preop, the entry is only in the dn table, not in the ID table! So we were never flushing the invalid entry, and it was appearing in searches. So we need to check both the ID and DN tables for invalid entries.

I think remove_it is not the appropriate flag. It is used for each entries in the previous loop. So if the last evaluated entry is "older" than the txn remove_it is set to 0 although the loop may have remove others entries.

You are right I am not using it correctly, I will revise it...

I agree with the testing in preop but are you sure we hit a cache corruption during a plugin preop ?
Which plugin updates an entry on a preop while it does not know the result of the operation ?

This was a concern of mine as well. Should I just have checks for failures for be txn pre and post op plugins? Or, flush the cache on any failure? I was worried about other corner cases and thought it might be best to flush the cache on any failure (regardless of its origin).

Would you prefer I just have two checks in the backend functions: one for be txn pre op, the other for be txn post op?

rebased onto 20287294619bd1dbc63c027990c9bb6f7c5c6058

5 years ago

The patch looks good to me. ACK.

I think it is better to have a common function that flushes the cache whatever the origin (pre/post) of the failure. I am a bit concerned that we may an entry in dntable but not in idtable, but it is outside of the scope of the patch. If this condition exists your patch address it.

The two loops are almost doing the same thing, it can be more generic if we rely on 'e->ep_type' to know if it is entry/dn cache. But your patch looks more simple to read. Could you add a comment to make more clear that we are flushing taking into account idtable, then dntable.

Thanks. You have my ACK

The patch looks good to me. ACK.
I think it is better to have a common function that flushes the cache whatever the origin (pre/post) of the failure.

Well right now, it is generic. Any failure from a be txn plugin, or a non-be txn plugin, or any error condition, will trigger the cache flushing. I was wondering if you wanted me to just do the flushing if an error came from be txn pre/post plugins.

I am a bit concerned that we may an entry in dntable but not in idtable, but it is outside of the scope of the patch. If this condition exists your patch address it.

The current patch always checks the DN and ID hashtables in the entry cache. So even if we find entries to invalidate in the ID table, we will still check the DN table. So no matter what we always check both hashtables

The two loops are almost doing the same thing, it can be more generic if we rely on 'e->ep_type' to know if it is entry/dn cache. But your patch looks more simple to read. Could you add a comment to make more clear that we are flushing taking into account idtable, then dntable.

Yeah I will update the comments!

I think the fix should flush all entries updated by the initial write operation. So revert in error_return, like the patch is doing, looks good to me because at this point all updated db entries will be revert.

rebased onto 33fbced

5 years ago

Pull-Request has been merged by mreynolds

5 years ago

@mreynolds, you are nominated entry cache guru :)

@mreynolds, you are nominated entry cache guru :)

Why does that sound like a punishment and not a complement :-p

@mreynolds No good deed goes unpunished?

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 pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3345

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi

3 years ago