Skip to content

Commit d3ebce7

Browse files
enjopenshift-cherrypick-robot
authored andcommitted
Migrate: handle NotFound via resource matching and during conflicts
This change fixes two issues with the migrate command: 1. It updates isNotFoundForInfo to compare the error's kind to the info's resource as well as its kind. The server error's use the resource value in places where the kind should be used. We simply accept both now. 2. It updates DefaultRetriable to not fail on conflict errors if they were caused by the deletion of the given resource. The following shell based tests were used to stress the edge cases that this change fixes. The migrate command will readily fail in these tests without the fixes from this change. In one shell we continuously create service accounts with random names as fast as possible. The resource type is not relevant. N=8 while true; do ((i=i%N)); ((i++==0)) && wait oc create serviceaccount $((1 + RANDOM % 10000000)) & done In a second shell we try to continuously delete all service accounts: while true; do oc delete sa --all done In a third shell we try to continuously migrate service accounts: while true; do oc adm migrate storage --include='serviceaccount' --confirm done Bug 1537751 Signed-off-by: Monis Khan <[email protected]>
1 parent d9ebdd2 commit d3ebce7

File tree

2 files changed

+78
-4
lines changed

2 files changed

+78
-4
lines changed

pkg/oc/admin/migrate/migrator.go

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -503,6 +503,11 @@ func DefaultRetriable(info *resource.Info, err error) error {
503503
return ErrNotRetriable{err}
504504
case errors.IsConflict(err):
505505
if refreshErr := info.Get(); refreshErr != nil {
506+
// tolerate the deletion of resources during migration
507+
// report unchanged since we did not actually migrate this object
508+
if isNotFoundForInfo(info, refreshErr) {
509+
return ErrUnchanged
510+
}
506511
return ErrNotRetriable{err}
507512
}
508513
return ErrRetriable{err}
@@ -526,11 +531,14 @@ func isNotFoundForInfo(info *resource.Info, err error) bool {
526531
if details == nil {
527532
return false
528533
}
529-
// get schema.GroupKind from the mapping since the actual object may not have type meta filled out
534+
// get schema.GroupKind and Resource from the mapping since the actual object may not have type meta filled out
530535
gk := info.Mapping.GroupVersionKind.GroupKind()
536+
mappingResource := info.Mapping.Resource
531537
// based on case-insensitive string comparisons, the error matches info iff
532-
// the name and kind match
533-
// the group match, but only if both the error and info specify a group
534-
return strings.EqualFold(details.Name, info.Name) && strings.EqualFold(details.Kind, gk.Kind) &&
538+
// the names match
539+
// the error's kind matches the mapping's kind or the mapping's resource
540+
// the groups match, but only if both the error and info specify a group
541+
return strings.EqualFold(details.Name, info.Name) &&
542+
(strings.EqualFold(details.Kind, gk.Kind) || strings.EqualFold(details.Kind, mappingResource)) &&
535543
(len(details.Group) == 0 || len(gk.Group) == 0 || strings.EqualFold(details.Group, gk.Group))
536544
}

pkg/oc/admin/migrate/migrator_test.go

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -334,6 +334,72 @@ func TestIsNotFoundForInfo(t *testing.T) {
334334
},
335335
want: false,
336336
},
337+
{
338+
name: "case-insensitive match due to different kinds but same resource",
339+
args: args{
340+
info: &resource.Info{
341+
Mapping: &meta.RESTMapping{
342+
Resource: "KIND2",
343+
GroupVersionKind: schema.GroupVersionKind{
344+
Group: "group1",
345+
Kind: "kind1",
346+
},
347+
},
348+
Name: "NOTname",
349+
},
350+
err: errors.NewNotFound(schema.GroupResource{
351+
Group: "GROUP1",
352+
Resource: "kind2", // this is the kind
353+
},
354+
"notNAME",
355+
),
356+
},
357+
want: true,
358+
},
359+
{
360+
name: "case-insensitive match due to different resource but same kinds",
361+
args: args{
362+
info: &resource.Info{
363+
Mapping: &meta.RESTMapping{
364+
Resource: "kind1",
365+
GroupVersionKind: schema.GroupVersionKind{
366+
Group: "group1",
367+
Kind: "KIND2",
368+
},
369+
},
370+
Name: "NOTname",
371+
},
372+
err: errors.NewNotFound(schema.GroupResource{
373+
Group: "GROUP1",
374+
Resource: "kind2", // this is the kind
375+
},
376+
"notNAME",
377+
),
378+
},
379+
want: true,
380+
},
381+
{
382+
name: "case-insensitive not match due to different resource and different kinds",
383+
args: args{
384+
info: &resource.Info{
385+
Mapping: &meta.RESTMapping{
386+
Resource: "kind1",
387+
GroupVersionKind: schema.GroupVersionKind{
388+
Group: "group1",
389+
Kind: "kind3",
390+
},
391+
},
392+
Name: "NOTname",
393+
},
394+
err: errors.NewNotFound(schema.GroupResource{
395+
Group: "GROUP1",
396+
Resource: "kind2", // this is the kind
397+
},
398+
"notNAME",
399+
),
400+
},
401+
want: false,
402+
},
337403
}
338404
for _, tt := range tests {
339405
t.Run(tt.name, func(t *testing.T) {

0 commit comments

Comments
 (0)