Streamline lock client usage (#2214)

* Ensure that the lock client is used in a similar way and not getting a lock is handled like an error

* Return error when owner is denied

Author: johscheuer

controllers/bounce_processes.go

@@ -135,7 +135,7 @@ func (c bounceProcesses) reconcile(_ context.Context, r *FoundationDBClusterReco
 		}
 	}
 
-	_, err = r.takeLock(logger, cluster, fmt.Sprintf("bouncing processes: %v", addresses))
+	err = r.takeLock(logger, cluster, fmt.Sprintf("bouncing processes: %v", addresses))
 	if err != nil {
 		return &requeue{curError: err}
 	}

controllers/change_coordinators.go

@@ -75,7 +75,7 @@ func (c changeCoordinators) reconcile(ctx context.Context, r *FoundationDBCluste
 		return nil
 	}
 
-	_, err = r.takeLock(logger, cluster, "changing coordinators")
+	err = r.takeLock(logger, cluster, "changing coordinators")
 	if err != nil {
 		return &requeue{curError: err, delayedRequeue: true}
 	}

controllers/cluster_controller.go

@@ -467,22 +467,14 @@ func (r *FoundationDBClusterReconciler) getLockClient(logger logr.Logger, cluste
 }
 
 // takeLock attempts to acquire a lock.
