mirrors/external-dns
1
0
Fork 0
mirror of https://github.com/kubernetes-sigs/external-dns.git synced 2025-08-05 17:16:59 +02:00
Code Issues Packages Projects Releases Wiki Activity

Don't run CRD informer during tests

Browse Source 
This change disables the CRD source's informer during tests.  I made the mistake
of not running `make test` before the previous commit, and thus didn't realize
that leaving the informer enabled during the tests introduced a race condition:

	WARNING: DATA RACE
	Write at 0x00c0005aa130 by goroutine 59:
	  k8s.io/client-go/rest/fake.(*RESTClient).do()
		  /Users/erath/go/pkg/mod/k8s.io/client-go@v0.18.8/rest/fake/fake.go:113 +0x69
	  k8s.io/client-go/rest/fake.(*RESTClient).do-fm()
		  /Users/erath/go/pkg/mod/k8s.io/client-go@v0.18.8/rest/fake/fake.go:109 +0x64
	  k8s.io/client-go/rest/fake.roundTripperFunc.RoundTrip()
		  /Users/erath/go/pkg/mod/k8s.io/client-go@v0.18.8/rest/fake/fake.go:43 +0x3d
	  net/http.send()
		  /usr/local/go/src/net/http/client.go:251 +0x6da
	  net/http.(*Client).send()
		  /usr/local/go/src/net/http/client.go:175 +0x1d5
	  net/http.(*Client).do()
		  /usr/local/go/src/net/http/client.go:717 +0x2cb
	  net/http.(*Client).Do()
		  /usr/local/go/src/net/http/client.go:585 +0x68b
	  k8s.io/client-go/rest.(*Request).request()
		  /Users/erath/go/pkg/mod/k8s.io/client-go@v0.18.8/rest/request.go:855 +0x209
	  k8s.io/client-go/rest.(*Request).Do()
		  /Users/erath/go/pkg/mod/k8s.io/client-go@v0.18.8/rest/request.go:928 +0xf0
	  sigs.k8s.io/external-dns/source.(*crdSource).List()
		  /Users/erath/go/src/github.com/ericrrath/external-dns/source/crd.go:250 +0x28c
	  sigs.k8s.io/external-dns/source.NewCRDSource.func1()
		  /Users/erath/go/src/github.com/ericrrath/external-dns/source/crd.go:125 +0x10a
	  k8s.io/client-go/tools/cache.(*ListWatch).List()
		  /Users/erath/go/pkg/mod/k8s.io/client-go@v0.18.8/tools/cache/listwatch.go:106 +0x94
	  k8s.io/client-go/tools/cache.(*Reflector).ListAndWatch.func1.1.2()
		  /Users/erath/go/pkg/mod/k8s.io/client-go@v0.18.8/tools/cache/reflector.go:233 +0xf4
	  k8s.io/client-go/tools/pager.SimplePageFunc.func1()
		  /Users/erath/go/pkg/mod/k8s.io/client-go@v0.18.8/tools/pager/pager.go:40 +0x94
	  k8s.io/client-go/tools/pager.(*ListPager).List()
		  /Users/erath/go/pkg/mod/k8s.io/client-go@v0.18.8/tools/pager/pager.go:91 +0x1f4
	  k8s.io/client-go/tools/cache.(*Reflector).ListAndWatch.func1.1()
		  /Users/erath/go/pkg/mod/k8s.io/client-go@v0.18.8/tools/cache/reflector.go:258 +0x2b7

	Previous write at 0x00c0005aa130 by goroutine 37:
	  k8s.io/client-go/rest/fake.(*RESTClient).do()
		  /Users/erath/go/pkg/mod/k8s.io/client-go@v0.18.8/rest/fake/fake.go:113 +0x69
	  k8s.io/client-go/rest/fake.(*RESTClient).do-fm()
		  /Users/erath/go/pkg/mod/k8s.io/client-go@v0.18.8/rest/fake/fake.go:109 +0x64
	  k8s.io/client-go/rest/fake.roundTripperFunc.RoundTrip()
		  /Users/erath/go/pkg/mod/k8s.io/client-go@v0.18.8/rest/fake/fake.go:43 +0x3d
	  net/http.send()
		  /usr/local/go/src/net/http/client.go:251 +0x6da
	  net/http.(*Client).send()
		  /usr/local/go/src/net/http/client.go:175 +0x1d5
	  net/http.(*Client).do()
		  /usr/local/go/src/net/http/client.go:717 +0x2cb
	  net/http.(*Client).Do()
		  /usr/local/go/src/net/http/client.go:585 +0x68b
	  k8s.io/client-go/rest.(*Request).request()
		  /Users/erath/go/pkg/mod/k8s.io/client-go@v0.18.8/rest/request.go:855 +0x209
	  k8s.io/client-go/rest.(*Request).Do()
		  /Users/erath/go/pkg/mod/k8s.io/client-go@v0.18.8/rest/request.go:928 +0xf0
	  sigs.k8s.io/external-dns/source.(*crdSource).List()
		  /Users/erath/go/src/github.com/ericrrath/external-dns/source/crd.go:250 +0x28c
	  sigs.k8s.io/external-dns/source.(*crdSource).Endpoints()
		  /Users/erath/go/src/github.com/ericrrath/external-dns/source/crd.go:171 +0x13c4
	  sigs.k8s.io/external-dns/source.testCRDSourceEndpoints.func1()
		  /Users/erath/go/src/github.com/ericrrath/external-dns/source/crd_test.go:388 +0x4f6
	  testing.tRunner()
		  /usr/local/go/src/testing/testing.go:1193 +0x202

	Goroutine 59 (running) created at:
	  k8s.io/client-go/tools/cache.(*Reflector).ListAndWatch.func1()
		  /Users/erath/go/pkg/mod/k8s.io/client-go@v0.18.8/tools/cache/reflector.go:224 +0x36f
	  k8s.io/client-go/tools/cache.(*Reflector).ListAndWatch()
		  /Users/erath/go/pkg/mod/k8s.io/client-go@v0.18.8/tools/cache/reflector.go:316 +0x1ab
	  k8s.io/client-go/tools/cache.(*Reflector).Run.func1()
		  /Users/erath/go/pkg/mod/k8s.io/client-go@v0.18.8/tools/cache/reflector.go:177 +0x4a
	  k8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1()
		  /Users/erath/go/pkg/mod/k8s.io/apimachinery@v0.18.8/pkg/util/wait/wait.go:155 +0x75
	  k8s.io/apimachinery/pkg/util/wait.BackoffUntil()
		  /Users/erath/go/pkg/mod/k8s.io/apimachinery@v0.18.8/pkg/util/wait/wait.go:156 +0xba
	  k8s.io/client-go/tools/cache.(*Reflector).Run()
		  /Users/erath/go/pkg/mod/k8s.io/client-go@v0.18.8/tools/cache/reflector.go:176 +0xee
	  k8s.io/client-go/tools/cache.(*Reflector).Run-fm()
		  /Users/erath/go/pkg/mod/k8s.io/client-go@v0.18.8/tools/cache/reflector.go:174 +0x54
	  k8s.io/apimachinery/pkg/util/wait.(*Group).StartWithChannel.func1()
		  /Users/erath/go/pkg/mod/k8s.io/apimachinery@v0.18.8/pkg/util/wait/wait.go:56 +0x45
	  k8s.io/apimachinery/pkg/util/wait.(*Group).Start.func1()
		  /Users/erath/go/pkg/mod/k8s.io/apimachinery@v0.18.8/pkg/util/wait/wait.go:73 +0x6d

	Goroutine 37 (running) created at:
	  testing.(*T).Run()
		  /usr/local/go/src/testing/testing.go:1238 +0x5d7
	  sigs.k8s.io/external-dns/source.testCRDSourceEndpoints()
		  /Users/erath/go/src/github.com/ericrrath/external-dns/source/crd_test.go:376 +0x1fcf
	  testing.tRunner()
		  /usr/local/go/src/testing/testing.go:1193 +0x202

