#49936 Ticket 49915 - Master ns-slapd had 100% CPU usage after starting replication and replication cannot finish
Closed 3 years ago by spichugi. Opened 5 years ago by tbordaz.
tbordaz/389-ds-base ticket_49915  into  master

@@ -283,6 +283,53 @@ 

      }

  }

  

+ /* This routine checks that the entry id of the suffix is

+  * stored in the parentid index

+  * The entry id of the suffix is stored with the equality key 0 (i.e. '=0')

+  * It first checks if the key '=0' exists. If it does not exists or if the first value

+  * stored with that key, does not match the suffix entryid (stored in the suffix entry

+  * from id2entry.db then it updates the value

+  */

+ static void

+ check_suffix_entryID(Slapi_Backend *be, Slapi_Entry *suffix)

+ {

+     u_int32_t entryid;

+     char *entryid_str;

+     struct _back_info_index_key bck_info;

+ 

+     /* we are using a specific key in parentid to store the suffix entry id: '=0' */

+     bck_info.index = SLAPI_ATTR_PARENTID;

+     bck_info.key = "0";

+ 

+     /* First try to retrieve from parentid index the suffix entryID */

+     if (slapi_back_get_info(be, BACK_INFO_INDEX_KEY, (void **) &bck_info)) {

+         slapi_log_err(SLAPI_LOG_REPL, "check_suffix_entryID", "Total update: fail to retrieve suffix entryID. Let's try to write it\n");

+     }

+ 

+     /* Second retrieve the suffix entryid from the suffix entry itself */

+     entryid_str = slapi_entry_attr_get_charptr(suffix, "entryid");

+     if (entryid_str == NULL) {

+         char *dn;

+         dn = slapi_entry_get_ndn(suffix);

+         slapi_log_err(SLAPI_LOG_ERR, "check_suffix_entryID", "Unable to retrieve entryid of the suffix entry %s\n", dn ? dn : "<unknown>");

+         slapi_ch_free_string(&entryid_str);

+         return;

+     }

+     entryid = (u_int32_t) atoi(entryid_str);

+     slapi_ch_free_string(&entryid_str);

+ 

+     if (!bck_info.key_found || bck_info.id != entryid) {

+         /* The suffix entryid is not present in parentid index

+          *  or differs from what is in id2entry (entry 'suffix')

+          * So write it to the parentid so that the range index used

+          * during total init will know the entryid of the suffix

+          */

+         bck_info.id = entryid;

+         if (slapi_back_set_info(be, BACK_INFO_INDEX_KEY, (void **) &bck_info)) {

+             slapi_log_err(SLAPI_LOG_ERR, "check_suffix_entryID", "Total update: fail to register suffix entryid, continue assuming suffix is the first entry\n");

+         }

+     }

+ }

  

  /*

   * Completely refresh a replica. The basic protocol interaction goes
@@ -467,6 +514,7 @@ 

          replica_subentry_check(area_sdn, rid);

  

          /* Send the subtree of the suffix in the order of parentid index plus ldapsubentry and nstombstone. */

+         check_suffix_entryID(be, suffix);

          slapi_search_internal_set_pb(pb, slapi_sdn_get_dn(area_sdn),

                                       LDAP_SCOPE_SUBTREE, "(parentid>=1)", NULL, 0, ctrls, NULL,

                                       repl_get_plugin_identity(PLUGIN_MULTIMASTER_REPLICATION), OP_FLAG_BULK_IMPORT);

@@ -7309,6 +7309,10 @@ 

          *(int *)info = entryrdn_get_switch();

          break;

      }

+     case BACK_INFO_INDEX_KEY : {

+         rc = get_suffix_key(be, (struct _back_info_index_key *)info);

+         break;

+     }

      default:

          break;

      }
@@ -7325,6 +7329,10 @@ 

      }

  

      switch (cmd) {

+     case BACK_INFO_INDEX_KEY : {

+         rc = set_suffix_key(be, (struct _back_info_index_key *)info);

+         break;

+     }

      default:

          break;

      }

@@ -320,6 +320,9 @@ 

   * In the total update (bulk import), an entry requires its ancestors already added.

   * To guarantee it, the range search with parentid is used with setting the flag

   * SLAPI_OP_RANGE_NO_IDL_SORT in operator.

