Merge pull request kubernetes#1192 from smarterclayton/standardize_et…

…cd_errors Return standard API errors from etcd registry by operation
duda · Sep 8, 2014 · 41754f5 · 41754f5
2 parents 81b502b + 3ffe259
commit 41754f5
Show file tree

Hide file tree

Showing 4 changed files with 121 additions and 58 deletions.
diff --git a/pkg/api/errors/etcd/doc.go b/pkg/api/errors/etcd/doc.go
@@ -0,0 +1,18 @@
+/*
+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 etcd provides conversion of etcd errors to API errors.
+package etcd
diff --git a/pkg/api/errors/etcd/etcd.go b/pkg/api/errors/etcd/etcd.go
@@ -0,0 +1,66 @@
+/*
+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 etcd
+
+import (
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/tools"
+)
+
+// InterpretGetError converts a generic etcd error on a retrieval
+// operation into the appropriate API error.
+func InterpretGetError(err error, kind, name string) error {
+	switch {
+	case tools.IsEtcdNotFound(err):
+		return errors.NewNotFound(kind, name)
+	default:
+		return err
+	}
+}
+
+// InterpretCreateError converts a generic etcd error on a create
+// operation into the appropriate API error.
+func InterpretCreateError(err error, kind, name string) error {
+	switch {
+	case tools.IsEtcdNodeExist(err):
+		return errors.NewAlreadyExists(kind, name)
+	default:
+		return err
+	}
+}
+
+// InterpretUpdateError converts a generic etcd error on a create
+// operation into the appropriate API error.
+func InterpretUpdateError(err error, kind, name string) error {
+	switch {
+	case tools.IsEtcdTestFailed(err), tools.IsEtcdNodeExist(err):
+		return errors.NewConflict(kind, name, err)
+	default:
+		return err
+	}
+}
+
+// InterpretDeleteError converts a generic etcd error on a create
+// operation into the appropriate API error.
+func InterpretDeleteError(err error, kind, name string) error {
+	switch {
+	case tools.IsEtcdNotFound(err):
+		return errors.NewNotFound(kind, name)
+	default:
+		return err
+	}
+}
diff --git a/pkg/registry/etcd/etcd.go b/pkg/registry/etcd/etcd.go
@@ -20,7 +20,7 @@ import (
 	"fmt"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
-	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
+	etcderr "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors/etcd"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/constraint"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/labels"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
@@ -96,7 +96,7 @@ func (r *Registry) WatchPods(resourceVersion uint64, filter func(*api.Pod) bool)
 func (r *Registry) GetPod(podID string) (*api.Pod, error) {
 	var pod api.Pod
 	if err := r.ExtractObj(makePodKey(podID), &pod, false); err != nil {
-		return nil, err
+		return nil, etcderr.InterpretGetError(err, "pod", podID)
 	}
 	// TODO: Currently nothing sets CurrentState.Host. We need a feedback loop that sets
 	// the CurrentState.Host and Status fields. Here we pretend that reality perfectly
@@ -117,12 +117,13 @@ func (r *Registry) CreatePod(pod *api.Pod) error {
 	// DesiredState.Host == "" is a signal to the scheduler that this pod needs scheduling.
 	pod.DesiredState.Status = api.PodRunning
 	pod.DesiredState.Host = ""
-	return r.CreateObj(makePodKey(pod.ID), pod)
+	err := r.CreateObj(makePodKey(pod.ID), pod)
+	return etcderr.InterpretCreateError(err, "pod", pod.ID)
 }
 
 // ApplyBinding implements binding's registry
 func (r *Registry) ApplyBinding(binding *api.Binding) error {
-	return r.assignPod(binding.PodID, binding.Host)
+	return etcderr.InterpretCreateError(r.assignPod(binding.PodID, binding.Host), "binding", "")
 }
 
 // setPodHostTo sets the given pod's host to 'machine' iff it was previously 'oldMachine'.
@@ -183,20 +184,14 @@ func (r *Registry) DeletePod(podID string) error {
 	var pod api.Pod
 	podKey := makePodKey(podID)
 	err := r.ExtractObj(podKey, &pod, false)
-	if tools.IsEtcdNotFound(err) {
-		return errors.NewNotFound("pod", podID)
-	}
 	if err != nil {
-		return err
+		return etcderr.InterpretDeleteError(err, "pod", podID)
 	}
 	// First delete the pod, so a scheduler doesn't notice it getting removed from the
 	// machine and attempt to put it somewhere.
 	err = r.Delete(podKey, true)
-	if tools.IsEtcdNotFound(err) {
-		return errors.NewNotFound("pod", podID)
-	}
 	if err != nil {
-		return err
+		return etcderr.InterpretDeleteError(err, "pod", podID)
 	}
 	machine := pod.DesiredState.Host
 	if machine == "" {
@@ -248,37 +243,29 @@ func (r *Registry) GetController(controllerID string) (*api.ReplicationControlle
 	var controller api.ReplicationController
 	key := makeControllerKey(controllerID)
 	err := r.ExtractObj(key, &controller, false)
-	if tools.IsEtcdNotFound(err) {
-		return nil, errors.NewNotFound("replicationController", controllerID)
-	}
 	if err != nil {
-		return nil, err
+		return nil, etcderr.InterpretGetError(err, "replicationController", controllerID)
 	}
 	return &controller, nil
 }
 
 // CreateController creates a new ReplicationController.
 func (r *Registry) CreateController(controller *api.ReplicationController) error {
 	err := r.CreateObj(makeControllerKey(controller.ID), controller)
-	if tools.IsEtcdNodeExist(err) {
-		return errors.NewAlreadyExists("replicationController", controller.ID)
-	}
-	return err
+	return etcderr.InterpretCreateError(err, "replicationController", controller.ID)
 }
 
 // UpdateController replaces an existing ReplicationController.
 func (r *Registry) UpdateController(controller *api.ReplicationController) error {
-	return r.SetObj(makeControllerKey(controller.ID), controller)
+	err := r.SetObj(makeControllerKey(controller.ID), controller)
+	return etcderr.InterpretUpdateError(err, "replicationController", controller.ID)
 }
 
 // DeleteController deletes a ReplicationController specified by its ID.
 func (r *Registry) DeleteController(controllerID string) error {
 	key := makeControllerKey(controllerID)
 	err := r.Delete(key, false)
-	if tools.IsEtcdNotFound(err) {
-		return errors.NewNotFound("replicationController", controllerID)
-	}
-	return err
+	return etcderr.InterpretDeleteError(err, "replicationController", controllerID)
 }
 
 func makeServiceKey(name string) string {
@@ -295,22 +282,16 @@ func (r *Registry) ListServices() (*api.ServiceList, error) {
 // CreateService creates a new Service.
 func (r *Registry) CreateService(svc *api.Service) error {
 	err := r.CreateObj(makeServiceKey(svc.ID), svc)
-	if tools.IsEtcdNodeExist(err) {
-		return errors.NewAlreadyExists("service", svc.ID)
-	}
-	return err
+	return etcderr.InterpretCreateError(err, "service", svc.ID)
 }
 
 // GetService obtains a Service specified by its name.
 func (r *Registry) GetService(name string) (*api.Service, error) {
 	key := makeServiceKey(name)
 	var svc api.Service
 	err := r.ExtractObj(key, &svc, false)
-	if tools.IsEtcdNotFound(err) {
-		return nil, errors.NewNotFound("service", name)
-	}
 	if err != nil {
-		return nil, err
+		return nil, etcderr.InterpretGetError(err, "service", name)
 	}
 	return &svc, nil
 }
@@ -320,11 +301,8 @@ func (r *Registry) GetEndpoints(name string) (*api.Endpoints, error) {
 	key := makeServiceEndpointsKey(name)
 	var endpoints api.Endpoints
 	err := r.ExtractObj(key, &endpoints, false)
-	if tools.IsEtcdNotFound(err) {
-		return nil, errors.NewNotFound("endpoints", name)
-	}
 	if err != nil {
-		return nil, err
+		return nil, etcderr.InterpretGetError(err, "endpoints", name)
 	}
 	return &endpoints, nil
 }
@@ -337,23 +315,23 @@ func makeServiceEndpointsKey(name string) string {
 func (r *Registry) DeleteService(name string) error {
 	key := makeServiceKey(name)
 	err := r.Delete(key, true)
-	if tools.IsEtcdNotFound(err) {
-		return errors.NewNotFound("service", name)
-	}
 	if err != nil {
-		return err
+		return etcderr.InterpretDeleteError(err, "service", name)
 	}
+
+	// TODO: can leave dangling endpoints, and potentially return incorrect
+	// endpoints if a new service is created with the same name
 	key = makeServiceEndpointsKey(name)
-	err = r.Delete(key, true)
-	if !tools.IsEtcdNotFound(err) {
-		return err
+	if err := r.Delete(key, true); err != nil && !tools.IsEtcdNotFound(err) {
+		return etcderr.InterpretDeleteError(err, "endpoints", name)
 	}
 	return nil
 }
 
 // UpdateService replaces an existing Service.
 func (r *Registry) UpdateService(svc *api.Service) error {
-	return r.SetObj(makeServiceKey(svc.ID), svc)
+	err := r.SetObj(makeServiceKey(svc.ID), svc)
+	return etcderr.InterpretUpdateError(err, "service", svc.ID)
 }
 
 // WatchServices begins watching for new, changed, or deleted service configurations.
@@ -380,11 +358,12 @@ func (r *Registry) ListEndpoints() (*api.EndpointsList, error) {
 // UpdateEndpoints update Endpoints of a Service.
 func (r *Registry) UpdateEndpoints(e *api.Endpoints) error {
 	// TODO: this is a really bad misuse of AtomicUpdate, need to compute a diff inside the loop.
-	return r.AtomicUpdate(makeServiceEndpointsKey(e.ID), &api.Endpoints{},
+	err := r.AtomicUpdate(makeServiceEndpointsKey(e.ID), &api.Endpoints{},
 		func(input runtime.Object) (runtime.Object, error) {
 			// TODO: racy - label query is returning different results for two simultaneous updaters
 			return e, nil
 		})
+	return etcderr.InterpretUpdateError(err, "endpoints", e.ID)
 }
 
 // WatchEndpoints begins watching for new, changed, or deleted endpoint configurations.

diff --git a/pkg/registry/etcd/etcd_test.go b/pkg/registry/etcd/etcd_test.go
@@ -63,8 +63,8 @@ func TestEtcdGetPodNotFound(t *testing.T) {
 	}
 	registry := NewTestEtcdRegistry(fakeClient)
 	_, err := registry.GetPod("foo")
-	if err == nil {
-		t.Errorf("Unexpected non-error.")
+	if !errors.IsNotFound(err) {
+		t.Errorf("Unexpected error returned: %#v", err)
 	}
 }
 
@@ -144,8 +144,8 @@ func TestEtcdCreatePodAlreadyExisting(t *testing.T) {
 			ID: "foo",
 		},
 	})
-	if err == nil {
-		t.Error("Unexpected non-error")
+	if !errors.IsAlreadyExists(err) {
+		t.Errorf("Unexpected error returned: %#v", err)
 	}
 }
 
@@ -162,7 +162,7 @@ func TestEtcdCreatePodWithContainersError(t *testing.T) {
 		R: &etcd.Response{
 			Node: nil,
 		},
-		E: tools.EtcdErrorValueRequired,
+		E: tools.EtcdErrorNodeExist, // validate that ApplyBinding is translating Create errors
 	}
 	registry := NewTestEtcdRegistry(fakeClient)
 	err := registry.CreatePod(&api.Pod{
@@ -176,16 +176,16 @@ func TestEtcdCreatePodWithContainersError(t *testing.T) {
 
 	// Suddenly, a wild scheduler appears:
 	err = registry.ApplyBinding(&api.Binding{PodID: "foo", Host: "machine"})
-	if err == nil {
-		t.Fatalf("Unexpected non error.")
+	if !errors.IsAlreadyExists(err) {
+		t.Fatalf("Unexpected error returned: %#v", err)
 	}
 
 	existingPod, err := registry.GetPod("foo")
 	if err != nil {
 		t.Fatalf("Unexpected error: %v", err)
 	}
 	if existingPod.DesiredState.Host == "machine" {
-		t.Fatal("Pod's host changed in response to an unappliable binding.")
+		t.Fatal("Pod's host changed in response to an non-apply-able binding.")
 	}
 }
 
@@ -568,8 +568,8 @@ func TestEtcdGetControllerNotFound(t *testing.T) {
 	if ctrl != nil {
 		t.Errorf("Unexpected non-nil controller: %#v", ctrl)
 	}
-	if err == nil {
-		t.Error("Unexpected non-error.")
+	if !errors.IsNotFound(err) {
+		t.Errorf("Unexpected error returned: %#v", err)
 	}
 }
 
@@ -745,8 +745,8 @@ func TestEtcdGetServiceNotFound(t *testing.T) {
 	}
 	registry := NewTestEtcdRegistry(fakeClient)
 	_, err := registry.GetService("foo")
-	if err == nil {
-		t.Errorf("Unexpected non-error.")
+	if !errors.IsNotFound(err) {
+		t.Errorf("Unexpected error returned: %#v", err)
 	}
 }