PR#50233: Ticket 50232 - export creates not importable ldif file - 389-ds-base

389-ds-base

#50233 Ticket 50232 - export creates not importable ldif file

Closed 3 years ago by spichugi. Opened 5 years ago by lkrispen.

lkrispen/389-ds-base t50232 into master

Ticket 50232 - export creates not importable ldif file

Ludwig Krispenz • 5 years ago

eb1b5c5

dirsrvtests/tests/tickets/ticket50232_test.py

file added

+163

		`@@ -0,0 +1,163 @@`
		`+ # --- BEGIN COPYRIGHT BLOCK ---`
		`+ # Copyright (C) 2016 Red Hat, Inc.`
		`+ # All rights reserved.`
		`+ #`
		`+ # License: GPL (version 3 or any later version).`
		`+ # See LICENSE for details.`
		`+ # --- END COPYRIGHT BLOCK ---`
		`+ #`
		`+ import logging`
		`+`
		`+ import pytest`
		`+ # from lib389.tasks import *`
		`+ # from lib389.utils import *`
		`+ from lib389.topologies import topology_st`
		`+ from lib389.replica import ReplicationManager,Replicas`
		`+`
		`+ from lib389._constants import DEFAULT_SUFFIX, BACKEND_NAME`
		`+`
		`+ from lib389.idm.user import UserAccounts`
		`+ from lib389.idm.organization import Organization`
		`+ from lib389.idm.organizationalunit import OrganizationalUnit`
		`+`
		`+ log = logging.getLogger(__name__)`
		`+`
		`+ NORMAL_SUFFIX = 'o=normal'`
		`+ NORMAL_BACKEND_NAME = 'normal'`
		`+ REVERSE_SUFFIX = 'o=reverse'`
		`+ REVERSE_BACKEND_NAME = 'reverse'`
		`+`
		`+ def _enable_replica(instance, suffix):`
		`+`
		`+ repl = ReplicationManager(DEFAULT_SUFFIX)`
		`+ repl._ensure_changelog(instance)`
		`+ replicas = Replicas(instance)`
		`+ replicas.create(properties={`
		`+ 'cn': 'replica',`
		`+ 'nsDS5ReplicaRoot': suffix,`
		`+ 'nsDS5ReplicaId': '1',`
		`+ 'nsDS5Flags': '1',`
		`+ 'nsDS5ReplicaType': '3'`
		`+ })`
		`+`
		`+ def _populate_suffix(instance, suffixname):`
		`+`
		`+ o = Organization(instance, 'o={}'.format(suffixname))`
		`+ o.create(properties={`
		`+ 'o': suffixname,`
		`+ 'description': 'test'`
		`+ })`
		`+ ou = OrganizationalUnit(instance, 'ou=people,o={}'.format(suffixname))`
		`+ ou.create(properties={`
		`+ 'ou': 'people'`
		`+ })`
		`+`
		`+ def _get_replica_generation(instance, suffix):`
		`+`
		`+ replicas = Replicas(instance)`
		`+ replica = replicas.get(suffix)`
		`+ ruv = replica.get_ruv()`
		`+ return ruv._data_generation`
		`+`
		`+ def _test_export_import(instance, suffix, backend):`
		`+`
		`+ before_generation = _get_replica_generation(instance, suffix)`
		`+`
		`+ instance.stop()`
		`+ instance.db2ldif(`
		`+ bename=backend,`
		`+ suffixes=[suffix],`
		`+ excludeSuffixes=[],`
		`+ encrypt=False,`
		`+ repl_data=True,`
		`+ outputfile="/tmp/output_file",`
		`+ )`
		`+ instance.ldif2db(`
		`+ bename=None,`
		`+ excludeSuffixes=None,`
		`+ encrypt=False,`
		`+ suffixes=[suffix],`
		`+ import_file="/tmp/output_file",`
		`+ )`
		`+ instance.start()`
		`+ after_generation = _get_replica_generation(instance, suffix)`
		`+`
		`+ assert (before_generation == after_generation)`
		`+`
		`+ def test_ticket50232_normal(topology_st):`
		`+ """`
		`+ The fix for ticket 50232`
		`+`
		`+`
		`+ The test sequence is:`
		`+ - create suffix`
		`+ - add suffix entry and some child entries`
		`+ - "normally" done after populating suffix: enable replication`
		`+ - get RUV and database generation`
		`+ - export -r`
		`+ - import`
		`+ - get RUV and database generation`
		`+ - assert database generation has not changed`
		`+ """`
		`+`
		`+ log.info('Testing Ticket 50232 - export creates not imprtable ldif file, normal creation order')`
		`+`
		`+ topology_st.standalone.backend.create(NORMAL_SUFFIX, {BACKEND_NAME: NORMAL_BACKEND_NAME})`
		`+ topology_st.standalone.mappingtree.create(NORMAL_SUFFIX, bename=NORMAL_BACKEND_NAME, parent=None)`
		`+`
		`+ _populate_suffix(topology_st.standalone, NORMAL_BACKEND_NAME)`
		`+`
		`+ repl = ReplicationManager(DEFAULT_SUFFIX)`
		`+ repl._ensure_changelog(topology_st.standalone)`
