volume size mismatch between driver and side car containers

[Madhu-1]

Hi we are facing an issue regarding volume size miss match between kubernetes and CSI driver

if I create a PVC of 1.5Gib, external-provisioner is sending the request to CSI driver in bytes, so the CSI driver will create a volume of size 1.5Gib but I think in sidecar container we are round up the value to 2Gib when creating the PV object.

external-provisioner/pkg/controller/controller.go

Line 636 in d0e4880

v1.ResourceName(v1.ResourceStorage): bytesToGiQuantity(respCap),

is the line which does the round-off.

here is the sample snippet which round of the 1.5Gi to 2Gi https://play.golang.org/p/QZMKmIB5VjF

[Madhu-1]

/CC @msau42

[Madhu-1]

@jsafrane @msau42 @davidz627 any idea how to fix this one. do we need to round off the value before sending the request to the driver?

[humblec]

@Madhu-1 afaict, this is working as expected/designed. Controller sent the request in bytes and in kube, the expectation is that, the user get atleast requested space , if its less its a failure. >= is expected. In this case user got what he requested. afaict, the round up is done deliberately. The reason is also kind of documented.

// Need to give Quantity nice whole numbers or else it
// sometimes spits out the value in milibytes. We round up.

[k8s-ci-robot]

@humblec: GitHub didn't allow me to assign the following users: humblec.

Note that only kubernetes-csi members and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @humblec

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[Madhu-1]

@humblec if the storage provider doesn't do roundoff before creating the volume, then they will see volume size mismatch between Kube and storage backend.

[humblec]

if I create a PVC of 1.5Gib, external-provisioner is sending the request to CSI driver in bytes, so the CSI driver will create a volume of size 1.5Gb

@Madhu-1 this is wrong! That said, if the request from user is 1.5Gib , storage provider should create NOT create 1.5Gb. 1.5G is < 1.5Gi which is a failure as per kube norms..
G and Gi are different. Storage provider should create 1.5Gib or above as mentioned in last comment.

[Madhu-1]

@humblec that was a typo storage provider is creating 1.5Gib

[saad-ali]

Either @davidz627 or @verult did bug fixes in this area. One of them may be able to fetch more light on what's going on.

[davidz627]

I'm working off memory here so feel free to fact check and correct me if I'm wrong but I believe we do the rounding in the PV for readability reasons. As @humblec mentioned earlier its sort of documented here:

external-provisioner/pkg/controller/controller.go

Lines 941 to 942 in cbad731

	// Need to give Quantity nice whole numbers or else it
	// sometimes spits out the value in milibytes. We round up.

The reason for this is documented in the quantity.go file which is the library we use for dealing with volume sizes:
https://github.com/kubernetes/kubernetes/blob/2981fb7a0171798184b4f860b07870de0ff7b234/staging/src/k8s.io/apimachinery/pkg/api/resource/quantity.go#L75

Thinking about it more now it might make more sense to round down instead of up. Since the PV represents the underlying volume, it is better for the PV say that 1Gi is available when it is actually 1.5Gi than saying 2Gi is available when it is actually 1.5Gi. This way when a PVC requests a certain amount of space it will always get how much space it requested or more instead of getting how much space it requested or less.
@saad-ali WDYT