-func (r *FoundationDBClusterReconciler) takeLock(logger logr.Logger, cluster *fdbv1beta2.FoundationDBCluster, action string) (bool, error) {
+func (r *FoundationDBClusterReconciler) takeLock(logger logr.Logger, cluster *fdbv1beta2.FoundationDBCluster, action string) error {
 	logger.Info("Taking lock on cluster", "action", action)
 	lockClient, err := r.getLockClient(logger, cluster)
 	if err != nil {
-		return false, err
-	}
-
-	hasLock, err := lockClient.TakeLock()
-	if err != nil {
-		return false, err
+		return err
 	}
 
-	if !hasLock {
-		r.Recorder.Event(cluster, corev1.EventTypeNormal, "LockAcquisitionFailed", fmt.Sprintf("Lock required before %s", action))
-	}
-	return hasLock, nil
+	return lockClient.TakeLock()
 }
 
 // releaseLock attempts to release a lock.

controllers/exclude_processes.go

@@ -73,12 +73,7 @@ func (e excludeProcesses) reconcile(ctx context.Context, r *FoundationDBClusterR
 
 	// Make sure the exclusions are coordinated across multiple operator instances.
 	if cluster.ShouldUseLocks() {
-		lockClient, err := r.getLockClient(logger, cluster)
-		if err != nil {
-			return &requeue{curError: err}
-		}
-
-		_, err = lockClient.TakeLock()
+		err = r.takeLock(logger, cluster, "exclude processes")
 		if err != nil {
 			return &requeue{curError: err, delayedRequeue: true}
 		}

controllers/maintenance_mode_checker.go

@@ -90,8 +90,8 @@ func (c maintenanceModeChecker) reconcile(_ context.Context, r *FoundationDBClus
 	}
 
 	// Make sure we take a lock before we continue.
-	hasLock, err := r.takeLock(logger, cluster, "maintenance mode check")
-	if !hasLock {
+	err = r.takeLock(logger, cluster, "maintenance mode check")
+	if err != nil {
 		return &requeue{curError: err}
 	}
 

controllers/remove_process_groups.go

@@ -278,12 +278,7 @@ func includeProcessGroup(ctx context.Context, logger logr.Logger, r *FoundationD
 
 	// Make sure the inclusion are coordinated across multiple operator instances.
 	if cluster.ShouldUseLocks() {
-		lockClient, err := r.getLockClient(logger, cluster)
-		if err != nil {
-			return err
-		}
-
-		_, err = lockClient.TakeLock()
+		err = r.takeLock(logger, cluster, "remove process groups")
 		if err != nil {
 			return err
 		}

controllers/update_database_configuration.go

@@ -95,9 +95,9 @@ func (u updateDatabaseConfiguration) reconcile(_ context.Context, r *FoundationD
 		}
 
 		if !initialConfig {
-			hasLock, err := r.takeLock(logger, cluster,
+			err := r.takeLock(logger, cluster,
 				fmt.Sprintf("reconfiguring the database to `%s`", configurationString))
-			if !hasLock {
+			if err != nil {
 				return &requeue{curError: err, delayedRequeue: true}
 			}
 		}

controllers/update_pods.go

@@ -370,7 +370,7 @@ func deletePodsForUpdates(ctx context.Context, r *FoundationDBClusterReconciler,
 	// Only lock the cluster if we are not running in the delete "All" mode.
 	// Otherwise, we want to delete all Pods and don't require a lock to sync with other clusters.
 	if deletionMode != fdbv1beta2.PodUpdateModeAll {
-		_, err := r.takeLock(logger, cluster, "updating pods")
+		err := r.takeLock(logger, cluster, "updating pods")
 		if err != nil {
 			return &requeue{curError: err}
 		}

fdbclient/lock_client.go

@@ -55,27 +55,24 @@ func (client *realLockClient) Disabled() bool {
 }
 
 // TakeLock attempts to acquire a lock.
-func (client *realLockClient) TakeLock() (bool, error) {
+func (client *realLockClient) TakeLock() error {
 	if client.disableLocks {
-		return true, nil
+		return nil
 	}
 
-	hasLock, err := client.database.Transact(func(transaction fdb.Transaction) (interface{}, error) {
-		return client.takeLockInTransaction(transaction)
+	_, err := client.database.Transact(func(transaction fdb.Transaction) (interface{}, error) {
+		lockErr := client.takeLockInTransaction(transaction)
+		return nil, lockErr
 	})
 
-	if hasLock == nil {
-		return false, err
-	}
-
-	return hasLock.(bool), err
+	return err
 }
 
 // takeLockInTransaction attempts to acquire a lock using an open transaction.
-func (client *realLockClient) takeLockInTransaction(transaction fdb.Transaction) (bool, error) {
+func (client *realLockClient) takeLockInTransaction(transaction fdb.Transaction) error {
 	err := transaction.Options().SetAccessSystemKeys()
 	if err != nil {
-		return false, err
+		return err
 	}
 
 	lockKey := fdb.Key(fmt.Sprintf("%s/global", client.cluster.GetLockPrefix()))
@@ -84,31 +81,31 @@ func (client *realLockClient) takeLockInTransaction(transaction fdb.Transaction)
 	if len(lockValue) == 0 {
 		client.log.Info("Setting initial lock")
 		client.updateLock(transaction, 0)
-		return true, nil
+		return nil
 	}
 
 	lockTuple, err := tuple.Unpack(lockValue)
 	if err != nil {
-		return false, err
+		return err
 	}
 
 	if len(lockTuple) < 3 {
-		return false, invalidLockValue{key: lockKey, value: lockValue}
+		return invalidLockValue{key: lockKey, value: lockValue}
 	}
 
 	currentLockOwnerID, valid := lockTuple[0].(string)
 	if !valid {
-		return false, invalidLockValue{key: lockKey, value: lockValue}
+		return invalidLockValue{key: lockKey, value: lockValue}
 	}
 
 	currentLockStartTime, valid := lockTuple[1].(int64)
 	if !valid {
-		return false, invalidLockValue{key: lockKey, value: lockValue}
+		return invalidLockValue{key: lockKey, value: lockValue}
 	}
 
 	currentLockEndTime, valid := lockTuple[2].(int64)
 	if !valid {
-		return false, invalidLockValue{key: lockKey, value: lockValue}
+		return invalidLockValue{key: lockKey, value: lockValue}
 	}
 
 	// ownerID represents the current cluster ID. If a lock is present the currentLockOwnerID represents the operator
@@ -123,7 +120,7 @@ func (client *realLockClient) takeLockInTransaction(transaction fdb.Transaction)
 	newOwnerDenied := transaction.Get(client.getDenyListKey(ownerID)).MustGet() != nil
 	if newOwnerDenied {
 		logger.Info("Failed to get lock due to deny list")
-		return false, nil
+		return fmt.Errorf("failed to get lock due to deny list, owner ID: %s is on deny list", ownerID)
 	}
 
 	oldOwnerDenied := transaction.Get(client.getDenyListKey(currentLockOwnerID)).MustGet() != nil
@@ -132,18 +129,18 @@ func (client *realLockClient) takeLockInTransaction(transaction fdb.Transaction)
 	if shouldClear {
 		logger.Info("Clearing expired lock")
 		client.updateLock(transaction, currentLockStartTime)
-		return true, nil
+		return nil
 	}
 
 	if currentLockOwnerID == ownerID {
 		logger.Info("Extending previous lock")
 		client.updateLock(transaction, currentLockStartTime)
-		return true, nil
+		return nil
 	}
 
 	logger.Info("Failed to get lock")
 
-	return false, nil
+	return fmt.Errorf("failed to get the lock")
 }
 
 // updateLock sets the keys to acquire a lock.

pkg/fdbadminclient/lock_client.go

@@ -30,7 +30,7 @@ type LockClient interface {
 	Disabled() bool
 
 	// TakeLock attempts to acquire a lock.
-	TakeLock() (bool, error)
+	TakeLock() error
 
 	// ReleaseLock will release the current lock. The method will only release the lock if the current
 	// operator is the lock holder.