diff --git a/pkg/controlplane/controller/leaderelection/leaderelection_controller.go b/pkg/controlplane/controller/leaderelection/leaderelection_controller.go index 15705a47ead..a4da8450657 100644 --- a/pkg/controlplane/controller/leaderelection/leaderelection_controller.go +++ b/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 - noRequeue = 0 - + defaultRequeueInterval = 5 * time.Second + 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. - if err == nil { - klog.Infof("Created lease %s for %q", leaseNN, electee.Name) - return defaultRequeueInterval, nil + + switch strategy { + case v1.OldestEmulationVersion: + electee := pickBestLeaderOldestEmulationVersion(ackedCandidates) + 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 - if !apierrors.IsAlreadyExists(err) { + // create the leader election lease + _, 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(existingLease) - if leaseClone.Spec.HolderIdentity == nil || *leaseClone.Spec.HolderIdentity == "" || (isExpired && *leaseClone.Spec.HolderIdentity != electeeName) { - klog.Infof("lease %s is expired, resetting it and setting holder to %q", leaseNN, electee.Name) - leaseClone.Spec.Strategy = &strategy - leaseClone.Spec.PreferredHolder = nil - leaseClone.Spec.HolderIdentity = &electeeName + isExpired := isLeaseExpired(existing) + noHolderIdentity := leaderLease.Spec.HolderIdentity != nil && existing.Spec.HolderIdentity == nil || *existing.Spec.HolderIdentity == "" + expiredAndNewHolder := isExpired && leaderLease.Spec.HolderIdentity != nil && *existing.Spec.HolderIdentity != *leaderLease.Spec.HolderIdentity + strategyChanged := existing.Spec.Strategy == nil || *existing.Spec.Strategy != strategy + differentHolder := leaderLease.Spec.HolderIdentity != nil && *leaderLease.Spec.HolderIdentity != *existing.Spec.HolderIdentity - leaseClone.Spec.RenewTime = &metav1.MicroTime{Time: time.Now()} - leaseClone.Spec.LeaseDurationSeconds = ptr.To(defaultLeaseDurationSeconds) - leaseClone.Spec.AcquireTime = nil - _, err = c.leaseClient.Leases(leaseNN.Namespace).Update(ctx, leaseClone, metav1.UpdateOptions{}) - 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 + // Update lease + if strategyChanged { + klog.Infof("Lease %s strategy changed to %q", leaseNN, strategy) + existing.Spec.Strategy = &strategy } + 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 } diff --git a/pkg/controlplane/controller/leaderelection/leaderelection_controller_test.go b/pkg/controlplane/controller/leaderelection/leaderelection_controller_test.go index 5e3686d4d74..58e2dcd4905 100644 --- a/pkg/controlplane/controller/leaderelection/leaderelection_controller_test.go +++ b/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 - if tc.expectedHolderIdentity != nil { - lease, err := client.CoordinationV1().Leases(tc.leaseNN.Namespace).Get(ctx, tc.leaseNN.Name, metav1.GetOptions{}) + lease, err := client.CoordinationV1().Leases(tc.leaseNN.Namespace).Get(ctx, tc.leaseNN.Name, metav1.GetOptions{}) + if tc.expectLease { 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 lease.Spec.PreferredHolder == nil || *lease.Spec.PreferredHolder != tc.expectedPreferredHolder { - t.Errorf("reconcileElectionStep() preferredHolder = %v, want %v", lease.Spec.PreferredHolder, tc.expectedPreferredHolder) - } + if tc.expectedPreferredHolder != nil && (lease.Spec.PreferredHolder == nil || *lease.Spec.PreferredHolder != *tc.expectedPreferredHolder) { + t.Errorf("reconcileElectionStep() preferredHolder = %s, want %s", strOrNil(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 "" + } + return fmt.Sprintf("%v", *s) +}