Issue 6136 - failure in freeipa tests #6137

progier389 · 2024-03-28T12:55:05Z

Several issue detected when adding a CI test that mimic one of freeipa nightly test :

bdb - offline import fail when trying to create the guardian file because instance is not yet fully initialized and the generated path is wrong - fixed by using the directory from ldbminfo and the instance names that are defined.

mdb - vlv index are not generated because for one level scoped vlv, the entryid is not properly set.
should use vlv_grok_new_import_entry to reset the vlv filter when the base entry is added (as it is done in bdb).
also added a function to mark the vlv_grok_new_import_entry as uninitialized before the import

mdb- crash while trying to import an entry without parent (i.e a suffix entry) that does not belong to the backend
fixed by avoiding the null pointer exception in that case

Issue: #6136

Reviewed by: @droideck, @jchapma (Thanks!)

progier389 · 2024-03-28T18:21:49Z

vlv CI test is hanging on lmdb standalone instance is looping withing dbmdb_rm_db_file,
looks like there is a problem with ai_dblayer_count (The surprising thing is that I do not hit it when running on a non root PREFIXed build)

@long-entryrdn was not open by dbmdb_open_all_files this leaded to failure when trying to open it in a read operation because at dblayer level, it is not possible to open write txn within a read txn - and although it is possible at lmdb level, the new file will not be visible within the read txn but we may need to access it. So the open failed, and entryrdn attrinfo should then be released before returning an error to avoid keeping entryrdn busy. That is what trigger the hang when removing a backend. Added some conditionnal debug code to understand why the server hang. Also added a missing dblayer_release_index_file in vlvIndex_checkforindex that may be the reason while there is a hang when removing vlv on bdb.

droideck

Besides the minor issues, looks good!

droideck · 2024-04-04T19:13:22Z

ldap/servers/slapd/back-ldbm/db-bdb/bdb_layer.c

-                                    inst->inst_parent_dir_name,
-                                    inst->inst_dir_name);
+    struct ldbminfo *li = inst->inst_li;
+    char *fname = slapi_ch_smprintf("%s/.import_%s", li->li_directory, inst->inst_name);
    slapi_log_err(SLAPI_LOG_DEBUG, "bdb__import_file_name", "DBG: fname=%s\n", fname);


It's not related exactly to this PR, but it still caught my eye. Do you know of the double underscore (bdb__) is intentional in this function name or just a typo?

There is no name conflict so IMHO it is a typo, I will change it.

droideck · 2024-04-04T19:19:06Z

dirsrvtests/tests/suites/vlv/regression_test.py

    log.info(f'Adding {users_num} users')
    for i in range(0, users_num):
        uid = STARTING_UID_INDEX + i
        add_user(inst, users, uid)


-def create_vlv_search_and_index(inst, hyphen='', basedn=DEFAULT_SUFFIX):
+def create_vlv_search_and_index(inst, basedn=DEFAULT_SUFFIX, bename='userRoot', scope=2, prefix="vlv"):


For better readability, I think we should use ldap.LDAP_SCOPE_SUBTREE for the scope here, instead of magic number.

OK, will use ldap.SCOPE_SUBTREE instead of 2

droideck · 2024-04-04T19:19:48Z

dirsrvtests/tests/suites/vlv/regression_test.py

+        # Configure vlv
+        vlv_search, vlv_index = create_vlv_search_and_index(
+            inst, basedn=suffix,
+            bename=bename, scope=1,


Same here regarding the scope.

OK, will use ldap.SCOPE_ONELEVEL instead of 1

jchapma · 2024-04-04T23:48:15Z

I reviewed the C files and it more or less makes sense to me. Re the CI test, it must be the attribute info db layer count that is not being decremented, surely...

Ack

tbordaz

LGTM
few questions

tbordaz · 2024-04-05T10:18:00Z

ldap/servers/slapd/back-ldbm/db-mdb/mdb_instance.c

@@ -456,6 +456,7 @@ dbmdb_open_all_files(dbmdb_ctx_t *ctx, backend *be)
            TST(add_dbi(&octx, be, special_names[i], ctxflags));
            sn_dbis[i] = octx.dbi;
        }
