Skip to content

client: Respect client.WithTimeout option#11508

Merged
estesp merged 1 commit intocontainerd:mainfrom
vvoland:client-timeout-dialer
Mar 13, 2025
Merged

client: Respect client.WithTimeout option#11508
estesp merged 1 commit intocontainerd:mainfrom
vvoland:client-timeout-dialer

Conversation

@vvoland
Copy link
Contributor

@vvoland vvoland commented Mar 7, 2025

Fix the gRPC client dialer not using the timeout passed by the containerd client timeout option.

Commit 63b4688 replaced the usage of deprecated grpc.DialContext with grpc.NewClient.

However, the dialer.ContextDialer relied on the context deadline to propagate the timeout:

ctx, cancel = context.WithTimeout(ctx, cc.dopts.timeout)

This assumption is now broken, because grpc.NewClient doesn't do any initial connection and defers it to the first RPC usage.

This commit passes the timeout to the connection dialer, so the timeout will be applied to every connection attempt (not just the first).

Signed-off-by: Paweł Gronowski pawel.gronowski@docker.com

@k8s-ci-robot
Copy link

Hi @vvoland. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

Fix the gRPC client dialer not using the timeout passed by the
containerd client timeout option.

Commit 63b4688 replaced the usage of deprecated `grpc.DialContext`
with `grpc.NewClient`.

However, the `dialer.ContextDialer` relied on the context deadline to
propagate the timeout:

https://github.com/containerd/containerd/blob/388fb336b0a458e2cf64212072743e622a3f44c7/vendor/google.golang.org/grpc/clientconn.go#L216

This assumption is now broken, because `grpc.NewClient` doesn't do any
initial connection and defers it to the first RPC usage.

This commit passes the timeout via the `MinConnectTimeout` grpc
connection param, which will be applied to **every** connection attempt
(not just the first).

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@github-project-automation github-project-automation bot moved this from Needs Triage to Review In Progress in Pull Request Review Mar 11, 2025
@dmcgowan dmcgowan added the cherry-pick/2.0.x Change to be cherry picked to release/2.0 branch label Mar 11, 2025
@dmcgowan dmcgowan added this pull request to the merge queue Mar 11, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 11, 2025
@dmcgowan dmcgowan added this pull request to the merge queue Mar 13, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 13, 2025
@estesp estesp added this pull request to the merge queue Mar 13, 2025
Merged via the queue into containerd:main with commit 92885b1 Mar 13, 2025
56 checks passed
@github-project-automation github-project-automation bot moved this from Review In Progress to Done in Pull Request Review Mar 13, 2025
@dmcgowan
Copy link
Member

/cherry-pick release/2.0

@k8s-infra-cherrypick-robot

@dmcgowan: new pull request created: #11536

Details

In response to this:

/cherry-pick release/2.0

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-sigs/prow repository.

@dmcgowan dmcgowan added cherry-picked/2.0.x PR commits are cherry picked into the release/2.0 branch and removed cherry-pick/2.0.x Change to be cherry picked to release/2.0 branch labels Mar 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/client Go client cherry-picked/2.0.x PR commits are cherry picked into the release/2.0 branch kind/bug needs-ok-to-test size/XS

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

7 participants