Skip to content

Commit 4b2714a

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #266 from ggriffiths/add_more_sidecar_ctrl_tests
Add more sidecar tests and cleanup sidecar code
2 parents 3dd4e07 + fe1c355 commit 4b2714a

File tree

4 files changed

+210
-47
lines changed

4 files changed

+210
-47
lines changed

‎pkg/sidecar-controller/content_create_test.go‎

Lines changed: 155 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -17,32 +17,168 @@ limitations under the License.
1717
package sidecar_controller
1818

1919
import (
20+
"errors"
2021
"testing"
2122
"time"
23+
24+
crdv1 "github.com/kubernetes-csi/external-snapshotter/v2/pkg/apis/volumesnapshot/v1beta1"
25+
"github.com/kubernetes-csi/external-snapshotter/v2/pkg/utils"
26+
v1 "k8s.io/api/core/v1"
2227
)
2328

2429
func TestSyncContent(t *testing.T) {
25-
var tests []controllerTest
26-
27-
tests = append(tests, controllerTest{
28-
name: "Basic content update ready to use",
29-
initialContents: newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "volume-handle-1-1", retainPolicy, nil, &defaultSize, &False, true),
30-
expectedContents: newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "volume-handle-1-1", retainPolicy, nil, &defaultSize, &True, true),
31-
expectedEvents: noevents,
32-
expectedCreateCalls: []createCall{
33-
{
34-
volumeHandle: "volume-handle-1-1",
35-
snapshotName: "snapshot-snapuid1-1",
36-
driverName: mockDriverName,
37-
snapshotId: "snapuid1-1",
38-
creationTime: timeNow,
39-
readyToUse: true,
30+
31+
tests := []controllerTest{
32+
{
33+
name: "1-1: Basic content update ready to use",
34+
initialContents: newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "volume-handle-1-1", retainPolicy, nil, &defaultSize, &False, true),
35+
expectedContents: newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "volume-handle-1-1", retainPolicy, nil, &defaultSize, &True, true),
36+
expectedEvents: noevents,
37+
expectedCreateCalls: []createCall{
38+
{
39+
volumeHandle: "volume-handle-1-1",
40+
snapshotName: "snapshot-snapuid1-1",
41+
driverName: mockDriverName,
42+
snapshotId: "snapuid1-1",
43+
creationTime: timeNow,
44+
readyToUse: true,
45+
},
46+
},
47+
expectedListCalls: []listCall{{"sid1-1", map[string]string{}, true, time.Now(), 1, nil}},
48+
errors: noerrors,
49+
test: testSyncContent,
50+
},
51+
{
52+
name: "1-2: Basic sync content create snapshot",
53+
initialContents: withContentStatus(newContentArray("content1-2", "snapuid1-2", "snap1-2", "sid1-2", defaultClass, "", "volume-handle-1-2", retainPolicy, nil, &defaultSize, true),
54+
nil),
55+
expectedContents: withContentStatus(newContentArray("content1-2", "snapuid1-2", "snap1-2", "sid1-2", defaultClass, "", "volume-handle-1-2", retainPolicy, nil, &defaultSize, true),
56+
&crdv1.VolumeSnapshotContentStatus{SnapshotHandle: toStringPointer("snapuid1-2"), RestoreSize: &defaultSize, ReadyToUse: &True}),
57+
expectedEvents: noevents,
58+
expectedCreateCalls: []createCall{
59+
{
60+
volumeHandle: "volume-handle-1-2",
61+
snapshotName: "snapshot-snapuid1-2",
62+
driverName: mockDriverName,
63+
snapshotId: "snapuid1-2",
64+
creationTime: timeNow,
65+
readyToUse: true,
66+
size: defaultSize,
67+
},
68+
},
69+
expectedListCalls: []listCall{{"sid1-2", map[string]string{}, true, time.Now(), 1, nil}},
70+
errors: noerrors,
71+
test: testSyncContent,
72+
},
73+
{
74+
name: "1-3: Basic sync content create snapshot with non-existent secret",
75+
initialContents: withContentAnnotations(withContentStatus(newContentArray("content1-3", "snapuid1-3", "snap1-3", "sid1-3", invalidSecretClass, "", "volume-handle-1-3", retainPolicy, nil, &defaultSize, true),
76+
nil), map[string]string{
77+
utils.AnnDeletionSecretRefName: "",
78+
utils.AnnDeletionSecretRefNamespace: "",
79+
}),
80+
expectedContents: withContentAnnotations(withContentStatus(newContentArray("content1-3", "snapuid1-3", "snap1-3", "sid1-3", invalidSecretClass, "", "volume-handle-1-3", retainPolicy, nil, &defaultSize, true),
81+
&crdv1.VolumeSnapshotContentStatus{
82+
SnapshotHandle: nil,
83+
RestoreSize: nil,
84+
ReadyToUse: &False,
85+
Error: newSnapshotError("Failed to create snapshot: failed to get input parameters to create snapshot for content content1-3: \"cannot retrieve secrets for snapshot content \\\"content1-3\\\", err: secret name or namespace not specified\""),
86+
}), map[string]string{
87+
utils.AnnDeletionSecretRefName: "",
88+
utils.AnnDeletionSecretRefNamespace: "",
89+
}), initialSecrets: []*v1.Secret{}, // no initial secret created
90+
expectedEvents: []string{"Warning SnapshotCreationFailed"},
91+
errors: noerrors,
92+
test: testSyncContent,
93+
},
94+
{
95+
name: "1-4: Basic sync content create snapshot with valid secret",
96+
initialContents: withContentAnnotations(withContentStatus(newContentArray("content1-4", "snapuid1-4", "snap1-4", "sid1-4", validSecretClass, "", "volume-handle-1-4", retainPolicy, nil, &defaultSize, true),
97+
nil), map[string]string{
98+
utils.AnnDeletionSecretRefName: "secret",
99+
utils.AnnDeletionSecretRefNamespace: "default",
100+
}),
101+
expectedContents: withContentAnnotations(withContentStatus(newContentArray("content1-4", "snapuid1-4", "snap1-4", "sid1-4", validSecretClass, "", "volume-handle-1-4", retainPolicy, nil, &defaultSize, true),
102+
&crdv1.VolumeSnapshotContentStatus{
103+
SnapshotHandle: toStringPointer("snapuid1-4"),
104+
RestoreSize: &defaultSize,
105+
ReadyToUse: &True,
106+
Error: nil,
107+
}), map[string]string{
108+
utils.AnnDeletionSecretRefName: "secret",
109+
utils.AnnDeletionSecretRefNamespace: "default",
110+
}),
111+
expectedCreateCalls: []createCall{
112+
{
113+
volumeHandle: "volume-handle-1-4",
114+
snapshotName: "snapshot-snapuid1-4",
115+
parameters: class5Parameters,
116+
secrets: map[string]string{
117+
"foo": "bar",
118+
},
119+
driverName: mockDriverName,
120+
snapshotId: "snapuid1-4",
121+
creationTime: timeNow,
122+
readyToUse: true,
123+
size: defaultSize,
124+
},
125+
},
126+
initialSecrets: []*v1.Secret{secret()},
127+
expectedEvents: noevents,
128+
errors: noerrors,
129+
test: testSyncContent,
130+
},
131+
{
132+
name: "1-5: Basic sync content create snapshot with failed secret call",
133+
initialContents: withContentAnnotations(withContentStatus(newContentArray("content1-5", "snapuid1-5", "snap1-5", "sid1-5", invalidSecretClass, "", "volume-handle-1-5", retainPolicy, nil, &defaultSize, true),
134+
nil), map[string]string{
135+
utils.AnnDeletionSecretRefName: "secret",
136+
utils.AnnDeletionSecretRefNamespace: "default",
137+
}),
138+
expectedContents: withContentAnnotations(withContentStatus(newContentArray("content1-5", "snapuid1-5", "snap1-5", "sid1-5", invalidSecretClass, "", "volume-handle-1-5", retainPolicy, nil, &defaultSize, true),
139+
&crdv1.VolumeSnapshotContentStatus{
140+
SnapshotHandle: nil,
141+
RestoreSize: nil,
142+
ReadyToUse: &False,
143+
Error: newSnapshotError("Failed to create snapshot: failed to get input parameters to create snapshot for content content1-5: \"cannot get credentials for snapshot content \\\"content1-5\\\"\""),
144+
}), map[string]string{
145+
utils.AnnDeletionSecretRefName: "secret",
146+
utils.AnnDeletionSecretRefNamespace: "default",
147+
}), initialSecrets: []*v1.Secret{}, // no initial secret created
148+
expectedEvents: []string{"Warning SnapshotCreationFailed"},
149+
errors: []reactorError{
150+
// Inject error to the first client.VolumesnapshotV1beta1().VolumeSnapshots().Update call.
151+
// All other calls will succeed.
152+
{"get", "secrets", errors.New("mock secrets error")},
153+
},
154+
test: testSyncContent,
155+
},
156+
{
157+
name: "1-6: Basic content update ready to use bad snapshot class",
158+
initialContents: newContentArrayWithReadyToUse("content1-6", "snapuid1-6", "snap1-6", "sid1-6", "bad-class", "", "volume-handle-1-6", retainPolicy, nil, &defaultSize, &False, true),
159+
expectedContents: withContentStatus(newContentArray("content1-6", "snapuid1-6", "snap1-6", "sid1-6", "bad-class", "", "volume-handle-1-6", retainPolicy, nil, &defaultSize, true),
160+
&crdv1.VolumeSnapshotContentStatus{
161+
SnapshotHandle: toStringPointer("sid1-6"),
162+
RestoreSize: &defaultSize,
163+
ReadyToUse: &False,
164+
Error: newSnapshotError("Failed to check and update snapshot content: failed to get input parameters to create snapshot content1-6: \"failed to retrieve snapshot class bad-class from the informer: \\\"volumesnapshotclass.snapshot.storage.k8s.io \\\\\\\"bad-class\\\\\\\" not found\\\"\""),
165+
}),
166+
expectedEvents: []string{"Warning SnapshotContentCheckandUpdateFailed"},
167+
expectedCreateCalls: []createCall{
168+
{
169+
volumeHandle: "volume-handle-1-6",
170+
snapshotName: "snapshot-snapuid1-6",
171+
driverName: mockDriverName,
172+
snapshotId: "snapuid1-6",
173+
creationTime: timeNow,
174+
readyToUse: true,
175+
},
40176
},
177+
expectedListCalls: []listCall{{"sid1-6", map[string]string{}, true, time.Now(), 1, nil}},
178+
errors: noerrors,
179+
test: testSyncContent,
41180
},
42-
expectedListCalls: []listCall{{"sid1-1", map[string]string{}, true, time.Now(), 1, nil}},
43-
errors: noerrors,
44-
test: testSyncContent,
45-
})
181+
}
46182

