From 30337364ceb0e6656e6e51bfd434d646e124192e Mon Sep 17 00:00:00 2001 From: Matt Moore Date: Mon, 10 Dec 2018 04:58:22 +0000 Subject: [PATCH] Refactor global resync tests to use our "hooks" package. I spent a little time looking at #2443 and it seemed like a good use of the hook stuff that @grantr put together in the early days of knative/serving. Running this with high iteration counts locally, I still see occasional errors, so I'm not going to close out the linked issue, but I feel like this is a step in the right direction as this library has been fairly well worn at this point. Related: https://github.com/knative/serving/issues/2443 --- .../clusteringress/clusteringress_test.go | 57 +++++----- pkg/reconciler/v1alpha1/route/route_test.go | 100 ++++++++---------- pkg/reconciler/v1alpha1/testing/aliases.go | 3 +- 3 files changed, 70 insertions(+), 90 deletions(-) diff --git a/pkg/reconciler/v1alpha1/clusteringress/clusteringress_test.go b/pkg/reconciler/v1alpha1/clusteringress/clusteringress_test.go index a2d7a95be788..178189541ea5 100644 --- a/pkg/reconciler/v1alpha1/clusteringress/clusteringress_test.go +++ b/pkg/reconciler/v1alpha1/clusteringress/clusteringress_test.go @@ -25,7 +25,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" - "k8s.io/apimachinery/pkg/watch" kubeinformers "k8s.io/client-go/informers" fakekubeclientset "k8s.io/client-go/kubernetes/fake" clientgotesting "k8s.io/client-go/testing" @@ -330,9 +329,28 @@ func TestGlobalResyncOnUpdateGatewayConfigMap(t *testing.T) { _, _, servingClient, controller, _, _, sharedInformer, servingInformer, watcher := newTestSetup(t) stopCh := make(chan struct{}) - defer func() { - close(stopCh) - }() + defer close(stopCh) + + h := NewHooks() + + // Check for ClusterIngress created as a signal that syncHandler ran + h.OnUpdate(&servingClient.Fake, "clusteringresses", func(obj runtime.Object) HookResult { + ci := obj.(*v1alpha1.ClusterIngress) + t.Logf("clusteringress updated: %q", ci.Name) + + gateways := ci.Status.LoadBalancer.Ingress + if len(gateways) != 1 { + t.Logf("Unexpected gateways: %v", gateways) + return HookIncomplete + } + expectedDomainInternal := newDomainInternal + if gateways[0].DomainInternal != expectedDomainInternal { + t.Logf("Expected gateway %q but got %q", expectedDomainInternal, gateways[0].DomainInternal) + return HookIncomplete + } + + return HookComplete + }) servingInformer.Start(stopCh) sharedInformer.Start(stopCh) @@ -362,17 +380,11 @@ func TestGlobalResyncOnUpdateGatewayConfigMap(t *testing.T) { }, ) ingressClient := servingClient.NetworkingV1alpha1().ClusterIngresses() - ingressWatcher, err := ingressClient.Watch(metav1.ListOptions{}) - if err != nil { - t.Fatalf("Could not create ingress watcher") - } - defer ingressWatcher.Stop() // Create a ingress. ingressClient.Create(ingress) // Test changes in gateway config map. ClusterIngress should get updated appropriately. - expectedDomainInternal := newDomainInternal domainConfig := corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: config.IstioConfigName, @@ -381,29 +393,8 @@ func TestGlobalResyncOnUpdateGatewayConfigMap(t *testing.T) { Data: newGateways, } watcher.OnChange(&domainConfig) - timer := time.NewTimer(10 * time.Second) - -loop: - for { - select { - case event := <-ingressWatcher.ResultChan(): - if event.Type == watch.Modified { - break loop - } - case <-timer.C: - t.Fatalf("ingressWatcher did not receive a Type==Modified event in 10s") - } - } - ingress, err = ingressClient.Get(ingress.Name, metav1.GetOptions{}) - if err != nil { - t.Fatalf("Error getting a ingress: %v", err) - } - gateways := ingress.Status.LoadBalancer.Ingress - if len(gateways) != 1 { - t.Errorf("Unexpected gateways: %v", gateways) - } - if gateways[0].DomainInternal != expectedDomainInternal { - t.Errorf("Expected gateway %q but got %q", expectedDomainInternal, gateways[0].DomainInternal) + if err := h.WaitForHooks(3 * time.Second); err != nil { + t.Error(err) } } diff --git a/pkg/reconciler/v1alpha1/route/route_test.go b/pkg/reconciler/v1alpha1/route/route_test.go index 86c7b6437457..098c70700af4 100644 --- a/pkg/reconciler/v1alpha1/route/route_test.go +++ b/pkg/reconciler/v1alpha1/route/route_test.go @@ -43,8 +43,8 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" - "k8s.io/apimachinery/pkg/watch" kubeinformers "k8s.io/client-go/informers" fakekubeclientset "k8s.io/client-go/kubernetes/fake" ) @@ -868,55 +868,17 @@ func TestUpdateDomainConfigMap(t *testing.T) { } func TestGlobalResyncOnUpdateDomainConfigMap(t *testing.T) { - _, servingClient, controller, _, kubeInformer, servingInformer, watcher := newTestSetup(t) - - stopCh := make(chan struct{}) - defer func() { - close(stopCh) - }() - - servingInformer.Start(stopCh) - kubeInformer.Start(stopCh) - if err := watcher.Start(stopCh); err != nil { - t.Fatalf("failed to start configuration manager: %v", err) - } - - go controller.Run(1, stopCh) - - route := getTestRouteWithTrafficTargets([]v1alpha1.TrafficTarget{}) - routeClient := servingClient.ServingV1alpha1().Routes(route.Namespace) - routeWatcher, err := routeClient.Watch(metav1.ListOptions{}) - if err != nil { - t.Fatalf("Could not create route watcher") - } - defer routeWatcher.Stop() - - routeModifiedCh := make(chan struct{}) - defer close(routeModifiedCh) - - go func() { - for event := range routeWatcher.ResultChan() { - if event.Type == watch.Modified { - routeModifiedCh <- struct{}{} - } - } - }() - - // Create a route. - route.Labels = map[string]string{"app": "prod"} - routeClient.Create(route) - // Test changes in domain config map. Routes should get updated appropriately. // We're expecting exactly one route modification per config-map change. tests := []struct { - doThings func() + doThings func(*configmap.ManualWatcher) expectedDomainSuffix string }{{ expectedDomainSuffix: prodDomainSuffix, - doThings: func() {}, // The update will still happen: status will be updated to match the route labels + doThings: func(*configmap.ManualWatcher) {}, // The update will still happen: status will be updated to match the route labels }, { expectedDomainSuffix: "mytestdomain.com", - doThings: func() { + doThings: func(watcher *configmap.ManualWatcher) { domainConfig := corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: config.DomainConfigName, @@ -931,7 +893,7 @@ func TestGlobalResyncOnUpdateDomainConfigMap(t *testing.T) { }, }, { expectedDomainSuffix: "newprod.net", - doThings: func() { + doThings: func(watcher *configmap.ManualWatcher) { domainConfig := corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: config.DomainConfigName, @@ -946,7 +908,7 @@ func TestGlobalResyncOnUpdateDomainConfigMap(t *testing.T) { }, }, { expectedDomainSuffix: defaultDomainSuffix, - doThings: func() { + doThings: func(watcher *configmap.ManualWatcher) { domainConfig := corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: config.DomainConfigName, @@ -959,23 +921,49 @@ func TestGlobalResyncOnUpdateDomainConfigMap(t *testing.T) { watcher.OnChange(&domainConfig) }, }} + for _, test := range tests { t.Run(test.expectedDomainSuffix, func(t *testing.T) { - test.doThings() + _, servingClient, controller, _, kubeInformer, servingInformer, watcher := newTestSetup(t) - select { - case <-routeModifiedCh: - case <-time.NewTimer(10 * time.Second).C: - t.Logf("routeWatcher did not receive a Type==Modified event for %s in 10s", test.expectedDomainSuffix) - } + stopCh := make(chan struct{}) + defer close(stopCh) + + h := NewHooks() + + // Check for ClusterIngress created as a signal that syncHandler ran + h.OnUpdate(&servingClient.Fake, "routes", func(obj runtime.Object) HookResult { + rt := obj.(*v1alpha1.Route) + t.Logf("route updated: %q", rt.Name) - route, err := routeClient.Get(route.Name, metav1.GetOptions{}) - if err != nil { - t.Fatalf("Error getting a route, test: '%s'", test.expectedDomainSuffix) + expectedDomain := fmt.Sprintf("%s.%s.%s", rt.Name, rt.Namespace, test.expectedDomainSuffix) + if rt.Status.Domain != expectedDomain { + t.Logf("Expected domain %q but saw %q", expectedDomain, rt.Status.Domain) + return HookIncomplete + } + + return HookComplete + }) + + servingInformer.Start(stopCh) + kubeInformer.Start(stopCh) + if err := watcher.Start(stopCh); err != nil { + t.Fatalf("failed to start configuration manager: %v", err) } - expectedDomain := fmt.Sprintf("%s.%s.%s", route.Name, route.Namespace, test.expectedDomainSuffix) - if route.Status.Domain != expectedDomain { - t.Errorf("Expected domain %q but saw %q", expectedDomain, route.Status.Domain) + + go controller.Run(1, stopCh) + + // Create a route. + route := getTestRouteWithTrafficTargets([]v1alpha1.TrafficTarget{}) + route.Labels = map[string]string{"app": "prod"} + + routeClient := servingClient.ServingV1alpha1().Routes(route.Namespace) + routeClient.Create(route) + + test.doThings(watcher) + + if err := h.WaitForHooks(3 * time.Second); err != nil { + t.Error(err) } }) } diff --git a/pkg/reconciler/v1alpha1/testing/aliases.go b/pkg/reconciler/v1alpha1/testing/aliases.go index b56158eaa5b2..8d24c7114fbb 100644 --- a/pkg/reconciler/v1alpha1/testing/aliases.go +++ b/pkg/reconciler/v1alpha1/testing/aliases.go @@ -45,5 +45,6 @@ var ( ) const ( - HookComplete = testing.HookComplete + HookComplete = testing.HookComplete + HookIncomplete = testing.HookIncomplete )