Add additional saftey checks for bouncing and excluding processes (#1924)

johscheuer · web-flow · commit 9d6bb7d61e6e · 2024-01-25T12:22:51.000+01:00
diff --git a/api/v1beta2/foundationdb_status.go b/api/v1beta2/foundationdb_status.go
@@ -129,6 +129,14 @@ type FoundationDBStatusClusterInfo struct {
 
 	// Messages represents the possible messages that are part of the cluster information.
 	Messages []FoundationDBStatusMessage `json:"messages,omitempty"`
+
+	// BounceImpact represents the bounce_impact part of the machine-readable status.
+	BounceImpact FoundationDBBounceImpact `json:"bounce_impact,omitempty"`
+}
+
+// FoundationDBBounceImpact represents the bounce_impact part of the machine-readable status.
+type FoundationDBBounceImpact struct {
+	CanCleanBounce *bool `json:"can_clean_bounce,omitempty"`
 }
 
 // FaultTolerance provides information about the fault tolerance status
diff --git a/api/v1beta2/foundationdb_status_test.go b/api/v1beta2/foundationdb_status_test.go
@@ -22,6 +22,7 @@ package v1beta2
 
 import (
 	"encoding/json"
+	"k8s.io/utils/pointer"
 	"net"
 	"os"
 	"path/filepath"
@@ -906,6 +907,9 @@ var _ = Describe("FoundationDBStatus", func() {
 				SecondsSinceLastRecovered: 76.8155,
 			},
 			Generation: 2,
+			BounceImpact: FoundationDBBounceImpact{
+				CanCleanBounce: pointer.Bool(true),
+			},
 		}
 
 		It("should parse all values correctly", func() {
diff --git a/api/v1beta2/zz_generated.deepcopy.go b/api/v1beta2/zz_generated.deepcopy.go
diff --git a/controllers/bounce_processes.go b/controllers/bounce_processes.go
@@ -78,19 +78,25 @@ func (bounceProcesses) reconcile(ctx context.Context, r *FoundationDBClusterReco
 
 	logger.V(1).Info("processes that can be restarted", "addresses", addresses)
 
-	if minimumUptime < float64(cluster.GetMinimumUptimeSecondsForBounce()) {
-		r.Recorder.Event(cluster, corev1.EventTypeNormal, "NeedsBounce",
-			fmt.Sprintf("Spec require a bounce of some processes, but the cluster has only been up for %f seconds", minimumUptime))
+	// Check if the cluster can safely bounce processes.
+	err = fdbstatus.CanSafelyBounceProcesses(minimumUptime, float64(cluster.GetMinimumUptimeSecondsForBounce()), status)
+	if err != nil {
+		r.Recorder.Event(cluster, corev1.EventTypeNormal, "NeedsBounce", err.Error())
 		cluster.Status.Generations.NeedsBounce = cluster.ObjectMeta.Generation
 		err = r.updateOrApply(ctx, cluster)
 		if err != nil {
 			logger.Error(err, "Error updating cluster status")
 		}
 
-		// Retry after we waited the minimum uptime
+		// Retry after we waited the minimum uptime or at least 15 seconds.
+		delayTime := cluster.GetMinimumUptimeSecondsForBounce() - int(minimumUptime)
+		if delayTime < 15 {
+			delayTime = 15
+		}
+
 		return &requeue{
-			message: "Cluster needs to stabilize before bouncing",
-			delay:   time.Second * time.Duration(cluster.GetMinimumUptimeSecondsForBounce()-int(minimumUptime)),
+			message: err.Error(),
+			delay:   time.Second * time.Duration(delayTime),
 		}
 	}
 
diff --git a/controllers/exclude_processes.go b/controllers/exclude_processes.go
@@ -66,57 +66,65 @@ func (e excludeProcesses) reconcile(_ context.Context, r *FoundationDBClusterRec
 	logger.Info("current exclusions", "exclusions", exclusions)
 	fdbProcessesToExcludeByClass, ongoingExclusionsByClass := getProcessesToExclude(exclusions, cluster)
 
-	if len(fdbProcessesToExcludeByClass) > 0 {
-		var fdbProcessesToExclude []fdbv1beta2.ProcessAddress
-		desiredProcesses, err := cluster.GetProcessCountsWithDefaults()
-		if err != nil {
-			return &requeue{curError: err, delayedRequeue: true}
-		}
-
-		desiredProcessesMap := desiredProcesses.Map()
-		for processClass := range fdbProcessesToExcludeByClass {
-			contextLogger := logger.WithValues("processClass", processClass)
-			ongoingExclusions := ongoingExclusionsByClass[processClass]
-			processesToExclude := fdbProcessesToExcludeByClass[processClass]
+	// No processes have to be excluded we can directly return.
+	if len(fdbProcessesToExcludeByClass) == 0 {
+		return nil
+	}
 
-			allowedExclusions, missingProcesses := getAllowedExclusionsAndMissingProcesses(contextLogger, cluster, processClass, desiredProcessesMap[processClass], ongoingExclusions, r.InSimulation)
-			if allowedExclusions <= 0 {
-				contextLogger.Info("Waiting for missing processes before continuing with the exclusion", "missingProcesses", missingProcesses, "addressesToExclude", processesToExclude, "allowedExclusions", allowedExclusions, "ongoingExclusions", ongoingExclusions)
-				continue
-			}
+	// Make sure it's safe to exclude processes.
+	err = fdbstatus.CanSafelyExcludeProcesses(status)
+	if err != nil {
+		return &requeue{curError: err, delayedRequeue: true}
+	}
 
-			// If we are not able to exclude all processes at once print a log message.
-			if len(processesToExclude) > allowedExclusions {
-				contextLogger.Info("Some processes are still missing but continuing with the exclusion", "missingProcesses", missingProcesses, "addressesToExclude", processesToExclude, "allowedExclusions", allowedExclusions, "ongoingExclusions", ongoingExclusions)
-			}
+	var fdbProcessesToExclude []fdbv1beta2.ProcessAddress
+	desiredProcesses, err := cluster.GetProcessCountsWithDefaults()
+	if err != nil {
+		return &requeue{curError: err, delayedRequeue: true}
+	}
 
-			if len(processesToExclude) < allowedExclusions {
-				allowedExclusions = len(processesToExclude)
-			}
+	desiredProcessesMap := desiredProcesses.Map()
+	for processClass := range fdbProcessesToExcludeByClass {
+		contextLogger := logger.WithValues("processClass", processClass)
+		ongoingExclusions := ongoingExclusionsByClass[processClass]
+		processesToExclude := fdbProcessesToExcludeByClass[processClass]
 
-			// TODO: As a next step we could exclude transaction (log + stateless) processes together and exclude
-			// storage processes with a separate call. This would make sure that no storage checks will block
-			// the exclusion of transaction processes.
+		allowedExclusions, missingProcesses := getAllowedExclusionsAndMissingProcesses(contextLogger, cluster, processClass, desiredProcessesMap[processClass], ongoingExclusions, r.InSimulation)
+		if allowedExclusions <= 0 {
+			contextLogger.Info("Waiting for missing processes before continuing with the exclusion", "missingProcesses", missingProcesses, "addressesToExclude", processesToExclude, "allowedExclusions", allowedExclusions, "ongoingExclusions", ongoingExclusions)
+			continue
+		}
 
-			// Add as many processes as allowed to the exclusion list.
-			fdbProcessesToExclude = append(fdbProcessesToExclude, processesToExclude[:allowedExclusions]...)
+		// If we are not able to exclude all processes at once print a log message.
+		if len(processesToExclude) > allowedExclusions {
+			contextLogger.Info("Some processes are still missing but continuing with the exclusion", "missingProcesses", missingProcesses, "addressesToExclude", processesToExclude, "allowedExclusions", allowedExclusions, "ongoingExclusions", ongoingExclusions)
 		}
 
-		if len(fdbProcessesToExclude) == 0 {
-			return &requeue{
-				message:        "more exclusions needed but not allowed, have to wait for new processes to come up",
-				delayedRequeue: true,
-			}
+		if len(processesToExclude) < allowedExclusions {
+			allowedExclusions = len(processesToExclude)
 		}
 
-		r.Recorder.Event(cluster, corev1.EventTypeNormal, "ExcludingProcesses", fmt.Sprintf("Excluding %v", fdbProcessesToExclude))
+		// TODO: As a next step we could exclude transaction (log + stateless) processes together and exclude
+		// storage processes with a separate call. This would make sure that no storage checks will block
+		// the exclusion of transaction processes.
 
-		err = adminClient.ExcludeProcesses(fdbProcessesToExclude)
-		if err != nil {
-			return &requeue{curError: err, delayedRequeue: true}
+		// Add as many processes as allowed to the exclusion list.
+		fdbProcessesToExclude = append(fdbProcessesToExclude, processesToExclude[:allowedExclusions]...)
+	}
+
+	if len(fdbProcessesToExclude) == 0 {
+		return &requeue{
+			message:        "more exclusions needed but not allowed, have to wait for new processes to come up",
+			delayedRequeue: true,
 		}
 	}
 
+	r.Recorder.Event(cluster, corev1.EventTypeNormal, "ExcludingProcesses", fmt.Sprintf("Excluding %v", fdbProcessesToExclude))
+	err = adminClient.ExcludeProcesses(fdbProcessesToExclude)
+	if err != nil {
+		return &requeue{curError: err, delayedRequeue: true}
+	}
+
 	return nil
 }
 
diff --git a/pkg/fdbstatus/status_checks.go b/pkg/fdbstatus/status_checks.go
@@ -71,22 +71,9 @@ func getRemainingAndExcludedFromStatus(logger logr.Logger, status *fdbv1beta2.Fo
 	// the cluster status information as only the latest log processes will have the log process role. If we don't check
 	// for the active generations we have the risk to remove a log process that still has mutations on it that must be
 	// popped.
-	if status.Cluster.RecoveryState.ActiveGenerations > 1 {
-		logger.Info("Skipping exclusion check as there are multiple active generations", "activeGenerations", status.Cluster.RecoveryState.ActiveGenerations)
-		return exclusionStatus{
-			inProgress:      nil,
-			fullyExcluded:   nil,
-			notExcluded:     addresses,
-			missingInStatus: nil,
-		}
-	}
-
-	// If the database is unavailable the status might contain the processes but with missing role information. In this
-	// case we should not perform any validation on the machine-readable status as this information might not be correct.
-	// We don't want to run the exclude command to check for the status of the exclusions as this might result in a recovery
-	// of the cluster or brings the cluster into a worse state.
-	if !status.Client.DatabaseStatus.Available {
-		logger.Info("Skipping exclusion check as the database is unavailable")
+	err := DefaultSafetyChecks(status, 1, "check exclusion status")
+	if err != nil {
+		logger.Info("Skipping exclusion check as there are issues with the machine-readable status", "error", err.Error())
 		return exclusionStatus{
 			inProgress:      nil,
 			fullyExcluded:   nil,
@@ -459,3 +446,51 @@ func HasDesiredFaultToleranceFromStatus(log logr.Logger, status *fdbv1beta2.Foun
 
 	return true
 }
+
+// DefaultSafetyChecks performs a set of default safety checks, e.g. it checks if the cluster is available from the
+// client perspective and it checks that there are not too many active generations.
+func DefaultSafetyChecks(status *fdbv1beta2.FoundationDBStatus, maximumActiveGenerations int, action string) error {
+	// If there are more than 10 active generations we should not allow the cluster to bounce processes as this could
+	// cause another recovery increasing the active generations. In general the active generations should be at 1 during
+	// normal operations.
+	if status.Cluster.RecoveryState.ActiveGenerations > maximumActiveGenerations {
+		return fmt.Errorf("cluster has %d active generations, but only %d active generations are allowed to safely %s", status.Cluster.RecoveryState.ActiveGenerations, maximumActiveGenerations, action)
+	}
+
+	// If the database is unavailable we shouldn't perform any action on the cluster.
+	if !status.Client.DatabaseStatus.Available {
+		return fmt.Errorf("cluster is unavailable, cannot %s", action)
+	}
+
+	return nil
+}
+
+// CanSafelyBounceProcesses returns nil when it is safe to do a bounce on the cluster or returns an error with more information
+// why it's not safe to bounce processes in the cluster.
+func CanSafelyBounceProcesses(currentUptime float64, minimumUptime float64, status *fdbv1beta2.FoundationDBStatus) error {
+	err := DefaultSafetyChecks(status, 10, "bounce processes")
+	if err != nil {
+		return err
+	}
+
+	// If the current uptime of the cluster is below the minimum uptime we should not allow to bounce processes. This is
+	// a safeguard to reduce the risk of repeated bounces in a short timeframe.
+	if currentUptime < minimumUptime {
+		return fmt.Errorf("cluster has only been up for %.2f seconds, but must be up for %.2f seconds to safely bounce", minimumUptime, minimumUptime)
+	}
+
+	// If the machine-readable status reports that a clean bounce is not possible, we shouldn't perform a bounce. This
+	// value will be false if the cluster is not fully recovered:
+	// https://github.com/apple/foundationdb/blob/7.3.29/fdbserver/Status.actor.cpp#L437-L448
+	if status.Cluster.BounceImpact.CanCleanBounce != nil && !*status.Cluster.BounceImpact.CanCleanBounce {
+		return fmt.Errorf("cannot perform a clean bounce based on cluster status, current recovery state: %s", status.Cluster.RecoveryState.Name)
+	}
+
+	return nil
+}
+
+// CanSafelyExcludeProcesses currently performs the DefaultSafetyChecks. In the future this check might be extended to
+// perform more specific checks.
+func CanSafelyExcludeProcesses(status *fdbv1beta2.FoundationDBStatus) error {
+	return DefaultSafetyChecks(status, 10, "exclude processes")
+}
diff --git a/pkg/fdbstatus/status_checks_test.go b/pkg/fdbstatus/status_checks_test.go
