Merge pull request #98 from kastenhq/prebound-fix

Fix for pre-bound snapshot empty source error

pkg/controller/framework_test.go

@@ -28,8 +28,6 @@ import (
 	"testing"
 	"time"
 
-	"k8s.io/klog"
-
 	crdv1 "github.com/kubernetes-csi/external-snapshotter/pkg/apis/volumesnapshot/v1alpha1"
 	clientset "github.com/kubernetes-csi/external-snapshotter/pkg/client/clientset/versioned"
 	"github.com/kubernetes-csi/external-snapshotter/pkg/client/clientset/versioned/fake"
@@ -54,6 +52,7 @@ import (
 	core "k8s.io/client-go/testing"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/tools/record"
+	"k8s.io/klog"
 )
 
 // This is a unit test framework for snapshot controller.
@@ -768,15 +767,17 @@ func newContent(name, className, snapshotHandle, volumeUID, volumeName, boundToS
 				},
 			},
 			VolumeSnapshotClassName: &className,
-			PersistentVolumeRef: &v1.ObjectReference{
+			DeletionPolicy:          deletionPolicy,
-				Kind:       "PersistentVolume",
-				APIVersion: "v1",
-				UID:        types.UID(volumeUID),
-				Name:       volumeName,
-			},
-			DeletionPolicy: deletionPolicy,
 		},
 	}
+	if volumeName != noVolume {
+		content.Spec.PersistentVolumeRef = &v1.ObjectReference{
+			Kind:       "PersistentVolume",
+			APIVersion: "v1",
+			UID:        types.UID(volumeUID),
+			Name:       volumeName,
+		}
+	}
 	if boundToSnapshotName != "" {
 		content.Spec.VolumeSnapshotRef = &v1.ObjectReference{
 			Kind:       "VolumeSnapshot",
@@ -817,10 +818,6 @@ func newSnapshot(name, className, boundToContent, snapshotUID, claimName string,
 			SelfLink:        "/apis/snapshot.storage.k8s.io/v1alpha1/namespaces/" + testNamespace + "/volumesnapshots/" + name,
 		},
 		Spec: crdv1.VolumeSnapshotSpec{
-			Source: &v1.TypedLocalObjectReference{
-				Name: claimName,
-				Kind: "PersistentVolumeClaim",
-			},
 			VolumeSnapshotClassName: &className,
 			SnapshotContentName:     boundToContent,
 		},
@@ -831,6 +828,12 @@ func newSnapshot(name, className, boundToContent, snapshotUID, claimName string,
 			RestoreSize:  size,
 		},
 	}
+	if claimName != noClaim {
+		snapshot.Spec.Source = &v1.TypedLocalObjectReference{
+			Name: claimName,
+			Kind: "PersistentVolumeClaim",
+		}
+	}
 
 	return withSnapshotFinalizer(&snapshot)
 }
@@ -969,6 +972,9 @@ var (
 	validSecretClass   = "valid-secret-class"
 	sameDriver         = "sameDriver"
 	diffDriver         = "diffDriver"
+	noClaim            = ""
+	noBoundUID         = ""
+	noVolume           = ""
 )
 
 // wrapTestWithInjectedOperation returns a testCall that:

pkg/controller/snapshot_controller.go