47183
runSyncContentTests(t, tests, snapshotClasses)
48184
}

‎pkg/sidecar-controller/framework_test.go‎

Lines changed: 43 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,7 @@ var noerrors = []reactorError{}
123123
// the list.
124124
type snapshotReactor struct {
125125
secrets map[string]*v1.Secret
126+
snapshotClasses map[string]*crdv1.VolumeSnapshotClass
126127
contents map[string]*crdv1.VolumeSnapshotContent
127128
changedObjects []interface{}
128129
changedSinceLastSync int
@@ -283,7 +284,12 @@ func (r *snapshotReactor) checkContents(expectedContents []*crdv1.VolumeSnapshot
283284
v := v.DeepCopy()
284285
v.ResourceVersion = ""
285286
v.Spec.VolumeSnapshotRef.ResourceVersion = ""
286-
v.Status.CreationTime = nil
287+
if v.Status != nil {
288+
v.Status.CreationTime = nil
289+
}
290+
if v.Status.Error != nil {
291+
v.Status.Error.Time = &metav1.Time{}
292+
}
287293
expectedMap[v.Name] = v
288294
}
289295
for _, v := range r.contents {
@@ -292,7 +298,13 @@ func (r *snapshotReactor) checkContents(expectedContents []*crdv1.VolumeSnapshot
292298
v := v.DeepCopy()
293299
v.ResourceVersion = ""
294300
v.Spec.VolumeSnapshotRef.ResourceVersion = ""
295-
v.Status.CreationTime = nil
301+
if v.Status != nil {
302+
v.Status.CreationTime = nil
303+
if v.Status.Error != nil {
304+
v.Status.Error.Time = &metav1.Time{}
305+
}
306+
}
307+
296308
gotMap[v.Name] = v
297309
}
298310
if !reflect.DeepEqual(expectedMap, gotMap) {
@@ -487,6 +499,7 @@ func (r *snapshotReactor) modifyContentEvent(content *crdv1.VolumeSnapshotConten
487499
func newSnapshotReactor(kubeClient *kubefake.Clientset, client *fake.Clientset, ctrl *csiSnapshotSideCarController, fakeVolumeWatch, fakeClaimWatch *watch.FakeWatcher, errors []reactorError) *snapshotReactor {
488500
reactor := &snapshotReactor{
489501
secrets: make(map[string]*v1.Secret),
502+
snapshotClasses: make(map[string]*crdv1.VolumeSnapshotClass),
490503
contents: make(map[string]*crdv1.VolumeSnapshotContent),
491504
ctrl: ctrl,
492505
fakeContentWatch: fakeVolumeWatch,
@@ -497,6 +510,7 @@ func newSnapshotReactor(kubeClient *kubefake.Clientset, client *fake.Clientset,
497510
client.AddReactor("update", "volumesnapshotcontents", reactor.React)
498511
client.AddReactor("get", "volumesnapshotcontents", reactor.React)
499512
client.AddReactor("delete", "volumesnapshotcontents", reactor.React)
513+
kubeClient.AddReactor("get", "secrets", reactor.React)
500514

501515
return reactor
502516
}
@@ -615,16 +629,6 @@ func newContentArrayWithReadyToUse(contentName, boundToSnapshotUID, boundToSnaps
615629
}
616630
}
617631

618-
func newContentWithUnmatchDriverArray(contentName, boundToSnapshotUID, boundToSnapshotName, snapshotHandle, snapshotClassName, desiredSnapshotHandle, volumeHandle string,
619-
deletionPolicy crdv1.DeletionPolicy, size, creationTime *int64,
620-
withFinalizer bool) []*crdv1.VolumeSnapshotContent {
621-
content := newContent(contentName, boundToSnapshotUID, boundToSnapshotName, snapshotHandle, snapshotClassName, desiredSnapshotHandle, volumeHandle, deletionPolicy, size, creationTime, withFinalizer, nil)
622-
content.Spec.Driver = "fake"
623-
return []*crdv1.VolumeSnapshotContent{
624-
content,
625-
}
626-
}
627-
628632
func newContentArrayWithDeletionTimestamp(contentName, boundToSnapshotUID, boundToSnapshotName, snapshotHandle, snapshotClassName, desiredSnapshotHandle, volumeHandle string,
629633
deletionPolicy crdv1.DeletionPolicy, size, creationTime *int64,
630634
withFinalizer bool, deletionTime *metav1.Time) []*crdv1.VolumeSnapshotContent {
@@ -633,6 +637,22 @@ func newContentArrayWithDeletionTimestamp(contentName, boundToSnapshotUID, bound
633637
}
634638
}
635639

640+
func withContentStatus(content []*crdv1.VolumeSnapshotContent, status *crdv1.VolumeSnapshotContentStatus) []*crdv1.VolumeSnapshotContent {
641+
for i := range content {
642+
content[i].Status = status
643+
}
644+
645+
return content
646+
}
647+
648+
func withContentAnnotations(content []*crdv1.VolumeSnapshotContent, annotations map[string]string) []*crdv1.VolumeSnapshotContent {
649+
for i := range content {
650+
content[i].ObjectMeta.Annotations = annotations
651+
}
652+
653+
return content
654+
}
655+
636656
func testSyncContent(ctrl *csiSnapshotSideCarController, reactor *snapshotReactor, test controllerTest) error {
637657
return ctrl.syncContent(test.initialContents[0])
638658
}
@@ -742,6 +762,7 @@ func runSyncContentTests(t *testing.T, tests []controllerTest, snapshotClasses [
742762
indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})
743763
for _, class := range snapshotClasses {
744764
indexer.Add(class)
765+
reactor.snapshotClasses[class.Name] = class
745766
}
746767
ctrl.classLister = storagelisters.NewVolumeSnapshotClassLister(indexer)
747768

@@ -875,7 +896,7 @@ func (f *fakeSnapshotter) CreateSnapshot(ctx context.Context, snapshotName strin
875896
err = fmt.Errorf("unexpected create snapshot call")
876897
}
877898

878-
if !reflect.DeepEqual(call.secrets, snapshotterCredentials) {
899+
if !reflect.DeepEqual(call.secrets, snapshotterCredentials) && !(len(call.secrets) == 0 && len(snapshotterCredentials) == 0) {
879900
f.t.Errorf("Wrong CSI Create Snapshot call: snapshotName=%s, volumeHandle=%s, expected secrets %+v, got %+v", snapshotName, volumeHandle, call.secrets, snapshotterCredentials)
880901
err = fmt.Errorf("unexpected create snapshot call")
881902
}
@@ -937,3 +958,12 @@ func (f *fakeSnapshotter) GetSnapshotStatus(ctx context.Context, snapshotID stri
937958

938959
return call.readyToUse, call.createTime, call.size, call.err
939960
}
961+
962+
func newSnapshotError(message string) *crdv1.VolumeSnapshotError {
963+
return &crdv1.VolumeSnapshotError{
964+
Time: &metav1.Time{},
965+
Message: &message,
966+
}
967+
}
968+
969+
func toStringPointer(str string) *string { return &str }

‎pkg/sidecar-controller/snapshot_controller.go‎

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,6 @@ const controllerUpdateFailMsg = "snapshot controller failed to update"
5555
func (ctrl *csiSnapshotSideCarController) syncContent(content *crdv1.VolumeSnapshotContent) error {
5656
klog.V(5).Infof("synchronizing VolumeSnapshotContent[%s]", content.Name)
5757

58-
var err error
5958
if ctrl.shouldDelete(content) {
6059
klog.V(4).Infof("VolumeSnapshotContent[%s]: the policy is %s", content.Name, content.Spec.DeletionPolicy)
6160
if content.Spec.DeletionPolicy == crdv1.VolumeSnapshotContentDelete &&
@@ -75,10 +74,7 @@ func (ctrl *csiSnapshotSideCarController) syncContent(content *crdv1.VolumeSnaps
7574
} else {
7675
if content.Spec.Source.VolumeHandle != nil && content.Status == nil {
7776
klog.V(5).Infof("syncContent: Call CreateSnapshot for content %s", content.Name)
78-
if err = ctrl.createSnapshot(content); err != nil {
79-
ctrl.updateContentErrorStatusWithEvent(content, v1.EventTypeWarning, "SnapshotCreationFailed", fmt.Sprintf("Failed to create snapshot with error %v", err))
80-
return err
81-
}
77+
ctrl.createSnapshot(content)
8278
} else {
8379
// Skip checkandUpdateContentStatus() if ReadyToUse is
8480
// already true. We don't want to keep calling CreateSnapshot
@@ -87,10 +83,7 @@ func (ctrl *csiSnapshotSideCarController) syncContent(content *crdv1.VolumeSnaps
8783
if content.Status != nil && content.Status.ReadyToUse != nil && *content.Status.ReadyToUse == true {
8884
return nil
8985
}
90-
if err = ctrl.checkandUpdateContentStatus(content); err != nil {
91-
ctrl.updateContentErrorStatusWithEvent(content, v1.EventTypeWarning, "SnapshotContentStatusUpdateFailed", fmt.Sprintf("Failed to update snapshot content status with error %v", err))
92-
return err
93-
}
86+
ctrl.checkandUpdateContentStatus(content)
9487
}
9588
}
9689
return nil
@@ -129,7 +122,7 @@ func (ctrl *csiSnapshotSideCarController) storeContentUpdate(content interface{}
129122
}
130123

131124
// createSnapshot starts new asynchronous operation to create snapshot
132-
func (ctrl *csiSnapshotSideCarController) createSnapshot(content *crdv1.VolumeSnapshotContent) error {
125+
func (ctrl *csiSnapshotSideCarController) createSnapshot(content *crdv1.VolumeSnapshotContent) {
133126
klog.V(5).Infof("createSnapshot for content [%s]: started", content.Name)
134127
opName := fmt.Sprintf("create-%s", content.Name)
135128
ctrl.scheduleOperation(opName, func() error {
@@ -147,10 +140,9 @@ func (ctrl *csiSnapshotSideCarController) createSnapshot(content *crdv1.VolumeSn
147140
}
148141
return nil
149142
})
150-
return nil
151143
}
152144

153-
func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatus(content *crdv1.VolumeSnapshotContent) error {
145+
func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatus(content *crdv1.VolumeSnapshotContent) {
154146
klog.V(5).Infof("checkandUpdateContentStatus[%s] started", content.Name)
155147
opName := fmt.Sprintf("check-%s", content.Name)
156148
ctrl.scheduleOperation(opName, func() error {
@@ -168,7 +160,6 @@ func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatus(content *c
168160

169161
return nil
170162
})
171-
return nil
172163
}
173164

174165
// updateContentStatusWithEvent saves new content.Status to API server and emits

‎pkg/sidecar-controller/snapshot_delete_test.go‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,11 @@ var class6Parameters = map[string]string{
6868
utils.PrefixedSnapshotterListSecretNamespaceKey: "default",
6969
}
7070

71+
var class7Annotations = map[string]string{
72+
utils.AnnDeletionSecretRefName: "secret-x",
73+
utils.AnnDeletionSecretRefNamespace: "default-x",
74+
}
75+
7176
var snapshotClasses = []*crdv1.VolumeSnapshotClass{
7277
{
7378
TypeMeta: metav1.TypeMeta{
@@ -107,10 +112,11 @@ var snapshotClasses = []*crdv1.VolumeSnapshotClass{
107112
Kind: "VolumeSnapshotClass",
108113
},
109114
ObjectMeta: metav1.ObjectMeta{
110-
Name: invalidSecretClass,
115+
Name: invalidSecretClass,
116+
Annotations: class7Annotations,
111117
},
112118
Driver: mockDriverName,
113-
Parameters: class3Parameters,
119+
Parameters: class2Parameters,
114120
DeletionPolicy: crdv1.VolumeSnapshotContentDelete,
115121
},
116122
{

0 commit comments

Comments
 (0)