From c76e342bb9cfd8c1fac6390110473a4083c29791 Mon Sep 17 00:00:00 2001 From: Madhu Rajanna Date: Mon, 1 Jul 2019 18:22:39 +0530 Subject: [PATCH] fix storageclass comparion for pvc datasource compare storage class string, instead of comparing the string address. Signed-off-by: Madhu Rajanna --- pkg/controller/controller.go | 13 ++- pkg/controller/controller_test.go | 169 +++++++++++++++++++++++++++--- 2 files changed, 165 insertions(+), 17 deletions(-) diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index c4d63683f7..ce3931fe55 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -680,9 +680,18 @@ func (p *csiProvisioner) getPVCSource(options controller.ProvisionOptions) (*csi if sourcePVC.ObjectMeta.DeletionTimestamp != nil { return nil, fmt.Errorf("the PVC DataSource %s is currently being deleted", options.PVC.Spec.DataSource.Name) } - if sourcePVC.Spec.StorageClassName != options.PVC.Spec.StorageClassName { + + if sourcePVC.Spec.StorageClassName == nil { + return nil, fmt.Errorf("the source PVC (%s) storageclass cannot be empty", sourcePVC.Name) + } + + if options.PVC.Spec.StorageClassName == nil { + return nil, fmt.Errorf("the requested PVC (%s) storageclass cannot be empty", options.PVC.Name) + } + + if *sourcePVC.Spec.StorageClassName != *options.PVC.Spec.StorageClassName { return nil, fmt.Errorf("the source PVC and destination PVCs must be in the same storage class for cloning. Source is in %v, but new PVC is in %v", - sourcePVC.Spec.StorageClassName, options.PVC.Spec.StorageClassName) + *sourcePVC.Spec.StorageClassName, *options.PVC.Spec.StorageClassName) } capacity := options.PVC.Spec.Resources.Requests[v1.ResourceName(v1.ResourceStorage)] requestedSize := capacity.Value() diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index ea1ce764ad..705267c16d 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -18,6 +18,7 @@ package controller import ( "context" + "errors" "fmt" "io/ioutil" "os" @@ -2230,8 +2231,11 @@ func runDeleteTest(t *testing.T, k string, tc deleteTestcase) { // TestProvisionFromPVC tests create volume clone func TestProvisionFromPVC(t *testing.T) { var requestedBytes int64 = 1000 - invalidSCName := "invalid-sc" + fakeSc1 := "fake-sc-1" + fakeSc2 := "fake-sc-2" srcName := "fake-pvc" + invalidPVC := "invalid-pv" + pvName := "test-testi" deletePolicy := v1.PersistentVolumeReclaimDelete type pvSpec struct { @@ -2245,6 +2249,7 @@ func TestProvisionFromPVC(t *testing.T) { testcases := map[string]struct { volOpts controller.ProvisionOptions restoredVolSizeSmall bool + restoredVolSizeBig bool wrongDataSource bool pvcStatusReady bool expectedPVSpec *pvSpec @@ -2263,7 +2268,8 @@ func TestProvisionFromPVC(t *testing.T) { UID: "testid", }, Spec: v1.PersistentVolumeClaimSpec{ - Selector: nil, + StorageClassName: &fakeSc1, + Selector: nil, Resources: v1.ResourceRequirements{ Requests: v1.ResourceList{ v1.ResourceName(v1.ResourceStorage): resource.MustParse(strconv.FormatInt(requestedBytes, 10)), @@ -2308,7 +2314,8 @@ func TestProvisionFromPVC(t *testing.T) { UID: "testid", }, Spec: v1.PersistentVolumeClaimSpec{ - Selector: nil, + StorageClassName: &fakeSc1, + Selector: nil, Resources: v1.ResourceRequirements{ Requests: v1.ResourceList{ v1.ResourceName(v1.ResourceStorage): resource.MustParse(strconv.FormatInt(requestedBytes, 10)), @@ -2339,7 +2346,7 @@ func TestProvisionFromPVC(t *testing.T) { UID: "testid", }, Spec: v1.PersistentVolumeClaimSpec{ - StorageClassName: &invalidSCName, + StorageClassName: &fakeSc2, Selector: nil, Resources: v1.ResourceRequirements{ Requests: v1.ResourceList{ @@ -2371,11 +2378,11 @@ func TestProvisionFromPVC(t *testing.T) { UID: "testid", }, Spec: v1.PersistentVolumeClaimSpec{ - StorageClassName: &invalidSCName, + StorageClassName: &fakeSc1, Selector: nil, Resources: v1.ResourceRequirements{ Requests: v1.ResourceList{ - v1.ResourceName(v1.ResourceStorage): resource.MustParse(strconv.FormatInt(requestedBytes-1, 10)), + v1.ResourceName(v1.ResourceStorage): resource.MustParse(strconv.FormatInt(requestedBytes+1, 10)), }, }, AccessModes: []v1.PersistentVolumeAccessMode{v1.ReadWriteOnce}, @@ -2386,12 +2393,78 @@ func TestProvisionFromPVC(t *testing.T) { }, }, }, + pvcStatusReady: true, + restoredVolSizeSmall: true, + expectedPVSpec: nil, + cloneSupport: true, + expectErr: true, + }, + "provision with pvc data source destination too large": { + volOpts: controller.ProvisionOptions{ + StorageClass: &storagev1.StorageClass{ + ReclaimPolicy: &deletePolicy, + Parameters: map[string]string{}, + }, + PVName: pvName, + PVC: &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + UID: "testid", + }, + Spec: v1.PersistentVolumeClaimSpec{ + StorageClassName: &fakeSc1, + Selector: nil, + Resources: v1.ResourceRequirements{ + Requests: v1.ResourceList{ + v1.ResourceName(v1.ResourceStorage): resource.MustParse(strconv.FormatInt(requestedBytes+1, 10)), + }, + }, + AccessModes: []v1.PersistentVolumeAccessMode{v1.ReadWriteOnce}, + DataSource: &v1.TypedLocalObjectReference{ + Name: srcName, + Kind: "PersistentVolumeClaim", + }, + }, + }, + }, + pvcStatusReady: true, + restoredVolSizeBig: true, + expectedPVSpec: nil, + cloneSupport: true, + expectErr: true, + }, + "provision with pvc data source not found": { + volOpts: controller.ProvisionOptions{ + StorageClass: &storagev1.StorageClass{ + ReclaimPolicy: &deletePolicy, + Parameters: map[string]string{}, + }, + PVName: pvName, + PVC: &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + UID: "testid", + }, + Spec: v1.PersistentVolumeClaimSpec{ + StorageClassName: &fakeSc1, + Selector: nil, + Resources: v1.ResourceRequirements{ + Requests: v1.ResourceList{ + v1.ResourceName(v1.ResourceStorage): resource.MustParse(strconv.FormatInt(requestedBytes-1, 10)), + }, + }, + AccessModes: []v1.PersistentVolumeAccessMode{v1.ReadWriteOnce}, + DataSource: &v1.TypedLocalObjectReference{ + Name: "source-not-found", + Kind: "PersistentVolumeClaim", + }, + }, + }, + }, pvcStatusReady: true, expectedPVSpec: nil, cloneSupport: false, expectErr: true, }, - "provision with pvc data source not found": { + "provision with source pvc storageclass nil": { volOpts: controller.ProvisionOptions{ StorageClass: &storagev1.StorageClass{ ReclaimPolicy: &deletePolicy, @@ -2403,8 +2476,8 @@ func TestProvisionFromPVC(t *testing.T) { UID: "testid", }, Spec: v1.PersistentVolumeClaimSpec{ - StorageClassName: &invalidSCName, Selector: nil, + StorageClassName: &fakeSc1, Resources: v1.ResourceRequirements{ Requests: v1.ResourceList{ v1.ResourceName(v1.ResourceStorage): resource.MustParse(strconv.FormatInt(requestedBytes-1, 10)), @@ -2412,7 +2485,7 @@ func TestProvisionFromPVC(t *testing.T) { }, AccessModes: []v1.PersistentVolumeAccessMode{v1.ReadWriteOnce}, DataSource: &v1.TypedLocalObjectReference{ - Name: "source-not-found", + Name: "pvc-sc-nil", Kind: "PersistentVolumeClaim", }, }, @@ -2423,19 +2496,50 @@ func TestProvisionFromPVC(t *testing.T) { cloneSupport: false, expectErr: true, }, - "provision with pvc data source destination too large": { + "provision with requested pvc storageclass nil": { volOpts: controller.ProvisionOptions{ StorageClass: &storagev1.StorageClass{ ReclaimPolicy: &deletePolicy, Parameters: map[string]string{}, }, - PVName: "test-name", + PVName: pvName, + PVC: &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + UID: "testid", + }, + Spec: v1.PersistentVolumeClaimSpec{ + Selector: nil, + Resources: v1.ResourceRequirements{ + Requests: v1.ResourceList{ + v1.ResourceName(v1.ResourceStorage): resource.MustParse(strconv.FormatInt(requestedBytes-1, 10)), + }, + }, + AccessModes: []v1.PersistentVolumeAccessMode{v1.ReadWriteOnce}, + DataSource: &v1.TypedLocalObjectReference{ + Name: srcName, + Kind: "PersistentVolumeClaim", + }, + }, + }, + }, + pvcStatusReady: true, + expectedPVSpec: nil, + cloneSupport: true, + expectErr: true, + }, + "provision with pvc data source when source pv not found": { + volOpts: controller.ProvisionOptions{ + StorageClass: &storagev1.StorageClass{ + ReclaimPolicy: &deletePolicy, + Parameters: map[string]string{}, + }, + PVName: "invalid-pv", PVC: &v1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{ UID: "testid", }, Spec: v1.PersistentVolumeClaimSpec{ - StorageClassName: &invalidSCName, + StorageClassName: &fakeSc1, Selector: nil, Resources: v1.ResourceRequirements{ Requests: v1.ResourceList{ @@ -2444,7 +2548,7 @@ func TestProvisionFromPVC(t *testing.T) { }, AccessModes: []v1.PersistentVolumeAccessMode{v1.ReadWriteOnce}, DataSource: &v1.TypedLocalObjectReference{ - Name: srcName, + Name: invalidPVC, Kind: "PersistentVolumeClaim", }, }, @@ -2479,9 +2583,31 @@ func TestProvisionFromPVC(t *testing.T) { clientSet = fakeclientset.NewSimpleClientset() + pv := &v1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: pvName, + }, + Spec: v1.PersistentVolumeSpec{ + PersistentVolumeSource: v1.PersistentVolumeSource{ + CSI: &v1.CSIPersistentVolumeSource{ + Driver: driverName, + VolumeHandle: "test-volume-id", + FSType: "ext3", + VolumeAttributes: map[string]string{ + "storage.kubernetes.io/csiProvisionerIdentity": "test-provisioner", + }, + }, + }, + }, + } + // Create a fake claim as our PVC DataSource - claim := fakeClaim(srcName, "fake-claim-uid", "1Gi", "test-pv", v1.ClaimBound, nil) - clientSet = fakeclientset.NewSimpleClientset(claim) + claim := fakeClaim(srcName, "fake-claim-uid", "1Gi", pvName, v1.ClaimBound, &fakeSc1) + // Create a fake claim with invalid PV + invalidClaim := fakeClaim(invalidPVC, "fake-claim-uid", "1Gi", "pv-not-present", v1.ClaimBound, &fakeSc1) + /// Create a fake claim as source PVC storageclass nil + scNilClaim := fakeClaim("pvc-sc-nil", "fake-claim-uid", "1Gi", pvName, v1.ClaimBound, nil) + clientSet = fakeclientset.NewSimpleClientset(claim, scNilClaim, pv, invalidClaim) pluginCaps, controllerCaps := provisionFromPVCCapabilities() if !tc.cloneSupport { @@ -2492,6 +2618,19 @@ func TestProvisionFromPVC(t *testing.T) { controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(out, nil).Times(1) } + if tc.restoredVolSizeSmall { + controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(out, nil).Times(1) + // if the volume created is less than the requested size, + // deletevolume will be called + controllerServer.EXPECT().DeleteVolume(gomock.Any(), &csi.DeleteVolumeRequest{ + VolumeId: "test-volume-id", + }).Return(&csi.DeleteVolumeResponse{}, nil).Times(1) + } + + if tc.restoredVolSizeBig { + controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(nil, errors.New("source volume size is bigger than requested volume size")).Times(1) + } + csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5, csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false) pv, err := csiProvisioner.Provision(tc.volOpts)