Addressed review comments

Author: xing-yang

cmd/csi-provisioner/csi-provisioner.go

@@ -87,6 +87,7 @@ func init() {
 	if err != nil {
 		glog.Fatalf("Failed to create client: %v", err)
 	}
+	// snapclientset.NewForConfig creates a new Clientset for VolumesnapshotV1alpha1Client
 	snapClient, err := snapclientset.NewForConfig(config)
 	if err != nil {
 		glog.Fatalf("Failed to create snapshot client: %v", err)

pkg/controller/controller.go

@@ -343,7 +343,7 @@ func (p *csiProvisioner) Provision(options controller.VolumeOptions) (*v1.Persis
 		return nil, fmt.Errorf("claim Selector is not supported")
 	}
 
-	var needSnapshotSupport bool
+	var needSnapshotSupport bool = false
 	if options.PVC.Spec.DataSource != nil {
 		// PVC.Spec.DataSource.Name is the name of the VolumeSnapshot API object
 		if options.PVC.Spec.DataSource.Name == "" {
@@ -394,7 +394,7 @@ func (p *csiProvisioner) Provision(options controller.VolumeOptions) (*v1.Persis
 		req.VolumeContentSource = volumeContentSource
 	}
 
-	glog.Infof("CreateVolumeRequest %+v", req)
+	glog.V(5).Infof("CreateVolumeRequest %+v", req)
 
 	rep := &csi.CreateVolumeResponse{}
 
@@ -529,6 +529,10 @@ func (p *csiProvisioner) getVolumeContentSource(options controller.VolumeOptions
 	}
 	glog.V(5).Infof("VolumeSnapshotContent %+v", snapContentObj)
 
+	if snapContentObj.Spec.VolumeSnapshotSource.CSI == nil || len(snapContentObj.Spec.VolumeSnapshotSource.CSI.SnapshotHandle) == 0 {
+		return nil, fmt.Errorf("error getting snapshot handle from snapshot:snapshotcontent %s:%s", snapshotObj.Name, snapshotObj.Spec.SnapshotContentName)
+	}
+
 	snapshotSource := csi.VolumeContentSource_Snapshot{
 		Snapshot: &csi.VolumeContentSource_SnapshotSource{
 			Id: snapContentObj.Spec.VolumeSnapshotSource.CSI.SnapshotHandle,
@@ -537,7 +541,10 @@ func (p *csiProvisioner) getVolumeContentSource(options controller.VolumeOptions
 	glog.V(5).Infof("VolumeContentSource_Snapshot %+v", snapshotSource)
 
 	if snapshotObj.Status.RestoreSize != nil {
-		capacity := options.PVC.Spec.Resources.Requests[v1.ResourceName(v1.ResourceStorage)]
+		capacity, exists := options.PVC.Spec.Resources.Requests[v1.ResourceName(v1.ResourceStorage)]
+		if !exists {
+			return nil, fmt.Errorf("error getting capacity for PVC %s when creating snapshot %s", options.PVC.Name, snapshotObj.Name)
+		}
 		volSizeBytes := capacity.Value()
 		glog.V(5).Infof("Requested volume size is %d and snapshot size is %d for the source snapshot %s", int64(volSizeBytes), int64(snapshotObj.Status.RestoreSize.Value()), snapshotObj.Name)
 		// When restoring volume from a snapshot, the volume size should

pkg/controller/controller_test.go

@@ -620,6 +620,8 @@ func provisionMockServerSetupExpectations(identityServer *driver.MockIdentitySer
 	}, nil).Times(1)
 }
 
+// provisionFromSnapshotMockServerSetupExpectations mocks plugin and controller capabilities reported
+// by a CSI plugin that supports the snapshot feature
 func provisionFromSnapshotMockServerSetupExpectations(identityServer *driver.MockIdentityServer, controllerServer *driver.MockControllerServer) {
 	identityServer.EXPECT().GetPluginCapabilities(gomock.Any(), gomock.Any()).Return(&csi.GetPluginCapabilitiesResponse{
 		Capabilities: []*csi.PluginCapability{
@@ -1166,7 +1168,7 @@ func TestProvisionFromSnapshot(t *testing.T) {
 		volOpts              controller.VolumeOptions
 		restoredVolSizeSmall bool
 		wrongDataSource      bool
-		validSnapshotStatus  bool
+		snapshotStatusReady  bool
 		expectedPVSpec       *pvSpec
 		expectErr            bool
 	}{
@@ -1195,7 +1197,7 @@ func TestProvisionFromSnapshot(t *testing.T) {
 				},
 				Parameters: map[string]string{},
 			},
-			validSnapshotStatus: true,
+			snapshotStatusReady: true,
 			expectedPVSpec: &pvSpec{
 				Name:          "test-testi",
 				ReclaimPolicy: v1.PersistentVolumeReclaimDelete,
@@ -1238,7 +1240,7 @@ func TestProvisionFromSnapshot(t *testing.T) {
 				Parameters: map[string]string{},
 			},
 			restoredVolSizeSmall: true,
-			validSnapshotStatus:  true,
+			snapshotStatusReady:  true,
 			expectErr:            true,
 		},
 		"fail empty snapshot name": {
@@ -1346,7 +1348,7 @@ func TestProvisionFromSnapshot(t *testing.T) {
 				},
 				Parameters: map[string]string{},
 			},
-			validSnapshotStatus: false,
+			snapshotStatusReady: false,
 			expectErr:           true,
 		},
 	}
@@ -1364,7 +1366,7 @@ func TestProvisionFromSnapshot(t *testing.T) {
 		client := &fake.Clientset{}
 
 		client.AddReactor("get", "volumesnapshots", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) {
-			snap := newSnapshot(snapName, snapClassName, "snapcontent-snapuid", "snapuid", "claim", tc.validSnapshotStatus, nil, metaTimeNowUnix, resource.NewQuantity(requestedBytes, resource.BinarySI))
+			snap := newSnapshot(snapName, snapClassName, "snapcontent-snapuid", "snapuid", "claim", tc.snapshotStatusReady, nil, metaTimeNowUnix, resource.NewQuantity(requestedBytes, resource.BinarySI))
 			return true, snap, nil
 		})
 
@@ -1382,11 +1384,19 @@ func TestProvisionFromSnapshot(t *testing.T) {
 			},
 		}
 
-		// Setup mock call expectations.
+		// Setup mock call expectations. If tc.wrongDataSource is false, DataSource is valid
+		// and the controller will proceed to check whether the plugin supports snapshot.
+		// So in this case, we need the plugin to report snapshot support capabilities;
+		// Otherwise, the controller will fail the operation so it won't check the capabilities.
 		if tc.wrongDataSource == false {
 			provisionFromSnapshotMockServerSetupExpectations(identityServer, controllerServer)
 		}
-		if tc.restoredVolSizeSmall == false && tc.wrongDataSource == false && tc.validSnapshotStatus {
+		// If tc.restoredVolSizeSmall is true, or tc.wrongDataSource is true, or
+		// tc.snapshotStatusReady is false,  create volume from snapshot operation will fail
+		// early and therefore CreateVolume is not expected to be called.
+		// When the following if condition is met, it is a valid create volume from snapshot
+		// operation and CreateVolume is expected to be called.
+		if tc.restoredVolSizeSmall == false && tc.wrongDataSource == false && tc.snapshotStatusReady {
 			controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(out, nil).Times(1)
 		}