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{