alexlebens/external-snapshotter
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Handle the CSI driver in VolumeSnapshotContent does not match case

Browse Source 
In VolumeSnapshotContent, if the CSI driver does not match the plugin's,
the controller should skip this content instead of always processing it.
This PR also add a few tests related to snapshot and content static
binding.

During binding, if content specify its bound snapshot uid and it
does not match the snapshot's uid, the content object and also the
physical snapshot will be deleted. In this case, the controller will
treat the content as an orphan content because its snapshot object does
not exist (deleted) any more.
This commit is contained in:
Jing Xu
2018-09-20 15:21:18 -07:00
parent 5ed076f3db
commit c401b2331c
5 changed files with 151 additions and 59 deletions
Download Patch File Download Diff File Expand all files Collapse all files

17
pkg/controller/framework_test.go
View File

@@ -34,6 +34,7 @@ import (
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"
snapshotscheme "github.com/kubernetes-csi/external-snapshotter/pkg/client/clientset/versioned/scheme"
informers "github.com/kubernetes-csi/external-snapshotter/pkg/client/informers/externalversions"
storagelisters "github.com/kubernetes-csi/external-snapshotter/pkg/client/listers/volumesnapshot/v1alpha1"
"k8s.io/api/core/v1"
@@ -48,6 +49,7 @@ import (
"k8s.io/apimachinery/pkg/watch"
"k8s.io/client-go/kubernetes"
kubefake "k8s.io/client-go/kubernetes/fake"
"k8s.io/client-go/kubernetes/scheme"
core "k8s.io/client-go/testing"
"k8s.io/client-go/tools/cache"
"k8s.io/client-go/tools/record"
@@ -786,6 +788,14 @@ func newContentArray(name, className, snapshotHandle, volumeUID, volumeName, bou
}
}
func newContentWithUnmatchDriverArray(name, className, snapshotHandle, volumeUID, volumeName, boundToSnapshotUID, boundToSnapshotName string, size *int64, creationTime *int64) []*crdv1.VolumeSnapshotContent {
content := newContent(name, className, snapshotHandle, volumeUID, volumeName, boundToSnapshotUID, boundToSnapshotName, size, creationTime)
content.Spec.VolumeSnapshotSource.CSI.Driver = "fake"
return []*crdv1.VolumeSnapshotContent{
content,
}
}
func newSnapshot(name, className, boundToContent, snapshotUID, claimName string, ready bool, err *storagev1beta1.VolumeError, creationTime *metav1.Time, size *resource.Quantity) *crdv1.VolumeSnapshot {
snapshot := crdv1.VolumeSnapshot{
ObjectMeta: metav1.ObjectMeta{
@@ -1005,6 +1015,7 @@ func evaluateTestResults(ctrl *csiSnapshotController, reactor *snapshotReactor,
// controllerTest.testCall *once*.
// 3. Compare resulting contents and snapshots with expected contents and snapshots.
func runSyncTests(t *testing.T, tests []controllerTest, snapshotClasses []*crdv1.VolumeSnapshotClass) {
snapshotscheme.AddToScheme(scheme.Scheme)
for _, test := range tests {
glog.V(4).Infof("starting test %q", test.name)
@@ -1023,8 +1034,10 @@ func runSyncTests(t *testing.T, tests []controllerTest, snapshotClasses []*crdv1
reactor.snapshots[snapshot.Name] = snapshot
}
for _, content := range test.initialContents {
ctrl.contentStore.Add(content)
reactor.contents[content.Name] = content
if ctrl.isDriverMatch(test.initialContents[0]) {
ctrl.contentStore.Add(content)
reactor.contents[content.Name] = content
}
}
for _, claim := range test.initialClaims {
reactor.claims[claim.Name] = claim

89
pkg/controller/snapshot_controller.go
View File

@@ -85,49 +85,49 @@ const IsDefaultSnapshotClassAnnotation = "snapshot.storage.kubernetes.io/is-defa
func (ctrl *csiSnapshotController) syncContent(content *crdv1.VolumeSnapshotContent) error {
glog.V(5).Infof("synchronizing VolumeSnapshotContent[%s]", content.Name)
// VolumeSnapshotContent is not bound to any VolumeSnapshot, this case rare and we just return err
// VolumeSnapshotContent is not bound to any VolumeSnapshot, in this case we just return err
if content.Spec.VolumeSnapshotRef == nil {
// content is not bound
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: VolumeSnapshotContent is not bound to any VolumeSnapshot", content.Name)
ctrl.eventRecorder.Event(content, v1.EventTypeWarning, "SnapshotContentNotBound", "VolumeSnapshotContent is not bound to any VolumeSnapshot")
return fmt.Errorf("volumeSnapshotContent %s is not bound to any VolumeSnapshot", content.Name)
} else {
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: content is bound to snapshot %s", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
// The VolumeSnapshotContent is reserved for a VolumeSnapshot;
// that VolumeSnapshot has not yet been bound to this VolumeSnapshotContent; the VolumeSnapshot sync will handle it.
if content.Spec.VolumeSnapshotRef.UID == "" {
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: VolumeSnapshotContent is pre-bound to VolumeSnapshot %s", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
return nil
}
// Get the VolumeSnapshot by _name_
var snapshot *crdv1.VolumeSnapshot
snapshotName := snapshotRefKey(content.Spec.VolumeSnapshotRef)
obj, found, err := ctrl.snapshotStore.GetByKey(snapshotName)
if err != nil {
return err
}
if !found {
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: snapshot %s not found", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
// Fall through with snapshot = nil
} else {
var ok bool
snapshot, ok = obj.(*crdv1.VolumeSnapshot)
if !ok {
return fmt.Errorf("cannot convert object from snapshot cache to snapshot %q!?: %#v", content.Name, obj)
}
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: snapshot %s found", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
}
if snapshot != nil && snapshot.UID != content.Spec.VolumeSnapshotRef.UID {
// The snapshot that the content was pointing to was deleted, and another
// with the same name created.
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: content %s has different UID, the old one must have been deleted", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
// Treat the volume as bound to a missing claim.
snapshot = nil
}
if snapshot == nil {
ctrl.deleteSnapshotContent(content)
}
}
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: content is bound to snapshot %s", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
// The VolumeSnapshotContent is reserved for a VolumeSnapshot;
// that VolumeSnapshot has not yet been bound to this VolumeSnapshotContent; the VolumeSnapshot sync will handle it.
if content.Spec.VolumeSnapshotRef.UID == "" {
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: VolumeSnapshotContent is pre-bound to VolumeSnapshot %s", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
return nil
}
// Get the VolumeSnapshot by _name_
var snapshot *crdv1.VolumeSnapshot
snapshotName := snapshotRefKey(content.Spec.VolumeSnapshotRef)
obj, found, err := ctrl.snapshotStore.GetByKey(snapshotName)
if err != nil {
return err
}
if !found {
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: snapshot %s not found", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
// Fall through with snapshot = nil
} else {
var ok bool
snapshot, ok = obj.(*crdv1.VolumeSnapshot)
if !ok {
return fmt.Errorf("cannot convert object from snapshot cache to snapshot %q!?: %#v", content.Name, obj)
}
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: snapshot %s found", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
}
if snapshot != nil && snapshot.UID != content.Spec.VolumeSnapshotRef.UID {
// The snapshot that the content was pointing to was deleted, and another
// with the same name created.
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: content %s has different UID, the old one must have been deleted", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
// Treat the content as bound to a missing snapshot.
snapshot = nil
}
if snapshot == nil {
ctrl.deleteSnapshotContent(content)
}
return nil
}
@@ -506,19 +506,20 @@ func (ctrl *csiSnapshotController) createSnapshotOperation(snapshot *crdv1.Volum
}
// Create VolumeSnapshotContent in the database
volumeRef, err := ref.GetReference(scheme.Scheme, volume)
if err != nil {
return nil, err
}
snapshotRef, err := ref.GetReference(scheme.Scheme, snapshot)
if err != nil {
return nil, err
}
snapshotContent := &crdv1.VolumeSnapshotContent{
ObjectMeta: metav1.ObjectMeta{
Name: contentName,
},
Spec: crdv1.VolumeSnapshotContentSpec{
VolumeSnapshotRef: &v1.ObjectReference{
Kind: "VolumeSnapshot",
Namespace: snapshot.Namespace,
Name: snapshot.Name,
UID: snapshot.UID,
APIVersion: "snapshot.storage.k8s.io/v1alpha1",
},
VolumeSnapshotRef: snapshotRef,
PersistentVolumeRef: volumeRef,
VolumeSnapshotSource: crdv1.VolumeSnapshotSource{
CSI: &crdv1.CSIVolumeSnapshotSource{

37
pkg/controller/snapshot_controller_base.go
View File

@@ -271,20 +271,12 @@ func (ctrl *csiSnapshotController) contentWorker() {
return false
}
content, err := ctrl.contentLister.Get(name)
// The content still exists in informer cache, the event must have
// been add/update/sync
if err == nil {
// Skip update if content is for another CSI driver
snapshotClassName := content.Spec.VolumeSnapshotClassName
if snapshotClassName != nil {
if snapshotClass, err := ctrl.classLister.Get(*snapshotClassName); err == nil {
if snapshotClass.Snapshotter != ctrl.snapshotterName {
return false
}
}
if ctrl.isDriverMatch(content) {
ctrl.updateContent(content)
}
// The content still exists in informer cache, the event must have
// been add/update/sync
ctrl.updateContent(content)
return false
}
if !errors.IsNotFound(err) {
@@ -322,6 +314,27 @@ func (ctrl *csiSnapshotController) contentWorker() {
}
}
// verify whether the driver specified in VolumeSnapshotContent matches the controller's driver name
func (ctrl *csiSnapshotController) isDriverMatch(content *crdv1.VolumeSnapshotContent) bool {
if content.Spec.VolumeSnapshotSource.CSI == nil {
// Skip this snapshot content if it not a CSI snapshot
return false
}
if content.Spec.VolumeSnapshotSource.CSI.Driver != ctrl.snapshotterName {
// Skip this snapshot content if the driver does not match
return false
}
snapshotClassName := content.Spec.VolumeSnapshotClassName
if snapshotClassName != nil {
if snapshotClass, err := ctrl.classLister.Get(*snapshotClassName); err == nil {
if snapshotClass.Snapshotter != ctrl.snapshotterName {
return false
}
}
}
return true
}
// checkAndUpdateSnapshotClass gets the VolumeSnapshotClass from VolumeSnapshot. If it is not set,
// gets it from default VolumeSnapshotClass and sets it. It also detects if snapshotter in the
// VolumeSnapshotClass is the same as the snapshotter in external controller.

47
pkg/controller/snapshot_delete_test.go
View File

@@ -118,7 +118,7 @@ var snapshotClasses = []*crdv1.VolumeSnapshotClass{
func TestDeleteSync(t *testing.T) {
tests := []controllerTest{
{
name: "1-1 - successful delete with empty snapshot class",
name: "1-1 - content with empty snapshot class is deleted if it is bound to a non-exist snapshot and also has a snapshot uid specified",
initialContents: newContentArray("content1-1", classEmpty, "sid1-1", "vuid1-1", "volume1-1", "snapuid1-1", "snap1-1", nil, nil),
expectedContents: nocontents,
initialSnapshots: nosnapshots,
@@ -128,6 +128,17 @@ func TestDeleteSync(t *testing.T) {
expectedDeleteCalls: []deleteCall{{"sid1-1", nil, nil}},
test: testSyncContent,
},
{
name: "2-1 - content with empty snapshot class will not be deleted if it is bound to a non-exist snapshot but it does not have a snapshot uid specified",
initialContents: newContentArray("content2-1", classEmpty, "sid2-1", "vuid2-1", "volume2-1", "", "snap2-1", nil, nil),
expectedContents: newContentArray("content2-1", classEmpty, "sid2-1", "vuid2-1", "volume2-1", "", "snap2-1", nil, nil),
initialSnapshots: nosnapshots,
expectedSnapshots: nosnapshots,
expectedEvents: noevents,
errors: noerrors,
expectedDeleteCalls: []deleteCall{{"sid2-1", nil, nil}},
test: testSyncContent,
},
{
name: "1-2 - successful delete with snapshot class that has empty secret parameter",
initialContents: newContentArray("content1-2", emptySecretClass, "sid1-2", "vuid1-2", "volume1-2", "snapuid1-2", "snap1-2", nil, nil),
@@ -222,6 +233,40 @@ func TestDeleteSync(t *testing.T) {
reactor.lock.Unlock()
}),
},
{
name: "1-9 - content will not be deleted if it is bound to a snapshot correctly, snapshot uid is specified",
initialContents: newContentArray("content1-9", validSecretClass, "sid1-9", "vuid1-9", "volume1-9", "snapuid1-9", "snap1-9", nil, nil),
expectedContents: newContentArray("content1-9", validSecretClass, "sid1-9", "vuid1-9", "volume1-9", "snapuid1-9", "snap1-9", nil, nil),
initialSnapshots: newSnapshotArray("snap1-9", validSecretClass, "content1-9", "snapuid1-9", "claim1-9", false, nil, nil, nil),
expectedSnapshots: newSnapshotArray("snap1-9", validSecretClass, "content1-9", "snapuid1-9", "claim1-9", false, nil, nil, nil),
expectedEvents: noevents,
initialSecrets: []*v1.Secret{secret()},
errors: noerrors,
test: testSyncContent,
},
{
name: "1-10 - should delete content which is bound to a snapshot incorrectly",
initialContents: newContentArray("content1-10", validSecretClass, "sid1-10", "vuid1-10", "volume1-10", "snapuid1-10-x", "snap1-10", nil, nil),
expectedContents: nocontents,
initialSnapshots: newSnapshotArray("snap1-10", validSecretClass, "content1-10", "snapuid1-10", "claim1-10", false, nil, nil, nil),
expectedSnapshots: newSnapshotArray("snap1-10", validSecretClass, "content1-10", "snapuid1-10", "claim1-10", false, nil, nil, nil),
expectedEvents: noevents,
initialSecrets: []*v1.Secret{secret()},
errors: noerrors,
expectedDeleteCalls: []deleteCall{{"sid1-10", map[string]string{"foo": "bar"}, nil}},
test: testSyncContent,
},
{
name: "1-11 - content will not be deleted if it is bound to a snapshot correctly, snapsht uid is not specified",
initialContents: newContentArray("content1-11", validSecretClass, "sid1-11", "vuid1-11", "volume1-11", "", "snap1-11", nil, nil),
expectedContents: newContentArray("content1-11", validSecretClass, "sid1-11", "vuid1-11", "volume1-11", "", "snap1-11", nil, nil),
initialSnapshots: newSnapshotArray("snap1-11", validSecretClass, "content1-11", "snapuid1-11", "claim1-11", false, nil, nil, nil),
expectedSnapshots: newSnapshotArray("snap1-11", validSecretClass, "content1-11", "snapuid1-11", "claim1-11", false, nil, nil, nil),
expectedEvents: noevents,
initialSecrets: []*v1.Secret{secret()},
errors: noerrors,
test: testSyncContent,
},
}
runSyncTests(t, tests, snapshotClasses)
}

20
pkg/controller/snapshot_ready_test.go
View File

@@ -232,6 +232,26 @@ func TestSync(t *testing.T) {
errors: noerrors,
test: testSyncSnapshot,
},
{
name: "3-5 - snapshot bound to content in which the driver does not match",
initialContents: newContentWithUnmatchDriverArray("content3-5", validSecretClass, "sid3-5", "vuid3-5", "volume3-5", "", "snap3-5", nil, nil),
expectedContents: nocontents,
initialSnapshots: newSnapshotArray("snap3-5", validSecretClass, "content3-5", "snapuid3-5", "claim3-5", false, nil, nil, nil),
expectedSnapshots: newSnapshotArray("snap3-5", validSecretClass, "content3-5", "snapuid3-5", "claim3-5", false, newVolumeError("VolumeSnapshotContent is missing"), nil, nil),
expectedEvents: []string{"Warning SnapshotContentMissing"},
errors: noerrors,
test: testSyncSnapshotError,
},
{
name: "3-6 - snapshot bound to content in which the snapshot uid does not match",
initialContents: newContentArray("content3-4", validSecretClass, "sid3-4", "vuid3-4", "volume3-4", "snapuid3-4-x", "snap3-6", nil, nil),
expectedContents: newContentArray("content3-4", validSecretClass, "sid3-4", "vuid3-4", "volume3-4", "snapuid3-4-x", "snap3-6", nil, nil),
initialSnapshots: newSnapshotArray("snap3-5", validSecretClass, "content3-5", "snapuid3-5", "claim3-5", false, nil, nil, nil),
expectedSnapshots: newSnapshotArray("snap3-5", validSecretClass, "content3-5", "snapuid3-5", "claim3-5", false, newVolumeError("VolumeSnapshotContent is missing"), nil, nil),
expectedEvents: []string{"Warning SnapshotContentMissing"},
errors: noerrors,
test: testSyncSnapshotError,
},
}
runSyncTests(t, tests, snapshotClasses)