Skip to content

Commit

Permalink
Refactor global resync tests to use our "hooks" package. (#2685)
Browse files Browse the repository at this point in the history 
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: #2443
  • Loading branch information
@mattmoor @knative-prow-robot
mattmoor authored and knative-prow-robot committed Dec 18, 2018
1 parent ce262fe commit d872d6a
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 90 deletions.
57 changes: 24 additions & 33 deletions pkg/reconciler/v1alpha1/clusteringress/clusteringress_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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,
Expand All @@ -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)
}
}
100 changes: 44 additions & 56 deletions pkg/reconciler/v1alpha1/route/route_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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)
}
})
}
Expand Down
3 changes: 2 additions & 1 deletion pkg/reconciler/v1alpha1/testing/aliases.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,5 +45,6 @@ var (
)

const (
HookComplete = testing.HookComplete
HookComplete = testing.HookComplete
HookIncomplete = testing.HookIncomplete
)

0 comments on commit d872d6a

Please sign in to comment.