Expand cleanup strategy for all use cases

It takes into accoount:
- cluster status,
- node status,
- fallback-enabled opt
- allow-partly-done opt

Author: boris-ilijic

pbm/restore/physical.go

@@ -218,9 +218,9 @@ func peekTmpPort(current int) (int, error) {
 }
 
 // close cleans up all temp restore files.
-// Based on error status it does cleanup of the node's db path or it
+// Based on cluster status it does cleanup of the node's db path or it
 // applies fallback sync recovery strategy.
-func (r *PhysRestore) close(noerr, cleanup bool) {
+func (r *PhysRestore) close(noerr bool, progress nodeStatus) {
 	if r.tmpConf != nil {
 		r.log.Debug("rm tmp conf")
 		err := os.Remove(r.tmpConf.Name())
@@ -249,9 +249,10 @@ func (r *PhysRestore) close(noerr, cleanup bool) {
 	if err != nil {
 		r.log.Warning("waiting for cluster status during cleanup: %v", err)
 	}
-	if cleanup {
-		r.resolveCleanupStrategy(cStatus)()
-	}
+
+	// resolve and exec cleanup
+	cleanup := r.resolveCleanupStrategy(cStatus, progress)
+	cleanup()
 
 	if r.fallback {
 		// free space by just deleting fallback dir in any other case
@@ -268,7 +269,7 @@ func (r *PhysRestore) close(noerr, cleanup bool) {
 
 // doFullCleanup is node's cleanup strategy for deleting the whole dbpath.
 func (r *PhysRestore) doFullCleanup() {
-	r.log.Debug("clean-up dbpath")
+	r.log.Warning("apply full cleanup strategy")
 	err := removeAll(r.dbpath, r.log, getInternalLogFileSkipRule())
 	if err != nil {
 		r.log.Error("flush dbpath %s: %v", r.dbpath, err)
@@ -278,41 +279,58 @@ func (r *PhysRestore) doFullCleanup() {
 // doFallbackCleanup is node's cleanup strategy for recovering dbpath
 // from fallbacksync dir.
 func (r *PhysRestore) doFallbackCleanup() {
-	r.log.Warning("apply db data from %s", fallbackDir)
+	r.log.Warning("apply fallback cleanup strategy: using db data from %s", fallbackDir)
 	err := r.migrateFromFallbackDirToDBDir()
 	if err != nil {
 		r.log.Error("migrate from fallback dir: %v", err)
 	}
 }
 
-// resolveCleanupStrategy is helper which returns cleanup strategy based on
-// config parameters and cluster status.
-func (r *PhysRestore) resolveCleanupStrategy(clusterStatus defs.Status) func() {
-	if !r.fallback {
-		// fallback strategy is disabled
-		return r.doFullCleanup
+func (r *PhysRestore) skipCleanup() {
+	r.log.Debug("no cleanup strategy to apply")
+}
+
+// resolveCleanupStrategy returns cleanup strategy based on config parameters, cluster status
+// and node progress.
+func (r *PhysRestore) resolveCleanupStrategy(
+	clusterStatus defs.Status,
+	progress nodeStatus,
+) func() {
+	if progress.isDbPathUntouched() {
+		return r.skipCleanup
 	}
 
-	// fallback strategy is enabled
+	// dbpath has been changed
 	switch clusterStatus {
+
 	case defs.StatusError:
-		// restore failed, fallback recovery will be applied
-		return r.doFallbackCleanup
+		if r.fallback {
+			return r.doFallbackCleanup
+		} else { // fallback is disabled
+			if progress.isForCleanup() {
+				return r.doFullCleanup
+			} else {
+				// node is in status done in this case
+				return r.skipCleanup
+			}
+		}
+
 	case defs.StatusPartlyDone:
-		if r.allowPartlyDone {
-			// cluster is partly-done with new restore, and this node will be cleaned
-			return r.doFullCleanup
-		} else {
-			// cluster is partly-done, but anyway fallback recovery will be applied
+		if r.allowPartlyDone || !r.fallback {
+			if progress.isForCleanup() {
+				return r.doFullCleanup
+			} else {
+				return r.skipCleanup
+			}
+		} else { // partly-done state is not allowed and fallback is enabled
 			return r.doFallbackCleanup
 		}
+
 	case defs.StatusDone:
-		// this shouldn't happen, because at least this node failed
-		// anyway, we'll cleanup this node
-		return r.doFullCleanup
+		return r.skipCleanup
 	}
 
-	return r.doFullCleanup
+	return r.skipCleanup
 }
 
 // waitClusterStatus blocks until cluster status file is set on one of final statuses.
@@ -918,6 +936,12 @@ func (n nodeStatus) isForCleanup() bool {
 	return n&restoreStared != 0 && n&restoreDone == 0
 }
 
+// isDbPathUntouched returns true when restore progress
+// didn't do any data changes within db path.
+func (n nodeStatus) isDbPathUntouched() bool {
+	return n&restoreStared == 0
+}
+
 // isFailed returns true when internal node state didn't reach
 // done state, no matter whether it was started or not.
 func (n nodeStatus) isFailed() bool {
@@ -1032,7 +1056,7 @@ func (r *PhysRestore) Snapshot(
 			r.MarkFailed(err)
 		}
 
-		r.close(err == nil, progress.isForCleanup())
+		r.close(err == nil, progress)
 	}()
 
 	err = r.init(ctx, cmd.Name, opid, l)

pbm/restore/physical_test.go

@@ -231,13 +231,18 @@ func TestResolveCleanupStrategy(t *testing.T) {
 	const (
 		fallbackStrategy    strategyDesc = "fallback"
 		fullCleanupStrategy strategyDesc = "fullCleanup"
+		skipCleanup         strategyDesc = "skipCleanup"
+
+		nodeUntouched nodeStatus = 0
+		nodeTouched   nodeStatus = 1
 	)
 
 	testCases := []struct {
 		desc            string
 		fallback        bool
 		allowPartlyDone bool
 		clusterStatus   defs.Status
+		nodeStatus      nodeStatus
 		wantStrategy    strategyDesc
 	}{
 		// fallback enabled
@@ -246,43 +251,49 @@ func TestResolveCleanupStrategy(t *testing.T) {
 			fallback:        true,
 			allowPartlyDone: true,
 			clusterStatus:   defs.StatusError,
+			nodeStatus:      nodeTouched,
 			wantStrategy:    fallbackStrategy,
 		},
 		{
 			desc:            "fallback: enabled, allow-partly-done: enabled, status: partly-done",
 			fallback:        true,
 			allowPartlyDone: true,
 			clusterStatus:   defs.StatusPartlyDone,
+			nodeStatus:      nodeTouched,
 			wantStrategy:    fullCleanupStrategy,
 		},
 		{
 			desc:            "fallback: enabled, allow-partly-done: enabled, status: done",
 			fallback:        true,
 			allowPartlyDone: true,
 			clusterStatus:   defs.StatusDone,
-			wantStrategy:    fullCleanupStrategy,
+			nodeStatus:      nodeTouched,
+			wantStrategy:    skipCleanup,
 		},
 
 		{
 			desc:            "fallback: enabled, allow-partly-done: disabled, status: error",
 			fallback:        true,
 			allowPartlyDone: false,
 			clusterStatus:   defs.StatusError,
+			nodeStatus:      nodeTouched,
 			wantStrategy:    fallbackStrategy,
 		},
 		{
 			desc:            "fallback: enabled, allow-partly-done: disabled, status: partly-done",
 			fallback:        true,
 			allowPartlyDone: false,
 			clusterStatus:   defs.StatusPartlyDone,
+			nodeStatus:      nodeTouched,
 			wantStrategy:    fallbackStrategy,
 		},
 		{
 			desc:            "fallback: enabled, allow-partly-done: disabled, status: done",
 			fallback:        true,
 			allowPartlyDone: false,
 			clusterStatus:   defs.StatusDone,
-			wantStrategy:    fullCleanupStrategy,
+			nodeStatus:      nodeTouched,
+			wantStrategy:    skipCleanup,
 		},
 
 		// fallback disabled
@@ -291,43 +302,66 @@ func TestResolveCleanupStrategy(t *testing.T) {
 			fallback:        false,
 			allowPartlyDone: true,
 			clusterStatus:   defs.StatusError,
+			nodeStatus:      nodeTouched,
 			wantStrategy:    fullCleanupStrategy,
 		},
 		{
 			desc:            "fallback: disabled, allow-partly-done: enabled, status: partly-done",
 			fallback:        false,
 			allowPartlyDone: true,
 			clusterStatus:   defs.StatusPartlyDone,
+			nodeStatus:      nodeTouched,
 			wantStrategy:    fullCleanupStrategy,
 		},
 		{
 			desc:            "fallback: disabled, allow-partly-done: enabled, status: done",
 			fallback:        false,
 			allowPartlyDone: true,
 			clusterStatus:   defs.StatusDone,
-			wantStrategy:    fullCleanupStrategy,
+			nodeStatus:      nodeTouched,
+			wantStrategy:    skipCleanup,
 		},
-
 		{
 			desc:            "fallback: disabled, allow-partly-done: disabled, status: error",
 			fallback:        false,
 			allowPartlyDone: false,
 			clusterStatus:   defs.StatusError,
+			nodeStatus:      nodeTouched,
 			wantStrategy:    fullCleanupStrategy,
 		},
 		{
 			desc:            "fallback: disabled, allow-partly-done: disabled, status: partly-done",
 			fallback:        false,
 			allowPartlyDone: false,
 			clusterStatus:   defs.StatusPartlyDone,
+			nodeStatus:      nodeTouched,
 			wantStrategy:    fullCleanupStrategy,
 		},
 		{
 			desc:            "fallback: disabled, allow-partly-done: disabled, status: done",
 			fallback:        false,
 			allowPartlyDone: false,
 			clusterStatus:   defs.StatusDone,
-			wantStrategy:    fullCleanupStrategy,
+			nodeStatus:      nodeTouched,
+			wantStrategy:    skipCleanup,
+		},
+
+		// db path is untouched
+		{
+			desc:            "fallback: enabled, allow-partly-done: enabled, status: error",
+			fallback:        true,
+			allowPartlyDone: true,
+			clusterStatus:   defs.StatusError,
+			nodeStatus:      nodeUntouched,
+			wantStrategy:    skipCleanup,
+		},
+		{
+			desc:            "fallback: disabled, allow-partly-done: enabled, status: error",
+			fallback:        false,
+			allowPartlyDone: true,
+			clusterStatus:   defs.StatusError,
+			nodeStatus:      nodeUntouched,
+			wantStrategy:    skipCleanup,
 		},
 	}
 
@@ -338,7 +372,7 @@ func TestResolveCleanupStrategy(t *testing.T) {
 				allowPartlyDone: tC.allowPartlyDone,
 				log:             log.DiscardLogger.NewDefaultEvent(),
 			}
-			strategy := r.resolveCleanupStrategy(tC.clusterStatus)
+			strategy := r.resolveCleanupStrategy(tC.clusterStatus, tC.nodeStatus)
 			if tC.wantStrategy == fullCleanupStrategy {
 				if cmpStrategy(strategy) != cmpStrategy(r.doFullCleanup) {
 					t.Fatalf("want=%s", fullCleanupStrategy)
@@ -348,6 +382,10 @@ func TestResolveCleanupStrategy(t *testing.T) {
 				if cmpStrategy(strategy) != cmpStrategy(r.doFallbackCleanup) {
 					t.Fatalf("want=%s", fallbackStrategy)
 				}
+			} else if tC.wantStrategy == skipCleanup {
+				if cmpStrategy(strategy) != cmpStrategy(r.skipCleanup) {
+					t.Fatalf("want=%s", skipCleanup)
+				}
 			}
 		})
 	}