firstyear commented 5 years ago I would think the changelog creation is ensured during the topology setup? But this doesn't really hurt here either.
		`+ replicas = Replicas(topology_st.standalone)`
		`+ replicas.create(properties={`
		`+ 'cn': 'replica',`
		`+ 'nsDS5ReplicaRoot': NORMAL_SUFFIX,`
		`+ 'nsDS5ReplicaId': '1',`
		`+ 'nsDS5Flags': '1',`
		`+ 'nsDS5ReplicaType': '3'`
		`+ })`
		`+`
		`+ _test_export_import(topology_st.standalone, NORMAL_SUFFIX, NORMAL_BACKEND_NAME)`
		`+`
		`+ def test_ticket50232_reverse(topology_st):`
		`+ """`
		`+ The fix for ticket 50232`
		`+`
		`+`
		`+ The test sequence is:`
		`+ - create suffix`
		`+ - enable replication before suffix enztry is added`
		`+ - add suffix entry and some child entries`
		`+ - get RUV and database generation`
		`+ - export -r`
		`+ - import`
		`+ - get RUV and database generation`
		`+ - assert database generation has not changed`
		`+ """`
		`+`
		`+ log.info('Testing Ticket 50232 - export creates not imprtable ldif file, normal creation order')`
		`+`
		`+ #`
		`+ # Setup Replication`
		`+ #`
		`+ log.info('Setting up replication...')`
		`+ repl = ReplicationManager(DEFAULT_SUFFIX)`
		`+ # repl.create_first_master(topology_st.standalone)`
		`+ #`
		`+ # enable dynamic plugins, memberof and retro cl plugin`
		`+ #`
		`+ topology_st.standalone.backend.create(REVERSE_SUFFIX, {BACKEND_NAME: REVERSE_BACKEND_NAME})`
		`+ topology_st.standalone.mappingtree.create(REVERSE_SUFFIX, bename=REVERSE_BACKEND_NAME, parent=None)`
		`+`
		`+ _enable_replica(topology_st.standalone, REVERSE_SUFFIX)`
		`+`
		`+ _populate_suffix(topology_st.standalone, REVERSE_BACKEND_NAME)`
		`+`
		`+ _test_export_import(topology_st.standalone, REVERSE_SUFFIX, REVERSE_BACKEND_NAME)`
		`+`
		`+ if __name__ == '__main__':`
		`+ # Run isolated`
		`+ # -s for DEBUG mode`
		`+ CURRENT_FILE = os.path.realpath(__file__)`
		`+ pytest.main("-s %s" % CURRENT_FILE)`

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

file modified

