Request coalescing for resizing and modifying volume

[hanyuel]

Is this a bug fix or adding new feature?
New feature

What is this PR about? / Why do we need it?
One restriction of the volume modification support provided by EBS CSI Driver is that if a user updates both annotations and capacity of a PVC at the same time, it will result in 2 RPC calls from external-resizer and volume-modifier sidecars to the CSI Driver, and only one of the them will succeed due to the 6-hour cool down period for EBS ModifyVolume.

This PR solves that problem by implementing the Request Coalescing feature in EBS CSI Driver that merges RPC requests from externa-resizer and volume-modifier sidecars.

What testing is done?
New unit tests && make test and make verify pass.
Built and Deployed a CSI Driver image with the changes in this PR.

1. Created a PVC

apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  name: ebs-claim
spec:
  accessModes:
    - ReadWriteOnce
  storageClassName: ebs-sc
  resources:
    requests:
      storage: 4Gi

2. Verified the PVC via kubectl describe pvc ebs-claim

Name:          ebs-claim
Namespace:     default
StorageClass:  ebs-sc
Status:        Bound
Volume:        pvc-fe46b239-1e50-4e7e-9d99-b041d964c048
Labels:        <none>
Annotations:   pv.kubernetes.io/bind-completed: yes
               pv.kubernetes.io/bound-by-controller: yes
               volume.beta.kubernetes.io/storage-provisioner: ebs.csi.aws.com
               volume.kubernetes.io/selected-node: ip-192-168-38-32.ec2.internal
               volume.kubernetes.io/storage-provisioner: ebs.csi.aws.com
Finalizers:    [kubernetes.io/pvc-protection]
Capacity:      4Gi

3. Updated the PVC with new size, volume type, throughput, iops via kubectl edit.

apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  annotations:
        ebs.csi.aws.com/volumeType: "io2"
        ebs.csi.aws.com/iops: "1000"
        ebs.csi.aws.com/throughput: "1000"

spec:
  accessModes:
  - ReadWriteOnce
  resources:
    requests:
      storage: 5Gi

4. Verified the PVC and PV was updated with the new size, volume type, iops and throughput.

Name:          ebs-claim
Namespace:     default
StorageClass:  ebs-sc
Status:        Bound
Volume:       pvc-fe46b239-1e50-4e7e-9d99-b041d964c048
Labels:        <none>
Annotations:   ebs.csi.aws.com/iops: 1000
               ebs.csi.aws.com/throughput: 1000
               ebs.csi.aws.com/volumeType: io2
               pv.kubernetes.io/bind-completed: yes
               pv.kubernetes.io/bound-by-controller: yes
               volume.beta.kubernetes.io/storage-provisioner: ebs.csi.aws.com
               volume.kubernetes.io/selected-node: ip-192-168-38-32.ec2.internal
               volume.kubernetes.io/storage-provisioner: ebs.csi.aws.com
Finalizers:    [kubernetes.io/pvc-protection]
Capacity:      5Gi
Access Modes:  RWO
VolumeMode:    Filesystem
Used By:       app
Events:
  Type     Reason                        Age                From                                                                                      Message
  ----     ------                        ----               ----                                                                                      -------
  Normal   VolumeModificationStarted     60s                  volume-modifier-for-k8s-ebs.csi.aws.com                                                   External modifier is modifying volume pvc-fe46b239-1e50-4e7e-9d99-b041d964c048
  Warning  ExternalExpanding             60s                  volume_expand                                                                             Ignoring the PVC: didn't find a plugin capable of expanding the volume; waiting for an external controller to process this PVC.
  Normal   Resizing                      60s                  external-resizer ebs.csi.aws.com                                                          External resizer is resizing volume pvc-fe46b239-1e50-4e7e-9d99-b041d964c048
  Normal   VolumeModificationSuccessful  55s                  volume-modifier-for-k8s-ebs.csi.aws.com                                                   External modifier has successfully modified volume pvc-fe46b239-1e50-4e7e-9d99-b041d964c048
  Normal   FileSystemResizeRequired      55s                  external-resizer ebs.csi.aws.com                                                          Require file system resize of volume on node
  Normal   FileSystemResizeSuccessful    26s                  kubelet                                                                                   MountVolume.NodeExpandVolume succeeded for volume "pvc-fe46b239-1e50-4e7e-9d99-b041d964c048" ip-192-168-38-32.ec2.internal

