From d092513685996d21dfd538ab1869d95d35e2fad6 Mon Sep 17 00:00:00 2001 From: Jefftree Date: Mon, 29 Jul 2024 19:36:53 +0000 Subject: [PATCH 1/2] Use fake clock for controller/leaderelection:TestController --- .../leaderelection/leaderelection_controller.go | 5 +++-- .../leaderelection_controller_test.go | 15 ++++++++++----- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/pkg/controlplane/controller/leaderelection/leaderelection_controller.go b/pkg/controlplane/controller/leaderelection/leaderelection_controller.go index dde1d8287e6..cb53df92e81 100644 --- a/pkg/controlplane/controller/leaderelection/leaderelection_controller.go +++ b/pkg/controlplane/controller/leaderelection/leaderelection_controller.go @@ -278,7 +278,8 @@ func (c *Controller) reconcileElectionStep(ctx context.Context, leaseNN types.Na if candidate.Spec.PingTime == nil || // If PingTime is outdated, send another PingTime only if it already acked the first one. - (candidate.Spec.PingTime.Add(electionDuration).Before(now) && candidate.Spec.PingTime.Before(candidate.Spec.RenewTime)) { + // This checks for pingTime <= renewTime because equality is possible in unit tests using a fake clock. + (candidate.Spec.PingTime.Add(electionDuration).Before(now) && !candidate.Spec.RenewTime.Before(candidate.Spec.PingTime)) { // TODO(jefftree): We should randomize the order of sending pings and do them in parallel // so that all candidates have equal opportunity to ack. clone := candidate.DeepCopy() @@ -300,7 +301,7 @@ func (c *Controller) reconcileElectionStep(ctx context.Context, leaseNN types.Na continue // shouldn't be the case after the above } - if candidate.Spec.RenewTime != nil && candidate.Spec.PingTime.Before(candidate.Spec.RenewTime) { + if candidate.Spec.RenewTime != nil && !candidate.Spec.RenewTime.Before(candidate.Spec.PingTime) { continue // this has renewed already } diff --git a/pkg/controlplane/controller/leaderelection/leaderelection_controller_test.go b/pkg/controlplane/controller/leaderelection/leaderelection_controller_test.go index 093d40faea2..24b4161e6fe 100644 --- a/pkg/controlplane/controller/leaderelection/leaderelection_controller_test.go +++ b/pkg/controlplane/controller/leaderelection/leaderelection_controller_test.go @@ -31,6 +31,7 @@ import ( "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/tools/cache" testingclock "k8s.io/utils/clock/testing" "k8s.io/utils/ptr" ) @@ -697,6 +698,7 @@ func TestController(t *testing.T) { if err != nil { t.Fatal(err) } + controller.clock = fakeClock for _, obj := range tc.leases { t.Logf("Pre-creating lease %s/%s", obj.Namespace, obj.Name) @@ -715,6 +717,9 @@ func TestController(t *testing.T) { informerFactory.Start(ctx.Done()) informerFactory.WaitForCacheSync(ctx.Done()) + if !cache.WaitForNamedCacheSync(controllerName, ctx.Done(), controller.leaseRegistration.HasSynced, controller.leaseCandidateRegistration.HasSynced) { + return + } go controller.Run(ctx, 1) if rand.Intn(2) == 0 { @@ -747,9 +752,9 @@ func TestController(t *testing.T) { continue // only candidate-aware controllers will follow preferredHolder } - t.Logf("Deleting lease %s/%s because of preferredHolder %q != %q", l.Namespace, l.Name, *ph, *l.Spec.HolderIdentity) + fmt.Printf("Deleting lease %s/%s because of preferredHolder %q != %q", l.Namespace, l.Name, *ph, *l.Spec.HolderIdentity) if err = client.CoordinationV1().Leases(expectedLease.Namespace).Delete(ctx, expectedLease.Name, metav1.DeleteOptions{}); err != nil { - t.Logf("Error deleting lease %s/%s: %v", l.Namespace, l.Name, err) + fmt.Printf("Error deleting lease %s/%s: %v", l.Namespace, l.Name, err) } } } @@ -768,12 +773,12 @@ func TestController(t *testing.T) { for _, lc := range tc.createAfterControllerStart { c, err := client.CoordinationV1alpha1().LeaseCandidates(lc.Namespace).Get(ctx, lc.Name, metav1.GetOptions{}) if err == nil { - if c.Spec.PingTime != nil { - t.Logf("Answering ping for %s/%s", c.Namespace, c.Name) + if c.Spec.PingTime != nil && c.Spec.PingTime.Before(c.Spec.RenewTime) { + fmt.Printf("Answering ping for %s/%s", c.Namespace, c.Name) c.Spec.RenewTime = &metav1.MicroTime{Time: fakeClock.Now()} _, err = client.CoordinationV1alpha1().LeaseCandidates(lc.Namespace).Update(ctx, c, metav1.UpdateOptions{}) if err != nil { - t.Logf("Error updating lease candidate %s/%s: %v", c.Namespace, c.Name, err) + fmt.Printf("Error updating lease candidate %s/%s: %v", c.Namespace, c.Name, err) } } } From 634c9cd135acc4a05b1649f232943e77579821f0 Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Tue, 30 Jul 2024 09:41:21 +0200 Subject: [PATCH 2/2] Address comments Signed-off-by: Dr. Stefan Schimanski --- .../leaderelection_controller_test.go | 71 ++++++++++--------- 1 file changed, 36 insertions(+), 35 deletions(-) diff --git a/pkg/controlplane/controller/leaderelection/leaderelection_controller_test.go b/pkg/controlplane/controller/leaderelection/leaderelection_controller_test.go index 24b4161e6fe..eb04b1e70af 100644 --- a/pkg/controlplane/controller/leaderelection/leaderelection_controller_test.go +++ b/pkg/controlplane/controller/leaderelection/leaderelection_controller_test.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "math/rand" + "sync" "testing" "time" @@ -31,7 +32,6 @@ import ( "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes/fake" - "k8s.io/client-go/tools/cache" testingclock "k8s.io/utils/clock/testing" "k8s.io/utils/ptr" ) @@ -175,7 +175,7 @@ func TestReconcileElectionStep(t *testing.T) { LeaseName: "component-A", EmulationVersion: "1.20.0", BinaryVersion: "1.20.0", - PingTime: ptr.To(metav1.NewMicroTime(fakeClock.Now().Add(-1 * time.Millisecond))), + PingTime: ptr.To(metav1.NewMicroTime(fakeClock.Now())), RenewTime: ptr.To(metav1.NewMicroTime(fakeClock.Now())), PreferredStrategies: []v1.CoordinatedLeaseStrategy{v1.OldestEmulationVersion}, }, @@ -440,8 +440,6 @@ func TestReconcileElectionStep(t *testing.T) { } func TestController(t *testing.T) { - fakeClock := testingclock.NewFakeClock(time.Now()) - cases := []struct { name string leases []*v1.Lease @@ -462,7 +460,7 @@ func TestController(t *testing.T) { LeaseName: "component-A", EmulationVersion: "1.19.0", BinaryVersion: "1.19.0", - RenewTime: ptr.To(metav1.NewMicroTime(fakeClock.Now())), + RenewTime: ptr.To(metav1.NewMicroTime(time.Now())), PreferredStrategies: []v1.CoordinatedLeaseStrategy{v1.OldestEmulationVersion}, }, }, @@ -491,7 +489,7 @@ func TestController(t *testing.T) { LeaseName: "component-A", EmulationVersion: "1.19.0", BinaryVersion: "1.19.0", - RenewTime: ptr.To(metav1.NewMicroTime(fakeClock.Now())), + RenewTime: ptr.To(metav1.NewMicroTime(time.Now())), PreferredStrategies: []v1.CoordinatedLeaseStrategy{v1.OldestEmulationVersion}, }, }, @@ -504,7 +502,7 @@ func TestController(t *testing.T) { LeaseName: "component-A", EmulationVersion: "1.19.0", BinaryVersion: "1.20.0", - RenewTime: ptr.To(metav1.NewMicroTime(fakeClock.Now())), + RenewTime: ptr.To(metav1.NewMicroTime(time.Now())), PreferredStrategies: []v1.CoordinatedLeaseStrategy{v1.OldestEmulationVersion}, }, }, @@ -517,7 +515,7 @@ func TestController(t *testing.T) { LeaseName: "component-A", EmulationVersion: "1.20.0", BinaryVersion: "1.20.0", - RenewTime: ptr.To(metav1.NewMicroTime(fakeClock.Now())), + RenewTime: ptr.To(metav1.NewMicroTime(time.Now())), PreferredStrategies: []v1.CoordinatedLeaseStrategy{v1.OldestEmulationVersion}, }, }, @@ -544,7 +542,7 @@ func TestController(t *testing.T) { }, Spec: v1.LeaseSpec{ HolderIdentity: ptr.To("some-other-component"), - RenewTime: ptr.To(metav1.NewMicroTime(fakeClock.Now().Add(time.Second))), + RenewTime: ptr.To(metav1.NewMicroTime(time.Now().Add(time.Second))), LeaseDurationSeconds: ptr.To(int32(10)), }, }, @@ -559,7 +557,7 @@ func TestController(t *testing.T) { LeaseName: "component-A", EmulationVersion: "1.19.0", BinaryVersion: "1.19.0", - RenewTime: ptr.To(metav1.NewMicroTime(fakeClock.Now())), + RenewTime: ptr.To(metav1.NewMicroTime(time.Now())), PreferredStrategies: []v1.CoordinatedLeaseStrategy{v1.OldestEmulationVersion}, }, }, @@ -589,7 +587,7 @@ func TestController(t *testing.T) { }, Spec: v1.LeaseSpec{ HolderIdentity: ptr.To("some-other-component"), - RenewTime: ptr.To(metav1.NewMicroTime(fakeClock.Now().Add(-11 * time.Second))), + RenewTime: ptr.To(metav1.NewMicroTime(time.Now().Add(-11 * time.Second))), LeaseDurationSeconds: ptr.To(int32(10)), Strategy: ptr.To[v1.CoordinatedLeaseStrategy]("OldestEmulationVersion"), }, @@ -605,7 +603,7 @@ func TestController(t *testing.T) { LeaseName: "component-A", EmulationVersion: "1.19.0", BinaryVersion: "1.19.0", - RenewTime: ptr.To(metav1.NewMicroTime(fakeClock.Now())), + RenewTime: ptr.To(metav1.NewMicroTime(time.Now())), PreferredStrategies: []v1.CoordinatedLeaseStrategy{v1.OldestEmulationVersion}, }, }, @@ -633,7 +631,7 @@ func TestController(t *testing.T) { Spec: v1.LeaseSpec{ HolderIdentity: ptr.To("component-identity-1"), Strategy: ptr.To[v1.CoordinatedLeaseStrategy]("OldestEmulationVersion"), - RenewTime: ptr.To(metav1.NewMicroTime(fakeClock.Now().Add(time.Second))), + RenewTime: ptr.To(metav1.NewMicroTime(time.Now().Add(time.Second))), LeaseDurationSeconds: ptr.To(int32(10)), }, }, @@ -648,7 +646,7 @@ func TestController(t *testing.T) { LeaseName: "component-A", EmulationVersion: "1.20.0", BinaryVersion: "1.20.0", - RenewTime: ptr.To(metav1.NewMicroTime(fakeClock.Now())), + RenewTime: ptr.To(metav1.NewMicroTime(time.Now())), PreferredStrategies: []v1.CoordinatedLeaseStrategy{v1.OldestEmulationVersion}, }, }, @@ -663,7 +661,7 @@ func TestController(t *testing.T) { LeaseName: "component-A", EmulationVersion: "1.19.0", BinaryVersion: "1.19.0", - RenewTime: ptr.To(metav1.NewMicroTime(fakeClock.Now())), + RenewTime: ptr.To(metav1.NewMicroTime(time.Now())), PreferredStrategies: []v1.CoordinatedLeaseStrategy{v1.OldestEmulationVersion}, }, }, @@ -684,6 +682,10 @@ func TestController(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { + // collect go routines using t.logf + var wg sync.WaitGroup + defer wg.Wait() + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) defer cancel() @@ -698,7 +700,6 @@ func TestController(t *testing.T) { if err != nil { t.Fatal(err) } - controller.clock = fakeClock for _, obj := range tc.leases { t.Logf("Pre-creating lease %s/%s", obj.Namespace, obj.Name) @@ -717,9 +718,6 @@ func TestController(t *testing.T) { informerFactory.Start(ctx.Done()) informerFactory.WaitForCacheSync(ctx.Done()) - if !cache.WaitForNamedCacheSync(controllerName, ctx.Done(), controller.leaseRegistration.HasSynced, controller.leaseCandidateRegistration.HasSynced) { - return - } go controller.Run(ctx, 1) if rand.Intn(2) == 0 { @@ -727,7 +725,9 @@ func TestController(t *testing.T) { time.Sleep(time.Second) } + wg.Add(1) go func() { + defer wg.Done() ticker := time.NewTicker(10 * time.Millisecond) // Mock out the removal of preferredHolder leases. // When controllers are running, they are expected to do this voluntarily @@ -752,16 +752,18 @@ func TestController(t *testing.T) { continue // only candidate-aware controllers will follow preferredHolder } - fmt.Printf("Deleting lease %s/%s because of preferredHolder %q != %q", l.Namespace, l.Name, *ph, *l.Spec.HolderIdentity) + t.Logf("Deleting lease %s/%s because of preferredHolder %q != %q", l.Namespace, l.Name, *ph, *l.Spec.HolderIdentity) if err = client.CoordinationV1().Leases(expectedLease.Namespace).Delete(ctx, expectedLease.Name, metav1.DeleteOptions{}); err != nil { - fmt.Printf("Error deleting lease %s/%s: %v", l.Namespace, l.Name, err) + t.Logf("Error deleting lease %s/%s: %v", l.Namespace, l.Name, err) } } } } }() + wg.Add(1) go func() { + defer wg.Done() ticker := time.NewTicker(10 * time.Millisecond) // Mock out leasecandidate ack. // When controllers are running, they are expected to watch and ack @@ -770,16 +772,18 @@ func TestController(t *testing.T) { case <-ctx.Done(): return case <-ticker.C: - for _, lc := range tc.createAfterControllerStart { - c, err := client.CoordinationV1alpha1().LeaseCandidates(lc.Namespace).Get(ctx, lc.Name, metav1.GetOptions{}) - if err == nil { - if c.Spec.PingTime != nil && c.Spec.PingTime.Before(c.Spec.RenewTime) { - fmt.Printf("Answering ping for %s/%s", c.Namespace, c.Name) - c.Spec.RenewTime = &metav1.MicroTime{Time: fakeClock.Now()} - _, err = client.CoordinationV1alpha1().LeaseCandidates(lc.Namespace).Update(ctx, c, metav1.UpdateOptions{}) - if err != nil { - fmt.Printf("Error updating lease candidate %s/%s: %v", c.Namespace, c.Name, err) - } + cs, err := client.CoordinationV1alpha1().LeaseCandidates("").List(ctx, metav1.ListOptions{}) + if err != nil { + t.Logf("Error listing lease candidates: %v", err) + continue + } + for _, c := range cs.Items { + if c.Spec.PingTime != nil && (c.Spec.RenewTime == nil || c.Spec.PingTime.Time.After(c.Spec.RenewTime.Time)) { + t.Logf("Answering ping for %s/%s", c.Namespace, c.Name) + c.Spec.RenewTime = &metav1.MicroTime{Time: time.Now()} + _, err = client.CoordinationV1alpha1().LeaseCandidates(c.Namespace).Update(ctx, &c, metav1.UpdateOptions{}) + if err != nil { + t.Logf("Error updating lease candidate %s/%s: %v", c.Namespace, c.Name, err) } } } @@ -820,10 +824,7 @@ func TestController(t *testing.T) { if lease.Spec.HolderIdentity == nil { return false, nil } - if *expectedLease.Spec.HolderIdentity != *lease.Spec.HolderIdentity { - return false, nil - } - return true, nil + return *expectedLease.Spec.HolderIdentity == *lease.Spec.HolderIdentity, nil }) if err != nil { if lease == nil {