Repository: kubernetes-csi/external-provisioner
Pull request #688 (2021-12-10 09:21:10)

bugfix: get capacity grpc request should have timeout
/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**:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #

**Special notes for your reviewer**:

**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".
-->
```release-note
Add getCapacity request timeout to avoid hang forever
```


k8s-ci-robot: @bai3shuo4: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our [release note process](https://git.k8s.io/community/contributors/guide/release-notes.md) to remove it.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
k8s-ci-robot: Hi @bai3shuo4. Thanks for your PR.

I'm waiting for a [kubernetes-csi](https://github.com/orgs/kubernetes-csi/people) 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](https://git.k8s.io/community/community-membership.md#member) 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](https://go.k8s.io/bot-commands?repo=kubernetes-csi%2Fexternal-provisioner).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>

k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-csi/external-provisioner/pull/688#" title="Author self-approved">bai3shuo4</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **jsafrane** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @jsafrane` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-csi%2Fexternal-provisioner).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes-csi/external-provisioner/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["jsafrane"]} -->
k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-csi/external-provisioner/pull/688#" title="Author self-approved">bai3shuo4</a>*, *<a href="https://github.com/kubernetes-csi/external-provisioner/pull/688#pullrequestreview-829873530" title="LGTM">pohly</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **jsafrane** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @jsafrane` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-csi%2Fexternal-provisioner).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes-csi/external-provisioner/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["jsafrane"]} -->
k8s-ci-robot: @bai3shuo4: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-kubernetes-csi-external-provisioner-unit | e0bdab2539d5c5600c98bcdd76f60db16583a416 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-csi_external-provisioner/688/pull-kubernetes-csi-external-provisioner-unit/1470304200435961856) | true | `/test pull-kubernetes-csi-external-provisioner-unit`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-csi&repo=external-provisioner&pr=688). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Abai3shuo4). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-csi/external-provisioner/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
k8s-ci-robot: New changes are detected. LGTM label has been removed.
bai3shuo4: /lgtm

k8s-ci-robot: @bai3shuo4: you cannot LGTM your own PR.

<details>

In response to [this](https://github.com/kubernetes-csi/external-provisioner/pull/688#issuecomment-992446669):

>/lgtm
>


Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
bai3shuo4: @pohly Sorry, I fix the unit test problem, why all this check stales here? How to recover it
bai3shuo4: /retest

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

<details>

In response to [this](https://github.com/kubernetes-csi/external-provisioner/pull/688#issuecomment-992450183):

>/retest
>


Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
pohly: /ok-to-test

k8s-ci-robot: @bai3shuo4: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-kubernetes-csi-external-provisioner-unit | 95d34c05961bd9094a23e8602695d52ff50185d0 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-csi_external-provisioner/688/pull-kubernetes-csi-external-provisioner-unit/1470383841901285376) | true | `/test pull-kubernetes-csi-external-provisioner-unit`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-csi&repo=external-provisioner&pr=688). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Abai3shuo4). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-csi/external-provisioner/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Patch:
diff --git a/cmd/csi-provisioner/csi-provisioner.go b/cmd/csi-provisioner/csi-provisioner.go
index 9ae14d45c..8b14a80da 100644
--- a/cmd/csi-provisioner/csi-provisioner.go
+++ b/cmd/csi-provisioner/csi-provisioner.go
@@ -74,7 +74,7 @@ var (
 	workerThreads        = flag.Uint("worker-threads", 100, "Number of provisioner worker threads, in other words nr. of simultaneous CSI calls.")
 	finalizerThreads     = flag.Uint("cloning-protection-threads", 1, "Number of simultaneously running threads, handling cloning finalizer removal")
 	capacityThreads      = flag.Uint("capacity-threads", 1, "Number of simultaneously running threads, handling CSIStorageCapacity objects")
-	operationTimeout     = flag.Duration("timeout", 10*time.Second, "Timeout for waiting for creation or deletion of a volume")
+	operationTimeout     = flag.Duration("timeout", 10*time.Second, "Timeout for waiting for volume operation (creation, deletion, capacity queries)")
 
 	enableLeaderElection = flag.Bool("leader-election", false, "Enables leader election. If leader election is enabled, additional RBAC rules are required. Please refer to the Kubernetes CSI documentation for instructions on setting up these RBAC rules.")
 
@@ -475,6 +475,7 @@ func main() {
 			factoryForNamespace.Storage().V1beta1().CSIStorageCapacities(),
 			*capacityPollInterval,
 			*capacityImmediateBinding,
+			*operationTimeout,
 		)
 		legacyregistry.CustomMustRegister(capacityController)
 
diff --git a/pkg/capacity/capacity.go b/pkg/capacity/capacity.go
index c1874a8b2..dc5debceb 100644
--- a/pkg/capacity/capacity.go
+++ b/pkg/capacity/capacity.go
@@ -91,6 +91,7 @@ type Controller struct {
 	cInformer        storageinformersv1beta1.CSIStorageCapacityInformer
 	pollPeriod       time.Duration
 	immediateBinding bool
+	timeout          time.Duration
 
 	// capacities contains one entry for each object that is
 	// supposed to exist. Entries that exist on the API server
@@ -164,6 +165,7 @@ func NewCentralCapacityController(
 	cInformer storageinformersv1beta1.CSIStorageCapacityInformer,
 	pollPeriod time.Duration,
 	immediateBinding bool,
+	timeout time.Duration,
 ) *Controller {
 	c := &Controller{
 		csiController:    csiController,
@@ -178,6 +180,7 @@ func NewCentralCapacityController(
 		cInformer:        cInformer,
 		pollPeriod:       pollPeriod,
 		immediateBinding: immediateBinding,
+		timeout:          timeout,
 		capacities:       map[workItem]*storagev1beta1.CSIStorageCapacity{},
 	}
 
@@ -596,7 +599,9 @@ func (c *Controller) syncCapacity(ctx context.Context, item workItem) error {
 			Segments: item.segment.GetLabelMap(),
 		}
 	}
-	resp, err := c.csiController.GetCapacity(ctx, req)
+	syncCtx, cancel := context.WithTimeout(ctx, c.timeout)
+	defer cancel()
+	resp, err := c.csiController.GetCapacity(syncCtx, req)
 	if err != nil {
 		return fmt.Errorf("CSI GetCapacity for %+v: %v", item, err)
 	}


Test patch:
diff --git a/pkg/capacity/capacity_test.go b/pkg/capacity/capacity_test.go
index 3d6fd0f60..513c41553 100644
--- a/pkg/capacity/capacity_test.go
+++ b/pkg/capacity/capacity_test.go
@@ -58,6 +58,7 @@ func init() {
 }
 
 const (
+	timeout        = 10 * time.Second
 	driverName     = "test-driver"
 	ownerNamespace = "testns"
 	csiscRev       = "CSISC-REV-"
@@ -1361,6 +1362,7 @@ func fakeController(ctx context.Context, client *fakeclientset.Clientset, owner
 		cInformer,
 		1000*time.Hour, // Not used, but even if it was, we wouldn't want automatic capacity polling while the test runs...
 		immediateBinding,
+		timeout,
 	)
 
 	// This ensures that the informers are running and up-to-date.