Allow the pod metadata updater to update the metdadata for Pods marked as removal (#2339)

* Allow the pod metadata updater to update the metdadata for Pods marked as removal

Author: johscheuer

controllers/update_metadata.go

@@ -44,8 +44,8 @@ func (updateMetadata) reconcile(
 ) *requeue {
 	var shouldRequeue bool
 	for _, processGroup := range cluster.Status.ProcessGroups {
-		if processGroup.IsMarkedForRemoval() {
-			logger.V(1).Info("Ignore process group marked for removal",
+		if processGroup.GetConditionTime(fdbv1beta2.ResourcesTerminating) != nil {
+			logger.V(1).Info("Ignore process group that is stuck in terminating state",
 				"processGroupID", processGroup.ProcessGroupID)
 			continue
 		}

controllers/update_pod_config.go

@@ -63,7 +63,7 @@ func (updatePodConfig) reconcile(
 	for _, processGroup := range cluster.Status.ProcessGroups {
 		curLogger := logger.WithValues("processGroupID", processGroup.ProcessGroupID)
 		if processGroup.GetConditionTime(fdbv1beta2.ResourcesTerminating) != nil {
-			curLogger.V(1).Info("Ignore process group marked that is stuck terminating")
+			curLogger.V(1).Info("Ignore process group that is stuck terminating")
 			continue
 		}
 

controllers/update_status.go

@@ -493,6 +493,9 @@ func checkAndSetProcessStatus(
 
 	processGroupStatus.UpdateCondition(fdbv1beta2.MissingProcesses, hasMissingProcesses)
 	processGroupStatus.UpdateCondition(fdbv1beta2.SidecarUnreachable, sidecarUnreachable)
+	// If the processes are not up and running, we will reset the incorrect command line, e.g. in case that the processes
+	// were just restarted by the operator.
+	processGroupStatus.UpdateCondition(fdbv1beta2.IncorrectCommandLine, hasIncorrectCommandLine)
 	// If the processes are absent, we are not able to determine the state of the processes, therefore we won't change it.
 	if hasMissingProcesses {
 		return nil
@@ -520,7 +523,6 @@ func checkAndSetProcessStatus(
 	if sidecarUnreachable {
 		return nil
 	}
-	processGroupStatus.UpdateCondition(fdbv1beta2.IncorrectCommandLine, hasIncorrectCommandLine)
 
 	return nil
 }

e2e/test_operator/operator_test.go

@@ -33,6 +33,7 @@ import (
 	"bytes"
 	"context"
 	"encoding/binary"
+	"encoding/json"
 	"fmt"
 	"log"
 	"math"
@@ -2477,4 +2478,116 @@ var _ = Describe("Operator", Label("e2e", "pr"), func() {
 			})
 		})
 	})
+
+	When(
+		"a process group should be removed and deletions are disabled and a new knob is rolled out",
+		func() {
+			var processGroupID fdbv1beta2.ProcessGroupID
+			var initialCustomParameters fdbv1beta2.FoundationDBCustomParameters
+
+			BeforeEach(func() {
+				availabilityCheck = false
+
+				spec := fdbCluster.GetCluster().Spec.DeepCopy()
+				spec.AutomationOptions.DeletionMode = fdbv1beta2.PodUpdateModeNone
+				fdbCluster.UpdateClusterSpecWithSpec(spec)
+
+				storagePod := factory.RandomPickOnePod(fdbCluster.GetStoragePods().Items)
+				log.Println("Selected Pod:", storagePod.Name, " to be replaced")
+				fdbCluster.ReplacePod(storagePod, false)
+				processGroupID = fixtures.GetProcessGroupID(storagePod)
+
+				// Wait until the process group is marked for removal.
+				Eventually(func(g Gomega) {
+					processGroup := fdbv1beta2.FindProcessGroupByID(
+						fdbCluster.GetCluster().Status.ProcessGroups,
+						processGroupID,
+					)
+
+					out, err := json.Marshal(processGroup)
+					g.Expect(err).NotTo(HaveOccurred())
+					log.Println("waiting for marked to be removed", string(out))
+					g.Expect(processGroup).NotTo(BeNil())
+					g.Expect(processGroup.RemovalTimestamp).NotTo(BeNil())
+				}).WithTimeout(10 * time.Minute).WithPolling(1 * time.Second).To(Succeed())
+
+				initialCustomParameters = fdbCluster.GetCustomParameters(
+					fdbv1beta2.ProcessClassStorage,
+				)
+				newCustomParameters := append(
+					initialCustomParameters,
+					"knob_max_trace_lines=1000000",
+				)
+
+				// Disable the operator bounce feature to ensure the targeted process group sees the changes.
+				fdbCluster.SetKillProcesses(false, false)
+
+				Expect(
+					fdbCluster.SetCustomParameters(
+						map[fdbv1beta2.ProcessClass]fdbv1beta2.FoundationDBCustomParameters{
+							fdbv1beta2.ProcessClassStorage: newCustomParameters,
+						},
+						false,
+					),
+				).NotTo(HaveOccurred())
+
+				// Wait until the process group has the incorrect command line
+				Eventually(func(g Gomega) {
+					processGroup := fdbv1beta2.FindProcessGroupByID(
+						fdbCluster.GetCluster().Status.ProcessGroups,
+						processGroupID,
+					)
+
+					out, err := json.Marshal(processGroup)
+					g.Expect(err).NotTo(HaveOccurred())
+					log.Println("Waiting for incorrect commandline", string(out))
+					g.Expect(processGroup).NotTo(BeNil())
+					g.Expect(processGroup.GetConditionTime(fdbv1beta2.IncorrectCommandLine)).
+						NotTo(BeNil())
+				}).WithTimeout(10 * time.Minute).WithPolling(1 * time.Second).To(Succeed())
+			})
+
+			AfterEach(func() {
+				spec := fdbCluster.GetCluster().Spec.DeepCopy()
+				spec.AutomationOptions.DeletionMode = fdbv1beta2.PodUpdateModeZone
+				fdbCluster.UpdateClusterSpecWithSpec(spec)
+
+				Expect(fdbCluster.SetCustomParameters(
+					map[fdbv1beta2.ProcessClass]fdbv1beta2.FoundationDBCustomParameters{
+						fdbv1beta2.ProcessClassStorage: initialCustomParameters,
+					},
+					false,
+				)).NotTo(HaveOccurred())
+			})
+
+			It("should update the pod that is marked for removal", func() {
+				log.Println(
+					"Make sure process group",
+					processGroupID,
+					"will be updated",
+				)
+
+				// Enable the kill feature again.
+				fdbCluster.SetKillProcesses(true, false)
+				// Make sure the process group is updated after some time.
+				Eventually(func(g Gomega) {
+					processGroup := fdbv1beta2.FindProcessGroupByID(
+						fdbCluster.GetCluster().Status.ProcessGroups,
+						processGroupID,
+					)
+
+					out, err := json.Marshal(processGroup)
+					g.Expect(err).NotTo(HaveOccurred())
+					log.Println("Waiting for process group to be updated", string(out))
+					// Process group should still exist.
+					g.Expect(processGroup).NotTo(BeNil())
+					// Process group should be marked for removal.
+					g.Expect(processGroup.RemovalTimestamp).NotTo(BeNil())
+					// Process group should be updated with the new knobs.
+					g.Expect(processGroup.GetConditionTime(fdbv1beta2.IncorrectCommandLine)).
+						To(BeNil())
+				}).WithTimeout(10 * time.Minute).WithPolling(5 * time.Second).Should(Succeed())
+			})
+		},
+	)
 })

internal/coordination/coordination.go

@@ -26,8 +26,9 @@ import (
 	"strings"
 	"time"
 
-	fdbv1beta2 "github.com/FoundationDB/fdb-kubernetes-operator/v2/api/v1beta2"
 	"github.com/FoundationDB/fdb-kubernetes-operator/v2/internal/restarts"
+
+	fdbv1beta2 "github.com/FoundationDB/fdb-kubernetes-operator/v2/api/v1beta2"
 	"github.com/FoundationDB/fdb-kubernetes-operator/v2/pkg/fdbadminclient"
 	"github.com/go-logr/logr"
 )
@@ -400,6 +401,29 @@ func UpdateGlobalCoordinationState(
 			continue
 		}
 
+		// If the process group is missing long enough to be ignored, ensure that it's removed from the pending
+		// and the ready list.
+		if processGroup.GetConditionTime(fdbv1beta2.IncorrectCommandLine) != nil &&
+			!restarts.ShouldBeIgnoredBecauseMissing(logger, cluster, processGroup) {
+			// Check if the process group is present in pendingForRestart.
+			// If not add it to the according set.
+			if _, ok := pendingForRestart[processGroup.ProcessGroupID]; !ok {
+				updatesPendingForRestart[processGroup.ProcessGroupID] = fdbv1beta2.UpdateActionAdd
+			}
+		} else {
+			// Check if the process group is present in pendingForRestart or readyForRestart.
+			// If so, add them to the set to remove those entries as the process has the correct command line.
+			if _, ok := pendingForRestart[processGroup.ProcessGroupID]; ok {
+				logger.V(1).Info("Removing from pendingForRestart", "processGroupID", processGroup.ProcessGroupID)
+				updatesPendingForRestart[processGroup.ProcessGroupID] = fdbv1beta2.UpdateActionDelete
+			}
+
+			if _, ok := readyForRestart[processGroup.ProcessGroupID]; ok {
+				logger.V(1).Info("Removing from readyForRestart", "processGroupID", processGroup.ProcessGroupID)
+				updatesReadyForRestart[processGroup.ProcessGroupID] = fdbv1beta2.UpdateActionDelete
+			}
+		}
+
 		// Keep track of the visited process group to remove entries from removed process groups.
 		visited[processGroup.ProcessGroupID] = fdbv1beta2.None{}
 		if processGroup.IsMarkedForRemoval() {
@@ -457,44 +481,29 @@ func UpdateGlobalCoordinationState(
 						updatesReadyForInclusion[processGroup.ProcessGroupID] = fdbv1beta2.UpdateActionAdd
 					}
 				}
-
-				// if the process group is excluded, we don't need to restart it.
-				if _, ok := pendingForRestart[processGroup.ProcessGroupID]; ok {
-					logger.V(1).
-						Info("Removing from pendingForRestart", "processGroupID", processGroup.ProcessGroupID, "reason", reason)
-					updatesPendingForRestart[processGroup.ProcessGroupID] = fdbv1beta2.UpdateActionDelete
-				}
-
-				if _, ok := readyForRestart[processGroup.ProcessGroupID]; ok {
-					logger.V(1).
-						Info("Removing from readyForRestart", "processGroupID", processGroup.ProcessGroupID, "reason", reason)
-					updatesReadyForRestart[processGroup.ProcessGroupID] = fdbv1beta2.UpdateActionDelete
-				}
 			}
 
 			// If the process group is stuck in terminating, we can add it to the ready for inclusion list.
 			if processGroup.GetConditionTime(fdbv1beta2.ResourcesTerminating) != nil {
 				if _, ok := pendingForInclusion[processGroup.ProcessGroupID]; !ok {
 					logger.V(1).
-						Info("Adding to pendingForInclusion and readyForInclusion", "processGroupID", processGroup.ProcessGroupID, "reason", "process group is marked for removal and in terminating")
+						Info("Adding to pendingForInclusion and readyForInclusion", "processGroupID", processGroup.ProcessGroupID, "reason", "process group is in terminating state")
 					updatesPendingForInclusion[processGroup.ProcessGroupID] = fdbv1beta2.UpdateActionAdd
 					updatesReadyForInclusion[processGroup.ProcessGroupID] = fdbv1beta2.UpdateActionAdd
 				}
 
 				// If the process group is marked for removal and the resources are stuck in terminating or the processes are not running, we should
 				// remove them from the restart list, because there are no processes to restart.
-				if processGroup.GetConditionTime(fdbv1beta2.MissingProcesses) != nil {
-					if _, ok := pendingForRestart[processGroup.ProcessGroupID]; ok {
-						logger.V(1).
-							Info("Removing from pendingForRestart", "processGroupID", processGroup.ProcessGroupID, "reason", "process group is marked for removal")
-						updatesPendingForRestart[processGroup.ProcessGroupID] = fdbv1beta2.UpdateActionDelete
-					}
+				if _, ok := pendingForRestart[processGroup.ProcessGroupID]; ok {
+					logger.V(1).
+						Info("Removing from pendingForRestart", "processGroupID", processGroup.ProcessGroupID, "reason", "process group is in terminating state")
+					updatesPendingForRestart[processGroup.ProcessGroupID] = fdbv1beta2.UpdateActionDelete
+				}
 
-					if _, ok := readyForRestart[processGroup.ProcessGroupID]; ok {
-						logger.V(1).
-							Info("Removing from readyForRestart", "processGroupID", processGroup.ProcessGroupID, "reason", "process group is marked for removal")
-						updatesReadyForRestart[processGroup.ProcessGroupID] = fdbv1beta2.UpdateActionDelete
-					}
+				if _, ok := readyForRestart[processGroup.ProcessGroupID]; ok {
+					logger.V(1).
+						Info("Removing from readyForRestart", "processGroupID", processGroup.ProcessGroupID, "reason", "process group is in terminating state")
+					updatesReadyForRestart[processGroup.ProcessGroupID] = fdbv1beta2.UpdateActionDelete
 				}
 			}
 
@@ -507,29 +516,6 @@ func UpdateGlobalCoordinationState(
 
 			continue
 		}
-
-		// If the process group is missing long enough to be ignored, ensure that it's removed from the pending
-		// and the ready list.
-		if processGroup.GetConditionTime(fdbv1beta2.IncorrectCommandLine) != nil &&
-			!restarts.ShouldBeIgnoredBecauseMissing(logger, cluster, processGroup) {
-			// Check if the process group is present in pendingForRestart.
-			// If not add it to the according set.
-			if _, ok := pendingForRestart[processGroup.ProcessGroupID]; !ok {
-				updatesPendingForRestart[processGroup.ProcessGroupID] = fdbv1beta2.UpdateActionAdd
-			}
-		} else {
-			// Check if the process group is present in pendingForRestart or readyForRestart.
-			// If so, add them to the set to remove those entries as the process has the correct command line.
-			if _, ok := pendingForRestart[processGroup.ProcessGroupID]; ok {
-				logger.V(1).Info("Removing from pendingForRestart", "processGroupID", processGroup.ProcessGroupID)
-				updatesPendingForRestart[processGroup.ProcessGroupID] = fdbv1beta2.UpdateActionDelete
-			}
-
-			if _, ok := readyForRestart[processGroup.ProcessGroupID]; ok {
-				logger.V(1).Info("Removing from readyForRestart", "processGroupID", processGroup.ProcessGroupID)
-				updatesReadyForRestart[processGroup.ProcessGroupID] = fdbv1beta2.UpdateActionDelete
-			}
-		}
 	}
 
 	// Iterate over all the sets and mark all entries that are associated with a removed process group to be