Although I also seem to recall thinking through this very carefully at the time and deciding on rounding up after debate. Don't remember why currently now though. I'll try to dig up that issue
Edit: found it but this was back when I was less diligent about documenting changes so its not very helpful :(
#78

[saad-ali]

Thinking about it more now it might make more sense to round down instead of up. Since the PV represents the underlying volume, it is better for the PV say that 1Gi is available when it is actually 1.5Gi than saying 2Gi is available when it is actually 1.5Gi. This way when a PVC requests a certain amount of space it will always get how much space it requested or more instead of getting how much space it requested or less.
@saad-ali WDYT

That seems reasonable to me.

Although I also seem to recall thinking through this very carefully at the time and deciding on rounding up after debate. Don't remember why currently now though.

I can't recall the details either. I remember there was a fix made and then a follow up changing that.

[msau42]

There was a fix made to in-tree GCE PD to use GB instead of Gi, but it was reverted because the G in pd's definition actually means Gi.

[gnufied]

This really depends on storage provider isn't it? afaik - GCE provisions in multiples of GB (we went back and forth on this) and hence if user asks for 2Gi then it will provision 3GB.

The docs in the comment says:

Need to give Quantity nice whole numbers or else it sometimes spits out the value in milibytes. We round up.

I don't remember the history behind it but we should be able to round up to 2 decimal places and we should still be good. cc @jsafrane @wongma7

[wongma7]

IMO, forget all rounding and use resource.NewQuantity on the returned volume.CapacityBytes which will handle it for you:

quantity := resource.NewQuantity(1610612736, resource.BinarySI)
fmt.Println(quantity.String())

prints out:
1536Mi

The problem with calling resource.MustParse is that "When a Quantity is parsed from a string, it will remember the type of suffix it had, and will use the same type again when it is serialized."https://github.com/kubernetes/kubernetes/blob/2981fb7a0171798184b4f860b07870de0ff7b234/staging/src/k8s.io/apimachinery/pkg/api/resource/quantity.go#L56. If you use resource.NewQuantity instead, you will get the nice as-readable-as-possible behaviour above, detailed here: https://github.com/kubernetes/kubernetes/blob/2981fb7a0171798184b4f860b07870de0ff7b234/staging/src/k8s.io/apimachinery/pkg/api/resource/quantity.go#L59

[gnufied]

Yeah but the problem I think is - we want size of PV to be reported in "nicer" formats. So arguably - the code here wants to avoid reporting large Milibyte numbers (like 1536Mi for example). It looks like we have optimized for user friendly formatting vs actual volume size.

[wongma7]

(1536Mi is mebibytes not millibytes) well frankly I think that is silly...4 digits is not too much for a user to handle. If the number is something hard to parse (i.e. not x1000^y or x1024^y) then either a) the user requested it and is getting exactly that back or b) it is > than the user's request & the csi createvolume arrived at the number on purpose somehow, so it's the csi driver's problem

as I said I think this is simply a case of needing to use resource.NewQuantity instead of resource.MustParse. i see @davidz627 already linked to quantity.go, the line there says it will serialize things in canonical form, the caveat I quoted is that it will only do that if you use NewQuantity, not MustParse

[davidz627]

@wongma7 I'm not opposed to the change. 4 digits seem fine but I want to avoid having something like 10032873429b

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[msau42]

/lifecycle frozen

We should come to a conclusion on what we want to do here wrt rounding

[yuansmin]

I think correctness is more important than "nicer formats". we can't get the real size now, and it's important. And also, a "nicer formats" is just the problem of Print, not the data we stored.

[Jiawei0227]

I think correctness is more important than "nicer formats". we can't get the real size now, and it's important. And also, a "nicer formats" is just the problem of Print, not the data we stored.

+1 to this. From what I understand, the exact size is more important than a nicer format. What matters here is what the storage provider returns in the CreateVolumeResponse call as that is what the provisioner thinks the real storage underlying, so it is the storage provider's responsibility to do round up/round off to the request volume size and return the correct underlying size:

external-provisioner/pkg/controller/controller.go

Line 769 in 41af8a3

respCap := rep.GetVolume().GetCapacityBytes()

For example, if user asks for 1.345Gi size of volume, in GCE PD CSI driver it will round up to 2Gi and create a 2Gi disk underlying and return the 2Gi PV. But there might be some other storage provider like NFS driver it does not return size or returns the exact 1444182754(1.345Gi) bytes, then we should also honor this size and set it for the PV as this is the underlying storage the driver provided.

In conclusion, I think resource.NewQuantity() makes sense here as it tries its best to give a nice format but also keep the precision.
For example,

1. 6,442,450,944(6 * 1024 * 1024 * 1024) will be parsed to 6Gi.
2. 1,610,612,736(1.5 * 1024 * 1024 * 1024) will be parsed to 1536Mi.
3. 5970004541(5.56 * 1024 * 1024 * 1024) will be parsed to 5970004541.

In the above cases, the third one is less readable. But the point here is that it does not loss precision. So if actually the CSI Driver returns 5970004541 as the volume size, provisioner should honor it and pass it to the PV. In terms of nicer format, my gut is telling me that is something we need to do in the quantity package. For example, a ReadableString() can parse the third case I mentioned above to 5.56Gi, but it is just a printout thing anyway not the actual data it is storing.

[Madhu-1]

Fixed by #541