From bbb7104f7ceeb9e5bddfdb2f4e83a5d767e268ec Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Fri, 27 Oct 2017 17:44:33 -0400 Subject: [PATCH 1/2] Correctly handle NotFound errors during migration This change makes it so that the migrate storage command will now reprocess the body of a failed GET request to follow the standard path of extracting a status error. This normalizes the structure of the errors the code must handle. When comparing the resource.Info to an error, the info's REST mapping is now used to extract group kind information since the associated runtime.Object is not guaranteed to have valid type meta data. This is combined with relaxed case-insensitive matching to make sure that we only fail the migrate storage command on NotFound errors that we know do not match the info. When a NotFound error occurs, the migrate command now correctly reports that it did not change the resource. Previously it reported that it had successfully migrated the resource, which is not true since it is impossible to migrate a resource that does not exist. Bug 1506006 Signed-off-by: Monis Khan --- pkg/oc/admin/migrate/migrator.go | 22 +- pkg/oc/admin/migrate/migrator_test.go | 345 ++++++++++++++++++++++++ pkg/oc/admin/migrate/storage/storage.go | 6 +- 3 files changed, 365 insertions(+), 8 deletions(-) create mode 100644 pkg/oc/admin/migrate/migrator_test.go diff --git a/pkg/oc/admin/migrate/migrator.go b/pkg/oc/admin/migrate/migrator.go index c354a6fb826b..21b7e347e00c 100644 --- a/pkg/oc/admin/migrate/migrator.go +++ b/pkg/oc/admin/migrate/migrator.go @@ -484,11 +484,13 @@ func canRetry(err error) bool { // All other errors are left in their natural state - they will not be retried unless // they define a Temporary() method that returns true. func DefaultRetriable(info *resource.Info, err error) error { - // tolerate the deletion of resources during migration - if err == nil || isNotFoundForInfo(info, err) { - return nil - } switch { + case err == nil: + return nil + case isNotFoundForInfo(info, err): + // tolerate the deletion of resources during migration + // report unchanged since we did not actually migrate this object + return ErrUnchanged case errors.IsMethodNotSupported(err): return ErrNotRetriable{err} case errors.IsConflict(err): @@ -498,8 +500,9 @@ func DefaultRetriable(info *resource.Info, err error) error { return ErrRetriable{err} case errors.IsServerTimeout(err): return ErrRetriable{err} + default: + return err } - return err } // isNotFoundForInfo returns true iff the error is a not found for the specific info object. @@ -515,6 +518,11 @@ func isNotFoundForInfo(info *resource.Info, err error) bool { if details == nil { return false } - gvk := info.Object.GetObjectKind().GroupVersionKind() - return details.Name == info.Name && details.Kind == gvk.Kind && (details.Group == "" || details.Group == gvk.Group) + // get schema.GroupKind from the mapping since the actual object may not have type meta filled out + gk := info.Mapping.GroupVersionKind.GroupKind() + // based on case-insensitive string comparisons, the error matches info iff + // the name and kind match + // the group match, but only if both the error and info specify a group + return strings.EqualFold(details.Name, info.Name) && strings.EqualFold(details.Kind, gk.Kind) && + (len(details.Group) == 0 || len(gk.Group) == 0 || strings.EqualFold(details.Group, gk.Group)) } diff --git a/pkg/oc/admin/migrate/migrator_test.go b/pkg/oc/admin/migrate/migrator_test.go new file mode 100644 index 000000000000..a8c62a392988 --- /dev/null +++ b/pkg/oc/admin/migrate/migrator_test.go @@ -0,0 +1,345 @@ +package migrate + +import ( + "fmt" + "net/http" + "testing" + + "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/kubernetes/pkg/kubectl/resource" +) + +func TestIsNotFoundForInfo(t *testing.T) { + type args struct { + info *resource.Info + err error + } + tests := []struct { + name string + args args + want bool + }{ + { + name: "nil err does not match", + args: args{ + info: nil, + err: nil, + }, + want: false, + }, + { + name: "simple not found match", + args: args{ + info: &resource.Info{ + Mapping: &meta.RESTMapping{ + GroupVersionKind: schema.GroupVersionKind{ + Group: "group1", + Kind: "kind1", + }, + }, + Name: "name1", + }, + err: errors.NewNotFound(schema.GroupResource{ + Group: "group1", + Resource: "kind1", // this is the kind + }, + "name1", + ), + }, + want: true, + }, + { + name: "simple not found match from generic 404 response", + args: args{ + info: &resource.Info{ + Mapping: &meta.RESTMapping{ + GroupVersionKind: schema.GroupVersionKind{ + Group: "group1", + Kind: "kind1", + }, + }, + Name: "name1", + }, + err: errors.NewGenericServerResponse( + http.StatusNotFound, + "", + schema.GroupResource{ + Group: "group1", + Resource: "kind1", // this is the kind + }, + "name1", + "", + 0, + false, + ), + }, + want: true, + }, + { + name: "simple not match from generic 400 response", + args: args{ + info: &resource.Info{ + Mapping: &meta.RESTMapping{ + GroupVersionKind: schema.GroupVersionKind{ + Group: "group1", + Kind: "kind1", + }, + }, + Name: "name1", + }, + err: errors.NewGenericServerResponse( + http.StatusBadRequest, + "", + schema.GroupResource{ + Group: "group1", + Resource: "kind1", // this is the kind + }, + "name1", + "", + 0, + false, + ), + }, + want: false, + }, + { + name: "different status error does not match", + args: args{ + info: &resource.Info{ + Mapping: &meta.RESTMapping{ + GroupVersionKind: schema.GroupVersionKind{ + Group: "group1", + Kind: "kind1", + }, + }, + Name: "name1", + }, + err: errors.NewAlreadyExists(schema.GroupResource{ + Group: "group1", + Resource: "kind1", // this is the kind + }, + "name1", + ), + }, + want: false, + }, + { + name: "non status error does not match", + args: args{ + info: &resource.Info{ + Mapping: &meta.RESTMapping{ + GroupVersionKind: schema.GroupVersionKind{ + Group: "group1", + Kind: "kind1", + }, + }, + Name: "name1", + }, + err: fmt.Errorf("%v", + schema.GroupVersionKind{ + Group: "group1", + Kind: "kind1", + }, + ), + }, + want: false, + }, + { + name: "case-insensitive not found match", + args: args{ + info: &resource.Info{ + Mapping: &meta.RESTMapping{ + GroupVersionKind: schema.GroupVersionKind{ + Group: "GROUPA", + Kind: "KINDB", + }, + }, + Name: "NOTname", + }, + err: errors.NewNotFound(schema.GroupResource{ + Group: "groupA", + Resource: "Kindb", // this is the kind + }, + "notNAME", + ), + }, + want: true, + }, + { + name: "case-insensitive not found match from generic 404 response", + args: args{ + info: &resource.Info{ + Mapping: &meta.RESTMapping{ + GroupVersionKind: schema.GroupVersionKind{ + Group: "ThisGroup", + Kind: "HasKinds", + }, + }, + Name: "AndAName", + }, + err: errors.NewGenericServerResponse( + http.StatusNotFound, + "", + schema.GroupResource{ + Group: "thisgroup", + Resource: "haskinds", // this is the kind + }, + "andaname", + "", + 0, + false, + ), + }, + want: true, + }, + { + name: "case-insensitive not found match, no group in info", + args: args{ + info: &resource.Info{ + Mapping: &meta.RESTMapping{ + GroupVersionKind: schema.GroupVersionKind{ + Kind: "KINDB", + }, + }, + Name: "NOTname", + }, + err: errors.NewNotFound(schema.GroupResource{ + Group: "groupA", + Resource: "Kindb", // this is the kind + }, + "notNAME", + ), + }, + want: true, + }, + { + name: "case-insensitive not found match, no group in error", + args: args{ + info: &resource.Info{ + Mapping: &meta.RESTMapping{ + GroupVersionKind: schema.GroupVersionKind{ + Group: "GROUPA", + Kind: "KINDB", + }, + }, + Name: "NOTname", + }, + err: errors.NewNotFound(schema.GroupResource{ + Resource: "Kindb", // this is the kind + }, + "notNAME", + ), + }, + want: true, + }, + { + name: "case-insensitive not match due to different groups", + args: args{ + info: &resource.Info{ + Mapping: &meta.RESTMapping{ + GroupVersionKind: schema.GroupVersionKind{ + Group: "group1", + Kind: "KINDB", + }, + }, + Name: "NOTname", + }, + err: errors.NewNotFound(schema.GroupResource{ + Group: "group2", + Resource: "Kindb", // this is the kind + }, + "notNAME", + ), + }, + want: false, + }, + { + name: "case-insensitive not found match from generic 404 response, no group in info", + args: args{ + info: &resource.Info{ + Mapping: &meta.RESTMapping{ + GroupVersionKind: schema.GroupVersionKind{ + Kind: "HasKinds", + }, + }, + Name: "AndAName", + }, + err: errors.NewGenericServerResponse( + http.StatusNotFound, + "", + schema.GroupResource{ + Group: "thisgroup", + Resource: "haskinds", // this is the kind + }, + "andaname", + "", + 0, + false, + ), + }, + want: true, + }, + { + name: "case-insensitive not found match from generic 404 response, no group in error", + args: args{ + info: &resource.Info{ + Mapping: &meta.RESTMapping{ + GroupVersionKind: schema.GroupVersionKind{ + Group: "ThisGroup", + Kind: "HasKinds", + }, + }, + Name: "AndAName", + }, + err: errors.NewGenericServerResponse( + http.StatusNotFound, + "", + schema.GroupResource{ + Resource: "haskinds", // this is the kind + }, + "andaname", + "", + 0, + false, + ), + }, + want: true, + }, + { + name: "case-insensitive not match from generic 404 response due to different groups", + args: args{ + info: &resource.Info{ + Mapping: &meta.RESTMapping{ + GroupVersionKind: schema.GroupVersionKind{ + Group: "thingA", + Kind: "HasKinds", + }, + }, + Name: "AndAName", + }, + err: errors.NewGenericServerResponse( + http.StatusNotFound, + "", + schema.GroupResource{ + Group: "thingB", + Resource: "haskinds", // this is the kind + }, + "andaname", + "", + 0, + false, + ), + }, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isNotFoundForInfo(tt.args.info, tt.args.err); got != tt.want { + t.Errorf("isNotFoundForInfo() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/oc/admin/migrate/storage/storage.go b/pkg/oc/admin/migrate/storage/storage.go index 39da2834c16b..2539792a4d00 100644 --- a/pkg/oc/admin/migrate/storage/storage.go +++ b/pkg/oc/admin/migrate/storage/storage.go @@ -202,7 +202,11 @@ func (o *MigrateAPIStorageOptions) save(info *resource.Info, reporter migrate.Re Name(info.Name).Do() data, err := get.Raw() if err != nil { - return migrate.DefaultRetriable(info, err) + // since we have an error, processing the body is safe because we are not going + // to send it back to the server. Thus we can safely call Result.Error(). + // This is required because we want to make sure we pass an errors.APIStatus so + // that DefaultRetriable can correctly determine if the error is safe to retry. + return migrate.DefaultRetriable(info, get.Error()) } update := info.Client.Put(). Resource(info.Mapping.Resource). From 24614f6f5270b3fb0221bcda89c7a97c707d0ec8 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Fri, 27 Oct 2017 22:42:49 -0400 Subject: [PATCH 2/2] Add timestamps to migration command's reporting This change adds glog style timestamps to the reporting output of the migration tracker. This will aid in debugging migration errors by making it easier to correlate client errors with master logs. Signed-off-by: Monis Khan --- pkg/oc/admin/migrate/migrator.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/pkg/oc/admin/migrate/migrator.go b/pkg/oc/admin/migrate/migrator.go index 21b7e347e00c..9c03567086ce 100644 --- a/pkg/oc/admin/migrate/migrator.go +++ b/pkg/oc/admin/migrate/migrator.go @@ -4,6 +4,7 @@ import ( "fmt" "io" "strings" + "time" "github.com/golang/glog" "github.com/spf13/cobra" @@ -49,6 +50,11 @@ func AlwaysRequiresMigration(_ *resource.Info) (Reporter, error) { return ReporterBool(true), nil } +// timeStampNow returns the current time in the same format as glog +func timeStampNow() string { + return time.Now().Format("0102 15:04:05.000000") +} + // NotChanged is a Reporter returned by operations that are guaranteed to be read-only var NotChanged = ReporterBool(false) @@ -397,9 +403,9 @@ func (t *migrateTracker) report(prefix string, info *resource.Info, err error) { ns = "-n " + ns } if err != nil { - fmt.Fprintf(t.out, "%-10s %s %s/%s: %v\n", prefix, ns, info.Mapping.Resource, info.Name, err) + fmt.Fprintf(t.out, "E%s %-10s %s %s/%s: %v\n", timeStampNow(), prefix, ns, info.Mapping.Resource, info.Name, err) } else { - fmt.Fprintf(t.out, "%-10s %s %s/%s\n", prefix, ns, info.Mapping.Resource, info.Name) + fmt.Fprintf(t.out, "I%s %-10s %s %s/%s\n", timeStampNow(), prefix, ns, info.Mapping.Resource, info.Name) } }