Review feedback: handle non-kube strategy correctly

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@gmail.com>

pkg/controlplane/controller/leaderelection/leaderelection_controller.go

@@ -19,6 +19,7 @@ package leaderelection
 import (
 	"context"
 	"fmt"
+	"reflect"
 	"time"
 
 	v1 "k8s.io/api/coordination/v1"
@@ -44,9 +45,9 @@ const (
 
 	// Requeue interval is the interval at which a Lease is requeued to verify that it is
 	// being renewed properly.
-	defaultRequeueInterval            = 5 * time.Second
+	defaultRequeueInterval = 5 * time.Second
-	noRequeue                         = 0
+	noRequeue              = 0
-	
+
 	defaultLeaseDurationSeconds int32 = 5
 
 	electionDuration = 5 * time.Second
@@ -322,73 +323,89 @@ func (c *Controller) reconcileElectionStep(ctx context.Context, leaseNN types.Na
 		return noRequeue, err
 	}
 
-	if strategy != v1.OldestEmulationVersion {
-		klog.V(2).Infof("strategy %s is not recognized by CLE.", strategy)
-		return noRequeue, nil
-	}
-	electee := pickBestLeaderOldestEmulationVersion(ackedCandidates)
-
-	if electee == nil {
-		return noRequeue, fmt.Errorf("should not happen, could not find suitable electee")
-	}
-
-	electeeName := electee.Name
-	// create the leader election lease
 	leaderLease := &v1.Lease{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: leaseNN.Namespace,
 			Name:      leaseNN.Name,
 		},
 		Spec: v1.LeaseSpec{
-			HolderIdentity:       &electeeName,
 			Strategy:             &strategy,
 			LeaseDurationSeconds: ptr.To(defaultLeaseDurationSeconds),
 			RenewTime:            &metav1.MicroTime{Time: time.Now()},
 		},
 	}
-	_, err = c.leaseClient.Leases(leaseNN.Namespace).Create(ctx, leaderLease, metav1.CreateOptions{})
+
-	// If the create was successful, then we can return here.
+	switch strategy {
-	if err == nil {
+	case v1.OldestEmulationVersion:
-		klog.Infof("Created lease %s for %q", leaseNN, electee.Name)
+		electee := pickBestLeaderOldestEmulationVersion(ackedCandidates)
-		return defaultRequeueInterval, nil
+		if electee == nil {
+			return noRequeue, fmt.Errorf("should not happen, could not find suitable electee")
+		}
+		leaderLease.Spec.HolderIdentity = &electee.Name
+	default:
+		// do not set the holder identity, but leave it to some other controller. But fall
+		// through to create the lease (without holder).
+		klog.V(2).Infof("Election for strategy %q is not handled by %s", strategy, controllerName)
 	}
 
-	// If there was an error, return
+	// create the leader election lease
-	if !apierrors.IsAlreadyExists(err) {
+	_, err = c.leaseClient.Leases(leaseNN.Namespace).Create(ctx, leaderLease, metav1.CreateOptions{})
+	if err == nil {
+		if leaderLease.Spec.HolderIdentity != nil {
+			klog.Infof("Created lease %s for %q", leaseNN, *leaderLease.Spec.HolderIdentity)
+		} else {
+			klog.Infof("Created lease %s without leader", leaseNN)
+		}
+		return defaultRequeueInterval, nil
+	} else if !apierrors.IsAlreadyExists(err) {
 		return noRequeue, err
 	}
 
-	existingLease, err := c.leaseClient.Leases(leaseNN.Namespace).Get(ctx, leaseNN.Name, metav1.GetOptions{})
+	// Get existing lease
+	existing, err := c.leaseClient.Leases(leaseNN.Namespace).Get(ctx, leaseNN.Name, metav1.GetOptions{})
 	if err != nil {
 		return noRequeue, err
 	}
-	leaseClone := existingLease.DeepCopy()
+	orig := existing.DeepCopy()
 
-	// Update the Lease if it either does not have a holder or is expired
+	isExpired := isLeaseExpired(existing)
-	isExpired := isLeaseExpired(existingLease)
+	noHolderIdentity := leaderLease.Spec.HolderIdentity != nil && existing.Spec.HolderIdentity == nil || *existing.Spec.HolderIdentity == ""
-	if leaseClone.Spec.HolderIdentity == nil || *leaseClone.Spec.HolderIdentity == "" || (isExpired && *leaseClone.Spec.HolderIdentity != electeeName) {
+	expiredAndNewHolder := isExpired && leaderLease.Spec.HolderIdentity != nil && *existing.Spec.HolderIdentity != *leaderLease.Spec.HolderIdentity
-		klog.Infof("lease %s is expired, resetting it and setting holder to %q", leaseNN, electee.Name)
+	strategyChanged := existing.Spec.Strategy == nil || *existing.Spec.Strategy != strategy
-		leaseClone.Spec.Strategy = &strategy
+	differentHolder := leaderLease.Spec.HolderIdentity != nil && *leaderLease.Spec.HolderIdentity != *existing.Spec.HolderIdentity
-		leaseClone.Spec.PreferredHolder = nil
-		leaseClone.Spec.HolderIdentity = &electeeName
 
-		leaseClone.Spec.RenewTime = &metav1.MicroTime{Time: time.Now()}
+	// Update lease
-		leaseClone.Spec.LeaseDurationSeconds = ptr.To(defaultLeaseDurationSeconds)
+	if strategyChanged {
-		leaseClone.Spec.AcquireTime = nil
+		klog.Infof("Lease %s strategy changed to %q", leaseNN, strategy)
-		_, err = c.leaseClient.Leases(leaseNN.Namespace).Update(ctx, leaseClone, metav1.UpdateOptions{})
+		existing.Spec.Strategy = &strategy
-		if err != nil {
-			return time.Until(leaseClone.Spec.RenewTime.Time), err
-		}
-	} else if leaseClone.Spec.HolderIdentity != nil && *leaseClone.Spec.HolderIdentity != electeeName {
-		klog.Infof("lease %s already exists for holder %q but should be held by %q, marking preferredHolder", leaseNN, *leaseClone.Spec.HolderIdentity, electee.Name)
-		leaseClone.Spec.PreferredHolder = &electeeName
-		leaseClone.Spec.Strategy = &strategy
-		_, err = c.leaseClient.Leases(leaseNN.Namespace).Update(ctx, leaseClone, metav1.UpdateOptions{})
-		if err != nil {
-			return noRequeue, err
-		}
-		return time.Until(leaseClone.Spec.RenewTime.Time), nil
 	}
+	if noHolderIdentity || expiredAndNewHolder {
+		if noHolderIdentity {
+			klog.Infof("Lease %s had no holder, setting holder to %q", leaseNN, *leaderLease.Spec.HolderIdentity)
+		} else {
+			klog.Infof("Lease %s expired, resetting it and setting holder to %q", leaseNN, *leaderLease.Spec.HolderIdentity)
+		}
+
+		existing.Spec.PreferredHolder = nil
+		existing.Spec.HolderIdentity = leaderLease.Spec.HolderIdentity
+		existing.Spec.RenewTime = &metav1.MicroTime{Time: time.Now()}
+		existing.Spec.LeaseDurationSeconds = ptr.To(defaultLeaseDurationSeconds)
+		existing.Spec.AcquireTime = nil
+	} else if differentHolder {
+		klog.Infof("Lease %s holder changed from %q to %q", leaseNN, *existing.Spec.HolderIdentity, *leaderLease.Spec.HolderIdentity)
+		existing.Spec.PreferredHolder = leaderLease.Spec.HolderIdentity
+	}
+
+	if reflect.DeepEqual(existing, orig) {
+		klog.V(5).Infof("Lease %s already has the most optimal leader %q", leaseNN, *existing.Spec.HolderIdentity)
+		return noRequeue, nil
+	}
+
+	_, err = c.leaseClient.Leases(leaseNN.Namespace).Update(ctx, existing, metav1.UpdateOptions{})
+	if err != nil {
+		return noRequeue, err
+	}
+
 	return defaultRequeueInterval, nil
 }
 

pkg/controlplane/controller/leaderelection/leaderelection_controller_test.go

@@ -18,6 +18,7 @@ package leaderelection
 
 import (
 	"context"
+	"fmt"
 	"testing"
 	"time"
 
@@ -41,10 +42,12 @@ func TestReconcileElectionStep(t *testing.T) {
 		leaseNN                 types.NamespacedName
 		candidates              []*v1alpha1.LeaseCandidate
 		existingLease           *v1.Lease
+		expectLease             bool
 		expectedHolderIdentity  *string
-		expectedPreferredHolder string
+		expectedPreferredHolder *string
 		expectedRequeue         bool
 		expectedError           bool
+		expectedStrategy        *v1.CoordinatedLeaseStrategy
 		candidatesPinged        bool
 	}{
 		{
@@ -52,7 +55,9 @@ func TestReconcileElectionStep(t *testing.T) {
 			leaseNN:                types.NamespacedName{Namespace: "default", Name: "component-A"},
 			candidates:             []*v1alpha1.LeaseCandidate{},
 			existingLease:          nil,
+			expectLease:            false,
 			expectedHolderIdentity: nil,
+			expectedStrategy:       nil,
 			expectedRequeue:        false,
 			expectedError:          false,
 		},
@@ -61,7 +66,9 @@ func TestReconcileElectionStep(t *testing.T) {
 			leaseNN:                types.NamespacedName{Namespace: "default", Name: "component-A"},
 			candidates:             []*v1alpha1.LeaseCandidate{},
 			existingLease:          &v1.Lease{},
+			expectLease:            false,
 			expectedHolderIdentity: nil,
+			expectedStrategy:       nil,
 			expectedRequeue:        false,
 			expectedError:          false,
 		},
@@ -84,7 +91,9 @@ func TestReconcileElectionStep(t *testing.T) {
 				},
 			},
 			existingLease:          nil,
+			expectLease:            true,
 			expectedHolderIdentity: ptr.To("component-identity-1"),
+			expectedStrategy:       ptr.To[v1.CoordinatedLeaseStrategy]("OldestEmulationVersion"),
 			expectedRequeue:        true,
 			expectedError:          false,
 		},
@@ -130,8 +139,10 @@ func TestReconcileElectionStep(t *testing.T) {
 					RenewTime:            ptr.To(metav1.NewMicroTime(time.Now())),
 				},
 			},
+			expectLease:             true,
 			expectedHolderIdentity:  ptr.To("component-identity-1"),
-			expectedPreferredHolder: "component-identity-2",
+			expectedPreferredHolder: ptr.To("component-identity-2"),
+			expectedStrategy:        ptr.To[v1.CoordinatedLeaseStrategy]("OldestEmulationVersion"),
 			expectedRequeue:         true,
 			expectedError:           false,
 		},
@@ -168,7 +179,9 @@ func TestReconcileElectionStep(t *testing.T) {
 				},
 			},
 			existingLease:          nil,
+			expectLease:            true,
 			expectedHolderIdentity: ptr.To("component-identity-2"),
+			expectedStrategy:       ptr.To[v1.CoordinatedLeaseStrategy]("OldestEmulationVersion"),
 			expectedRequeue:        true,
 			expectedError:          false,
 		},
