Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Proposal: Add a ModifyVolume RPC to the Controller Service #491

Open
vdhanan opened this issue Oct 6, 2021 · 12 comments
Open

Proposal: Add a ModifyVolume RPC to the Controller Service #491

vdhanan opened this issue Oct 6, 2021 · 12 comments

Comments

@vdhanan
Copy link

vdhanan commented Oct 6, 2021

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
Copy link
Member

jdef commented Oct 6, 2021 via email

@vdhanan
Copy link
Author

vdhanan commented Nov 19, 2021

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
Copy link
Member

jdef commented Nov 19, 2021 via email

@gnufied
Copy link
Contributor

gnufied commented Nov 19, 2021

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
Copy link

dhilipkumars commented Dec 21, 2021 •
edited
Loading

+1 we have similar requirement for azure ultradisk here

@jdef
Copy link
Member

jdef commented Dec 21, 2021 via email

@wongma7 wongma7 mentioned this issue Jan 5, 2022
Closed
@mikee
Copy link

mikee commented May 19, 2022

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
Copy link
Member

jdef commented May 19, 2022 via email

@mikee
Copy link

mikee commented May 19, 2022 •
edited
Loading

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
  1. create a pvc for it
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  name: testpvc
spec:
  accessModes:
    - ReadWriteOnce
  storageClassName: io1-50ipg-sc
  resources:
    requests:
      storage: 30Gi
  1. 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
  2. Patch/modify the pvc to expand the volume
spec:
  resources:
    requests:
      storage: 50Gi
  1. 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
Copy link

rdpsin commented May 19, 2022

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)

@mikee
Copy link

mikee commented May 19, 2022

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

@rdpsin rdpsin mentioned this issue Aug 10, 2022
Closed
@DerekTBrown
Copy link

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@gnufied @mikee @jdef @DerekTBrown @dhilipkumars @vdhanan @rdpsin