Switch to using goleaks to check for leaks

This switches to using the goleaks package to check for leaks, which
should give us a more complete picture of the particular goroutine
that's leaking, and should avoid issues where we leak a goroutine, but
also stop an old one.

This also force-closes keep-alive connections in the leak tests, since
those look like leaks, but will actually time out after 30s (outside the
timescope of the test).

Author: DirectXMan12

go.mod

@@ -15,6 +15,7 @@ require (
 	github.com/onsi/gomega v1.10.1
 	github.com/prometheus/client_golang v1.7.1
 	github.com/prometheus/client_model v0.2.0
+	go.uber.org/goleak v1.1.10
 	go.uber.org/zap v1.15.0
 	golang.org/x/time v0.0.0-20200630173020-3af7569d3a1e
 	gomodules.xyz/jsonpatch/v2 v2.1.0

go.sum

@@ -255,6 +255,7 @@ github.com/mailru/easyjson v0.7.0/go.mod h1:KAzv3t3aY1NaHWoQz1+4F1ccyAH66Jk7yos7
 github.com/mattn/go-colorable v0.0.9/go.mod h1:9vuHe8Xs5qXnSaW/c/ABM9alt+Vo+STaOChaDxuIBZU=
 github.com/mattn/go-isatty v0.0.4/go.mod h1:M+lRXTBqGeGNdLjl/ufCoiOlB5xdOkqRJdNxMWT7Zi4=
 github.com/mattn/go-runewidth v0.0.2/go.mod h1:LwmH8dsx7+W8Uxz3IHJYH5QSwggIsqBzpuz5H//U1FU=
+github.com/matttproud/golang_protobuf_extensions v1.0.1 h1:4hp9jkHxhMHkqkrB3Ix0jegS5sx/RkqARlsWZ6pIwiU=
 github.com/matttproud/golang_protobuf_extensions v1.0.1/go.mod h1:D8He9yQNgCq6Z5Ld7szi9bcBfOoFv/3dc6xSMkL2PC0=
 github.com/matttproud/golang_protobuf_extensions v1.0.2-0.20181231171920-c182affec369 h1:I0XW9+e1XWDxdcEniV4rQAIOPUGDq67JSCiRCgGCZLI=
 github.com/matttproud/golang_protobuf_extensions v1.0.2-0.20181231171920-c182affec369/go.mod h1:BSXmuO+STAnVfrANrmjBb36TMTDstsz7MSK+HVaYKv4=
@@ -374,6 +375,8 @@ go.uber.org/atomic v1.4.0 h1:cxzIVoETapQEqDhQu3QfnvXAV4AlzcvUCxkVUFw3+EU=
 go.uber.org/atomic v1.4.0/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE=
 go.uber.org/atomic v1.6.0 h1:Ezj3JGmsOnG1MoRWQkPBsKLe9DwWD9QeXzTRzzldNVk=
 go.uber.org/atomic v1.6.0/go.mod h1:sABNBOSYdrvTF6hTgEIbc7YasKWGhgEQZyfxyTvoXHQ=
+go.uber.org/goleak v1.1.10 h1:z+mqJhf6ss6BSfSM671tgKyZBFPTTJM+HLxnhPC3wu0=
+go.uber.org/goleak v1.1.10/go.mod h1:8a7PlsEVH3e/a/GLqe5IIrQx6GzcnRmZEufDUTk4A7A=
 go.uber.org/multierr v1.1.0 h1:HoEmRHQPVSqub6w2z2d2EOVs2fjyFRGyofhKuyDq0QI=
 go.uber.org/multierr v1.1.0/go.mod h1:wR5kodmAFQ0UK8QlbwjlSNy0Z68gJhDJUG5sjR94q/0=
 go.uber.org/multierr v1.5.0 h1:KCa4XfM8CWFCpxXRGok+Q0SS/0XBhMDbHHGABQLvD2A=
@@ -521,6 +524,7 @@ golang.org/x/tools v0.0.0-20190911174233-4f2ddba30aff/go.mod h1:b+2E5dAYhXwXZwtn
 golang.org/x/tools v0.0.0-20191012152004-8de300cfc20a/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo=
 golang.org/x/tools v0.0.0-20191029041327-9cc4af7d6b2c/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo=
 golang.org/x/tools v0.0.0-20191029190741-b9c20aec41a5/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo=
+golang.org/x/tools v0.0.0-20191108193012-7d206e10da11/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo=
 golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo=
 golang.org/x/tools v0.0.0-20191125144606-a911d9008d1f/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo=
 golang.org/x/tools v0.0.0-20191227053925-7b8e75db28f4/go.mod h1:TB2adYChydJhpapKDTa4BR/hXlZSLoq2Wpct/0txZ28=

pkg/controller/controller_suite_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package controller_test
 
 import (
+	"net/http"
 	"testing"
 
 	. "github.com/onsi/ginkgo"
@@ -45,6 +46,9 @@ var testenv *envtest.Environment
 var cfg *rest.Config
 var clientset *kubernetes.Clientset
 
+// clientTransport is used to force-close keep-alives in tests that check for leaks
+var clientTransport *http.Transport
+
 var _ = BeforeSuite(func(done Done) {
 	logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)))
 
@@ -64,6 +68,9 @@ var _ = BeforeSuite(func(done Done) {
 	cfg, err = testenv.Start()
 	Expect(err).NotTo(HaveOccurred())
 
+	clientTransport = &http.Transport{}
+	cfg.Transport = clientTransport
+
 	clientset, err = kubernetes.NewForConfig(cfg)
 	Expect(err).NotTo(HaveOccurred())
 

pkg/controller/controller_test.go

@@ -19,12 +19,10 @@ package controller_test
 import (
 	"context"
 	"fmt"
-	"os"
-	rt "runtime"
-	"runtime/pprof"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
+	"go.uber.org/goleak"
 
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
@@ -97,35 +95,38 @@ var _ = Describe("controller.Controller", func() {
 		})
 
 		It("should not leak goroutines when stopped", func() {
-			// NB(directxman12): this test was flaky before on CI, but my guess
-			// is that the flakiness was caused by an expect on the count.
-			// Eventually should fix it, but if not, consider disabling again.
+			currentGRs := goleak.IgnoreCurrent()
+
 			m, err := manager.New(cfg, manager.Options{})
 			Expect(err).NotTo(HaveOccurred())
 
 			_, err = controller.New("new-controller", m, controller.Options{Reconciler: rec})
 			Expect(err).NotTo(HaveOccurred())
 
-			startGoroutines := rt.NumGoroutine()
 			s := make(chan struct{})
 			close(s)
 
-			Expect(m.Start(s)).NotTo(HaveOccurred())
-			Eventually(rt.NumGoroutine /* pass the function, don't call it */).Should(Equal(startGoroutines))
+			Expect(m.Start(s)).To(Succeed())
+
+			// force-close keep-alive connections.  These'll time anyway (after
+			// like 30s or so) but force it to speed up the tests.
+			clientTransport.CloseIdleConnections()
+			Eventually(func() error { return goleak.Find(currentGRs) }).Should(Succeed())
 		})
 
 		It("should not create goroutines if never started", func() {
-			startGoroutines := rt.NumGoroutine()
-			Expect(pprof.Lookup("goroutine").WriteTo(os.Stdout, 1)).To(Succeed())
+			currentGRs := goleak.IgnoreCurrent()
 
 			m, err := manager.New(cfg, manager.Options{})
 			Expect(err).NotTo(HaveOccurred())
 
 			_, err = controller.New("new-controller", m, controller.Options{Reconciler: rec})
 			Expect(err).NotTo(HaveOccurred())
 
-			Expect(pprof.Lookup("goroutine").WriteTo(os.Stdout, 1)).To(Succeed())
-			Eventually(rt.NumGoroutine /* pass func, don't call */).Should(Equal(startGoroutines))
+			// force-close keep-alive connections.  These'll time anyway (after
+			// like 30s or so) but force it to speed up the tests.
+			clientTransport.CloseIdleConnections()
+			Eventually(func() error { return goleak.Find(currentGRs) }).Should(Succeed())
 		})
 	})
 })

