Skip to content

Commit f1f0c90

Browse files
yuxiangqianyuxiangqian
committed
revamp find content logic to fix 290 291 292
add snapshot source validation in syncSnapshot add content source validation in syncContent (cherry picked from commit 45324b7)
1 parent a2d5c6e commit f1f0c90

File tree

7 files changed

+726
-437
lines changed

7 files changed

+726
-437
lines changed

‎pkg/common-controller/framework_test.go‎

Lines changed: 10 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -683,7 +683,7 @@ func (r *snapshotReactor) modifyContentEvent(content *crdv1.VolumeSnapshotConten
683683
}
684684
}
685685

686-
// addSnapshotEvent simulates that a snapshot has been deleted in etcd and the
686+
// addSnapshotEvent simulates that a snapshot has been created in etcd and the
687687
// controller receives 'snapshot added' event.
688688
func (r *snapshotReactor) addSnapshotEvent(snapshot *crdv1.VolumeSnapshot) {
689689
r.lock.Lock()
@@ -795,13 +795,11 @@ func newContent(contentName, boundToSnapshotUID, boundToSnapshotName, snapshotHa
795795
}
796796

797797
if volumeHandle != "" {
798-
content.Spec.Source = crdv1.VolumeSnapshotContentSource{
799-
VolumeHandle: &volumeHandle,
800-
}
801-
} else if desiredSnapshotHandle != "" {
802-
content.Spec.Source = crdv1.VolumeSnapshotContentSource{
803-
SnapshotHandle: &desiredSnapshotHandle,
804-
}
798+
content.Spec.Source.VolumeHandle = &volumeHandle
799+
}
800+
801+
if desiredSnapshotHandle != "" {
802+
content.Spec.Source.SnapshotHandle = &desiredSnapshotHandle
805803
}
806804

807805
if boundToSnapshotName != "" {
@@ -916,13 +914,10 @@ func newSnapshot(
916914
}
917915

918916
if pvcName != "" {
919-
snapshot.Spec.Source = crdv1.VolumeSnapshotSource{
920-
PersistentVolumeClaimName: &pvcName,
921-
}
922-
} else if targetContentName != "" {
923-
snapshot.Spec.Source = crdv1.VolumeSnapshotSource{
924-
VolumeSnapshotContentName: &targetContentName,
925-
}
917+
snapshot.Spec.Source.PersistentVolumeClaimName = &pvcName
918+
}
919+
if targetContentName != "" {
920+
snapshot.Spec.Source.VolumeSnapshotContentName = &targetContentName
926921
}
927922
if withAllFinalizers {
928923
return withSnapshotFinalizers([]*crdv1.VolumeSnapshot{&snapshot}, utils.VolumeSnapshotAsSourceFinalizer, utils.VolumeSnapshotBoundFinalizer)[0]

‎pkg/common-controller/snapshot_controller.go‎

Lines changed: 319 additions & 259 deletions
Large diffs are not rendered by default.

‎pkg/common-controller/snapshot_create_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -110,8 +110,8 @@ func TestCreateSnapshotSync(t *testing.T) {
110110

111111
{
112112
name: "7-2 - fail to update snapshot reports warning event",
113-
initialContents: newContentArrayWithReadyToUse("snapcontent-snapuid7-2", "snapuid7-2", "snap7-2", "sid7-2", classGold, "sid7-2", "pv-handle7-2", deletionPolicy, nil, nil, &True, false),
114-
expectedContents: newContentArrayWithReadyToUse("snapcontent-snapuid7-2", "snapuid7-2", "snap7-2", "sid7-2", classGold, "sid7-2", "pv-handle7-2", deletionPolicy, nil, nil, &True, false),
113+
initialContents: newContentArrayWithReadyToUse("snapcontent-snapuid7-2", "snapuid7-2", "snap7-2", "sid7-2", classGold, "", "pv-handle7-2", deletionPolicy, nil, nil, &True, false),
114+
expectedContents: newContentArrayWithReadyToUse("snapcontent-snapuid7-2", "snapuid7-2", "snap7-2", "sid7-2", classGold, "", "pv-handle7-2", deletionPolicy, nil, nil, &True, false),
115115
initialSnapshots: newSnapshotArray("snap7-2", "snapuid7-2", "claim7-2", "", classGold, "snapcontent-snapuid7-2", &False, nil, nil, nil, false, true, nil),
116116
expectedSnapshots: newSnapshotArray("snap7-2", "snapuid7-2", "claim7-2", "", classGold, "snapcontent-snapuid7-2", &False, nil, nil, newVolumeError("Snapshot status update failed, snapshot controller failed to update default/snap7-2 on API server: mock update error"), false, true, nil),
117117
initialClaims: newClaimArray("claim7-2", "pvc-uid7-2", "1Gi", "volume7-2", v1.ClaimBound, &classGold),
@@ -232,7 +232,7 @@ func TestCreateSnapshotSync(t *testing.T) {
232232
initialContents: nocontents,
233233
expectedContents: nocontents,
234234
initialSnapshots: newSnapshotArray("snap7-10", "snapuid7-10", "claim7-10", "", invalidSecretClass, "", &False, nil, nil, nil, false, true, nil),
235-
expectedSnapshots: newSnapshotArray("snap7-10", "snapuid7-10", "claim7-10", "snap7-10", invalidSecretClass, "", &False, nil, nil, newVolumeError("Failed to create snapshot content with error failed to get input parameters to create snapshot snap7-10: \"failed to get name and namespace template from params: either name and namespace for Snapshotter secrets specified, Both must be specified\""), false, true, nil),
235+
expectedSnapshots: newSnapshotArray("snap7-10", "snapuid7-10", "claim7-10", "", invalidSecretClass, "", &False, nil, nil, newVolumeError("Failed to create snapshot content with error failed to get input parameters to create snapshot snap7-10: \"failed to get name and namespace template from params: either name and namespace for Snapshotter secrets specified, Both must be specified\""), false, true, nil),
236236
initialClaims: newClaimArray("claim7-10", "pvc-uid7-10", "1Gi", "volume7-10", v1.ClaimBound, &classEmpty),
237237
initialVolumes: newVolumeArray("volume7-10", "pv-uid7-10", "pv-handle7-10", "1Gi", "pvc-uid7-10", "claim7-10", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
238238
initialSecrets: []*v1.Secret{}, // no initial secret created

‎pkg/common-controller/snapshot_delete_test.go‎

Lines changed: 131 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -169,11 +169,11 @@ func TestDeleteSync(t *testing.T) {
169169
test: testSyncContent,
170170
},
171171
{
172-
name: "3-1 - content will be deleted if snapshot deletion timestamp is set",
173-
initialContents: newContentArray("content3-1", "", "snap3-1", "sid3-1", validSecretClass, "", "", deletePolicy, nil, nil, true),
172+
name: "3-1 - (dynamic) content will be deleted if snapshot deletion timestamp is set",
173+
initialContents: newContentArray("snapcontent-snapuid3-1", "snapuid3-1", "snap3-1", "sid3-1", validSecretClass, "", "volume3-1", deletePolicy, nil, nil, true),
174174
expectedContents: nocontents,
175-
initialSnapshots: newSnapshotArray("snap3-1", "snapuid3-1", "claim3-1", "", validSecretClass, "content3-1", &True, nil, nil, nil, false, true, &timeNowMetav1),
176-
expectedSnapshots: withSnapshotFinalizers(newSnapshotArray("snap3-1", "snapuid3-1", "claim3-1", "", validSecretClass, "content3-1", &True, nil, nil, nil, false, false, &timeNowMetav1),
175+
initialSnapshots: newSnapshotArray("snap3-1", "snapuid3-1", "claim3-1", "", validSecretClass, "snapcontent-snapuid3-1", &True, nil, nil, nil, false, true, &timeNowMetav1),
176+
expectedSnapshots: withSnapshotFinalizers(newSnapshotArray("snap3-1", "snapuid3-1", "claim3-1", "", validSecretClass, "snapcontent-snapuid3-1", &True, nil, nil, nil, false, false, &timeNowMetav1),
177177
utils.VolumeSnapshotBoundFinalizer,
178178
),
179179
initialClaims: newClaimArray("claim3-1", "pvc-uid3-1", "1Gi", "volume3-1", v1.ClaimBound, &classEmpty),
@@ -183,11 +183,14 @@ func TestDeleteSync(t *testing.T) {
183183
test: testSyncSnapshot,
184184
},
185185
{
186-
name: "3-2 - content will not be deleted if deletion API call fails",
187-
initialContents: newContentArray("content3-2", "", "snap3-2", "sid3-2", validSecretClass, "", "", deletePolicy, nil, nil, true),
188-
expectedContents: newContentArray("content3-2", "", "snap3-2", "sid3-2", validSecretClass, "", "", deletePolicy, nil, nil, true),
189-
initialSnapshots: newSnapshotArray("snap3-2", "snapuid3-2", "claim3-2", "", validSecretClass, "content3-2", &True, nil, nil, nil, false, true, &timeNowMetav1),
190-
expectedSnapshots: newSnapshotArray("snap3-2", "snapuid3-2", "claim3-2", "", validSecretClass, "content3-2", &True, nil, nil, nil, false, true, &timeNowMetav1),
186+
name: "3-2 - (dynamic) content will not be deleted if deletion API call fails",
187+
initialContents: newContentArray("snapcontent-snapuid3-2", "snapuid3-2", "snap3-2", "sid3-2", validSecretClass, "", "volume3-2", deletePolicy, nil, nil, true),
188+
expectedContents: withContentAnnotations(newContentArray("snapcontent-snapuid3-2", "snapuid3-2", "snap3-2", "sid3-2", validSecretClass, "", "volume3-2", deletePolicy, nil, nil, true),
189+
map[string]string{
190+
"snapshot.storage.kubernetes.io/volumesnapshot-being-deleted": "yes",
191+
}),
192+
initialSnapshots: newSnapshotArray("snap3-2", "snapuid3-2", "claim3-2", "", validSecretClass, "snapcontent-snapuid3-2", &False, nil, nil, nil, false, true, &timeNowMetav1),
193+
expectedSnapshots: newSnapshotArray("snap3-2", "snapuid3-2", "claim3-2", "", validSecretClass, "snapcontent-snapuid3-2", &False, nil, nil, nil, false, true, &timeNowMetav1),
191194
initialClaims: newClaimArray("claim3-2", "pvc-uid3-2", "1Gi", "volume3-2", v1.ClaimBound, &classEmpty),
192195
expectedEvents: []string{"Warning SnapshotContentObjectDeleteError"},
193196
initialSecrets: []*v1.Secret{secret()},
@@ -200,22 +203,135 @@ func TestDeleteSync(t *testing.T) {
200203
test: testSyncSnapshotError,
201204
},
202205
{
203-
name: "3-3 - content will not be deleted if retainPolicy is set",
204-
initialContents: newContentArray("content3-3", "", "snap3-3", "sid3-3", validSecretClass, "", "", retainPolicy, nil, nil, true),
205-
expectedContents: withContentAnnotations(newContentArray("content3-3", "", "snap3-3", "sid3-3", validSecretClass, "", "", retainPolicy, nil, nil, true),
206+
name: "3-3 - (dynamic) content will not be deleted if retainPolicy is set, snapshot should have its finalizer removed",
207+
initialContents: newContentArray("snapcontent-snapuid3-3", "snapuid3-3", "snap3-3", "sid3-3", validSecretClass, "", "volume3-3", retainPolicy, nil, nil, true),
208+
expectedContents: withContentAnnotations(newContentArray("snapcontent-snapuid3-3", "snapuid3-3", "snap3-3", "sid3-3", validSecretClass, "", "volume3-3", retainPolicy, nil, nil, true),
206209
map[string]string{
207210
"snapshot.storage.kubernetes.io/volumesnapshot-being-deleted": "yes",
208211
}),
209-
initialSnapshots: newSnapshotArray("snap3-3", "snapuid3-3", "claim3-3", "", validSecretClass, "content3-3", &True, nil, nil, nil, false, true, &timeNowMetav1),
210-
expectedSnapshots: withSnapshotFinalizers(newSnapshotArray("snap3-3", "snapuid3-3", "claim3-3", "", validSecretClass, "content3-3", &True, nil, nil, nil, false, false, &timeNowMetav1),
212+
initialSnapshots: newSnapshotArray("snap3-3", "snapuid3-3", "claim3-3", "", validSecretClass, "snapcontent-snapuid3-3", &False, nil, nil, nil, false, true, &timeNowMetav1),
213+
expectedSnapshots: newSnapshotArray("snap3-3", "snapuid3-3", "claim3-3", "", validSecretClass, "snapcontent-snapuid3-3", &False, nil, nil, nil, false, false, &timeNowMetav1),
214+
initialClaims: newClaimArray("claim3-3", "pvc-uid3-3", "1Gi", "volume3-3", v1.ClaimBound, &classEmpty),
215+
expectedEvents: noevents,
216+
initialSecrets: []*v1.Secret{secret()},
217+
errors: noerrors,
218+
test: testSyncSnapshot,
219+
},
220+
{
221+
name: "3-4 - (dynamic) snapshot should have its finalizer removed if no content has been found",
222+
initialContents: nocontents,
223+
expectedContents: nocontents,
224+
initialSnapshots: newSnapshotArray("snap3-4", "snapuid3-4", "claim3-4", "", validSecretClass, "", &False, nil, nil, nil, false, true, &timeNowMetav1),
225+
expectedSnapshots: newSnapshotArray("snap3-4", "snapuid3-4", "claim3-4", "", validSecretClass, "", &False, nil, nil, nil, false, false, &timeNowMetav1),
226+
initialClaims: newClaimArray("claim3-4", "pvc-uid3-4", "1Gi", "volume3-4", v1.ClaimBound, &classEmpty),
227+
expectedEvents: noevents,
228+
initialSecrets: []*v1.Secret{secret()},
229+
errors: noerrors,
230+
test: testSyncSnapshot,
231+
},
232+
{
233+
name: "3-5 - (dynamic) snapshot should have its finalizer removed if a content is found but points to a different snapshot - uid mismatch",
234+
initialContents: newContentArray("snapcontent-snapuid3-5", "snapuid3-5-x", "snap3-5", "sid3-5", validSecretClass, "", "volume3-5", deletePolicy, nil, nil, true),
235+
expectedContents: newContentArray("snapcontent-snapuid3-5", "snapuid3-5-x", "snap3-5", "sid3-5", validSecretClass, "", "volume3-5", deletePolicy, nil, nil, true),
236+
initialSnapshots: newSnapshotArray("snap3-5", "snapuid3-5", "claim3-5", "", validSecretClass, "snapcontent-snapuid3-5", &False, nil, nil, nil, false, true, &timeNowMetav1),
237+
expectedSnapshots: newSnapshotArray("snap3-5", "snapuid3-5", "claim3-5", "", validSecretClass, "snapcontent-snapuid3-5", &False, nil, nil, nil, false, false, &timeNowMetav1),
238+
initialClaims: newClaimArray("claim3-5", "pvc-uid3-5", "1Gi", "volume3-5", v1.ClaimBound, &classEmpty),
239+
expectedEvents: noevents,
240+
initialSecrets: []*v1.Secret{secret()},
241+
errors: noerrors,
242+
test: testSyncSnapshot,
243+
},
244+
{
245+
name: "3-6 - (dynamic) snapshot should have its finalizer removed if a content is found but points to a different snapshot - name mismatch",
246+
initialContents: newContentArray("snapcontent-snapuid3-6", "snapuid3-6", "snap3-6-x", "sid3-6", validSecretClass, "", "volume3-6", deletePolicy, nil, nil, true),
247+
expectedContents: newContentArray("snapcontent-snapuid3-6", "snapuid3-6", "snap3-6-x", "sid3-6", validSecretClass, "", "volume3-6", deletePolicy, nil, nil, true),
248+
initialSnapshots: newSnapshotArray("snap3-6", "snapuid3-6", "claim3-6", "", validSecretClass, "snapcontent-snapuid3-6", &False, nil, nil, nil, false, true, &timeNowMetav1),
249+
expectedSnapshots: newSnapshotArray("snap3-6", "snapuid3-6", "claim3-6", "", validSecretClass, "snapcontent-snapuid3-6", &False, nil, nil, nil, false, false, &timeNowMetav1),
250+
initialClaims: newClaimArray("claim3-6", "pvc-uid3-6", "1Gi", "volume3-6", v1.ClaimBound, &classEmpty),
251+
expectedEvents: noevents,
252+
initialSecrets: []*v1.Secret{secret()},
253+
errors: noerrors,
254+
test: testSyncSnapshot,
255+
},
256+
{
257+
name: "3-7 - (static) content will be deleted if snapshot deletion timestamp is set, snapshot should have its finalizers removed",
258+
initialContents: newContentArray("content-3-7", "snapuid3-7", "snap3-7", "sid3-7", validSecretClass, "sid3-7", "", deletePolicy, nil, nil, true),
259+
expectedContents: nocontents,
260+
initialSnapshots: newSnapshotArray("snap3-7", "snapuid3-7", "", "content-3-7", validSecretClass, "content-3-7", &False, nil, nil, nil, false, true, &timeNowMetav1),
261+
expectedSnapshots: withSnapshotFinalizers(newSnapshotArray("snap3-7", "snapuid3-7", "", "content-3-7", validSecretClass, "content-3-7", &False, nil, nil, nil, false, false, &timeNowMetav1),
211262
utils.VolumeSnapshotBoundFinalizer,
212263
),
213-
initialClaims: newClaimArray("claim3-3", "pvc-uid3-3", "1Gi", "volume3-3", v1.ClaimBound, &classEmpty),
214264
expectedEvents: noevents,
215265
initialSecrets: []*v1.Secret{secret()},
216266
errors: noerrors,
217267
test: testSyncSnapshot,
218268
},
269+
{
270+
name: "3-8 - (static) content will not be deleted if deletion API call fails, snapshot should have its finalizers remained",
271+
initialContents: newContentArray("content-3-8", "snapuid3-8", "snap3-8", "sid3-8", validSecretClass, "sid3-8", "", deletePolicy, nil, nil, true),
272+
expectedContents: withContentAnnotations(newContentArray("content-3-8", "snapuid3-8", "snap3-8", "sid3-8", validSecretClass, "sid3-8", "", deletePolicy, nil, nil, true),
273+
map[string]string{
274+
"snapshot.storage.kubernetes.io/volumesnapshot-being-deleted": "yes",
275+
}),
276+
initialSnapshots: newSnapshotArray("snap3-8", "snapuid3-8", "", "content-3-8", validSecretClass, "content-3-8", &False, nil, nil, nil, false, true, &timeNowMetav1),
277+
expectedSnapshots: newSnapshotArray("snap3-8", "snapuid3-8", "", "content-3-8", validSecretClass, "content-3-8", &False, nil, nil, nil, false, true, &timeNowMetav1),
278+
expectedEvents: []string{"Warning SnapshotContentObjectDeleteError"},
279+
initialSecrets: []*v1.Secret{secret()},
280+
errors: []reactorError{
281+
// Inject error to the first client.VolumesnapshotV1beta1().VolumeSnapshotContents().Delete call.
282+
// All other calls will succeed.
283+
{"delete", "volumesnapshotcontents", errors.New("mock delete error")},
284+
},
285+
expectSuccess: false,
286+
test: testSyncSnapshotError,
287+
},
288+
{
289+
name: "3-9 - (static) content will not be deleted if retainPolicy is set, snapshot should have its finalizer removed",
290+
initialContents: newContentArray("content-3-9", "snapuid3-9", "snap3-9", "sid3-9", validSecretClass, "sid3-9", "", retainPolicy, nil, nil, true),
291+
expectedContents: withContentAnnotations(newContentArray("content-3-9", "snapuid3-9", "snap3-9", "sid3-9", validSecretClass, "sid3-9", "", retainPolicy, nil, nil, true),
292+
map[string]string{
293+
"snapshot.storage.kubernetes.io/volumesnapshot-being-deleted": "yes",
294+
}),
295+
initialSnapshots: newSnapshotArray("snap3-9", "snapuid3-9", "", "content-3-9", validSecretClass, "content-3-9", &False, nil, nil, nil, false, true, &timeNowMetav1),
296+
expectedSnapshots: newSnapshotArray("snap3-9", "snapuid3-9", "", "content-3-9", validSecretClass, "content-3-9", &False, nil, nil, nil, false, false, &timeNowMetav1),
297+
expectedEvents: noevents,
298+
initialSecrets: []*v1.Secret{secret()},
299+
errors: noerrors,
300+
test: testSyncSnapshot,
301+
},
302+
{
303+
name: "3-10 - (static) snapshot should have its finalizer removed if no content has been found",
304+
initialContents: nocontents,
305+
expectedContents: nocontents,
306+
initialSnapshots: newSnapshotArray("snap3-10", "snapuid3-10", "", "content-3-10", validSecretClass, "", &False, nil, nil, nil, false, true, &timeNowMetav1),
307+
expectedSnapshots: newSnapshotArray("snap3-10", "snapuid3-10", "", "content-3-10", validSecretClass, "", &False, nil, nil, nil, false, false, &timeNowMetav1),
308+
expectedEvents: noevents,
309+
initialSecrets: []*v1.Secret{secret()},
310+
errors: noerrors,
311+
test: testSyncSnapshot,
312+
},
313+
{
314+
name: "3-11 - (static) snapshot should have its finalizer removed if a content is found but points to a different snapshot - uid mismatch",
315+
initialContents: newContentArray("content-3-11", "snapuid3-11-x", "snap3-11", "sid3-11", validSecretClass, "sid3-11", "", deletePolicy, nil, nil, true),
316+
expectedContents: newContentArray("content-3-11", "snapuid3-11-x", "snap3-11", "sid3-11", validSecretClass, "sid3-11", "", deletePolicy, nil, nil, true),
317+
initialSnapshots: newSnapshotArray("snap3-11", "snapuid3-11", "", "content-3-11", validSecretClass, "content-3-11", &False, nil, nil, nil, false, true, &timeNowMetav1),
318+
expectedSnapshots: newSnapshotArray("snap3-11", "snapuid3-11", "", "content-3-11", validSecretClass, "content-3-11", &False, nil, nil, nil, false, false, &timeNowMetav1),
319+
expectedEvents: noevents,
320+
initialSecrets: []*v1.Secret{secret()},
321+
errors: noerrors,
322+
test: testSyncSnapshot,
323+
},
324+
{
325+
name: "3-12 - (static) snapshot should have its finalizer removed if a content is found but points to a different snapshot - name mismatch",
326+
initialContents: newContentArray("content-3-12", "snapuid3-12", "snap3-12-x", "sid3-12", validSecretClass, "sid3-12", "", deletePolicy, nil, nil, true),
327+
expectedContents: newContentArray("content-3-12", "snapuid3-12", "snap3-12-x", "sid3-12", validSecretClass, "sid3-12", "", deletePolicy, nil, nil, true),
328+
initialSnapshots: newSnapshotArray("snap3-12", "snapuid3-12", "", "content-3-12", validSecretClass, "content-3-12", &False, nil, nil, nil, false, true, &timeNowMetav1),
329+
expectedSnapshots: newSnapshotArray("snap3-12", "snapuid3-12", "", "content-3-12", validSecretClass, "content-3-12", &False, nil, nil, nil, false, false, &timeNowMetav1),
330+
expectedEvents: noevents,
331+
initialSecrets: []*v1.Secret{secret()},
332+
errors: noerrors,
333+
test: testSyncSnapshot,
334+
},
219335
}
220336
runSyncTests(t, tests, snapshotClasses)
221337
}

0 commit comments

Comments
 (0)