+        TST(add_dbi(&octx, be, LDBM_LONG_ENTRYRDN_STR, ctxflags));


Any reason why having not added LDBM_LONG_ENTRYRDN_STR in special_names ?

tbordaz · 2024-04-05T10:35:26Z

ldap/servers/slapd/back-ldbm/vlv.c

+            entry_address addr;
+            addr.sdn = p->vlv_base;
+            addr.uniqueid = NULL;
+            e = find_entry(pb, be, &addr, txn, NULL);


If the base entry does not exist at that time, it will stay vlv_initialized=0. That looks fine to me. Does it require documentation ?

Interesting point. IMHO the right thing to do would be to check the uninitialized vlv when adding an entry ...

tbordaz · 2024-04-05T10:47:20Z

ldap/servers/slapd/back-ldbm/vlv_srch.c

@@ -776,6 +776,7 @@ vlvIndex_checkforindex(struct vlvIndex *p, backend *be)
        /* In lmdb case, always open the dbi */
        if (li->li_flags & LI_LMDB_IMPL) {
            (void) dblayer_get_index_file(be, p->vlv_attrinfo, &db, 0) ;
+            dblayer_release_index_file(be, p->vlv_attrinfo, db);


It looks open/close. What is it for ?

dblayer_release_index_file decrease a ref count to tell that the index is no more used.
On bdb it could lead to db instance closure but that is not the case with lmdb (db instances are not closed until closing the whole db environment (i.e at shutdown time) )
So the open/release partern just create/open the database instance and keep it open (but not busy).
Having it open is important because it cannot be open when needed by a search operation:
because there is an already open read txn and there is no way we can open a new file within that txn:
We can open the file with a new write txn, but in that case the previously created read txn has no visibility of the new file.
The big rule to remember with LMDB is is that ALL DATABASE INSTANCES MUST BE OPEN when bringing a backend on-line. The only case we could open/create db instance is when changing the config.

@droideck

* Issue 6136 - failure in freeipa tests Several issue detected when adding a CI test that mimic one of freeipa nightly test : bdb - offline import fail when trying to create the guardian file because instance is not yet fully initialized and the generated path is wrong - fixed by using the directory from ldbminfo and the instance names that are defined. mdb - vlv index are not generated because for one level scoped vlv, the entryid is not properly set. should use vlv_grok_new_import_entry to reset the vlv filter when the base entry is added (as it is done in bdb). also added a function to mark the vlv_grok_new_import_entry as uninitialized before the import mdb- crash while trying to import an entry without parent (i.e a suffix entry) that does not belong to the backend fixed by avoiding the null pointer exception in that case Issue: #6136 Reviewed by: @droideck, @jchapma (Thanks!) * Fix vlv CI test deadlock @long-entryrdn was not open by dbmdb_open_all_files this leaded to failure when trying to open it in a read operation because at dblayer level, it is not possible to open write txn within a read txn - and although it is possible at lmdb level, the new file will not be visible within the read txn but we may need to access it. So the open failed, and entryrdn attrinfo should then be released before returning an error to avoid keeping entryrdn busy. That is what trigger the hang when removing a backend. Added some conditionnal debug code to understand why the server hang. Also added a missing dblayer_release_index_file in vlvIndex_checkforindex that may be the reason while there is a hang when removing vlv on bdb. * Issue 6136 - failure in freeipa tests - Fix review comments

@droideck

* Issue 6136 - failure in freeipa tests Several issue detected when adding a CI test that mimic one of freeipa nightly test : bdb - offline import fail when trying to create the guardian file because instance is not yet fully initialized and the generated path is wrong - fixed by using the directory from ldbminfo and the instance names that are defined. mdb - vlv index are not generated because for one level scoped vlv, the entryid is not properly set. should use vlv_grok_new_import_entry to reset the vlv filter when the base entry is added (as it is done in bdb). also added a function to mark the vlv_grok_new_import_entry as uninitialized before the import mdb- crash while trying to import an entry without parent (i.e a suffix entry) that does not belong to the backend fixed by avoiding the null pointer exception in that case Issue: #6136 Reviewed by: @droideck, @jchapma (Thanks!) * Fix vlv CI test deadlock @long-entryrdn was not open by dbmdb_open_all_files this leaded to failure when trying to open it in a read operation because at dblayer level, it is not possible to open write txn within a read txn - and although it is possible at lmdb level, the new file will not be visible within the read txn but we may need to access it. So the open failed, and entryrdn attrinfo should then be released before returning an error to avoid keeping entryrdn busy. That is what trigger the hang when removing a backend. Added some conditionnal debug code to understand why the server hang. Also added a missing dblayer_release_index_file in vlvIndex_checkforindex that may be the reason while there is a hang when removing vlv on bdb. * Issue 6136 - failure in freeipa tests - Fix review comments (cherry picked from commit cc3a864)

progier389 linked an issue Mar 28, 2024 that may be closed by this pull request

failure in freeipa test #6136

Closed

progier389 self-assigned this Mar 28, 2024

progier389 force-pushed the i6136 branch from a0749aa to 3255d69 Compare March 28, 2024 13:00

progier389 added this to the 3.0.0 milestone Mar 28, 2024

Issue 6136 - failure in freeipa tests

d670d5d

progier389 added the work in progress Work in Progress - can be reviewed, but not ready for merge. label Mar 28, 2024

progier389 force-pushed the i6136 branch from 3255d69 to d803c44 Compare March 29, 2024 20:04

progier389 force-pushed the i6136 branch from d803c44 to f44e94b Compare April 2, 2024 14:28

progier389 removed the work in progress Work in Progress - can be reviewed, but not ready for merge. label Apr 2, 2024

droideck approved these changes Apr 4, 2024

View reviewed changes

Issue 6136 - failure in freeipa tests - Fix review comments

fe0ea3f

progier389 merged commit cc3a864 into 389ds:main Apr 5, 2024

tbordaz reviewed Apr 5, 2024

View reviewed changes

progier389 mentioned this pull request Apr 5, 2024

lmdb - onelevel scoped vlv are not initialized if base dn does not exists when backend is started. #6139

Open

progier389 mentioned this pull request May 31, 2024

failure in freeipa test #6136

Closed

progier389 deleted the i6136 branch May 20, 2025 13:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Issue 6136 - failure in freeipa tests #6137

Issue 6136 - failure in freeipa tests #6137

Uh oh!

progier389 commented Mar 28, 2024 •

edited

Loading

Uh oh!

progier389 commented Mar 28, 2024

Uh oh!

droideck left a comment

Uh oh!

droideck Apr 4, 2024

Uh oh!

progier389 Apr 5, 2024

Uh oh!

droideck Apr 4, 2024

Uh oh!

progier389 Apr 5, 2024

Uh oh!

droideck Apr 4, 2024

Uh oh!

progier389 Apr 5, 2024

Uh oh!

jchapma commented Apr 4, 2024

Uh oh!

tbordaz left a comment

Uh oh!

tbordaz Apr 5, 2024

Uh oh!

tbordaz Apr 5, 2024

Uh oh!

progier389 Apr 5, 2024

Uh oh!

tbordaz Apr 5, 2024

Uh oh!

progier389 Apr 5, 2024 •

edited

Loading

Uh oh!

Uh oh!

Issue 6136 - failure in freeipa tests #6137

Issue 6136 - failure in freeipa tests #6137

Uh oh!

Conversation

progier389 commented Mar 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

progier389 commented Mar 28, 2024

Uh oh!

droideck left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jchapma commented Apr 4, 2024

Uh oh!

tbordaz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

progier389 Apr 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

progier389 commented Mar 28, 2024 •

edited

Loading

progier389 Apr 5, 2024 •

edited

Loading