Repository: kubernetes-sigs/aws-ebs-csi-driver
Pull request #2029 (2024-05-01 15:00:58)

Tune batcher EC2 Describe* delays
**Is this a bug fix or adding new feature?**
Improvement

**What is this PR about? / Why do we need it?**
Given that no throttling is observed for DescribeVolumes with 7k volume scalability tests, we reduced the current 1 second max batch delay to 500ms, and set that as the standard batcher delay. 

This PR makes sure each RPC takes ~0.25s of batch latency per EC2 Describe call on average (and worst case extra delay of 0.5s).  

This PR also makes sure each batcher will execute twice per second. In the rare case that each batcher is executing at once, the combined 12 requests per second is under the default [EC2 Non-Mutating Action Bucket Refill Rate](https://docs.aws.amazon.com/ec2/latest/devguide/ec2-api-throttling.html) of 20.

**What testing is done?** 
Scalability tests with batcher delays of 1, 0.5, 0.3, and 0.2 seconds for DescribeVolumes and DescribeInstances. 

Final 5000 pod scalability test on default limits account.

|               API | Count |   OK | Client Error | Throttled |
|--|--|--|--|--|
|      CreateVolume |  5676 | 5030 |            0 |       646 |
|      DeleteVolume |  5563 | 4999 |            2 |       562 |
|      AttachVolume |  5646 | 4989 |            0 |       657 |
|      DetachVolume |  5747 | 4996 |          101 |       650 |
|   DescribeVolumes |  4482 | 4482 |            0 |         0 |
| DescribeInstances |  3760 | 3760 |            0 |         0 |



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

This pull-request has been approved by:
**Once this PR has been reviewed and has the lgtm label**, please ask for approval from [andrewsirenko](https://github.com/andrewsirenko). For more information see [the Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

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

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

- **[OWNERS](https://github.com/kubernetes-sigs/aws-ebs-csi-driver/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":["andrewsirenko"]} -->
github-actions[bot]: <!-- pr-coverage -->
## Code Coverage Diff
File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/cloud/cloud.go | 84.2% | 84.1% | -0.1
k8s-ci-robot: @AndrewSirenko: 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-aws-ebs-csi-driver-unit | c8b79c2a2bca30837b821195c98eec6f1352d875 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_aws-ebs-csi-driver/2029/pull-aws-ebs-csi-driver-unit/1785685864232259584) | true | `/test pull-aws-ebs-csi-driver-unit`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=aws-ebs-csi-driver&pr=2029). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3AAndrewSirenko). 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-sigs/aws-ebs-csi-driver/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: @AndrewSirenko: 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-aws-ebs-csi-driver-verify | 51bc41946be85590ed60f542f0d2be4a88689773 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_aws-ebs-csi-driver/2029/pull-aws-ebs-csi-driver-verify/1785720213572947968) | true | `/test pull-aws-ebs-csi-driver-verify`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=aws-ebs-csi-driver&pr=2029). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3AAndrewSirenko). 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-sigs/aws-ebs-csi-driver/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: @AndrewSirenko: 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-aws-ebs-csi-driver-unit | 458701d6d59f8ce6777d90c5df99da32ed5662fd | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_aws-ebs-csi-driver/2029/pull-aws-ebs-csi-driver-unit/1785724766234087424) | true | `/test pull-aws-ebs-csi-driver-unit`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=aws-ebs-csi-driver&pr=2029). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3AAndrewSirenko). 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-sigs/aws-ebs-csi-driver/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: @AndrewSirenko: 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-aws-ebs-csi-driver-verify | 1282f7856de064febe40e7896da09031840e0ff2 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_aws-ebs-csi-driver/2029/pull-aws-ebs-csi-driver-verify/1785737895680675840) | true | `/test pull-aws-ebs-csi-driver-verify`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=aws-ebs-csi-driver&pr=2029). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3AAndrewSirenko). 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-sigs/aws-ebs-csi-driver/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/pkg/cloud/cloud.go b/pkg/cloud/cloud.go
index 9889c6c1..3537e216 100644
--- a/pkg/cloud/cloud.go
+++ b/pkg/cloud/cloud.go
@@ -86,6 +86,20 @@ var (
 		VolumeTypeST1,
 		VolumeTypeStandard,
 	}
