Proposal: Add a ModifyVolume RPC to the Controller Service

[vdhanan]

AWS customers would like to modify their volumes (e.g. increase IOPS, change volume type, etc.), but there is currently no way to do this through the CSI Spec. The current workaround some customers use is to modify their storage through the AWS CLI, but that solution does not work for everyone. Can we introduce a generic ModifyVolume RPC in the Controller Service to modify volume type and throughput/IOPS? I see that Azure's CLI has a similar command to update a Disk's properties, so this may be useful for other Storage Providers' customers as well.

[jdef]

How do you see this playing out in higher level, CO implementations?

On Wed, Oct 6, 2021, 6:49 PM Varun Dhananjaya @.***> wrote:

AWS customers would like to modify their volumes (e.g. increase IOPS, change volume type, etc.), but there is currently no way to do this through the CSI Spec. The current workaround some customers use is to modify their storage through the AWS CLI https://docs.aws.amazon.com/cli/latest/reference/ec2/modify-volume.html, but that solution does not work for everyone. Can we introduce a generic ModifyVolume RPC in the Controller Service to modify volume type and throughput/IOPS? I see that Azure's CLI https://docs.microsoft.com/en-us/cli/azure/disk?view=azure-cli-latest#az_disk_update has a similar command to update a Disk's properties, so this may be useful for other Storage Providers' customers as well.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/container-storage-interface/spec/issues/491, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAR5KLAEIGTPSYTA3UL7BWTUFTG7RANCNFSM5FP5KSNQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[vdhanan]

I can speak on K8s, since that's the CO I'm most familiar with... I'm wondering if we could introduce a field in the StorageClass object (immutable) called allowVolumeModification and move the modifiable volume properties like IOPS, volume type, etc. into the PersistentVolumeClaim (mutable). I'm not sure if this is feasible, and there might be a better way, so I'm open to suggestions.

[jdef]

Sounds like you need to get k8s folks on board before we tackle anything in the spec. Maybe start with a KEP in sig-storage?

On Fri, Nov 19, 2021, 11:38 AM Varun Dhananjaya @.***> wrote:

I can speak on K8s, since that's the CO I'm most familiar with... I'm wondering if we could introduce a field in the StorageClass object (immutable) called allowVolumeModification and move the modifiable volume properties like IOPS, volume type, etc. into the PersistentVolumeClaim (mutable). I'm not sure if this is feasible, and there might be a better way, so I'm open to suggestions.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/container-storage-interface/spec/issues/491#issuecomment-974227214, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAR5KLG2AO5QIBQJZ4V6Y4TUMZ4PXANCNFSM5FP5KSNQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[gnufied]

Since these fields are not exposed as properties in PVC, I don't see how users can modify them. I am not even sure if k8s will want to add those properties to PVC, so I have to agree with @jdef you will need a KEP to tackle this in k8s before a proposal can be brought forward to CSI.

[dhilipkumars]

+1 we have similar requirement for azure ultradisk here

[jdef]

My answer is unchanged. K8s KEP is likely the way to start with this.

On Tue, Dec 21, 2021, 8:04 AM Dhilip @.***> wrote:

