🐛 Close etcd client, connections, and data streams after each use

[vincepri]

Signed-off-by: Vince Prignano vincepri@vmware.com

What this PR does / why we need it:

This PR fixes a bug where we were not closing the etcd clients after each use.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #3482

/milestone v0.3.9
/kind bug

[vincepri]

/assign @ncdc

[vincepri]

cc @jdef @fabriziopandini

[jdef]

this addresses the first part of the problem. closing the etcd client isn't enough though because the dialer leaks a SPDY connection, as I also documented in the bug.

spdy multiplexes connections, and closing the etcd client will close an "inner" stream, but leaves the outer connection (the one that holds all the inner streams) remaining open. if this is not fixed, then the memory leak persists.

[jdef]

this addresses the first part of the problem. closing the etcd client isn't enough though because the dialer leaks a SPDY connection, as I also documented in the bug.

spdy multiplexes connections, and closing the etcd client will close an "inner" stream, but leaves the outer connection (the one that holds all the inner streams) remaining open. if this is not fixed, then the memory leak persists.

dataStream is created here:
https://github.com/kubernetes-sigs/cluster-api/blob/master/controlplane/kubeadm/internal/proxy/dial.go#L115

and then it's returned as the stream that the etcd client uses. but p (which produces dataStream) is left dangling and is never properly closed, even if dataStream is closed. my solution was to embed dataStream in a wrapper struct that satisfied the needed interface, and overloaded Close and Reset such that (effectively) p.Close was also called in such cases.

[ncdc]

Sounds like we should fix dial.go so p isn't dangling/inaccessible

[ncdc]

Maybe NewConn should take in p as well so it can close it

[jdef]

Maybe NewConn should take in p as well so it can close it

either way. i embedded the value of p in the struct that wraps the dataStream value (so the struct embeds the value of dataStream as an unnamed field, and p as a named field). and the just overloaded:

func (..) Close() error {... }
func (..) Reset() error { ... }

to also invoke p.Close() after closing the data stream. this keeps NewConn simple and focused on what it needs to do, and keeps the complexity of the wrapper in the dialer, which is already making decisions about how to deal w/ multiplexing.

also, in the dialer func, where it it returns early due to errors, it should close p before actually returning, otherwise it'll leak there too

[vincepri]

@ncdc @jdef Take another look, I ended up cleaning up the functions as well given the confusing they created (very similar, but different purposes)

[vincepri]

While having a connection and a stream tied together defeats the purpose of multiplexing, that's what we're doing today + leaking the connection

[vincepri]

/milestone v0.3.9
/kind bug

[jdef]

this looks functionally equiv to the fix that I made in my prototype

/approve

[ncdc]

Reviewing
/hold
for comments

[ncdc]

/lgtm

I'd like @randomvariable to review as well.

/assign @randomvariable

Please feel free to remove the hold if you're happy with it.

[vincepri]

/test pull-cluster-api-e2e

[fabriziopandini]

sgtm for me.
only a question but not blocking

[fabriziopandini]

For my understanding: why are we creating an error string and then closing it immediately after?

[randomvariable]

my understanding is that you need to create the error stream in all cases for the websocket to function, even though for port-forwarding, it's not used. I could be wrong. It was based off of https://github.com/kubernetes/client-go/blob/master/tools/portforward/portforward.go#L324:L324

[ncdc]

The protocol I designed for port-forward, attach, and exec multiplexes different streams in a single connection. All of these require an error stream, which is used for communicating errors about the port-forward/attach/exec back to the client. With SPDY, either side (client or server) can close its write end of the stream. With the error stream here, the client never writes to it, so we need to create it and then "close" it, which really only closes the client's write half.

[randomvariable]

looks good, thanks for deborking.

/unhold

[vincepri]

/retitle Close etcd client, connections, and data streams after each use

[fabriziopandini]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, jdef

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [fabriziopandini]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment