Skip to content

Commit 6327ae5

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #32 from jingxu97/contentdefaultclass
Handle the CSI driver in VolumeSnapshotContent does not match case
2 parents 02306da + c401b23 commit 6327ae5

File tree

5 files changed

+149
-57
lines changed

5 files changed

+149
-57
lines changed

‎pkg/controller/framework_test.go‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ import (
3434
crdv1 "github.com/kubernetes-csi/external-snapshotter/pkg/apis/volumesnapshot/v1alpha1"
3535
clientset "github.com/kubernetes-csi/external-snapshotter/pkg/client/clientset/versioned"
3636
"github.com/kubernetes-csi/external-snapshotter/pkg/client/clientset/versioned/fake"
37+
snapshotscheme "github.com/kubernetes-csi/external-snapshotter/pkg/client/clientset/versioned/scheme"
3738
informers "github.com/kubernetes-csi/external-snapshotter/pkg/client/informers/externalversions"
3839
storagelisters "github.com/kubernetes-csi/external-snapshotter/pkg/client/listers/volumesnapshot/v1alpha1"
3940
"k8s.io/api/core/v1"
@@ -48,6 +49,7 @@ import (
4849
"k8s.io/apimachinery/pkg/watch"
4950
"k8s.io/client-go/kubernetes"
5051
kubefake "k8s.io/client-go/kubernetes/fake"
52+
"k8s.io/client-go/kubernetes/scheme"
5153
core "k8s.io/client-go/testing"
5254
"k8s.io/client-go/tools/cache"
5355
"k8s.io/client-go/tools/record"
@@ -786,6 +788,14 @@ func newContentArray(name, className, snapshotHandle, volumeUID, volumeName, bou
786788
}
787789
}
788790

791+
func newContentWithUnmatchDriverArray(name, className, snapshotHandle, volumeUID, volumeName, boundToSnapshotUID, boundToSnapshotName string, size *int64, creationTime *int64) []*crdv1.VolumeSnapshotContent {
792+
content := newContent(name, className, snapshotHandle, volumeUID, volumeName, boundToSnapshotUID, boundToSnapshotName, size, creationTime)
793+
content.Spec.VolumeSnapshotSource.CSI.Driver = "fake"
794+
return []*crdv1.VolumeSnapshotContent{
795+
content,
796+
}
797+
}
798+
789799
func newSnapshot(name, className, boundToContent, snapshotUID, claimName string, ready bool, err *storagev1beta1.VolumeError, creationTime *metav1.Time, size *resource.Quantity) *crdv1.VolumeSnapshot {
790800
snapshot := crdv1.VolumeSnapshot{
791801
ObjectMeta: metav1.ObjectMeta{
@@ -1005,6 +1015,7 @@ func evaluateTestResults(ctrl *csiSnapshotController, reactor *snapshotReactor,
10051015
// controllerTest.testCall *once*.
10061016
// 3. Compare resulting contents and snapshots with expected contents and snapshots.
10071017
func runSyncTests(t *testing.T, tests []controllerTest, snapshotClasses []*crdv1.VolumeSnapshotClass) {
1018+
snapshotscheme.AddToScheme(scheme.Scheme)
10081019
for _, test := range tests {
10091020
glog.V(4).Infof("starting test %q", test.name)
10101021

@@ -1023,8 +1034,10 @@ func runSyncTests(t *testing.T, tests []controllerTest, snapshotClasses []*crdv1
10231034
reactor.snapshots[snapshot.Name] = snapshot
10241035
}
10251036
for _, content := range test.initialContents {
1026-
ctrl.contentStore.Add(content)
1027-
reactor.contents[content.Name] = content
1037+
if ctrl.isDriverMatch(test.initialContents[0]) {
1038+
ctrl.contentStore.Add(content)
1039+
reactor.contents[content.Name] = content
1040+
}
10281041
}
10291042
for _, claim := range test.initialClaims {
10301043
reactor.claims[claim.Name] = claim

‎pkg/controller/snapshot_controller.go‎

Lines changed: 43 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -85,49 +85,49 @@ const IsDefaultSnapshotClassAnnotation = "snapshot.storage.kubernetes.io/is-defa
8585
func (ctrl *csiSnapshotController) syncContent(content *crdv1.VolumeSnapshotContent) error {
8686
glog.V(5).Infof("synchronizing VolumeSnapshotContent[%s]", content.Name)
8787

88-
// VolumeSnapshotContent is not bound to any VolumeSnapshot, this case rare and we just return err
88+
// VolumeSnapshotContent is not bound to any VolumeSnapshot, in this case we just return err
8989
if content.Spec.VolumeSnapshotRef == nil {
9090
// content is not bound
9191
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: VolumeSnapshotContent is not bound to any VolumeSnapshot", content.Name)
9292
ctrl.eventRecorder.Event(content, v1.EventTypeWarning, "SnapshotContentNotBound", "VolumeSnapshotContent is not bound to any VolumeSnapshot")
9393
return fmt.Errorf("volumeSnapshotContent %s is not bound to any VolumeSnapshot", content.Name)
94+
}
95+
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: content is bound to snapshot %s", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
96+
// The VolumeSnapshotContent is reserved for a VolumeSnapshot;
97+
// that VolumeSnapshot has not yet been bound to this VolumeSnapshotContent; the VolumeSnapshot sync will handle it.
98+
if content.Spec.VolumeSnapshotRef.UID == "" {
99+
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: VolumeSnapshotContent is pre-bound to VolumeSnapshot %s", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
100+
return nil
101+
}
102+
// Get the VolumeSnapshot by _name_
103+
var snapshot *crdv1.VolumeSnapshot
104+
snapshotName := snapshotRefKey(content.Spec.VolumeSnapshotRef)
105+
obj, found, err := ctrl.snapshotStore.GetByKey(snapshotName)
106+
if err != nil {
107+
return err
108+
}
109+
if !found {
110+
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: snapshot %s not found", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
111+
// Fall through with snapshot = nil
94112
} else {
95-
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: content is bound to snapshot %s", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
96-
// The VolumeSnapshotContent is reserved for a VolumeSnapshot;
97-
// that VolumeSnapshot has not yet been bound to this VolumeSnapshotContent; the VolumeSnapshot sync will handle it.
98-
if content.Spec.VolumeSnapshotRef.UID == "" {
99-
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: VolumeSnapshotContent is pre-bound to VolumeSnapshot %s", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
100-
return nil
101-
}
102-
// Get the VolumeSnapshot by _name_
103-
var snapshot *crdv1.VolumeSnapshot
104-
snapshotName := snapshotRefKey(content.Spec.VolumeSnapshotRef)
105-
obj, found, err := ctrl.snapshotStore.GetByKey(snapshotName)
106-
if err != nil {
107-
return err
108-
}
109-
if !found {
110-
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: snapshot %s not found", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
111-
// Fall through with snapshot = nil
112-
} else {
113-
var ok bool
114-
snapshot, ok = obj.(*crdv1.VolumeSnapshot)
115-
if !ok {
116-
return fmt.Errorf("cannot convert object from snapshot cache to snapshot %q!?: %#v", content.Name, obj)
117-
}
118-
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: snapshot %s found", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
119-
}
120-
if snapshot != nil && snapshot.UID != content.Spec.VolumeSnapshotRef.UID {
121-
// The snapshot that the content was pointing to was deleted, and another
122-
// with the same name created.
123-
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))
124-
// Treat the volume as bound to a missing claim.
125-
snapshot = nil
126-
}
127-
if snapshot == nil {
128-
ctrl.deleteSnapshotContent(content)
113+
var ok bool
114+
snapshot, ok = obj.(*crdv1.VolumeSnapshot)
115+
if !ok {
116+
return fmt.Errorf("cannot convert object from snapshot cache to snapshot %q!?: %#v", content.Name, obj)
129117
}
118+
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: snapshot %s found", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
130119
}
120+
if snapshot != nil && snapshot.UID != content.Spec.VolumeSnapshotRef.UID {
121+
// The snapshot that the content was pointing to was deleted, and another
122+
// with the same name created.
123+
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))
124+
// Treat the content as bound to a missing snapshot.
125+
snapshot = nil
126+
}
127+
if snapshot == nil {
128+
ctrl.deleteSnapshotContent(content)
129+
}
130+
131131
return nil
132132
}
133133

@@ -506,19 +506,20 @@ func (ctrl *csiSnapshotController) createSnapshotOperation(snapshot *crdv1.Volum
506506
}
507507
// Create VolumeSnapshotContent in the database
508508
volumeRef, err := ref.GetReference(scheme.Scheme, volume)
509+
if err != nil {
510+
return nil, err
511+
}
512+
snapshotRef, err := ref.GetReference(scheme.Scheme, snapshot)
513+
if err != nil {
514+
return nil, err
515+
}
509516

510517
snapshotContent := &crdv1.VolumeSnapshotContent{
511518
ObjectMeta: metav1.ObjectMeta{
512519
Name: contentName,
513520
},
514521
Spec: crdv1.VolumeSnapshotContentSpec{
515-
VolumeSnapshotRef: &v1.ObjectReference{
516-
Kind: "VolumeSnapshot",
517-
Namespace: snapshot.Namespace,
518-
Name: snapshot.Name,
519-
UID: snapshot.UID,
520-
APIVersion: "snapshot.storage.k8s.io/v1alpha1",
521-
},
522+
VolumeSnapshotRef: snapshotRef,
522523
PersistentVolumeRef: volumeRef,
523524
VolumeSnapshotSource: crdv1.VolumeSnapshotSource{
524525
CSI: &crdv1.CSIVolumeSnapshotSource{

‎pkg/controller/snapshot_controller_base.go‎

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -271,20 +271,12 @@ func (ctrl *csiSnapshotController) contentWorker() {
271271
return false
272272
}
273273
content, err := ctrl.contentLister.Get(name)
274+
// The content still exists in informer cache, the event must have
275+
// been add/update/sync
274276
if err == nil {
275-
// Skip update if content is for another CSI driver
276-
snapshotClassName := content.Spec.VolumeSnapshotClassName
277-
if snapshotClassName != nil {
278-
if snapshotClass, err := ctrl.classLister.Get(*snapshotClassName); err == nil {
279-
if snapshotClass.Snapshotter != ctrl.snapshotterName {
280-
return false
281-
}
282-
}
277+
if ctrl.isDriverMatch(content) {
278+
ctrl.updateContent(content)
283279
}
284-
285-
// The content still exists in informer cache, the event must have
286-
// been add/update/sync
287-
ctrl.updateContent(content)
288280
return false
289281
}
290282
if !errors.IsNotFound(err) {
@@ -322,6 +314,27 @@ func (ctrl *csiSnapshotController) contentWorker() {
322314
}
323315
}
324316

317+
// verify whether the driver specified in VolumeSnapshotContent matches the controller's driver name
318+
func (ctrl *csiSnapshotController) isDriverMatch(content *crdv1.VolumeSnapshotContent) bool {
319+
if content.Spec.VolumeSnapshotSource.CSI == nil {
320+
// Skip this snapshot content if it not a CSI snapshot
321+
return false
322+
}
323+
if content.Spec.VolumeSnapshotSource.CSI.Driver != ctrl.snapshotterName {
324+
// Skip this snapshot content if the driver does not match
325+
return false
326+
}
327+
snapshotClassName := content.Spec.VolumeSnapshotClassName
328+
if snapshotClassName != nil {
329+
if snapshotClass, err := ctrl.classLister.Get(*snapshotClassName); err == nil {
330+
if snapshotClass.Snapshotter != ctrl.snapshotterName {
331+
return false
332+
}
333+
}
334+
}
335+
return true
336+
}
337+
325338
// checkAndUpdateSnapshotClass gets the VolumeSnapshotClass from VolumeSnapshot. If it is not set,
326339
// gets it from default VolumeSnapshotClass and sets it. It also detects if snapshotter in the
327340
// VolumeSnapshotClass is the same as the snapshotter in external controller.

‎pkg/controller/snapshot_delete_test.go‎

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ var snapshotClasses = []*crdv1.VolumeSnapshotClass{
118118
func TestDeleteSync(t *testing.T) {
119119
tests := []controllerTest{
120120
{
121-
name: "1-1 - successful delete with empty snapshot class",
121+
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",
122122
initialContents: newContentArray("content1-1", classEmpty, "sid1-1", "vuid1-1", "volume1-1", "snapuid1-1", "snap1-1", nil, nil),
123123
expectedContents: nocontents,
124124
initialSnapshots: nosnapshots,
@@ -128,6 +128,17 @@ func TestDeleteSync(t *testing.T) {
128128
expectedDeleteCalls: []deleteCall{{"sid1-1", nil, nil}},
129129
test: testSyncContent,
130130
},
131+
{
132+
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",
133+
initialContents: newContentArray("content2-1", classEmpty, "sid2-1", "vuid2-1", "volume2-1", "", "snap2-1", nil, nil),
134+
expectedContents: newContentArray("content2-1", classEmpty, "sid2-1", "vuid2-1", "volume2-1", "", "snap2-1", nil, nil),
135+
initialSnapshots: nosnapshots,
136+
expectedSnapshots: nosnapshots,
137+
expectedEvents: noevents,
138+
errors: noerrors,
139+
expectedDeleteCalls: []deleteCall{{"sid2-1", nil, nil}},
140+
test: testSyncContent,
141+
},
131142
{
132143
name: "1-2 - successful delete with snapshot class that has empty secret parameter",
133144
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) {
222233
reactor.lock.Unlock()
223234
}),
224235
},
236+
{
237+
name: "1-9 - content will not be deleted if it is bound to a snapshot correctly, snapshot uid is specified",
238+
initialContents: newContentArray("content1-9", validSecretClass, "sid1-9", "vuid1-9", "volume1-9", "snapuid1-9", "snap1-9", nil, nil),
239+
expectedContents: newContentArray("content1-9", validSecretClass, "sid1-9", "vuid1-9", "volume1-9", "snapuid1-9", "snap1-9", nil, nil),
240+
initialSnapshots: newSnapshotArray("snap1-9", validSecretClass, "content1-9", "snapuid1-9", "claim1-9", false, nil, nil, nil),
241+
expectedSnapshots: newSnapshotArray("snap1-9", validSecretClass, "content1-9", "snapuid1-9", "claim1-9", false, nil, nil, nil),
242+
expectedEvents: noevents,
243+
initialSecrets: []*v1.Secret{secret()},
244+
errors: noerrors,
245+
test: testSyncContent,
246+
},
247+
{
248+
name: "1-10 - should delete content which is bound to a snapshot incorrectly",
249+
initialContents: newContentArray("content1-10", validSecretClass, "sid1-10", "vuid1-10", "volume1-10", "snapuid1-10-x", "snap1-10", nil, nil),
250+
expectedContents: nocontents,
251+
initialSnapshots: newSnapshotArray("snap1-10", validSecretClass, "content1-10", "snapuid1-10", "claim1-10", false, nil, nil, nil),
252+
expectedSnapshots: newSnapshotArray("snap1-10", validSecretClass, "content1-10", "snapuid1-10", "claim1-10", false, nil, nil, nil),
253+
expectedEvents: noevents,
254+
initialSecrets: []*v1.Secret{secret()},
255+
errors: noerrors,
256+
expectedDeleteCalls: []deleteCall{{"sid1-10", map[string]string{"foo": "bar"}, nil}},
257+
test: testSyncContent,
258+
},
259+
{
260+
name: "1-11 - content will not be deleted if it is bound to a snapshot correctly, snapsht uid is not specified",
261+
initialContents: newContentArray("content1-11", validSecretClass, "sid1-11", "vuid1-11", "volume1-11", "", "snap1-11", nil, nil),
262+
expectedContents: newContentArray("content1-11", validSecretClass, "sid1-11", "vuid1-11", "volume1-11", "", "snap1-11", nil, nil),
263+
initialSnapshots: newSnapshotArray("snap1-11", validSecretClass, "content1-11", "snapuid1-11", "claim1-11", false, nil, nil, nil),
264+
expectedSnapshots: newSnapshotArray("snap1-11", validSecretClass, "content1-11", "snapuid1-11", "claim1-11", false, nil, nil, nil),
265+
expectedEvents: noevents,
266+
initialSecrets: []*v1.Secret{secret()},
267+
errors: noerrors,
268+
test: testSyncContent,
269+
},
225270
}
226271
runSyncTests(t, tests, snapshotClasses)
227272
}

‎pkg/controller/snapshot_ready_test.go‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,26 @@ func TestSync(t *testing.T) {
232232
errors: noerrors,
233233
test: testSyncSnapshot,
234234
},
235+
{
236+
name: "3-5 - snapshot bound to content in which the driver does not match",
237+
initialContents: newContentWithUnmatchDriverArray("content3-5", validSecretClass, "sid3-5", "vuid3-5", "volume3-5", "", "snap3-5", nil, nil),
238+
expectedContents: nocontents,
239+
initialSnapshots: newSnapshotArray("snap3-5", validSecretClass, "content3-5", "snapuid3-5", "claim3-5", false, nil, nil, nil),
240+
expectedSnapshots: newSnapshotArray("snap3-5", validSecretClass, "content3-5", "snapuid3-5", "claim3-5", false, newVolumeError("VolumeSnapshotContent is missing"), nil, nil),
241+
expectedEvents: []string{"Warning SnapshotContentMissing"},
242+
errors: noerrors,
243+
test: testSyncSnapshotError,
244+
},
245+
{
246+
name: "3-6 - snapshot bound to content in which the snapshot uid does not match",
247+
initialContents: newContentArray("content3-4", validSecretClass, "sid3-4", "vuid3-4", "volume3-4", "snapuid3-4-x", "snap3-6", nil, nil),
248+
expectedContents: newContentArray("content3-4", validSecretClass, "sid3-4", "vuid3-4", "volume3-4", "snapuid3-4-x", "snap3-6", nil, nil),
249+
initialSnapshots: newSnapshotArray("snap3-5", validSecretClass, "content3-5", "snapuid3-5", "claim3-5", false, nil, nil, nil),
250+
expectedSnapshots: newSnapshotArray("snap3-5", validSecretClass, "content3-5", "snapuid3-5", "claim3-5", false, newVolumeError("VolumeSnapshotContent is missing"), nil, nil),
251+
expectedEvents: []string{"Warning SnapshotContentMissing"},
252+
errors: noerrors,
253+
test: testSyncSnapshotError,
254+
},
235255
}
236256

237257
runSyncTests(t, tests, snapshotClasses)

0 commit comments

Comments
 (0)