+
+	volumeCreationPollInitialDelay    = 1250 * time.Millisecond
+	volumeCreationPollBackoffDuration = 500 * time.Millisecond
+	volumeCreationPollBackoffFactor   = 1.5
+	volumeCreationPollBackoffSteps    = 25
+	volumeCreationPollBackoffCap      = 3 * time.Second
+
+	volumeModificationDuration   = 1 * time.Second
+	volumeModificationWaitFactor = 1.7
+	volumeModificationWaitSteps  = 10
+
+	volumeAttachmentStatePollSteps  = 13
+	volumeAttachmentStatePollDelay  = 1 * time.Second
+	volumeAttachmentStatePollFactor = 1.8
 )
 
 const (
@@ -137,6 +151,7 @@ const (
 	snapshotTagBatcher
 
 	batchDescribeTimeout = 30 * time.Second
+	batchMaxDelay        = 500 * time.Millisecond // Minimizes RPC latency and EC2 API calls. Tuned via scalability tests.
 )
 
 var (
@@ -251,47 +266,6 @@ type ec2ListSnapshotsResponse struct {
 	NextToken *string
 }
 
-// volumeWaitParameters dictates how to poll for volume events.
-// E.g. how often to check if volume is created after an EC2 CreateVolume call.
-type volumeWaitParameters struct {
-	creationInitialDelay time.Duration
-	creationBackoff      wait.Backoff
-	modificationBackoff  wait.Backoff
-	attachmentBackoff    wait.Backoff
-}
-
-var (
-	vwp = volumeWaitParameters{
-		// Based on our testing in us-west-2 and ap-south-1, the median/p99 time until volume creation is ~1.5/~4 seconds.
-		// We have found that the following parameters are optimal for minimizing provisioning time and DescribeVolumes calls
-		// we queue DescribeVolume calls after [1.25, 0.5, 0.75, 1.125, 1.7, 2.5, 3] seconds.
-		// In total, we wait for ~60 seconds.
-		creationInitialDelay: 1250 * time.Millisecond,
-		creationBackoff: wait.Backoff{
-			Duration: 500 * time.Millisecond,
-			Factor:   1.5,
-			Steps:    25,
-			Cap:      3 * time.Second,
-		},
-
-		// Most attach/detach operations on AWS finish within 1-4 seconds.
-		// By using 1 second starting interval with a backoff of 1.8,
-		// we get [1, 1.8, 3.24, 5.832000000000001, 10.4976].
-		// In total, we wait for 2601 seconds.
-		attachmentBackoff: wait.Backoff{
-			Duration: 1 * time.Second,
-			Factor:   1.8,
-			Steps:    13,
-		},
-
-		modificationBackoff: wait.Backoff{
-			Duration: 1 * time.Second,
-			Factor:   1.7,
-			Steps:    10,
-		},
-	}
-)
-
 // volumeBatcherType is an enum representing the types of volume batchers available.
 type volumeBatcherType int
 
@@ -314,7 +288,6 @@ type cloud struct {
 	dm     dm.DeviceManager
 	bm     *batcherManager
 	rm     *retryManager
-	vwp    volumeWaitParameters
 }
 
 var _ Cloud = &cloud{}
@@ -371,35 +344,38 @@ func newEC2Cloud(region string, awsSdkDebugLog bool, userAgentExtra string, batc
 		bm = newBatcherManager(svc)
 	}
 
+	rm := newRetryManager()
+
 	return &cloud{
 		region: region,
 		dm:     dm.NewDeviceManager(),
 		ec2:    svc,
 		bm:     bm,
-		rm:     newRetryManager(),
-		vwp:    vwp,
+		rm:     rm,
 	}
 }
 
 // newBatcherManager initializes a new instance of batcherManager.
+// Each batcher's `entries` set to maximum results returned by relevant EC2 API call without pagination.
+// Each batcher's `delay` minimizes RPC latency and EC2 API calls. Tuned via scalability tests.
 func newBatcherManager(svc EC2API) *batcherManager {
 	return &batcherManager{
-		volumeIDBatcher: batcher.New(500, 1*time.Second, func(ids []string) (map[string]*types.Volume, error) {
+		volumeIDBatcher: batcher.New(500, batchMaxDelay, func(ids []string) (map[string]*types.Volume, error) {
 			return execBatchDescribeVolumes(svc, ids, volumeIDBatcher)
 		}),
-		volumeTagBatcher: batcher.New(500, 1*time.Second, func(names []string) (map[string]*types.Volume, error) {
+		volumeTagBatcher: batcher.New(500, batchMaxDelay, func(names []string) (map[string]*types.Volume, error) {
 			return execBatchDescribeVolumes(svc, names, volumeTagBatcher)
 		}),
-		instanceIDBatcher: batcher.New(50, 300*time.Millisecond, func(ids []string) (map[string]*types.Instance, error) {
+		instanceIDBatcher: batcher.New(50, batchMaxDelay, func(ids []string) (map[string]*types.Instance, error) {
 			return execBatchDescribeInstances(svc, ids)
 		}),
-		snapshotIDBatcher: batcher.New(1000, 300*time.Millisecond, func(ids []string) (map[string]*types.Snapshot, error) {
+		snapshotIDBatcher: batcher.New(1000, batchMaxDelay, func(ids []string) (map[string]*types.Snapshot, error) {
 			return execBatchDescribeSnapshots(svc, ids, snapshotIDBatcher)
 		}),
-		snapshotTagBatcher: batcher.New(1000, 300*time.Millisecond, func(names []string) (map[string]*types.Snapshot, error) {
+		snapshotTagBatcher: batcher.New(1000, batchMaxDelay, func(names []string) (map[string]*types.Snapshot, error) {
 			return execBatchDescribeSnapshots(svc, names, snapshotTagBatcher)
 		}),
-		volumeModificationIDBatcher: batcher.New(500, 300*time.Millisecond, func(names []string) (map[string]*types.VolumeModification, error) {
+		volumeModificationIDBatcher: batcher.New(500, batchMaxDelay, func(names []string) (map[string]*types.VolumeModification, error) {
 			return execBatchDescribeVolumesModifications(svc, names)
 		}),
 	}
@@ -994,6 +970,16 @@ func (c *cloud) DetachDisk(ctx context.Context, volumeID, nodeID string) error {
 
 // WaitForAttachmentState polls until the attachment status is the expected value.
 func (c *cloud) WaitForAttachmentState(ctx context.Context, volumeID, expectedState string, expectedInstance string, expectedDevice string, alreadyAssigned bool) (*types.VolumeAttachment, error) {
+	// Most attach/detach operations on AWS finish within 1-4 seconds.
+	// By using 1 second starting interval with a backoff of 1.8,
+	// we get [1, 1.8, 3.24, 5.832000000000001, 10.4976].
+	// In total we wait for 2601 seconds.
+	backoff := wait.Backoff{
+		Duration: volumeAttachmentStatePollDelay,
+		Factor:   volumeAttachmentStatePollFactor,
+		Steps:    volumeAttachmentStatePollSteps,
+	}
+
 	var attachment *types.VolumeAttachment
 
 	verifyVolumeFunc := func(ctx context.Context) (bool, error) {
@@ -1080,7 +1066,7 @@ func (c *cloud) WaitForAttachmentState(ctx context.Context, volumeID, expectedSt
 		return false, nil
 	}
 
-	return attachment, wait.ExponentialBackoffWithContext(ctx, c.vwp.attachmentBackoff, verifyVolumeFunc)
+	return attachment, wait.ExponentialBackoffWithContext(ctx, backoff, verifyVolumeFunc)
 }
 
 func (c *cloud) GetDiskByName(ctx context.Context, name string, capacityBytes int64) (*Disk, error) {
@@ -1550,13 +1536,26 @@ func (c *cloud) listSnapshots(ctx context.Context, request *ec2.DescribeSnapshot
 
 // waitForVolume waits for volume to be in the "available" state.
 func (c *cloud) waitForVolume(ctx context.Context, volumeID string) error {
-	time.Sleep(c.vwp.creationInitialDelay)
+	// Based on our testing in us-west-2 and ap-south-1, the median/p99 time until volume creation is ~1.5/~4 seconds.
+	// We have found that the following parameters are optimal for minimizing provisioning time and DescribeVolumes calls
+	// we queue DescribeVolume calls after [1.25, 0.5, 0.75, 1.125, 1.7, 2.5, 3] seconds.
+	// In total, we wait for ~60 seconds.
+	var (
+		backoff = wait.Backoff{
+			Duration: volumeCreationPollBackoffDuration,
+			Factor:   volumeCreationPollBackoffFactor,
+			Steps:    volumeCreationPollBackoffSteps,
+			Cap:      volumeCreationPollBackoffCap,
+		}
+	)
+
+	time.Sleep(volumeCreationPollInitialDelay)
 
 	request := &ec2.DescribeVolumesInput{
 		VolumeIds: []string{volumeID},
 	}
 
-	err := wait.ExponentialBackoffWithContext(ctx, c.vwp.creationBackoff, func(ctx context.Context) (done bool, err error) {
+	err := wait.ExponentialBackoffWithContext(ctx, backoff, func(ctx context.Context) (done bool, err error) {
 		vol, err := c.getVolume(ctx, request)
 		if err != nil {
 			return true, err
@@ -1686,7 +1685,13 @@ func (c *cloud) checkDesiredState(ctx context.Context, volumeID string, desiredS
 
 // waitForVolumeModification waits for a volume modification to finish.
 func (c *cloud) waitForVolumeModification(ctx context.Context, volumeID string) error {
-	waitErr := wait.ExponentialBackoff(c.vwp.modificationBackoff, func() (bool, error) {
+	backoff := wait.Backoff{
+		Duration: volumeModificationDuration,
+		Factor:   volumeModificationWaitFactor,
+		Steps:    volumeModificationWaitSteps,
+	}
+
+	waitErr := wait.ExponentialBackoff(backoff, func() (bool, error) {
 		m, err := c.getLatestVolumeModification(ctx, volumeID, true)
 		// Consider volumes that have never been modified as done
 		if err != nil && errors.Is(err, VolumeNotBeingModified) {
diff --git a/pkg/cloud/retry_manager.go b/pkg/cloud/retry_manager.go
index 077991d7..5258d38d 100644
--- a/pkg/cloud/retry_manager.go
+++ b/pkg/cloud/retry_manager.go
@@ -1,19 +1,3 @@
-/*
-Copyright 2024 The Kubernetes Authors.
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-    http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
-
 package cloud
 
 import (


Test patch:
diff --git a/pkg/cloud/cloud_test.go b/pkg/cloud/cloud_test.go
index 99b21ed8..65fd1cee 100644
--- a/pkg/cloud/cloud_test.go
+++ b/pkg/cloud/cloud_test.go
@@ -20,7 +20,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"k8s.io/apimachinery/pkg/util/wait"
 	"reflect"
 	"strings"
 	"sync"
@@ -932,7 +931,7 @@ func TestCreateDisk(t *testing.T) {
 				AvailabilityZone: "",
 			},
 			expCreateVolumeInput: &ec2.CreateVolumeInput{},
-			expErr:               fmt.Errorf("timed out waiting for volume to create: timed out waiting for the condition"),
+			expErr:               fmt.Errorf("timed out waiting for volume to create: context deadline exceeded"),
 		},
 		{
 			name:       "success: normal from snapshot",
@@ -2388,6 +2387,8 @@ func TestResizeOrModifyDisk(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			mockCtrl := gomock.NewController(t)
 			mockEC2 := NewMockEC2API(mockCtrl)
+			// reduce number of steps to reduce test time
+			volumeModificationWaitSteps = 3
 			c := newCloud(mockEC2)
 
 			ctx := context.Background()
@@ -2989,6 +2990,8 @@ func TestWaitForAttachmentState(t *testing.T) {
 		},
 	}
 
+	volumeAttachmentStatePollSteps = 1
+
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 			mockCtrl := gomock.NewController(t)
@@ -3069,28 +3072,12 @@ func TestWaitForAttachmentState(t *testing.T) {
 	}
 }
 
-func testVolumeWaitParameters() volumeWaitParameters {
-	testBackoff := wait.Backoff{
-		Duration: 100 * time.Millisecond,
-		Factor:   1,
-		Steps:    3,
-	}
-
-	return volumeWaitParameters{
-		creationInitialDelay: 0,
-		creationBackoff:      testBackoff,
-		attachmentBackoff:    testBackoff,
-		modificationBackoff:  testBackoff,
-	}
-}
-
 func newCloud(mockEC2 EC2API) Cloud {
 	c := &cloud{
 		region: "test-region",
 		dm:     dm.NewDeviceManager(),
 		ec2:    mockEC2,
 		rm:     newRetryManager(),
-		vwp:    testVolumeWaitParameters(),
 	}
 	return c
 }