Repository: FoundationDB/fdb-kubernetes-operator
Pull request #1796 (2023-09-07 10:48:20)

Fix the lock ID to return the correct value
# Description

Fixes: https://github.com/FoundationDB/fdb-kubernetes-operator/issues/1795

## Type of change

*Please select one of the options below.*

- Bug fix (non-breaking change which fixes an issue)

## Discussion

This bug was introduced in the migration from `v1beta1` to `v1beta2`.

## Testing

e2e test will be running and I will add additional unit tests.

## Documentation

-

## Follow-up

-


foundationdb-ci: 
### Result of fdb-kubernetes-operator-pr on Linux CentOS 7

* Commit ID: 1700b4c83eed9d436cf14e3c1997a0d1dc4b999e
* Duration 4:07:08
* Result: :x: **FAILED**
* Error: `Error while executing command: if $fail_test; then exit 1; fi. Reason: exit status 1`
* [Build Log](https://d1e0l78xbkh3xa.cloudfront.net/fdb-kubernetes-operator/fdb-kubernetes-operator-pr/8f281f96-fb05-4520-ad83-82157588c7f0.gz) terminal output (available for 30 days)
* [Build Workspace](https://d1e0l78xbkh3xa.cloudfront.net/fdb-kubernetes-operator-pr/8f281f96-fb05-4520-ad83-82157588c7f0/workspace.zip) zip file of the working directory (available for 30 days)

foundationdb-ci: 
### Result of fdb-kubernetes-operator-pr on Linux CentOS 7

* Commit ID: 003976122bbb69691d6e5756d2f194b25de5a4b1
* Duration 4:06:57
* Result: :x: **FAILED**
* Error: `Error while executing command: if $fail_test; then exit 1; fi. Reason: exit status 1`
* [Build Log](https://d1e0l78xbkh3xa.cloudfront.net/fdb-kubernetes-operator/fdb-kubernetes-operator-pr/b3cab44d-893a-4681-b406-992d12dbb315.gz) terminal output (available for 30 days)
* [Build Workspace](https://d1e0l78xbkh3xa.cloudfront.net/fdb-kubernetes-operator-pr/b3cab44d-893a-4681-b406-992d12dbb315/workspace.zip) zip file of the working directory (available for 30 days)

foundationdb-ci: 
### Result of fdb-kubernetes-operator-pr on Linux CentOS 7

* Commit ID: e34363ba6449af578dbdaebe50b9788ee87c6a72
* Duration 2:38:52
* Result: :x: **FAILED**
* Error: `Error while executing command: if $fail_test; then exit 1; fi. Reason: exit status 1`
* [Build Log](https://d1e0l78xbkh3xa.cloudfront.net/fdb-kubernetes-operator/fdb-kubernetes-operator-pr/6b9a7bab-2654-4153-97e5-2c9f7df91a52.gz) terminal output (available for 30 days)
* [Build Workspace](https://d1e0l78xbkh3xa.cloudfront.net/fdb-kubernetes-operator-pr/6b9a7bab-2654-4153-97e5-2c9f7df91a52/workspace.zip) zip file of the working directory (available for 30 days)

foundationdb-ci: 
### Result of fdb-kubernetes-operator-pr on Linux CentOS 7

* Commit ID: e34363ba6449af578dbdaebe50b9788ee87c6a72
* Duration 2:43:35
* Result: :x: **FAILED**
* Error: `Error while executing command: if $fail_test; then exit 1; fi. Reason: exit status 1`
* [Build Log](https://d1e0l78xbkh3xa.cloudfront.net/fdb-kubernetes-operator/fdb-kubernetes-operator-pr/b61cdd7b-de19-4d30-9f25-6f4d01c31de9.gz) terminal output (available for 30 days)
* [Build Workspace](https://d1e0l78xbkh3xa.cloudfront.net/fdb-kubernetes-operator-pr/b61cdd7b-de19-4d30-9f25-6f4d01c31de9/workspace.zip) zip file of the working directory (available for 30 days)

foundationdb-ci: 
### Result of fdb-kubernetes-operator-pr on Linux CentOS 7

* Commit ID: 4ba1805555dc7a6338569dc7b75c7f2fc3aef773
* Duration 2:42:11
* Result: :x: **FAILED**
* Error: `Error while executing command: if $fail_test; then exit 1; fi. Reason: exit status 1`
* [Build Log](https://d1e0l78xbkh3xa.cloudfront.net/fdb-kubernetes-operator/fdb-kubernetes-operator-pr/63c19370-e89e-460b-8235-0bb87bce8f5c.gz) terminal output (available for 30 days)
* [Build Workspace](https://d1e0l78xbkh3xa.cloudfront.net/fdb-kubernetes-operator-pr/63c19370-e89e-460b-8235-0bb87bce8f5c/workspace.zip) zip file of the working directory (available for 30 days)


Patch:
diff --git a/api/v1beta2/foundationdbcluster_types.go b/api/v1beta2/foundationdbcluster_types.go
index 11db3195..131152b9 100644
--- a/api/v1beta2/foundationdbcluster_types.go
+++ b/api/v1beta2/foundationdbcluster_types.go
@@ -1848,9 +1848,9 @@ func (cluster *FoundationDBCluster) GetLockDuration() time.Duration {
 }
 
 // GetLockID gets the identifier for this instance of the operator when taking
-// locks.
+// locks. This is the `ProcessGroupIDPrefix` defined for this cluster.
 func (cluster *FoundationDBCluster) GetLockID() string {
-	return cluster.Spec.LockOptions.LockKeyPrefix
+	return cluster.Spec.ProcessGroupIDPrefix
 }
 
 // NeedsExplicitListenAddress determines whether we pass a listen address
diff --git a/fdbclient/lock_client.go b/fdbclient/lock_client.go
index 790b5009..95dde7f1 100644
--- a/fdbclient/lock_client.go
+++ b/fdbclient/lock_client.go
@@ -96,44 +96,55 @@ func (client *realLockClient) takeLockInTransaction(transaction fdb.Transaction)
 		return false, invalidLockValue{key: lockKey, value: lockValue}
 	}
 
-	ownerID, valid := lockTuple[0].(string)
+	currentLockOwnerID, valid := lockTuple[0].(string)
 	if !valid {
 		return false, invalidLockValue{key: lockKey, value: lockValue}
 	}
 
-	startTime, valid := lockTuple[1].(int64)
+	currentLockStartTime, valid := lockTuple[1].(int64)
 	if !valid {
 		return false, invalidLockValue{key: lockKey, value: lockValue}
 	}
 
-	endTime, valid := lockTuple[2].(int64)
+	currentLockEndTime, valid := lockTuple[2].(int64)
 	if !valid {
 		return false, invalidLockValue{key: lockKey, value: lockValue}
 	}
 
-	cluster := client.cluster
-	newOwnerDenied := transaction.Get(client.getDenyListKey(cluster.GetLockID())).MustGet() != nil
+	// ownerID represents the current cluster ID. If a lock is present the currentLockOwnerID represents the operator
+	// instance holding the lock.
+	ownerID := client.cluster.GetLockID()
+
+	logger := client.log.WithValues(
+		"namespace", client.cluster.Namespace,
+		"cluster", client.cluster.Name,
+		"ownerID", ownerID,
+		"currentLockOwnerID", currentLockOwnerID,
+		"startTime", time.Unix(currentLockStartTime, 0),
+		"endTime", time.Unix(currentLockEndTime, 0))
+
+	newOwnerDenied := transaction.Get(client.getDenyListKey(ownerID)).MustGet() != nil
 	if newOwnerDenied {
-		client.log.Info("Failed to get lock due to deny list", "namespace", cluster.Namespace, "cluster", cluster.Name)
+		logger.Info("Failed to get lock due to deny list")
 		return false, nil
 	}
 
-	oldOwnerDenied := transaction.Get(client.getDenyListKey(ownerID)).MustGet() != nil
-	shouldClear := endTime < time.Now().Unix() || oldOwnerDenied
+	oldOwnerDenied := transaction.Get(client.getDenyListKey(currentLockOwnerID)).MustGet() != nil
+	shouldClear := currentLockEndTime < time.Now().Unix() || oldOwnerDenied
 
 	if shouldClear {
-		client.log.Info("Clearing expired lock", "namespace", cluster.Namespace, "cluster", cluster.Name, "owner", ownerID, "startTime", time.Unix(startTime, 0), "endTime", time.Unix(endTime, 0))
-		client.updateLock(transaction, startTime)
+		logger.Info("Clearing expired lock")
+		client.updateLock(transaction, currentLockStartTime)
 		return true, nil
 	}
 
-	if ownerID == cluster.GetLockID() {
-		client.log.Info("Extending previous lock", "namespace", cluster.Namespace, "cluster", cluster.Name, "owner", ownerID, "startTime", time.Unix(startTime, 0), "endTime", time.Unix(endTime, 0))
-		client.updateLock(transaction, startTime)
+	if currentLockOwnerID == ownerID {
+		logger.Info("Extending previous lock")
+		client.updateLock(transaction, currentLockStartTime)
 		return true, nil
 	}
 
-	client.log.Info("Failed to get lock", "namespace", cluster.Namespace, "cluster", cluster.Name, "owner", ownerID, "startTime", time.Unix(startTime, 0), "endTime", time.Unix(endTime, 0))
+	logger.Info("Failed to get lock")
 
 	return false, nil
 }
@@ -146,12 +157,13 @@ func (client *realLockClient) updateLock(transaction fdb.Transaction, start int6
 		start = time.Now().Unix()
 	}
 	end := time.Now().Add(client.cluster.GetLockDuration()).Unix()
+	ownerID := client.cluster.GetLockID()
 	lockValue := tuple.Tuple{
-		client.cluster.GetLockID(),
+		ownerID,
 		start,
 		end,
 	}
-	client.log.Info("Setting new lock", "namespace", client.cluster.Namespace, "cluster", client.cluster.Name, "lockValue", lockValue)
+	client.log.Info("Setting new lock", "namespace", client.cluster.Namespace, "cluster", client.cluster.Name, "owner", ownerID, "lockValue", lockValue)
 	transaction.Set(lockKey, lockValue.Pack())
 }
 


Test patch:
diff --git a/api/v1beta2/foundationdbcluster_types_test.go b/api/v1beta2/foundationdbcluster_types_test.go
index b606bbb1..e642507c 100644
--- a/api/v1beta2/foundationdbcluster_types_test.go
+++ b/api/v1beta2/foundationdbcluster_types_test.go
@@ -5532,4 +5532,19 @@ var _ = Describe("[api] FoundationDBCluster", func() {
 				},
 			}, PodUpdateModeNone),
 	)
+
+	DescribeTable("when getting the lock ID", func(cluster *FoundationDBCluster, expected string) {
+		Expect(cluster.GetLockID()).To(Equal(expected))
+	},
+		Entry("no ProcessGroupIDPrefix is defined",
+			&FoundationDBCluster{
+				Spec: FoundationDBClusterSpec{},
+			}, ""),
+		Entry("ProcessGroupIDPrefix is defined",
+			&FoundationDBCluster{
+				Spec: FoundationDBClusterSpec{
+					ProcessGroupIDPrefix: "testing",
+				},
+			}, "testing"),
+	)
 })
diff --git a/e2e/test_operator_ha_upgrades/operator_ha_upgrade_test.go b/e2e/test_operator_ha_upgrades/operator_ha_upgrade_test.go
index 9a8eed6a..6a300ee3 100644
--- a/e2e/test_operator_ha_upgrades/operator_ha_upgrade_test.go
+++ b/e2e/test_operator_ha_upgrades/operator_ha_upgrade_test.go
@@ -111,19 +111,6 @@ func checkVersion(cluster *fixtures.HaFdbCluster, expectedVersion string) {
 	}).WithTimeout(10 * time.Minute).WithPolling(2 * time.Second).Should(BeTrue())
 }
 
-func upgradeAndVerify(cluster *fixtures.HaFdbCluster, expectedVersion string) {
-	// Upgrade the cluster and then verify that the processes have upgraded to "targetVersion".
-	startTime := time.Now()
-	Expect(cluster.UpgradeCluster(expectedVersion, true)).NotTo(HaveOccurred())
-	checkVersion(cluster, expectedVersion)
-	log.Println(
-		"Multi-DC cluster upgraded to version",
-		expectedVersion,
-		"in minutes",
-		time.Since(startTime).Minutes(),
-	)
-}
-
 // Verify that bouncing is blocked in primary/remote/primary-satellite clusters.
 // We do this by verifying that the processes in primary/remote/primary-satellite clusters
 // are at "IncorrectCommandLine" state (which means that they have the new binaries and have
@@ -166,7 +153,17 @@ var _ = Describe("Operator HA Upgrades", Label("e2e", "pr"), func() {
 		"Upgrading a multi-DC cluster without chaos",
 		func(beforeVersion string, targetVersion string) {
 			clusterSetup(beforeVersion, false)
-			upgradeAndVerify(fdbCluster, targetVersion)
+
+			// Upgrade the cluster and then verify that the processes have upgraded to "targetVersion".
+			startTime := time.Now()
+			Expect(fdbCluster.UpgradeCluster(targetVersion, true)).NotTo(HaveOccurred())
+			checkVersion(fdbCluster, targetVersion)
+			log.Println(
+				"Multi-DC cluster upgraded to version",
+				targetVersion,
+				"in minutes",
+				time.Since(startTime).Minutes(),
+			)
 		},
 		EntryDescription("Upgrade, without chaos, from %s to %s"),
 		fixtures.GenerateUpgradeTableEntries(testOptions),
@@ -312,8 +309,9 @@ var _ = Describe("Operator HA Upgrades", Label("e2e", "pr"), func() {
 				return primary.AllProcessGroupsHaveCondition(fdbv1beta2.IncorrectCommandLine)
 			}).WithTimeout(10 * time.Minute).WithPolling(5 * time.Second).MustPassRepeatedly(30).Should(BeTrue())
 
+			Expect(fdbCluster.UpgradeCluster(targetVersion, false)).NotTo(HaveOccurred())
 			// Verify that the upgrade proceeds
-			upgradeAndVerify(fdbCluster, targetVersion)
+			checkVersion(fdbCluster, targetVersion)
 		},
 		EntryDescription("Upgrade from %[1]s to %[2]s with one DC upgraded before the rest"),
 		fixtures.GenerateUpgradeTableEntries(testOptions),
@@ -443,8 +441,9 @@ var _ = Describe("Operator HA Upgrades", Label("e2e", "pr"), func() {
 		fixtures.GenerateUpgradeTableEntries(testOptions),
 	)
 
-	DescribeTable(
-		"upgrading a HA cluster with link that drops some packets",
+	// TODO(johscheuer): Enable tests again, once they are stable.
+	PDescribeTable(
+		"with network link that drops some packets",
 		func(beforeVersion string, targetVersion string) {
 			if !factory.ChaosTestsEnabled() {
 				Skip("chaos mesh is disabled")
@@ -474,101 +473,98 @@ var _ = Describe("Operator HA Upgrades", Label("e2e", "pr"), func() {
 				fixtures.PodsSelector(operatorPrimarySatellitePods.Items),
 			}, "20")
 
-			upgradeAndVerify(fdbCluster, targetVersion)
+			Expect(fdbCluster.UpgradeCluster(targetVersion, false)).NotTo(HaveOccurred())
+			// Verify that the upgrade proceeds
+			checkVersion(fdbCluster, targetVersion)
 		},
-		EntryDescription("Upgrade from %[1]s to %[2]s with network link that drops some packets"),
+		EntryDescription("Upgrade from %[1]s to %[2]s"),
 		fixtures.GenerateUpgradeTableEntries(testOptions),
 	)
 
 	DescribeTable(
-		"upgrading a cluster when no remote storage processes are restarted",
+		"when no remote storage processes are restarted",
 		func(beforeVersion string, targetVersion string) {
-			isAtLeast := factory.OperatorIsAtLeast(
-				"v1.14.0",
-			)
-
-			if !isAtLeast {
-				Skip("operator doesn't support feature for test case")
-			}
-
 			clusterSetup(beforeVersion, false)
 
 			// Select remote storage processes and use the buggify option to skip those
 			// processes during the restart command.
-			storagePods := fdbCluster.GetRemote().GetStoragePods()
-			Expect(storagePods.Items).NotTo(BeEmpty())
+			remoteProcessGroups := fdbCluster.GetRemote().GetCluster().Status.ProcessGroups
 
 			ignoreDuringRestart := make(
 				[]fdbv1beta2.ProcessGroupID,
 				0,
-				len(storagePods.Items),
+				len(remoteProcessGroups),
 			)
 
-			for _, pod := range storagePods.Items {
+			for _, processGroup := range remoteProcessGroups {
+				if processGroup.ProcessClass != fdbv1beta2.ProcessClassStorage {
+					continue
+				}
+
 				ignoreDuringRestart = append(
 					ignoreDuringRestart,
-					fdbv1beta2.ProcessGroupID(pod.Labels[fdbCluster.GetRemote().GetCachedCluster().GetProcessGroupIDLabel()]),
+					processGroup.ProcessGroupID,
 				)
 			}
 
 			log.Println(
-				"Selected Pods:",
+				"Selected Process groups:",
 				ignoreDuringRestart,
 				"to be skipped during the restart",
 			)
 			fdbCluster.GetRemote().SetIgnoreDuringRestart(ignoreDuringRestart)
 
 			// The cluster should still be able to upgrade.
-			Expect(fdbCluster.UpgradeCluster(targetVersion, true)).NotTo(HaveOccurred())
+			Expect(fdbCluster.UpgradeCluster(targetVersion, false)).NotTo(HaveOccurred())
+			// Verify that the upgrade proceeds
+			checkVersion(fdbCluster, targetVersion)
+
+			// TODO add validation here processes are updated new version
 		},
-		EntryDescription("Upgrade from %[1]s to %[2]s when no remote storage processes are restarted"),
+		EntryDescription("Upgrade from %[1]s to %[2]s"),
 		fixtures.GenerateUpgradeTableEntries(testOptions),
 	)
 
 	DescribeTable(
-		"upgrading a cluster when no remote processes are restarted",
+		"when no remote processes are restarted",
 		func(beforeVersion string, targetVersion string) {
-			isAtLeast := factory.OperatorIsAtLeast(
-				"v1.14.0",
-			)
-
-			if !isAtLeast {
-				Skip("operator doesn't support feature for test case")
-			}
-
 			clusterSetup(beforeVersion, false)
 
 			// Select remote processes and use the buggify option to skip those
 			// processes during the restart command.
-			pods := fdbCluster.GetRemote().GetPods()
-			Expect(pods.Items).NotTo(BeEmpty())
-
+			remoteProcessGroups := fdbCluster.GetRemote().GetCluster().Status.ProcessGroups
 			ignoreDuringRestart := make(
 				[]fdbv1beta2.ProcessGroupID,
 				0,
-				len(pods.Items),
+				len(remoteProcessGroups),
 			)
 
-			for _, pod := range pods.Items {
+			for _, processGroup := range remoteProcessGroups {
 				ignoreDuringRestart = append(
 					ignoreDuringRestart,
-					fdbv1beta2.ProcessGroupID(pod.Labels[fdbCluster.GetRemote().GetCachedCluster().GetProcessGroupIDLabel()]),
+					processGroup.ProcessGroupID,
 				)
 			}
 
 			log.Println(
-				"Selected Pods:",
+				"Selected Process Groups:",
 				ignoreDuringRestart,
 				"to be skipped during the restart",
 			)
+
+			// We have to set this to all clusters as any operator could be doing the cluster wide restart.
 			for _, cluster := range fdbCluster.GetAllClusters() {
 				cluster.SetIgnoreDuringRestart(ignoreDuringRestart)
 			}
 
 			// The cluster should still be able to upgrade.
-			Expect(fdbCluster.UpgradeCluster(targetVersion, true)).NotTo(HaveOccurred())
+			Expect(fdbCluster.UpgradeCluster(targetVersion, false)).NotTo(HaveOccurred())
+			// Verify that the upgrade proceeds
+			checkVersion(fdbCluster, targetVersion)
+
+			// TODO add validation here processes are updated new version
 		},
-		EntryDescription("Upgrade from %[1]s to %[2]s when no remote processes are restarted"),
+		EntryDescription("Upgrade from %[1]s to %[2]s"),
 		fixtures.GenerateUpgradeTableEntries(testOptions),
 	)
 })