Skip to content

Commit

Permalink
Merge pull request #98 from kastenhq/prebound-fix
Browse files Browse the repository at this point in the history 
Fix for pre-bound snapshot empty source error
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Mar 28, 2019
2 parents 76f6d7f + 2fcfc8b commit 84cca93
Show file tree
Hide file tree
Showing 3 changed files with 67 additions and 32 deletions.
32 changes: 19 additions & 13 deletions pkg/controller/framework_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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.
Expand Down Expand Up @@ -768,15 +767,17 @@ func newContent(name, className, snapshotHandle, volumeUID, volumeName, boundToS
},
},
VolumeSnapshotClassName: &className,
PersistentVolumeRef: &v1.ObjectReference{
Kind: "PersistentVolume",
APIVersion: "v1",
UID: types.UID(volumeUID),
Name: volumeName,
},
DeletionPolicy: deletionPolicy,
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",
Expand Down Expand Up @@ -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,
},
Expand All @@ -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)
}
Expand Down Expand Up @@ -969,6 +972,9 @@ var (
validSecretClass = "valid-secret-class"
sameDriver = "sameDriver"
diffDriver = "diffDriver"
noClaim = ""
noBoundUID = ""
noVolume = ""
)

// wrapTestWithInjectedOperation returns a testCall that:
Expand Down
52 changes: 33 additions & 19 deletions pkg/controller/snapshot_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -256,16 +256,15 @@ func (ctrl *csiSnapshotController) syncUnreadySnapshot(snapshot *crdv1.VolumeSna
if !ok {
return fmt.Errorf("expected volume snapshot content, got %+v", contentObj)
}

if err := ctrl.checkandBindSnapshotContent(snapshot, content); err != nil {
contentBound, err := ctrl.checkandBindSnapshotContent(snapshot, content)
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
Expand Down Expand Up @@ -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
Expand All @@ -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) {
Expand Down Expand Up @@ -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)
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
var driverName string
var snapshotID string

if snapshot.Spec.Source == nil {
klog.V(5).Infof("checkandUpdateBoundSnapshotStatusOperation: checking whether snapshot [%s] is pre-bound to content [%s]", snapshot.Name, content.Name)
readyToUse, timestamp, size, err = ctrl.handler.GetSnapshotStatus(content)
if err != nil {
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)

Expand Down
15 changes: 15 additions & 0 deletions pkg/controller/snapshot_ready_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down

0 comments on commit 84cca93

Please sign in to comment.