Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit 8e83534

Browse files
enjenj
authoredNov 3, 2017
Merge pull request #17153 from enj/automated-cherry-pick-of-#17080-upstream-stage
Automated cherry pick of #17080
2 parents 81f9fab + 24614f6 commit 8e83534

File tree

3 files changed

+373
-10
lines changed

3 files changed

+373
-10
lines changed
 

‎pkg/oc/admin/migrate/migrator.go

+23-9
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"fmt"
55
"io"
66
"strings"
7+
"time"
78

89
"github.com/golang/glog"
910
"github.com/spf13/cobra"
@@ -49,6 +50,11 @@ func AlwaysRequiresMigration(_ *resource.Info) (Reporter, error) {
4950
return ReporterBool(true), nil
5051
}
5152

53+
// timeStampNow returns the current time in the same format as glog
54+
func timeStampNow() string {
55+
return time.Now().Format("0102 15:04:05.000000")
56+
}
57+
5258
// NotChanged is a Reporter returned by operations that are guaranteed to be read-only
5359
var NotChanged = ReporterBool(false)
5460

@@ -397,9 +403,9 @@ func (t *migrateTracker) report(prefix string, info *resource.Info, err error) {
397403
ns = "-n " + ns
398404
}
399405
if err != nil {
400-
fmt.Fprintf(t.out, "%-10s %s %s/%s: %v\n", prefix, ns, info.Mapping.Resource, info.Name, err)
406+
fmt.Fprintf(t.out, "E%s %-10s %s %s/%s: %v\n", timeStampNow(), prefix, ns, info.Mapping.Resource, info.Name, err)
401407
} else {
402-
fmt.Fprintf(t.out, "%-10s %s %s/%s\n", prefix, ns, info.Mapping.Resource, info.Name)
408+
fmt.Fprintf(t.out, "I%s %-10s %s %s/%s\n", timeStampNow(), prefix, ns, info.Mapping.Resource, info.Name)
403409
}
404410
}
405411

