Storage size requests lower than 1GiB are not respected

johanfleury commented 1 month ago

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): https://github.com/sergelogvinov/proxmox-csi-plugin/blob/76c899e4e1c8f2287854a4e08fbbdd8a1c7360a9/pkg/csi/controller.go#L126 https://github.com/sergelogvinov/proxmox-csi-plugin/blob/76c899e4e1c8f2287854a4e08fbbdd8a1c7360a9/pkg/csi/controller.go#L126

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

https://github.com/sergelogvinov/proxmox-csi-plugin/blob/c5769c1831027a6474d3a18d28b9e4add1e02814/pkg/csi/utils.go#L216

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

sergelogvinov commented 1 month ago

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 commented 1 month ago

Is it rounded to 1 Gibibyte or 1 Gigabyte?

sergelogvinov commented 1 month ago

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
}

sergelogvinov / proxmox-csi-plugin