Allow to replace process groups with a high run loop busy value (#2367)

Author: johscheuer

api/v1beta2/foundationdb_status.go

@@ -198,6 +198,9 @@ type FoundationDBStatusProcessInfo struct {
 	// The time that the process has been up for.
 	UptimeSeconds float64 `json:"uptime_seconds,omitempty"`
 
+	// RunLoopBusy represents the busyness of the run loop of the fdbserver.
+	RunLoopBusy float64 `json:"run_loop_busy,omitempty"`
+
 	// Roles contains a slice of all roles of the process
 	Roles []FoundationDBStatusProcessRoleInfo `json:"roles,omitempty"`
 

api/v1beta2/foundationdb_status_test.go

@@ -119,7 +119,8 @@ var _ = Describe("FoundationDBStatus", func() {
 							},
 						},
 					},
-					Messages: []FoundationDBStatusProcessMessage{},
+					Messages:    []FoundationDBStatusProcessMessage{},
+					RunLoopBusy: 0.024864200000000003,
 				},
 				"eab0db1aa7aae81a50ca97e9814a1b7d": {
 					Address: ProcessAddress{
@@ -161,7 +162,8 @@ var _ = Describe("FoundationDBStatus", func() {
 							ID:   "dfd679875a386d06",
 						},
 					},
-					Messages: []FoundationDBStatusProcessMessage{},
+					Messages:    []FoundationDBStatusProcessMessage{},
+					RunLoopBusy: 0.0080509299999999978,
 				},
 				"f483247d4d5f279ef02c549680cbde64": {
 					Address: ProcessAddress{
@@ -208,7 +210,8 @@ var _ = Describe("FoundationDBStatus", func() {
 							},
 						},
 					},
-					Messages: []FoundationDBStatusProcessMessage{},
+					Messages:    []FoundationDBStatusProcessMessage{},
+					RunLoopBusy: 0.0101502,
 				},
 				"f6e0f7fd80da429d20329ad95d793ca3": {
 					Address: ProcessAddress{
@@ -240,7 +243,8 @@ var _ = Describe("FoundationDBStatus", func() {
 							ID:   "cbeb915c6cceb4a9",
 						},
 					},
-					Messages: []FoundationDBStatusProcessMessage{},
+					Messages:    []FoundationDBStatusProcessMessage{},
+					RunLoopBusy: 0.027578200000000001,
 				},
 				"f75644abdf1b06c803b5c3c124fdd0a0": {
 					Address: ProcessAddress{
@@ -264,7 +268,8 @@ var _ = Describe("FoundationDBStatus", func() {
 							ID:   "1f953018ad2e746f",
 						},
 					},
-					Messages: []FoundationDBStatusProcessMessage{},
+					Messages:    []FoundationDBStatusProcessMessage{},
+					RunLoopBusy: 0.0075524900000000002,
 				},
 				"105bf6c041f8ec315d03e889c2746ecf": {
 					Address: ProcessAddress{
@@ -292,7 +297,8 @@ var _ = Describe("FoundationDBStatus", func() {
 							KVStoreAvailableBytes: ptr.To[int64](84178214912),
 						},
 					},
-					Messages: []FoundationDBStatusProcessMessage{},
+					Messages:    []FoundationDBStatusProcessMessage{},
+					RunLoopBusy: 0.0081051000000000005,
 				},
 				"78c1c84af4481f0df628d40358f0930a": {
 					Address: ProcessAddress{
@@ -320,7 +326,8 @@ var _ = Describe("FoundationDBStatus", func() {
 							KVStoreAvailableBytes: ptr.To[int64](84178214912),
 						},
 					},
-					Messages: []FoundationDBStatusProcessMessage{},
+					Messages:    []FoundationDBStatusProcessMessage{},
+					RunLoopBusy: 0.0088742700000000001,
 				},
 				"83084479b50c9c3a09b0286297be3796": {
 					Address: ProcessAddress{
@@ -348,7 +355,8 @@ var _ = Describe("FoundationDBStatus", func() {
 							KVStoreAvailableBytes: ptr.To[int64](84178202624),
 						},
 					},
-					Messages: []FoundationDBStatusProcessMessage{},
+					Messages:    []FoundationDBStatusProcessMessage{},
+					RunLoopBusy: 0.0089497099999999979,
 				},
 			},
 			Data: FoundationDBStatusDataStatistics{

api/v1beta2/foundationdbcluster_types.go

@@ -68,16 +68,20 @@ type FoundationDBClusterList struct {
 	Items           []FoundationDBCluster `json:"items"`
 }
 
-var conditionsThatNeedReplacement = []ProcessGroupConditionType{
-	MissingProcesses,
-	PodFailing,
-	MissingPod,
-	MissingPVC,
-	MissingService,
-	PodPending,
-	NodeTaintReplacing,
-	ProcessIsMarkedAsExcluded,
-	ProcessHasIOError,
+// TODO (johscheuer): I think it would make sense to expose this as a setting in the FoundationDBCluster automation options
+// that way users can define what conditions should be used for the replacement logic.
+var defaultConditionsThatNeedReplacement = map[ProcessGroupConditionType]None{
+	MissingProcesses:          {},
+	PodFailing:                {},
+	MissingPod:                {},
+	MissingPVC:                {},
+	MissingService:            {},
+	PodPending:                {},
+	NodeTaintReplacing:        {},
+	ProcessIsMarkedAsExcluded: {},
+	ProcessHasIOError:         {},
+	SidecarUnreachable:        {},
+	ProcessHasHighRunLoopBusy: {},
 }
 
 const (
@@ -547,12 +551,13 @@ func (processGroupStatus *ProcessGroupStatus) GetPvcName(cluster *FoundationDBCl
 	return fmt.Sprintf("%s-data", processGroupStatus.GetPodName(cluster))
 }
 
-// NeedsReplacement checks if the ProcessGroupStatus has conditions that require a replacement of the failed Process Group.
+// NeedsReplacementWithConditions checks if the ProcessGroupStatus has conditions that require a replacement of the failed Process Group.
 // The method will return the failure condition and the timestamp. If no failure is detected an empty condition and a 0
-// will be returned.
-func (processGroupStatus *ProcessGroupStatus) NeedsReplacement(
+// will be returned. The conditions that should trigger a replacement can be passed to this method.
+func (processGroupStatus *ProcessGroupStatus) NeedsReplacementWithConditions(
 	failureTime int,
 	taintReplacementTime int,
+	conditionsThatNeedReplacement map[ProcessGroupConditionType]None,
 ) (ProcessGroupConditionType, int64) {
 	var earliestFailureTime int64 = math.MaxInt64
 	var earliestTaintReplacementTime int64 = math.MaxInt64
@@ -563,30 +568,38 @@ func (processGroupStatus *ProcessGroupStatus) NeedsReplacement(
 	}
 
 	var failureCondition ProcessGroupConditionType
-	for _, conditionType := range conditionsThatNeedReplacement {
-		conditionTimePtr := processGroupStatus.GetConditionTime(conditionType)
-		if conditionTimePtr == nil {
+
+	// Iterate over all the conditions that the process group has, under normal circumstances the process group
+	// should have no oder a minimal set of conditions. If any of the condition is part of the conditionsThatNeedReplacement
+	// check how long the condition is present and check if the process group should be replaced.
+	var hasConditionThatRequiresReplacement bool
+	for _, condition := range processGroupStatus.ProcessGroupConditions {
+		_, ok := conditionsThatNeedReplacement[condition.ProcessGroupConditionType]
+		if !ok {
 			continue
 		}
 
-		conditionTime := *conditionTimePtr
-		if conditionType == NodeTaintReplacing {
-			if earliestTaintReplacementTime > conditionTime {
-				earliestTaintReplacementTime = conditionTime
+		hasConditionThatRequiresReplacement = true
+		if condition.ProcessGroupConditionType == NodeTaintReplacing {
+			if earliestTaintReplacementTime > condition.Timestamp {
+				earliestTaintReplacementTime = condition.Timestamp
 			}
 
-			failureCondition = conditionType
+			failureCondition = condition.ProcessGroupConditionType
 			continue
 		}
 
-		if earliestFailureTime > conditionTime {
-			earliestFailureTime = conditionTime
-			failureCondition = conditionType
+		if earliestFailureTime > condition.Timestamp {
+			earliestFailureTime = condition.Timestamp
+			failureCondition = condition.ProcessGroupConditionType
 		}
 	}
 
-	failureWindowStart := time.Now().Add(-1 * time.Duration(failureTime) * time.Second).Unix()
-	if earliestFailureTime < failureWindowStart {
+	if !hasConditionThatRequiresReplacement {
+		return "", 0
+	}
+
+	if earliestFailureTime < time.Now().Add(-1*time.Duration(failureTime)*time.Second).Unix() {
 		return failureCondition, earliestFailureTime
 	}
 
@@ -601,6 +614,21 @@ func (processGroupStatus *ProcessGroupStatus) NeedsReplacement(
 	return "", 0
 }
 
+// NeedsReplacement checks if the ProcessGroupStatus has conditions that require a replacement of the failed Process Group.
+// The method will return the failure condition and the timestamp. If no failure is detected an empty condition and a 0
+// will be returned.
+// Deprecated: Use NeedsReplacementWithConditions.
+func (processGroupStatus *ProcessGroupStatus) NeedsReplacement(
+	failureTime int,
+	taintReplacementTime int,
+) (ProcessGroupConditionType, int64) {
+	return processGroupStatus.NeedsReplacementWithConditions(
+		failureTime,
+		taintReplacementTime,
+		defaultConditionsThatNeedReplacement,
+	)
+}
+
 // AddAddresses adds the new address to the ProcessGroupStatus and removes duplicates and old addresses
 // if the process group is not marked as removal.
 func (processGroupStatus *ProcessGroupStatus) AddAddresses(
@@ -1071,6 +1099,9 @@ const (
 	// This condition can occur during the migration of the image type, the change of the image configuration
 	// for the sidecar or during version incompatible upgrades until the sidecar is updated to the new desired version.
 	IncorrectSidecarImage ProcessGroupConditionType = "IncorrectSidecarImage"
+	// ProcessHasHighRunLoopBusy represents a process group that has a high run loop busy value. A high run loop busy
+	// value can be caused by infrastructure issues or by overloaded processes.
+	ProcessHasHighRunLoopBusy ProcessGroupConditionType = "ProcessHasHighRunLoopBusy"
 )
 
 // AllProcessGroupConditionTypes returns all ProcessGroupConditionType
@@ -1093,6 +1124,7 @@ func AllProcessGroupConditionTypes() []ProcessGroupConditionType {
 		ProcessIsMarkedAsExcluded,
 		ProcessHasIOError,
 		IncorrectSidecarImage,
+		ProcessHasHighRunLoopBusy,
 	}
 }
 
@@ -1137,6 +1169,8 @@ func GetProcessGroupConditionType(
 		return ProcessHasIOError, nil
 	case "IncorrectSidecarImage":
 		return IncorrectSidecarImage, nil
+	case "ProcessHasHighRunLoopBusy":
+		return ProcessHasHighRunLoopBusy, nil
 	}
 
 	return "", fmt.Errorf("unknown process group condition type: %s", processGroupConditionType)
@@ -1759,7 +1793,14 @@ func (cluster *FoundationDBCluster) CheckReconciliation(log logr.Logger) (bool,
 				0,
 				len(processGroup.ProcessGroupConditions),
 			)
+
 			for _, condition := range processGroup.ProcessGroupConditions {
+				// The ProcessHasHighRunLoopBusy is currently only informational and shouldn't block the reconciliation.
+				if condition.ProcessGroupConditionType == ProcessHasHighRunLoopBusy {
+					logger.V(1).
+						Info("Detected process with high run loop busy value", "processGroupID", processGroup.ProcessGroupID)
+				}
+
 				// If there is at least one process with an incorrect command line, that means the operator has to restart
 				// processes.
 				if condition.ProcessGroupConditionType == IncorrectCommandLine &&
@@ -1780,18 +1821,20 @@ func (cluster *FoundationDBCluster) CheckReconciliation(log logr.Logger) (bool,
 				conditions = append(conditions, condition.ProcessGroupConditionType)
 			}
 
-			logger.Info(
-				"Has unhealthy process group",
-				"processGroupID",
-				processGroup.ProcessGroupID,
-				"state",
-				"HasUnhealthyProcess",
-				"conditions",
-				conditions,
-			)
-			cluster.Status.Generations.HasUnhealthyProcess = cluster.Generation
-			reconciled = false
-			continue
+			if len(conditions) > 0 {
+				logger.Info(
+					"Has unhealthy process group",
+					"processGroupID",
+					processGroup.ProcessGroupID,
+					"state",
+					"HasUnhealthyProcess",
+					"conditions",
+					conditions,
+				)
+				cluster.Status.Generations.HasUnhealthyProcess = cluster.Generation
+				reconciled = false
+				continue
+			}
 		}
 
 		cluster.Status.ReconciledProcessGroups++
@@ -3609,3 +3652,8 @@ func (cluster *FoundationDBCluster) GetDatabaseInteractionMode() DatabaseInterac
 
 	return *cluster.Spec.AutomationOptions.DatabaseInteractionMode
 }
+
+// GetConditionsThatNeedReplacement returns the conditions that should trigger a replacement.
+func (cluster *FoundationDBCluster) GetConditionsThatNeedReplacement() map[ProcessGroupConditionType]None {
+	return defaultConditionsThatNeedReplacement
+}

controllers/cluster_controller.go

@@ -141,6 +141,8 @@ type FoundationDBClusterReconciler struct {
 	ClusterLabelKeyForNodeTrigger string
 	decodingSerializer            runtime.Serializer
 	SimulationOptions             SimulationOptions
+	// Defines the threshold for the high run loop busy condition, the default is 1.0.
+	HighRunLoopBusyThreshold float64
 }
 
 // NewFoundationDBClusterReconciler creates a new FoundationDBClusterReconciler with defaults.

controllers/replace_failed_process_groups_test.go

@@ -795,6 +795,56 @@ var _ = Describe("replace_failed_process_groups", func() {
 					})
 				},
 			)
+
+			When("a process has the ProcessHasHighRunLoopBusy condition",
+				func() {
+					BeforeEach(func() {
+						processGroup.ProcessGroupConditions = append(
+							processGroup.ProcessGroupConditions,
+							&fdbv1beta2.ProcessGroupCondition{
+								ProcessGroupConditionType: fdbv1beta2.ProcessHasHighRunLoopBusy,
+								Timestamp:                 time.Now().Add(-1 * time.Hour).Unix(),
+							},
+						)
+					})
+
+					It("should requeue", func() {
+						Expect(result).NotTo(BeNil())
+					})
+
+					It("should mark the process group to be removed", func() {
+						removedIDs := getRemovedProcessGroupIDs(cluster)
+						Expect(removedIDs).To(HaveLen(1))
+						Expect(
+							removedIDs,
+						).To(ConsistOf([]fdbv1beta2.ProcessGroupID{processGroup.ProcessGroupID}))
+					})
+				})
+
+			When("a process has the SidecarUnreachable condition",
+				func() {
+					BeforeEach(func() {
+						processGroup.ProcessGroupConditions = append(
+							processGroup.ProcessGroupConditions,
+							&fdbv1beta2.ProcessGroupCondition{
+								ProcessGroupConditionType: fdbv1beta2.SidecarUnreachable,
+								Timestamp:                 time.Now().Add(-1 * time.Hour).Unix(),
+							},
+						)
+					})
+
+					It("should requeue", func() {
+						Expect(result).NotTo(BeNil())
+					})
+
+					It("should mark the process group to be removed", func() {
+						removedIDs := getRemovedProcessGroupIDs(cluster)
+						Expect(removedIDs).To(HaveLen(1))
+						Expect(
+							removedIDs,
+						).To(ConsistOf([]fdbv1beta2.ProcessGroupID{processGroup.ProcessGroupID}))
+					})
+				})
 		})
 
 		When("fault domain replacements are enabled", func() {

controllers/suite_test.go

@@ -214,5 +214,6 @@ func createTestClusterReconciler() *FoundationDBClusterReconciler {
 		DatabaseClientProvider:       mock.DatabaseClientProvider{},
 		MaintenanceListStaleDuration: 4 * time.Hour,
 		MaintenanceListWaitDuration:  5 * time.Minute,
+		HighRunLoopBusyThreshold:     1.0,
 	}
 }

controllers/update_status.go

@@ -372,7 +372,7 @@ func checkAndSetProcessStatus(
 		return nil
 	}
 
-	var excluded, hasIncorrectCommandLine, hasMissingProcesses, sidecarUnreachable, hasIOError bool
+	var excluded, hasIncorrectCommandLine, hasMissingProcesses, sidecarUnreachable, hasIOError, hasHighRunLoopBusy bool
 	var substitutions map[string]string
 	var err error
 
@@ -438,6 +438,13 @@ func checkAndSetProcessStatus(
 				excluded = true
 			}
 
+			// Check if the process has a high run loop busy value. If multiple processes are running inside this pod
+			// a one process with a high busy value will be enough to flag this process groups with the fdbv1beta2.ProcessHasHighRunLoopBusy
+			// condition.
+			if process.RunLoopBusy >= r.HighRunLoopBusyThreshold && !hasHighRunLoopBusy {
+				hasHighRunLoopBusy = true
+			}
+
 			if len(substitutions) == 0 {
 				continue
 			}
@@ -519,6 +526,7 @@ func checkAndSetProcessStatus(
 
 	processGroupStatus.UpdateCondition(fdbv1beta2.ProcessIsMarkedAsExcluded, excluded)
 	processGroupStatus.UpdateCondition(fdbv1beta2.ProcessHasIOError, hasIOError)
+	processGroupStatus.UpdateCondition(fdbv1beta2.ProcessHasHighRunLoopBusy, hasHighRunLoopBusy)
 	// If the sidecar is unreachable we are not able to compute the desired commandline.
 	if sidecarUnreachable {
 		return nil