@@ -201,7 +214,47 @@ func TestReconcileElectionStep(t *testing.T) {
 					RenewTime:            ptr.To(metav1.NewMicroTime(time.Now().Add(-1 * time.Minute))),
 				},
 			},
+			expectLease:            true,
 			expectedHolderIdentity: ptr.To("component-identity-1"),
+			expectedStrategy:       ptr.To[v1.CoordinatedLeaseStrategy]("OldestEmulationVersion"),
+			expectedRequeue:        true,
+			expectedError:          false,
+		},
+		{
+			name:    "candidates exist, lease exists, lease expired, 3rdparty strategy",
+			leaseNN: types.NamespacedName{Namespace: "default", Name: "component-A"},
+			candidates: []*v1alpha1.LeaseCandidate{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: "default",
+						Name:      "component-identity-1",
+					},
+					Spec: v1alpha1.LeaseCandidateSpec{
+						LeaseName:           "component-A",
+						EmulationVersion:    "1.19.0",
+						BinaryVersion:       "1.19.0",
+						RenewTime:           ptr.To(metav1.NewMicroTime(time.Now())),
+						PreferredStrategies: []v1.CoordinatedLeaseStrategy{"foo.com/bar"},
+					},
+				},
+			},
+			existingLease: &v1.Lease{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "default",
+					Name:      "component-A",
+					Annotations: map[string]string{
+						electedByAnnotationName: controllerName,
+					},
+				},
+				Spec: v1.LeaseSpec{
+					HolderIdentity:       ptr.To("component-identity-expired"),
+					LeaseDurationSeconds: ptr.To(int32(10)),
+					RenewTime:            ptr.To(metav1.NewMicroTime(time.Now().Add(-1 * time.Minute))),
+				},
+			},
+			expectLease:            true,
+			expectedHolderIdentity: ptr.To("component-identity-expired"),
+			expectedStrategy:       ptr.To[v1.CoordinatedLeaseStrategy]("foo.com/bar"),
 			expectedRequeue:        true,
 			expectedError:          false,
 		},
