From e7ae2b6abd7bf7859893849b9678dafa5c535f68 Mon Sep 17 00:00:00 2001
From: Noah Watkins <noahwatkins@gmail.com>
Date: Fri, 5 Jul 2019 14:52:29 -0700
Subject: [PATCH] ceph: adjust M-to-Mi rounding in PV creation

Example of failure scenarios:

   - requested PVC 500M = 500,000,000 bytes
   - PV is created as:
        (500M / 2**20)Mi = "476"Mi
        But 476Mi = 476 * 2**20 = 499122176 < 500M

From the k8s documentation:

   Claims will remain unbound indefinitely if a matching volume does not
   exist. Claims will be bound as matching volumes become available. For
   example, a cluster provisioned with many 50Gi PVs would not match a
   PVC requesting 100Gi. The PVC can be bound when a 100Gi PV is added
   to the cluster.

which is inline with the behavior observed in which a 500M PVC is never
bound. However, the documentation also states:

   If a PV was dynamically provisioned for a new PVC, the loop will
   always bind that PV to the PVC. Otherwise, the user will always get
   at least what they asked for

which suggests that an _explicit_ binding can be created, and perhaps
something has changed with respect to this binding that has caused the
failures in newer versions of k8s.

fixes: #3391
fixes: #2922

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
---
 pkg/daemon/ceph/client/image.go              | 14 +++++++++++++-
 pkg/operator/ceph/provisioner/provisioner.go | 16 ++++++++++++++--
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/pkg/daemon/ceph/client/image.go b/pkg/daemon/ceph/client/image.go
index 9c857dfdb..1186fbcef 100644
--- a/pkg/daemon/ceph/client/image.go
+++ b/pkg/daemon/ceph/client/image.go
@@ -25,6 +25,7 @@ import (
 	"regexp"
 
 	"github.com/rook/rook/pkg/clusterd"
+	"github.com/rook/rook/pkg/util/display"
 	"github.com/rook/rook/pkg/util/exec"
 )
 
@@ -68,6 +69,9 @@ func ListImages(context *clusterd.Context, clusterName, poolName string) ([]Ceph
 
 // CreateImage creates a block storage image.
 // If dataPoolName is not empty, the image will use poolName as the metadata pool and the dataPoolname for data.
+// If size is zero an empty image will be created. Otherwise, an image will be
+// created with a size rounded up to the nearest Mi. The adjusted image size is
+// placed in return value CephBlockImage.Size.
 func CreateImage(context *clusterd.Context, clusterName, name, poolName, dataPoolName string, size uint64) (*CephBlockImage, error) {
 	if size > 0 && size < ImageMinSize {
 		// rbd tool uses MB as the smallest unit for size input.  0 is OK but anything else smaller
@@ -100,7 +104,15 @@ func CreateImage(context *clusterd.Context, clusterName, name, poolName, dataPoo
 		}
 	}
 
-	return &CephBlockImage{Name: name, Size: size}, nil
+	// report the adjusted size which will always be >= to the requested size
+	var newSizeBytes uint64
+	if sizeMB > 0 {
+		newSizeBytes = display.MbTob(uint64(sizeMB))
+	} else {
+		newSizeBytes = 0
+	}
+
+	return &CephBlockImage{Name: name, Size: newSizeBytes}, nil
 }
 
 func DeleteImage(context *clusterd.Context, clusterName, name, poolName string) error {
diff --git a/pkg/operator/ceph/provisioner/provisioner.go b/pkg/operator/ceph/provisioner/provisioner.go
index 49d9dd33e..fc4211f63 100644
--- a/pkg/operator/ceph/provisioner/provisioner.go
+++ b/pkg/operator/ceph/provisioner/provisioner.go
@@ -100,8 +100,20 @@ func (p *RookVolumeProvisioner) Provision(options controller.VolumeOptions) (*v1
 		return nil, err
 	}
 
-	// since we can guarantee the size of the volume image generated have to be in `MB` boundary, so we can
-	// convert it to `MB` unit safely here
+	// the size of the PV needs to be at least as large as the size in the PVC
+	// or binding won't be successful. createVolume uses the requestBytes
+	// parameter as a target, and guarantees that the size created as at least
+	// that large. the adjusted value is placed in blockImage.Size and it is
+	// suitable to be converted into Mi.
+	//
+	// note that the rounding error that can occur if the original non-adjusted
+	// request is used in the original formulation here:
+	//
+	//    s := fmt.Sprintf("%dMi", blockImage.Size/sizeMB)
+	//    Size = 500M = 500,000,000 bytes
+	//    500M / 2**20 = 476
+	//    476Mi = 476 * 2**20 = 499122176 < 500M
+	//
 	s := fmt.Sprintf("%dMi", blockImage.Size/sizeMB)
 	quantity, err := resource.ParseQuantity(s)
 	if err != nil {