io1 volumes with <100 iops fail to provision

[kcking]

/kind bug

What happened?
provisioning io1 volume with < 100 IOPS fails

What you expected to happen?
provision the volume with a min of 100 IOPS instead of failing, as this was the behavior of the in-tree EBS plugin

How to reproduce it (as minimally and precisely as possible)?
provision an io1 volume with < 100 IOPS

Anything else we need to know?:
Maybe this is intended behavior, but was surprising to me as we were migrating our infra over from in-tree to this driver

Environment

• Kubernetes version (use kubectl version): 1.17
• Driver version: 0.6

[wongma7]

I think it is better to throw an error than silently accept and correct an invalid input.

Can you share your StorageClass? Do you trigger this situation by setting a low iopsPerGB value in it? Since even with CSI migration enabled there is no guarantee of compatibility between in-tree and CSI StorageClasses, I don't think we are violating compatibility by tweaking the validation around iopsPerGB.

Open to discussion.

[msau42]

I agree that it's better to error than silently round up, so that users are not surprised when they get their bill.

For CSI migration, you could preserve in-tree behavior by rounding up when you translate the in-memory StorageClass parameters.

[ayberk]

I, too, concur that we should keep this behavior, but at the same time we shouldn't surprise users migrating from the in-tree plugin. Maybe it's worth having a section in the README to note differences for migration.

[kcking]

Totally hear the point of a surprise bill. Yes the storage class in question had a very low IOPS/GB and was a 16GB volume because it was a testing environment.

I'm not using storage class migration because AFAICT it is not enabled in k8s 1.17 (the current latest for EKS). Instead I had my k8s database operator (foundationdb) migrate the cluster to the new CSI storage class, but ran into errors because the CSI driver could not bring up any io1 volumes with that configuration. I had to manually intervene and fix the storage class (I just changed it to gp2).

Perhaps this edge case is a rare one, but documentation regarding the io1 storage class difference between the in-tree plugin and the csi driver might have saved me from this suprise :)

[wongma7]

OK, I am fine with changing the behavior to be like in-tree to avoid situations like that. I do want to at least change the "silent" part of it...we definitely need to document what happens if iopsPerGB * requestedGB is less than minimum, and a log message won't hurt, something like "Setting iops to minimum 100 because x would be too low"

[msau42]

I think folks in this thread were actually ok with the opposite, ie error, but add more documentation in the repo that this is different than intree behavior.

And in terms of csi migration, we can retain the old behavior in the translation layer.

[wongma7]

Got it, opened #582 to fix the doc, yes let's have translation layer retain old behavior and spit out some log message .