From 1588970ec429df804708e8f00b66e4b94ba70723 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Wed, 28 Jan 2015 23:11:29 -0500 Subject: [PATCH] Turn 409 into 500 Try Again Later when using generateName If a client says they want the name to be generated, a 409 is not appropriate (since they didn't specify a name). Instead, we should return the next most appropriate error, which is a 5xx error indicating the request failed but the client *should* try again. Since there is no 5xx error that exactly fits this purpose, use 500 with StatusReasonTryAgainLater set. This commit does not implement client retry on TryAgainLater, but clients should retry up to a certain number of times. --- pkg/api/errors/errors.go | 27 ++++++++++++++++ pkg/api/errors/errors_test.go | 12 +++++++ pkg/api/rest/create.go | 42 ++++++++++++++++++++----- pkg/api/rest/create_test.go | 41 ++++++++++++++++++++++++ pkg/api/rest/resttest/resttest.go | 36 +++++++++++++++++++-- pkg/api/types.go | 11 +++++++ pkg/api/v1beta1/types.go | 11 +++++++ pkg/api/v1beta2/types.go | 11 +++++++ pkg/api/v1beta3/types.go | 11 +++++++ pkg/registry/controller/rest.go | 1 + pkg/registry/controller/rest_test.go | 7 +++-- pkg/registry/minion/rest.go | 4 +-- pkg/registry/minion/rest_test.go | 3 +- pkg/registry/pod/rest.go | 1 + pkg/registry/pod/rest_test.go | 5 +-- pkg/registry/registrytest/controller.go | 11 +++++++ pkg/registry/registrytest/minion.go | 6 ++++ pkg/registry/registrytest/pod.go | 6 ++++ pkg/registry/registrytest/service.go | 6 ++++ pkg/registry/service/rest.go | 1 + pkg/registry/service/rest_test.go | 2 +- 21 files changed, 237 insertions(+), 18 deletions(-) create mode 100644 pkg/api/rest/create_test.go diff --git a/pkg/api/errors/errors.go b/pkg/api/errors/errors.go index 930efe7dab2..e4345761ab1 100644 --- a/pkg/api/errors/errors.go +++ b/pkg/api/errors/errors.go @@ -174,6 +174,21 @@ func NewMethodNotSupported(kind, action string) error { }} } +// NewTryAgainLater returns an error indicating the requested action could not be completed due to a +// transient error, and the client should try again. +func NewTryAgainLater(kind, operation string) error { + return &StatusError{api.Status{ + Status: api.StatusFailure, + Code: http.StatusInternalServerError, + Reason: api.StatusReasonTryAgainLater, + Details: &api.StatusDetails{ + Kind: kind, + ID: operation, + }, + Message: fmt.Sprintf("The %s operation against %s could not be completed at this time, please try again.", operation, kind), + }} +} + // NewInternalError returns an error indicating the item is invalid and cannot be processed. func NewInternalError(err error) error { return &StatusError{api.Status{ @@ -218,6 +233,18 @@ func IsBadRequest(err error) bool { return reasonForError(err) == api.StatusReasonBadRequest } +// IsForbidden determines if err is an error which indicates that the request is forbidden and cannot +// be completed as requested. +func IsForbidden(err error) bool { + return reasonForError(err) == api.StatusReasonForbidden +} + +// IsTryAgainLater determines if err is an error which indicates that the request needs to be retried +// by the client. +func IsTryAgainLater(err error) bool { + return reasonForError(err) == api.StatusReasonTryAgainLater +} + func reasonForError(err error) api.StatusReason { switch t := err.(type) { case *StatusError: diff --git a/pkg/api/errors/errors_test.go b/pkg/api/errors/errors_test.go index d452b7cc7aa..a46583e25eb 100644 --- a/pkg/api/errors/errors_test.go +++ b/pkg/api/errors/errors_test.go @@ -43,6 +43,12 @@ func TestErrorNew(t *testing.T) { if IsBadRequest(err) { t.Errorf("expected to not be %s", api.StatusReasonBadRequest) } + if IsForbidden(err) { + t.Errorf("expected to not be %s", api.StatusReasonForbidden) + } + if IsTryAgainLater(err) { + t.Errorf("expected to not be %s", api.StatusReasonTryAgainLater) + } if IsMethodNotSupported(err) { t.Errorf("expected to not be %s", api.StatusReasonMethodNotAllowed) } @@ -59,6 +65,12 @@ func TestErrorNew(t *testing.T) { if !IsBadRequest(NewBadRequest("reason")) { t.Errorf("expected to be %s", api.StatusReasonBadRequest) } + if !IsForbidden(NewForbidden("test", "2", errors.New("reason"))) { + t.Errorf("expected to be %s", api.StatusReasonForbidden) + } + if !IsTryAgainLater(NewTryAgainLater("test", "reason")) { + t.Errorf("expected to be %s", api.StatusReasonTryAgainLater) + } if !IsMethodNotSupported(NewMethodNotSupported("foo", "delete")) { t.Errorf("expected to be %s", api.StatusReasonMethodNotAllowed) } diff --git a/pkg/api/rest/create.go b/pkg/api/rest/create.go index 67967497c4b..36b931b95c5 100644 --- a/pkg/api/rest/create.go +++ b/pkg/api/rest/create.go @@ -45,13 +45,9 @@ type RESTCreateStrategy interface { // errors that can be converted to api.Status. It invokes ResetBeforeCreate, then GenerateName, then Validate. // It returns nil if the object should be created. func BeforeCreate(strategy RESTCreateStrategy, ctx api.Context, obj runtime.Object) error { - _, kind, err := strategy.ObjectVersionAndKind(obj) - if err != nil { - return errors.NewInternalError(err) - } - objectMeta, err := api.ObjectMetaFor(obj) - if err != nil { - return errors.NewInternalError(err) + objectMeta, kind, kerr := objectMetaAndKind(strategy, obj) + if kerr != nil { + return kerr } if strategy.NamespaceScoped() { @@ -70,3 +66,35 @@ func BeforeCreate(strategy RESTCreateStrategy, ctx api.Context, obj runtime.Obje } return nil } + +// CheckGeneratedNameError checks whether an error that occured creating a resource is due +// to generation being unable to pick a valid name. +func CheckGeneratedNameError(strategy RESTCreateStrategy, err error, obj runtime.Object) error { + if !errors.IsAlreadyExists(err) { + return err + } + + objectMeta, kind, kerr := objectMetaAndKind(strategy, obj) + if kerr != nil { + return kerr + } + + if len(objectMeta.GenerateName) == 0 { + return err + } + + return errors.NewTryAgainLater(kind, "POST") +} + +// objectMetaAndKind retrieves kind and ObjectMeta from a runtime object, or returns an error. +func objectMetaAndKind(strategy RESTCreateStrategy, obj runtime.Object) (*api.ObjectMeta, string, error) { + objectMeta, err := api.ObjectMetaFor(obj) + if err != nil { + return nil, "", errors.NewInternalError(err) + } + _, kind, err := strategy.ObjectVersionAndKind(obj) + if err != nil { + return nil, "", errors.NewInternalError(err) + } + return objectMeta, kind, nil +} diff --git a/pkg/api/rest/create_test.go b/pkg/api/rest/create_test.go new file mode 100644 index 00000000000..c8ad1490225 --- /dev/null +++ b/pkg/api/rest/create_test.go @@ -0,0 +1,41 @@ +/* +Copyright 2014 Google Inc. All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package rest + +import ( + "testing" + + "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" +) + +func TestCheckGeneratedNameError(t *testing.T) { + expect := errors.NewNotFound("foo", "bar") + if err := CheckGeneratedNameError(Pods, expect, &api.Pod{}); err != expect { + t.Errorf("NotFoundError should be ignored: %v", err) + } + + expect = errors.NewAlreadyExists("foo", "bar") + if err := CheckGeneratedNameError(Pods, expect, &api.Pod{}); err != expect { + t.Errorf("AlreadyExists should be returned when no GenerateName field: %v", err) + } + + expect = errors.NewAlreadyExists("foo", "bar") + if err := CheckGeneratedNameError(Pods, expect, &api.Pod{ObjectMeta: api.ObjectMeta{GenerateName: "foo"}}); err == nil || !errors.IsTryAgainLater(err) { + t.Errorf("expected try again later error: %v", err) + } +} diff --git a/pkg/api/rest/resttest/resttest.go b/pkg/api/rest/resttest/resttest.go index 3b0a855aaef..fa3fb7de03e 100644 --- a/pkg/api/rest/resttest/resttest.go +++ b/pkg/api/rest/resttest/resttest.go @@ -30,16 +30,26 @@ import ( type Tester struct { *testing.T storage apiserver.RESTStorage + storageError injectErrorFunc clusterScope bool } -func New(t *testing.T, storage apiserver.RESTStorage) *Tester { +type injectErrorFunc func(err error) + +func New(t *testing.T, storage apiserver.RESTStorage, storageError injectErrorFunc) *Tester { return &Tester{ - T: t, - storage: storage, + T: t, + storage: storage, + storageError: storageError, } } +func (t *Tester) withStorageError(err error, fn func()) { + t.storageError(err) + defer t.storageError(nil) + fn() +} + func (t *Tester) ClusterScope() *Tester { t.clusterScope = true return t @@ -56,6 +66,7 @@ func copyOrDie(obj runtime.Object) runtime.Object { func (t *Tester) TestCreate(valid runtime.Object, invalid ...runtime.Object) { t.TestCreateHasMetadata(copyOrDie(valid)) t.TestCreateGeneratesName(copyOrDie(valid)) + t.TestCreateGeneratesNameReturnsTryAgain(copyOrDie(valid)) if t.clusterScope { t.TestCreateRejectsNamespace(copyOrDie(valid)) } else { @@ -129,6 +140,25 @@ func (t *Tester) TestCreateGeneratesName(valid runtime.Object) { } } +func (t *Tester) TestCreateGeneratesNameReturnsTryAgain(valid runtime.Object) { + objectMeta, err := api.ObjectMetaFor(valid) + if err != nil { + t.Fatalf("object does not have ObjectMeta: %v\n%#v", err, valid) + } + + objectMeta.GenerateName = "test-" + t.withStorageError(errors.NewAlreadyExists("kind", "thing"), func() { + ch, err := t.storage.(apiserver.RESTCreater).Create(api.NewDefaultContext(), valid) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + res := <-ch + if err := errors.FromObject(res.Object); err == nil || !errors.IsTryAgainLater(err) { + t.Fatalf("Unexpected error: %v", err) + } + }) +} + func (t *Tester) TestCreateInvokesValidation(invalid ...runtime.Object) { for i, obj := range invalid { ctx := api.NewDefaultContext() diff --git a/pkg/api/types.go b/pkg/api/types.go index c9f94215d95..05f1b0d0ac7 100644 --- a/pkg/api/types.go +++ b/pkg/api/types.go @@ -952,6 +952,17 @@ const ( // Status code 422 StatusReasonInvalid StatusReason = "Invalid" + // StatusReasonTryAgainLater means the server can be reached and understood the request, + // but cannot complete the action in a reasonable time. The client should retry the request. + // This is may be due to temporary server load or a transient communication issue with + // another server. Status code 500 is used because the HTTP spec provides no suitable + // server-requested client retry and the 5xx class represents actionable errors. + // Details (optional): + // "kind" string - the kind attribute of the resource being acted on. + // "id" string - the operation that is being attempted. + // Status code 500 + StatusReasonTryAgainLater StatusReason = "TryAgainLater" + // StatusReasonTimeout means that the request could not be completed within the given time. // Clients can get this response only when they specified a timeout param in the request. // The request might succeed with an increased value of timeout param. diff --git a/pkg/api/v1beta1/types.go b/pkg/api/v1beta1/types.go index 4c02861ea87..a3e49a49c34 100644 --- a/pkg/api/v1beta1/types.go +++ b/pkg/api/v1beta1/types.go @@ -745,6 +745,17 @@ const ( // conflict. // Status code 409 StatusReasonConflict StatusReason = "Conflict" + + // StatusReasonTryAgainLater means the server can be reached and understood the request, + // but cannot complete the action in a reasonable time. The client should retry the request. + // This is may be due to temporary server load or a transient communication issue with + // another server. Status code 500 is used because the HTTP spec provides no suitable + // server-requested client retry and the 5xx class represents actionable errors. + // Details (optional): + // "kind" string - the kind attribute of the resource being acted on. + // "id" string - the operation that is being attempted. + // Status code 500 + StatusReasonTryAgainLater StatusReason = "TryAgainLater" ) // StatusCause provides more information about an api.Status failure, including diff --git a/pkg/api/v1beta2/types.go b/pkg/api/v1beta2/types.go index 2d00faff0aa..79353bf3fd5 100644 --- a/pkg/api/v1beta2/types.go +++ b/pkg/api/v1beta2/types.go @@ -719,6 +719,17 @@ const ( // field attributes will be set. // Status code 422 StatusReasonInvalid StatusReason = "Invalid" + + // StatusReasonTryAgainLater means the server can be reached and understood the request, + // but cannot complete the action in a reasonable time. The client should retry the request. + // This is may be due to temporary server load or a transient communication issue with + // another server. Status code 500 is used because the HTTP spec provides no suitable + // server-requested client retry and the 5xx class represents actionable errors. + // Details (optional): + // "kind" string - the kind attribute of the resource being acted on. + // "id" string - the operation that is being attempted. + // Status code 500 + StatusReasonTryAgainLater StatusReason = "TryAgainLater" ) // StatusCause provides more information about an api.Status failure, including diff --git a/pkg/api/v1beta3/types.go b/pkg/api/v1beta3/types.go index 761e1c58f20..c8918aa6f69 100644 --- a/pkg/api/v1beta3/types.go +++ b/pkg/api/v1beta3/types.go @@ -949,6 +949,17 @@ const ( // field attributes will be set. // Status code 422 StatusReasonInvalid StatusReason = "Invalid" + + // StatusReasonTryAgainLater means the server can be reached and understood the request, + // but cannot complete the action in a reasonable time. The client should retry the request. + // This is may be due to temporary server load or a transient communication issue with + // another server. Status code 500 is used because the HTTP spec provides no suitable + // server-requested client retry and the 5xx class represents actionable errors. + // Details (optional): + // "kind" string - the kind attribute of the resource being acted on. + // "id" string - the operation that is being attempted. + // Status code 500 + StatusReasonTryAgainLater StatusReason = "TryAgainLater" ) // StatusCause provides more information about an api.Status failure, including diff --git a/pkg/registry/controller/rest.go b/pkg/registry/controller/rest.go index bd8f75e3259..94c13664ef1 100644 --- a/pkg/registry/controller/rest.go +++ b/pkg/registry/controller/rest.go @@ -62,6 +62,7 @@ func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan apiserver.RE return apiserver.MakeAsync(func() (runtime.Object, error) { if err := rs.registry.CreateController(ctx, controller); err != nil { + err = rest.CheckGeneratedNameError(rest.ReplicationControllers, err, controller) return apiserver.RESTResult{}, err } return rs.registry.GetController(ctx, controller.Name) diff --git a/pkg/registry/controller/rest_test.go b/pkg/registry/controller/rest_test.go index ed76e04ed81..aaaea3e9460 100644 --- a/pkg/registry/controller/rest_test.go +++ b/pkg/registry/controller/rest_test.go @@ -51,7 +51,9 @@ func TestListControllersError(t *testing.T) { } func TestListEmptyControllerList(t *testing.T) { - mockRegistry := registrytest.ControllerRegistry{nil, &api.ReplicationControllerList{ListMeta: api.ListMeta{ResourceVersion: "1"}}} + mockRegistry := registrytest.ControllerRegistry{ + Controllers: &api.ReplicationControllerList{ListMeta: api.ListMeta{ResourceVersion: "1"}}, + } storage := REST{ registry: &mockRegistry, } @@ -444,7 +446,8 @@ func TestUpdateControllerWithConflictingNamespace(t *testing.T) { } func TestCreate(t *testing.T) { - test := resttest.New(t, NewREST(®istrytest.ControllerRegistry{}, nil)) + registry := ®istrytest.ControllerRegistry{} + test := resttest.New(t, NewREST(registry, nil), registry.SetError) test.TestCreate( // valid &api.ReplicationController{ diff --git a/pkg/registry/minion/rest.go b/pkg/registry/minion/rest.go index b67fce85505..5c0ab637383 100644 --- a/pkg/registry/minion/rest.go +++ b/pkg/registry/minion/rest.go @@ -60,8 +60,8 @@ func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan apiserver.RE } return apiserver.MakeAsync(func() (runtime.Object, error) { - err := rs.registry.CreateMinion(ctx, minion) - if err != nil { + if err := rs.registry.CreateMinion(ctx, minion); err != nil { + err = rest.CheckGeneratedNameError(rest.Nodes, err, minion) return nil, err } return minion, nil diff --git a/pkg/registry/minion/rest_test.go b/pkg/registry/minion/rest_test.go index 7dfb530d8e3..0e3e2c87b83 100644 --- a/pkg/registry/minion/rest_test.go +++ b/pkg/registry/minion/rest_test.go @@ -154,7 +154,8 @@ func contains(nodes *api.NodeList, nodeID string) bool { } func TestCreate(t *testing.T) { - test := resttest.New(t, NewREST(registrytest.NewMinionRegistry([]string{"foo", "bar"}, api.NodeResources{}))).ClusterScope() + registry := registrytest.NewMinionRegistry([]string{"foo", "bar"}, api.NodeResources{}) + test := resttest.New(t, NewREST(registry), registry.SetError).ClusterScope() test.TestCreate( // valid &api.Node{ diff --git a/pkg/registry/pod/rest.go b/pkg/registry/pod/rest.go index c7f9ee6dfcd..b27290b1fdb 100644 --- a/pkg/registry/pod/rest.go +++ b/pkg/registry/pod/rest.go @@ -64,6 +64,7 @@ func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan apiserver.RE return apiserver.MakeAsync(func() (runtime.Object, error) { if err := rs.registry.CreatePod(ctx, pod); err != nil { + err = rest.CheckGeneratedNameError(rest.Pods, err, pod) return nil, err } return rs.registry.GetPod(ctx, pod.Name) diff --git a/pkg/registry/pod/rest_test.go b/pkg/registry/pod/rest_test.go index c89d5f26662..aefbb509fcb 100644 --- a/pkg/registry/pod/rest_test.go +++ b/pkg/registry/pod/rest_test.go @@ -636,10 +636,11 @@ func TestDeletePod(t *testing.T) { } func TestCreate(t *testing.T) { + registry := registrytest.NewPodRegistry(nil) test := resttest.New(t, &REST{ - registry: registrytest.NewPodRegistry(nil), + registry: registry, podCache: &fakeCache{statusToReturn: &api.PodStatus{}}, - }) + }, registry.SetError) test.TestCreate( // valid &api.Pod{ diff --git a/pkg/registry/registrytest/controller.go b/pkg/registry/registrytest/controller.go index e0476574042..9e83ff86668 100644 --- a/pkg/registry/registrytest/controller.go +++ b/pkg/registry/registrytest/controller.go @@ -17,6 +17,8 @@ limitations under the License. package registrytest import ( + "sync" + "github.com/GoogleCloudPlatform/kubernetes/pkg/api" "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" "github.com/GoogleCloudPlatform/kubernetes/pkg/watch" @@ -26,6 +28,13 @@ import ( type ControllerRegistry struct { Err error Controllers *api.ReplicationControllerList + sync.Mutex +} + +func (r *ControllerRegistry) SetError(err error) { + r.Lock() + defer r.Unlock() + r.Err = err } func (r *ControllerRegistry) ListControllers(ctx api.Context) (*api.ReplicationControllerList, error) { @@ -37,6 +46,8 @@ func (r *ControllerRegistry) GetController(ctx api.Context, ID string) (*api.Rep } func (r *ControllerRegistry) CreateController(ctx api.Context, controller *api.ReplicationController) error { + r.Lock() + defer r.Unlock() return r.Err } diff --git a/pkg/registry/registrytest/minion.go b/pkg/registry/registrytest/minion.go index 13db8ff2592..c44b2802e2e 100644 --- a/pkg/registry/registrytest/minion.go +++ b/pkg/registry/registrytest/minion.go @@ -52,6 +52,12 @@ func NewMinionRegistry(minions []string, nodeResources api.NodeResources) *Minio } } +func (r *MinionRegistry) SetError(err error) { + r.Lock() + defer r.Unlock() + r.Err = err +} + func (r *MinionRegistry) ListMinions(ctx api.Context) (*api.NodeList, error) { r.Lock() defer r.Unlock() diff --git a/pkg/registry/registrytest/pod.go b/pkg/registry/registrytest/pod.go index 571b61074b2..6bfa0eed471 100644 --- a/pkg/registry/registrytest/pod.go +++ b/pkg/registry/registrytest/pod.go @@ -40,6 +40,12 @@ func NewPodRegistry(pods *api.PodList) *PodRegistry { } } +func (r *PodRegistry) SetError(err error) { + r.Lock() + defer r.Unlock() + r.Err = err +} + func (r *PodRegistry) ListPodsPredicate(ctx api.Context, filter func(*api.Pod) bool) (*api.PodList, error) { r.Lock() defer r.Unlock() diff --git a/pkg/registry/registrytest/service.go b/pkg/registry/registrytest/service.go index ec0d393b3eb..d1cb2ff63a0 100644 --- a/pkg/registry/registrytest/service.go +++ b/pkg/registry/registrytest/service.go @@ -41,6 +41,12 @@ type ServiceRegistry struct { UpdatedID string } +func (r *ServiceRegistry) SetError(err error) { + r.mu.Lock() + defer r.mu.Unlock() + r.Err = err +} + func (r *ServiceRegistry) ListServices(ctx api.Context) (*api.ServiceList, error) { r.mu.Lock() defer r.mu.Unlock() diff --git a/pkg/registry/service/rest.go b/pkg/registry/service/rest.go index e7af0cd605f..70ed8ca69ef 100644 --- a/pkg/registry/service/rest.go +++ b/pkg/registry/service/rest.go @@ -152,6 +152,7 @@ func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan apiserver.RE } if err := rs.registry.CreateService(ctx, service); err != nil { + err = rest.CheckGeneratedNameError(rest.Services, err, service) return nil, err } return rs.registry.GetService(ctx, service.Name) diff --git a/pkg/registry/service/rest_test.go b/pkg/registry/service/rest_test.go index 4c4d1beb5f3..b18d7de0ad8 100644 --- a/pkg/registry/service/rest_test.go +++ b/pkg/registry/service/rest_test.go @@ -666,7 +666,7 @@ func TestCreate(t *testing.T) { rest := NewREST(registry, fakeCloud, registrytest.NewMinionRegistry(machines, api.NodeResources{}), makeIPNet(t)) rest.portalMgr.randomAttempts = 0 - test := resttest.New(t, rest) + test := resttest.New(t, rest, registry.SetError) test.TestCreate( // valid &api.Service{