+  * In bulk import the range search is parentid>=1 to retrieve all the entries

+  * But we need to order the IDL with the parents first => retrieve the suffix entry ID

+  * to store the children

   *

   * If the flag is set,

   * 1. the IDList is not sorted by the ID.
@@ -366,6 +369,23 @@ 

      if (NULL == flag_err) {

          return NULL;

      }

+     if (operator & SLAPI_OP_RANGE_NO_IDL_SORT) {

+             struct _back_info_index_key bck_info;

+             int rc;

int32_t ?

Why ? slapi_back_get_info returns 'int'.
The backend ID are u_int32_t and are retrieved from bck_info structure.

+             /* We are doing a bulk import

+              * try to retrieve the suffix entry id from the index

+              */

+ 

+             bck_info.index = SLAPI_ATTR_PARENTID;

+             bck_info.key = "0";

+ 

+             if (rc = slapi_back_get_info(be, BACK_INFO_INDEX_KEY, (void **)&bck_info)) {

+                 slapi_log_err(SLAPI_LOG_WARNING, "idl_new_range_fetch", "Total update: fail to retrieve suffix entryID, continue assuming it is the first entry\n");

+             }

+             if (bck_info.key_found) {

+                 suffix = bck_info.id;

+             }

+     }

  

      if (NEW_IDL_NOOP == *flag_err) {

          return NULL;
@@ -455,7 +475,7 @@ 

              *flag_err = LDAP_TIMELIMIT_EXCEEDED;

              goto error;

          }