It looks like client-go's fake.RESTClient (used by crd_test.go) is known to
cause race conditions when used with informers:
<https://github.com/kubernetes/kubernetes/issues/95372>.  None of the CRD tests
_depend_ on the informer yet, so disabling the informer at least allows the
existing tests to pass without race conditions.  I'll look into further changes
that 1) test the new event-handler behavior, and 2) allow all tests to pass
without race conditions.
This commit is contained in:
Eric R. Rath 2021-08-13 11:45:22 -07:00
parent 5146dab6fa
commit 585d752ca4
3 changed files with 37 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

67
source/crd.go
View File

@ -108,7 +108,7 @@ func NewCRDClientForAPIVersionKind(client kubernetes.Interface, kubeConfig, apiS
}
// NewCRDSource creates a new crdSource with the given config.
func NewCRDSource(crdClient rest.Interface, namespace, kind string, annotationFilter string, labelSelector labels.Selector, scheme *runtime.Scheme) (Source, error) {
func NewCRDSource(crdClient rest.Interface, namespace, kind string, annotationFilter string, labelSelector labels.Selector, scheme *runtime.Scheme, startInformer bool) (Source, error) {
sourceCrd := crdSource{
crdResource: strings.ToLower(kind) + "s",
namespace: namespace,
@ -117,43 +117,46 @@ func NewCRDSource(crdClient rest.Interface, namespace, kind string, annotationFi
crdClient: crdClient,
codec: runtime.NewParameterCodec(scheme),
}
// external-dns already runs its sync-handler periodically (controlled by `--interval` flag) to ensure any
// missed or dropped events are handled. specify a resync period 0 to avoid unnecessary sync handler invocations.
informer := cache.NewSharedInformer(
&cache.ListWatch{
ListFunc: func(lo metav1.ListOptions) (result runtime.Object, err error) {
return sourceCrd.List(context.TODO(), &lo)
if startInformer {
// external-dns already runs its sync-handler periodically (controlled by `--interval` flag) to ensure any
// missed or dropped events are handled. specify a resync period 0 to avoid unnecessary sync handler invocations.
informer := cache.NewSharedInformer(
&cache.ListWatch{
ListFunc: func(lo metav1.ListOptions) (result runtime.Object, err error) {
return sourceCrd.List(context.TODO(), &lo)
},
WatchFunc: func(lo metav1.ListOptions) (watch.Interface, error) {
return sourceCrd.watch(context.TODO(), &lo)
},
},
WatchFunc: func(lo metav1.ListOptions) (watch.Interface, error) {
return sourceCrd.watch(context.TODO(), &lo)
},
},
&endpoint.DNSEndpoint{},
0)
sourceCrd.informer = &informer
go informer.Run(wait.NeverStop)
&endpoint.DNSEndpoint{},
0)
sourceCrd.informer = &informer
go informer.Run(wait.NeverStop)
}
return &sourceCrd, nil
}
func (cs *crdSource) AddEventHandler(ctx context.Context, handler func()) {
log.Debug("Adding event handler for CRD")
// Right now there is no way to remove event handler from informer, see:
// https://github.com/kubernetes/kubernetes/issues/79610
informer := *cs.informer
informer.AddEventHandler(
cache.ResourceEventHandlerFuncs{
AddFunc: func(obj interface{}) {
handler()
if cs.informer != nil {
log.Debug("Adding event handler for CRD")
// Right now there is no way to remove event handler from informer, see:
// https://github.com/kubernetes/kubernetes/issues/79610
informer := *cs.informer
informer.AddEventHandler(
cache.ResourceEventHandlerFuncs{
AddFunc: func(obj interface{}) {
handler()
},
UpdateFunc: func(old interface{}, new interface{}) {
handler()
},
DeleteFunc: func(obj interface{}) {
handler()
},
},
UpdateFunc: func(old interface{}, new interface{}) {
handler()
},
DeleteFunc: func(obj interface{}) {
handler()
},
},
)
)
}
}
// Endpoints returns endpoint objects.

2
source/crd_test.go
View File

@ -387,7 +387,7 @@ func testCRDSourceEndpoints(t *testing.T) {
labelSelector, err := labels.Parse(ti.labelFilter)
require.NoError(t, err)
cs, err := NewCRDSource(restClient, ti.namespace, ti.kind, ti.annotationFilter, labelSelector, scheme)
cs, err := NewCRDSource(restClient, ti.namespace, ti.kind, ti.annotationFilter, labelSelector, scheme, false)
require.NoError(t, err)
receivedEndpoints, err := cs.Endpoints(context.Background())

2
source/store.go
View File

@ -270,7 +270,7 @@ func BuildWithConfig(ctx context.Context, source string, p ClientGenerator, cfg
if err != nil {
return nil, err
}
return NewCRDSource(crdClient, cfg.Namespace, cfg.CRDSourceKind, cfg.AnnotationFilter, cfg.LabelFilter, scheme)
return NewCRDSource(crdClient, cfg.Namespace, cfg.CRDSourceKind, cfg.AnnotationFilter, cfg.LabelFilter, scheme, true)
case "skipper-routegroup":
apiServerURL := cfg.APIServerURL
tokenPath := ""