+1 we have similar requirement for azure ultradisk [here] ( kubernetes-sigs/azuredisk-csi-driver#1118 https://github.com/kubernetes-sigs/azuredisk-csi-driver/issues/1118)

— Reply to this email directly, view it on GitHub https://github.com/container-storage-interface/spec/issues/491#issuecomment-998761998, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAR5KLG4S65VXOIMZTKACLTUSB3ORANCNFSM5FP5KSNQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

[mikee]

Could I argue that some cases of this are to align with user expectations based on the way IOPS are specified For example: AWS based storage class specifies IOPS per GB, but the underlying API uses a raw IOPS figure. However - when a PV is expanded to a larger size, the driver does not recalculate the IOPs - so the size is increased, but the IOPS stays the same as what was calculated in the original create.

While changing things like the volume type should go to KEP - I'd suggest the case I've outlined is a bug

[jdef]

I'm trying to understand this use case (and thanks for providing it), so please bear with me...

(a) User creates PV => results in AWS vol created w/ "IOPS/GB" CSI vol parameter (b) User resizes PV => results in larger AWS vol, GB increases but IOPS do not -- so former IOPS/GB CSI vol parameter becomes invalid (because it no longer reflects reality)?

.. and it is being stated that this is a fundamental limitation of the CSI specification, and the proposed solution is to add an RPC to the specification so that the IOPS/GB vol parameter can be sync'd w/ reality. Is that correct?

If so, I'd suggest looking for other solutions to this first. For example, if IOPS/GB is not guaranteed to be stable w/ respect to the lifetime of the volume, perhaps it should not be modeled as a CSI volume parameter?

Of course, if I'm completely off base w/ my understanding, can you please provide a walkthrough of how a particular problem manifests?

On Thu, May 19, 2022 at 12:54 AM Mike Elmsly @.***> wrote:

Could I argue that some cases of this are to align with user expectations based on the way IOPS are specified For example: AWS based storage class specifies IOPS per GB, but the underlying API uses a raw IOPS figure. However - when a PV is expanded to a larger size, the driver does not recalculate the IOPs - so the size is increased, but the IOPS stays the same as what was calculated in the original create.

While changing things like the volume type should go to KEP - I'd suggest the case I've outlined is a bug

— Reply to this email directly, view it on GitHub https://github.com/container-storage-interface/spec/issues/491#issuecomment-1131200265, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAR5KLBIUPIHIGPZWHMXKTLVKXCPDANCNFSM5FP5KSNQ . You are receiving this because you were mentioned.Message ID: @.***>

-- James DeFelice 585.241.9488 (voice) 650.649.6071 (fax)

[mikee]

Thanks @jdef -I chimed in here after following a discussion on the AWS driver issue for this which pointed to the spec, but re reading the docs I think this is an implementation bug rather than a spec bug. Specifically because iopsPerGB is AWS EBS io1 only apparently.

Regardless, specific steps to illustrate this issue

1. Create a storage class with a set iopspergb

   apiVersion: storage.k8s.io/v1
   kind: StorageClass
   metadata:
   name: io1-50ipg-sc
   provisioner: kubernetes.io/aws-ebs
   parameters:
   type: io1
   iopsPerGB: "50"
   fsType: ext4

2. create a pvc for it

   apiVersion: v1
   kind: PersistentVolumeClaim
   metadata:
   name: testpvc
   spec:
   accessModes:
   - ReadWriteOnce
   storageClassName: io1-50ipg-sc
   resources:
   requests:
     storage: 30Gi

3. check the underlying AWS EBS Volume. You'll note that the ebs provisioner has multiplied the iopspergb by the size of the volume to set the iops. So 30*50=1500 IOPS

4. Patch/modify the pvc to expand the volume

   spec:
   resources:
   requests:
     storage: 50Gi

5. when resize complete - check the underlying AWS EBS volume - you would expect that the resize has used iopsPerGB * new volume size to update the IOPS, but it hasn't. The iops calculated in create is still there.

I'd argue that given the name and declarative approach of kube api objects that this should have changed. Obviously admins can manually go in afterwards and fix this up, but it's not intuitive that the underlying CSI implementation wouldn't do this.

[rdpsin]

Hi, @mikee, I agree that it's an implementation bug rather than a spec bug.

The issue is that during volume expand, the EBS CSI driver has no information about the volume or it's StorageClass parameters. There may workarounds for this. Specifically, for ioperPerGb, we can do things like store it in volume tags or compute them from current volume properties. A better way would be to allow external-resizer to pass in the StorageClass.parameters during ExpandVolume.

P.S. I don't think this is the right place to discuss EBS CSI-related stuff. :) Happy to discuss this more over at https://github.com/kubernetes-sigs/aws-ebs-csi-driver (Also see related issue https://github.com/kubernetes-sigs/aws-ebs-csi-driver/issues/818)

[mikee]

Hi, @mikee, I agree that it's an implementation bug rather than a spec bug.

The issue is that during volume expand, the EBS CSI driver has no information about the volume or it's StorageClass parameters. There may workarounds for this. Specifically, for ioperPerGb, we can do things like store it in volume tags or compute them from current volume properties. A better way would be to allow external-resizer to pass in the StorageClass.parameters during ExpandVolume.

P.S. I don't think this is the right place to discuss EBS CSI-related stuff. :) Happy to discuss this more over at https://github.com/kubernetes-sigs/aws-ebs-csi-driver (Also see related issue kubernetes-sigs/aws-ebs-csi-driver#818)

agreed sorry - will take the discussion there

[DerekTBrown]

Of course, if I'm completely off base w/ my understanding, can you please provide a walkthrough of how a particular problem manifests?

We have a slightly different need/usecase for ModifyVolume than the ones described above:

• The service owner configured particular PIOPS/Throughput/Storage Class settings at some time in the past.
• The service resource needs have changed over time, and we need to change PIOPS/Throughput/Storage class to minimize cost.
• Because the StorageClass resource is immutable, we are unable to change these parameters.
• As far as I can tell, our only option is to take down the service, create a new StorageClass with the updated parameters, and manually copy data from the old volumes to the new volumes.

Ultimately, I think this is an API modeling problem. AWS has the capability to change these volume parameters on-the-fly, but they are blocked from integrating this behavior with Kubernetes because StorageClass is immutable and PersistentVolume lacks a modification interface.

Maybe start with a KEP in sig-storage?

Did we end up starting a KEP?