add optional timeout to CSI lib connection utils

[andrewsykim]

Allows users of the connection lib to set an optional timeout. If no timeout is set, the default is to retry forever.

/assign @msau42 @jsafrane @vladimirvivien

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: andrewsykim
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: pohly

If they are not already assigned, you can assign the PR to them by writing /assign @pohly in a comment when ready.

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

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

[andrewsykim]

Decided to remove dialOptions here because I don't think it makes sense to pass in dial options into an unexported function. Also, it seems it was only added for the connect timeout test case anyway.

[pohly]

A more flexible solution for specifying a timeout is to add a context.Context parameter to the Connect function. Then one can specify both a deadline and a timeout, depending on the situation. If we expose a timeout option at all in the API, then I would prefer the solution based on context.

@andrewsykim why do you think such an option is needed?

When @jsafrane added this function, he intentionally limited the API to just what the CSI sidecars need, to ensure consistent behavior. That's why the testing is partly using an internal helper function.

[msau42]

The connect forever option doesn't work well with the liveness probe sidecar because the health check will hang instead of returning an error.

@andrewsykim can you confirm if the Kubernetes liveness probe can appropriately handle the health check hanging?

[andrewsykim]

IIRC, the default (and minimum) livenessprobe timeout on kubelet is 1s, so liveness check hanging and not returning an error would still allow the kubelet to restart the container within the timeout specified on the container spec.

If we're okay assuming this, then maybe a timeout option is not needed at all. If we do want the livenessprobe sidecar to explicitly return 500 when a timeout is reached then being able to pass in a timeout via the connection lib would be useful.

[andrewsykim]

A more flexible solution for specifying a timeout is to add a context.Context parameter to the Connect function. Then one can specify both a deadline and a timeout, depending on the situation. If we expose a timeout option at all in the API, then I would prefer the solution based on context.

I can update to use context instead, though we may lose some of the functionality we get passing in the timeout as a grpc dial option. Either works for me.

[msau42]

liveness check hanging and not returning an error would still allow the kubelet to restart the container within the timeout specified

Then I think in that case, it is fine to have the liveness probe hang on connect(). The only downside I see, is that healthz requests may pile up on the liveness probe if the driver is forever in an unready state.

How about something like this for the liveness probe:

1. Instead of connecting to the driver on every healthz check, connect to it once in the beginning. This can try to connect forever.
2. Whenever a healthz request comes, call Probe() with timeout, so that we can return a 500 immediately and not have outstanding requests.

[pohly]

What functionality could get lost?

Note that gRPC already deprecated WithTimeout in favor of context.WithTimeout - https://godoc.org/google.golang.org/grpc#WithTimeout.

[andrewsykim]

How about something like this for the liveness probe:

1. Instead of connecting to the driver on every healthz check, connect to it once in the beginning. This can try to connect forever.
2. Whenever a healthz request comes, call Probe() with timeout, so that we can return a 500 immediately and not have outstanding requests.

Sounds good. I'll update livenessprobe to do this as soon as #17 is merged

Note that gRPC already deprecated WithTimeout in favor of context.WithTimeout - https://godoc.org/google.golang.org/grpc#WithTimeout.

I didn't know this, this is good to know thanks!

[andrewsykim]

/close

Closing for now, see #19 (comment) for more details

[k8s-ci-robot]

@andrewsykim: Closed this PR.

In response to this:

/close

Closing for now, see #19 (comment) for more details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.