Issue 6229 - After an initial failure, subsequent online backups fail (…

…#6230)

* Issue 6229 - After an initial failure, subsequent online backups will not work

Several issues related to backup task error handling:
Backends stay busy after the failure
Exit code is 0 in some cases
Crash if failing to open the backup directory
And a more general one:
lib389 Task DN collision

Solutions:
Always reset the busy flags that have been set
Ensure that 0 is not returned in error case
Avoid closing NULL directory descriptor
Use a timestamp having milliseconds precision to create the task DN

Issue: #6229

Reviewed by: @droideck (Thanks!)

(cherry picked from commit 04a0b6a)

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

@@ -16,6 +16,8 @@
 #include "back-ldbm.h"
 #include "dblayer.h"
 
+#define NO_OBJECT ((Object*)-1)
+
 int
 ldbm_temporary_close_all_instances(Slapi_PBlock *pb)
 {
@@ -270,6 +272,7 @@ ldbm_back_ldbm2archive(Slapi_PBlock *pb)
     int run_from_cmdline = 0;
     Slapi_Task *task;
     struct stat sbuf;
+    Object *last_busy_inst_obj = NO_OBJECT;
 
     slapi_pblock_get(pb, SLAPI_PLUGIN_PRIVATE, &li);
     slapi_pblock_get(pb, SLAPI_SEQ_VAL, &rawdirectory);
@@ -380,13 +383,12 @@ ldbm_back_ldbm2archive(Slapi_PBlock *pb)
 
     /* to avoid conflict w/ import, do this check for commandline, as well */
     {
-        Object *inst_obj, *inst_obj2;
         ldbm_instance *inst = NULL;
 
         /* server is up -- mark all backends busy */
-        for (inst_obj = objset_first_obj(li->li_instance_set); inst_obj;
-             inst_obj = objset_next_obj(li->li_instance_set, inst_obj)) {
-            inst = (ldbm_instance *)object_get_data(inst_obj);
+        for (last_busy_inst_obj = objset_first_obj(li->li_instance_set); last_busy_inst_obj;
+             last_busy_inst_obj = objset_next_obj(li->li_instance_set, last_busy_inst_obj)) {
+            inst = (ldbm_instance *)object_get_data(last_busy_inst_obj);
 
             /* check if an import/restore is already ongoing... */
             if (instance_set_busy(inst) != 0 || dblayer_in_import(inst) != 0) {
@@ -400,20 +402,6 @@ ldbm_back_ldbm2archive(Slapi_PBlock *pb)
                                           "another task and cannot be disturbed.",
                                           inst->inst_name);
                 }
-
-                /* painfully, we have to clear the BUSY flags on the
-                 * backends we'd already marked...
-                 */
-                for (inst_obj2 = objset_first_obj(li->li_instance_set);
-                     inst_obj2 && (inst_obj2 != inst_obj);
-                     inst_obj2 = objset_next_obj(li->li_instance_set,
-                                                 inst_obj2)) {
-                    inst = (ldbm_instance *)object_get_data(inst_obj2);
-                    instance_set_not_busy(inst);
-                }
-                if (inst_obj2 && inst_obj2 != inst_obj)
-                    object_release(inst_obj2);
-                object_release(inst_obj);
                 goto err;
             }
         }
@@ -427,18 +415,26 @@ ldbm_back_ldbm2archive(Slapi_PBlock *pb)
         goto err;
     }
 
-    if (!run_from_cmdline) {
+err:
+    /* Clear all BUSY flags that have been previously set */
+    if (last_busy_inst_obj != NO_OBJECT) {
         ldbm_instance *inst;
         Object *inst_obj;
 
-        /* none of these backends are busy anymore */
-        for (inst_obj = objset_first_obj(li->li_instance_set); inst_obj;
+        for (inst_obj = objset_first_obj(li->li_instance_set);
+             inst_obj && (inst_obj != last_busy_inst_obj);
              inst_obj = objset_next_obj(li->li_instance_set, inst_obj)) {
             inst = (ldbm_instance *)object_get_data(inst_obj);
             instance_set_not_busy(inst);
         }
+        if (last_busy_inst_obj != NULL) {
+            /* release last seen object for aborted objset_next_obj iterations */
+            if (inst_obj != NULL) {
+                object_release(inst_obj);
+            }
+            object_release(last_busy_inst_obj);
+        }
     }
-err:
     if (return_value) {
         if (dir_bak) {
             slapi_log_err(SLAPI_LOG_ERR,
@@ -727,7 +723,10 @@ ldbm_archive_config(char *bakdir, Slapi_Task *task)
     }
 
 error:
-    PR_CloseDir(dirhandle);
+    if (NULL != dirhandle) {
+        PR_CloseDir(dirhandle);
+        dirhandle = NULL;
+    }
     dse_backup_unlock();
     slapi_ch_free_string(&backup_config_dir);
     slapi_ch_free_string(&dse_file);

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

@@ -983,6 +983,9 @@ dbmdb_backup(struct ldbminfo *li, char *dest_dir, Slapi_Task *task)
     if (ldbm_archive_config(dest_dir, task) != 0) {
         slapi_log_err(SLAPI_LOG_ERR, "dbmdb_backup",
                 "Backup of config files failed or is incomplete\n");
+         if (0 == return_value) {
+            return_value = -1;
+        }
     }
 
     goto bail;

src/lib389/lib389/__init__.py

@@ -69,7 +69,7 @@
     get_user_is_root)
 from lib389.paths import Paths
 from lib389.nss_ssl import NssSsl
-from lib389.tasks import BackupTask, RestoreTask
+from lib389.tasks import BackupTask, RestoreTask, Task
 from lib389.dseldif import DSEldif
 
 # mixin
@@ -1424,7 +1424,7 @@ def backupFS(self):
                                        name, self.ds_paths.prefix)
 
         # create the archive
-        name = "backup_%s_%s.tar.gz" % (self.serverid, time.strftime("%m%d%Y_%H%M%S"))
+        name = "backup_%s_%s.tar.gz" % (self.serverid, Task.get_timestamp())
         backup_file = os.path.join(backup_dir, name)
         tar = tarfile.open(backup_file, "w:gz")
         tar.extraction_filter = (lambda member, path: member)
@@ -2810,7 +2810,7 @@ def db2ldif(self, bename, suffixes, excludeSuffixes, encrypt, repl_data,
         else:
             # No output file specified.  Use the default ldif location/name
             cmd.append('-a')
-            tnow = datetime.now().strftime("%Y_%m_%d_%H_%M_%S")
+            tnow = Task.get_timestamp()
             if bename:
                 ldifname = os.path.join(self.ds_paths.ldif_dir, "%s-%s-%s.ldif" % (self.serverid, bename, tnow))
             else:
@@ -2881,7 +2881,7 @@ def db2bak(self, archive_dir):
 
         if archive_dir is None:
             # Use the instance name and date/time as the default backup name
-            tnow = datetime.now().strftime("%Y_%m_%d_%H_%M_%S")
+            tnow = Task.get_timestamp()
             archive_dir = os.path.join(self.ds_paths.backup_dir, "%s-%s" % (self.serverid, tnow))
         elif not archive_dir.startswith("/"):
             # Relative path, append it to the bak directory
@@ -3506,7 +3506,7 @@ def backup_online(self, archive=None, db_type=None):
 
         if archive is None:
             # Use the instance name and date/time as the default backup name
-            tnow = datetime.now().strftime("%Y_%m_%d_%H_%M_%S")
+            tnow = Task.get_timestamp()
             if self.serverid is not None:
                 backup_dir_name = "%s-%s" % (self.serverid, tnow)
             else: