Skip to content

Commit 181e2d4

Browse files
mxpvmxpv
authored
Merge pull request #5250 from dmcgowan/cri-fix-reference-ordering
Fix reference ordering in CRI image store
2 parents 664088d + 0886cea commit 181e2d4

4 files changed

Lines changed: 177 additions & 18 deletions

File tree

‎pkg/cri/store/image/image.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -210,8 +210,8 @@ func (s *store) add(img Image) error {
210210
s.images[img.ID] = img
211211
return nil
212212
}
213-
// Or else, merge the references.
214-
i.References = util.MergeStringSlices(i.References, img.References)
213+
// Or else, merge and sort the references.
214+
i.References = sortReferences(util.MergeStringSlices(i.References, img.References))
215215
s.images[img.ID] = i
216216
return nil
217217
}

‎pkg/cri/store/image/image_test.go‎

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -32,24 +32,24 @@ func TestInternalStore(t *testing.T) {
3232
{
3333
ID: "sha256:1123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef",
3434
ChainID: "test-chain-id-1",
35-
References: []string{"ref-1"},
35+
References: []string{"containerd.io/ref-1"},
3636
Size: 10,
3737
},
3838
{
3939
ID: "sha256:2123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef",
4040
ChainID: "test-chain-id-2abcd",
41-
References: []string{"ref-2abcd"},
41+
References: []string{"containerd.io/ref-2abcd"},
4242
Size: 20,
4343
},
4444
{
4545
ID: "sha256:3123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef",
46-
References: []string{"ref-4a333"},
46+
References: []string{"containerd.io/ref-4a333"},
4747
ChainID: "test-chain-id-4a333",
4848
Size: 30,
4949
},
5050
{
5151
ID: "sha256:4123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef",
52-
References: []string{"ref-4abcd"},
52+
References: []string{"containerd.io/ref-4abcd"},
5353
ChainID: "test-chain-id-4abcd",
5454
Size: 40,
5555
},
@@ -143,7 +143,7 @@ func TestImageStore(t *testing.T) {
143143
image := Image{
144144
ID: id,
145145
ChainID: "test-chain-id-1",
146-
References: []string{"ref-1"},
146+
References: []string{"containerd.io/ref-1"},
147147
Size: 10,
148148
}
149149
assert := assertlib.New(t)
@@ -159,64 +159,64 @@ func TestImageStore(t *testing.T) {
159159
expected []Image
160160
}{
161161
"nothing should happen if a non-exist ref disappear": {
162-
ref: "ref-2",
162+
ref: "containerd.io/ref-2",
163163
image: nil,
164164
expected: []Image{image},
165165
},
166166
"new ref for an existing image": {
167-
ref: "ref-2",
167+
ref: "containerd.io/ref-2",
168168
image: &Image{
169169
ID: id,
170170
ChainID: "test-chain-id-1",
171-
References: []string{"ref-2"},
171+
References: []string{"containerd.io/ref-2"},
172172
Size: 10,
173173
},
174174
expected: []Image{
175175
{
176176
ID: id,
177177
ChainID: "test-chain-id-1",
178-
References: []string{"ref-1", "ref-2"},
178+
References: []string{"containerd.io/ref-1", "containerd.io/ref-2"},
179179
Size: 10,
180180
},
181181
},
182182
},
183183
"new ref for a new image": {
184-
ref: "ref-2",
184+
ref: "containerd.io/ref-2",
185185
image: &Image{
186186
ID: newID,
187187
ChainID: "test-chain-id-2",
188-
References: []string{"ref-2"},
188+
References: []string{"containerd.io/ref-2"},
189189
Size: 20,
190190
},
191191
expected: []Image{
192192
image,
193193
{
194194
ID: newID,
195195
ChainID: "test-chain-id-2",
196-
References: []string{"ref-2"},
196+
References: []string{"containerd.io/ref-2"},
197197
Size: 20,
198198
},
199199
},
200200
},
201201
"existing ref point to a new image": {
202-
ref: "ref-1",
202+
ref: "containerd.io/ref-1",
203203
image: &Image{
204204
ID: newID,
205205
ChainID: "test-chain-id-2",
206-
References: []string{"ref-1"},
206+
References: []string{"containerd.io/ref-1"},
207207
Size: 20,
208208
},
209209
expected: []Image{
210210
{
211211
ID: newID,
212212
ChainID: "test-chain-id-2",
213-
References: []string{"ref-1"},
213+
References: []string{"containerd.io/ref-1"},
214214
Size: 20,
215215
},
216216
},
217217
},
218218
"existing ref disappear": {
219-
ref: "ref-1",
219+
ref: "containerd.io/ref-1",
220220
image: nil,
221221
expected: []Image{},
222222
},

‎pkg/cri/store/image/sort.go‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
/*
2+
Copyright The containerd Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package image
18+
19+
import (
20+
"sort"
21+
22+
"github.com/containerd/containerd/reference/docker"
23+
)
24+
25+
// sortReferences sorts references by refRank then string comparison
26+
func sortReferences(references []string) []string {
27+
var prefs []docker.Reference
28+
var bad []string
29+
30+
for _, ref := range references {
31+
pref, err := docker.ParseAnyReference(ref)
32+
if err != nil {
33+
bad = append(bad, ref)
34+
} else {
35+
prefs = append(prefs, pref)
36+
}
37+
}
38+
sort.Slice(prefs, func(a, b int) bool {
39+
ar := refRank(prefs[a])
40+
br := refRank(prefs[b])
41+
if ar == br {
42+
return prefs[a].String() < prefs[b].String()
43+
}
44+
return ar < br
45+
})
46+
sort.Strings(bad)
47+
var refs []string
48+
for _, pref := range prefs {
49+
refs = append(refs, pref.String())
50+
}
51+
return append(refs, bad...)
52+
}
53+
54+
// refRank ranks precedence for reference type, preferring higher information references
55+
// 1. Name + Tag + Digest
56+
// 2. Name + Tag
57+
// 3. Name + Digest
58+
// 4. Name
59+
// 5. Digest
60+
// 6. Parse error
61+
func refRank(ref docker.Reference) uint8 {
62+
if _, ok := ref.(docker.Named); ok {
63+
if _, ok = ref.(docker.Tagged); ok {
64+
if _, ok = ref.(docker.Digested); ok {
65+
return 1
66+
}
67+
return 2
68+
}
69+
if _, ok = ref.(docker.Digested); ok {
70+
return 3
71+
}
72+
return 4
73+
}
74+
return 5
75+
}

‎pkg/cri/store/image/sort_test.go‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
/*
2+
Copyright The containerd Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package image
18+
19+
import (
20+
"io"
21+
"io/ioutil"
22+
"math/rand"
23+
"testing"
24+
25+
"github.com/opencontainers/go-digest"
26+
)
27+
28+
func TestReferenceSorting(t *testing.T) {
29+
digested := func(seed int64) string {
30+
b, err := ioutil.ReadAll(io.LimitReader(rand.New(rand.NewSource(seed)), 64))
31+
if err != nil {
32+
panic(err)
33+
}
34+
return digest.FromBytes(b).String()
35+
}
36+
// Add z. prefix to string sort after "sha256:"
37+
r1 := func(name, tag string, seed int64) string {
38+
return "z.containerd.io/" + name + ":" + tag + "@" + digested(seed)
39+
}
40+
r2 := func(name, tag string) string {
41+
return "z.containerd.io/" + name + ":" + tag
42+
}
43+
r3 := func(name string, seed int64) string {
44+
return "z.containerd.io/" + name + "@" + digested(seed)
45+
}
46+
47+
for i, tc := range []struct {
48+
unsorted []string
49+
expected []string
50+
}{
51+
{
52+
unsorted: []string{r2("name", "latest"), r3("name", 1), r1("name", "latest", 1)},
53+
expected: []string{r1("name", "latest", 1), r2("name", "latest"), r3("name", 1)},
54+
},
55+
{
56+
unsorted: []string{"can't parse this:latest", r3("name", 1), r2("name", "latest")},
57+
expected: []string{r2("name", "latest"), r3("name", 1), "can't parse this:latest"},
58+
},
59+
{
60+
unsorted: []string{digested(1), r3("name", 1), r2("name", "latest")},
61+
expected: []string{r2("name", "latest"), r3("name", 1), digested(1)},
62+
},
63+
{
64+
unsorted: []string{r2("name", "tag2"), r2("name", "tag3"), r2("name", "tag1")},
65+
expected: []string{r2("name", "tag1"), r2("name", "tag2"), r2("name", "tag3")},
66+
},
67+
{
68+
unsorted: []string{r2("name-2", "tag"), r2("name-3", "tag"), r2("name-1", "tag")},
69+
expected: []string{r2("name-1", "tag"), r2("name-2", "tag"), r2("name-3", "tag")},
70+
},
71+
} {
72+
sorted := sortReferences(tc.unsorted)
73+
if len(sorted) != len(tc.expected) {
74+
t.Errorf("[%d]: Mismatched sized, got %d, expected %d", i, len(sorted), len(tc.expected))
75+
continue
76+
}
77+
for j := range sorted {
78+
if sorted[j] != tc.expected[j] {
79+
t.Errorf("[%d]: Wrong value at %d, got %q, expected %q", i, j, sorted[j], tc.expected[j])
80+
break
81+
}
82+
}
83+
}
84+
}

0 commit comments

Comments
 (0)