-         if (operator&SLAPI_OP_RANGE_NO_IDL_SORT) {

+         if (operator & SLAPI_OP_RANGE_NO_IDL_SORT) {

              key = (ID)strtol((char *)cur_key.data + 1, (char **)NULL, 10);

          }

          while (PR_TRUE) {
@@ -487,9 +507,13 @@ 

              /* note the last id read to check for dups */

              lastid = id;

              /* we got another ID, add it to our IDL */

-             if (operator&SLAPI_OP_RANGE_NO_IDL_SORT) {

-                 if (count == 0) {

-                     /* First time.  Keep the suffix ID. */

+             if (operator & SLAPI_OP_RANGE_NO_IDL_SORT) {

+                 if ((count == 0) && (suffix == 0)) {

+                     /* First time.  Keep the suffix ID. 

+                      * note that 'suffix==0' mean we did not retrieve the suffix entry id

+                      * from the parentid index (key '=0'), so let assume the first

+                      * found entry is the one from the suffix

+                      */

                      suffix = key;

                      idl_rc = idl_append_extend(&idl, id);

                  } else if ((key == suffix) || idl_id_is_in_idlist(idl, key)) {
@@ -615,9 +639,11 @@ 

      }

      if (operator&SLAPI_OP_RANGE_NO_IDL_SORT) {

          size_t remaining = leftovercnt;

+ 

          while(remaining > 0) {

              for (size_t i = 0; i < leftovercnt; i++) {

                  if (leftover[i].key > 0 && idl_id_is_in_idlist(idl, leftover[i].key) != 0) {

+                     /* if the leftover key has its parent in the idl */

                      idl_rc = idl_append_extend(&idl, leftover[i].id);

                      if (idl_rc) {

                          slapi_log_err(SLAPI_LOG_ERR, "idl_new_range_fetch",

@@ -1236,6 +1236,120 @@ 

      return ret;

  }

  

+ /* This routine add in a given index (parentid)

+  * the key/value = '=0'/<suffix entryID>

+  * Input: 

+  *      info->key contains the key to lookup (i.e. '0')

+  *      info->index index name used to retrieve syntax and db file

+  *      info->id  the entryID of the suffix

+  */

+ int

Same here int32_t please :)

This is callback from slapi_back_get_info that returns 'int' not int32_t.
Do you suggest to change slapi_back_get_info returned type ?

+ set_suffix_key(Slapi_Backend *be, struct _back_info_index_key *info)

+ {

+     struct ldbminfo *li;

+     int rc;

+     back_txn txn;

+     Slapi_Value *sv_key[2];

+     Slapi_Value tmpval;

+ 

+     if (info->index== NULL || info->key == NULL) {

+         slapi_log_err(SLAPI_LOG_ERR, "set_suffix_key", "Invalid index %s or key %s\n",

+                 info->index ? info->index : "NULL",

+                 info->key ? info->key : "NULL");

+         return -1;

+     }

+     

+     /* Start a txn */

+     li = (struct ldbminfo *)be->be_database->plg_private;

+     dblayer_txn_init(li, &txn);

With txn's like this, I think it's good to avoid nested txns in the future, so my question is if we are already in a txn when the function is called here, or if we really are a parent making new txns for data manipulation

Well currently this is a new interface BACK_INFO_INDEX_KEY is only used during online total init. There is no parent txn.
Now considering it lands in slapi-plugin, any plugin could call it and may have a txn.
One possibility it to add the txn in the 'info' struct.

I think with our current txn mechanism you don't need to know if you are in a txn already, txn_init will check the txn stack in the thread and nest txns automatically.
For me that is good enough for this I do not see a need to do this change using an already existing txn explicitely.

+     if (rc = dblayer_txn_begin(be, txn.back_txn_txn, &txn)) {

+         slapi_log_err(SLAPI_LOG_ERR, "set_suffix_key", "Fail to update %s index with  %s/%d (key/ID): txn begin fails\n",

+                   info->index, info->key, info->id);

+         return rc;

+     }

+ 

+     sv_key[0] = &tmpval;

+     sv_key[1] = NULL;

+     slapi_value_init_string(sv_key[0], info->key);

+ 

+     if (rc = index_addordel_values_sv(be, info->index, sv_key, NULL, info->id, BE_INDEX_ADD, &txn)) {

+         value_done(sv_key[0]);

+         dblayer_txn_abort(be, &txn);

+         slapi_log_err(SLAPI_LOG_ERR, "set_suffix_key", "Fail to update %s index with  %s/%d (key/ID): index_addordel_values_sv fails\n",

+                   info->index, info->key, info->id);

+         return rc;

+     }

+ 

+     value_done(sv_key[0]);

+     if (rc = dblayer_txn_commit(be, &txn)) {

+         slapi_log_err(SLAPI_LOG_ERR, "set_suffix_key", "Fail to update %s index with  %s/%d (key/ID): commit fails\n",

+                   info->index, info->key, info->id);

+         return rc;

+     }

+ 

+     return 0;

+ }

+ /* This routine retrieves from a given index (parentid)

+  * the key/value = '=0'/<suffix entryID>

+  * Input: 

+  *      info->key contains the key to lookup (i.e. '0')

+  *      info->index index name used to retrieve syntax and db file

+  * Output

+  *      info->id It returns the first id that is found for the key.

+  *               If the key is not found, or there is no value for the key

+  *               it contains '0'

+  *      info->key_found  Boolean that says if the key leads to a valid ID in info->id

+  */

+ int

+ get_suffix_key(Slapi_Backend *be, struct _back_info_index_key *info)

+ {

+     struct berval bv;

+     int err;

+     IDList *idl = NULL;

+     ID id;

+     int rc = 0;

+ 

+     if (info->index== NULL || info->key == NULL) {

+         slapi_log_err(SLAPI_LOG_ERR, "get_suffix_key", "Invalid index %s or key %s\n",

+                 info->index ? info->index : "NULL",

+                 info->key ? info->key : "NULL");

+         return -1;

+     }

+ 

+     /* This is the key to retrieve */

+     bv.bv_val = info->key;

+     bv.bv_len = strlen(bv.bv_val);

+ 

+     /* Assuming we are not going to find the key*/

+     info->key_found = PR_FALSE;

+     id = 0;

+     idl = index_read(be, info->index, indextype_EQUALITY, &bv, NULL, &err);

+ 

+     if (idl == NULL) {

+         if (err != 0 && err != DB_NOTFOUND) {

+             slapi_log_err(SLAPI_LOG_ERR, "get_suffix_key", "Fail to read key %s (err=%d)\n",

+                     info->key ? info->key : "NULL",

+                     err);

+             rc = err;

+         }

+     } else {

+         /* info->key was found */

+         id = idl_firstid(idl);

+         if (id != NOID) {

+             info->key_found = PR_TRUE;

+         } else {

+             /* there is no ID in that key, make it as it was not found */

+             id = 0;

+         }

+         idl_free(&idl);

+     }

+ 

+     /* now set the returned id */

+     info->id = id;

+ 

+     return rc;

+ }

+ 

  IDList *

  index_range_read_ext(

      Slapi_PBlock *pb,

@@ -7745,9 +7745,17 @@ 

      BACK_INFO_CRYPT_DECRYPT_VALUE, /* Ctrl: clcrypt_decrypt_value */

      BACK_INFO_DIRECTORY,           /* Get the directory path */

      BACK_INFO_LOG_DIRECTORY,       /* Get the txn log directory */

-     BACK_INFO_IS_ENTRYRDN          /* Get the flag for entryrdn */

+     BACK_INFO_IS_ENTRYRDN,         /* Get the flag for entryrdn */

+     BACK_INFO_INDEX_KEY            /* Get the status of a key in an index */

  };

  

+ struct _back_info_index_key

+ {

+     char *index;              /* input: name of the index (parentid) */

+     char *key;                /* input: searched key (0) with equality -> '=0' */

+     PRBool key_found;         /* output: TRUE if '=0' is found in the index */

+     u_int32_t id;             /* output: if key_found it is the first value (suffix entryID) */

+ };

  struct _back_info_crypt_init

  {

      char *dn;                  /* input -- entry to store nsSymmetricKey */

Bug Description:
During a total initialization the supplier builds a candidate list of the entries to send.
Because of https://fedorahosted.org/389/ticket/48755, the candidate list relies on parentid attribute.
All entries, except tombstones and suffix itself, have parentid.
There is an assumption that the first found key (i.e. '=1') contains the suffix children.
So when it finally finds the suffix key it adds its children to a leftover list rather to the candidate list.
Later idl_new_range_fetch loops for ever trying to add suffix children from leftover to candidate list.

Fix Description:
The fix consist to detect that the suffix key was not the expected one. Then to identify the suffix key and
add its children to the candidate list. So remaining key/id in the leftover array will find their parents

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

Reviewed by: ?

Platforms tested: F27

Flag Day: no

Doc impact: no

I think the patch will work, but I wonder if we cannot pass the information we have.
In repl5_tot_run we do an internal search with "parentid>=1" and the bulk import flag, but we did send the suffix entry before, so we know the entryid of the suffix and the search could be "parentid>=suffix_id". Unfortunately the parentid index is ordered by the string representation of the id and 2>11, but couldn't we pass the real suffix entry id in the search filter and the check in the idl func if it is 1 or requires a special handling, but with an already known id

Indeed this is a nice idea. suffix entryid is known so it worth using it rather than retrieving it. 'parentid>=<suffix_id>' will be translated into 'parentid>=2' (if suffix is the second entry) and that will miss in the range search all entries starting with '1'. So suffix ID is not usable for the filter.

I will update the patch.. and also with an automated testcase

After deeper thoughts I realize that the submitted patch is wrong. It does not guarantee that the IDs in the candidate list are ordered so that parent are created before children. Indeed some IDs before the suffix ID are going into the idl but it is not sure that those IDs are directly under the suffix children.

The solution is to start with '=1' key to be sure to grab all keys, then put all IDs before suffix ID into the leftover array. When suffix ID is found put its children into the idl and let the remaining keys tests if they go either to IDL (parent is already in IDL) or to the leftover.
Finally the consolidation of the leftover into the IDL creates the final list.

The difficult point is to determine the suffix key and to use it in idl_new_range_fetch.
Discussing with @lkrispen we evaluated

  • add a new suffix_id field in op or pblock and use bulk_import flag to use it.
  • put the suffix id directly in the filter and use the bulk_import flag to restore the valid filter
  • store the suffix_id (if it does not exist already) in the parentid index (with the key '=0') in repl5_tot_run, then get it to detect the suffix key from the index in idl_new_range_fetch.

The last option looks the less messy.

An other option was abandoned of compatibility issue: send entries randomly and let the consumer create the appropriate glue entries.

rebased onto 71c77fc1ab689065e1e84a13d128b4eafd6570bb

5 years ago

In general I think this fix is good, some small comments:
- in check_suffix_entry() you handle a few error cases, but the function is void and doesn't return the errors, could they be serious enought not to continue with the online init ?
- in check_suffix_entry() shoudn't you init all fields of back_info ?
- in get/set_suffix_key always 0 is returned, could the index operations fail, should we propagate errors ?
- in get_suffix_key() do you want to keep the #if 0 sections ?
- in set_suffix_key() you use a pblock just to get the ldbminfo from the backend, but you're inside backend code, so I think
struct ldbminfo li = (struct ldbminfo ) be->be_database->plg_private;

should work

rebased onto 2c34e08a5adbb123c2b9537e4067bb60db0f9565

5 years ago

With txn's like this, I think it's good to avoid nested txns in the future, so my question is if we are already in a txn when the function is called here, or if we really are a parent making new txns for data manipulation

I have added some minor comments myself, but I will trust @lkrispens comments about some of the more interesting parts of the change :)

Why ? slapi_back_get_info returns 'int'.
The backend ID are u_int32_t and are retrieved from bck_info structure.

This is callback from slapi_back_get_info that returns 'int' not int32_t.
Do you suggest to change slapi_back_get_info returned type ?

Well currently this is a new interface BACK_INFO_INDEX_KEY is only used during online total init. There is no parent txn.
Now considering it lands in slapi-plugin, any plugin could call it and may have a txn.
One possibility it to add the txn in the 'info' struct.

int and int32_t are the same, but it's clearer, and we should do the right thing.

I think with our current txn mechanism you don't need to know if you are in a txn already, txn_init will check the txn stack in the thread and nest txns automatically.
For me that is good enough for this I do not see a need to do this change using an already existing txn explicitely.

@firstyear , @lkrispen , currently there is no possibility of nested txn. If in the future the interface is called with an opened txn, the current framework will take care of it (like @lkrispen mentioned).
So there is no immediate concern regarding this parent txn.

In order to prepare the interface for a potential need, do you want to add a txn field in the info struct, even if the db layer (set_suffix_key) does not use it ?
Else is it okay to push the patch ?

rebased onto bdb8676

5 years ago

Pull-Request has been merged by tbordaz

5 years ago

This PR caused some compiier issues:

ldap/servers/slapd/back-ldbm/idl_new.c: In function 'idl_new_range_fetch':
ldap/servers/slapd/back-ldbm/idl_new.c:382:17: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
             if (rc = slapi_back_get_info(be, BACK_INFO_INDEX_KEY, (void **)&bck_info)) {


ldap/servers/slapd/back-ldbm/dblayer.c: In function 'ldbm_back_get_info':
ldap/servers/slapd/back-ldbm/dblayer.c:7313:14: warning: implicit declaration of function 'get_suffix_key'; did you mean 'get_filter'? [-Wimplicit-function-declaration]
         rc = get_suffix_key(be, (struct _back_info_index_key *)info);
              ^~~~~~~~~~~~~~
              get_filter
ldap/servers/slapd/back-ldbm/dblayer.c: In function 'ldbm_back_set_info':
ldap/servers/slapd/back-ldbm/dblayer.c:7333:14: warning: implicit declaration of function 'set_suffix_key'; did you mean 'setbuffer'? [-Wimplicit-function-declaration]
         rc = set_suffix_key(be, (struct _back_info_index_key *)info);
              ^~~~~~~~~~~~~~
              setbuffer

ldap/servers/slapd/back-ldbm/index.c: In function 'set_suffix_key':
ldap/servers/slapd/back-ldbm/index.c:1265:9: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
     if (rc = dblayer_txn_begin(be, txn.back_txn_txn, &txn)) {
         ^~
ldap/servers/slapd/back-ldbm/index.c:1275:9: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
     if (rc = index_addordel_values_sv(be, info->index, sv_key, NULL, info->id, BE_INDEX_ADD, &txn)) {
         ^~
ldap/servers/slapd/back-ldbm/index.c:1284:9: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
     if (rc = dblayer_txn_commit(be, &txn)) {

I fixed them in master, but this needs to be done in 1.3.8, and 1.3.7 (this might impact downstream)

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/2995

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