Skip to content

Commit 0fc1671

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#126446 from Jefftree/fix-leaderelection-flake-testcontroller
Use fake clock for controller/leaderelection:TestController
2 parents 17d7d28 + 634c9cd commit 0fc1671

File tree

2 files changed

+37
-30
lines changed

2 files changed

+37
-30
lines changed

‎pkg/controlplane/controller/leaderelection/leaderelection_controller.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -278,7 +278,8 @@ func (c *Controller) reconcileElectionStep(ctx context.Context, leaseNN types.Na
278278

279279
if candidate.Spec.PingTime == nil ||
280280
// If PingTime is outdated, send another PingTime only if it already acked the first one.
281-
(candidate.Spec.PingTime.Add(electionDuration).Before(now) && candidate.Spec.PingTime.Before(candidate.Spec.RenewTime)) {
281+
// This checks for pingTime <= renewTime because equality is possible in unit tests using a fake clock.
282+
(candidate.Spec.PingTime.Add(electionDuration).Before(now) && !candidate.Spec.RenewTime.Before(candidate.Spec.PingTime)) {
282283
// TODO(jefftree): We should randomize the order of sending pings and do them in parallel
283284
// so that all candidates have equal opportunity to ack.
284285
clone := candidate.DeepCopy()
@@ -300,7 +301,7 @@ func (c *Controller) reconcileElectionStep(ctx context.Context, leaseNN types.Na
300301
continue // shouldn't be the case after the above
301302
}
302303

303-
if candidate.Spec.RenewTime != nil && candidate.Spec.PingTime.Before(candidate.Spec.RenewTime) {
304+
if candidate.Spec.RenewTime != nil && !candidate.Spec.RenewTime.Before(candidate.Spec.PingTime) {
304305
continue // this has renewed already
305306
}
306307

‎pkg/controlplane/controller/leaderelection/leaderelection_controller_test.go‎

Lines changed: 34 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"context"
2121
"fmt"
2222
"math/rand"
23+
"sync"
2324
"testing"
2425
"time"
2526

@@ -174,7 +175,7 @@ func TestReconcileElectionStep(t *testing.T) {
174175
LeaseName: "component-A",
175176
EmulationVersion: "1.20.0",
176177
BinaryVersion: "1.20.0",
177-
PingTime: ptr.To(metav1.NewMicroTime(fakeClock.Now().Add(-1 * time.Millisecond))),
178+
PingTime: ptr.To(metav1.NewMicroTime(fakeClock.Now())),
178179
RenewTime: ptr.To(metav1.NewMicroTime(fakeClock.Now())),
179180
PreferredStrategies: []v1.CoordinatedLeaseStrategy{v1.OldestEmulationVersion},
180181
},
@@ -439,8 +440,6 @@ func TestReconcileElectionStep(t *testing.T) {
439440
}
440441

441442
func TestController(t *testing.T) {
442-
fakeClock := testingclock.NewFakeClock(time.Now())
443-
444443
cases := []struct {
445444
name string
446445
leases []*v1.Lease
@@ -461,7 +460,7 @@ func TestController(t *testing.T) {
461460
LeaseName: "component-A",
462461
EmulationVersion: "1.19.0",
463462
BinaryVersion: "1.19.0",
464-
RenewTime: ptr.To(metav1.NewMicroTime(fakeClock.Now())),
463+
RenewTime: ptr.To(metav1.NewMicroTime(time.Now())),
465464
PreferredStrategies: []v1.CoordinatedLeaseStrategy{v1.OldestEmulationVersion},
466465
},
467466
},
@@ -490,7 +489,7 @@ func TestController(t *testing.T) {
490489
LeaseName: "component-A",
491490
EmulationVersion: "1.19.0",
492491
BinaryVersion: "1.19.0",
493-
RenewTime: ptr.To(metav1.NewMicroTime(fakeClock.Now())),
492+
RenewTime: ptr.To(metav1.NewMicroTime(time.Now())),
494493
PreferredStrategies: []v1.CoordinatedLeaseStrategy{v1.OldestEmulationVersion},
495494
},
496495
},
@@ -503,7 +502,7 @@ func TestController(t *testing.T) {
503502
LeaseName: "component-A",
504503
EmulationVersion: "1.19.0",
505504
BinaryVersion: "1.20.0",
506-
RenewTime: ptr.To(metav1.NewMicroTime(fakeClock.Now())),
505+
RenewTime: ptr.To(metav1.NewMicroTime(time.Now())),
507506
PreferredStrategies: []v1.CoordinatedLeaseStrategy{v1.OldestEmulationVersion},
508507
},
509508
},
@@ -516,7 +515,7 @@ func TestController(t *testing.T) {
516515
LeaseName: "component-A",
517516
EmulationVersion: "1.20.0",
518517
BinaryVersion: "1.20.0",
519-
RenewTime: ptr.To(metav1.NewMicroTime(fakeClock.Now())),
518+
RenewTime: ptr.To(metav1.NewMicroTime(time.Now())),
520519
PreferredStrategies: []v1.CoordinatedLeaseStrategy{v1.OldestEmulationVersion},
521520
},
522521
},
@@ -543,7 +542,7 @@ func TestController(t *testing.T) {
543542
},
544543
Spec: v1.LeaseSpec{
545544
HolderIdentity: ptr.To("some-other-component"),
546-
RenewTime: ptr.To(metav1.NewMicroTime(fakeClock.Now().Add(time.Second))),
545+
RenewTime: ptr.To(metav1.NewMicroTime(time.Now().Add(time.Second))),
547546
LeaseDurationSeconds: ptr.To(int32(10)),
548547
},
549548
},
@@ -558,7 +557,7 @@ func TestController(t *testing.T) {
558557
LeaseName: "component-A",
559558
EmulationVersion: "1.19.0",
560559
BinaryVersion: "1.19.0",
561-
RenewTime: ptr.To(metav1.NewMicroTime(fakeClock.Now())),
560+
RenewTime: ptr.To(metav1.NewMicroTime(time.Now())),
562561
PreferredStrategies: []v1.CoordinatedLeaseStrategy{v1.OldestEmulationVersion},
563562
},
564563
},
@@ -588,7 +587,7 @@ func TestController(t *testing.T) {
588587
},
589588
Spec: v1.LeaseSpec{
590589
HolderIdentity: ptr.To("some-other-component"),
591-
RenewTime: ptr.To(metav1.NewMicroTime(fakeClock.Now().Add(-11 * time.Second))),
590+
RenewTime: ptr.To(metav1.NewMicroTime(time.Now().Add(-11 * time.Second))),
592591
LeaseDurationSeconds: ptr.To(int32(10)),
593592
Strategy: ptr.To[v1.CoordinatedLeaseStrategy]("OldestEmulationVersion"),
594593
},
@@ -604,7 +603,7 @@ func TestController(t *testing.T) {
604603
LeaseName: "component-A",
605604
EmulationVersion: "1.19.0",
606605
BinaryVersion: "1.19.0",
607-
RenewTime: ptr.To(metav1.NewMicroTime(fakeClock.Now())),
606+
RenewTime: ptr.To(metav1.NewMicroTime(time.Now())),
608607
PreferredStrategies: []v1.CoordinatedLeaseStrategy{v1.OldestEmulationVersion},
609608
},
610609
},
@@ -632,7 +631,7 @@ func TestController(t *testing.T) {
632631
Spec: v1.LeaseSpec{
633632
HolderIdentity: ptr.To("component-identity-1"),
634633
Strategy: ptr.To[v1.CoordinatedLeaseStrategy]("OldestEmulationVersion"),
635-
RenewTime: ptr.To(metav1.NewMicroTime(fakeClock.Now().Add(time.Second))),
634+
RenewTime: ptr.To(metav1.NewMicroTime(time.Now().Add(time.Second))),
636635
LeaseDurationSeconds: ptr.To(int32(10)),
637636
},
638637
},
@@ -647,7 +646,7 @@ func TestController(t *testing.T) {
647646
LeaseName: "component-A",
648647
EmulationVersion: "1.20.0",
649648
BinaryVersion: "1.20.0",
650-
RenewTime: ptr.To(metav1.NewMicroTime(fakeClock.Now())),
649+
RenewTime: ptr.To(metav1.NewMicroTime(time.Now())),
651650
PreferredStrategies: []v1.CoordinatedLeaseStrategy{v1.OldestEmulationVersion},
652651
},
653652
},
@@ -662,7 +661,7 @@ func TestController(t *testing.T) {
662661
LeaseName: "component-A",
663662
EmulationVersion: "1.19.0",
664663
BinaryVersion: "1.19.0",
665-
RenewTime: ptr.To(metav1.NewMicroTime(fakeClock.Now())),
664+
RenewTime: ptr.To(metav1.NewMicroTime(time.Now())),
666665
PreferredStrategies: []v1.CoordinatedLeaseStrategy{v1.OldestEmulationVersion},
667666
},
668667
},
@@ -683,6 +682,10 @@ func TestController(t *testing.T) {
683682

684683
for _, tc := range cases {
685684
t.Run(tc.name, func(t *testing.T) {
685+
// collect go routines using t.logf
686+
var wg sync.WaitGroup
687+
defer wg.Wait()
688+
686689
ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
687690
defer cancel()
688691

@@ -722,7 +725,9 @@ func TestController(t *testing.T) {
722725
time.Sleep(time.Second)
723726
}
724727

728+
wg.Add(1)
725729
go func() {
730+
defer wg.Done()
726731
ticker := time.NewTicker(10 * time.Millisecond)
727732
// Mock out the removal of preferredHolder leases.
728733
// When controllers are running, they are expected to do this voluntarily
@@ -756,7 +761,9 @@ func TestController(t *testing.T) {
756761
}
757762
}()
758763

764+
wg.Add(1)
759765
go func() {
766+
defer wg.Done()
760767
ticker := time.NewTicker(10 * time.Millisecond)
761768
// Mock out leasecandidate ack.
762769
// When controllers are running, they are expected to watch and ack
@@ -765,16 +772,18 @@ func TestController(t *testing.T) {
765772
case <-ctx.Done():
766773
return
767774
case <-ticker.C:
768-
for _, lc := range tc.createAfterControllerStart {
769-
c, err := client.CoordinationV1alpha1().LeaseCandidates(lc.Namespace).Get(ctx, lc.Name, metav1.GetOptions{})
770-
if err == nil {
771-
if c.Spec.PingTime != nil {
772-
t.Logf("Answering ping for %s/%s", c.Namespace, c.Name)
773-
c.Spec.RenewTime = &metav1.MicroTime{Time: fakeClock.Now()}
774-
_, err = client.CoordinationV1alpha1().LeaseCandidates(lc.Namespace).Update(ctx, c, metav1.UpdateOptions{})
775-
if err != nil {
776-
t.Logf("Error updating lease candidate %s/%s: %v", c.Namespace, c.Name, err)
777-
}
775+
cs, err := client.CoordinationV1alpha1().LeaseCandidates("").List(ctx, metav1.ListOptions{})
776+
if err != nil {
777+
t.Logf("Error listing lease candidates: %v", err)
778+
continue
779+
}
780+
for _, c := range cs.Items {
781+
if c.Spec.PingTime != nil && (c.Spec.RenewTime == nil || c.Spec.PingTime.Time.After(c.Spec.RenewTime.Time)) {
782+
t.Logf("Answering ping for %s/%s", c.Namespace, c.Name)
783+
c.Spec.RenewTime = &metav1.MicroTime{Time: time.Now()}
784+
_, err = client.CoordinationV1alpha1().LeaseCandidates(c.Namespace).Update(ctx, &c, metav1.UpdateOptions{})
785+
if err != nil {
786+
t.Logf("Error updating lease candidate %s/%s: %v", c.Namespace, c.Name, err)
778787
}
779788
}
780789
}
@@ -815,10 +824,7 @@ func TestController(t *testing.T) {
815824
if lease.Spec.HolderIdentity == nil {
816825
return false, nil
817826
}
818-
if *expectedLease.Spec.HolderIdentity != *lease.Spec.HolderIdentity {
819-
return false, nil
820-
}
821-
return true, nil
827+
return *expectedLease.Spec.HolderIdentity == *lease.Spec.HolderIdentity, nil
822828
})
823829
if err != nil {
824830
if lease == nil {

0 commit comments

Comments
 (0)