pkg/manager/manager_suite_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package manager
 
 import (
+	"net/http"
 	"testing"
 
 	. "github.com/onsi/ginkgo"
@@ -40,6 +41,9 @@ var testenv *envtest.Environment
 var cfg *rest.Config
 var clientset *kubernetes.Clientset
 
+// clientTransport is used to force-close keep-alives in tests that check for leaks
+var clientTransport *http.Transport
+
 var _ = BeforeSuite(func(done Done) {
 	logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)))
 
@@ -49,6 +53,9 @@ var _ = BeforeSuite(func(done Done) {
 	cfg, err = testenv.Start()
 	Expect(err).NotTo(HaveOccurred())
 
+	clientTransport = &http.Transport{}
+	cfg.Transport = clientTransport
+
 	clientset, err = kubernetes.NewForConfig(cfg)
 	Expect(err).NotTo(HaveOccurred())
 

pkg/manager/manager_test.go

@@ -24,7 +24,6 @@ import (
 	"net"
 	"net/http"
 	"path"
-	rt "runtime"
 	"strings"
 	"sync"
 	"time"
@@ -33,6 +32,7 @@ import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	"github.com/prometheus/client_golang/prometheus"
+	"go.uber.org/goleak"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -1204,25 +1204,24 @@ var _ = Describe("manger.Manager", func() {
 		})
 	})
 
-	// This test has been marked as pending because it has been causing lots of flakes in CI.
-	// It should be rewritten at some point later in the future.
 	It("should not leak goroutines when stopped", func() {
-		// NB(directxman12): this test was flaky before on CI, but my guess
-		// is that the flakiness was caused by an expect on the count.
-		// Eventually should fix it, but if not, consider disabling again.
+		currentGRs := goleak.IgnoreCurrent()
+
 		m, err := New(cfg, Options{})
 		Expect(err).NotTo(HaveOccurred())
-		startGoroutines := rt.NumGoroutine()
 
 		s := make(chan struct{})
 		close(s)
 		Expect(m.Start(s)).NotTo(HaveOccurred())
 
-		Eventually(rt.NumGoroutine /* pass the function, don't call it */).Should(Equal(startGoroutines))
+		// force-close keep-alive connections.  These'll time anyway (after
+		// like 30s or so) but force it to speed up the tests.
+		clientTransport.CloseIdleConnections()
+		Eventually(func() error { return goleak.Find(currentGRs) }).Should(Succeed())
 	})
 
 	It("should not leak goroutines if the default event broadcaster is used & events are emitted", func() {
-		startGoroutines := rt.NumGoroutine()
+		currentGRs := goleak.IgnoreCurrent()
 
 		m, err := New(cfg, Options{ /* implicit: default setting for EventBroadcaster */ })
 		Expect(err).NotTo(HaveOccurred())
@@ -1261,7 +1260,10 @@ var _ = Describe("manger.Manager", func() {
 		close(stopCh)
 		<-doneCh
 
-		Eventually(rt.NumGoroutine /* pass the function, don't call it */).Should(Equal(startGoroutines))
+		// force-close keep-alive connections.  These'll time anyway (after
+		// like 30s or so) but force it to speed up the tests.
+		clientTransport.CloseIdleConnections()
+		Eventually(func() error { return goleak.Find(currentGRs) }).Should(Succeed())
 	})
 
 	It("should provide a function to get the Config", func() {