kubectl describe pv   
Name:              pvc-fe46b239-1e50-4e7e-9d99-b041d964c048
Labels:            <none>
Annotations:       ebs.csi.aws.com/iops: 1000
                   ebs.csi.aws.com/throughput: 1000
                   ebs.csi.aws.com/volumeType: io2
                   pv.kubernetes.io/provisioned-by: ebs.csi.aws.com
                   volume.kubernetes.io/provisioner-deletion-secret-name: 
                   volume.kubernetes.io/provisioner-deletion-secret-namespace: 
Finalizers:        [kubernetes.io/pv-protection external-attacher/ebs-csi-aws-com]
StorageClass:      resize-sc
Status:            Bound
Claim:             default/ebs-claim
Reclaim Policy:    Delete
Access Modes:      RWO
VolumeMode:        Filesystem
Capacity:          5Gi

5. Inspected logs.
   csi controller logs:

I0810 17:13:22.132209       1 controller_modify_volume.go:172] "ModifyVolumeProperties called" req="name:\"vol-0b280a33be37a7458\" parameters:{key:\"iops\" value:\"1000\"} parameters:{key:\"throughput\" value:\"1000\"} parameters:{key:\"volumeType\" value:\"io2\"}"
I0810 17:13:22.132413       1 controller_modify_volume.go:107] "Start processing ModifyVolumeRequest for " volume ID="vol-0b280a33be37a7458"
I0810 17:13:22.143910       1 controller.go:442] "ControllerGetCapabilities: called" args={}
I0810 17:13:22.144537       1 controller.go:496] "ControllerExpandVolume: called" args={"volume_id":"vol-0b280a33be37a7458","capacity_range":{"required_bytes":5368709120},"volume_capability":{"AccessType":{"Mount":{"fs_type":"ext4"}},"access_mode":{"mode":1}}}
I0810 17:13:24.144910       1 cloud.go:476] "Received Resize and/or Modify Disk request" volumeID="vol-0b280a33be37a7458" newSizeBytes=5368709120 options={"VolumeType":"io2","IOPS":1000,"Throughput":1000}

resizer sidecar logs:

I0810 17:13:22.143733       1 event.go:298] Event(v1.ObjectReference{Kind:"PersistentVolumeClaim", Namespace:"default", Name:"ebs-claim", UID:"fe46b239-1e50-4e7e-9d99-b041d964c048", APIVersion:"v1", ResourceVersion:"14114656", FieldPath:""}): type: 'Normal' reason: 'Resizing' External resizer is resizing volume pvc-fe46b239-1e50-4e7e-9d99-b041d964c048
I0810 17:13:27.449721       1 controller.go:483] Resize volume succeeded for volume "pvc-fe46b239-1e50-4e7e-9d99-b041d964c048", start to update PV's capacity

volume-modifier sidecar logs:

I0810 17:13:22.131085       1 event.go:298] Event(v1.ObjectReference{Kind:"PersistentVolumeClaim", Namespace:"default", Name:"ebs-claim", UID:"fe46b239-1e50-4e7e-9d99-b041d964c048", APIVersion:"v1", ResourceVersion:"14114654", FieldPath:""}): type: 'Normal' reason: 'VolumeModificationStarted' External modifier is modifying volume pvc-fe46b239-1e50-4e7e-9d99-b041d964c048
I0810 17:13:22.131614       1 csi_modifier.go:62] "Calling modify volume for volume" volumeID="vol-0b280a33be37a7458"
I0810 17:13:27.449787       1 csi_client.go:67] "Volume modification completed" volumeID="vol-0b280a33be37a7458"

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[wmesard]

Looks good!

A couple of comment nits, and one potential locking issue.

[wmesard]

validateModifyVolumeRequest and mergeModifyVolumeRequest need to happen in the same lock-context, otherwise you create a window where an incompatible request can race in after the validate, but before the merge, and we won't notice.

[hanyuel]

Done. See the latest addModifyVolumeRequest and startVolumeTimer methods.

[k8s-ci-robot]

@wmesard: changing LGTM is restricted to collaborators

In response to this:

Looks good!

A couple of comment nits, and one potential locking issue.

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.

[torredil]

This revision largely lgtm, left a few comments.

[ConnorJC3]

Top level comments (some of these I'm sure are obvious, but for the sake of the review):

• Needs the issues with make test and make verify fixed
• Needs unit tests written for the feature
• Otherwise, largely good, but one major deadlock possible (see individual comments)

[ConnorJC3]

/retest

[ConnorJC3]

lgtm

[k8s-ci-robot]

@hanyuel: 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-test-e2e-external-eks-windows	3705632	link	false	/test pull-aws-ebs-csi-driver-test-e2e-external-eks-windows

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[torredil]

/test pull-aws-ebs-csi-driver-external-test

[ConnorJC3]

/lgtm

[torredil]

Thanks @hanyuel /lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: torredil

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

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