+56 -2

		`@@ -1103,6 +1103,7 @@`
		`* (reunified at last)`
		`*/`
		`#define LDBM2LDIF_BUSY (-2)`
		`+ #define RUVRDN SLAPI_ATTR_UNIQUEID "=" RUV_STORAGE_ENTRY_UNIQUEID`
		`int`
		`ldbm_back_ldbm2ldif(Slapi_PBlock *pb)`
		`{`
		`@@ -1111,6 +1112,7 @@`
		`DB *db = NULL;`
		`DBC *dbc = NULL;`
		`struct backentry *ep;`
		`+ struct backentry *pending_ruv = NULL;`
		`DBT key = {0};`
		`DBT data = {0};`
		`char *fname = NULL;`
		`@@ -1146,6 +1148,8 @@`
		`static int load_dse = 1; /* We'd like to load dse just once. */`
		`int server_running;`
		`export_args eargs = {0};`
		`+ int32_t suffix_written = 0;`
		`+ int32_t skip_ruv = 0;`

		`slapi_log_err(SLAPI_LOG_TRACE, "ldbm_back_ldbm2ldif", "=>\n");`

		`@@ -1463,8 +1467,25 @@`
		`}`
		`}`

		`- if (0 != return_value)`
		`+ if (DB_NOTFOUND == return_value) {`
		`+ /* reached the end of the database,`
		`+ * check if ruv is pending and write it`
		`+ */`
		`+ if (pending_ruv) {`
		`+ eargs.ep = pending_ruv;`
		`+ eargs.idindex = idindex;`
		`+ eargs.cnt = &cnt;`
		`+ eargs.lastcnt = &lastcnt;`
		`+ rc = export_one_entry(li, inst, &eargs);`
		`+ backentry_free(&pending_ruv);`
		`+ }`
		`+ break;`
		`+ }`
		`+`
		`+ if (0 != return_value) {`
		`+ /* error reading database */`
		`break;`
		`+ }`

		`/* back to internal format */`
		`temp_id = id_stored_to_internal((char *)key.data);`
		`@@ -1501,7 +1522,30 @@`
		`rc = get_value_from_string((const char *)data.dptr,`
		`LDBM_PARENTID_STR, &pid_str);`
		`if (rc) {`
		`- rc = 0; /* assume this is a suffix */`
		`+ /* this could be a suffix or the RUV entry.`
		`+ * If it is the ruv and the suffix is not written`
		`+ * keep the ruv and export as last entry.`
		`+ *`
		`+ * The reason for this is that if the RUV entry is in the`
		`+ * ldif before the suffix entry then at an attempt to import`
		`+ * that ldif the RUV entry would be skipped because the parent`
		`+ * does not exist. Later a new RUV would be generated with`
		`+ * a different database generation and replication is broken`
		`+ */`
		`+ if (suffix_written) {`
		`+ /* this must be the RUV, just continue and write it */`
		`+ rc = 0;`
		`+ } else if (0 == strcasecmp(rdn, RUVRDN)) {`
		`+ /* this is the RUV and the suffix is not yet written`
		`+ * make it pending and continue with next entry`
		`+ */`
		`+ skip_ruv = 1;`
		`+ rc = 0;`
		`+ } else {`
		`+ /* this has to be the suffix */`
		`+ suffix_written = 1;`
		`+ rc = 0;`
		`+ }`
		`} else {`
		`pid = (ID)strtol(pid_str, (char **)NULL, 10);`
		`slapi_ch_free_string(&pid_str);`
		`@@ -1614,6 +1658,16 @@`
		`continue;`
		`}`

		`+ if (skip_ruv) {`
		`+ /* now we keep a copy of the ruv entry`
		`+ * and continue with the next entry`
		`+ */`
		`+ pending_ruv = ep;`
		`+ ep = NULL;`
		`+ skip_ruv = 0;`
		`+ continue;`
		`+ }`
		`+`
		`eargs.ep = ep;`
		`eargs.idindex = idindex;`
		`eargs.cnt = &cnt;`

lkrispen commented 5 years ago

Bug: If the RUV entry hasa smaller entryid than the suffix entry it will be
exported before the suffix. If that ldif is used for import the RUV entry
is skipped and a new one generated with a different database generation

Fix: Before exporting the RUV check that the suffix is alread exported, if not
make the RUV entry pending and write it after all othere entries

Reviewed by: ?

firstyear commented on line 112 of dirsrvtests/tests/tickets/ticket50232_test.py 5 years ago

I would think the changelog creation is ensured during the topology setup? But this doesn't really hurt here either.

firstyear commented 5 years ago

Ignore my last comment, you are doing a standalone, and enabling their changelogs.

firstyear commented 5 years ago

I think the test looks good, and the C patch looks fine too. I think I would like to see some more comments in the C code explaining the conditions and what is occuring to prompt these checks being revelant, I think reading this test and that patch, I'm hazy on what the fault is, and how this fixes it.

firstyear commented on line 21 of ldap/servers/slapd/back-ldbm/ldif2ldbm.c 5 years ago

int32_t?