@@ -256,16 +256,15 @@ func (ctrl *csiSnapshotController) syncUnreadySnapshot(snapshot *crdv1.VolumeSna
 		if !ok {
 			return fmt.Errorf("expected volume snapshot content, got %+v", contentObj)
 		}
-
+		contentBound, err := ctrl.checkandBindSnapshotContent(snapshot, content)
-		if err := ctrl.checkandBindSnapshotContent(snapshot, content); err != nil {
+		if err != nil {
 			// snapshot is bound but content is not bound to snapshot correctly
 			ctrl.updateSnapshotErrorStatusWithEvent(snapshot, v1.EventTypeWarning, "SnapshotBindFailed", fmt.Sprintf("Snapshot failed to bind VolumeSnapshotContent, %v", err))
 			return fmt.Errorf("snapshot %s is bound, but VolumeSnapshotContent %s is not bound to the VolumeSnapshot correctly, %v", uniqueSnapshotName, content.Name, err)
 		}
-
 		// snapshot is already bound correctly, check the status and update if it is ready.
 		klog.V(5).Infof("Check and update snapshot %s status", uniqueSnapshotName)
-		if err = ctrl.checkandUpdateBoundSnapshotStatus(snapshot, content); err != nil {
+		if err = ctrl.checkandUpdateBoundSnapshotStatus(snapshot, contentBound); err != nil {
 			return err
 		}
 		return nil
@@ -492,13 +491,13 @@ func (ctrl *csiSnapshotController) isVolumeBeingCreatedFromSnapshot(snapshot *cr
 }
 
 // The function checks whether the volumeSnapshotRef in snapshot content matches the given snapshot. If match, it binds the content with the snapshot
-func (ctrl *csiSnapshotController) checkandBindSnapshotContent(snapshot *crdv1.VolumeSnapshot, content *crdv1.VolumeSnapshotContent) error {
+func (ctrl *csiSnapshotController) checkandBindSnapshotContent(snapshot *crdv1.VolumeSnapshot, content *crdv1.VolumeSnapshotContent) (*crdv1.VolumeSnapshotContent, error) {
 	if content.Spec.VolumeSnapshotRef == nil || content.Spec.VolumeSnapshotRef.Name != snapshot.Name {
-		return fmt.Errorf("Could not bind snapshot %s and content %s, the VolumeSnapshotRef does not match", snapshot.Name, content.Name)
+		return nil, fmt.Errorf("Could not bind snapshot %s and content %s, the VolumeSnapshotRef does not match", snapshot.Name, content.Name)
 	} else if content.Spec.VolumeSnapshotRef.UID != "" && content.Spec.VolumeSnapshotRef.UID != snapshot.UID {
-		return fmt.Errorf("Could not bind snapshot %s and content %s, the VolumeSnapshotRef does not match", snapshot.Name, content.Name)
+		return nil, fmt.Errorf("Could not bind snapshot %s and content %s, the VolumeSnapshotRef does not match", snapshot.Name, content.Name)
 	} else if content.Spec.VolumeSnapshotRef.UID != "" && content.Spec.VolumeSnapshotClassName != nil {
-		return nil
+		return content, nil
 	}
 	contentClone := content.DeepCopy()
 	contentClone.Spec.VolumeSnapshotRef.UID = snapshot.UID
@@ -507,14 +506,14 @@ func (ctrl *csiSnapshotController) checkandBindSnapshotContent(snapshot *crdv1.V
 	newContent, err := ctrl.clientset.VolumesnapshotV1alpha1().VolumeSnapshotContents().Update(contentClone)
 	if err != nil {
 		klog.V(4).Infof("updating VolumeSnapshotContent[%s] error status failed %v", newContent.Name, err)
-		return err
+		return nil, err
 	}
 	_, err = ctrl.storeContentUpdate(newContent)
 	if err != nil {
 		klog.V(4).Infof("updating VolumeSnapshotContent[%s] error status: cannot update internal cache %v", newContent.Name, err)
-		return err
+		return nil, err
 	}
-	return nil
+	return newContent, nil
 }
 
 func (ctrl *csiSnapshotController) getCreateSnapshotInput(snapshot *crdv1.VolumeSnapshot) (*crdv1.VolumeSnapshotClass, *v1.PersistentVolume, string, map[string]string, error) {
@@ -560,14 +559,29 @@ func (ctrl *csiSnapshotController) checkandUpdateBoundSnapshotStatusOperation(sn
 	var timestamp int64
 	var size int64
 	var readyToUse = false
-	class, volume, _, snapshotterCredentials, err := ctrl.getCreateSnapshotInput(snapshot)
+	var driverName string
-	if err != nil {
+	var snapshotID string
-		return nil, fmt.Errorf("failed to get input parameters to create snapshot %s: %q", snapshot.Name, err)
+
-	}
+	if snapshot.Spec.Source == nil {
-	driverName, snapshotID, timestamp, size, readyToUse, err := ctrl.handler.CreateSnapshot(snapshot, volume, class.Parameters, snapshotterCredentials)
+		klog.V(5).Infof("checkandUpdateBoundSnapshotStatusOperation: checking whether snapshot [%s] is pre-bound to content [%s]", snapshot.Name, content.Name)
-	if err != nil {
+		readyToUse, timestamp, size, err = ctrl.handler.GetSnapshotStatus(content)
-		klog.Errorf("checkandUpdateBoundSnapshotStatusOperation: failed to call create snapshot to check whether the snapshot is ready to use %q", err)
+		if err != nil {
-		return nil, err
+			klog.Errorf("checkandUpdateBoundSnapshotStatusOperation: failed to call get snapshot status to check whether snapshot is ready to use %q", err)
+			return nil, err
+		}
+		if content.Spec.CSI != nil {
+			driverName, snapshotID = content.Spec.CSI.Driver, content.Spec.CSI.SnapshotHandle
+		}
+	} else {
+		class, volume, _, snapshotterCredentials, err := ctrl.getCreateSnapshotInput(snapshot)
+		if err != nil {
+			return nil, fmt.Errorf("failed to get input parameters to create snapshot %s: %q", snapshot.Name, err)
+		}
+		driverName, snapshotID, timestamp, size, readyToUse, err = ctrl.handler.CreateSnapshot(snapshot, volume, class.Parameters, snapshotterCredentials)
+		if err != nil {
+			klog.Errorf("checkandUpdateBoundSnapshotStatusOperation: failed to call create snapshot to check whether the snapshot is ready to use %q", err)
+			return nil, err
+		}
 	}
 	klog.V(5).Infof("checkandUpdateBoundSnapshotStatusOperation: driver %s, snapshotId %s, timestamp %d, size %d, readyToUse %t", driverName, snapshotID, timestamp, size, readyToUse)
 

pkg/controller/snapshot_ready_test.go

@@ -123,6 +123,21 @@ func TestSync(t *testing.T) {
 			errors: noerrors,
 			test:   testSyncSnapshot,
 		},
+		{
+			name:              "2-6 - snapshot bound to prebound content correctly, status ready false -> true, ref.UID '' -> 'snapuid2-6'",
+			initialContents:   newContentArray("content2-6", validSecretClass, "sid2-6", noVolume, noVolume, noBoundUID, "snap2-6", &deletePolicy, nil, &timeNow, false),
+			expectedContents:  newContentArray("content2-6", validSecretClass, "sid2-6", noVolume, noVolume, "snapuid2-6", "snap2-6", &deletePolicy, nil, &timeNow, false),
+			initialSnapshots:  newSnapshotArray("snap2-6", validSecretClass, "content2-6", "snapuid2-6", noClaim, false, nil, metaTimeNow, nil),
+			expectedSnapshots: newSnapshotArray("snap2-6", validSecretClass, "content2-6", "snapuid2-6", noClaim, true, nil, metaTimeNow, nil),
+			expectedListCalls: []listCall{
+				{
+					snapshotID: "sid2-6",
+					readyToUse: true,
+				},
+			},
+			errors: noerrors,
+			test:   testSyncSnapshot,
+		},
 		{
 			name:              "2-7 - snapshot and content bound, csi driver get status error",
 			initialContents:   newContentArray("content2-7", validSecretClass, "sid2-7", "vuid2-7", "volume2-7", "snapuid2-7", "snap2-7", &deletePolicy, nil, nil, false),