terminate plugin clients explicitly instead of using managed ones

[skriss]

Signed-off-by: Steve Kriss steve@heptio.com

Fixes #780

[skriss]

@ncdc this look right? Gonna see if I can add some units but wanted to ensure approach is right first.

[ncdc]

@skriss yes, I think so. I can test it locally to confirm no leak. Give me a few mins...

[skriss]

awesome, thanks!

[ncdc]

I can confirm this fixes the gzip flate Writer memory leak:

heap profile: 9: 2051776 [265: 28097984] @ heap/1048576
1: 663552 [19: 12607488] @ 0x1269e71 0x1275961 0x1af0185 0x1af058c 0x1de65f8 0x1f69df7 0x1f68c4d 0x1f8707e 0x1f684c1 0x1f683cb 0x1f870da 0x14237e4 0x14230cd 0x1422ffd 0x1f84078 0x105b7e1
#	0x1269e70	compress/flate.NewWriter+0x30												/usr/local/Cellar/go/1.10.3/libexec/src/compress/flate/deflate.go:667
#	0x1275960	compress/gzip.(*Writer).Write+0x3b0											/usr/local/Cellar/go/1.10.3/libexec/src/compress/gzip/gzip.go:191
#	0x1af0184	github.com/heptio/ark/vendor/github.com/sirupsen/logrus.Entry.log+0x354							/Users/andy/go/src/github.com/heptio/ark/vendor/github.com/sirupsen/logrus/entry.go:114
#	0x1af058b	github.com/heptio/ark/vendor/github.com/sirupsen/logrus.(*Entry).Info+0xab						/Users/andy/go/src/github.com/heptio/ark/vendor/github.com/sirupsen/logrus/entry.go:141
#	0x1de65f7	github.com/heptio/ark/pkg/backup.(*kubernetesBackupper).Backup+0x3c7							/Users/andy/go/src/github.com/heptio/ark/pkg/backup/backup.go:230
#	0x1f69df6	github.com/heptio/ark/pkg/controller.(*backupController).runBackup+0x3d6						/Users/andy/go/src/github.com/heptio/ark/pkg/controller/backup_controller.go:365
#	0x1f68c4c	github.com/heptio/ark/pkg/controller.(*backupController).processBackup+0x5fc						/Users/andy/go/src/github.com/heptio/ark/pkg/controller/backup_controller.go:279
#	0x1f8707d	github.com/heptio/ark/pkg/controller.(*backupController).(github.com/heptio/ark/pkg/controller.processBackup)-fm+0x3d	/Users/andy/go/src/github.com/heptio/ark/pkg/controller/backup_controller.go:102
#	0x1f684c0	github.com/heptio/ark/pkg/controller.(*backupController).processNextWorkItem+0xd0					/Users/andy/go/src/github.com/heptio/ark/pkg/controller/backup_controller.go:191
#	0x1f683ca	github.com/heptio/ark/pkg/controller.(*backupController).runWorker+0x2a							/Users/andy/go/src/github.com/heptio/ark/pkg/controller/backup_controller.go:178
#	0x1f870d9	github.com/heptio/ark/pkg/controller.(*backupController).(github.com/heptio/ark/pkg/controller.runWorker)-fm+0x29	/Users/andy/go/src/github.com/heptio/ark/pkg/controller/backup_controller.go:165
#	0x14237e3	github.com/heptio/ark/vendor/k8s.io/apimachinery/pkg/util/wait.JitterUntil.func1+0x53					/Users/andy/go/src/github.com/heptio/ark/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:133
#	0x14230cc	github.com/heptio/ark/vendor/k8s.io/apimachinery/pkg/util/wait.JitterUntil+0xbc						/Users/andy/go/src/github.com/heptio/ark/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:134
#	0x1422ffc	github.com/heptio/ark/vendor/k8s.io/apimachinery/pkg/util/wait.Until+0x4c						/Users/andy/go/src/github.com/heptio/ark/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:88
#	0x1f84077	github.com/heptio/ark/pkg/controller.(*backupController).Run.func2+0x67							/Users/andy/go/src/github.com/heptio/ark/pkg/controller/backup_controller.go:165

Note that there's only 1 in-use flate Writer out of 19 allocated.

I need to finish up something else first before I can review the diff, FYI

[skriss]

np - working on a few unit tests rn, though the manual test is the best verification of this.

[skriss]

OK, added some unit tests (didn't go too crazy since this is all changed in master, plus manual verification is more effective for this issue, but hit the core scenarios for closing clients).

[ncdc]

@nrb @carlisia PTAL

[skriss]

@ncdc did you get to do a review on this?

[ncdc]

@skriss minimal, which is why I'd like Nolan & Carlisia to take a look.

[nrb]

LGTM; we're closing the item actions in the controllers, and the actual test shows the leak is gone.

I'm fine merging this unless @skriss wants a 2nd pair of in depth eyes.

[skriss]

Per slack I'm fine with this merging now given the reviews/validation that have been done.