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

@@ -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:

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)
 	}

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
+}

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)
+	}
+}

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()

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.

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

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

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

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)

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(&registrytest.ControllerRegistry{}, nil))
+	registry := &registrytest.ControllerRegistry{}
+	test := resttest.New(t, NewREST(registry, nil), registry.SetError)
 	test.TestCreate(
 		// valid
 		&api.ReplicationController{

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

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{

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)

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{

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
 }
 

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()

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()

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()

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)

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{