@@ -484,11 +490,13 @@ func canRetry(err error) bool {
484490
// All other errors are left in their natural state - they will not be retried unless
485491
// they define a Temporary() method that returns true.
486492
func DefaultRetriable(info *resource.Info, err error) error {
487-
// tolerate the deletion of resources during migration
488-
if err == nil || isNotFoundForInfo(info, err) {
489-
return nil
490-
}
491493
switch {
494+
case err == nil:
495+
return nil
496+
case isNotFoundForInfo(info, err):
497+
// tolerate the deletion of resources during migration
498+
// report unchanged since we did not actually migrate this object
499+
return ErrUnchanged
492500
case errors.IsMethodNotSupported(err):
493501
return ErrNotRetriable{err}
494502
case errors.IsConflict(err):
@@ -498,8 +506,9 @@ func DefaultRetriable(info *resource.Info, err error) error {
498506
return ErrRetriable{err}
499507
case errors.IsServerTimeout(err):
500508
return ErrRetriable{err}
509+
default:
510+
return err
501511
}
502-
return err
503512
}
504513

505514
// isNotFoundForInfo returns true iff the error is a not found for the specific info object.
@@ -515,6 +524,11 @@ func isNotFoundForInfo(info *resource.Info, err error) bool {
515524
if details == nil {
516525
return false
517526
}
518-
gvk := info.Object.GetObjectKind().GroupVersionKind()
519-
return details.Name == info.Name && details.Kind == gvk.Kind && (details.Group == "" || details.Group == gvk.Group)
527+
// get schema.GroupKind from the mapping since the actual object may not have type meta filled out
528+
gk := info.Mapping.GroupVersionKind.GroupKind()
529+
// based on case-insensitive string comparisons, the error matches info iff
530+
// the name and kind match
531+
// the group match, but only if both the error and info specify a group
532+
return strings.EqualFold(details.Name, info.Name) && strings.EqualFold(details.Kind, gk.Kind) &&
533+
(len(details.Group) == 0 || len(gk.Group) == 0 || strings.EqualFold(details.Group, gk.Group))
520534
}

‎pkg/oc/admin/migrate/migrator_test.go

+345
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,345 @@
1+
package migrate
2+
3+
import (
4+
"fmt"
5+
"net/http"
6+
"testing"
7+
8+
"k8s.io/apimachinery/pkg/api/errors"
9+
"k8s.io/apimachinery/pkg/api/meta"
10+
"k8s.io/apimachinery/pkg/runtime/schema"
11+
"k8s.io/kubernetes/pkg/kubectl/resource"
12+
)
13+
14+
func TestIsNotFoundForInfo(t *testing.T) {
15+
type args struct {
16+
info *resource.Info
17+
err error
18+
}
19+
tests := []struct {
20+
name string
21+
args args
22+
want bool
23+
}{
24+
{
25+
name: "nil err does not match",
26+
args: args{
27+
info: nil,
28+
err: nil,
29+
},
30+
want: false,
31+
},
32+
{
33+
name: "simple not found match",
34+
args: args{
35+
info: &resource.Info{
36+
Mapping: &meta.RESTMapping{
37+
GroupVersionKind: schema.GroupVersionKind{
38+
Group: "group1",
39+
Kind: "kind1",
40+
},
41+
},
42+
Name: "name1",
43+
},
44+
err: errors.NewNotFound(schema.GroupResource{
45+
Group: "group1",
46+
Resource: "kind1", // this is the kind
47+
},
48+
"name1",
49+
),
50+
},
51+
want: true,
52+
},
53+
{
54+
name: "simple not found match from generic 404 response",
55+
args: args{
56+
info: &resource.Info{
57+
Mapping: &meta.RESTMapping{
58+
GroupVersionKind: schema.GroupVersionKind{
59+
Group: "group1",
60+
Kind: "kind1",
61+
},
62+
},
63+
Name: "name1",
64+
},
65+
err: errors.NewGenericServerResponse(
66+
http.StatusNotFound,
67+
"",
68+
schema.GroupResource{
69+
Group: "group1",
70+
Resource: "kind1", // this is the kind
71+
},
72+
"name1",
73+
"",
74+
0,
75+
false,
76+
),
77+
},
78+
want: true,
79+
},
80+
{
81+
name: "simple not match from generic 400 response",
82+
args: args{
83+
info: &resource.Info{
84+
Mapping: &meta.RESTMapping{
85+
GroupVersionKind: schema.GroupVersionKind{
86+
Group: "group1",
87+
Kind: "kind1",
88+
},
89+
},
90+
Name: "name1",
91+
},
92+
err: errors.NewGenericServerResponse(
93+
http.StatusBadRequest,
94+
"",
95+
schema.GroupResource{
96+
Group: "group1",
97+
Resource: "kind1", // this is the kind
98+
},
99+
"name1",
100+
"",
101+
0,
102+
false,
103+
),
104+
},
105+
want: false,
106+
},
107+
{
108+
name: "different status error does not match",
109+
args: args{
110+
info: &resource.Info{
111+
Mapping: &meta.RESTMapping{
112+
GroupVersionKind: schema.GroupVersionKind{
113+
Group: "group1",
114+
Kind: "kind1",
115+
},
116+
},
117+
Name: "name1",
118+
},
119+
err: errors.NewAlreadyExists(schema.GroupResource{
120+
Group: "group1",
121+
Resource: "kind1", // this is the kind
122+
},
123+
"name1",
124+
),
125+
},
126+
want: false,
127+
},
128+
{
129+
name: "non status error does not match",
130+
args: args{
131+
info: &resource.Info{
132+
Mapping: &meta.RESTMapping{
133+
GroupVersionKind: schema.GroupVersionKind{
134+
Group: "group1",
135+
Kind: "kind1",
136+
},
137+
},
138+
Name: "name1",
139+
},
140+
err: fmt.Errorf("%v",
141+
schema.GroupVersionKind{
142+
Group: "group1",
143+
Kind: "kind1",
144+
},
145+
),
146+
},
147+
want: false,
148+
},
149+
{
150+
name: "case-insensitive not found match",
151+
args: args{
152+
info: &resource.Info{
153+
Mapping: &meta.RESTMapping{
154+
GroupVersionKind: schema.GroupVersionKind{
155+
Group: "GROUPA",
156+
Kind: "KINDB",
157+
},
158+
},
159+
Name: "NOTname",
160+
},
161+
err: errors.NewNotFound(schema.GroupResource{
162+
Group: "groupA",
163+
Resource: "Kindb", // this is the kind
164+
},
165+
"notNAME",
166+
),
167+
},
168+
want: true,
169+
},
170+
{
171+
name: "case-insensitive not found match from generic 404 response",
172+
args: args{
173+
info: &resource.Info{
174+
Mapping: &meta.RESTMapping{
175+
GroupVersionKind: schema.GroupVersionKind{
176+
Group: "ThisGroup",
177+
Kind: "HasKinds",
178+
},
179+
},
180+
Name: "AndAName",
181+
},
182+
err: errors.NewGenericServerResponse(
183+
http.StatusNotFound,
184+
"",
185+
schema.GroupResource{
186+
Group: "thisgroup",
187+
Resource: "haskinds", // this is the kind
188+
},
189+
"andaname",
190+
"",
191+
0,
192+
false,
193+
),
194+
},
195+
want: true,
196+
},
197+
{
198+
name: "case-insensitive not found match, no group in info",
199+
args: args{
200+
info: &resource.Info{
201+
Mapping: &meta.RESTMapping{
202+
GroupVersionKind: schema.GroupVersionKind{
203+
Kind: "KINDB",
204+
},
205+
},
206+
Name: "NOTname",
207+
},
208+
err: errors.NewNotFound(schema.GroupResource{
209+
Group: "groupA",
210+
Resource: "Kindb", // this is the kind
211+
},
212+
"notNAME",
213+
),
214+
},
215+
want: true,
216+
},
217+
{
218+
name: "case-insensitive not found match, no group in error",
219+
args: args{
220+
info: &resource.Info{
221+
Mapping: &meta.RESTMapping{
222+
GroupVersionKind: schema.GroupVersionKind{
223+
Group: "GROUPA",
224+
Kind: "KINDB",
225+
},
226+
},
227+
Name: "NOTname",
228+
},
229+
err: errors.NewNotFound(schema.GroupResource{
230+
Resource: "Kindb", // this is the kind
231+
},
232+
"notNAME",
233+
),
234+
},
235+
want: true,
236+
},
237+
{
238+
name: "case-insensitive not match due to different groups",
239+
args: args{
240+
info: &resource.Info{
241+
Mapping: &meta.RESTMapping{
242+
GroupVersionKind: schema.GroupVersionKind{
243+
Group: "group1",
244+
Kind: "KINDB",
245+
},
246+
},
247+
Name: "NOTname",
248+
},
249+
err: errors.NewNotFound(schema.GroupResource{
250+
Group: "group2",
251+
Resource: "Kindb", // this is the kind
252+
},
253+
"notNAME",
254+
),
255+
},
256+
want: false,
257+
},
258+
{
259+
name: "case-insensitive not found match from generic 404 response, no group in info",
260+
args: args{
261+
info: &resource.Info{
262+
Mapping: &meta.RESTMapping{
263+
GroupVersionKind: schema.GroupVersionKind{
264+
Kind: "HasKinds",
265+
},
266+
},
267+
Name: "AndAName",
268+
},
269+
err: errors.NewGenericServerResponse(
270+
http.StatusNotFound,
271+
"",
272+
schema.GroupResource{
273+
Group: "thisgroup",
274+
Resource: "haskinds", // this is the kind
275+
},
276+
"andaname",
277+
"",
278+
0,
279+
false,
280+
),
281+
},
282+
want: true,
283+
},
284+
{
285+
name: "case-insensitive not found match from generic 404 response, no group in error",
286+
args: args{
287+
info: &resource.Info{
288+
Mapping: &meta.RESTMapping{
289+
GroupVersionKind: schema.GroupVersionKind{
290+
Group: "ThisGroup",
291+
Kind: "HasKinds",
292+
},
293+
},
294+
Name: "AndAName",
295+
},
296+
err: errors.NewGenericServerResponse(
297+
http.StatusNotFound,
298+
"",
299+
schema.GroupResource{
300+
Resource: "haskinds", // this is the kind
301+
},
302+
"andaname",
303+
"",
304+
0,
305+
false,
306+
),
307+
},
308+
want: true,
309+
},
310+
{
311+
name: "case-insensitive not match from generic 404 response due to different groups",
312+
args: args{
313+
info: &resource.Info{
314+
Mapping: &meta.RESTMapping{
315+
GroupVersionKind: schema.GroupVersionKind{
316+
Group: "thingA",
317+
Kind: "HasKinds",
318+
},
319+
},
320+
Name: "AndAName",
321+
},
322+
err: errors.NewGenericServerResponse(
323+
http.StatusNotFound,
324+
"",
325+
schema.GroupResource{
326+
Group: "thingB",
327+
Resource: "haskinds", // this is the kind
328+
},
329+
"andaname",
330+
"",
331+
0,
332+
false,
333+
),
334+
},
335+
want: false,
336+
},
337+
}
338+
for _, tt := range tests {
339+
t.Run(tt.name, func(t *testing.T) {
340+
if got := isNotFoundForInfo(tt.args.info, tt.args.err); got != tt.want {
341+
t.Errorf("isNotFoundForInfo() = %v, want %v", got, tt.want)
342+
}
343+
})
344+
}
345+
}

‎pkg/oc/admin/migrate/storage/storage.go

+5-1
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,11 @@ func (o *MigrateAPIStorageOptions) save(info *resource.Info, reporter migrate.Re
202202
Name(info.Name).Do()
203203
data, err := get.Raw()
204204
if err != nil {
205-
return migrate.DefaultRetriable(info, err)
205+
// since we have an error, processing the body is safe because we are not going
206+
// to send it back to the server. Thus we can safely call Result.Error().
207+
// This is required because we want to make sure we pass an errors.APIStatus so
208+
// that DefaultRetriable can correctly determine if the error is safe to retry.
209+
return migrate.DefaultRetriable(info, get.Error())
206210
}
207211
update := info.Client.Put().
208212
Resource(info.Mapping.Resource).

0 commit comments

Comments
 (0)
Please sign in to comment.