Merge pull request #11 from xing-yang/fixes

Change VolumeSnapshotClassName to a pointer to string

pkg/apis/volumesnapshot/v1alpha1/types.go

@@ -80,7 +80,7 @@ type VolumeSnapshotSpec struct {
 	// Name of the VolumeSnapshotClass used by the VolumeSnapshot. If not specified, a default snapshot class will
 	// be used if it is available.
 	// +optional
-	VolumeSnapshotClassName string `json:"snapshotClassName" protobuf:"bytes,3,opt,name=snapshotClassName"`
+	VolumeSnapshotClassName *string `json:"snapshotClassName" protobuf:"bytes,3,opt,name=snapshotClassName"`
 }
 
 // VolumeSnapshotStatus is the status of the VolumeSnapshot
@@ -92,7 +92,7 @@ type VolumeSnapshotStatus struct {
 
 	// When restoring volume from the snapshot, the volume size should be equal to or
 	// larger than the RestoreSize if it is specified. If RestoreSize is set to nil, it means
-	// that the storage plugin does not have this information avaialble.
+	// that the storage plugin does not have this information available.
 	// +optional
 	RestoreSize *resource.Quantity `json:"restoreSize" protobuf:"bytes,2,opt,name=restoreSize"`
 
@@ -205,7 +205,7 @@ type VolumeSnapshotContentSpec struct {
 	// Name of the VolumeSnapshotClass used by the VolumeSnapshot. If not specified, a default snapshot class will
 	// be used if it is available.
 	// +optional
-	VolumeSnapshotClassName string `json:"snapshotClassName" protobuf:"bytes,4,opt,name=snapshotClassName"`
+	VolumeSnapshotClassName *string `json:"snapshotClassName" protobuf:"bytes,4,opt,name=snapshotClassName"`
 }
 
 // VolumeSnapshotSource represents the actual location and type of the snapshot. Only one of its members may be specified.
@@ -239,7 +239,7 @@ type CSIVolumeSnapshotSource struct {
 
 	// When restoring volume from the snapshot, the volume size should be equal to or
 	// larger than the RestoreSize if it is specified. If RestoreSize is set to nil, it means
-	// that the storage plugin does not have this information avaialble.
+	// that the storage plugin does not have this information available.
 	// +optional
 	RestoreSize *resource.Quantity `json:"restoreSize" protobuf:"bytes,4,opt,name=restoreSize"`
 }

pkg/apis/volumesnapshot/v1alpha1/zz_generated.deepcopy.go

@@ -236,6 +236,11 @@ func (in *VolumeSnapshotContentSpec) DeepCopyInto(out *VolumeSnapshotContentSpec
 		*out = new(v1.ObjectReference)
 		**out = **in
 	}
+	if in.VolumeSnapshotClassName != nil {
+		in, out := &in.VolumeSnapshotClassName, &out.VolumeSnapshotClassName
+		*out = new(string)
+		**out = **in
+	}
 	return
 }
 
@@ -311,6 +316,11 @@ func (in *VolumeSnapshotSpec) DeepCopyInto(out *VolumeSnapshotSpec) {
 		*out = new(TypedLocalObjectReference)
 		**out = **in
 	}
+	if in.VolumeSnapshotClassName != nil {
+		in, out := &in.VolumeSnapshotClassName, &out.VolumeSnapshotClassName
+		*out = new(string)
+		**out = **in
+	}
 	return
 }
 

pkg/controller/snapshot_controller.go

