Apply suggestions from code review
Co-authored-by: Steven Powell <44844360+spowelljr@users.noreply.github.com>pull/16578/head
Товарищ программист
Товарищ программист
parent
8a06b68bc2
commit
d4c33ff371
|
|
@ -31,61 +31,189 @@ type CacheImageTestCase struct {
|
|||
}
|
||||
|
||||
func TestMergeImageLists(t *testing.T) {
|
||||
// test case 0: from #16556
|
||||
// e.g. on node 1 image1 have an item with k8s.gcr.io/image1:v1.0.0 tag
|
||||
// and another item with registry.k8s.io/image1:v1.0.0 tag too
|
||||
testCases := []CacheImageTestCase{
|
||||
description: "same images with multiple tags appear on one node",
|
||||
images: [][]cruntime.ListImage{
|
||||
[]cruntime.ListImage{
|
||||
// test case 0: from #16556
|
||||
// e.g. on node 1 image1 have an item with k8s.gcr.io/image1:v1.0.0 tag
|
||||
// and another item with registry.k8s.io/image1:v1.0.0 tag too
|
||||
{
|
||||
description: "same images with multiple tags appear on one node",
|
||||
images: [][]cruntime.ListImage{
|
||||
{
|
||||
ID: "image_id_1",
|
||||
RepoDigests: []string{"image_digest_1"},
|
||||
RepoTags: []string{"k8s.gcr.io/image1:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
{
|
||||
ID: "image_id_2",
|
||||
RepoDigests: []string{"image_digest_2"},
|
||||
RepoTags: []string{"registry.k8s.io/image2:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
{
|
||||
ID: "image_id_1",
|
||||
RepoDigests: []string{"image_digest_1"},
|
||||
RepoTags: []string{"k8s.gcr.io/image1:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
{
|
||||
ID: "image_id_2",
|
||||
RepoDigests: []string{"image_digest_2"},
|
||||
RepoTags: []string{"registry.k8s.io/image2:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
|
||||
{
|
||||
ID: "image_id_1",
|
||||
RepoDigests: []string{"image_digest_1"},
|
||||
RepoTags: []string{"registry.k8s.io/image1:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
{
|
||||
ID: "image_id_2",
|
||||
RepoDigests: []string{"image_digest_2"},
|
||||
RepoTags: []string{"k8s.gcr.io/image2:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
},
|
||||
},
|
||||
expected: []cruntime.ListImage{
|
||||
{
|
||||
ID: "image_id_1",
|
||||
RepoDigests: []string{"image_digest_1"},
|
||||
RepoTags: []string{"registry.k8s.io/image1:v1.0.0"},
|
||||
RepoTags: []string{"k8s.gcr.io/image1:v1.0.0", "registry.k8s.io/image1:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
{
|
||||
ID: "image_id_2",
|
||||
RepoDigests: []string{"image_digest_2"},
|
||||
RepoTags: []string{"k8s.gcr.io/image2:v1.0.0"},
|
||||
RepoTags: []string{"k8s.gcr.io/image2:v1.0.0", "registry.k8s.io/image2:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
},
|
||||
},
|
||||
expected: []cruntime.ListImage{
|
||||
{
|
||||
ID: "image_id_1",
|
||||
RepoDigests: []string{"image_digest_1"},
|
||||
RepoTags: []string{"k8s.gcr.io/image1:v1.0.0", "registry.k8s.io/image1:v1.0.0"},
|
||||
Size: "1",
|
||||
// test case 1: from #16557
|
||||
// e.g. image1 have k8s.gcr.io/image1:v1.0.0 tag on node 1 and registry.k8s.io/image1:v1.0.0 on node 2
|
||||
{
|
||||
description: "same images with multiple tags appear on two node",
|
||||
images: [][]cruntime.ListImage{
|
||||
{
|
||||
{
|
||||
ID: "image_id_1",
|
||||
RepoDigests: []string{"image_digest_1"},
|
||||
RepoTags: []string{"k8s.gcr.io/image1:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
{
|
||||
ID: "image_id_2",
|
||||
RepoDigests: []string{"image_digest_2"},
|
||||
RepoTags: []string{"registry.k8s.io/image2:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
},
|
||||
{
|
||||
{
|
||||
ID: "image_id_1",
|
||||
RepoDigests: []string{"image_digest_1"},
|
||||
RepoTags: []string{"registry.k8s.io/image1:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
{
|
||||
ID: "image_id_2",
|
||||
RepoDigests: []string{"image_digest_2"},
|
||||
RepoTags: []string{"k8s.gcr.io/image2:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
},
|
||||
},
|
||||
{
|
||||
ID: "image_id_2",
|
||||
RepoDigests: []string{"image_digest_2"},
|
||||
RepoTags: []string{"k8s.gcr.io/image2:v1.0.0", "registry.k8s.io/image2:v1.0.0"},
|
||||
Size: "1",
|
||||
expected: []cruntime.ListImage{
|
||||
{
|
||||
ID: "image_id_1",
|
||||
RepoDigests: []string{"image_digest_1"},
|
||||
RepoTags: []string{"k8s.gcr.io/image1:v1.0.0", "registry.k8s.io/image1:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
{
|
||||
ID: "image_id_2",
|
||||
RepoDigests: []string{"image_digest_2"},
|
||||
RepoTags: []string{"k8s.gcr.io/image2:v1.0.0", "registry.k8s.io/image2:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
},
|
||||
},
|
||||
}
|
||||
// test case 1: from #16557
|
||||
// e.g. image1 have k8s.gcr.io/image1:v1.0.0 tag on node 1 and registry.k8s.io/image1:v1.0.0 on node 2
|
||||
testCase1 := CacheImageTestCase{
|
||||
description: "same images with multiple tags appear on two node",
|
||||
images: [][]cruntime.ListImage{
|
||||
{
|
||||
// test case 2: from #16557
|
||||
// e.g. image1 have k8s.gcr.io/image1:v1.0.0 tag on node 1
|
||||
// and both k8s.gcr.io/image1:v1.0.0 and registry.k8s.io/image1:v1.0.0 on node 2
|
||||
{
|
||||
description: "image has tag1 on node1 and both tag1 & tag2 on node 2",
|
||||
images: [][]cruntime.ListImage{
|
||||
{
|
||||
{
|
||||
ID: "image_id_1",
|
||||
RepoDigests: []string{"image_digest_1"},
|
||||
RepoTags: []string{"k8s.gcr.io/image1:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
{
|
||||
ID: "image_id_2",
|
||||
RepoDigests: []string{"image_digest_2"},
|
||||
RepoTags: []string{"registry.k8s.io/image2:v1.0.0", "k8s.gcr.io/image2:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
},
|
||||
{
|
||||
{
|
||||
ID: "image_id_1",
|
||||
RepoDigests: []string{"image_digest_1"},
|
||||
RepoTags: []string{"registry.k8s.io/image1:v1.0.0", "k8s.gcr.io/image1:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
{
|
||||
ID: "image_id_2",
|
||||
RepoDigests: []string{"image_digest_2"},
|
||||
RepoTags: []string{"k8s.gcr.io/image2:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
},
|
||||
},
|
||||
expected: []cruntime.ListImage{
|
||||
{
|
||||
ID: "image_id_1",
|
||||
RepoDigests: []string{"image_digest_1"},
|
||||
RepoTags: []string{"k8s.gcr.io/image1:v1.0.0", "registry.k8s.io/image1:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
{
|
||||
ID: "image_id_2",
|
||||
RepoDigests: []string{"image_digest_2"},
|
||||
RepoTags: []string{"k8s.gcr.io/image2:v1.0.0", "registry.k8s.io/image2:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
},
|
||||
},
|
||||
|
||||
{
|
||||
description: "normal case",
|
||||
images: [][]cruntime.ListImage{
|
||||
{
|
||||
{
|
||||
ID: "image_id_1",
|
||||
RepoDigests: []string{"image_digest_1"},
|
||||
RepoTags: []string{"k8s.gcr.io/image1:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
{
|
||||
ID: "image_id_2",
|
||||
RepoDigests: []string{"image_digest_2"},
|
||||
RepoTags: []string{"registry.k8s.io/image2:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
},
|
||||
{
|
||||
{
|
||||
ID: "image_id_3",
|
||||
RepoDigests: []string{"image_digest_3"},
|
||||
RepoTags: []string{"registry.k8s.io/image3:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
{
|
||||
ID: "image_id_4",
|
||||
RepoDigests: []string{"image_digest_4"},
|
||||
RepoTags: []string{"k8s.gcr.io/image4:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
},
|
||||
},
|
||||
expected: []cruntime.ListImage{
|
||||
{
|
||||
ID: "image_id_1",
|
||||
RepoDigests: []string{"image_digest_1"},
|
||||
|
|
@ -98,105 +226,6 @@ func TestMergeImageLists(t *testing.T) {
|
|||
RepoTags: []string{"registry.k8s.io/image2:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
},
|
||||
{
|
||||
{
|
||||
ID: "image_id_1",
|
||||
RepoDigests: []string{"image_digest_1"},
|
||||
RepoTags: []string{"registry.k8s.io/image1:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
{
|
||||
ID: "image_id_2",
|
||||
RepoDigests: []string{"image_digest_2"},
|
||||
RepoTags: []string{"k8s.gcr.io/image2:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
},
|
||||
},
|
||||
expected: []cruntime.ListImage{
|
||||
{
|
||||
ID: "image_id_1",
|
||||
RepoDigests: []string{"image_digest_1"},
|
||||
RepoTags: []string{"k8s.gcr.io/image1:v1.0.0", "registry.k8s.io/image1:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
{
|
||||
ID: "image_id_2",
|
||||
RepoDigests: []string{"image_digest_2"},
|
||||
RepoTags: []string{"k8s.gcr.io/image2:v1.0.0", "registry.k8s.io/image2:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
},
|
||||
}
|
||||
// test case 2: from #16557
|
||||
// e.g. image1 have k8s.gcr.io/image1:v1.0.0 tag on node 1
|
||||
// and both k8s.gcr.io/image1:v1.0.0 and registry.k8s.io/image1:v1.0.0 on node 2
|
||||
testCase2 := CacheImageTestCase{
|
||||
description: "image has tag1 on node1 and both tag1 & tag2 on node 2",
|
||||
images: [][]cruntime.ListImage{
|
||||
{
|
||||
{
|
||||
ID: "image_id_1",
|
||||
RepoDigests: []string{"image_digest_1"},
|
||||
RepoTags: []string{"k8s.gcr.io/image1:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
{
|
||||
ID: "image_id_2",
|
||||
RepoDigests: []string{"image_digest_2"},
|
||||
RepoTags: []string{"registry.k8s.io/image2:v1.0.0", "k8s.gcr.io/image2:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
},
|
||||
{
|
||||
{
|
||||
ID: "image_id_1",
|
||||
RepoDigests: []string{"image_digest_1"},
|
||||
RepoTags: []string{"registry.k8s.io/image1:v1.0.0", "k8s.gcr.io/image1:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
{
|
||||
ID: "image_id_2",
|
||||
RepoDigests: []string{"image_digest_2"},
|
||||
RepoTags: []string{"k8s.gcr.io/image2:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
},
|
||||
},
|
||||
expected: []cruntime.ListImage{
|
||||
{
|
||||
ID: "image_id_1",
|
||||
RepoDigests: []string{"image_digest_1"},
|
||||
RepoTags: []string{"k8s.gcr.io/image1:v1.0.0", "registry.k8s.io/image1:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
{
|
||||
ID: "image_id_2",
|
||||
RepoDigests: []string{"image_digest_2"},
|
||||
RepoTags: []string{"k8s.gcr.io/image2:v1.0.0", "registry.k8s.io/image2:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
},
|
||||
}
|
||||
testCase3 := CacheImageTestCase{
|
||||
description: "normal case",
|
||||
images: [][]cruntime.ListImage{
|
||||
{
|
||||
{
|
||||
ID: "image_id_1",
|
||||
RepoDigests: []string{"image_digest_1"},
|
||||
RepoTags: []string{"k8s.gcr.io/image1:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
{
|
||||
ID: "image_id_2",
|
||||
RepoDigests: []string{"image_digest_2"},
|
||||
RepoTags: []string{"registry.k8s.io/image2:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
},
|
||||
{
|
||||
{
|
||||
ID: "image_id_3",
|
||||
RepoDigests: []string{"image_digest_3"},
|
||||
|
|
@ -211,47 +240,20 @@ func TestMergeImageLists(t *testing.T) {
|
|||
},
|
||||
},
|
||||
},
|
||||
expected: []cruntime.ListImage{
|
||||
{
|
||||
ID: "image_id_1",
|
||||
RepoDigests: []string{"image_digest_1"},
|
||||
RepoTags: []string{"k8s.gcr.io/image1:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
{
|
||||
ID: "image_id_2",
|
||||
RepoDigests: []string{"image_digest_2"},
|
||||
RepoTags: []string{"registry.k8s.io/image2:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
{
|
||||
ID: "image_id_3",
|
||||
RepoDigests: []string{"image_digest_3"},
|
||||
RepoTags: []string{"registry.k8s.io/image3:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
{
|
||||
ID: "image_id_4",
|
||||
RepoDigests: []string{"image_digest_4"},
|
||||
RepoTags: []string{"k8s.gcr.io/image4:v1.0.0"},
|
||||
Size: "1",
|
||||
},
|
||||
},
|
||||
}
|
||||
testCases := []CacheImageTestCase{testCase0, testCase1, testCase2, testCase3}
|
||||
|
||||
for _, testCase := range testCases {
|
||||
actualResult := mergeImageLists(testCase.images)
|
||||
sort.Slice(actualResult, func(i, j int) bool {
|
||||
return actualResult[i].ID < actualResult[j].ID
|
||||
for _, tc := range testCases {
|
||||
got := mergeImageLists(tc.images)
|
||||
sort.Slice(got, func(i, j int) bool {
|
||||
return got[i].ID < got[j].ID
|
||||
})
|
||||
for _, img := range actualResult {
|
||||
for _, img := range got {
|
||||
sort.Slice(img.RepoTags, func(i, j int) bool {
|
||||
return img.RepoTags[i] < img.RepoTags[j]
|
||||
})
|
||||
}
|
||||
if ok := reflect.DeepEqual(actualResult, testCase.expected); !ok {
|
||||
t.Errorf("test case %s failed,\n actual %v,\n expected %v", testCase.description, actualResult, testCase.expected)
|
||||
if ok := reflect.DeepEqual(got, tc.expected); !ok {
|
||||
t.Errorf("%s:\nmergeImageLists() = %+v;\nwant %+v", tc.description, got, tc.expected)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -773,21 +773,18 @@ func mergeImageLists(lists [][]cruntime.ListImage) []cruntime.ListImage {
|
|||
for _, list := range lists {
|
||||
for _, img := range list {
|
||||
img := img
|
||||
if _, ok := images[img.ID]; !ok {
|
||||
_, existingImg := images[img.ID]
|
||||
if !existingImg {
|
||||
images[img.ID] = &img
|
||||
for _, repoTag := range img.RepoTags {
|
||||
imageTagAndIDSet[img.ID+repoTag] = struct{}{}
|
||||
}
|
||||
continue
|
||||
}
|
||||
for _, repoTag := range img.RepoTags {
|
||||
if _, ok := imageTagAndIDSet[img.ID+repoTag]; ok {
|
||||
continue
|
||||
}
|
||||
// if there is any repo tag which is not included in the map's corresponding item
|
||||
// add it to the repo tag list
|
||||
imageTagAndIDSet[img.ID+repoTag] = struct{}{}
|
||||
images[img.ID].RepoTags = append(images[img.ID].RepoTags, repoTag)
|
||||
if existingImg {
|
||||
images[img.ID].RepoTags = append(images[img.ID].RepoTags, repoTag)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue