Storage size requests lower than 1GiB are not respected

[johanfleury]

Bug Report

Description

When creating a PV with spec.resources.requests.storage lower than 1GiB, a volume of 1GiB is created.

My understanding is that calls to RoundUpSize in CreateVolume and ExpandVolume are forcing disk size to be a 1GiB multiple (i.e. 100Mi → 1Gi, 1.5Gi → 2Gi):

proxmox-csi-plugin/pkg/csi/controller.go

Line 126 in 76c899e

volSizeGB := int(util.RoundUpSize(volSizeBytes, 1024*1024*1024))

proxmox-csi-plugin/pkg/csi/controller.go

Line 126 in 76c899e

volSizeGB := int(util.RoundUpSize(volSizeBytes, 1024*1024*1024))

I tested on my cluster (PVE 8.2) and creating a disk of 100Mi is working well, so I feel like this rounding up is not necessary.

Logs

Controller:

I0715 15:25:29.113188       1 controller.go:87] "CreateVolume: called" args="{\"accessibility_requirements\":{\"preferred\":[{\"segments\":{\"topology.kubernetes.io/region\":\"virt01\",\"topology.kubernetes.io/zo
ne\":\"virt01\"}}],\"requisite\":[{\"segments\":{\"topology.kubernetes.io/region\":\"virt01\",\"topology.kubernetes.io/zone\":\"virt01\"}}]},\"capacity_range\":{\"required_bytes\":104857600},\"name\":\"pvc-2be4ec
b2-09d0-4fe2-bd03-5e92a4700b31\",\"parameters\":{\"cache\":\"none\",\"ssd\":\"true\",\"storage\":\"ssd\"},\"volume_capabilities\":[{\"AccessType\":{\"Mount\":{\"fs_type\":\"ext4\"}},\"access_mode\":{\"mode\":7}}]
}"
I0715 15:25:29.130310       1 controller.go:166] "CreateVolume" storageConfig={"content":"rootdir,images","digest":"893d05dfb422ac491ca99bc3fea71b4298e04a26","storage":"ssd","thinpool":"ssd","type":"lvmthin","vgn
ame":"pve"}
I0715 15:25:29.607009       1 controller.go:218] "CreateVolume: volume created" cluster="virt01" volumeID="virt01/virt01/ssd/vm-9999-pvc-2be4ecb2-09d0-4fe2-bd03-5e92a4700b31" size=1

Node: not relevant

Environment

• Plugin version:

  I0715 15:21:07.496862       1 main.go:52] Driver version 0.4.0, GitVersion v0.7.0, GitCommit 9424c06
  I0715 15:21:07.496938       1 main.go:53] Driver CSI Spec version: 1.9.0
  

• Kubernetes version: v1.29.6
• CSI capasity:

  CLASS   AVAIL          ZONE
  ssd     173591840687   map[topology.kubernetes.io/region:virt01 topology.kubernetes.io/zone:virt01]
  

• CSI resource on the node: not relevant
• Node describe: not relevant
• OS version: not relevant

[sergelogvinov]

Hi, thank you for bug report. It is definitely a bug.
The idea was to have the same limitation as big clouds have. And 1Gi usually enough. I do not think that proxmox has minimal size limitation (i did not check)

Thanks.

[johanfleury]

It was not clear in my message, but I tested adding a 100MB disk to a VM from the UI and it worked well. I don’t think there’s a minimum size requirement and the API allows you to set size in bytes.

[vehagn]

@johanfleury @sergelogvinov I think i has something to do with this line

proxmox-csi-plugin/pkg/csi/utils.go

Line 216 in c5769c1

"size": fmt.Sprintf("%dG", sizeGB),

which indicates that the size is rounded off to the nearest GB.

[sergelogvinov]

I've run test and get rounded size 100Mi -> 1Gi, so util.RoundUpSize works as expected.

spec:
  accessModes:
  - ReadWriteOnce
  resources:
    requests:
      storage: 100Mi
  storageClassName: proxmox-xfs
  volumeMode: Filesystem
  volumeName: pvc-14edca40-2d3c-4cb8-bd05-6db5b18350b2
