Configurable throughput for clients to the API server.

[RaunakShah]

Adding configurable throughput for clients to the API server.
Use a separate client for leader election go routine.

Testing
Kubernetes version: 1.17.4
CSI driver: Dummy driver that always returns success for CreateVolume() and DeleteVolume() callbacks with randomized VolumeID's.

Test: Use 64-threads to create and delete PVCs in a loop for 5 minutes. Tracking the number of operations successfully executed within this time helps calculate the throughput.
Logs - https://gist.github.com/RaunakShah/ac18639695ff968df84021c1ba12df95

1. QPS = 5.0, Burst = 10 (Default)
   a. Number of successful Create and Delete operations - 728
   b. Ops/sec - 2.435
   I have not pasted the entire log file here but there were 1780 instances of "Throttling ..." within these 5 minutes.

root@k8-master-440:/var/log/containers# cat !$ | grep Throttl | wc -l
cat vsphere-csi-controller-69cbc685fb-8gn9p_kube-system_csi-provisioner-bc80ce4ae96c1b0025f73b2166da2028fc59be55c9aa5b1ea12d848df0fa3fa6.log | grep Throttl | wc -l
1780

2. QPS = 10.0, Burst = 20
   a. Number of successful Create and Delete operations - 1404
   b. Ops/sec - 4.69

3. QPS = 25.0, Burst = 50
   a. Number of successful Create and Delete operations - 1666
   b. Ops/sec - 5.57

4. QPS = 50.0, Burst = 100
   a. Number of successful Create and Delete operations - 1703
   b. Ops/sec - 5.696
   Zero instances of "Throttling ..." within these 5 minutes.

root@k8-master-440:/var/log/containers# cat !$ | grep Throt | wc -l
cat vsphere-csi-controller-7bcbdbc485-9g9vp_kube-system_csi-provisioner-16fff97dcb70cf23f09004656e5b32e574d59be39d4f1ec5ebd5f3a36b25f29a.log | grep Throt | wc -l
0

What type of PR is this?
/kind bug

What this PR does / why we need it:
This change adds two new flags "kube-api-qps" and "kube-api-burst" to configure the QPS and Burst of clients to the API server. These values default to 5 which is not very efficient at larger scales.
The default QPS and Burst for clients is changed from 5 to 50.
This change also uses a separate client for leader election go routine.

Which issue(s) this PR fixes:

Fixes #416

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Use a separate client for leader election go routine and add kube-api-qps and kube-api-burst configurable parameters for the provisioner's kubernetes client. 

[k8s-ci-robot]

Hi @RaunakShah. 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.

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.

[RaunakShah]

/assign @xing-yang @jsafrane

[xing-yang]

/ok-to-test

[xing-yang]

We probably want to have different sets of QPS/Burst config parameters for the leader election client and the client for create/delete operations.

[RaunakShah]

I thought about that too, but it seemed a little superfluous to add two more flags just for a leader election client. There's also a snapshot client that uses the same config, so that would need its own QPS and Burst too then..

[xing-yang]

I think it's fine for the snapshot client to use the same config as the client that handles create/delete operations.
Let's see if @msau42 has any suggestions here.

[msau42]

Can you explain how Burst interacts with QPS?

Also are you able to get a profile about what operations are coming from the provisioner? I want to make sure we're not hiding inefficiencies or bugs that could be fixed, such as using informers or not rate limiting/backing off. It may be worth talking to sig-scalability or sig-api-machinery about reasonable qps values would be because there are tradeoffs if we give everything a high qps.

[RaunakShah]

NewForConfig() uses a "token bucket approach". QPS is the guaranteed number of queries that are processed per second. Burst is the size of the bucket/queue that is filled at the rate of QPS. So requests to the API server will only be throttled after there are "Burst" number of items in the bucket/queue.

There are some Get() operations on the API server that could potentially be replaced with informers, but Create()/Update()/Delete() are still limited by the throughput of these clients. I've updated the testing section with more information but using QPS/Burst of 50/100 increases ops/sec of CreateVolume and DeleteVolume from ~2.5 to ~5.5.

Can we potentially tag someone from sig-scalability or sig-api-machinery here for their input?

[msau42]

I'm surprised we need a qps of 50 to achieve 5.5 ops per second. Does that imply we're making 10 api calls per operation?

cc @wojtek-t if you have any thoughts on reasonable client qps/burst values

[wojtek-t]

Forgive my ignorance here, but is csi-provisioner a O(1) instance in the cluster or there can be a lot of them (e.g. one per node or sth like that).
If there is O(1), setting to 50 sounds fine, if there are a lot of them, that sounds like too much too me.

