From c424e68cef728a38c29e92c6631a69dfc263018b Mon Sep 17 00:00:00 2001 From: Hakan Memisoglu Date: Fri, 1 Mar 2019 14:56:22 -0800 Subject: [PATCH 1/3] Fix for pre-bound snapshot empty source error --- pkg/controller/snapshot_controller.go | 30 +++++++++++++++++++-------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/pkg/controller/snapshot_controller.go b/pkg/controller/snapshot_controller.go index bcb7bbbf7..e8a34b3de 100644 --- a/pkg/controller/snapshot_controller.go +++ b/pkg/controller/snapshot_controller.go @@ -21,6 +21,7 @@ import ( "strings" "time" + "github.com/golang/glog" crdv1 "github.com/kubernetes-csi/external-snapshotter/pkg/apis/volumesnapshot/v1alpha1" "k8s.io/api/core/v1" storagev1 "k8s.io/api/storage/v1" @@ -560,16 +561,27 @@ 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 + + if snapshot.Spec.Source == nil { + klog.V(5).Infof("checkandUpdateBoundSnapshotStatusOperation: snapshot [%s] is pre-bound to content [%s]", snapshot.Name, content.Name) + readyToUse, timestamp, size, err = ctrl.handler.GetSnapshotStatus(content) + if err != nil { + return nil, err + } + } 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) + } + var driverName string + var snapshotID string + driverName, snapshotID, timestamp, size, readyToUse, err = ctrl.handler.CreateSnapshot(snapshot, volume, class.Parameters, snapshotterCredentials) + if err != nil { + glog.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) } - klog.V(5).Infof("checkandUpdateBoundSnapshotStatusOperation: driver %s, snapshotId %s, timestamp %d, size %d, readyToUse %t", driverName, snapshotID, timestamp, size, readyToUse) if timestamp == 0 { timestamp = time.Now().UnixNano() From d6c5d512c6da3a9f5dc0bd49e6c70b62c3fcc98b Mon Sep 17 00:00:00 2001 From: Hakan Memisoglu Date: Wed, 6 Mar 2019 16:29:38 -0800 Subject: [PATCH 2/3] Add test for prebound case --- pkg/controller/framework_test.go | 32 ++++++++++++++++----------- pkg/controller/snapshot_controller.go | 21 +++++++++--------- pkg/controller/snapshot_ready_test.go | 15 +++++++++++++ 3 files changed, 44 insertions(+), 24 deletions(-) diff --git a/pkg/controller/framework_test.go b/pkg/controller/framework_test.go index da617cca4..751f530ae 100644 --- a/pkg/controller/framework_test.go +++ b/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{ - 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", @@ -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: diff --git a/pkg/controller/snapshot_controller.go b/pkg/controller/snapshot_controller.go index e8a34b3de..84f8b7644 100644 --- a/pkg/controller/snapshot_controller.go +++ b/pkg/controller/snapshot_controller.go @@ -257,16 +257,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 @@ -493,13 +492,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 @@ -508,14 +507,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) { diff --git a/pkg/controller/snapshot_ready_test.go b/pkg/controller/snapshot_ready_test.go index 51e8194ec..b495af781 100644 --- a/pkg/controller/snapshot_ready_test.go +++ b/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), From 2fcfc8be2815f91bba7a157753a5fa94beae3317 Mon Sep 17 00:00:00 2001 From: Hakan Memisoglu Date: Mon, 18 Mar 2019 13:04:19 -0700 Subject: [PATCH 3/3] Improve the logs --- pkg/controller/snapshot_controller.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/pkg/controller/snapshot_controller.go b/pkg/controller/snapshot_controller.go index 84f8b7644..b5777bb5e 100644 --- a/pkg/controller/snapshot_controller.go +++ b/pkg/controller/snapshot_controller.go @@ -21,7 +21,6 @@ import ( "strings" "time" - "github.com/golang/glog" crdv1 "github.com/kubernetes-csi/external-snapshotter/pkg/apis/volumesnapshot/v1alpha1" "k8s.io/api/core/v1" storagev1 "k8s.io/api/storage/v1" @@ -560,27 +559,31 @@ func (ctrl *csiSnapshotController) checkandUpdateBoundSnapshotStatusOperation(sn var timestamp int64 var size int64 var readyToUse = false + var driverName string + var snapshotID string if snapshot.Spec.Source == nil { - klog.V(5).Infof("checkandUpdateBoundSnapshotStatusOperation: snapshot [%s] is pre-bound to content [%s]", snapshot.Name, content.Name) + 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) } - var driverName string - var snapshotID string driverName, snapshotID, timestamp, size, readyToUse, err = ctrl.handler.CreateSnapshot(snapshot, volume, class.Parameters, snapshotterCredentials) if err != nil { - glog.Errorf("checkandUpdateBoundSnapshotStatusOperation: failed to call create snapshot to check whether the snapshot is ready to use %q", err) + 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) } + klog.V(5).Infof("checkandUpdateBoundSnapshotStatusOperation: driver %s, snapshotId %s, timestamp %d, size %d, readyToUse %t", driverName, snapshotID, timestamp, size, readyToUse) if timestamp == 0 { timestamp = time.Now().UnixNano()