@@ -225,6 +278,7 @@ func TestReconcileElectionStep(t *testing.T) {
 				},
 			},
 			existingLease:          nil,
+			expectLease:            false,
 			expectedHolderIdentity: nil,
 			expectedRequeue:        false,
 			expectedError:          true,
@@ -248,7 +302,9 @@ func TestReconcileElectionStep(t *testing.T) {
 				},
 			},
 			existingLease:          nil,
+			expectLease:            false,
 			expectedHolderIdentity: nil,
+			expectedStrategy:       nil,
 			expectedRequeue:        true,
 			expectedError:          false,
 			candidatesPinged:       true,
@@ -273,7 +329,9 @@ func TestReconcileElectionStep(t *testing.T) {
 				},
 			},
 			existingLease:          nil,
+			expectLease:            false,
 			expectedHolderIdentity: nil,
+			expectedStrategy:       nil,
 			expectedRequeue:        false,
 			expectedError:          false,
 		},
@@ -323,20 +381,32 @@ func TestReconcileElectionStep(t *testing.T) {
 				t.Errorf("reconcileElectionStep() error = %v, want nil", err)
 			}
 
-			// Check the lease holder identity
+			lease, err := client.CoordinationV1().Leases(tc.leaseNN.Namespace).Get(ctx, tc.leaseNN.Name, metav1.GetOptions{})
-			if tc.expectedHolderIdentity != nil {
+			if tc.expectLease {
-				lease, err := client.CoordinationV1().Leases(tc.leaseNN.Namespace).Get(ctx, tc.leaseNN.Name, metav1.GetOptions{})
 				if err != nil {
 					t.Fatal(err)
 				}
-				if lease.Spec.HolderIdentity == nil || *lease.Spec.HolderIdentity != *tc.expectedHolderIdentity {
+
-					t.Errorf("reconcileElectionStep() holderIdentity = %v, want %v", *lease.Spec.HolderIdentity, *tc.expectedHolderIdentity)
+				// Check the lease holder identity
+				if tc.expectedHolderIdentity != nil && (lease.Spec.HolderIdentity == nil || *lease.Spec.HolderIdentity != *tc.expectedHolderIdentity) {
+					t.Errorf("reconcileElectionStep() holderIdentity = %s, want %s", strOrNil(lease.Spec.HolderIdentity), *tc.expectedHolderIdentity)
+				} else if tc.expectedHolderIdentity == nil && lease.Spec.HolderIdentity != nil && *lease.Spec.HolderIdentity != "" {
+					t.Errorf("reconcileElectionStep() holderIdentity = %s, want nil", *lease.Spec.HolderIdentity)
 				}
-				if tc.expectedPreferredHolder != "" {
+				if tc.expectedPreferredHolder != nil && (lease.Spec.PreferredHolder == nil || *lease.Spec.PreferredHolder != *tc.expectedPreferredHolder) {
-					if lease.Spec.PreferredHolder == nil || *lease.Spec.PreferredHolder != tc.expectedPreferredHolder {
+					t.Errorf("reconcileElectionStep() preferredHolder = %s, want %s", strOrNil(lease.Spec.PreferredHolder), *tc.expectedPreferredHolder)
-						t.Errorf("reconcileElectionStep() preferredHolder = %v, want %v", lease.Spec.PreferredHolder, tc.expectedPreferredHolder)
+				} else if tc.expectedPreferredHolder == nil && lease.Spec.PreferredHolder != nil && *lease.Spec.PreferredHolder != "" {
-					}
+					t.Errorf("reconcileElectionStep() preferredHolder = %s, want nil", *lease.Spec.PreferredHolder)
 				}
+
+				// Check chosen strategy in the Lease
+				if tc.expectedStrategy != nil && (lease.Spec.Strategy == nil || *lease.Spec.Strategy != *tc.expectedStrategy) {
+					t.Errorf("reconcileElectionStep() strategy = %s, want %s", strOrNil(lease.Spec.Strategy), *tc.expectedStrategy)
+				} else if tc.expectedStrategy == nil && lease.Spec.Strategy != nil && *lease.Spec.Strategy != "" {
+					t.Errorf("reconcileElectionStep() strategy = %s, want nil", *lease.Spec.Strategy)
+				}
+			} else if err == nil {
+				t.Errorf("reconcileElectionStep() expected no lease to be created")
 			}
 
 			// Verify that ping to candidate was issued
@@ -672,3 +742,10 @@ func TestController(t *testing.T) {
 		})
 	}
 }
+
+func strOrNil[T any](s *T) string {
+	if s == nil {
+		return "<nil>"
+	}
+	return fmt.Sprintf("%v", *s)
+}