BTW - I second Michelle`s question - if you need 50qps to achieve 5.5ops that doesn't sound very promissing...

[msau42]

@RaunakShah thanks for putting together the benchmarks! The results are concerning to me, that increasing qps 10x only achieves 2x in ops throughput. Could you also try increasing qps 2x, and 5x to get some more datapoints?

(Also separately, I think your benchmarking work is very valuable, and we should consider adding it into our CI and also try to get CSI tested in sig-scalability's test framework as well)

[RaunakShah]

Yup i'll play around a little and add some more results shortly!
I'm not sure if these results are entirely accurate because i've done some hacking with the CSI driver as well, but a reasonable baseline i would think

[RaunakShah]

@msau42 i've updated the PR description with the test results. Please take a look!

I also did some further analysis. Interestingly, there were "Throttling ... " messages for four operations:

1. Posting events:

{"log":"I0625 17:10:17.542869       1 request.go:565] Throttling request took 221.204186ms, request: POST:https://10.96.0.1:443/api/v1/namespaces/default/events\n","stream":"stderr","time":"2020-06-25T17:10:17.54314291Z"}

2. Posting PVs:

{"log":"I0625 17:10:17.741553       1 request.go:565] Throttling request took 402.941089ms, request: POST:https://10.96.0.1:443/api/v1/persistentvolumes\n","stream":"stderr","time":"2020-06-25T17:10:17.741860753Z"}

3. Deleting PVs:

{"log":"I0625 17:10:21.741317       1 request.go:565] Throttling request took 3.330344146s, request: DELETE:https://10.96.0.1:443/api/v1/persistentvolumes/pvc-90b8a0f3-6914-4ae3-9744-74fbc6e0f3c4\n","stream":"stderr","time":"2020-06-25T17:10:21.741666156Z"}

4. Get PVs:

{"log":"I0625 17:10:20.541396       1 request.go:565] Throttling request took 2.367093054s, request: GET:https://10.96.0.1:443/api/v1/persistentvolumes/pvc-c6fca517-345a-42b5-bde1-ea707f16e084\n","stream":"stderr","time":"2020-06-25T17:10:20.541660324Z"}

(1), (2) and (3) above probably can't be replaced but external-provisioner lib does a Get() on the API server for every provisioning and delete operation (ref: https://github.com/kubernetes-sigs/sig-storage-lib-external-provisioner/blob/master/controller/controller.go#L1290 and https://github.com/kubernetes-sigs/sig-storage-lib-external-provisioner/blob/master/controller/controller.go#L1428). The same clientset is used to initialize ProvisionController - https://github.com/kubernetes-csi/external-provisioner/blob/master/cmd/csi-provisioner/csi-provisioner.go#L246

I did another test by adding a pvLister in sig-storage-lib-external-provisioner instead of querying API server and got a ~2x increase in throughput for default qps and burst! I still see "Throttling ... " messages upto a QPS of 25 but not for any Get() operations

1. QPS = 5.0, Burst = 10
   a. Number of successful Create and Delete operations - 1425
   b. Ops/sec - 4.76

root@k8-master-440:/var/log/containers/with-lib-change# cat qps5.log | wc -l
2450

2. QPS = 10.0, Burst = 20
   a. Number of successful Create and Delete operations - 1586
   b. Ops/sec - 5.30

root@k8-master-440:/var/log/containers/with-lib-change# cat qps10.log | wc -l
3137

3. QPS = 25.0, Burst = 50
   a. Number of successful Create and Delete operations - 1694
   b. Ops/sec - 5.66

root@k8-master-440:/var/log/containers/with-lib-change# cat qps25.log | wc -l
0

Since this change doesn't address sig-storage-lib-external-provisioner, i kept this out of the PR description, but i think its worth addressing in subsequent changes..

[msau42]

That's really great! Assuming we can fix lib-external-provisioner, it doesn't look like increasing the default qps/burst values will significantly improve throughput. What do you think of this plan?

• Make improvements to sig-storage-lib-external-provisioner to use informers
• Separate out clientset used for leader election
• Make qps/burst configurable but use client default values as the default
• Future: tuning other parameters like backoff ratio/interval
• Future: Add benchmarking integration test using mock driver to our CI
• Future: Add e2e scalability tests using mock driver to sig-scalability test framework

[RaunakShah]

Yup, sounds good to me. I've updated this PR accordingly. Should i go ahead and raise an issue/PR/both on lib-external-provisioner?

[msau42]

Yup that sounds good!

[msau42]

/lgtm
/approve

Can you also extend the release note to say that the leader election client is separate? Go ahead and "/hold cancel" when updated thanks!
/hold

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msau42, RaunakShah

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 [msau42]

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

[RaunakShah]

/hold cancel

[chrishenzie]

Hi @RaunakShah , do you mind sharing the benchmark setup / tool you used to collect these metrics? I'm trying to gain more context on this issue and am curious how contributors collect these metrics.