Skip to content

Commit e5239c4

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #303 from yuxiangqian/cherrypick-2.1
cherry-pick #293: revamp find content logic to fix 290 291 292
2 parents a2d5c6e + f1f0c90 commit e5239c4

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)