From 1747f910df2e95977ad8f2d354c25a2760e5a64b Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Nov 06 2019 16:08:50 +0000 Subject: Issue 50689 - Failed db restore task does not report an error Bug Description: If you have a back up that contains a backend that is not configured the restore fails, but a success return code is returned to the client. This happens becuase the return code gets overwritten after the failure. Fix Description: Preserve the error code upon failure and properly update the task exit code. relates: https://pagure.io/389-ds-base/issue/50689 Reviewed by: tboardaz & lkrispen(Thanks!!) Never rewrite the orginal error code --- diff --git a/dirsrvtests/tests/suites/backups/backup_test.py b/dirsrvtests/tests/suites/backups/backup_test.py new file mode 100644 index 0000000..e938914 --- /dev/null +++ b/dirsrvtests/tests/suites/backups/backup_test.py @@ -0,0 +1,73 @@ +import logging +import pytest +import os +from datetime import datetime +from lib389._constants import DEFAULT_SUFFIX, INSTALL_LATEST_CONFIG +from lib389.properties import BACKEND_SAMPLE_ENTRIES, TASK_WAIT +from lib389.topologies import topology_st as topo +from lib389.backend import Backend +from lib389.tasks import BackupTask, RestoreTask + +DEBUGGING = os.getenv("DEBUGGING", default=False) +if DEBUGGING: + logging.getLogger(__name__).setLevel(logging.DEBUG) +else: + logging.getLogger(__name__).setLevel(logging.INFO) +log = logging.getLogger(__name__) + + +def test_missing_backend(topo): + """Test that an error is returned when a restore is performed for a + backend that is no longer present. + + :id: 889b8028-35cf-41d7-91f6-bc5193683646 + :setup: Standalone Instance + :steps: + 1. Create a second backend + 2. Perform a back up + 3. Remove one of the backends from the config + 4. Perform a restore + :expectedresults: + 1. Success + 2. Success + 3. Success + 4. Failure + """ + + # Create a new backend + BE_NAME = 'backupRoot' + BE_SUFFIX = 'dc=back,dc=up' + props = { + 'cn': BE_NAME, + 'nsslapd-suffix': BE_SUFFIX, + BACKEND_SAMPLE_ENTRIES: INSTALL_LATEST_CONFIG + } + be = Backend(topo.standalone) + backend_entry = be.create(properties=props) + + # perform backup + backup_dir_name = "backup-%s" % datetime.now().strftime("%Y_%m_%d_%H_%M_%S") + archive = os.path.join(topo.standalone.ds_paths.backup_dir, backup_dir_name) + backup_task = BackupTask(topo.standalone) + task_properties = {'nsArchiveDir': archive} + backup_task.create(properties=task_properties) + backup_task.wait() + assert backup_task.get_exit_code() == 0 + + # Remove new backend + backend_entry.delete() + + # Restore the backup - it should fail + restore_task = RestoreTask(topo.standalone) + task_properties = {'nsArchiveDir': archive} + restore_task.create(properties=task_properties) + restore_task.wait() + assert restore_task.get_exit_code() != 0 + + +if __name__ == '__main__': + # Run isolated + # -s for DEBUG mode + CURRENT_FILE = os.path.realpath(__file__) + pytest.main(["-s", CURRENT_FILE]) + diff --git a/ldap/servers/slapd/back-ldbm/archive.c b/ldap/servers/slapd/back-ldbm/archive.c index e103395..4eb0eb5 100644 --- a/ldap/servers/slapd/back-ldbm/archive.c +++ b/ldap/servers/slapd/back-ldbm/archive.c @@ -19,14 +19,14 @@ int ldbm_back_archive2ldbm(Slapi_PBlock *pb) { struct ldbminfo *li; - char *rawdirectory = NULL; /* -a */ - char *directory = NULL; /* normalized */ - int return_value = -1; - int task_flags = 0; - int run_from_cmdline = 0; Slapi_Task *task; - int is_old_to_new = 0; ldbm_instance *inst = NULL; + char *rawdirectory = NULL; /* -a */ + char *directory = NULL; /* normalized */ + int32_t return_value = -1; + int32_t task_flags = 0; + int32_t run_from_cmdline = 0; + int32_t is_old_to_new = 0; slapi_pblock_get(pb, SLAPI_PLUGIN_PRIVATE, &li); slapi_pblock_get(pb, SLAPI_SEQ_VAL, &rawdirectory); @@ -165,8 +165,7 @@ ldbm_back_archive2ldbm(Slapi_PBlock *pb) "the backup set. error=%d (%s)\n", return_value, dblayer_strerror(return_value)); if (task) { - slapi_task_log_notice(task, "Failed to read the backup file set " - "from %s", + slapi_task_log_notice(task, "Failed to read the backup file set from %s", directory); } } @@ -177,7 +176,7 @@ ldbm_back_archive2ldbm(Slapi_PBlock *pb) char *p; char c; char *bakup_dir = NULL; - int skipinit = SLAPI_UPGRADEDB_SKIPINIT; + int32_t skipinit = SLAPI_UPGRADEDB_SKIPINIT; p = strrchr(directory, '/'); if (NULL == p) { @@ -204,25 +203,26 @@ ldbm_back_archive2ldbm(Slapi_PBlock *pb) return_value = ldbm_back_upgradedb(pb); } } else { - ldbm_instance *inst; Object *inst_obj; - int ret; + int32_t ret; if (0 != return_value) { - /* error case (607331) - * just to go back to the previous state if possible */ - if ((return_value = dblayer_start(li, DBLAYER_NORMAL_MODE))) { + /* + * error case (607331) + * just to go back to the previous state if possible (preserve + * original error for now) + */ + if ((ret = dblayer_start(li, DBLAYER_NORMAL_MODE))) { slapi_log_err(SLAPI_LOG_ERR, "ldbm_back_archive2ldbm", "Unable to to start database in [%s]\n", li->li_directory); if (task) { - slapi_task_log_notice(task, "Failed to start the database in " - "%s", + slapi_task_log_notice(task, "Failed to start the database in %s", li->li_directory); } - goto out; } } + /* bring all backends and changelog back online */ plugin_call_plugins(pb, SLAPI_PLUGIN_BE_POST_OPEN_FN); for (inst_obj = objset_first_obj(li->li_instance_set); inst_obj; @@ -234,8 +234,7 @@ ldbm_back_archive2ldbm(Slapi_PBlock *pb) "ldbm_back_archive2ldbm", "Unable to restart '%s'\n", inst->inst_name); if (task) { - slapi_task_log_notice(task, "Unable to restart '%s'", - inst->inst_name); + slapi_task_log_notice(task, "Unable to restart '%s'", inst->inst_name); } } else { slapi_mtn_be_enable(inst->inst_be); @@ -243,6 +242,7 @@ ldbm_back_archive2ldbm(Slapi_PBlock *pb) } } } + out: if (run_from_cmdline && (0 == return_value)) { dblayer_restore_file_update(li, directory); diff --git a/ldap/servers/slapd/back-ldbm/db-bdb/bdb_layer.c b/ldap/servers/slapd/back-ldbm/db-bdb/bdb_layer.c index 8a51aed..55a9f25 100644 --- a/ldap/servers/slapd/back-ldbm/db-bdb/bdb_layer.c +++ b/ldap/servers/slapd/back-ldbm/db-bdb/bdb_layer.c @@ -5294,8 +5294,7 @@ bdb_restore(struct ldbminfo *li, char *src_dir, Slapi_Task *task) "exist.\n", src_dir); if (task) { - slapi_task_log_notice(task, "Restore: backup directory %s does not " - "exist.\n", + slapi_task_log_notice(task, "Restore: backup directory %s does not exist.", src_dir); } return LDAP_UNWILLING_TO_PERFORM; @@ -5304,8 +5303,7 @@ bdb_restore(struct ldbminfo *li, char *src_dir, Slapi_Task *task) "a directory.\n", src_dir); if (task) { - slapi_task_log_notice(task, "Restore: backup directory %s is not " - "a directory.\n", + slapi_task_log_notice(task, "Restore: backup directory %s is not a directory.", src_dir); } return LDAP_UNWILLING_TO_PERFORM; @@ -5345,12 +5343,13 @@ bdb_restore(struct ldbminfo *li, char *src_dir, Slapi_Task *task) inst = ldbm_instance_find_by_name(li, (char *)direntry->name); if (inst == NULL) { slapi_log_err(SLAPI_LOG_ERR, - "bdb_restore", "Target server has no %s configured\n", + "bdb_restore", "Target server has no backend (%s) configured\n", direntry->name); if (task) { slapi_task_log_notice(task, - "bdb_restore - Target server has no %s configured\n", + "bdb_restore - Target server has no backend (%s) configured", direntry->name); + slapi_task_cancel(task, LDAP_UNWILLING_TO_PERFORM); } PR_CloseDir(dirhandle); return_value = LDAP_UNWILLING_TO_PERFORM; @@ -5363,7 +5362,7 @@ bdb_restore(struct ldbminfo *li, char *src_dir, Slapi_Task *task) src_dir, inst->inst_parent_dir_name); if (task) { slapi_task_log_notice(task, - "Restore: backup dir %s and target dir %s are identical\n", + "Restore: backup dir %s and target dir %s are identical", src_dir, inst->inst_parent_dir_name); } PR_CloseDir(dirhandle); @@ -5398,7 +5397,7 @@ bdb_restore(struct ldbminfo *li, char *src_dir, Slapi_Task *task) "bdb_restore", "Failed to open the directory \"%s\"\n", real_src_dir); if (task) { slapi_task_log_notice(task, - "Restore: failed to open the directory \"%s\"\n", real_src_dir); + "Restore: failed to open the directory \"%s\"", real_src_dir); } return_value = -1; goto error_out; @@ -5431,7 +5430,7 @@ bdb_restore(struct ldbminfo *li, char *src_dir, Slapi_Task *task) changelogdir); if (task) { slapi_task_log_notice(task, - "Restore: broken changelog dir path %s\n", + "Restore: broken changelog dir path %s", changelogdir); } goto error_out; @@ -5540,8 +5539,7 @@ bdb_restore(struct ldbminfo *li, char *src_dir, Slapi_Task *task) char *dataversion = NULL; if (bdb_version_read(li, home_dir, &ldbmversion, &dataversion) != 0) { - slapi_log_err(SLAPI_LOG_WARNING, "bdb_restore", "Unable to read dbversion " - "file in %s\n", + slapi_log_err(SLAPI_LOG_WARNING, "bdb_restore", "Unable to read dbversion file in %s\n", home_dir); } else { adjust_idl_switch(ldbmversion, li); diff --git a/src/lib389/lib389/cli_conf/backup.py b/src/lib389/lib389/cli_conf/backup.py index 1e73fba..c53a39b 100644 --- a/src/lib389/lib389/cli_conf/backup.py +++ b/src/lib389/lib389/cli_conf/backup.py @@ -26,11 +26,12 @@ def backup_restore(inst, basedn, log, args): task = inst.restore_online(archive=args.archive, db_type=args.db_type) task.wait() result = task.get_exit_code() + task_log = task.get_task_log() if task.is_complete() and result == 0: log.info("The backup restore task has finished successfully") else: - raise ValueError("The backup restore task has failed with the error code: ({})".format(result)) + raise ValueError("The backup restore task has failed with the error code: {}\n{}".format(result, task_log)) def create_parser(subparsers):