[konnectivity-client] Ensure grpc tunnel is closed on dial failure#398
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
| // Override goleakVerifyNone to set t.Helper. | ||
| // TODO: delete this once goleak has been updated to include | ||
| // https://github.com/uber-go/goleak/commit/2dfebe88ddf19de216c4ab15a1189fc640b1ea9f | ||
| func goleakVerifyNone(t *testing.T, options ...goleak.Option) { |
There was a problem hiding this comment.
Consider making this part of a util in konn-client.
There was a problem hiding this comment.
Ack. Will follow up in a separate PR.
| klog.V(5).InfoS("Context canceled waiting for DialResp", "ctxErr", requestCtx.Err(), "dialID", random) | ||
| go t.closeDial(random) | ||
| return nil, &dialFailure{"dial timeout, context", DialFailureContext} | ||
| case <-t.done: |
There was a problem hiding this comment.
Do we really need 3 paths for we're shutting down?
There was a problem hiding this comment.
Unfortunately yes, I don't see a clean way to consolidate these. The timeout & context canceled cases could easily be collapsed, but then we wouldn't be able to distinguish between them in the logs. This <-t.done case is not strictly necessary (the timeout case would be hit eventually), but it makes for a cleaner shutdown if the underlying connection gets closed prematurely (outside of konnectivity-client).
tallclair
force-pushed
the
client-dial-cls
branch
from
4d50eb9 to
529507c
Compare
k8s-ci-robot
added
size/XL
|
Addressed feedback. Also added a commit to to ensure the tunnel is always closed when the |
|
|
||
| if !ok { | ||
| // If the DIAL_RSP does not match a pending dial, it means one of two things: | ||
| // 1. There was a second DIAL_RSP for the connection request (this is very unlikely but possible) |
There was a problem hiding this comment.
I don't think we should ever get two DIAL_RSP for a pending dial. Collisions shouldn't be possible given the client sets req.Random. Maybe this could happen if the server had a race between sending the DIAL_RESP and a DIAL_CLS on something like a timeout?
There was a problem hiding this comment.
Yeah, I was also confused by this comment. The comment was there before (moved to here), but I can't think of when this would occur. I'll update it.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheftako, tallclair The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
3 participants
3 commits:
Note to reviewers: I'm hoping to backport this to older Kubernetes versions, so consider backwards compatibility (both with older k8s, as well as older proxy-servers). I think it's OK, but would appreciate extra attention on this detail.
/assign @cheftako