Skip to content

Conversation

@qts0312
Copy link

@qts0312 qts0312 commented Dec 5, 2025

This PR follows concurrency: add timeout for LeaseGrant in NewSession #20022 as the original PR has no updates and been closed now. The credit should belong to @Antraxmin.

What this PR does

This PR adds a timeout to the LeaseGrant RPC call in the NewSession function to prevent infinite blocking.

Why this change is needed

Currently, LeaseGrant in NewSession is called without a deadline, which can cause the function to block indefinitely if the RPC call hangs. This is inconsistent with the Close() method, which properly uses context.WithTimeout for the LeaseRevoke call.

Changes made

  • Modified NewSession function in client/v3/concurrency/session.go to use context.WithTimeout for LeaseGrant call
  • The timeout is set to defaultLeaseTimeout seconds
  • Added integration tests to verify timeout behavior

Testing

  • All existing integration tests pass
  • Added new tests to verify timeout behavior works correctly
  • Verified normal session creation still works as expected

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: qts0312
Once this PR has been reviewed and has the lgtm label, please assign fuweid for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

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

@k8s-ci-robot
Copy link

Hi @qts0312. Thanks for your PR.

I'm waiting for a github.com 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.

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.

@serathius
Copy link
Member

Please sign DCO

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already tests/integration/clientv3/concurrency/session_test.go that has only 100 lines, don't think we need a separate file for timeout.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have simplified the tests and moved them to session_test.go

@qts0312 qts0312 force-pushed the fix-lease-grant-timeout branch 2 times, most recently from 95fd1c1 to 2c01014 Compare December 5, 2025 09:23
@qts0312 qts0312 force-pushed the fix-lease-grant-timeout branch from 2c01014 to 7b5df49 Compare December 5, 2025 09:27
@qts0312 qts0312 force-pushed the fix-lease-grant-timeout branch from 4436dbf to 1db0837 Compare December 5, 2025 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

Missing deadline for LeaseGrant when creating session

3 participants