Skip to content

Commit

Permalink
Split GetClassFromVolumeSnapshot to two functions
Browse files Browse the repository at this point in the history
This commit splits GetClassFromVolumeSnapshot to two functions,
GetSnapshotClass and SetDefaultSnapshotClass.
  • Loading branch information
xing-yang committed Aug 22, 2018
1 parent 61c67ae commit d95ff46
Show file tree
Hide file tree
Showing 2 changed files with 99 additions and 74 deletions.
149 changes: 80 additions & 69 deletions pkg/controller/snapshot_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -447,11 +447,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 {
glog.Errorf("createSnapshotOperation failed to getClassFromVolumeSnapshot %s", err)
return nil, err

className := snapshot.Spec.VolumeSnapshotClassName
glog.V(5).Infof("createSnapshotOperation [%s]: VolumeSnapshotClassName [%s]", snapshot.Name, className)
var class *crdv1.VolumeSnapshotClass
var err error
if len(className) > 0 {

This comment has been minimized.

Copy link
@jingxu97

jingxu97 Aug 23, 2018

Collaborator

When createSnapshotOperation is called, snapshotClassName should be already set and it matches the driver name. But just to be safe, we can add a check for nil, and return error. Don' t need to check empty string here because shouldProcessSnapshot should already return error for empty snapshotClassName.

This comment has been minimized.

Copy link
@xing-yang

xing-yang Aug 23, 2018

Author Collaborator

Hmm, I have already changed this. I wonder if you are reviewing an older version of the commit.

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)
Expand Down Expand Up @@ -504,7 +515,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{
Expand Down Expand Up @@ -727,75 +738,75 @@ func (ctrl *csiSnapshotController) getStorageClassFromVolumeSnapshot(snapshot *c
return storageclass, nil
}

// GetClassFromVolumeSnapshot is a helper function to get snapshot class from VolumeSnapshot.
// If snapshot spec does not specify a VolumeSnapshotClass name, this function will try to figure out
// the default one from the PVC/PV StorageClass
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)
// GetSnapshotClass is a helper function to get snapshot class from the class name.
func (ctrl *csiSnapshotController) GetSnapshotClass(className string) (*crdv1.VolumeSnapshotClass, error) {
glog.V(5).Infof("getSnapshotClass: VolumeSnapshotClassName [%s]", className)

if len(className) > 0 {
obj, found, err := ctrl.classStore.GetByKey(className)
if found {
class, ok := obj.(*crdv1.VolumeSnapshotClass)
if ok {
return class, nil
}
obj, found, err := ctrl.classStore.GetByKey(className)
if found {
class, ok := obj.(*crdv1.VolumeSnapshotClass)
if ok {
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{}
}
class, err := ctrl.clientset.VolumesnapshotV1alpha1().VolumeSnapshotClasses().Get(className, metav1.GetOptions{})

This comment has been minimized.

Copy link
@jingxu97

jingxu97 Aug 23, 2018

Collaborator

Can we use lister to get class here?

This comment has been minimized.

Copy link
@xing-yang

xing-yang Aug 23, 2018

Author Collaborator

Done

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
}

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)
// 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 {

This comment has been minimized.

Copy link
@jingxu97

jingxu97 Aug 23, 2018

Collaborator

class.Snapshotter should also match ctrl.snapshotter. Otherwise multiple controller might try to set default class at the same time?

This comment has been minimized.

Copy link
@xing-yang

xing-yang Aug 23, 2018

Author Collaborator

Done

defaultClasses = append(defaultClasses, class)
glog.V(5).Infof("get defaultClass added: %s", class.Name)
}
return defaultClasses[0], nil
}
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])
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.
Expand Down
24 changes: 19 additions & 5 deletions pkg/controller/snapshot_controller_base.go
Original file line number Diff line number Diff line change
Expand Up @@ -325,12 +325,26 @@ 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)
if err != nil {
glog.V(2).Infof("fail to get snapshot class for snapshot %s: %v", snapshotKey(snapshot), err)
ctrl.updateSnapshotErrorStatusWithEvent(snapshot, v1.EventTypeWarning, "SnapshotCreationFailed", fmt.Sprintf("Failed to create snapshot with error %v", err))
return false
className := snapshot.Spec.VolumeSnapshotClassName
glog.V(5).Infof("shouldProcessSnapshot [%s]: VolumeSnapshotClassName [%s]", snapshot.Name, className)
var class *crdv1.VolumeSnapshotClass
var err error
if len(className) > 0 {
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 {
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)
Expand Down

0 comments on commit d95ff46

Please sign in to comment.