@@ -247,7 +247,9 @@ func (ctrl *csiSnapshotController) getMatchSnapshotContent(snapshot *crdv1.Volum
 		if content.Spec.VolumeSnapshotRef != nil &&
 			content.Spec.VolumeSnapshotRef.Name == snapshot.Name &&
 			content.Spec.VolumeSnapshotRef.Namespace == snapshot.Namespace &&
-			content.Spec.VolumeSnapshotRef.UID == snapshot.UID {
+			content.Spec.VolumeSnapshotRef.UID == snapshot.UID &&
+			content.Spec.VolumeSnapshotClassName != nil && snapshot.Spec.VolumeSnapshotClassName != nil &&
+			*(content.Spec.VolumeSnapshotClassName) == *(snapshot.Spec.VolumeSnapshotClassName) {
 			found = true
 			snapshotContentObj = content
 			break
@@ -407,7 +409,8 @@ func (ctrl *csiSnapshotController) checkandBindSnapshotContent(snapshot *crdv1.V
 	} else if content.Spec.VolumeSnapshotRef.UID == "" {
 		contentClone := content.DeepCopy()
 		contentClone.Spec.VolumeSnapshotRef.UID = snapshot.UID
-		contentClone.Spec.VolumeSnapshotClassName = snapshot.Spec.VolumeSnapshotClassName
+		className := *(snapshot.Spec.VolumeSnapshotClassName)
+		contentClone.Spec.VolumeSnapshotClassName = &className
 		newContent, err := ctrl.clientset.VolumesnapshotV1alpha1().VolumeSnapshotContents().Update(contentClone)
 		if err != nil {
 			glog.V(4).Infof("updating VolumeSnapshotContent[%s] error status failed %v", newContent.Name, err)
@@ -447,11 +450,22 @@ func (ctrl *csiSnapshotController) createSnapshotOperation(snapshot *crdv1.Volum
 		glog.V(4).Infof("error is already set in snapshot, do not retry to create: %s", snapshot.Status.Error.Message)
 		return snapshot, nil
 	}
-	class, err := ctrl.GetClassFromVolumeSnapshot(snapshot)
+
-	if err != nil {
+	className := snapshot.Spec.VolumeSnapshotClassName
-		glog.Errorf("createSnapshotOperation failed to getClassFromVolumeSnapshot %s", err)
+	glog.V(5).Infof("createSnapshotOperation [%s]: VolumeSnapshotClassName [%s]", snapshot.Name, *className)
-		return nil, err
+	var class *crdv1.VolumeSnapshotClass
+	var err error
+	if className != nil {
+		class, err = ctrl.GetSnapshotClass(*className)
+		if err != nil {
+			glog.Errorf("createSnapshotOperation failed to getClassFromVolumeSnapshot %s", err)
+			return nil, err
+		}
+	} else {
+		glog.Errorf("failed to take snapshot %s without a snapshot class", snapshot.Name)
+		return nil, fmt.Errorf("failed to take snapshot %s without a snapshot class", snapshot.Name)
 	}
+
 	volume, err := ctrl.getVolumeFromVolumeSnapshot(snapshot)
 	if err != nil {
 		glog.Errorf("createSnapshotOperation failed to get PersistentVolume object [%s]: Error: [%#v]", snapshot.Name, err)
@@ -504,7 +518,7 @@ func (ctrl *csiSnapshotController) createSnapshotOperation(snapshot *crdv1.Volum
 				Namespace:  snapshot.Namespace,
 				Name:       snapshot.Name,
 				UID:        snapshot.UID,
-				APIVersion: "v1alpha1",
+				APIVersion: "snapshot.storage.k8s.io/v1alpha1",
 			},
 			PersistentVolumeRef: volumeRef,
 			VolumeSnapshotSource: crdv1.VolumeSnapshotSource{
@@ -515,7 +529,7 @@ func (ctrl *csiSnapshotController) createSnapshotOperation(snapshot *crdv1.Volum
 					RestoreSize:    resource.NewQuantity(size, resource.BinarySI),
 				},
 			},
-			VolumeSnapshotClassName: class.Name,
+			VolumeSnapshotClassName: &(class.Name),
 		},
 	}
 	// Try to create the VolumeSnapshotContent object several times
@@ -567,8 +581,8 @@ func (ctrl *csiSnapshotController) deleteSnapshotContentOperation(content *crdv1
 	// get secrets if VolumeSnapshotClass specifies it
 	var snapshotterCredentials map[string]string
 	snapshotClassName := content.Spec.VolumeSnapshotClassName
-	if len(snapshotClassName) != 0 {
+	if snapshotClassName != nil {
-		if snapshotClass, err := ctrl.clientset.VolumesnapshotV1alpha1().VolumeSnapshotClasses().Get(snapshotClassName, metav1.GetOptions{}); err == nil {
+		if snapshotClass, err := ctrl.classLister.Get(*snapshotClassName); err == nil {
 			// Resolve snapshotting secret credentials.
 			// No VolumeSnapshot is provided when resolving delete secret names, since the VolumeSnapshot may or may not exist at delete time.
 			snapshotterSecretRef, err := GetSecretReference(snapshotClass.Parameters, content.Name, nil)
@@ -727,75 +741,75 @@ func (ctrl *csiSnapshotController) getStorageClassFromVolumeSnapshot(snapshot *c
 	return storageclass, nil
 }
 
-// GetClassFromVolumeSnapshot is a helper function to get snapshot class from VolumeSnapshot.
+// GetSnapshotClass is a helper function to get snapshot class from the class name.
-// If snapshot spec does not specify a VolumeSnapshotClass name, this function will try to figure out
+func (ctrl *csiSnapshotController) GetSnapshotClass(className string) (*crdv1.VolumeSnapshotClass, error) {
-// the default one from the PVC/PV StorageClass
+	glog.V(5).Infof("getSnapshotClass: VolumeSnapshotClassName [%s]", className)
-func (ctrl *csiSnapshotController) GetClassFromVolumeSnapshot(snapshot *crdv1.VolumeSnapshot) (*crdv1.VolumeSnapshotClass, error) {
-	className := snapshot.Spec.VolumeSnapshotClassName
-	glog.V(5).Infof("getClassFromVolumeSnapshot [%s]: VolumeSnapshotClassName [%s]", snapshot.Name, className)
 
-	if len(className) > 0 {
+	obj, found, err := ctrl.classStore.GetByKey(className)
-		obj, found, err := ctrl.classStore.GetByKey(className)
+	if found {
-		if found {
+		class, ok := obj.(*crdv1.VolumeSnapshotClass)
-			class, ok := obj.(*crdv1.VolumeSnapshotClass)
+		if ok {
-			if ok {
+			return class, nil
-				return class, nil
-			}
 		}
-		class, err := ctrl.clientset.VolumesnapshotV1alpha1().VolumeSnapshotClasses().Get(className, metav1.GetOptions{})
-		if err != nil {
-			glog.Errorf("failed to retrieve snapshot class %s from the API server: %q", className, err)
-			return nil, fmt.Errorf("failed to retrieve snapshot class %s from the API server: %q", className, err)
-		}
-		_, updateErr := ctrl.storeClassUpdate(class)
-		if updateErr != nil {
-			glog.V(4).Infof("GetClassFromVolumeSnapshot [%s]: cannot update internal cache: %v", class.Name, updateErr)
-		}
-		glog.V(5).Infof("getClassFromVolumeSnapshot [%s]: VolumeSnapshotClassName [%s]", snapshot.Name, className)
-		return class, nil
-	} else {
-		storageclass, err := ctrl.getStorageClassFromVolumeSnapshot(snapshot)
-		if err != nil {
-			return nil, err
-		}
-		// Find default snapshot class if available
-		list, err := ctrl.classLister.List(labels.Everything())
-		if err != nil {
-			return nil, err
-		}
-		defaultClasses := []*crdv1.VolumeSnapshotClass{}
-
-		for _, class := range list {
-			if IsDefaultAnnotation(class.ObjectMeta) && storageclass.Provisioner == class.Snapshotter {
-				defaultClasses = append(defaultClasses, class)
-				glog.V(5).Infof("getDefaultClass added: %s", class.Name)
-			}
-		}
-		if len(defaultClasses) == 0 {
-			return nil, fmt.Errorf("cannot find default snapshot class")
-		}
-		if len(defaultClasses) > 1 {
-			glog.V(4).Infof("getDefaultClass %d defaults found", len(defaultClasses))
-			return nil, fmt.Errorf("%d default snapshot classes were found", len(defaultClasses))
-		}
-		glog.V(5).Infof("getClassFromVolumeSnapshot [%s]: default VolumeSnapshotClassName [%s]", snapshot.Name, defaultClasses[0])
-		snapshotClone := snapshot.DeepCopy()
-		snapshotClone.Spec.VolumeSnapshotClassName = defaultClasses[0].Name
-		newSnapshot, err := ctrl.clientset.VolumesnapshotV1alpha1().VolumeSnapshots(snapshotClone.Namespace).Update(snapshotClone)
-		if err != nil {
-			glog.V(4).Infof("updating VolumeSnapshot[%s] default class failed %v", snapshotKey(snapshot), err)
-		}
-		_, updateErr := ctrl.storeSnapshotUpdate(newSnapshot)
-		if updateErr != nil {
-			// We will get an "snapshot update" event soon, this is not a big error
-			glog.V(4).Infof("createSnapshot [%s]: cannot update internal cache: %v", snapshotKey(snapshot), updateErr)
-		}
-		_, updateErr = ctrl.storeClassUpdate(defaultClasses[0])
-		if updateErr != nil {
-			glog.V(4).Infof("GetClassFromVolumeSnapshot [%s]: cannot update internal cache: %v", defaultClasses[0].Name, updateErr)
-		}
-		return defaultClasses[0], nil
 	}
+	class, err := ctrl.classLister.Get(className)
+	if err != nil {
+		glog.Errorf("failed to retrieve snapshot class %s from the API server: %q", className, err)
+		return nil, fmt.Errorf("failed to retrieve snapshot class %s from the API server: %q", className, err)
+	}
+	_, updateErr := ctrl.storeClassUpdate(class)
+	if updateErr != nil {
+		glog.V(4).Infof("getSnapshotClass [%s]: cannot update internal cache: %v", class.Name, updateErr)
+	}
+	return class, nil
+}
+
+// SetDefaultSnapshotClass is a helper function to figure out the default snapshot class from
+// PVC/PV StorageClass and update VolumeSnapshot with this snapshot class name.
+func (ctrl *csiSnapshotController) SetDefaultSnapshotClass(snapshot *crdv1.VolumeSnapshot) (*crdv1.VolumeSnapshotClass, *crdv1.VolumeSnapshot, error) {
+	glog.V(5).Infof("SetDefaultSnapshotClass for snapshot [%s]", snapshot.Name)
+
+	storageclass, err := ctrl.getStorageClassFromVolumeSnapshot(snapshot)
+	if err != nil {
+		return nil, nil, err
+	}
+	// Find default snapshot class if available
+	list, err := ctrl.classLister.List(labels.Everything())
+	if err != nil {
+		return nil, nil, err
+	}
+	defaultClasses := []*crdv1.VolumeSnapshotClass{}
+
+	for _, class := range list {
+		if IsDefaultAnnotation(class.ObjectMeta) && storageclass.Provisioner == class.Snapshotter && ctrl.snapshotterName == class.Snapshotter {
+			defaultClasses = append(defaultClasses, class)
+			glog.V(5).Infof("get defaultClass added: %s", class.Name)
+		}
+	}
+	if len(defaultClasses) == 0 {
+		return nil, nil, fmt.Errorf("cannot find default snapshot class")
+	}
+	if len(defaultClasses) > 1 {
+		glog.V(4).Infof("get DefaultClass %d defaults found", len(defaultClasses))
+		return nil, nil, fmt.Errorf("%d default snapshot classes were found", len(defaultClasses))
+	}
+	glog.V(5).Infof("setDefaultSnapshotClass [%s]: default VolumeSnapshotClassName [%s]", snapshot.Name, defaultClasses[0].Name)
+	snapshotClone := snapshot.DeepCopy()
+	snapshotClone.Spec.VolumeSnapshotClassName = &(defaultClasses[0].Name)
+	newSnapshot, err := ctrl.clientset.VolumesnapshotV1alpha1().VolumeSnapshots(snapshotClone.Namespace).Update(snapshotClone)
+	if err != nil {
+		glog.V(4).Infof("updating VolumeSnapshot[%s] default class failed %v", snapshotKey(snapshot), err)
+	}
+	_, updateErr := ctrl.storeSnapshotUpdate(newSnapshot)
+	if updateErr != nil {
+		// We will get an "snapshot update" event soon, this is not a big error
+		glog.V(4).Infof("setDefaultSnapshotClass [%s]: cannot update internal cache: %v", snapshotKey(snapshot), updateErr)
+	}
+	_, updateErr = ctrl.storeClassUpdate(defaultClasses[0])
+	if updateErr != nil {
+		glog.V(4).Infof("setDefaultSnapshotClass [%s]: cannot update internal cache: %v", defaultClasses[0].Name, updateErr)
+	}
+	return defaultClasses[0], newSnapshot, nil
 }
 
 // getClaimFromVolumeSnapshot is a helper function to get PVC from VolumeSnapshot.

pkg/controller/snapshot_controller_base.go

@@ -28,7 +28,6 @@ import (
 	"github.com/kubernetes-csi/external-snapshotter/pkg/connection"
 	"k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/kubernetes"
@@ -274,8 +273,8 @@ func (ctrl *csiSnapshotController) contentWorker() {
 		if err == nil {
 			// Skip update if content is for another CSI driver
 			snapshotClassName := content.Spec.VolumeSnapshotClassName
-			if len(snapshotClassName) != 0 {
+			if snapshotClassName != nil {
-				if snapshotClass, err := ctrl.clientset.VolumesnapshotV1alpha1().VolumeSnapshotClasses().Get(snapshotClassName, metav1.GetOptions{}); err == nil {
+				if snapshotClass, err := ctrl.classLister.Get(*snapshotClassName); err == nil {
 					if snapshotClass.Snapshotter != ctrl.snapshotterName {
 						return false
 					}
@@ -325,12 +324,27 @@ func (ctrl *csiSnapshotController) contentWorker() {
 // shouldProcessSnapshot detect if snapshotter in the VolumeSnapshotClass is the same as the snapshotter
 // in external controller.
 func (ctrl *csiSnapshotController) shouldProcessSnapshot(snapshot *crdv1.VolumeSnapshot) bool {
-	class, err := ctrl.GetClassFromVolumeSnapshot(snapshot)
+	className := snapshot.Spec.VolumeSnapshotClassName
-	if err != nil {
+	var class *crdv1.VolumeSnapshotClass
-		glog.V(2).Infof("fail to get snapshot class for snapshot %s: %v", snapshotKey(snapshot), err)
+	var err error
-		ctrl.updateSnapshotErrorStatusWithEvent(snapshot, v1.EventTypeWarning, "SnapshotCreationFailed", fmt.Sprintf("Failed to create snapshot with error %v", err))
+	if className != nil {
-		return false
+		glog.V(5).Infof("shouldProcessSnapshot [%s]: VolumeSnapshotClassName [%s]", snapshot.Name, *className)
+		class, err = ctrl.GetSnapshotClass(*className)
+		if err != nil {
+			glog.Errorf("shouldProcessSnapshot failed to getSnapshotClass %s", err)
+			ctrl.updateSnapshotErrorStatusWithEvent(snapshot, v1.EventTypeWarning, "GetSnapshotClassFailed", fmt.Sprintf("Failed to get snapshot class with error %v", err))
+			return false
+		}
+	} else {
+		glog.V(5).Infof("shouldProcessSnapshot [%s]: SetDefaultSnapshotClass", snapshot.Name)
+		class, snapshot, err = ctrl.SetDefaultSnapshotClass(snapshot)
+		if err != nil {
+			glog.Errorf("shouldProcessSnapshot failed to setDefaultClass %s", err)
+			ctrl.updateSnapshotErrorStatusWithEvent(snapshot, v1.EventTypeWarning, "SetDefaultSnapshotClassFailed", fmt.Sprintf("Failed to set default snapshot class with error %v", err))
+			return false
+		}
 	}
+
 	glog.V(5).Infof("VolumeSnapshotClass Snapshotter [%s] Snapshot Controller snapshotterName [%s]", class.Snapshotter, ctrl.snapshotterName)
 	if class.Snapshotter != ctrl.snapshotterName {
 		glog.V(4).Infof("Skipping VolumeSnapshot %s for snapshotter [%s] in VolumeSnapshotClass because it does not match with the snapshotter for controller [%s]", snapshotKey(snapshot), class.Snapshotter, ctrl.snapshotterName)