Skip to content

bugfix: get capacity grpc request should have timeout#688

Merged
k8s-ci-robot merged 1 commit intokubernetes-csi:masterfrom
bai3shuo4:bugfix/sync-capacity-timeout
Dec 14, 2021
Merged

bugfix: get capacity grpc request should have timeout#688
k8s-ci-robot merged 1 commit intokubernetes-csi:masterfrom
bai3shuo4:bugfix/sync-capacity-timeout

Conversation

@bai3shuo4
Copy link
Copy Markdown
Contributor

@bai3shuo4 bai3shuo4 commented Dec 10, 2021 •
edited
Loading

/kind bug

bug
What this PR does / why we need it:
If one node csidriver fails or it does not set any timeout for grpc, this sync capacity will stuck here. If we only use one thread to process CSIStoragecapacities, it will stuck forever. Provisioning pv or delete all has timeout, it should have timeout for GetCapacity as well
Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Add getCapacity request timeout to avoid hang forever

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 10, 2021
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Dec 10, 2021

Hi @bai3shuo4. Thanks for your PR.

I'm waiting for a kubernetes-csi 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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 10, 2021
pohly
pohly reviewed Dec 10, 2021
View reviewed changes
cmd/csi-provisioner/csi-provisioner.go
factoryForNamespace.Storage().V1beta1().CSIStorageCapacities(),
*capacityPollInterval,
*capacityImmediateBinding,
*operationTimeout,
Copy link
Copy Markdown
Contributor

@pohly pohly Dec 10, 2021

Choose a reason for hiding this comment

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

Setting some timeout makes sense, I just wonder whether we should use operationTimeout for it. It's currently defined as

        operationTimeout     = flag.Duration("timeout", 10*time.Second, "Timeout for waiting for creation or deletion of a volume")

Perhaps change that into Timeout for volume operations (creation, deletion. capacity queries)?

Copy link
Copy Markdown
Contributor Author

@bai3shuo4 bai3shuo4 Dec 11, 2021

Choose a reason for hiding this comment

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

Creation and deletion all use same timeoutOperation. I think we should keep this pattern. We don't need define too much timeout which is unnecessary

Copy link
Copy Markdown
Contributor

@pohly pohly Dec 12, 2021

Choose a reason for hiding this comment

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

I agree, but then the description should be changed to include the new usage. Querying capacity is not "creation or deletion of a volume" (current description).

Copy link
Copy Markdown
Contributor Author

@bai3shuo4 bai3shuo4 Dec 13, 2021

Choose a reason for hiding this comment

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

Oh！I miss it, I will add it

Copy link
Copy Markdown
Contributor Author

@bai3shuo4 bai3shuo4 Dec 13, 2021

Choose a reason for hiding this comment

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

@pohly done~

@bai3shuo4 bai3shuo4 force-pushed the bugfix/sync-capacity-timeout branch from 332fa4b to e0bdab2 Compare December 13, 2021 03:13
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 13, 2021
@bai3shuo4 bai3shuo4 requested a review from pohly December 13, 2021 08:04
pohly
pohly approved these changes Dec 13, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@pohly pohly left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 13, 2021
@bai3shuo4 bai3shuo4 force-pushed the bugfix/sync-capacity-timeout branch from e0bdab2 to 83eb8c7 Compare December 13, 2021 11:42
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 13, 2021
@bai3shuo4 bai3shuo4 closed this Dec 13, 2021
@bai3shuo4 bai3shuo4 reopened this Dec 13, 2021
@bai3shuo4
Copy link
Copy Markdown
Contributor Author

bai3shuo4 commented Dec 13, 2021

/lgtm

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Dec 13, 2021

@bai3shuo4: you cannot LGTM your own PR.

Details

In response to this:

/lgtm

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.

@bai3shuo4 bai3shuo4 force-pushed the bugfix/sync-capacity-timeout branch from 83eb8c7 to 95d34c0 Compare December 13, 2021 12:52
@bai3shuo4
Copy link
Copy Markdown
Contributor Author

bai3shuo4 commented Dec 13, 2021

@pohly Sorry, I fix the unit test problem, why all this check stales here? How to recover it

@bai3shuo4 bai3shuo4 requested a review from pohly December 13, 2021 12:55
@bai3shuo4
Copy link
Copy Markdown
Contributor Author

bai3shuo4 commented Dec 13, 2021

/retest

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Dec 13, 2021

@bai3shuo4: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/retest

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.

@pohly
Copy link
Copy Markdown
Contributor

pohly commented Dec 13, 2021

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 13, 2021
@pohly pohly mentioned this pull request Dec 13, 2021
Merged
@xing-yang
Copy link
Copy Markdown
Contributor

xing-yang commented Dec 13, 2021

Can you add a release note?

@bai3shuo4 bai3shuo4 force-pushed the bugfix/sync-capacity-timeout branch from 95d34c0 to dcae33f Compare December 14, 2021 03:12
@bai3shuo4
Copy link
Copy Markdown
Contributor Author

bai3shuo4 commented Dec 14, 2021

/retest

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Dec 14, 2021
@bai3shuo4
Copy link
Copy Markdown
Contributor Author

bai3shuo4 commented Dec 14, 2021 •
edited
Loading

Can you add a release note?

@xing-yang Done

@bai3shuo4
Copy link
Copy Markdown
Contributor Author

bai3shuo4 commented Dec 14, 2021

@pohly Hi, I think it's ok to merge :)

@pohly
Copy link
Copy Markdown
Contributor

pohly commented Dec 14, 2021

/lgtm
/assign @xing-yang

For approval.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 14, 2021
@xing-yang
Copy link
Copy Markdown
Contributor

xing-yang commented Dec 14, 2021

/lgtm
/approve

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Dec 14, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bai3shuo4, pohly, xing-yang

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

The pull request process is described here

Details 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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 14, 2021
@k8s-ci-robot k8s-ci-robot merged commit 0b71727 into kubernetes-csi:master Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Jiawei0227 Jiawei0227 Awaiting requested review from Jiawei0227

@pohly pohly Awaiting requested review from pohly

Assignees

@pohly pohly

@xing-yang xing-yang

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bai3shuo4 @k8s-ci-robot @pohly @xing-yang