status:
  accessModes:
  - ReadWriteOnce
  capacity:
    storage: 1Gi
  phase: Bound

Yep you can resize disk thought UI.
Proxmox 7.4 has 1Gb minimum chunk, Proxmox 8.2 has 1Gb minimum chunk too.

Round size is only for "to have the same limitations as well-known cloud have".
I do not think that Proxmox has minimum size limit (only lvm, zfs, etc).
And 1Gi minimum size doesn't required extra checks of file system backend.

[vehagn]

Is it rounded to 1 Gibibyte or 1 Gigabyte?

[sergelogvinov]

Is it rounded to 1 Gibibyte or 1 Gigabyte?

it should be Gibibytes (power of two)

// RoundUpSize calculates how many allocation units are needed to accommodate
// a volume of given size. E.g. when user wants 1500MiB volume, while AWS EBS
// allocates volumes in gibibyte-sized chunks,
// RoundUpSize(1500 * 1024*1024, 1024*1024*1024) returns '2'
// (2 GiB is the smallest allocatable volume that can hold 1500MiB)
func RoundUpSize(volumeSizeBytes int64, allocationUnitBytes int64) int64 {
	roundedUp := volumeSizeBytes / allocationUnitBytes
	if volumeSizeBytes%allocationUnitBytes > 0 {
		roundedUp++
	}
	return roundedUp
}

[johanfleury]

I do not think that Proxmox has minimum size limit (only lvm, zfs, etc).
And 1Gi minimum size doesn't required extra checks of file system backend.

I’m not sure I understand this. As far as I know, there are no storage backends in Proxmox that have a 1GB lower limit on the disk size.

I know for a fact that ISCSI, LVM, ZFS and qcow files (local or remote) all support less-than-1GiB disks. I’m not entirely sure about CEPH and I couldn’t find an answer online.

Yep you can resize disk thought UI.
Proxmox 7.4 has 1Gb minimum chunk, Proxmox 8.2 has 1Gb minimum chunk too.

From the UI maybe (probably for UX reasons), but from the API there’s no such limitation.

[sergelogvinov]

I'm afraid I may have lost track of the main idea here.

If the required size is less than 1Gb - I believe a good minimum would be 100Mb.
Please let me know if there's an issue, as it should be quite easy to fix.

[johanfleury]

Please let me know if there's an issue, as it should be quite easy to fix.

My issue is that a PVC with spec.capacity.storage: 150Mi will create a volume of 1GB in capacity.

If the required size is less than 1Gb - I believe a good minimum would be 100Mb.

100MB seems reasonable to me.

If you’re ok with lowering RoundUpSize to 100MB, I can make a PR.

[sergelogvinov]

My issue is that a PVC with spec.capacity.storage: 150Mi will create a volume of 1GB in capacity.

common behavior most of the clouds...

Yeah, make sense to change it to 100M.

[johanfleury]

I did some tests with RoundUpSize, because I have a hard time wrapping my head around what it does. Here are some result for 100 * util.RoundUpSize(s, 100*1024*1024) (i.e. trying to round at the upper 100MiB):

req: 10 / alloc: 100MiB
req: 10M / alloc: 100MiB
req: 10Mi / alloc: 100MiB
req: 100M / alloc: 100MiB
req: 100Mi / alloc: 100MiB
req: 1024M / alloc: 1000MiB
req: 1024Mi / alloc: 1100MiB
req: 1G / alloc: 1000MiB
req: 1Gi / alloc: 1100MiB
req: 2G / alloc: 2000MiB
req: 2Gi / alloc: 2100MiB

I don’t think this is what we want, so either we remove RoundUpSize entirely (I made #266) for this, or we find another solution.

[sergelogvinov]

These are very interesting results. I hadn't checked them before, so thank you for sharing. I've reviewed #266, and I was thinking, what do you think about moving the RoundUpSize function to our utilities and fixing everything there?

Also, we should check the minimum size, at least. Someone could accidentally set the size to '10' (bytes), and Proxmox might return an error.

Thank you for the effort.