Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Vsphere rouding up of volume size #76719

Merged
merged 2 commits into from Apr 24, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions pkg/volume/vsphere_volume/vsphere_volume_util.go
Expand Up @@ -94,11 +94,12 @@ func (util *VsphereDiskUtil) CreateVolume(v *vsphereVolumeProvisioner, selectedZ
}

capacity := v.options.PVC.Spec.Resources.Requests[v1.ResourceName(v1.ResourceStorage)]
// vSphere works with kilobytes, convert to KiB with rounding up
volSizeKiB, err := volumehelpers.RoundUpToKiBInt(capacity)
// vSphere works with KiB, but its minimum allocation unit is 1 MiB
volSizeMiB, err := volumehelpers.RoundUpToMiBInt(capacity)
if err != nil {
return nil, err
}
volSizeKiB := volSizeMiB * 1024
name := volumeutil.GenerateVolumeName(v.options.ClusterName, v.options.PVName, 255)
volumeOptions := &vclib.VolumeOptions{
CapacityKB: volSizeKiB,
Expand Down
63 changes: 32 additions & 31 deletions test/e2e/storage/vsphere/vsphere_volume_disksize.go
Expand Up @@ -17,13 +17,12 @@ limitations under the License.
package vsphere

import (
"fmt"
"strings"
"time"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
"k8s.io/api/core/v1"
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
clientset "k8s.io/client-go/kubernetes"
"k8s.io/kubernetes/test/e2e/framework"
Expand All @@ -35,13 +34,12 @@ const (
)

/*
Test to verify disk size specified in PVC is being honored while volume creation.
Test to verify disk size specified in PVC is being rounded up correctly.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we don't run these tests at all, however, I updated this one to make sure it works.


Steps
1. Create StorageClass.
2. Create PVC with invalid disk size which uses the StorageClass created in step 1.
3. Expect the PVC to fail.
4. Verify the error returned on PVC failure is the correct.
3. Verify the provisioned PV size is correct.
*/

var _ = utils.SIGDescribe("Volume Disk Size [Feature:vsphere]", func() {
Expand All @@ -61,35 +59,38 @@ var _ = utils.SIGDescribe("Volume Disk Size [Feature:vsphere]", func() {
datastore = GetAndExpectStringEnvVar(StorageClassDatastoreName)
})

It("verify dynamically provisioned pv using storageclass with an invalid disk size fails", func() {
By("Invoking Test for invalid disk size")
It("verify dynamically provisioned pv has size rounded up correctly", func() {
By("Invoking Test disk size")
scParameters[Datastore] = datastore
scParameters[DiskFormat] = ThinDisk
diskSize := "1"
err := invokeInvalidDiskSizeTestNeg(client, namespace, scParameters, diskSize)
Expect(err).To(HaveOccurred())
errorMsg := `Failed to provision volume with StorageClass \"` + DiskSizeSCName + `\": A specified parameter was not correct`
if !strings.Contains(err.Error(), errorMsg) {
framework.ExpectNoError(err, errorMsg)
}
})
})
expectedDiskSize := "1Mi"

By("Creating Storage Class")
storageclass, err := client.StorageV1().StorageClasses().Create(getVSphereStorageClassSpec(DiskSizeSCName, scParameters, nil))
framework.ExpectNoError(err)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error validation in this package is done in many different ways, example:

Expect(err).To(HaveOccurred())
framework.ExpectNoError(err, fmt.Sprintf("Failed to create storage class with err: %v", err))
framework.ExpectNoError(err)

For consistency's sake, in this patch I only used framework.ExpectNoError(err) .

defer client.StorageV1().StorageClasses().Delete(storageclass.Name, nil)

func invokeInvalidDiskSizeTestNeg(client clientset.Interface, namespace string, scParameters map[string]string, diskSize string) error {
By("Creating Storage Class With invalid disk size")
storageclass, err := client.StorageV1().StorageClasses().Create(getVSphereStorageClassSpec(DiskSizeSCName, scParameters, nil))
framework.ExpectNoError(err, fmt.Sprintf("Failed to create storage class with err: %v", err))
defer client.StorageV1().StorageClasses().Delete(storageclass.Name, nil)
By("Creating PVC using the Storage Class")
pvclaim, err := framework.CreatePVC(client, namespace, getVSphereClaimSpecWithStorageClass(namespace, diskSize, storageclass))
framework.ExpectNoError(err)
defer framework.DeletePersistentVolumeClaim(client, pvclaim.Name, namespace)

By("Creating PVC using the Storage Class")
pvclaim, err := framework.CreatePVC(client, namespace, getVSphereClaimSpecWithStorageClass(namespace, diskSize, storageclass))
framework.ExpectNoError(err)
defer framework.DeletePersistentVolumeClaim(client, pvclaim.Name, namespace)
By("Waiting for claim to be in bound phase")
err = framework.WaitForPersistentVolumeClaimPhase(v1.ClaimBound, client, pvclaim.Namespace, pvclaim.Name, framework.Poll, 2*time.Minute)
framework.ExpectNoError(err)

By("Expect claim to fail provisioning volume")
err = framework.WaitForPersistentVolumeClaimPhase(v1.ClaimBound, client, pvclaim.Namespace, pvclaim.Name, framework.Poll, 2*time.Minute)
Expect(err).To(HaveOccurred())
By("Getting new copy of PVC")
pvclaim, err = client.CoreV1().PersistentVolumeClaims(pvclaim.Namespace).Get(pvclaim.Name, metav1.GetOptions{})
framework.ExpectNoError(err)

eventList, err := client.CoreV1().Events(pvclaim.Namespace).List(metav1.ListOptions{})
return fmt.Errorf("Failure message: %+q", eventList.Items[0].Message)
}
By("Getting PV created")
pv, err := client.CoreV1().PersistentVolumes().Get(pvclaim.Spec.VolumeName, metav1.GetOptions{})
framework.ExpectNoError(err)

By("Verifying if provisioned PV has the correct size")
expectedCapacity := resource.MustParse(expectedDiskSize)
pvCapacity := pv.Spec.Capacity[v1.ResourceName(v1.ResourceStorage)]
Expect(pvCapacity.Value()).To(Equal(expectedCapacity.Value()))
})
})